Add behavioural tests for Cluster Menu K8s Resources in Sidebar menu not being shown #7280

Nokel81 · 2023-03-03T22:10:28Z

This PR fixes an oversight in #6657. Namely, I previously assumed that all the applicable resources rules would be merged together on the server side before sending a response back for a given SelfSubjectRulesReview. That, however, is not the case and instead it seems that most kubernetes distros (or at least K0s) just find all the applicable rules and then send them all back in a list.

To reproduce this yourself:

Create an LDK
Share it with a space
Pause the updater and remove the rules targeting lens space Members and Admins
Add two new ClusterRoles's, one called namespace-get which allows the get verb on namespaces and one called namespace-list which allows the list verb on namespaces
Add two new ClusterRoleBindings, one for each of the above, targeting your own user.
Connect to the shared version of the LDK and run the following command in the terminal.
- curl -H "Content-Type: application/json" -X POST 127.0.0.1:8001/apis/authorization.k8s.io/v1/selfsubjectrulesreviews --data '{ "apiVersion": "authorization.k8s.io/v1", "kind": "SelfSubjectRulesReview", "spec": { "namespace": "default" }}'
See something like the following:

status:
  resourceRules:
    - apiGroups:
        - ''
      resourceNames: null
      resources:
        - 'namespaces'
      verbs:
        - 'list'
    - apiGroups:
        - 'policy'
      resourceNames:
        - '00-k0s-privileged'
      resources:
        - 'podsecuritypolicies'
      verbs:
         - 'use'
    - apiGroups:
        - 'authorization.k8s.io'
      resourceNames: null
      resources:
        - 'selfsubjectaccessreviews'
        - 'selfsubjectrulesreviews'
      verbs:
        - 'create'
    - apiGroups:
        - ''
      resourceNames: null
      resources:
        - 'namespaces'
      verbs:
        - 'get'

packages/core/src/common/cluster/request-namespace-list-permissions.injectable.ts

jakolehm

IMHO this PR touches too many places for a hotfix (patch release).

jansav · 2023-03-06T06:48:56Z

packages/core/src/renderer/components/test-utils/get-application-builder.tsx

+  const {
+    fakeTime = true,
+  } = options;
+


I don't think that there's case where we want to support behavioural unit tests without fake time.

github-actions · 2023-03-06T08:49:04Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2023-03-06T16:58:50Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2023-03-07T17:48:24Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2023-03-07T20:27:31Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2023-03-10T07:38:56Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2023-03-10T16:22:28Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2023-03-13T13:37:51Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: Sebastian Malton <sebastian@malton.name>

github-actions · 2023-03-13T14:09:01Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

jim-docker

This is all refactoring? (I don't see any tests added)

I guess my comments are for the previous review, and not really an issue if "CreateCanI" is called more than once, so "CanI" can change, if some auth/api error gets corrected.

jim-docker · 2023-03-21T21:33:32Z

packages/core/src/common/cluster/create-can-i.injectable.ts

+      } catch (error) {
+        logger.error(`[AUTHORIZATION-REVIEW]: failed to create access review: ${error}`, { resourceAttributes });
+
+        return false;
+      }


If there's an error do we know for sure the answer is false? Seems strange that the caller doesn't know there was an error.

No we don't, but since these are currently only used for optimizations it is fine if they are turned off in case of error.

Plus this has always been the case.

jim-docker · 2023-03-21T21:48:44Z

packages/core/src/common/cluster/create-request-namespace-list-permissions.injectable.ts

+          logger.warn(`[AUTHORIZATION-NAMESPACE-REVIEW]: allowing all resources in namespace="${namespace}" due to incomplete SelfSubjectRulesReview: ${status?.evaluationError}`);
+
+          return () => true;


The caller has no idea here, or in the catch below, that api.createSelfSubjectRulesReview() failed

It is safer to show all the resources of this fails then to show none.

Sure, I'm really questioning whether that decision should be made here or by the caller, which might be able to show a helpful notification to the end-user. Now there is no indication to the user why there are suddenly all these resources listed in the sidebar that perhaps can't be listed.

Nokel81 · 2023-03-22T01:58:12Z

packages/core/src/features/cluster/refresh-accessibility-technical.test.ts was added

The diff is large so it is hidden by default.

Nokel81 added bug Something isn't working area/rbac Something related to Kube RBAC permissions labels Mar 3, 2023

Nokel81 added this to the 6.4.2 milestone Mar 3, 2023

Nokel81 requested a review from a team as a code owner March 3, 2023 22:10

Nokel81 requested review from jansav and ixrock and removed request for a team March 3, 2023 22:10

jakolehm reviewed Mar 4, 2023

View reviewed changes

packages/core/src/common/cluster/request-namespace-list-permissions.injectable.ts Outdated Show resolved Hide resolved

Nokel81 linked an issue Mar 5, 2023 that may be closed by this pull request

[Windows]After updating the latest version,i cannot see all the pods #7270

Closed

jakolehm reviewed Mar 6, 2023

View reviewed changes

jakolehm mentioned this pull request Mar 6, 2023

