Teleterm: fix incorrect cluster URI when fetching kube namespaces on a leaf cluster (access request)

[kimlisa]

resolves #48510

• teleterm: fixes fetching namespaces with incorrect cluster URI when on leaf cluster. we already had the correct URI info from the kube_cluster's originalItem, so i just used that
• teleterm: fixes re-ordering of selected namespaces when done selecting. this is only half fixed. it's fixed for teleterm but not for web UI. teleterm uses a set, while web UI still uses a map (which doesn't preserve order). web UI really needs an entire data refactor, but i didn't deem this little quirk to be bad enough to spend time on it

changelog: Fix incorrect cluster name when querying for Kubernetes namespaces on a leaf cluster for Connect UI.

[kimlisa]

@ravicious also gave feedback on removing the nextKey, but i didn't think it hurt to be there, b/c what if we did want to provide pagination in the future? i can remove it if it still bothers you though

eg: b42e163

[gzdunek]

So we need kubeClusterUri to validate if all namespaceUris come from the same kube cluster?
It feels to me too complicated, perhaps we shouldn't store namespace uris but just their names.
Since a namespace is always kept in a specific kube cluster, we don't need its whole uri.
So it would be:

async updateNamespacesForKubeCluster(
    kubeUri: KubeUri
    namespaces: string[],
)

[ravicious]

I think it's fine. While it's true that in this specific context it might seem overboard, in the broader view it lets us treat each namespace as a unique item. In that sense, it simplifies things. If we stored sole namespace names, we'd have to always remember about converting them to URIs when taking them out of AccessRequestsService.

I'm not 100% sure about this, but it feels like long-term it's a better choice.

[gzdunek]

If we stored sole namespace names, we'd have to always remember about converting them to URIs when taking them out of AccessRequestsService.

Right, converting to URI could be inconvenient. Well, I don't have a strong opinion on this, so let's keep it as is.

[ravicious]

Web UI still has this weird re-ordering because web UI still stores resource ids in a map which doesn't preserve order

Do you mean the JavaScript Map? It should preserve the insertion order. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

[kimlisa]

no it's just a js object, now that you mention it... maybe it will be easy to just replace it with a js map (but not in this PR)

[ravicious]

I think it's fine. While it's true that in this specific context it might seem overboard, in the broader view it lets us treat each namespace as a unique item. In that sense, it simplifies things. If we stored sole namespace names, we'd have to always remember about converting them to URIs when taking them out of AccessRequestsService.

I'm not 100% sure about this, but it feels like long-term it's a better choice.

[ravicious]

@ravicious also gave feedback on removing the nextKey, but i didn't think it hurt to be there, b/c what if we did want to provide pagination in the future? i can remove it if it still bothers you though

I think we should just remove it to signify that the RPC does not offer pagination. If we ever need to add it back, we can create ListKubernetesResourcesV2. I wouldn't want someone to look at the response message and think that the RPC supports pagination.

[ravicious]

await on act is not necessary, unless the callback passed to act returns a promise. This is the case for a lot of await act in this file. TypeScript's lang server should highlight all of them if you have it enabled in your editor. There's also the @typescript-eslint/await-thenable rule, but enabling it requires adjusting the ESLint config a little bit.

[kimlisa]

hrm, i actually do need them in almost all cases, b/c most are react state setters which are async? (the test fails without them)

[ravicious]

React state setters are technically sync and they don't need to be awaited. TS lang server highlights those callsites because act is given a sync function, so there's nothing to await.

The errors after removing await stem from the fact that useAccessRequestCheckout has an effect which fetches resource roles every time you change pendingAccessRequestRequest. This means each call to updateNamespacesForKubeCluster causes this effect to be run again.

At first I removed those await calls and added waitFor for fetchResourceRolesAttempt. But then I looked up the docs for act and the React team actually recommends passing an async function to act. So I left the awaits, but I made sure that we always pass an async function there. This way the TS language server doesn't complain about those callsites.

[ravicious]

I made a PR with those changes: #48969

[ravicious]

I tested this against #48510 and the repro for selection order from #47347 (comment) and it works for both.

[ravicious]

@gzdunek In the test for useAccessRequestCheckout I noticed that we await a few methods from AccessRequestsService. I looked at them and it's because they do this:

    if (!(await this.canUpdateRequest('resource'))) {
      return;
    }

I think making those methods async might have been a mistake. This layer should be kept as simple as possible and canUpdateRequest could be taken care of one layer above, on a layer that has access to both AccessRequestsService and ModalsService. Like in some kind of a hook or a context.

This would reduce coupling between services and avoid async methods in a class that's largely responsible for just managing some state.

I know we use async methods in WorkspacesService already, but that class is in a bit of a mess itself. ClustersService's usage of async methods feels better because it's all about getting data and storing data. The way WorkspacesService and AccessRequestsService use async methods mixes together the data layer and the UI layer.

[gzdunek]

I think my reasoning was to have a centralized place where changing request type is handled. But yeah, it would be better to do it on a different level, and AccessRequestsService should just overwrite the existing state with the new request (e.g. when trying to add role to a resource request etc).
I will keep that mind.

[public-teleport-github-review-bot]

@kimlisa See the table below for backport results.

Branch	Result
branch/v16	Failed
branch/v17	Create PR