test/extended/authorization/rbac: Condition console RBAC on 'Console' capability #27681

wking · 2023-01-23T23:25:15Z

Clusters that disable the Console capability are currently failing this test-case:

  : [sig-auth][Feature:OpenShiftAuthorization] The default cluster RBAC policy should have correct RBAC rules [Suite:openshift/conformance/parallel] expand_less
  Run #0: Failed expand_less	3s
  {  fail [github.com/openshift/origin/test/extended/authorization/rbac/groups_default_rules.go:229]: Jan  3 13:43:14.134: test data for system:authenticated has too many unnecessary permissions:
  {APIGroups:["console.openshift.io"], Resources:["consoleclidownloads"], Verbs:["get" "list" "watch"]}
  {APIGroups:["console.openshift.io"], Resources:["consoleexternalloglinks"], Verbs:["get" "list" "watch"]}
  {APIGroups:["console.openshift.io"], Resources:["consolelinks"], Verbs:["get" "list" "watch"]}
  {APIGroups:["console.openshift.io"], Resources:["consolenotifications"], Verbs:["get" "list" "watch"]}
  {APIGroups:["console.openshift.io"], Resources:["consoleplugins"], Verbs:["get" "list" "watch"]}
  {APIGroups:["console.openshift.io"], Resources:["consolequickstarts"], Verbs:["get" "list" "watch"]}
  {APIGroups:["console.openshift.io"], Resources:["consoleyamlsamples"], Verbs:["get" "list" "watch"]}
  {APIGroups:["helm.openshift.io"], Resources:["helmchartrepositories"], Verbs:["get" "list"]}
  {APIGroups:["snapshot.storage.k8s.io"], Resources:["volumesnapshotclasses"], Verbs:["get" "list" "watch"]}
  Ginkgo exit error 1: exit with code 1}