Fix request namespace list permissions check if a list rule is not the first match #7282

Merged

jansav reviewed Mar 6, 2023

View reviewed changes

github-actions bot added the PR: needs rebase label Mar 6, 2023

jweak modified the milestones: 6.4.2, 6.4.3 Mar 6, 2023

msa0311 changed the title ~~Fix Pods sidebar menu not being shown~~ Fix Cluster Menu K8s Resources in Sidebar menu not being shown Mar 6, 2023

msa0311 removed a link to an issue Mar 6, 2023

[Windows]After updating the latest version,i cannot see all the pods #7270

Closed

Nokel81 changed the base branch from release/v6.4 to master March 6, 2023 15:18

Nokel81 force-pushed the attempt-2-fix-pods-missing-sidebar branch from 7e0556b to 931048e Compare March 6, 2023 16:58

github-actions bot removed the PR: needs rebase label Mar 6, 2023

jim-docker modified the milestones: 6.4.3, 6.4.4 Mar 6, 2023

Nokel81 modified the milestones: 6.4.4, 6.5.0 Mar 7, 2023

github-actions bot added the PR: needs rebase label Mar 7, 2023

Nokel81 force-pushed the attempt-2-fix-pods-missing-sidebar branch from 931048e to c7ef585 Compare March 7, 2023 20:27

github-actions bot removed the PR: needs rebase label Mar 7, 2023

Nokel81 force-pushed the attempt-2-fix-pods-missing-sidebar branch from c7ef585 to b3773c8 Compare March 7, 2023 20:42

github-actions bot added the PR: needs rebase label Mar 10, 2023

Nokel81 force-pushed the attempt-2-fix-pods-missing-sidebar branch from b3773c8 to c5e1778 Compare March 10, 2023 16:22

github-actions bot removed the PR: needs rebase label Mar 10, 2023

Nokel81 changed the title ~~Fix Cluster Menu K8s Resources in Sidebar menu not being shown~~ Add behavioural tests for Cluster Menu K8s Resources in Sidebar menu not being shown Mar 10, 2023

github-actions bot added the PR: needs rebase label Mar 13, 2023

Nokel81 added 10 commits March 13, 2023 10:08

Add behavioural tests to cover bug fix

786fa39

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Remove previous fix to fix last test

8cf4252

Signed-off-by: Sebastian Malton <sebastian@malton.name>

More consistent impl of flushPromises

008b990

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Fixup tests

7407c24

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Remove ContextHandler test (dead code)

604a143

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Fix PrometheusHandler describe text

a6ae8f1

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Fix type errors

5632f43

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Add useful case test-utils helper

69e46d0

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Rename file to match token

a99e922

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Cleanup tests to fix type errors and use tables

7c48248

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 force-pushed the attempt-2-fix-pods-missing-sidebar branch from 93abc4c to 7c48248 Compare March 13, 2023 14:08

github-actions bot removed the PR: needs rebase label Mar 13, 2023

jim-docker approved these changes Mar 21, 2023

View reviewed changes

Nokel81 merged commit 8a80607 into master Mar 22, 2023

Nokel81 deleted the attempt-2-fix-pods-missing-sidebar branch March 22, 2023 02:00

Nokel81 mentioned this pull request Apr 12, 2023

Release 6.5.0-alpha.4 #7535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add behavioural tests for Cluster Menu K8s Resources in Sidebar menu not being shown #7280

Add behavioural tests for Cluster Menu K8s Resources in Sidebar menu not being shown #7280

Nokel81 commented Mar 3, 2023 •

edited

Loading

jakolehm left a comment

jansav Mar 6, 2023

github-actions bot commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

jim-docker left a comment

jim-docker Mar 21, 2023

Nokel81 Mar 22, 2023

jim-docker Mar 21, 2023

Nokel81 Mar 22, 2023

jim-docker Mar 22, 2023

Nokel81 commented Mar 22, 2023

		logger.warn(`[AUTHORIZATION-NAMESPACE-REVIEW]: allowing all resources in namespace="${namespace}" due to incomplete SelfSubjectRulesReview: ${status?.evaluationError}`);

		return () => true;

Add behavioural tests for Cluster Menu K8s Resources in Sidebar menu not being shown #7280

Add behavioural tests for Cluster Menu K8s Resources in Sidebar menu not being shown #7280

Conversation

Nokel81 commented Mar 3, 2023 • edited Loading

jakolehm left a comment

Choose a reason for hiding this comment

jansav Mar 6, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2023

github-actions bot commented Mar 6, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 13, 2023

github-actions bot commented Mar 13, 2023

jim-docker left a comment

Choose a reason for hiding this comment

jim-docker Mar 21, 2023

Choose a reason for hiding this comment

Nokel81 Mar 22, 2023

Choose a reason for hiding this comment

jim-docker Mar 21, 2023

Choose a reason for hiding this comment

Nokel81 Mar 22, 2023

Choose a reason for hiding this comment

jim-docker Mar 22, 2023

Choose a reason for hiding this comment

Nokel81 commented Mar 22, 2023

Nokel81 commented Mar 3, 2023 •

edited

Loading