WIP: CNTRLPLANE-84: auth-operator manages the RoleBindingRestrictions CRD based on auth type #796
Conversation
configuration In particular, check if the RoleBindingREstrictions CRD exists or not, and also if any RoleBindingRestrictions exist or not.
openshift-ci-robot
added
the
jira/valid-reference
|
@liouk: This pull request references CNTRLPLANE-84 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
do-not-merge/work-in-progress
WalkthroughAdds OIDC precondition checks in ExternalOIDC controller using CRD lister and authorization client. Wires apiextensions informers and authz client into operator starters. Introduces conditional RoleBindingRestrictions resource management based on CRD presence and existing objects. Updates unit and e2e tests, exposing Keycloak namespace from helper to validate RBR scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liouk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/replacement_starter.go (1)
780-782: Add CRD informer to ExternalOIDC controller WithInformers
- In pkg/controllers/externaloidc/externaloidc_controller.go, extend
NewExternalOIDCControllerto accept the CRD informer and append
.Apiextensions().V1().CustomResourceDefinitions().Informer()to theWithInformers(...)call.- In pkg/operator/replacement_starter.go, propagate that CRD informer into the
prepareExternalOIDCinvocation so its cache is started before using the lister.
🧹 Nitpick comments (3)
pkg/operator/starter.go (1)
882-892: Minor: log level.Consider Warningf for unexpected errors to aid triage.
- klog.Infof("error while checking if CRD '%s' exists: %v", crdName, err) + klog.Warningf("error while checking if CRD '%s' exists: %v", crdName, err)pkg/controllers/externaloidc/externaloidc_controller_test.go (1)
589-589: Nit: test name typo.TestExternalOIDCCOntroller_deleteAuthConfig → TestExternalOIDCController_deleteAuthConfig.
test/e2e-oidc/external_oidc_test.go (1)
62-69: Consider adding cleanup for test resources.The test function deploys Keycloak but doesn't clean up the resources afterward. This may leave test artifacts in the cluster.
Apply this diff to add cleanup:
func TestDeployKeycloak(t *testing.T) { testClient, err := newTestClient(t, t.Context()) require.NoError(t, err) - kcClient, idpName, _, _ := test.AddKeycloakIDP(t, testClient.kubeConfig, true) + kcClient, idpName, _, cleanups := test.AddKeycloakIDP(t, testClient.kubeConfig, true) + t.Cleanup(test.IDPCleanupWrapper(func() { + for _, c := range cleanups { + c() + } + })) t.Logf("keycloak issuer URL: %s", kcClient.IssuerURL()) t.Logf("idpName: %s", idpName) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (59)
vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/clusterrole.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/clusterrolebinding.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/grouprestriction.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/policyrule.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/role.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/rolebinding.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/rolebindingrestriction.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/rolebindingrestrictionspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/serviceaccountreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/serviceaccountrestriction.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/authorization/v1/userrestriction.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/applyconfigurations/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/clientset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/fake/clientset_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/fake/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/scheme/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/scheme/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/authorization_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/clusterrole.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/clusterrolebinding.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_authorization_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_clusterrole.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_clusterrolebinding.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_localresourceaccessreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_localsubjectaccessreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_resourceaccessreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_role.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_rolebinding.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_rolebindingrestriction.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_selfsubjectrulesreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_subjectaccessreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/fake/fake_subjectrulesreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/generated_expansion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/localresourceaccessreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/localsubjectaccessreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/resourceaccessreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/role.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/rolebinding.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/rolebindingrestriction.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/selfsubjectrulesreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/subjectaccessreview.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/authorization/clientset/versioned/typed/authorization/v1/subjectrulesreview.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/apiextensions/interface.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/apiextensions/v1/customresourcedefinition.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/apiextensions/v1/interface.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/apiextensions/v1beta1/customresourcedefinition.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/apiextensions/v1beta1/interface.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/factory.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/generic.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/internalinterfaces/factory_interfaces.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1/customresourcedefinition.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1/expansion_generated.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1beta1/customresourcedefinition.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1beta1/expansion_generated.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
pkg/controllers/externaloidc/externaloidc_controller.go(7 hunks)pkg/controllers/externaloidc/externaloidc_controller_test.go(9 hunks)pkg/operator/replacement_starter.go(9 hunks)pkg/operator/starter.go(5 hunks)test/e2e-oidc/external_oidc_test.go(7 hunks)test/e2e/keycloak_test.go(1 hunks)test/library/keycloakidp.go(2 hunks)
🔇 Additional comments (18)
pkg/operator/replacement_starter.go (5)
21-27: New deps wiring looks correct.apiextensions informers and authz client imports are appropriate for the new logic.
62-62: Input struct extension OK.authzClient field addition is consistent with usage downstream.
105-108: Client initialization LGTM.Creating authz client from both MOM and controller context is consistent with other clients.
Also applies to: 195-198
152-153: Propagation LGTM.authzClient plumbed into authenticationOperatorInput in both constructors.
Also applies to: 234-235
250-250: apiextensions informer integration looks good.Factory, initialization, and inclusion in SimplifiedInformerFactories are correct so it will be started.
Also applies to: 277-278, 294-295
pkg/controllers/externaloidc/externaloidc_controller.go (3)
18-19: New imports OK.authz client and CRD lister imports are correct for preconditions.
Also applies to: 28-29
55-57: Controller fields extension LGTM.crdLister and authzClient are appropriate additions.
108-112: Precondition gate placement LGTM.Runs before generation/apply and degrades on error via WithSyncDegradedOnError.
pkg/controllers/externaloidc/externaloidc_controller_test.go (3)
24-28: Test wiring LGTM.Authz API types/fake client and CRD listers used correctly for new paths.
Also applies to: 35-36
502-518: CRD/RBR precondition tests LGTM.Good coverage for CRD lister error, RBR list error, and existing RBRs.
Also applies to: 538-547, 554-556
216-216: No update needed for Go version requirement. go.mod specifies Go 1.24.0 (≥1.22), which supports t.Context().test/e2e/keycloak_test.go (1)
40-40: Signature change handled correctly.Capturing the added return value with blank identifier keeps behavior intact.
test/library/keycloakidp.go (1)
33-33: LGTM! Exposing namespace for test coordination.The function signature correctly adds
nsNameas a third return value, and the return statement properly includes it. This change enables callers to create namespace-scoped resources like RoleBindingRestrictions in the Keycloak deployment namespace for test scenarios.Also applies to: 187-187
test/e2e-oidc/external_oidc_test.go (5)
17-17: LGTM! Imports for RoleBindingRestriction testing.The new imports provide the necessary types and client for RoleBindingRestriction operations in the extended test scenarios.
Also applies to: 21-21
71-106: LGTM! Test setup improvements.The changes improve test context management and cleanup handling:
- Using
t.Context()for proper context propagation- Using
t.Cleanupfor more robust cleanup- Capturing
testNSfor namespace-scoped RBR operations
225-275: LGTM! Comprehensive test for RBR precondition check.The test case thoroughly validates the new precondition check:
- Creates a RoleBindingRestriction in the test namespace
- Attempts OIDC configuration, which should be blocked
- Verifies the operator enters a degraded state with the expected reason and message
- Properly cleans up the RBR resource
The test aligns with the PR objectives to block external OIDC configuration when RBRs exist.
557-557: LGTM! Authorization client wiring.The
authzClientfield is properly added to the test client struct and initialized innewTestClient, following the established pattern for other clients. This enables RoleBindingRestriction operations in test scenarios.Also applies to: 596-599
811-811: LGTM! RBR CRD validation.The addition of the RoleBindingRestrictions CRD to the OAuth resource validation aligns with the PR objective to conditionally manage this CRD based on authentication type.
| crdLister apiextensionsv1lister.CustomResourceDefinitionLister, | ||
| rbrClient authzclient.Interface, | ||
| recorder events.Recorder, | ||
| featureGates featuregates.FeatureGate, | ||
| ) factory.Controller { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Constructor wiring LGTM, but add CRD informer to WithInformers to avoid cache-sync race.
Currently only configmaps/authentications informers are waited on. The CRD lister can return stale/empty before sync. Prefer passing a typed CRD informer into the constructor, use its Lister for c.crdLister, and add its Informer to WithInformers so factory waits for sync.
Apply this diff (constructor signature + body + WithInformers):
@@
- crdLister apiextensionsv1lister.CustomResourceDefinitionLister,
- rbrClient authzclient.Interface,
+ crdInformer apiextensionsv1informers.CustomResourceDefinitionInformer,
+ rbrClient authzclient.Interface,
@@
- crdLister: crdLister,
+ crdLister: crdInformer.Lister(),
authzClient: rbrClient,
@@
- return factory.New().WithInformers(
+ return factory.New().WithInformers(
// track openshift-config for changes to the provider's CA bundle
kubeInformersForNamespaces.InformersFor(configNamespace).Core().V1().ConfigMaps().Informer(),
// track auth resource
- configInformer.Config().V1().Authentications().Informer(),
+ configInformer.Config().V1().Authentications().Informer(),
+ // track CRD cache sync to avoid false negatives in preconditions
+ crdInformer.Informer(),
).WithFilteredEventsInformers(Add the needed import at top:
+ apiextensionsv1informers "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions/apiextensions/v1"And update the call sites (see starter files) to pass the typed informer.
Also applies to: 78-81
🤖 Prompt for AI Agents
In pkg/controllers/externaloidc/externaloidc_controller.go around lines 66 to 70
(and similarly 78 to 81), the constructor should accept a typed
CustomResourceDefinition informer instead of a lister to avoid cache-sync races;
update the function signature to add
apiextensionsinformersv1.CustomResourceDefinitionInformer, set c.crdLister =
crdInformer.Lister(), include crdInformer.Informer() in the
factory.WithInformers(...) call so the controller factory waits for CRD cache
sync, add the apiextensionsinformersv1 import at the top of the file, and update
all call sites to pass the typed CRD informer instead of the lister.
| func (c *externalOIDCController) checkOIDCPreconditions(ctx context.Context) error { | ||
| if _, err := c.crdLister.Get("rolebindingrestrictions.authorization.openshift.io"); err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("could not get RoleBindingRestrictions CRD: %v", err) | ||
| } | ||
| // CRD doesn't exist - continue with OIDC config | ||
| return nil | ||
| } | ||
|
|
||
| // CRD exists - check if any RoleBindingRestriction objects exist | ||
| rbrs, err := c.authzClient.AuthorizationV1().RoleBindingRestrictions("").List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("could not list existing RoleBindingRestrictions: %v", err) | ||
| } | ||
|
|
||
| if len(rbrs.Items) > 0 { | ||
| // RoleBindingRestriction objects exist - precondition failed | ||
| var rbrNames []string | ||
| for _, rbr := range rbrs.Items { | ||
| rbrNames = append(rbrNames, fmt.Sprintf("%s/%s", rbr.Namespace, rbr.Name)) | ||
| } | ||
| return fmt.Errorf("no RoleBindingRestriction objects must exist; found: %s", strings.Join(rbrNames, " ")) | ||
| } | ||
|
|
||
| // no RoleBindingRestriction objects exist - continue with OIDC config | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Guard against unsynced CRD cache.
Without waiting for the CRD informer's cache to sync, Get() may temporarily return NotFound and let OIDC proceed even if the CRD exists. The constructor change above addresses this by adding the CRD informer to WithInformers.
| WithConditionalResources(bindata.Asset, | ||
| []string{ | ||
| "oauth-openshift/authorization.openshift.io_rolebindingrestrictions.yaml", | ||
| }, | ||
| // shouldCreateFnArg | ||
| func() bool { | ||
| return !oidcAvailable(authConfigChecker) | ||
| }, | ||
| // shouldDeleteFnArg | ||
| func() bool { | ||
| return oidcAvailable(authConfigChecker) && | ||
| crdExists(ctx, informerFactories.apiextensionsInformer, "rolebindingrestrictions.authorization.openshift.io") && | ||
| !rbrsExist(ctx, authOperatorInput.authzClient) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Deletion condition is unsafe on RBR List errors.
ShouldDelete uses !rbrsExist(...). rbrsExist currently returns false on List error, which would allow CRD deletion even if RBRs exist but are temporarily unreadable. Make the check conservative: treat errors as “RBRs exist” (do not delete). See fix diff on rbrsExist below.
Also verify the operator RBAC permits cluster-wide List on RoleBindingRestrictions to avoid persistent errors (script below).
🏁 Script executed:
#!/bin/bash
echo "=== rbrsExist implementation ==="
rg -n "func rbrsExist" -C5 .
echo "=== ClusterRole definitions (YAML) ==="
rg -n --glob '*.yaml' "ClusterRole" -C5 .
echo "=== RoleBindingRestrictions rules in YAML ==="
rg -n --glob '*.yaml' "rolebindingrestrictions" -C3 .Length of output: 43359
Treat RBR list errors as existing resources to prevent unsafe deletion
In pkg/operator/starter.go, change rbrsExist to return true on List errors (instead of false) so that !rbrsExist(err) cannot trigger CRD deletion when the List call fails.
Operator’s service account is already bound to cluster-admin in manifests/04_roles.yaml, so it has cluster-wide List permission on RoleBindingRestrictions.
🤖 Prompt for AI Agents
In pkg/operator/starter.go around lines 172 to 186, the call site uses rbrsExist
to decide whether to delete the RoleBindingRestrictions CRD; change rbrsExist so
that if the underlying List call returns an error it treats the resource as
existing (i.e., return true) instead of false. Update rbrsExist to detect List
errors, log the error at warning/verbose level for diagnostics, and return true
on error so that the deletion condition (!rbrsExist) cannot be triggered by
transient or permission errors.
| informerFactories.apiextensionsInformer.Apiextensions().V1().CustomResourceDefinitions().Lister(), | ||
| authOperatorInput.authzClient, | ||
| authOperatorInput.eventRecorder, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same informer-sync concern as in replacement_starter.go.
Pass the CRD informer (not only the lister) into NewExternalOIDCController and add it to WithInformers to avoid false negatives before cache sync. See suggested constructor change in the controller file.
🤖 Prompt for AI Agents
In pkg/operator/starter.go around lines 780 to 782, the call currently passes
only the CRD lister into NewExternalOIDCController which risks operations
running before the informer cache is synced; change the call to pass the CRD
informer (not just the lister) into NewExternalOIDCController and ensure that
informer is added to the controller manager's WithInformers list so the
controller waits for the CRD informer to sync before starting. Update the
constructor invocation to accept the informer, adjust any parameter ordering if
needed, and add the same informer variable to the WithInformers(...) call
alongside the other informers.
| func rbrsExist(ctx context.Context, authzClient authzclient.Interface) bool { | ||
| rbrs, err := authzClient.AuthorizationV1().RoleBindingRestrictions("").List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| klog.Infof("error while checking if any RoleBindingRestrictions exist: %v", err) | ||
| return false | ||
| } | ||
|
|
||
| return len(rbrs.Items) > 0 | ||
| } |
There was a problem hiding this comment.
Fix rbrsExist to be conservative on errors.
Avoid deleting the CRD if we cannot list RBRs. Return true on error (assume they exist) and log at warning level.
func rbrsExist(ctx context.Context, authzClient authzclient.Interface) bool {
rbrs, err := authzClient.AuthorizationV1().RoleBindingRestrictions("").List(ctx, metav1.ListOptions{})
if err != nil {
- klog.Infof("error while checking if any RoleBindingRestrictions exist: %v", err)
- return false
+ klog.Warningf("failed to list RoleBindingRestrictions; conservatively assuming they exist: %v", err)
+ return true
}
return len(rbrs.Items) > 0
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func rbrsExist(ctx context.Context, authzClient authzclient.Interface) bool { | |
| rbrs, err := authzClient.AuthorizationV1().RoleBindingRestrictions("").List(ctx, metav1.ListOptions{}) | |
| if err != nil { | |
| klog.Infof("error while checking if any RoleBindingRestrictions exist: %v", err) | |
| return false | |
| } | |
| return len(rbrs.Items) > 0 | |
| } | |
| func rbrsExist(ctx context.Context, authzClient authzclient.Interface) bool { | |
| rbrs, err := authzClient.AuthorizationV1().RoleBindingRestrictions("").List(ctx, metav1.ListOptions{}) | |
| if err != nil { | |
| klog.Warningf("failed to list RoleBindingRestrictions; conservatively assuming they exist: %v", err) | |
| return true | |
| } | |
| return len(rbrs.Items) > 0 | |
| } |
🤖 Prompt for AI Agents
In pkg/operator/starter.go around lines 894 to 902, the rbrsExist function
currently logs list errors at info level and returns false, which can cause the
CRD to be deleted when listing fails; change the error handling to log at
warning level (klog.Warningf) and return true on error (assume
RoleBindingRestrictions exist), keeping the existing successful-path return of
len(rbrs.Items) > 0; also defensively handle a nil rbrs by returning true on
error and only evaluating Items when err is nil.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
|
@liouk: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
3 participants
This PR introduces two main changes in the auth-operator:
Blocked by:
openshift/api#2414
/hold