Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frontend: Use multiple queries in useKubeObjectList and add support for allowedNamespaces #2486

Merged
merged 3 commits into from
Nov 1, 2024

Conversation

sniok
Copy link
Contributor

@sniok sniok commented Oct 29, 2024

Create a kubeObjectListQuery, for a singular request
useKubeObjectList can handle multiple clusters and namespaces by using multiple queries

_useKubeObjectList and _useKubeObjectLists are no longer neccessary since useKubeObjectList can handle 1 and more lists

Manual testing done:

  • No allowedNamespaces, loads /pods
  • two working allowedNamespaces
  • one working and one non existing allowedNamespaces
  • token auth with access to only one namespace, no allowedNamespaces (fails as expected)
  • token auth with access to only one namespace, one allowedNamespaces (works)
  • token auth with access to only one namespace, one working and one non existing (works, shows error)
  • multi cluster, one cluster with allowedNamespaces[a,b], second cluster with token auth with allowedNamespaces[c] (works)

@sniok sniok force-pushed the use-lists-queries branch 5 times, most recently from bb75a31 to 732ba00 Compare October 29, 2024 17:28
@joaquimrocha
Copy link
Collaborator

Tested and works fine:

  • Check cluster regularly, no noticeable differences
  • Check cluster with allowed namespaces: only the allowed namespaces are being shown
  • Check cluster with the namespaces filtered: only the filtered namespaces show up in the table results

@sniok sniok force-pushed the use-lists-queries branch 3 times, most recently from 88ffb44 to a816a4e Compare October 30, 2024 11:03
@sniok
Copy link
Contributor Author

sniok commented Oct 30, 2024

Extracted some logic to a function and added unit tests for it

@sniok sniok marked this pull request as ready for review October 30, 2024 12:11
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 30, 2024
frontend/src/lib/k8s/api/v2/webSocket.ts Outdated Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/webSocket.ts Outdated Show resolved Hide resolved
frontend/src/components/cluster/Overview.tsx Outdated Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/webSocket.ts Show resolved Hide resolved
@sniok sniok force-pushed the use-lists-queries branch 2 times, most recently from 1f096f4 to 56cea3d Compare October 30, 2024 14:10
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 30, 2024
@illume
Copy link
Collaborator

illume commented Oct 30, 2024

@vyncent-t @skoeva and I are giving this a test...

Copy link
Contributor

@vyncent-t vyncent-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed the first three commits and left some comments

frontend/src/lib/k8s/api/v2/makeUrl.ts Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/hooks.ts Outdated Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/webSocket.ts Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/webSocket.ts Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/webSocket.ts Outdated Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/webSocket.ts Show resolved Hide resolved
Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
@sniok
Copy link
Contributor Author

sniok commented Oct 30, 2024

I've found a bug here, working on a fix

@sniok sniok force-pushed the use-lists-queries branch from 56cea3d to b659dcf Compare October 30, 2024 17:15
…nections at once

Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
@sniok sniok force-pushed the use-lists-queries branch from b659dcf to c60fafb Compare October 30, 2024 17:19
@sniok
Copy link
Contributor Author

sniok commented Oct 30, 2024

It was recreating connections way too often. Now it's working as expected

Tested by updating a deployment and making sure /deployment and /pods websockets are not recreated

@illume illume added bug Something isn't working frontend Issues related to the frontend multi Multi cluster aggregated view labels Oct 31, 2024
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Left a few notes about the third commit.

Additionally, the third commit message could use some more detail.

frontend/src/lib/k8s/api/v2/webSocket.ts Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/useKubeObjectList.ts Outdated Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/useKubeObjectList.ts Outdated Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/useKubeObjectList.ts Outdated Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/useKubeObjectList.ts Outdated Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/hooks.ts Show resolved Hide resolved
frontend/src/lib/k8s/api/v2/hooks.ts Show resolved Hide resolved
@dosubot dosubot bot removed the lgtm This PR has been approved by a maintainer label Oct 31, 2024
@joaquimrocha
Copy link
Collaborator

Tested the allowedNamespaces again and it's still working.

Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
@sniok sniok force-pushed the use-lists-queries branch from c60fafb to 6b4d7b8 Compare October 31, 2024 11:13
@illume illume added the regression Bugs for things that used to work in previous releases. label Oct 31, 2024
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 1, 2024
@illume illume merged commit 2596766 into main Nov 1, 2024
18 checks passed
@illume illume deleted the use-lists-queries branch November 1, 2024 08:25
@illume illume mentioned this pull request Nov 1, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working frontend Issues related to the frontend lgtm This PR has been approved by a maintainer multi Multi cluster aggregated view regression Bugs for things that used to work in previous releases. size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants