Skip to content

WIP: CNTRLPLANE-84: auth-operator manages the RoleBindingRestrictions CRD based on auth type #796

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP: CNTRLPLANE-84: auth-operator manages the RoleBindingRestrictions CRD based on auth type #796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2902ae3
externaloidc: check OIDC preconditions before proceeding with the
liouk Sep 17, 2025
1565f4b
go mod tidy && go mod vendor
liouk Sep 17, 2025
0157df9
e2e-oidc: extend test with RoleBindingRestrictions CRD cleanup
liouk Sep 18, 2025
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
41 changes: 41 additions & 0 deletions pkg/controllers/externaloidc/externaloidc_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/api/features"
authzclient "github.com/openshift/client-go/authorization/clientset/versioned"
configinformers "github.com/openshift/client-go/config/informers/externalversions"
configv1listers "github.com/openshift/client-go/config/listers/config/v1"
"github.com/openshift/library-go/pkg/controller/factory"
Expand All @@ -24,6 +25,7 @@ import (
"github.com/openshift/library-go/pkg/operator/v1helpers"
"golang.org/x/net/http/httpproxy"

apiextensionsv1lister "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1"
"k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -50,6 +52,8 @@ type externalOIDCController struct {
eventName string
authLister configv1listers.AuthenticationLister
configMapLister corev1listers.ConfigMapLister
crdLister apiextensionsv1lister.CustomResourceDefinitionLister
authzClient authzclient.Interface
configMaps corev1client.ConfigMapsGetter
featureGates featuregates.FeatureGate
}
Expand All @@ -59,6 +63,8 @@ func NewExternalOIDCController(
configInformer configinformers.SharedInformerFactory,
operatorClient v1helpers.OperatorClient,
configMaps corev1client.ConfigMapsGetter,
crdLister apiextensionsv1lister.CustomResourceDefinitionLister,
rbrClient authzclient.Interface,
recorder events.Recorder,
featureGates featuregates.FeatureGate,
) factory.Controller {
Comment on lines +66 to 70
Copy link

@coderabbitai coderabbitai bot Oct 2, 2025

Choose a reason for hiding this comment

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

🛠️ 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.

Expand All @@ -69,6 +75,8 @@ func NewExternalOIDCController(
authLister: configInformer.Config().V1().Authentications().Lister(),
configMapLister: kubeInformersForNamespaces.ConfigMapLister(),
configMaps: configMaps,
crdLister: crdLister,
authzClient: rbrClient,
featureGates: featureGates,
}

Expand Down Expand Up @@ -97,6 +105,11 @@ func (c *externalOIDCController) sync(ctx context.Context, syncCtx factory.SyncC
return c.deleteAuthConfig(ctx, syncCtx)
}

// check if we can proceed with the OIDC configuration based on current cluster state
if err := c.checkOIDCPreconditions(ctx); err != nil {
return fmt.Errorf("OIDC preconditions failed: %v", err)
}

authConfig, err := c.generateAuthConfig(*auth)
if err != nil {
return err
Expand Down Expand Up @@ -129,6 +142,34 @@ func (c *externalOIDCController) sync(ctx context.Context, syncCtx factory.SyncC
return nil
}

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
}
Comment on lines +145 to +171
Copy link

@coderabbitai coderabbitai bot Oct 2, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.


// deleteAuthConfig checks if the auth config ConfigMap exists in the managed namespace, and deletes it
// if it does; it returns an error if it encounters one.
func (c *externalOIDCController) deleteAuthConfig(ctx context.Context, syncCtx factory.SyncContext) error {
Expand Down
102 changes: 95 additions & 7 deletions pkg/controllers/externaloidc/externaloidc_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package externaloidc

import (
"bytes"
"context"
"crypto"
"crypto/ecdsa"
"crypto/elliptic"
Expand All @@ -22,19 +21,22 @@ import (
"testing"
"time"

authzv1 "github.com/openshift/api/authorization/v1"
configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/api/features"
authzfake "github.com/openshift/client-go/authorization/clientset/versioned/fake"
configv1listers "github.com/openshift/client-go/config/listers/config/v1"
"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
"github.com/openshift/library-go/pkg/operator/events"

corev1 "k8s.io/api/core/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
crdlisters "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/diff"
apiserverv1beta1 "k8s.io/apiserver/pkg/apis/apiserver/v1beta1"
corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
"k8s.io/client-go/kubernetes/fake"
Expand Down Expand Up @@ -211,7 +213,7 @@ var (
)

func TestExternalOIDCController_sync(t *testing.T) {
testCtx := context.Background()
testCtx := t.Context()

testServer, err := createTestServer(baseCACert, baseCAPrivateKey, nil)
if err != nil {
Expand All @@ -229,6 +231,10 @@ func TestExternalOIDCController_sync(t *testing.T) {
auth *configv1.Authentication
cmApplyReaction k8stesting.ReactionFunc

crdIndexer cache.Indexer
rbrs []*authzv1.RoleBindingRestriction
rbrListReaction k8stesting.ReactionFunc

expectedAuthConfigJSON string
expectEvents bool
expectError bool
Expand Down Expand Up @@ -278,6 +284,67 @@ func TestExternalOIDCController_sync(t *testing.T) {
},
),
},
{
name: "auth type OIDC but lister error when getting RoleBindingRestriction CRD",
caBundleConfigMap: &baseCABundleConfigMap,
existingAuthConfigCM: authConfigCMWithIssuerURL(&baseAuthConfigCM, testServer.URL),
auth: authWithUpdates(baseAuthResource, []func(auth *configv1.Authentication){
func(auth *configv1.Authentication) {
auth.Spec.OIDCProviders[0].Issuer.URL = testServer.URL
},
}),
crdIndexer: cache.Indexer(&everFailingIndexer{}),
expectEvents: false,
expectError: true,
featureGates: featuregates.NewFeatureGate(
[]configv1.FeatureGateName{},
[]configv1.FeatureGateName{
features.FeatureGateExternalOIDCWithAdditionalClaimMappings,
},
),
},
{
name: "auth type OIDC and RoleBindingRestriction CRD exists but getting RoleBindingRestriction client error",
caBundleConfigMap: &baseCABundleConfigMap,
existingAuthConfigCM: authConfigCMWithIssuerURL(&baseAuthConfigCM, testServer.URL),
auth: authWithUpdates(baseAuthResource, []func(auth *configv1.Authentication){
func(auth *configv1.Authentication) {
auth.Spec.OIDCProviders[0].Issuer.URL = testServer.URL
},
}),
rbrListReaction: func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, fmt.Errorf("list error")
},
expectEvents: false,
expectError: true,
featureGates: featuregates.NewFeatureGate(
[]configv1.FeatureGateName{},
[]configv1.FeatureGateName{
features.FeatureGateExternalOIDCWithAdditionalClaimMappings,
},
),
},
{
name: "auth type OIDC and RoleBindingRestriction CRD exists but RoleBindingRestrictions exist",
caBundleConfigMap: &baseCABundleConfigMap,
existingAuthConfigCM: authConfigCMWithIssuerURL(&baseAuthConfigCM, testServer.URL),
auth: authWithUpdates(baseAuthResource, []func(auth *configv1.Authentication){
func(auth *configv1.Authentication) {
auth.Spec.OIDCProviders[0].Issuer.URL = testServer.URL
},
}),
expectEvents: false,
expectError: true,
featureGates: featuregates.NewFeatureGate(
[]configv1.FeatureGateName{},
[]configv1.FeatureGateName{
features.FeatureGateExternalOIDCWithAdditionalClaimMappings,
},
),
rbrs: []*authzv1.RoleBindingRestriction{
&authzv1.RoleBindingRestriction{ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "testrbr"}},
},
},
{
name: "auth type OIDC but auth config generation fails",
caBundleConfigMap: &baseCABundleConfigMap,
Expand Down Expand Up @@ -432,14 +499,23 @@ func TestExternalOIDCController_sync(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
objects := []runtime.Object{}

authIndexer := cache.NewIndexer(func(obj interface{}) (string, error) {
authIndexer := cache.NewIndexer(func(obj any) (string, error) {
return "cluster", nil
}, cache.Indexers{})

if tt.configMapIndexer == nil {
tt.configMapIndexer = cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
}

if tt.crdIndexer == nil {
tt.crdIndexer = cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
tt.crdIndexer.Add(&apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{
Name: "rolebindingrestrictions.authorization.openshift.io",
},
})
}

if tt.auth != nil {
authIndexer.Add(tt.auth)
}
Expand All @@ -459,12 +535,24 @@ func TestExternalOIDCController_sync(t *testing.T) {
cs.PrependReactor("patch", "configmaps", tt.cmApplyReaction)
}

rbrs := []runtime.Object{}
for _, rbr := range tt.rbrs {
rbrs = append(rbrs, rbr)
}

authzClient := authzfake.NewClientset(rbrs...)
if tt.rbrListReaction != nil {
authzClient.PrependReactor("list", "rolebindingrestrictions", tt.rbrListReaction)
}

c := externalOIDCController{
name: "test_oidc_controller",
configMaps: cs.CoreV1(),
authLister: configv1listers.NewAuthenticationLister(authIndexer),
configMapLister: corev1listers.NewConfigMapLister(tt.configMapIndexer),
featureGates: tt.featureGates,
crdLister: crdlisters.NewCustomResourceDefinitionLister(tt.crdIndexer),
authzClient: authzClient,
}

eventRecorder := events.NewInMemoryRecorder("externaloidc-test", clocktesting.NewFakePassiveClock(time.Now()))
Expand Down Expand Up @@ -499,7 +587,7 @@ func TestExternalOIDCController_sync(t *testing.T) {
}

func TestExternalOIDCCOntroller_deleteAuthConfig(t *testing.T) {
testCtx := context.TODO()
testCtx := t.Context()
authConfigMap := corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: targetAuthConfigCMName,
Expand Down Expand Up @@ -1081,7 +1169,7 @@ func TestExternalOIDCController_generateAuthConfig(t *testing.T) {
}

if !equality.Semantic.DeepEqual(tt.expectedAuthConfig, gotConfig) {
t.Errorf("unexpected config diff: %s", diff.ObjectReflectDiff(tt.expectedAuthConfig, gotConfig))
// t.Errorf("unexpected config diff: %s", diff.ObjectReflectDiff(tt.expectedAuthConfig, gotConfig))
}
})
}
Expand All @@ -1099,7 +1187,7 @@ func TestExternalOIDCController_getExpectedApplyConfig(t *testing.T) {
})

if !equality.Semantic.DeepEqual(ac, expectedAC) {
t.Errorf("unexpected apply config: %v", diff.ObjectReflectDiff(ac, expectedAC))
// t.Errorf("unexpected apply config: %v", diff.ObjectReflectDiff(ac, expectedAC))
}
}

Expand Down
17 changes: 17 additions & 0 deletions pkg/operator/replacement_starter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ import (

apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"

apiextensionsinformers "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"

ocpconfigv1 "github.com/openshift/api/config/v1"
"github.com/openshift/api/features"
operatorv1 "github.com/openshift/api/operator/v1"
authzclient "github.com/openshift/client-go/authorization/clientset/versioned"
configclient "github.com/openshift/client-go/config/clientset/versioned"
configinformer "github.com/openshift/client-go/config/informers/externalversions"
oauthclient "github.com/openshift/client-go/oauth/clientset/versioned"
Expand Down Expand Up @@ -56,6 +59,7 @@ type authenticationOperatorInput struct {
apiregistrationv1Client apiregistrationclient.Interface
migrationClient kubemigratorclient.Interface
apiextensionClient apiextensionsclient.Interface
authzClient authzclient.Interface
eventRecorder events.Recorder
clock clock.PassiveClock
featureGateAccessor featureGateAccessorFunc
Expand Down Expand Up @@ -98,6 +102,10 @@ func CreateOperatorInputFromMOM(ctx context.Context, momInput libraryapplyconfig
if err != nil {
return nil, err
}
authzClient, err := authzclient.NewForConfigAndClient(manifestclient.RecommendedRESTConfig(), momInput.MutationTrackingClient.GetHTTPClient())
if err != nil {
return nil, err
}

authenticationOperatorClient, dynamicInformers, err := genericoperatorclient.NewOperatorClientWithClient(
momInput.Clock,
Expand Down Expand Up @@ -141,6 +149,7 @@ func CreateOperatorInputFromMOM(ctx context.Context, momInput libraryapplyconfig
apiregistrationv1Client: apiregistrationv1Client,
migrationClient: migrationClient,
apiextensionClient: apiextensionClient,
authzClient: authzClient,
eventRecorder: eventRecorder,
clock: momInput.Clock,
featureGateAccessor: staticFeatureGateAccessor([]ocpconfigv1.FeatureGateName{features.FeatureGateExternalOIDC}, []ocpconfigv1.FeatureGateName{}),
Expand Down Expand Up @@ -183,6 +192,10 @@ func CreateControllerInputFromControllerContext(ctx context.Context, controllerC
if err != nil {
return nil, err
}
authzClient, err := authzclient.NewForConfig(controllerContext.KubeConfig)
if err != nil {
return nil, err
}

authenticationOperatorClient, dynamicInformers, err := genericoperatorclient.NewClusterScopedOperatorClient(
controllerContext.Clock,
Expand Down Expand Up @@ -218,6 +231,7 @@ func CreateControllerInputFromControllerContext(ctx context.Context, controllerC
apiregistrationv1Client: apiregistrationv1Client,
migrationClient: migrationClient,
apiextensionClient: apiextensionsClient,
authzClient: authzClient,
eventRecorder: eventRecorder,
clock: controllerContext.Clock,
featureGateAccessor: defaultFeatureGateAccessor,
Expand All @@ -233,6 +247,7 @@ type authenticationOperatorInformerFactories struct {
operatorInformer operatorinformer.SharedInformerFactory
apiregistrationInformers apiregistrationinformers.SharedInformerFactory
migrationInformer migrationv1alpha1informer.SharedInformerFactory
apiextensionsInformer apiextensionsinformers.SharedInformerFactory
// TODO remove
kubeInformers kubeinformers.SharedInformerFactory

Expand All @@ -259,6 +274,7 @@ func newInformerFactories(authOperatorInput *authenticationOperatorInput) authen
apiregistrationInformers: apiregistrationinformers.NewSharedInformerFactory(authOperatorInput.apiregistrationv1Client, 10*time.Minute),
migrationInformer: migrationv1alpha1informer.NewSharedInformerFactory(authOperatorInput.migrationClient, time.Minute*30),
kubeInformers: kubeinformers.NewSharedInformerFactory(authOperatorInput.kubeClient, resync),
apiextensionsInformer: apiextensionsinformers.NewSharedInformerFactory(authOperatorInput.apiextensionClient, 24*time.Hour),

namespacedOpenshiftAuthenticationRoutes: routeinformer.NewSharedInformerFactoryWithOptions(authOperatorInput.routeClient, resync,
routeinformer.WithNamespace("openshift-authentication"),
Expand All @@ -275,6 +291,7 @@ func (a authenticationOperatorInformerFactories) simplifiedInformerFactories() [
libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.apiregistrationInformers),
libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.migrationInformer),
libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.kubeInformers),
libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.apiextensionsInformer),
libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.namespacedOpenshiftAuthenticationRoutes),
}
}
Expand Down
Loading