This commit uses the pattern that 6d1143a (#26998) began using for CRDs to only add the console-linked rules when Console is enabled, and to only add the snapshot-linked rules when CSISnapshot is enabled.

wking · 2023-01-23T23:29:59Z

Launching a payload run to see whether this fix works or needs further tweaks:

/payload-job periodic-ci-openshift-release-master-ci-4.13-e2e-aws-sdn-no-capabilities

/hold

openshift-ci · 2023-01-23T23:30:02Z

@wking: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.13-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/da538920-9b75-11ed-90d2-600418f4ba34-0

bparees · 2023-01-23T23:30:27Z

/approve

let's see how the test job does

wking · 2023-01-25T00:10:10Z

/payload-job periodic-ci-openshift-release-master-ci-4.13-e2e-aws-sdn-no-capabilities

openshift-ci · 2023-01-25T00:10:14Z

@wking: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-ci-4.13-e2e-aws-sdn-no-capabilities

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a1aa86d0-9c44-11ed-831c-3b8f6065f0b5-0

wking · 2023-01-25T04:41:19Z

Payload job:

$ curl -s https://storage.googleapis.com/origin-ci-test/logs/openshift-origin-27681-ci-4.13-e2e-aws-sdn-no-capabilities/1618038496394481664/build-log.txt | grep 'The default cluster RBAC policy should have correct RBAC rules'
started: 0/127/413 "[sig-auth][Feature:OpenShiftAuthorization] The default cluster RBAC policy should have correct RBAC rules [Suite:openshift/conformance/parallel]"
passed: (4.7s) 2023-01-25T01:07:26 "[sig-auth][Feature:OpenShiftAuthorization] The default cluster RBAC policy should have correct RBAC rules [Suite:openshift/conformance/parallel]"

/hold cancel

bparees · 2023-01-25T14:09:23Z

test/extended/authorization/rbac/groups_default_rules.go

+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+		defer cancel()
+
+		exist, err := exutil.DoesApiResourceExist(oc.AdminConfig(), "clusterversions", "config.openshift.io")


do we ever expect this not to exist? when?

and if so, i'm inclined to think we'd want to check all the rbac rules (because if clusterversions doesn't exist somehow, then presumably all caps should be enabled), rather than what the current logic does which is to ignore the rbac for optionalcaps when the clusterversion resource type does not exist.

This portion of my code is pattern-matching from here, where it looks like the goal is supporting MicroShift (#27540). I'm not sure how reliable the "no ClusterVersion" -> "so it's MicroShift" -> "so we want all these conditional RBAC rules" chain is, so maybe we want to pivot to "if the resource the RBAC relates to is present, expect the RBAC to be present"?

hrrrrrm. yeah, the lack of a clusterversion resource, which strongly implies microshift, would imply potentially a different subset of rbac rules to be expected. (e.g. microshift might have some "optional capabilities" always enabled, and some other ones always disabled, so just saying "if it's microshift, assume all optional caps are disabled" doesn't seem quite right to me).

the fact that the microshift team hadn't previously found the need to disable these checks for their CI would seem to further confirm that these rbac rules do exist on microshift clusters (though admittedly that seems slightly surprising).

@dhellmann can you weigh in on what rbac rules would/would not be expected on a microshift cluster, compared to a traditional OCP cluster with all capabilities enabled?

There's a ConfigMap that can be used to detect a MicroShift cluster, so we don't need to make any assumptions. https://github.com/openshift/microshift/blob/main/docs/debugging_tips.md#checking-the-microshift-version

I'm having a little trouble making sense of what the test is trying to check. I see the code below is looking at rbacv1.PolicyRule. Is that an API type? I do not see that present in MicroShift. Is that being translated into some other type somehow?

$ oc api-resources | grep -i rbac clusterrolebindings rbac.authorization.k8s.io/v1 false ClusterRoleBinding clusterroles rbac.authorization.k8s.io/v1 false ClusterRole rolebindings rbac.authorization.k8s.io/v1 true RoleBinding roles rbac.authorization.k8s.io/v1 true Role

@ingvagabund or @pacevedom can you help here? Is this test even relevant to MicroShift?

Since we need to rework this one I would agree with these changes.

you agree with the changes as they currently exist in the PR, or you agree w/ the changes i proposed?

Sorry, I meant the ones you proposed. There is no need to check if clusterversion is there when the test is already broken by over 20 other resources permissions.
Even if the check was kept, the test would still need to be worked on to fit it into MicroShift.

From the point of enabling running all e2e tests over MicroShift this is "just" another instance where we need to improve the logic. We will need to re-iterate on all affected tests again. So +1 for what @bparees suggests to unblock the PR.

@wking are you able to move this forward?

I've pushed 61fdce8 -> f655a70, with an uneventful rebase onto the current dev tip, and a change to make !exist a hard-fail. Once the MicroShift folks have a plan for how they would like to handle it, they can come back and make those changes in follow-up work.

… capability Clusters that disable the 'Console' capability are currently failing this test-case [1]: : [sig-auth][Feature:OpenShiftAuthorization] The default cluster RBAC policy should have correct RBAC rules [Suite:openshift/conformance/parallel] expand_less Run #0: Failed expand_less 3s { fail [github.com/openshift/origin/test/extended/authorization/rbac/groups_default_rules.go:229]: Jan 3 13:43:14.134: test data for system:authenticated has too many unnecessary permissions: {APIGroups:["console.openshift.io"], Resources:["consoleclidownloads"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consoleexternalloglinks"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consolelinks"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consolenotifications"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consoleplugins"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consolequickstarts"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consoleyamlsamples"], Verbs:["get" "list" "watch"]} {APIGroups:["helm.openshift.io"], Resources:["helmchartrepositories"], Verbs:["get" "list"]} {APIGroups:["snapshot.storage.k8s.io"], Resources:["volumesnapshotclasses"], Verbs:["get" "list" "watch"]} Ginkgo exit error 1: exit with code 1} This commit uses the pattern that 6d1143a (cli: remove metal3 CRDs when capabilities are none, 2022-04-08, openshift#26998) began using for CRDs to only add the console-linked rules when Console is enabled, and to only add the snapshot-linked rules when `CSISnapshot` is enabled. MicroShift won't have a ClusterVersion custom resource definition, but the test is already failing there [2], so this pivot doesn't break them any worse. Once they have a plan for how they would like to handle it, they can come back and make those changes in follow-up work. It has also been around three years since bb09b26 (Add system:authenticated exception for CRDs used by console for extensions, 2019-06-21, openshift#23231)'s "eliminating this exception in the near future", so I'm softening that to "may eventually". Extending system:authenticated is still not a great pattern to follow, but it may never be worth the time it would take the console team to build an alternative mechanism. [1]: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/periodic-ci-openshift-release-master-ci-4.13-e2e-aws-sdn-no-capabilities/1610257913278894080 [2]: openshift#27681 (comment)

bparees · 2023-02-14T20:32:35Z

/lgtm

openshift-ci · 2023-02-14T20:34:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/extended/authorization/rbac/OWNERS~~ [bparees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-02-14T22:59:26Z

/retest-required

Remaining retests: 0 against base HEAD 9812f2e and 2 for PR HEAD f655a70 in total

openshift-ci-robot · 2023-02-15T11:29:41Z

/retest-required

Remaining retests: 0 against base HEAD 7f660fc and 1 for PR HEAD f655a70 in total

openshift-ci-robot · 2023-02-15T18:44:04Z

/retest-required

Remaining retests: 0 against base HEAD 7c04676 and 0 for PR HEAD f655a70 in total

openshift-ci-robot · 2023-02-15T23:30:24Z

/hold

Revision f655a70 was retested 3 times: holding

bparees · 2023-02-16T14:39:04Z

/hold cancel
/retest

openshift-ci-robot · 2023-02-16T20:39:39Z

/retest-required

Remaining retests: 0 against base HEAD 55ca147 and 2 for PR HEAD f655a70 in total

openshift-ci · 2023-02-16T22:29:44Z

@wking: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-azure-ovn-etcd-scaling	`f655a70`	link	false	`/test e2e-azure-ovn-etcd-scaling`
ci/prow/e2e-aws-ovn-single-node-upgrade	`f655a70`	link	false	`/test e2e-aws-ovn-single-node-upgrade`
ci/prow/e2e-aws-ovn-single-node-serial	`f655a70`	link	false	`/test e2e-aws-ovn-single-node-serial`
ci/prow/e2e-aws-ovn-single-node	`f655a70`	link	false	`/test e2e-aws-ovn-single-node`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2023-02-17T18:11:49Z

/retest-required

Remaining retests: 0 against base HEAD 6834f50 and 1 for PR HEAD f655a70 in total

… capability Clusters that disable the 'Console' capability are currently failing this test-case [1]: : [sig-auth][Feature:OpenShiftAuthorization] The default cluster RBAC policy should have correct RBAC rules [Suite:openshift/conformance/parallel] expand_less Run #0: Failed expand_less 3s { fail [github.com/openshift/origin/test/extended/authorization/rbac/groups_default_rules.go:229]: Jan 3 13:43:14.134: test data for system:authenticated has too many unnecessary permissions: {APIGroups:["console.openshift.io"], Resources:["consoleclidownloads"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consoleexternalloglinks"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consolelinks"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consolenotifications"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consoleplugins"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consolequickstarts"], Verbs:["get" "list" "watch"]} {APIGroups:["console.openshift.io"], Resources:["consoleyamlsamples"], Verbs:["get" "list" "watch"]} {APIGroups:["helm.openshift.io"], Resources:["helmchartrepositories"], Verbs:["get" "list"]} {APIGroups:["snapshot.storage.k8s.io"], Resources:["volumesnapshotclasses"], Verbs:["get" "list" "watch"]} Ginkgo exit error 1: exit with code 1} This commit uses the pattern that 6d1143a (cli: remove metal3 CRDs when capabilities are none, 2022-04-08, openshift#26998) began using for CRDs to only add the console-linked rules when Console is enabled, and to only add the snapshot-linked rules when `CSISnapshot` is enabled. MicroShift won't have a ClusterVersion custom resource definition, but the test is already failing there [2], so this pivot doesn't break them any worse. Once they have a plan for how they would like to handle it, they can come back and make those changes in follow-up work. It has also been around three years since bb09b26 (Add system:authenticated exception for CRDs used by console for extensions, 2019-06-21, openshift#23231)'s "eliminating this exception in the near future", so I'm softening that to "may eventually". Extending system:authenticated is still not a great pattern to follow, but it may never be worth the time it would take the console team to build an alternative mechanism. [1]: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/periodic-ci-openshift-release-master-ci-4.13-e2e-aws-sdn-no-capabilities/1610257913278894080 [2]: openshift#27681 (comment)

openshift-ci bot requested review from ibihim and s-urbaniak January 23, 2023 23:25

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 23, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2023

wking force-pushed the console-conditional-rbac-rules branch 2 times, most recently from fa64951 to 61fdce8 Compare January 24, 2023 23:31

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2023

bparees reviewed Jan 25, 2023

View reviewed changes

wking force-pushed the console-conditional-rbac-rules branch from 61fdce8 to f655a70 Compare February 14, 2023 20:30

openshift-ci bot assigned bparees Feb 14, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2023

openshift-merge-robot merged commit 2b61865 into openshift:master Feb 17, 2023

wking deleted the console-conditional-rbac-rules branch September 10, 2025 20:52

test/extended/authorization/rbac: Condition console RBAC on 'Console' capability #27681

test/extended/authorization/rbac: Condition console RBAC on 'Console' capability #27681

Uh oh!

Conversation

wking commented Jan 23, 2023

Uh oh!

wking commented Jan 23, 2023

Uh oh!

openshift-ci bot commented Jan 23, 2023

Uh oh!

bparees commented Jan 23, 2023

Uh oh!

wking commented Jan 25, 2023

Uh oh!

openshift-ci bot commented Jan 25, 2023

Uh oh!

wking commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingvagabund Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Feb 14, 2023

Uh oh!

openshift-ci bot commented Feb 14, 2023

Uh oh!

openshift-ci-robot commented Feb 14, 2023

Uh oh!

openshift-ci-robot commented Feb 15, 2023

Uh oh!

openshift-ci-robot commented Feb 15, 2023

Uh oh!

openshift-ci-robot commented Feb 15, 2023

Uh oh!

bparees commented Feb 16, 2023

Uh oh!

openshift-ci-robot commented Feb 16, 2023

Uh oh!

openshift-ci bot commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wking commented Jan 25, 2023 •

edited

Loading

ingvagabund Jan 30, 2023 •

edited

Loading

openshift-ci bot commented Feb 16, 2023 •

edited

Loading