Skip to content

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

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Feb 17, 2023
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 45 additions & 16 deletions test/extended/authorization/rbac/groups_default_rules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (

"github.com/openshift/api/authorization"
"github.com/openshift/api/build"
configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/api/console"
"github.com/openshift/api/image"
"github.com/openshift/api/oauth"
Expand Down Expand Up @@ -118,19 +119,10 @@ var (
rbacv1helpers.NewRule("get", "list").Groups(authzGroup, legacyAuthzGroup).Resources("clusterroles").RuleOrDie(),
rbacv1helpers.NewRule(read...).Groups(rbacGroup).Resources("clusterroles").RuleOrDie(),
rbacv1helpers.NewRule("get", "list").Groups(storageGroup).Resources("storageclasses").RuleOrDie(),
rbacv1helpers.NewRule("get", "list", "watch").Groups(snapshotGroup).Resources("volumesnapshotclasses").RuleOrDie(),
rbacv1helpers.NewRule("list", "watch").Groups(projectGroup, legacyProjectGroup).Resources("projects").RuleOrDie(),

rbacv1helpers.NewRule("use").Groups(security.GroupName).Resources("securitycontextconstraints").Names("restricted-v2").RuleOrDie(),

// These custom resources are used to extend console functionality
// The console team is working on eliminating this exception in the near future
rbacv1helpers.NewRule(read...).Groups(consoleGroup).Resources("consoleclidownloads", "consolelinks", "consoleexternalloglinks", "consolenotifications", "consoleyamlsamples", "consolequickstarts", "consoleplugins").RuleOrDie(),

// HelmChartRepository instances keep Helm chart repository configuration
// By default users are able to browse charts from all configured repositories through console UI
rbacv1helpers.NewRule("get", "list").Groups("helm.openshift.io").Resources("helmchartrepositories").RuleOrDie(),

// TODO: remove when openshift-apiserver has removed these
rbacv1helpers.NewRule("get").URLs(
"/healthz/",
Expand Down Expand Up @@ -183,15 +175,52 @@ var _ = g.Describe("[sig-auth][Feature:OpenShiftAuthorization] The default clust
oc := exutil.NewCLI("default-rbac-policy")

g.It("should have correct RBAC rules", func() {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
defer cancel()

enabledCapabilities := []configv1.ClusterVersionCapability{}

exist, err := exutil.DoesApiResourceExist(oc.AdminConfig(), "clusterversions", "config.openshift.io")
Copy link
Copy Markdown
Contributor

@bparees bparees Jan 25, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@wking wking Jan 25, 2023

Choose a reason for hiding this comment

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

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"?

Copy link
Copy Markdown
Contributor

@bparees bparees Jan 25, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@dhellmann dhellmann Jan 26, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@dhellmann dhellmann Jan 26, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@bparees bparees Jan 27, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@pacevedom pacevedom Jan 27, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@ingvagabund ingvagabund Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@bparees bparees Feb 14, 2023

Choose a reason for hiding this comment

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

@wking are you able to move this forward?

Copy link
Copy Markdown
Member Author

@wking wking Feb 14, 2023

Choose a reason for hiding this comment

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

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.

o.Expect(err).NotTo(o.HaveOccurred())
if exist {
clusterVersion, err := oc.AdminConfigClient().ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{})
if err != nil {
e2e.Failf("Failed to get cluster version: %v", err)
}
enabledCapabilities = append(enabledCapabilities, clusterVersion.Status.Capabilities.EnabledCapabilities...)
} else {
e2e.Fail("Cluster version API resource does not exist")
}

// Conditional, capability-specific rules
for _, capability := range enabledCapabilities {
switch capability {
case configv1.ClusterVersionCapabilityConsole:
allAuthenticatedRules = append(
allAuthenticatedRules,
[]rbacv1.PolicyRule{
// These custom resources are used to extend console functionality
// The console team may eventually eliminate this exception
rbacv1helpers.NewRule(read...).Groups(consoleGroup).Resources("consoleclidownloads", "consolelinks", "consoleexternalloglinks", "consolenotifications", "consoleyamlsamples", "consolequickstarts", "consoleplugins").RuleOrDie(),

// HelmChartRepository instances keep Helm chart repository configuration
// By default users are able to browse charts from all configured repositories through console UI
rbacv1helpers.NewRule("get", "list").Groups("helm.openshift.io").Resources("helmchartrepositories").RuleOrDie(),
}...,
)

case configv1.ClusterVersionCapabilityCSISnapshot:
allAuthenticatedRules = append(
allAuthenticatedRules,
rbacv1helpers.NewRule("get", "list", "watch").Groups(snapshotGroup).Resources("volumesnapshotclasses").RuleOrDie(),
)
}
}

kubeInformers := informers.NewSharedInformerFactory(oc.AdminKubeClient(), 20*time.Minute)
ruleResolver := exutil.NewRuleResolver(kubeInformers.Rbac().V1()) // signal what informers we want to use early

stopCh := make(chan struct{})
defer func() { close(stopCh) }()
kubeInformers.Start(stopCh)

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
kubeInformers.Start(ctx.Done())

if ok := cache.WaitForCacheSync(ctx.Done(),
kubeInformers.Rbac().V1().ClusterRoles().Informer().HasSynced,
Expand All @@ -202,7 +231,7 @@ var _ = g.Describe("[sig-auth][Feature:OpenShiftAuthorization] The default clust
exutil.FatalErr("failed to sync RBAC cache")
}

namespaces, err := oc.AdminKubeClient().CoreV1().Namespaces().List(context.Background(), metav1.ListOptions{})
namespaces, err := oc.AdminKubeClient().CoreV1().Namespaces().List(ctx, metav1.ListOptions{})
if err != nil {
exutil.FatalErr(err)
}
Expand Down