From 9af586bc828dbcf172db5cb040b9164821d257b0 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Wed, 13 Nov 2024 16:31:18 +0100 Subject: [PATCH 01/23] common: add helper func to determine whether OIDC is enabled on KAS pods --- pkg/controllers/common/external_oidc.go | 65 ++++ pkg/controllers/common/external_oidc_test.go | 311 +++++++++++++++++++ 2 files changed, 376 insertions(+) create mode 100644 pkg/controllers/common/external_oidc.go create mode 100644 pkg/controllers/common/external_oidc_test.go diff --git a/pkg/controllers/common/external_oidc.go b/pkg/controllers/common/external_oidc.go new file mode 100644 index 000000000..39978abcd --- /dev/null +++ b/pkg/controllers/common/external_oidc.go @@ -0,0 +1,65 @@ +package common + +import ( + "fmt" + "strings" + + configv1 "github.com/openshift/api/config/v1" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" + + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/util/sets" + corelistersv1 "k8s.io/client-go/listers/core/v1" +) + +// ExternalOIDCConfigAvailable checks the kubeapiservers/cluster resource for KAS pod +// rollout status; it returns true if auth type is OIDC, all KAS pods are currently on a revision +// that includes the structured auth-config ConfigMap, and the KAS args include the respective +// arg that enables usage of the structured auth-config. It returns false otherwise. +func ExternalOIDCConfigAvailable(authLister configv1listers.AuthenticationLister, kasLister operatorv1listers.KubeAPIServerLister, cmLister corelistersv1.ConfigMapLister) (bool, error) { + auth, err := authLister.Get("cluster") + if err != nil { + return false, err + } + + if auth.Spec.Type != configv1.AuthenticationTypeOIDC { + return false, nil + } + + kas, err := kasLister.Get("cluster") + if err != nil { + return false, err + } + + observedRevisions := sets.New[int32]() + for _, nodeStatus := range kas.Status.NodeStatuses { + observedRevisions.Insert(nodeStatus.CurrentRevision) + } + + for _, revision := range observedRevisions.UnsortedList() { + // ensure every observed revision includes an auth-config revisioned configmap + _, err := cmLister.ConfigMaps("openshift-kube-apiserver").Get(fmt.Sprintf("auth-config-%d", revision)) + if errors.IsNotFound(err) { + return false, nil + } else if err != nil { + return false, err + } + + // every observed revision includes a copy of the KAS config configmap + cm, err := cmLister.ConfigMaps("openshift-kube-apiserver").Get(fmt.Sprintf("config-%d", revision)) + if err != nil { + return false, err + } + + // ensure the KAS config of every observed revision contains the appropriate CLI arg for OIDC + // but not the respective ones for OAuth + if !strings.Contains(cm.Data["config.yaml"], `"oauthMetadataFile":""`) || + strings.Contains(cm.Data["config.yaml"], `"authentication-token-webhook-config-file":`) || + !strings.Contains(cm.Data["config.yaml"], `"authentication-config":["/etc/kubernetes/static-pod-resources/configmaps/auth-config/auth-config.json"]`) { + return false, nil + } + } + + return true, nil +} diff --git a/pkg/controllers/common/external_oidc_test.go b/pkg/controllers/common/external_oidc_test.go new file mode 100644 index 000000000..cd9635aa0 --- /dev/null +++ b/pkg/controllers/common/external_oidc_test.go @@ -0,0 +1,311 @@ +package common + +import ( + "testing" + + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corelistersv1 "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" +) + +const ( + kasConfigJSONWithOIDC = `"spec":{"apiServerArguments":{"authentication-config":["/etc/kubernetes/static-pod-resources/configmaps/auth-config/auth-config.json"]},"oauthMetadataFile":""}` + kasConfigJSONWithoutOIDC = `"spec":{"apiServerArguments":{"authentication-token-webhook-config-file":["/etc/kubernetes/static-pod-resources/secrets/webhook-authenticator/kubeConfig"]},"oauthMetadataFile":"/etc/kubernetes/static-pod-resources/configmaps/oauth-metadata/oauthMetadata"}` +) + +func TestExternalOIDCConfigAvailable(t *testing.T) { + for _, tt := range []struct { + name string + configMaps []*corev1.ConfigMap + authType configv1.AuthenticationType + nodeStatuses []operatorv1.NodeStatus + expectAvailable bool + expectError bool + }{ + { + name: "oidc disabled, no rollout", + configMaps: []*corev1.ConfigMap{cm("config-10", "config.yaml", kasConfigJSONWithoutOIDC)}, + authType: configv1.AuthenticationTypeIntegratedOAuth, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 10}, + {CurrentRevision: 10}, + {CurrentRevision: 10}, + }, + expectAvailable: false, + expectError: false, + }, + { + name: "oidc getting enabled, rollout in progress", + configMaps: []*corev1.ConfigMap{ + cm("config-10", "config.yaml", kasConfigJSONWithoutOIDC), + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("auth-config-11", "", ""), + }, + authType: configv1.AuthenticationTypeOIDC, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 10, TargetRevision: 11}, + {CurrentRevision: 10}, + {CurrentRevision: 10}, + }, + expectAvailable: false, + expectError: false, + }, + { + name: "oidc getting enabled, rollout in progress, one node ready", + configMaps: []*corev1.ConfigMap{ + cm("config-10", "config.yaml", kasConfigJSONWithoutOIDC), + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("auth-config-11", "", ""), + }, + authType: configv1.AuthenticationTypeOIDC, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 11}, + {CurrentRevision: 10, TargetRevision: 11}, + {CurrentRevision: 10}, + }, + expectAvailable: false, + expectError: false, + }, + { + name: "oidc getting enabled, rollout in progress, two nodes ready", + configMaps: []*corev1.ConfigMap{ + cm("config-10", "config.yaml", kasConfigJSONWithoutOIDC), + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("auth-config-11", "", ""), + }, + authType: configv1.AuthenticationTypeOIDC, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 11}, + {CurrentRevision: 11}, + {CurrentRevision: 10, TargetRevision: 11}, + }, + expectAvailable: false, + expectError: false, + }, + { + name: "oidc got enabled", + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("auth-config-11", "", ""), + }, + authType: configv1.AuthenticationTypeOIDC, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 11}, + {CurrentRevision: 11}, + {CurrentRevision: 11}, + }, + expectAvailable: true, + expectError: false, + }, + { + name: "oidc enabled, rollout in progress", + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-12", "config.yaml", kasConfigJSONWithOIDC), + cm("auth-config-11", "", ""), + cm("auth-config-12", "", ""), + }, + authType: configv1.AuthenticationTypeOIDC, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 11, TargetRevision: 12}, + {CurrentRevision: 11}, + {CurrentRevision: 11}, + }, + expectAvailable: true, + expectError: false, + }, + { + name: "oidc enabled, rollout in progress, one node ready", + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-12", "config.yaml", kasConfigJSONWithOIDC), + cm("auth-config-11", "", ""), + cm("auth-config-12", "", ""), + }, + authType: configv1.AuthenticationTypeOIDC, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 12}, + {CurrentRevision: 11, TargetRevision: 12}, + {CurrentRevision: 11}, + }, + expectAvailable: true, + expectError: false, + }, + { + name: "oidc enabled, rollout in progress, two nodes ready", + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-12", "config.yaml", kasConfigJSONWithOIDC), + cm("auth-config-11", "", ""), + cm("auth-config-12", "", ""), + }, + authType: configv1.AuthenticationTypeOIDC, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 12}, + {CurrentRevision: 12}, + {CurrentRevision: 11, TargetRevision: 12}, + }, + expectAvailable: true, + expectError: false, + }, + { + name: "oidc still enabled", + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-12", "config.yaml", kasConfigJSONWithOIDC), + cm("auth-config-11", "", ""), + cm("auth-config-12", "", ""), + }, + authType: configv1.AuthenticationTypeOIDC, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 12}, + {CurrentRevision: 12}, + {CurrentRevision: 12}, + }, + expectAvailable: true, + expectError: false, + }, + { + name: "oidc getting disabled, rollout in progress", + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-12", "config.yaml", kasConfigJSONWithOIDC), + cm("config-13", "config.yaml", kasConfigJSONWithoutOIDC), + cm("auth-config-11", "", ""), + cm("auth-config-12", "", ""), + }, + authType: configv1.AuthenticationTypeIntegratedOAuth, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 12, TargetRevision: 13}, + {CurrentRevision: 12}, + {CurrentRevision: 12}, + }, + expectAvailable: false, + expectError: false, + }, + { + name: "oidc getting disabled, rollout in progress, one node ready", + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-12", "config.yaml", kasConfigJSONWithOIDC), + cm("config-13", "config.yaml", kasConfigJSONWithoutOIDC), + cm("auth-config-11", "", ""), + cm("auth-config-12", "", ""), + }, + authType: configv1.AuthenticationTypeIntegratedOAuth, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 13}, + {CurrentRevision: 12, TargetRevision: 13}, + {CurrentRevision: 12}, + }, + expectAvailable: false, + expectError: false, + }, + { + name: "oidc getting disabled, rollout in progress, two nodes ready", + authType: configv1.AuthenticationTypeIntegratedOAuth, + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-13", "config.yaml", kasConfigJSONWithoutOIDC), + cm("auth-config-11", "", ""), + cm("auth-config-12", "", ""), + }, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 13}, + {CurrentRevision: 13}, + {CurrentRevision: 12, TargetRevision: 13}, + }, + expectAvailable: false, + expectError: false, + }, + { + name: "oidc got disabled", + configMaps: []*corev1.ConfigMap{ + cm("config-11", "config.yaml", kasConfigJSONWithOIDC), + cm("config-12", "config.yaml", kasConfigJSONWithOIDC), + cm("config-13", "config.yaml", kasConfigJSONWithoutOIDC), + cm("auth-config-11", "", ""), + cm("auth-config-12", "", ""), + }, + authType: configv1.AuthenticationTypeIntegratedOAuth, + nodeStatuses: []operatorv1.NodeStatus{ + {CurrentRevision: 13}, + {CurrentRevision: 13}, + {CurrentRevision: 13}, + }, + expectAvailable: false, + expectError: false, + }, + } { + t.Run(tt.name, func(t *testing.T) { + + cmIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + for _, cm := range tt.configMaps { + cmIndexer.Add(cm) + } + + kasIndexer := cache.NewIndexer(func(obj interface{}) (string, error) { + return "cluster", nil + }, cache.Indexers{}) + + kasIndexer.Add(&operatorv1.KubeAPIServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Status: operatorv1.KubeAPIServerStatus{ + StaticPodOperatorStatus: operatorv1.StaticPodOperatorStatus{ + NodeStatuses: tt.nodeStatuses, + }, + }, + }) + + authIndexer := cache.NewIndexer(func(obj interface{}) (string, error) { + return "cluster", nil + }, cache.Indexers{}) + + authIndexer.Add(&configv1.Authentication{ + Spec: configv1.AuthenticationSpec{ + Type: tt.authType, + }, + }) + + available, err := ExternalOIDCConfigAvailable( + configv1listers.NewAuthenticationLister(authIndexer), + operatorv1listers.NewKubeAPIServerLister(kasIndexer), + corelistersv1.NewConfigMapLister(cmIndexer), + ) + + if tt.expectError != (err != nil) { + t.Fatalf("expected error %v; got %v", tt.expectError, err) + } + + if tt.expectAvailable != available { + t.Fatalf("expected available %v; got %v", tt.expectAvailable, available) + } + }) + } +} + +func cm(name, dataKey, dataValue string) *corev1.ConfigMap { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "openshift-kube-apiserver", + }, + } + + if len(dataKey) > 0 { + cm.Data = map[string]string{ + dataKey: dataValue, + } + } + + return cm +} From 65ae5854dcab2559f1e1422b94544df921dd6ecd Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Wed, 20 Nov 2024 11:34:16 +0100 Subject: [PATCH 02/23] operator: set number of replicas of workloads with a custom wrapper If OIDC is available then set replicas to 0, otherwise fall back to the default behaviour of counting nodes. --- pkg/operator/replacement_starter.go | 1 + pkg/operator/starter.go | 39 +++++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/pkg/operator/replacement_starter.go b/pkg/operator/replacement_starter.go index 2b08fb14e..b8f4af51b 100644 --- a/pkg/operator/replacement_starter.go +++ b/pkg/operator/replacement_starter.go @@ -236,6 +236,7 @@ func newInformerFactories(authOperatorInput *authenticationOperatorInput) authen "", // an informer for non-namespaced resources "kube-system", libgoetcd.EtcdEndpointNamespace, + "openshift-kube-apiserver", ), operatorConfigInformer: configinformer.NewSharedInformerFactoryWithOptions(authOperatorInput.configClient, resync), operatorInformer: operatorinformer.NewSharedInformerFactory(authOperatorInput.operatorClient, 24*time.Hour), diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index d0ea159bc..81ee60195 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -14,7 +14,9 @@ import ( configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" "github.com/openshift/cluster-authentication-operator/bindata" "github.com/openshift/cluster-authentication-operator/pkg/controllers/configobservation/configobservercontroller" componentroutesecretsync "github.com/openshift/cluster-authentication-operator/pkg/controllers/customroute" @@ -64,9 +66,11 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/util/sets" + corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/klog/v2" apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" utilpointer "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) const ( @@ -128,6 +132,10 @@ func prepareOauthOperator( authOperatorInput.eventRecorder, ) + authLister := informerFactories.operatorConfigInformer.Config().V1().Authentications().Lister() + kasLister := informerFactories.operatorInformer.Operator().V1().KubeAPIServers().Lister() + kasConfigMapLister := informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver").Core().V1().ConfigMaps().Lister() + staticResourceController := staticresourcecontroller.NewStaticResourceController( "OpenshiftAuthenticationStaticResources", bindata.Asset, @@ -231,7 +239,12 @@ func prepareOauthOperator( deploymentController := deployment.NewOAuthServerWorkloadController( authOperatorInput.authenticationOperatorClient, - workloadcontroller.CountNodesFuncWrapper(informerFactories.kubeInformersForNamespaces.InformersFor("").Core().V1().Nodes().Lister()), + countNodesFuncWrapper( + informerFactories.kubeInformersForNamespaces.InformersFor("").Core().V1().Nodes().Lister(), + authLister, + kasLister, + kasConfigMapLister, + ), workloadcontroller.EnsureAtMostOnePodPerNode, authOperatorInput.kubeClient, informerFactories.kubeInformersForNamespaces.InformersFor("").Core().V1().Nodes(), @@ -407,9 +420,18 @@ func prepareOauthAPIServerOperator( } migrator := migrators.NewKubeStorageVersionMigrator(authOperatorInput.migrationClient, informerFactories.migrationInformer.Migration().V1alpha1(), authOperatorInput.kubeClient.Discovery()) + authLister := informerFactories.operatorConfigInformer.Config().V1().Authentications().Lister() + kasLister := informerFactories.operatorInformer.Operator().V1().KubeAPIServers().Lister() + kasConfigMapLister := informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver").Core().V1().ConfigMaps().Lister() + authAPIServerWorkload := workload.NewOAuthAPIServerWorkload( authOperatorInput.authenticationOperatorClient, - workloadcontroller.CountNodesFuncWrapper(informerFactories.kubeInformersForNamespaces.InformersFor("").Core().V1().Nodes().Lister()), + countNodesFuncWrapper( + informerFactories.kubeInformersForNamespaces.InformersFor("").Core().V1().Nodes().Lister(), + authLister, + kasLister, + kasConfigMapLister, + ), workloadcontroller.EnsureAtMostOnePodPerNode, "openshift-oauth-apiserver", os.Getenv("IMAGE_OAUTH_APISERVER"), @@ -765,3 +787,16 @@ func extractOperatorStatus(obj *unstructured.Unstructured, fieldManager string) } return &ret.Status.OperatorStatusApplyConfiguration, nil } + +func countNodesFuncWrapper(nodeLister corev1listers.NodeLister, authLister configv1listers.AuthenticationLister, kasLister operatorv1listers.KubeAPIServerLister, kasConfigMapLister corev1listers.ConfigMapLister) func(nodeSelector map[string]string) (*int32, error) { + return func(nodeSelector map[string]string) (*int32, error) { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(authLister, kasLister, kasConfigMapLister); err != nil { + return nil, err + } else if oidcAvailable { + return ptr.To(int32(0)), nil + } + + defaultFunc := workloadcontroller.CountNodesFuncWrapper(nodeLister) + return defaultFunc(nodeSelector) + } +} From 3c687a77159e0a86ff9e321e661aab7ba4a727fc Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Wed, 20 Nov 2024 10:40:15 +0100 Subject: [PATCH 03/23] deployment: bypass precondition checks if OIDC is available This allows the controller to proceed with the sync and eventually scale down the deployment to 0 replicas. --- .../deployment/deployment_controller.go | 22 +++++++++++++++++++ pkg/operator/starter.go | 2 ++ 2 files changed, 24 insertions(+) diff --git a/pkg/controllers/deployment/deployment_controller.go b/pkg/controllers/deployment/deployment_controller.go index 4889a0a93..f8455a52e 100644 --- a/pkg/controllers/deployment/deployment_controller.go +++ b/pkg/controllers/deployment/deployment_controller.go @@ -21,8 +21,11 @@ import ( configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" configinformer "github.com/openshift/client-go/config/informers/externalversions" configv1listers "github.com/openshift/client-go/config/listers/config/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" routeinformers "github.com/openshift/client-go/route/informers/externalversions" routev1listers "github.com/openshift/client-go/route/listers/route/v1" + "github.com/openshift/cluster-authentication-operator/pkg/controllers/common" bootstrap "github.com/openshift/library-go/pkg/authentication/bootstrapauthenticator" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/apiserver/controller/workload" @@ -60,6 +63,10 @@ type oauthServerDeploymentSyncer struct { proxyLister configv1listers.ProxyLister routeLister routev1listers.RouteLister + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1listers.ConfigMapLister + bootstrapUserDataGetter bootstrap.BootstrapUserDataGetter bootstrapUserChangeRollOut bool } @@ -77,6 +84,8 @@ func NewOAuthServerWorkloadController( eventsRecorder events.Recorder, versionRecorder status.VersionGetter, kubeInformersForTargetNamespace informers.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, ) factory.Controller { targetNS := "openshift-authentication" @@ -94,6 +103,10 @@ func NewOAuthServerWorkloadController( proxyLister: configInformers.Config().V1().Proxies().Lister(), routeLister: routeInformersForTargetNamespace.Route().V1().Routes().Lister(), + authLister: configInformers.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), + bootstrapUserDataGetter: bootstrapUserDataGetter, } @@ -117,7 +130,9 @@ func NewOAuthServerWorkloadController( []factory.Informer{ configInformers.Config().V1().Ingresses().Informer(), configInformers.Config().V1().Proxies().Informer(), + configInformers.Config().V1().Authentications().Informer(), nodeInformer.Informer(), + kasInformer.Informer(), }, []factory.Informer{ kubeInformersForTargetNamespace.Apps().V1().Deployments().Informer(), @@ -135,6 +150,13 @@ func NewOAuthServerWorkloadController( } func (c *oauthServerDeploymentSyncer) PreconditionFulfilled(_ context.Context) (bool, error) { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return false, err + } else if oidcAvailable { + // the route is no longer a pre-requisite + return true, nil + } + route, err := c.routeLister.Routes("openshift-authentication").Get("oauth-openshift") if err != nil { return false, fmt.Errorf("waiting for the oauth-openshift route to appear: %w", err) diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 81ee60195..747efd810 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -255,6 +255,8 @@ func prepareOauthOperator( authOperatorInput.eventRecorder, versionRecorder, informerFactories.kubeInformersForNamespaces.InformersFor("openshift-authentication"), + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), ) workersAvailableController := ingressnodesavailable.NewIngressNodesAvailableController( From f74f0b3bc75442bbc7a45dcd5aa86f52437808bb Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Wed, 13 Nov 2024 16:46:45 +0100 Subject: [PATCH 04/23] endpointaccessible: disable controller checks upon specific conditions --- .../oauth_endpoints_controller.go | 41 +++++++++++++++++-- .../endpoint_accessible_controller.go | 33 ++++++++++----- pkg/operator/starter.go | 9 ++++ 3 files changed, 70 insertions(+), 13 deletions(-) diff --git a/pkg/controllers/oauthendpoints/oauth_endpoints_controller.go b/pkg/controllers/oauthendpoints/oauth_endpoints_controller.go index dfb2e2d89..a5165d09b 100644 --- a/pkg/controllers/oauthendpoints/oauth_endpoints_controller.go +++ b/pkg/controllers/oauthendpoints/oauth_endpoints_controller.go @@ -15,8 +15,10 @@ import ( corev1listers "k8s.io/client-go/listers/core/v1" routev1 "github.com/openshift/api/route/v1" + configinformer "github.com/openshift/client-go/config/informers/externalversions" configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1" configv1lister "github.com/openshift/client-go/config/listers/config/v1" + operatorv1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" routev1informers "github.com/openshift/client-go/route/informers/externalversions/route/v1" routev1listers "github.com/openshift/client-go/route/listers/route/v1" "github.com/openshift/library-go/pkg/controller/factory" @@ -34,6 +36,9 @@ func NewOAuthRouteCheckController( kubeInformersForConfigManagedNS informers.SharedInformerFactory, routeInformerNamespaces routev1informers.RouteInformer, ingressInformerAllNamespaces configv1informers.IngressInformer, + operatorConfigInformer configinformer.SharedInformerFactory, + kasInformer operatorv1.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, systemCABundle []byte, recorder events.Recorder, ) factory.Controller { @@ -55,10 +60,18 @@ func NewOAuthRouteCheckController( return getOAuthRouteTLSConfig(cmLister, secretLister, ingressLister, systemCABundle) } + endpointCheckDisabledFunc := func() (bool, error) { + return common.ExternalOIDCConfigAvailable( + operatorConfigInformer.Config().V1().Authentications().Lister(), + kasInformer.Lister(), + kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), + ) + } + return endpointaccessible.NewEndpointAccessibleController( "OAuthServerRoute", operatorClient, - endpointListFunc, getTLSConfigFunc, + endpointListFunc, getTLSConfigFunc, endpointCheckDisabledFunc, []factory.Informer{ cmInformer, secretInformer, @@ -72,6 +85,9 @@ func NewOAuthRouteCheckController( func NewOAuthServiceCheckController( operatorClient v1helpers.OperatorClient, kubeInformersForTargetNS informers.SharedInformerFactory, + operatorConfigInformer configinformer.SharedInformerFactory, + kasInformer operatorv1.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, recorder events.Recorder, ) factory.Controller { endpointsListFunc := func() ([]string, error) { @@ -82,10 +98,18 @@ func NewOAuthServiceCheckController( return getOAuthEndpointTLSConfig(kubeInformersForTargetNS.Core().V1().ConfigMaps().Lister()) } + endpointCheckDisabledFunc := func() (bool, error) { + return common.ExternalOIDCConfigAvailable( + operatorConfigInformer.Config().V1().Authentications().Lister(), + kasInformer.Lister(), + kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), + ) + } + return endpointaccessible.NewEndpointAccessibleController( "OAuthServerService", operatorClient, - endpointsListFunc, getTLSConfigFunc, + endpointsListFunc, getTLSConfigFunc, endpointCheckDisabledFunc, []factory.Informer{ kubeInformersForTargetNS.Core().V1().ConfigMaps().Informer(), kubeInformersForTargetNS.Core().V1().Services().Informer(), @@ -98,6 +122,9 @@ func NewOAuthServiceCheckController( func NewOAuthServiceEndpointsCheckController( operatorClient v1helpers.OperatorClient, kubeInformersForTargetNS informers.SharedInformerFactory, + operatorConfigInformer configinformer.SharedInformerFactory, + kasInformer operatorv1.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, recorder events.Recorder, ) factory.Controller { endpointsListFn := func() ([]string, error) { @@ -108,10 +135,18 @@ func NewOAuthServiceEndpointsCheckController( return getOAuthEndpointTLSConfig(kubeInformersForTargetNS.Core().V1().ConfigMaps().Lister()) } + endpointCheckDisabledFunc := func() (bool, error) { + return common.ExternalOIDCConfigAvailable( + operatorConfigInformer.Config().V1().Authentications().Lister(), + kasInformer.Lister(), + kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), + ) + } + return endpointaccessible.NewEndpointAccessibleController( "OAuthServerServiceEndpoints", operatorClient, - endpointsListFn, getTLSConfigFunc, + endpointsListFn, getTLSConfigFunc, endpointCheckDisabledFunc, []factory.Informer{ kubeInformersForTargetNS.Core().V1().Endpoints().Informer(), kubeInformersForTargetNS.Core().V1().ConfigMaps().Informer(), diff --git a/pkg/libs/endpointaccessible/endpoint_accessible_controller.go b/pkg/libs/endpointaccessible/endpoint_accessible_controller.go index e4a6dd96d..f8372a18c 100644 --- a/pkg/libs/endpointaccessible/endpoint_accessible_controller.go +++ b/pkg/libs/endpointaccessible/endpoint_accessible_controller.go @@ -21,15 +21,17 @@ import ( ) type endpointAccessibleController struct { - controllerInstanceName string - operatorClient v1helpers.OperatorClient - endpointListFn EndpointListFunc - getTLSConfigFn EndpointTLSConfigFunc - availableConditionName string + controllerInstanceName string + operatorClient v1helpers.OperatorClient + endpointListFn EndpointListFunc + getTLSConfigFn EndpointTLSConfigFunc + availableConditionName string + endpointCheckDisabledFunc EndpointCheckDisabledFunc } type EndpointListFunc func() ([]string, error) type EndpointTLSConfigFunc func() (*tls.Config, error) +type EndpointCheckDisabledFunc func() (bool, error) // NewEndpointAccessibleController returns a controller that checks if the endpoints // listed by endpointListFn are reachable @@ -38,17 +40,19 @@ func NewEndpointAccessibleController( operatorClient v1helpers.OperatorClient, endpointListFn EndpointListFunc, getTLSConfigFn EndpointTLSConfigFunc, + endpointCheckDisabledFunc EndpointCheckDisabledFunc, triggers []factory.Informer, recorder events.Recorder, ) factory.Controller { controllerName := name + "EndpointAccessibleController" c := &endpointAccessibleController{ - controllerInstanceName: factory.ControllerInstanceName(name, "EndpointAccessible"), - operatorClient: operatorClient, - endpointListFn: endpointListFn, - getTLSConfigFn: getTLSConfigFn, - availableConditionName: name + "EndpointAccessibleControllerAvailable", + controllerInstanceName: factory.ControllerInstanceName(name, "EndpointAccessible"), + operatorClient: operatorClient, + endpointListFn: endpointListFn, + getTLSConfigFn: getTLSConfigFn, + availableConditionName: name + "EndpointAccessibleControllerAvailable", + endpointCheckDisabledFunc: endpointCheckDisabledFunc, } return factory.New(). @@ -73,6 +77,15 @@ func humanizeError(err error) error { } func (c *endpointAccessibleController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if c.endpointCheckDisabledFunc != nil { + if skip, err := c.endpointCheckDisabledFunc(); err != nil { + return err + + } else if skip { + return nil + } + } + endpoints, err := c.endpointListFn() if err != nil { if apierrors.IsNotFound(err) { diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 747efd810..c1b0225a0 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -278,6 +278,9 @@ func prepareOauthOperator( informerFactories.kubeInformersForNamespaces.InformersFor("openshift-config-managed"), informerFactories.namespacedOpenshiftAuthenticationRoutes.Route().V1().Routes(), informerFactories.operatorConfigInformer.Config().V1().Ingresses(), + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), systemCABundle, authOperatorInput.eventRecorder, ) @@ -285,12 +288,18 @@ func prepareOauthOperator( authServiceCheckController := oauthendpoints.NewOAuthServiceCheckController( authOperatorInput.authenticationOperatorClient, informerFactories.kubeInformersForNamespaces.InformersFor("openshift-authentication"), + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.eventRecorder, ) authServiceEndpointCheckController := oauthendpoints.NewOAuthServiceEndpointsCheckController( authOperatorInput.authenticationOperatorClient, informerFactories.kubeInformersForNamespaces.InformersFor("openshift-authentication"), + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.eventRecorder, ) From 3850c62b71f09aec620965a615b42c165f8a3b9f Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Thu, 14 Nov 2024 11:50:00 +0100 Subject: [PATCH 05/23] proxyconfig: disable proxy checks when external OIDC config is available --- .../proxyconfig/proxyconfig_controller.go | 23 +++++++++++++++++++ pkg/operator/starter.go | 3 +++ 2 files changed, 26 insertions(+) diff --git a/pkg/controllers/proxyconfig/proxyconfig_controller.go b/pkg/controllers/proxyconfig/proxyconfig_controller.go index 897dd9b2f..f77654432 100644 --- a/pkg/controllers/proxyconfig/proxyconfig_controller.go +++ b/pkg/controllers/proxyconfig/proxyconfig_controller.go @@ -12,11 +12,17 @@ import ( "golang.org/x/net/http/httpproxy" + "k8s.io/client-go/informers" corev1lister "k8s.io/client-go/listers/core/v1" "k8s.io/klog/v2" + configinformer "github.com/openshift/client-go/config/informers/externalversions" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" + operatorv1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" routeinformer "github.com/openshift/client-go/route/informers/externalversions/route/v1" v1 "github.com/openshift/client-go/route/listers/route/v1" + "github.com/openshift/cluster-authentication-operator/pkg/controllers/common" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/v1helpers" @@ -30,11 +36,18 @@ type proxyConfigChecker struct { routeName string routeNamespace string caConfigMaps map[string][]string // ns -> []configmapNames + + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1lister.ConfigMapLister } func NewProxyConfigChecker( routeInformer routeinformer.RouteInformer, configMapInformers v1helpers.KubeInformersForNamespaces, + configInformers configinformer.SharedInformerFactory, + kasInformer operatorv1.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, routeNamespace string, routeName string, caConfigMaps map[string][]string, @@ -46,6 +59,10 @@ func NewProxyConfigChecker( routeName: routeName, routeNamespace: routeNamespace, caConfigMaps: caConfigMaps, + + authLister: configInformers.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } c := factory.New(). @@ -68,6 +85,12 @@ func NewProxyConfigChecker( // sync attempts to connect to route using configured proxy settings and reports any error. func (p *proxyConfigChecker) sync(ctx context.Context, _ factory.SyncContext) error { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(p.authLister, p.kasLister, p.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + return nil + } + proxyConfig := httpproxy.FromEnvironment() if !isProxyConfigured(proxyConfig) { // If proxy is not configured, then it is a no-op. diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index c1b0225a0..67626d0a7 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -306,6 +306,9 @@ func prepareOauthOperator( proxyConfigController := proxyconfig.NewProxyConfigChecker( informerFactories.namespacedOpenshiftAuthenticationRoutes.Route().V1().Routes(), informerFactories.kubeInformersForNamespaces, + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), "openshift-authentication", "oauth-openshift", map[string][]string{ From ebe2b3f3ee7411a61d827c09d54e62a6c6b290ce Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Thu, 14 Nov 2024 11:51:41 +0100 Subject: [PATCH 06/23] ingressnodesavailable: disable checks when external OIDC config is available --- .../ingress_nodes_available_controller.go | 21 +++++++++++++++++++ pkg/operator/starter.go | 3 +++ 2 files changed, 24 insertions(+) diff --git a/pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller.go b/pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller.go index f8c15fba9..c8e1150c6 100644 --- a/pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller.go +++ b/pkg/controllers/ingressnodesavailable/ingress_nodes_available_controller.go @@ -6,18 +6,22 @@ import ( "time" operatorv1 "github.com/openshift/api/operator/v1" + configinformer "github.com/openshift/client-go/config/informers/externalversions" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" "github.com/openshift/cluster-authentication-operator/pkg/controllers/common" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/v1helpers" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/informers" corev1informers "k8s.io/client-go/informers/core/v1" corev1listers "k8s.io/client-go/listers/core/v1" ) @@ -32,6 +36,10 @@ type ingressNodesAvailableController struct { operatorClient v1helpers.OperatorClient ingressLister operatorv1listers.IngressControllerLister nodeLister corev1listers.NodeLister + + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1listers.ConfigMapLister } func NewIngressNodesAvailableController( @@ -40,12 +48,19 @@ func NewIngressNodesAvailableController( ingressControllerInformer operatorv1informers.IngressControllerInformer, eventRecorder events.Recorder, nodeInformer corev1informers.NodeInformer, + configInformers configinformer.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, ) factory.Controller { controller := &ingressNodesAvailableController{ controllerInstanceName: factory.ControllerInstanceName(instanceName, "IngressNodesAvailable"), operatorClient: operatorClient, ingressLister: ingressControllerInformer.Lister(), nodeLister: nodeInformer.Lister(), + + authLister: configInformers.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } return factory.New(). @@ -72,6 +87,12 @@ func countReadyWorkerNodes(nodes []*corev1.Node) int { } func (c *ingressNodesAvailableController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + return common.ApplyControllerConditions(ctx, c.operatorClient, c.controllerInstanceName, knownConditionNames, nil) + } + foundConditions := []operatorv1.OperatorCondition{} workers, err := c.nodeLister.List(labels.SelectorFromSet(labels.Set{"node-role.kubernetes.io/worker": ""})) diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 67626d0a7..e8888077d 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -265,6 +265,9 @@ func prepareOauthOperator( informerFactories.operatorInformer.Operator().V1().IngressControllers(), authOperatorInput.eventRecorder, informerFactories.kubeInformersForNamespaces.InformersFor("").Core().V1().Nodes(), + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), ) systemCABundle, err := loadSystemCACertBundle() From bc6089e066b0dce581b7d3854c8e425177d48800 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Thu, 14 Nov 2024 11:53:09 +0100 Subject: [PATCH 07/23] ingressstate: disable checks when external OIDC config is available --- .../ingressstate/ingress_state_controller.go | 27 +++++++++++++++++-- pkg/operator/starter.go | 3 +++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/ingressstate/ingress_state_controller.go b/pkg/controllers/ingressstate/ingress_state_controller.go index 190b3bc28..4f192fabc 100644 --- a/pkg/controllers/ingressstate/ingress_state_controller.go +++ b/pkg/controllers/ingressstate/ingress_state_controller.go @@ -7,9 +7,15 @@ import ( "time" operatorv1 "github.com/openshift/api/operator/v1" + configinformer "github.com/openshift/client-go/config/informers/externalversions" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" + "github.com/openshift/cluster-authentication-operator/pkg/controllers/common" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/v1helpers" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -17,8 +23,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/informers" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" - - "github.com/openshift/cluster-authentication-operator/pkg/controllers/common" + corev1listers "k8s.io/client-go/listers/core/v1" ) const ( @@ -37,11 +42,18 @@ type ingressStateController struct { podsGetter corev1client.PodsGetter targetNamespace string operatorClient v1helpers.OperatorClient + + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1listers.ConfigMapLister } func NewIngressStateController( instanceName string, kubeInformersForTargetNamespace informers.SharedInformerFactory, + configInformers configinformer.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, endpointsGetter corev1client.EndpointsGetter, podsGetter corev1client.PodsGetter, operatorClient v1helpers.OperatorClient, @@ -54,6 +66,10 @@ func NewIngressStateController( podsGetter: podsGetter, targetNamespace: targetNamespace, operatorClient: operatorClient, + + authLister: configInformers.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } return factory.New(). @@ -76,6 +92,13 @@ func (c *ingressStateController) checkPodStatus(ctx context.Context, reference * } func (c *ingressStateController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + // clear all operator conditions + return common.ApplyControllerConditions(ctx, c.operatorClient, c.controllerInstanceName, degradedConditionTypes, nil) + } + endpoints, err := c.endpointsGetter.Endpoints(c.targetNamespace).Get(context.TODO(), "oauth-openshift", metav1.GetOptions{}) if apierrors.IsNotFound(err) { // Clear the error to allow checkSubset to report degraded because endpoints == nil diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index e8888077d..14cd5c125 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -182,6 +182,9 @@ func prepareOauthOperator( ingressStateController := ingressstate.NewIngressStateController( "openshift-authentication", informerFactories.kubeInformersForNamespaces.InformersFor("openshift-authentication"), + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.kubeClient.CoreV1(), authOperatorInput.kubeClient.CoreV1(), authOperatorInput.authenticationOperatorClient, From a8081742f818da329049bdd1876eb0d3f32a87dc Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Thu, 14 Nov 2024 11:54:26 +0100 Subject: [PATCH 08/23] readiness: disable checks when external OIDC config is available --- .../readiness/wellknown_ready_controller.go | 37 +++++++++++++++++-- pkg/operator/starter.go | 2 + 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/pkg/controllers/readiness/wellknown_ready_controller.go b/pkg/controllers/readiness/wellknown_ready_controller.go index 578ab9e8f..2d286c695 100644 --- a/pkg/controllers/readiness/wellknown_ready_controller.go +++ b/pkg/controllers/readiness/wellknown_ready_controller.go @@ -19,6 +19,8 @@ import ( configinformer "github.com/openshift/client-go/config/informers/externalversions" configv1lister "github.com/openshift/client-go/config/listers/config/v1" applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" routeinformer "github.com/openshift/client-go/route/informers/externalversions/route/v1" routev1lister "github.com/openshift/client-go/route/listers/route/v1" "github.com/openshift/cluster-authentication-operator/pkg/controllers/common" @@ -26,12 +28,14 @@ import ( "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/v1helpers" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" netutil "k8s.io/apimachinery/pkg/util/net" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/informers" corev1lister "k8s.io/client-go/listers/core/v1" "k8s.io/klog/v2" ) @@ -58,12 +62,23 @@ type wellKnownReadyController struct { configMapLister corev1lister.ConfigMapLister routeLister routev1lister.RouteLister infrastructureLister configv1lister.InfrastructureLister + + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1lister.ConfigMapLister } const controllerName = "WellKnownReadyController" -func NewWellKnownReadyController(instanceName string, kubeInformers v1helpers.KubeInformersForNamespaces, configInformers configinformer.SharedInformerFactory, routeInformer routeinformer.RouteInformer, - operatorClient v1helpers.OperatorClient, recorder events.Recorder) factory.Controller { +func NewWellKnownReadyController( + instanceName string, + kubeInformers v1helpers.KubeInformersForNamespaces, + configInformers configinformer.SharedInformerFactory, + routeInformer routeinformer.RouteInformer, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, + operatorClient v1helpers.OperatorClient, + recorder events.Recorder, +) factory.Controller { nsOpenshiftConfigManagedInformers := kubeInformers.InformersFor("openshift-config-managed") nsDefaultInformers := kubeInformers.InformersFor("default") @@ -77,6 +92,9 @@ func NewWellKnownReadyController(instanceName string, kubeInformers v1helpers.Ku configMapLister: nsOpenshiftConfigManagedInformers.Core().V1().ConfigMaps().Lister(), routeLister: routeInformer.Lister(), operatorClient: operatorClient, + + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } return factory.New().WithInformers( @@ -128,6 +146,19 @@ func (c *wellKnownReadyController) sync(ctx context.Context, controllerContext f } }() + // if OIDC is enabled, clear operator conditions and skip checks + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + available = available. + WithStatus(operatorv1.ConditionFalse). + WithMessage("well-known endpoint checks skipped."). + WithReason("NotAvailableForOIDC") + progressing = progressing. + WithStatus(operatorv1.ConditionFalse) + return nil + } + // the well-known endpoint cannot be ready until we know the oauth-server's hostname _, err = c.routeLister.Routes("openshift-authentication").Get("oauth-openshift") if apierrors.IsNotFound(err) { @@ -185,7 +216,7 @@ func (c *wellKnownReadyController) isWellknownEndpointsReady(ctx context.Context return nil } - caData, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/ca.crt") + caData, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/ca.crt") if err != nil { return fmt.Errorf("failed to read SA ca.crt: %v", err) } diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 14cd5c125..47daee466 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -196,6 +196,8 @@ func prepareOauthOperator( informerFactories.kubeInformersForNamespaces, informerFactories.operatorConfigInformer, informerFactories.namespacedOpenshiftAuthenticationRoutes.Route().V1().Routes(), + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.authenticationOperatorClient, authOperatorInput.eventRecorder, ) From 2b6ca40883631afd0888c46fbbdf273a950742a9 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Thu, 14 Nov 2024 11:48:23 +0100 Subject: [PATCH 09/23] oauthclientscontroller: remove operands when external OIDC config is available --- .../oauthclientscontroller.go | 52 +++++++++++++++++-- .../oauthclientscontroller_test.go | 14 +++++ pkg/operator/starter.go | 2 + 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/pkg/controllers/oauthclientscontroller/oauthclientscontroller.go b/pkg/controllers/oauthclientscontroller/oauthclientscontroller.go index 5f6fba868..f589b06d1 100644 --- a/pkg/controllers/oauthclientscontroller/oauthclientscontroller.go +++ b/pkg/controllers/oauthclientscontroller/oauthclientscontroller.go @@ -8,9 +8,12 @@ import ( "time" "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/informers" + corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/util/retry" configv1 "github.com/openshift/api/config/v1" @@ -20,6 +23,8 @@ import ( oauthclient "github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1" oauthinformers "github.com/openshift/client-go/oauth/informers/externalversions" oauthv1listers "github.com/openshift/client-go/oauth/listers/oauth/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" routeinformers "github.com/openshift/client-go/route/informers/externalversions" routev1listers "github.com/openshift/client-go/route/listers/route/v1" "github.com/openshift/library-go/pkg/controller/factory" @@ -38,6 +43,10 @@ type oauthsClientsController struct { oauthClientLister oauthv1listers.OAuthClientLister routeLister routev1listers.RouteLister ingressLister configv1listers.IngressLister + + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1listers.ConfigMapLister } func NewOAuthClientsController( @@ -45,7 +54,9 @@ func NewOAuthClientsController( oauthsClientClient oauthclient.OAuthClientInterface, oauthInformers oauthinformers.SharedInformerFactory, routeInformers routeinformers.SharedInformerFactory, - ingressInformers configinformers.SharedInformerFactory, + operatorConfigInformers configinformers.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, eventRecorder events.Recorder, ) factory.Controller { c := &oauthsClientsController{ @@ -53,7 +64,11 @@ func NewOAuthClientsController( oauthClientLister: oauthInformers.Oauth().V1().OAuthClients().Lister(), routeLister: routeInformers.Route().V1().Routes().Lister(), - ingressLister: ingressInformers.Config().V1().Ingresses().Lister(), + ingressLister: operatorConfigInformers.Config().V1().Ingresses().Lister(), + + authLister: operatorConfigInformers.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } return factory.New(). @@ -67,12 +82,22 @@ func NewOAuthClientsController( factory.NamesFilter("oauth-openshift"), routeInformers.Route().V1().Routes().Informer(), ). - WithInformers(ingressInformers.Config().V1().Ingresses().Informer()). + WithInformers( + operatorConfigInformers.Config().V1().Ingresses().Informer(), + operatorConfigInformers.Config().V1().Authentications().Informer(), + kasInformer.Informer(), + ). ResyncEvery(wait.Jitter(time.Minute, 1.0)). ToController("OAuthClientsController", eventRecorder.WithComponentSuffix("oauth-clients-controller")) } func (c *oauthsClientsController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + return c.ensureBootstrappedOAuthClientsMissing(ctx) + } + ingress, err := c.getIngressConfig() if err != nil { return err @@ -146,6 +171,27 @@ func (c *oauthsClientsController) ensureBootstrappedOAuthClients(ctx context.Con return nil } +func (c *oauthsClientsController) ensureBootstrappedOAuthClientsMissing(ctx context.Context) error { + for _, clientName := range []string{ + "openshift-browser-client", + "openshift-challenging-client", + "openshift-cli-client", + } { + _, err := c.oauthClientLister.Get(clientName) + if errors.IsNotFound(err) { + continue + } else if err != nil { + return err + } + + if err := c.oauthClientClient.Delete(ctx, clientName, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) { + return err + } + } + + return nil +} + func randomBits(bits uint) []byte { size := bits / 8 if bits%8 != 0 { diff --git a/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go b/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go index 71346a40f..48bd87a33 100644 --- a/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go +++ b/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go @@ -104,12 +104,26 @@ func newRouteLister(t *testing.T, routes ...*routev1.Route) routev1listers.Route return routev1listers.NewRouteLister(routeIndexer) } +func newAuthLister(t *testing.T) configv1listers.AuthenticationLister { + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + indexer.Add(&configv1.Authentication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Spec: configv1.AuthenticationSpec{ + Type: configv1.AuthenticationTypeIntegratedOAuth, + }, + }) + return configv1listers.NewAuthenticationLister(indexer) +} + func newTestOAuthsClientsController(t *testing.T) *oauthsClientsController { return &oauthsClientsController{ oauthClientClient: fakeoauthclient.NewSimpleClientset().OauthV1().OAuthClients(), oauthClientLister: oauthv1listers.NewOAuthClientLister(cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})), routeLister: newRouteLister(t, defaultRoute), ingressLister: newIngressLister(t, defaultIngress), + authLister: newAuthLister(t), } } diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 47daee466..640e7341b 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -239,6 +239,8 @@ func prepareOauthOperator( informerFactories.oauthInformers, informerFactories.namespacedOpenshiftAuthenticationRoutes, informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.eventRecorder, ) From 35e4f195b828972074c9e1558ba6691027d36464 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Thu, 14 Nov 2024 14:13:41 +0100 Subject: [PATCH 10/23] metadata: remove operands if external OIDC config is available --- .../metadata/metadata_controller.go | 54 +++++++++++++++++-- pkg/operator/starter.go | 2 + 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/pkg/controllers/metadata/metadata_controller.go b/pkg/controllers/metadata/metadata_controller.go index 267c2ea9a..d3d3d3081 100644 --- a/pkg/controllers/metadata/metadata_controller.go +++ b/pkg/controllers/metadata/metadata_controller.go @@ -7,6 +7,7 @@ import ( "time" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" @@ -19,6 +20,8 @@ import ( configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" configinformers "github.com/openshift/client-go/config/informers/externalversions" configv1listers "github.com/openshift/client-go/config/listers/config/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" routeclient "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1" routeinformer "github.com/openshift/client-go/route/informers/externalversions" "github.com/openshift/library-go/pkg/controller/factory" @@ -43,36 +46,66 @@ type metadataController struct { ingressLister configv1listers.IngressLister route routeclient.RouteInterface secretLister corev1listers.SecretLister + configMapLister corev1listers.ConfigMapLister configMaps corev1client.ConfigMapsGetter authentication configv1client.AuthenticationInterface operatorClient v1helpers.OperatorClient + + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1listers.ConfigMapLister } // NewMetadataController assure that ingress configuration is available to determine the domain suffix that this controller use to create // a route for oauth. The controller then update the oauth metadata config map and update the cluster authentication config. // The controller use degraded condition if any part of the process fail and use the "AuthMetadataProgressing=false" condition when the controller job is done // and all resources exists. -func NewMetadataController(instanceName string, kubeInformersForTargetNamespace informers.SharedInformerFactory, configInformer configinformers.SharedInformerFactory, routeInformer routeinformer.SharedInformerFactory, - configMaps corev1client.ConfigMapsGetter, route routeclient.RouteInterface, authentication configv1client.AuthenticationInterface, operatorClient v1helpers.OperatorClient, - recorder events.Recorder) factory.Controller { +func NewMetadataController(instanceName string, + kubeInformersForTargetNamespace informers.SharedInformerFactory, + configInformer configinformers.SharedInformerFactory, + routeInformer routeinformer.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, + configMaps corev1client.ConfigMapsGetter, + route routeclient.RouteInterface, + authentication configv1client.AuthenticationInterface, + operatorClient v1helpers.OperatorClient, + recorder events.Recorder, +) factory.Controller { c := &metadataController{ controllerInstanceName: factory.ControllerInstanceName(instanceName, "Metadata"), ingressLister: configInformer.Config().V1().Ingresses().Lister(), secretLister: kubeInformersForTargetNamespace.Core().V1().Secrets().Lister(), + configMapLister: kubeInformersForTargetNamespace.Core().V1().ConfigMaps().Lister(), configMaps: configMaps, route: route, authentication: authentication, operatorClient: operatorClient, + + authLister: configInformer.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } return factory.New().WithInformers( kubeInformersForTargetNamespace.Core().V1().Secrets().Informer(), configInformer.Config().V1().Authentications().Informer(), configInformer.Config().V1().Ingresses().Informer(), routeInformer.Route().V1().Routes().Informer(), + kasInformer.Informer(), ).ResyncEvery(wait.Jitter(time.Minute, 1.0)).WithSync(c.sync).ToController(c.controllerInstanceName, recorder.WithComponentSuffix("metadata-controller")) } func (c *metadataController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + if err := c.removeOperands(ctx); err != nil { + return err + } + + return common.ApplyControllerConditions(ctx, c.operatorClient, c.controllerInstanceName, knownConditionNames, nil) + } + foundConditions := []operatorv1.OperatorCondition{} foundConditions = append(foundConditions, c.handleOAuthMetadataConfigMap(ctx, syncCtx.Recorder())...) @@ -156,6 +189,21 @@ func (c *metadataController) handleAuthConfig(ctx context.Context) []operatorv1. return nil } +func (c *metadataController) removeOperands(ctx context.Context) error { + if _, err := c.configMapLister.ConfigMaps("openshift-authentication").Get("v4-0-config-system-metadata"); errors.IsNotFound(err) { + return nil + } else if err != nil { + return err + } + + err := c.configMaps.ConfigMaps("openshift-authentication").Delete(ctx, "v4-0-config-system-metadata", metav1.DeleteOptions{}) + if err != nil && !errors.IsNotFound(err) { + return err + } + + return nil +} + func getOAuthMetadata(host string) string { stubMetadata := ` { diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 640e7341b..2cded66b8 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -207,6 +207,8 @@ func prepareOauthOperator( informerFactories.kubeInformersForNamespaces.InformersFor("openshift-authentication"), informerFactories.operatorConfigInformer, informerFactories.namespacedOpenshiftAuthenticationRoutes, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.kubeClient.CoreV1(), authOperatorInput.routeClient.RouteV1().Routes("openshift-authentication"), authOperatorInput.configClient.ConfigV1().Authentications(), From 9b2ff761fd1415569e3527b343fdf52356eefba6 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Wed, 20 Nov 2024 11:32:23 +0100 Subject: [PATCH 11/23] operator: configure static resources that depend on oidc --- pkg/operator/starter.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 2cded66b8..71d3e4206 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -18,6 +18,7 @@ import ( applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" "github.com/openshift/cluster-authentication-operator/bindata" + "github.com/openshift/cluster-authentication-operator/pkg/controllers/common" "github.com/openshift/cluster-authentication-operator/pkg/controllers/configobservation/configobservercontroller" componentroutesecretsync "github.com/openshift/cluster-authentication-operator/pkg/controllers/customroute" "github.com/openshift/cluster-authentication-operator/pkg/controllers/deployment" @@ -139,21 +140,30 @@ func prepareOauthOperator( staticResourceController := staticresourcecontroller.NewStaticResourceController( "OpenshiftAuthenticationStaticResources", bindata.Asset, - []string{ + []string{ // always created, never deleted "oauth-openshift/audit-policy.yaml", "oauth-openshift/ns.yaml", "oauth-openshift/authentication-clusterrolebinding.yaml", - "oauth-openshift/cabundle.yaml", - "oauth-openshift/branding-secret.yaml", "oauth-openshift/serviceaccount.yaml", - "oauth-openshift/oauth-service.yaml", "oauth-openshift/trust_distribution_role.yaml", "oauth-openshift/trust_distribution_rolebinding.yaml", }, resourceapply.NewKubeClientHolder(authOperatorInput.kubeClient), authOperatorInput.authenticationOperatorClient, authOperatorInput.eventRecorder, - ).AddKubeInformers(informerFactories.kubeInformersForNamespaces) + ).AddKubeInformers(informerFactories.kubeInformersForNamespaces). + WithConditionalResources(bindata.Asset, + []string{ // created if external OIDC disabled, deleted otherwise + "oauth-openshift/cabundle.yaml", + "oauth-openshift/branding-secret.yaml", + "oauth-openshift/oauth-service.yaml", + }, + func() bool { + oidcAvailable, _ := common.ExternalOIDCConfigAvailable(authLister, kasLister, kasConfigMapLister) + return !oidcAvailable + }, + nil, + ) configObserver := configobservercontroller.NewConfigObserver( authOperatorInput.authenticationOperatorClient, From 7af8526217859333a8acfb0b907d8284e96b2ba6 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Fri, 15 Nov 2024 08:32:10 +0100 Subject: [PATCH 12/23] routercerts: remove operands if external OIDC config is available --- pkg/controllers/routercerts/controller.go | 26 ++++++++++++++++++- .../routercerts/controller_test.go | 10 +++++++ pkg/operator/starter.go | 3 +++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/pkg/controllers/routercerts/controller.go b/pkg/controllers/routercerts/controller.go index 4327b009b..c3fc7b190 100644 --- a/pkg/controllers/routercerts/controller.go +++ b/pkg/controllers/routercerts/controller.go @@ -15,9 +15,12 @@ import ( "k8s.io/klog/v2" operatorv1 "github.com/openshift/api/operator/v1" + configinformers "github.com/openshift/client-go/config/informers/externalversions" configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1" configv1listers "github.com/openshift/client-go/config/listers/config/v1" applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/library-go/pkg/operator/events" @@ -45,6 +48,10 @@ type routerCertsDomainValidationController struct { customSecretName string routeName string + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1listers.ConfigMapLister + systemCertPool func() (*x509.CertPool, error) // enables unit testing } @@ -57,6 +64,9 @@ func NewRouterCertsDomainValidationController( targetNSsecretInformer corev1informers.SecretInformer, machineConfigNSSecretInformer corev1informers.SecretInformer, configMapInformer corev1informers.ConfigMapInformer, + operatorConfigInformers configinformers.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer corev1informers.ConfigMapInformer, secretNamespace string, defaultSecretName string, customSecretName string, @@ -74,6 +84,10 @@ func NewRouterCertsDomainValidationController( customSecretName: customSecretName, routeName: routeName, systemCertPool: x509.SystemCertPool, + + authLister: operatorConfigInformers.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Lister(), } return factory.New(). @@ -81,7 +95,9 @@ func NewRouterCertsDomainValidationController( operatorClient.Informer(), ingressInformer.Informer(), targetNSsecretInformer.Informer(), - configMapInformer.Informer()). + configMapInformer.Informer(), + operatorConfigInformers.Config().V1().Authentications().Informer(), + kasInformer.Informer()). WithFilteredEventsInformers( factory.NamesFilter("router-certs"), machineConfigNSSecretInformer.Informer(), @@ -117,6 +133,14 @@ func (c *routerCertsDomainValidationController) sync(ctx context.Context, syncCt WithMessage(condition.Message))) }() + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + // do not remove secret "v4-0-config-system-router-certs" as the ConfigObserver controller + // monitors it and it will go degraded if missing + return nil + } + // get ingress ingress, err := c.ingressLister.Get("cluster") if err != nil { diff --git a/pkg/controllers/routercerts/controller_test.go b/pkg/controllers/routercerts/controller_test.go index 54aeb4abe..dd14fb3c5 100644 --- a/pkg/controllers/routercerts/controller_test.go +++ b/pkg/controllers/routercerts/controller_test.go @@ -237,11 +237,21 @@ func TestValidateRouterCertificates(t *testing.T) { secretsClient = fake.NewSimpleClientset() } + authIndexer := cache.NewIndexer(func(_ interface{}) (string, error) { + return "cluster", nil + }, cache.Indexers{}) + authIndexer.Add(&configv1.Authentication{ + Spec: configv1.AuthenticationSpec{ + Type: configv1.AuthenticationTypeIntegratedOAuth, + }, + }) + controller := routerCertsDomainValidationController{ operatorClient: operatorClient, ingressLister: configv1listers.NewIngressLister(ingresses), secretLister: corev1listers.NewSecretLister(secrets), configMapLister: corev1listers.NewConfigMapLister(configMapsIndexer), + authLister: configv1listers.NewAuthenticationLister(authIndexer), secretNamespace: "openshift-authentication", defaultSecretName: "v4-0-config-system-router-certs", customSecretName: "v4-0-config-system-custom-router-certs", diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 71d3e4206..a74e824b4 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -183,6 +183,9 @@ func prepareOauthOperator( informerFactories.kubeInformersForNamespaces.InformersFor("openshift-authentication").Core().V1().Secrets(), informerFactories.kubeInformersForNamespaces.InformersFor("openshift-config-managed").Core().V1().Secrets(), informerFactories.kubeInformersForNamespaces.InformersFor("openshift-config-managed").Core().V1().ConfigMaps(), + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver").Core().V1().ConfigMaps(), "openshift-authentication", "v4-0-config-system-router-certs", "v4-0-config-system-custom-router-certs", From edfb3e27a81f5c1f128397ea9d6cc203d54f96e9 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Fri, 15 Nov 2024 13:47:47 +0100 Subject: [PATCH 13/23] serviceca: remove operands if external OIDC config is available --- .../serviceca/service_ca_controller.go | 42 ++++++++++++++++++- pkg/operator/starter.go | 2 + 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/serviceca/service_ca_controller.go b/pkg/controllers/serviceca/service_ca_controller.go index 5f1725f3b..9606a46fc 100644 --- a/pkg/controllers/serviceca/service_ca_controller.go +++ b/pkg/controllers/serviceca/service_ca_controller.go @@ -18,6 +18,9 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" configinformers "github.com/openshift/client-go/config/informers/externalversions" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/v1helpers" @@ -37,18 +40,36 @@ type serviceCAController struct { controllerInstanceName string serviceLister corev1lister.ServiceLister secretLister corev1lister.SecretLister + configMapLister corev1lister.ConfigMapLister configMaps corev1client.ConfigMapsGetter operatorClient v1helpers.OperatorClient + + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1lister.ConfigMapLister } -func NewServiceCAController(instanceName string, kubeInformersForTargetNamespace informers.SharedInformerFactory, configInformer configinformers.SharedInformerFactory, configMaps corev1client.ConfigMapsGetter, - operatorClient v1helpers.OperatorClient, recorder events.Recorder) factory.Controller { +func NewServiceCAController( + instanceName string, + kubeInformersForTargetNamespace informers.SharedInformerFactory, + configInformer configinformers.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, + configMaps corev1client.ConfigMapsGetter, + operatorClient v1helpers.OperatorClient, + recorder events.Recorder, +) factory.Controller { c := &serviceCAController{ controllerInstanceName: factory.ControllerInstanceName(instanceName, "ServiceCA"), serviceLister: kubeInformersForTargetNamespace.Core().V1().Services().Lister(), secretLister: kubeInformersForTargetNamespace.Core().V1().Secrets().Lister(), + configMapLister: kubeInformersForTargetNamespace.Core().V1().ConfigMaps().Lister(), configMaps: configMaps, operatorClient: operatorClient, + + authLister: configInformer.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } return factory.New().WithInformers( kubeInformersForTargetNamespace.Core().V1().Secrets().Informer(), @@ -56,6 +77,7 @@ func NewServiceCAController(instanceName string, kubeInformersForTargetNamespace kubeInformersForTargetNamespace.Core().V1().ConfigMaps().Informer(), configInformer.Config().V1().Authentications().Informer(), configInformer.Config().V1().Ingresses().Informer(), + kasInformer.Informer(), ).ResyncEvery( wait.Jitter(time.Minute, 1.0), ).WithSync( @@ -66,6 +88,22 @@ func NewServiceCAController(instanceName string, kubeInformersForTargetNamespace } func (c *serviceCAController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + if _, err := c.configMapLister.ConfigMaps("openshift-authentication").Get("v4-0-config-system-service-ca"); errors.IsNotFound(err) { + return nil + } else if err != nil { + return err + } + + if err := c.configMaps.ConfigMaps("openshift-authentication").Delete(ctx, "v4-0-config-system-service-ca", metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) { + return err + } + + return common.ApplyControllerConditions(ctx, c.operatorClient, c.controllerInstanceName, knownConditionNames, nil) + } + foundConditions := []operatorv1.OperatorCondition{} _, serviceConditions := common.GetOAuthServerService(c.serviceLister, "OAuthService") diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index a74e824b4..af3a4d2e8 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -233,6 +233,8 @@ func prepareOauthOperator( "openshift-authentication", informerFactories.kubeInformersForNamespaces.InformersFor("openshift-authentication"), informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.kubeClient.CoreV1(), authOperatorInput.authenticationOperatorClient, authOperatorInput.eventRecorder, From a3d37a377653eeab4500b96a4ccabbb72b9d62ee Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Fri, 15 Nov 2024 14:03:53 +0100 Subject: [PATCH 14/23] payload: remove operands if external OIDC config is available --- .../payload/payload_config_controller.go | 40 +++++++++++++++++++ pkg/operator/starter.go | 3 ++ 2 files changed, 43 insertions(+) diff --git a/pkg/controllers/payload/payload_config_controller.go b/pkg/controllers/payload/payload_config_controller.go index bfa72b374..6f862da95 100644 --- a/pkg/controllers/payload/payload_config_controller.go +++ b/pkg/controllers/payload/payload_config_controller.go @@ -10,6 +10,7 @@ import ( "time" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -25,6 +26,10 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" osinv1 "github.com/openshift/api/osin/v1" routev1 "github.com/openshift/api/route/v1" + configinformers "github.com/openshift/client-go/config/informers/externalversions" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" routeinformer "github.com/openshift/client-go/route/informers/externalversions/route/v1" routev1lister "github.com/openshift/client-go/route/listers/route/v1" "github.com/openshift/library-go/pkg/controller/factory" @@ -66,11 +71,18 @@ type payloadConfigController struct { configMaps corev1client.ConfigMapsGetter secrets corev1client.SecretsGetter operatorClient v1helpers.OperatorClient + + authLister configv1listers.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1lister.ConfigMapLister } func NewPayloadConfigController( instanceName string, kubeInformersForTargetNamespace informers.SharedInformerFactory, + operatorConfigInformers configinformers.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, secrets corev1client.SecretsGetter, configMaps corev1client.ConfigMapsGetter, operatorClient v1helpers.OperatorClient, @@ -83,6 +95,10 @@ func NewPayloadConfigController( secrets: secrets, configMaps: configMaps, operatorClient: operatorClient, + + authLister: operatorConfigInformers.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } return factory.New().WithInformers( kubeInformersForTargetNamespace.Core().V1().Secrets().Informer(), @@ -90,6 +106,8 @@ func NewPayloadConfigController( kubeInformersForTargetNamespace.Core().V1().ConfigMaps().Informer(), routeInformer.Informer(), operatorClient.Informer(), + operatorConfigInformers.Config().V1().Authentications().Informer(), + kasInformer.Informer(), ).ResyncEvery(wait.Jitter(time.Minute, 1.0)).WithSync(c.sync).ToController(c.controllerInstanceName, recorder.WithComponentSuffix("payload-config-controller")) } @@ -138,6 +156,16 @@ func (c *payloadConfigController) getSessionSecret(ctx context.Context, recorder } func (c *payloadConfigController) sync(ctx context.Context, syncContext factory.SyncContext) error { + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + if err := c.removeOperands(ctx); err != nil { + return err + } + + return common.ApplyControllerConditions(ctx, c.operatorClient, c.controllerInstanceName, knownConditionNames, nil) + } + foundConditions := []operatorv1.OperatorCondition{} foundConditions = append(foundConditions, c.getSessionSecret(ctx, syncContext.Recorder())...) @@ -159,6 +187,18 @@ func (c *payloadConfigController) sync(ctx context.Context, syncContext factory. return common.ApplyControllerConditions(ctx, c.operatorClient, c.controllerInstanceName, knownConditionNames, foundConditions) } +func (c *payloadConfigController) removeOperands(ctx context.Context) error { + if err := c.secrets.Secrets("openshift-authentication").Delete(ctx, "v4-0-config-system-session", metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) { + return err + } + + if err := c.configMaps.ConfigMaps("openshift-authentication").Delete(ctx, "v4-0-config-system-cliconfig", metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) { + return err + } + + return nil +} + func (c *payloadConfigController) handleOAuthConfig(ctx context.Context, operatorSpec *operatorv1.OperatorSpec, route *routev1.Route, service *corev1.Service, recorder events.Recorder) []operatorv1.OperatorCondition { ca := "/var/config/system/configmaps/v4-0-config-system-service-ca/service-ca.crt" cliConfig := &osinv1.OsinServerConfig{ diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index af3a4d2e8..eed3dd8d1 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -243,6 +243,9 @@ func prepareOauthOperator( payloadConfigController := payload.NewPayloadConfigController( "openshift-authentication", informerFactories.kubeInformersForNamespaces.InformersFor("openshift-authentication"), + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.kubeClient.CoreV1(), authOperatorInput.kubeClient.CoreV1(), authOperatorInput.authenticationOperatorClient, From 3725212fc85ee280a469ff20415a704cfdf0a10c Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Mon, 18 Nov 2024 13:13:54 +0100 Subject: [PATCH 15/23] customroute: remove operands and clear custom route ingress status if external OIDC config is available --- .../customroute/custom_route_controller.go | 70 ++++++++++++++++++- pkg/operator/starter.go | 3 + 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/customroute/custom_route_controller.go b/pkg/controllers/customroute/custom_route_controller.go index b9271b4e6..95ebe173d 100644 --- a/pkg/controllers/customroute/custom_route_controller.go +++ b/pkg/controllers/customroute/custom_route_controller.go @@ -12,6 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/informers" corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/klog/v2" @@ -19,8 +20,11 @@ import ( routev1 "github.com/openshift/api/route/v1" applyconfigv1 "github.com/openshift/client-go/config/applyconfigurations/config/v1" configsetterv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" - configinformers "github.com/openshift/client-go/config/informers/externalversions/config/v1" + configinformers "github.com/openshift/client-go/config/informers/externalversions" + configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1" configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" routeclient "github.com/openshift/client-go/route/clientset/versioned/typed/route/v1" routeinformer "github.com/openshift/client-go/route/informers/externalversions/route/v1" routev1lister "github.com/openshift/client-go/route/listers/route/v1" @@ -51,6 +55,10 @@ type customRouteController struct { secretLister corev1listers.SecretLister resourceSyncer resourcesynccontroller.ResourceSyncer operatorClient v1helpers.OperatorClient + + authLister configlistersv1.AuthenticationLister + kasLister operatorv1listers.KubeAPIServerLister + kasConfigMapLister corev1listers.ConfigMapLister } func NewCustomRouteController( @@ -58,11 +66,14 @@ func NewCustomRouteController( componentRouteName string, destSecretNamespace string, destSecretName string, - ingressInformer configinformers.IngressInformer, + ingressInformer configinformersv1.IngressInformer, ingressClient configsetterv1.IngressInterface, routeInformer routeinformer.RouteInformer, routeClient routeclient.RouteInterface, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, + operatorConfigInformers configinformers.SharedInformerFactory, + kasInformer operatorv1informers.KubeAPIServerInformer, + kasConfigMapInformer informers.SharedInformerFactory, operatorClient v1helpers.OperatorClient, eventRecorder events.Recorder, resourceSyncer resourcesynccontroller.ResourceSyncer, @@ -83,6 +94,10 @@ func NewCustomRouteController( secretLister: kubeInformersForNamespaces.SecretLister(), operatorClient: operatorClient, resourceSyncer: resourceSyncer, + + authLister: operatorConfigInformers.Config().V1().Authentications().Lister(), + kasLister: kasInformer.Lister(), + kasConfigMapLister: kasConfigMapInformer.Core().V1().ConfigMaps().Lister(), } return factory.New(). @@ -91,6 +106,8 @@ func NewCustomRouteController( routeInformer.Informer(), kubeInformersForNamespaces.InformersFor("openshift-config").Core().V1().Secrets().Informer(), kubeInformersForNamespaces.InformersFor("openshift-authentication").Core().V1().Secrets().Informer(), + operatorConfigInformers.Config().V1().Authentications().Informer(), + kasInformer.Informer(), ). WithSyncDegradedOnError(operatorClient). WithSync(controller.sync). @@ -116,6 +133,12 @@ func (c *customRouteController) sync(ctx context.Context, syncCtx factory.SyncCo return fmt.Errorf("custom route configuration failed verification: %v", errors) } + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + return c.removeOperands(ctx, ingressConfigCopy, secretName) + } + // create or modify the existing route if err = c.applyRoute(ctx, expectedRoute); err != nil { return err @@ -289,3 +312,46 @@ func (c *customRouteController) getFieldManager() string { // TODO find a way to get the client name and combine it with the controller name automatically return "AuthenticationCustomRouteController" } + +func (c *customRouteController) removeOperands(ctx context.Context, ingressConfig *configv1.Ingress, secretName string) error { + if _, err := c.routeLister.Routes(c.componentRoute.Namespace).Get(c.componentRoute.Name); err != nil && !errors.IsNotFound(err) { + return err + } else if !errors.IsNotFound(err) { + if err := c.routeClient.Delete(ctx, c.componentRoute.Name, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) { + return err + } + } + + ingressStatus, err := applyconfigv1.ExtractIngressStatus(ingressConfig, c.getFieldManager()) + if err != nil { + return err + } + + if ingressStatus != nil && ingressStatus.Status != nil { + componentRoutes := make([]applyconfigv1.ComponentRouteStatusApplyConfiguration, 0) + routeFound := false + for _, cr := range ingressStatus.Status.ComponentRoutes { + if *cr.Name == c.componentRoute.Name && *cr.Namespace == c.componentRoute.Namespace { + routeFound = true + continue + } + + componentRoutes = append(componentRoutes, cr) + } + + if routeFound { + ingressStatus.Status.ComponentRoutes = componentRoutes + ingress := applyconfigv1.Ingress(ingressConfig.Name).WithStatus(ingressStatus.Status) + if _, err := c.ingressClient.ApplyStatus(ctx, ingress, c.forceApply()); err != nil { + return err + } + } + } + + // delete secret by syncing an empty source + if err := c.syncSecret(""); err != nil { + return err + } + + return nil +} diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index eed3dd8d1..c512d7a24 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -359,6 +359,9 @@ func prepareOauthOperator( informerFactories.namespacedOpenshiftAuthenticationRoutes.Route().V1().Routes(), authOperatorInput.routeClient.RouteV1().Routes("openshift-authentication"), informerFactories.kubeInformersForNamespaces, + informerFactories.operatorConfigInformer, + informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), + informerFactories.kubeInformersForNamespaces.InformersFor("openshift-kube-apiserver"), authOperatorInput.authenticationOperatorClient, authOperatorInput.eventRecorder, resourceSyncController, From 3d9fb80e3a9d047111ec85cc686b156f3e1c7b66 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Wed, 20 Nov 2024 11:30:30 +0100 Subject: [PATCH 16/23] operator: enable or disable API services depending on whether OIDC is enabled --- pkg/operator/starter.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index c512d7a24..d8ee95f76 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -584,9 +584,7 @@ func prepareOauthAPIServerOperator( ).WithAPIServiceController( "openshift-apiserver", "openshift-oauth-apiserver", - func() ([]*apiregistrationv1.APIService, []*apiregistrationv1.APIService, error) { - return apiServices(), nil, nil - }, + apiServicesFuncWrapper(authLister, kasLister, kasConfigMapLister), informerFactories.apiregistrationInformers, authOperatorInput.apiregistrationv1Client.ApiregistrationV1(), informerFactories.kubeInformersForNamespaces, @@ -835,6 +833,20 @@ func extractOperatorStatus(obj *unstructured.Unstructured, fieldManager string) return &ret.Status.OperatorStatusApplyConfiguration, nil } +func apiServicesFuncWrapper(authLister configv1listers.AuthenticationLister, kasLister operatorv1listers.KubeAPIServerLister, kasConfigMapLister corev1listers.ConfigMapLister) func() ([]*apiregistrationv1.APIService, []*apiregistrationv1.APIService, error) { + return func() ([]*apiregistrationv1.APIService, []*apiregistrationv1.APIService, error) { + apiServices := apiServices() + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(authLister, kasLister, kasConfigMapLister); err != nil { + return nil, nil, err + } else if oidcAvailable { + // return apiServices as disabled + return nil, apiServices, nil + } + + return apiServices, nil, nil + } +} + func countNodesFuncWrapper(nodeLister corev1listers.NodeLister, authLister configv1listers.AuthenticationLister, kasLister operatorv1listers.KubeAPIServerLister, kasConfigMapLister corev1listers.ConfigMapLister) func(nodeSelector map[string]string) (*int32, error) { return func(nodeSelector map[string]string) (*int32, error) { if oidcAvailable, err := common.ExternalOIDCConfigAvailable(authLister, kasLister, kasConfigMapLister); err != nil { From 15d18833835a0547514290b08e27df0452965fa9 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Fri, 22 Nov 2024 10:54:13 +0100 Subject: [PATCH 17/23] controllers: add switchable informer controller --- ...oauthclientsswitchedinformer_controller.go | 129 +++++++++++++++++ ...clientsswitchedinformer_controller_test.go | 130 ++++++++++++++++++ 2 files changed, 259 insertions(+) create mode 100644 pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller.go create mode 100644 pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller_test.go diff --git a/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller.go b/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller.go new file mode 100644 index 000000000..8d35507ed --- /dev/null +++ b/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller.go @@ -0,0 +1,129 @@ +package oauthclientsswitchedinformer + +import ( + "context" + "time" + + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + + "k8s.io/client-go/tools/cache" + "k8s.io/klog/v2" +) + +// InformerWithSwitch is a controller that can start and stop an informer based on +// a condition func (shouldStopFn), that returns a bool and an error. If an error +// is returned, then the controller's sync will fail with that error. If no error +// is returned, then the controller stops/starts the informer based on the bool value +// (true means stop). +type InformerWithSwitch struct { + delegateInformer cache.SharedIndexInformer + switchController factory.Controller + shouldStopFn func() (bool, error) + parentCtx context.Context + runCtx context.Context + stopFunc func() +} + +type alwaysSyncedInformer struct { + isRunning func() bool + cache.SharedIndexInformer +} + +// HasSynced returns true when the informer's caches have synced, false otherwise. +// Since the SwitchedInformer can be stopped, waiting for its cache to sync can lead to +// timeouts, as a stopped informer will never sync. We override the HasSynced() +// method to always return true when stopped; clients should explicitly call cache.WaitForCacheSync. +func (s *alwaysSyncedInformer) HasSynced() bool { + if s.isRunning() { + return s.SharedIndexInformer.HasSynced() + } + return true +} + +func NewSwitchedInformer( + name string, + ctx context.Context, + shouldStopFn func() (bool, error), + delegateInformer cache.SharedIndexInformer, + resync time.Duration, + informers []factory.Informer, + recorder events.Recorder, +) *InformerWithSwitch { + + s := &InformerWithSwitch{ + parentCtx: ctx, + delegateInformer: delegateInformer, + shouldStopFn: shouldStopFn, + } + + controllerFactory := factory.New().WithSync(s.sync) + + if len(informers) > 0 { + controllerFactory.WithInformers(informers...) + } + + if resync > 0 { + controllerFactory.ResyncEvery(resync) + } + + s.switchController = controllerFactory.ToController(name, recorder) + return s +} + +func (s *InformerWithSwitch) Controller() factory.Controller { + return s.switchController +} + +func (s *InformerWithSwitch) Informer() cache.SharedIndexInformer { + return &alwaysSyncedInformer{ + isRunning: func() bool { return s.runCtx != nil }, + SharedIndexInformer: s.delegateInformer, + } +} + +func (s *InformerWithSwitch) Start(stopCh <-chan struct{}) { + go s.switchController.Run(s.parentCtx, 1) + go func() { + <-stopCh + s.stop() + }() +} + +func (s *InformerWithSwitch) ensureRunning() { + if s.runCtx != nil { + return + } + + klog.Infof("%s delegate informer starting", s.switchController.Name()) + s.runCtx, s.stopFunc = context.WithCancel(s.parentCtx) + go s.delegateInformer.Run(s.runCtx.Done()) +} + +func (s *InformerWithSwitch) stop() { + if s.runCtx == nil { + return + } + + klog.Infof("%s delegate informer stopping", s.switchController.Name()) + s.stopFunc() + s.runCtx = nil + s.stopFunc = nil +} + +func (s *InformerWithSwitch) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if s.shouldStopFn != nil { + shouldStop, err := s.shouldStopFn() + if err != nil { + return err + } + + if shouldStop { + s.stop() + return nil + } + } + + s.ensureRunning() + return nil +} diff --git a/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller_test.go b/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller_test.go new file mode 100644 index 000000000..36f3c84fc --- /dev/null +++ b/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller_test.go @@ -0,0 +1,130 @@ +package oauthclientsswitchedinformer + +import ( + "context" + "fmt" + "testing" + "time" + + oauthv1 "github.com/openshift/api/oauth/v1" + fakeoauthclient "github.com/openshift/client-go/oauth/clientset/versioned/fake" + oauthinformers "github.com/openshift/client-go/oauth/informers/externalversions" + "github.com/openshift/library-go/pkg/operator/events" + "k8s.io/apimachinery/pkg/util/wait" +) + +func TestSync(t *testing.T) { + testCtx, testCancel := context.WithCancel(context.TODO()) + defer testCancel() + + testOAuthClient := &oauthv1.OAuthClient{} + testClient := fakeoauthclient.NewSimpleClientset(testOAuthClient) + testInformer := oauthinformers.NewSharedInformerFactory(testClient, 0).Oauth().V1().OAuthClients() + + var makeItStop bool + var shouldStopFnErr error + shouldStopFn := func() (bool, error) { + return makeItStop, shouldStopFnErr + } + + informerSwitch := NewSwitchedInformer( + "TestInformerWithSwitchController", + testCtx, + shouldStopFn, + testInformer.Informer(), + 0, + nil, + events.NewInMemoryRecorder("oauthclientscontroller_test"), + ) + + t.Run("start informer", func(tt *testing.T) { + makeItStop = false + shouldStopFnErr = nil + err := informerSwitch.sync(testCtx, nil) + if err != nil { + tt.Errorf("unexpected sync error: %v", err) + } + waitForInformerSynced(tt, testCtx, informerSwitch) + + // informer should be running + + if informerSwitch.runCtx == nil { + tt.Error("EnsureRunning: runCtx is nil when it should be non-nil") + } + + if informerSwitch.stopFunc == nil { + tt.Error("EnsureRunning: stopFunc is nil when it should be non-nil") + } + + if informerSwitch.Informer().IsStopped() { + tt.Error("EnsureRunning: informer is stopped when it should be started") + } + }) + + t.Run("stop informer with error", func(tt *testing.T) { + makeItStop = true + shouldStopFnErr = fmt.Errorf("stop fails") + err := informerSwitch.sync(testCtx, nil) + if err == nil { + tt.Errorf("got no error while expecting one") + } + waitForInformerSynced(tt, testCtx, informerSwitch) + + // informer should still be running + + if informerSwitch.runCtx == nil { + tt.Error("EnsureRunning: runCtx is nil when it should be non-nil") + } + + if informerSwitch.stopFunc == nil { + tt.Error("EnsureRunning: stopFunc is nil when it should be non-nil") + } + + if informerSwitch.Informer().IsStopped() { + tt.Error("EnsureRunning: informer is stopped when it should be started") + } + + }) + + t.Run("stop informer without error", func(tt *testing.T) { + makeItStop = true + shouldStopFnErr = nil + err := informerSwitch.sync(testCtx, nil) + if err != nil { + tt.Errorf("unexpected sync error: %v", err) + } + waitForInformerStopped(tt, testCtx, informerSwitch) + + // informer should stop + + if informerSwitch.runCtx != nil { + tt.Error("Stop: runCtx is not nil when it should be nil") + } + + if informerSwitch.stopFunc != nil { + tt.Error("Stop: stopFunc is not nil when it should be nil") + } + + if !informerSwitch.Informer().IsStopped() { + tt.Error("Stop: informer is started when it should be stopped") + } + }) +} + +func waitForInformerSynced(t *testing.T, ctx context.Context, informerSwitch *InformerWithSwitch) { + err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 1*time.Second, true, func(ctx context.Context) (done bool, err error) { + return informerSwitch.Informer().HasSynced(), nil + }) + if err != nil { + t.Fatalf("unexpected error while waiting for informer to sync: %v", err) + } +} + +func waitForInformerStopped(t *testing.T, ctx context.Context, informerSwitch *InformerWithSwitch) { + err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 1*time.Second, true, func(ctx context.Context) (done bool, err error) { + return informerSwitch.Informer().IsStopped(), nil + }) + if err != nil { + t.Fatalf("unexpected error while waiting for informer to stop: %v", err) + } +} From 5198aeab3c711258c969e3a41d168927c63e24ca Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Fri, 22 Nov 2024 10:59:25 +0100 Subject: [PATCH 18/23] oauthclientscontroller: use a switched informer for oauthclients So that it can be stopped when auth type is OIDC. --- .../oauthclientscontroller.go | 27 ++++++++++++------- .../oauthclientscontroller_test.go | 25 +++++++++++++---- pkg/operator/replacement_starter.go | 4 --- pkg/operator/starter.go | 22 ++++++++++++++- 4 files changed, 59 insertions(+), 19 deletions(-) diff --git a/pkg/controllers/oauthclientscontroller/oauthclientscontroller.go b/pkg/controllers/oauthclientscontroller/oauthclientscontroller.go index f589b06d1..01159f175 100644 --- a/pkg/controllers/oauthclientscontroller/oauthclientscontroller.go +++ b/pkg/controllers/oauthclientscontroller/oauthclientscontroller.go @@ -14,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/informers" corev1listers "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/retry" configv1 "github.com/openshift/api/config/v1" @@ -21,7 +22,6 @@ import ( configinformers "github.com/openshift/client-go/config/informers/externalversions" configv1listers "github.com/openshift/client-go/config/listers/config/v1" oauthclient "github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1" - oauthinformers "github.com/openshift/client-go/oauth/informers/externalversions" oauthv1listers "github.com/openshift/client-go/oauth/listers/oauth/v1" operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" @@ -35,14 +35,16 @@ import ( "github.com/openshift/cluster-authentication-operator/pkg/controllers/common" "github.com/openshift/cluster-authentication-operator/pkg/controllers/customroute" + "github.com/openshift/cluster-authentication-operator/pkg/controllers/oauthclientsswitchedinformer" ) type oauthsClientsController struct { oauthClientClient oauthclient.OAuthClientInterface - oauthClientLister oauthv1listers.OAuthClientLister - routeLister routev1listers.RouteLister - ingressLister configv1listers.IngressLister + oauthClientInformer cache.SharedIndexInformer + oauthClientLister oauthv1listers.OAuthClientLister + routeLister routev1listers.RouteLister + ingressLister configv1listers.IngressLister authLister configv1listers.AuthenticationLister kasLister operatorv1listers.KubeAPIServerLister @@ -52,7 +54,7 @@ type oauthsClientsController struct { func NewOAuthClientsController( operatorClient v1helpers.OperatorClient, oauthsClientClient oauthclient.OAuthClientInterface, - oauthInformers oauthinformers.SharedInformerFactory, + oauthClientsSwitchedInformer *oauthclientsswitchedinformer.InformerWithSwitch, routeInformers routeinformers.SharedInformerFactory, operatorConfigInformers configinformers.SharedInformerFactory, kasInformer operatorv1informers.KubeAPIServerInformer, @@ -62,9 +64,10 @@ func NewOAuthClientsController( c := &oauthsClientsController{ oauthClientClient: oauthsClientClient, - oauthClientLister: oauthInformers.Oauth().V1().OAuthClients().Lister(), - routeLister: routeInformers.Route().V1().Routes().Lister(), - ingressLister: operatorConfigInformers.Config().V1().Ingresses().Lister(), + oauthClientInformer: oauthClientsSwitchedInformer.Informer(), + oauthClientLister: oauthv1listers.NewOAuthClientLister(oauthClientsSwitchedInformer.Informer().GetIndexer()), + routeLister: routeInformers.Route().V1().Routes().Lister(), + ingressLister: operatorConfigInformers.Config().V1().Ingresses().Lister(), authLister: operatorConfigInformers.Config().V1().Authentications().Lister(), kasLister: kasInformer.Lister(), @@ -76,7 +79,7 @@ func NewOAuthClientsController( WithSyncDegradedOnError(operatorClient). WithFilteredEventsInformers( factory.NamesFilter("openshift-browser-client", "openshift-challenging-client", "openshift-cli-client"), - oauthInformers.Oauth().V1().OAuthClients().Informer(), + oauthClientsSwitchedInformer.Informer(), ). WithFilteredEventsInformers( factory.NamesFilter("oauth-openshift"), @@ -114,6 +117,12 @@ func (c *oauthsClientsController) sync(ctx context.Context, syncCtx factory.Sync return err } + waitCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + if !cache.WaitForCacheSync(waitCtx.Done(), c.oauthClientInformer.HasSynced) { + return fmt.Errorf("timed out waiting for OAuthClients informer cache sync") + } + return c.ensureBootstrappedOAuthClients(ctx, "https://"+routeHost) } diff --git a/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go b/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go index 48bd87a33..667fe6584 100644 --- a/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go +++ b/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "fmt" "testing" + "time" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -20,8 +21,10 @@ import ( routev1 "github.com/openshift/api/route/v1" configv1listers "github.com/openshift/client-go/config/listers/config/v1" fakeoauthclient "github.com/openshift/client-go/oauth/clientset/versioned/fake" + oauthinformers "github.com/openshift/client-go/oauth/informers/externalversions" oauthv1listers "github.com/openshift/client-go/oauth/listers/oauth/v1" routev1listers "github.com/openshift/client-go/route/listers/route/v1" + "github.com/openshift/cluster-authentication-operator/pkg/controllers/oauthclientsswitchedinformer" "github.com/openshift/library-go/pkg/oauth/oauthdiscovery" "github.com/openshift/library-go/pkg/operator/events" ) @@ -118,12 +121,24 @@ func newAuthLister(t *testing.T) configv1listers.AuthenticationLister { } func newTestOAuthsClientsController(t *testing.T) *oauthsClientsController { + oauthClientset := fakeoauthclient.NewSimpleClientset() + switchedInformer := oauthclientsswitchedinformer.NewSwitchedInformer( + "TestOAuthClientsInformerWithSwitchController", + context.TODO(), + func() (bool, error) { return false, nil }, + oauthinformers.NewSharedInformerFactoryWithOptions(oauthClientset, 1*time.Minute).Oauth().V1().OAuthClients().Informer(), + 0, + nil, + events.NewInMemoryRecorder("oauthclientscontroller_test"), + ) + return &oauthsClientsController{ - oauthClientClient: fakeoauthclient.NewSimpleClientset().OauthV1().OAuthClients(), - oauthClientLister: oauthv1listers.NewOAuthClientLister(cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})), - routeLister: newRouteLister(t, defaultRoute), - ingressLister: newIngressLister(t, defaultIngress), - authLister: newAuthLister(t), + oauthClientInformer: switchedInformer.Informer(), + oauthClientClient: oauthClientset.OauthV1().OAuthClients(), + oauthClientLister: oauthv1listers.NewOAuthClientLister(cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})), + routeLister: newRouteLister(t, defaultRoute), + ingressLister: newIngressLister(t, defaultIngress), + authLister: newAuthLister(t), } } diff --git a/pkg/operator/replacement_starter.go b/pkg/operator/replacement_starter.go index b8f4af51b..644ba5583 100644 --- a/pkg/operator/replacement_starter.go +++ b/pkg/operator/replacement_starter.go @@ -19,7 +19,6 @@ import ( 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" - oauthinformers "github.com/openshift/client-go/oauth/informers/externalversions" operatorclient "github.com/openshift/client-go/operator/clientset/versioned" operatorinformer "github.com/openshift/client-go/operator/informers/externalversions" routeclient "github.com/openshift/client-go/route/clientset/versioned" @@ -214,7 +213,6 @@ type authenticationOperatorInformerFactories struct { kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces operatorConfigInformer configinformer.SharedInformerFactory operatorInformer operatorinformer.SharedInformerFactory - oauthInformers oauthinformers.SharedInformerFactory apiregistrationInformers apiregistrationinformers.SharedInformerFactory migrationInformer migrationv1alpha1informer.SharedInformerFactory // TODO remove @@ -240,7 +238,6 @@ func newInformerFactories(authOperatorInput *authenticationOperatorInput) authen ), operatorConfigInformer: configinformer.NewSharedInformerFactoryWithOptions(authOperatorInput.configClient, resync), operatorInformer: operatorinformer.NewSharedInformerFactory(authOperatorInput.operatorClient, 24*time.Hour), - oauthInformers: oauthinformers.NewSharedInformerFactory(authOperatorInput.oauthClient, resync), apiregistrationInformers: apiregistrationinformers.NewSharedInformerFactory(authOperatorInput.apiregistrationv1Client, 10*time.Minute), migrationInformer: migrationv1alpha1informer.NewSharedInformerFactory(authOperatorInput.migrationClient, time.Minute*30), kubeInformers: kubeinformers.NewSharedInformerFactory(authOperatorInput.kubeClient, resync), @@ -257,7 +254,6 @@ func (a authenticationOperatorInformerFactories) simplifiedInformerFactories() [ libraryapplyconfiguration.GeneratedNamespacedInformerFactoryAdapter(a.kubeInformersForNamespaces), libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.operatorInformer), libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.operatorConfigInformer), - libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.oauthInformers), libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.apiregistrationInformers), libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.migrationInformer), libraryapplyconfiguration.GeneratedInformerFactoryAdapter(a.kubeInformers), diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index d8ee95f76..21912b288 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -15,6 +15,7 @@ import ( operatorv1 "github.com/openshift/api/operator/v1" routev1 "github.com/openshift/api/route/v1" configv1listers "github.com/openshift/client-go/config/listers/config/v1" + oauthinformers "github.com/openshift/client-go/oauth/informers/externalversions" applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" operatorv1listers "github.com/openshift/client-go/operator/listers/operator/v1" "github.com/openshift/cluster-authentication-operator/bindata" @@ -26,6 +27,7 @@ import ( "github.com/openshift/cluster-authentication-operator/pkg/controllers/ingressstate" "github.com/openshift/cluster-authentication-operator/pkg/controllers/metadata" "github.com/openshift/cluster-authentication-operator/pkg/controllers/oauthclientscontroller" + "github.com/openshift/cluster-authentication-operator/pkg/controllers/oauthclientsswitchedinformer" "github.com/openshift/cluster-authentication-operator/pkg/controllers/oauthendpoints" "github.com/openshift/cluster-authentication-operator/pkg/controllers/payload" "github.com/openshift/cluster-authentication-operator/pkg/controllers/proxyconfig" @@ -39,6 +41,7 @@ import ( "github.com/openshift/cluster-authentication-operator/pkg/operator/workload" "github.com/openshift/library-go/pkg/authentication/bootstrapauthenticator" "github.com/openshift/library-go/pkg/controller/controllercmd" + "github.com/openshift/library-go/pkg/controller/factory" workloadcontroller "github.com/openshift/library-go/pkg/operator/apiserver/controller/workload" apiservercontrollerset "github.com/openshift/library-go/pkg/operator/apiserver/controllerset" "github.com/openshift/library-go/pkg/operator/certrotation" @@ -253,10 +256,25 @@ func prepareOauthOperator( authOperatorInput.eventRecorder, ) + oauthClientsSwitchedInformer := oauthclientsswitchedinformer.NewSwitchedInformer( + "OAuthClientsInformerWithSwitchController", + ctx, + func() (bool, error) { + return common.ExternalOIDCConfigAvailable(authLister, kasLister, kasConfigMapLister) + }, + oauthinformers.NewSharedInformerFactoryWithOptions(authOperatorInput.oauthClient, 1*time.Minute).Oauth().V1().OAuthClients().Informer(), + 0, + []factory.Informer{ + informerFactories.operatorInformer.Operator().V1().KubeAPIServers().Informer(), + informerFactories.operatorConfigInformer.Config().V1().Authentications().Informer(), + }, + authOperatorInput.eventRecorder, + ) + oauthClientsController := oauthclientscontroller.NewOAuthClientsController( authOperatorInput.authenticationOperatorClient, authOperatorInput.oauthClient.OauthV1().OAuthClients(), - informerFactories.oauthInformers, + oauthClientsSwitchedInformer, informerFactories.namespacedOpenshiftAuthenticationRoutes, informerFactories.operatorConfigInformer, informerFactories.operatorInformer.Operator().V1().KubeAPIServers(), @@ -383,6 +401,7 @@ func prepareOauthOperator( libraryapplyconfiguration.AdaptSyncFn(authOperatorInput.eventRecorder, "TODO-deploymentController", deploymentController.Sync), libraryapplyconfiguration.AdaptSyncFn(authOperatorInput.eventRecorder, "TODO-managementStateController", managementStateController.Sync), libraryapplyconfiguration.AdaptSyncFn(authOperatorInput.eventRecorder, "TODO-metadataController", metadataController.Sync), + libraryapplyconfiguration.AdaptSyncFn(authOperatorInput.eventRecorder, "TODO-oauthClientsSwitchedInformerController", oauthClientsSwitchedInformer.Controller().Sync), libraryapplyconfiguration.AdaptSyncFn(authOperatorInput.eventRecorder, "TODO-oauthClientsController", oauthClientsController.Sync), libraryapplyconfiguration.AdaptSyncFn(authOperatorInput.eventRecorder, "TODO-payloadConfigController", payloadConfigController.Sync), libraryapplyconfiguration.AdaptSyncFn(authOperatorInput.eventRecorder, "TODO-routerCertsController", routerCertsController.Sync), @@ -405,6 +424,7 @@ func prepareOauthOperator( libraryapplyconfiguration.AdaptRunFn(deploymentController.Run), libraryapplyconfiguration.AdaptRunFn(managementStateController.Run), libraryapplyconfiguration.AdaptRunFn(metadataController.Run), + libraryapplyconfiguration.AdaptRunFn(oauthClientsSwitchedInformer.Controller().Run), libraryapplyconfiguration.AdaptRunFn(oauthClientsController.Run), libraryapplyconfiguration.AdaptRunFn(payloadConfigController.Run), libraryapplyconfiguration.AdaptRunFn(routerCertsController.Run), From 09d5d7718987536e62f68a2d78e989dd1d14efcd Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Fri, 29 Nov 2024 13:18:43 +0100 Subject: [PATCH 19/23] fixup! readiness: disable checks when external OIDC config is available --- pkg/controllers/readiness/wellknown_ready_controller.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pkg/controllers/readiness/wellknown_ready_controller.go b/pkg/controllers/readiness/wellknown_ready_controller.go index 2d286c695..1904bbcdd 100644 --- a/pkg/controllers/readiness/wellknown_ready_controller.go +++ b/pkg/controllers/readiness/wellknown_ready_controller.go @@ -150,12 +150,8 @@ func (c *wellKnownReadyController) sync(ctx context.Context, controllerContext f if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { return err } else if oidcAvailable { - available = available. - WithStatus(operatorv1.ConditionFalse). - WithMessage("well-known endpoint checks skipped."). - WithReason("NotAvailableForOIDC") - progressing = progressing. - WithStatus(operatorv1.ConditionFalse) + available = available.WithStatus(operatorv1.ConditionTrue) + progressing = progressing.WithStatus(operatorv1.ConditionFalse) return nil } From 4200edde03363dde52a1473266ce0f0b22be825d Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Fri, 29 Nov 2024 13:20:20 +0100 Subject: [PATCH 20/23] fixup! customroute: remove operands and clear custom route ingress status if external OIDC config is available --- .../customroute/custom_route_controller.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/controllers/customroute/custom_route_controller.go b/pkg/controllers/customroute/custom_route_controller.go index 95ebe173d..1d935aff0 100644 --- a/pkg/controllers/customroute/custom_route_controller.go +++ b/pkg/controllers/customroute/custom_route_controller.go @@ -123,6 +123,12 @@ func (c *customRouteController) sync(ctx context.Context, syncCtx factory.SyncCo ingressConfigCopy := ingressConfig.DeepCopy() + if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { + return err + } else if oidcAvailable { + return c.removeOperands(ctx, ingressConfigCopy) + } + // configure the expected route expectedRoute, secretName, errors := c.getOAuthRouteAndSecretName(ingressConfigCopy) if errors != nil { @@ -133,12 +139,6 @@ func (c *customRouteController) sync(ctx context.Context, syncCtx factory.SyncCo return fmt.Errorf("custom route configuration failed verification: %v", errors) } - if oidcAvailable, err := common.ExternalOIDCConfigAvailable(c.authLister, c.kasLister, c.kasConfigMapLister); err != nil { - return err - } else if oidcAvailable { - return c.removeOperands(ctx, ingressConfigCopy, secretName) - } - // create or modify the existing route if err = c.applyRoute(ctx, expectedRoute); err != nil { return err @@ -313,7 +313,7 @@ func (c *customRouteController) getFieldManager() string { return "AuthenticationCustomRouteController" } -func (c *customRouteController) removeOperands(ctx context.Context, ingressConfig *configv1.Ingress, secretName string) error { +func (c *customRouteController) removeOperands(ctx context.Context, ingressConfig *configv1.Ingress) error { if _, err := c.routeLister.Routes(c.componentRoute.Namespace).Get(c.componentRoute.Name); err != nil && !errors.IsNotFound(err) { return err } else if !errors.IsNotFound(err) { From 4bdb1b4ef5ee99821fc33063f8fbf3221f8f3cc4 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Tue, 3 Dec 2024 15:06:07 +0100 Subject: [PATCH 21/23] fixup! controllers: add switchable informer controller --- .../oauthclientsswitchedinformer_controller.go | 9 +++++---- .../oauthclientsswitchedinformer_controller_test.go | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller.go b/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller.go index 8d35507ed..0bf106e8e 100644 --- a/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller.go +++ b/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller.go @@ -4,6 +4,7 @@ import ( "context" "time" + oauthinformersv1 "github.com/openshift/client-go/oauth/informers/externalversions/oauth/v1" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" @@ -17,7 +18,7 @@ import ( // is returned, then the controller stops/starts the informer based on the bool value // (true means stop). type InformerWithSwitch struct { - delegateInformer cache.SharedIndexInformer + delegateInformer oauthinformersv1.OAuthClientInformer switchController factory.Controller shouldStopFn func() (bool, error) parentCtx context.Context @@ -45,7 +46,7 @@ func NewSwitchedInformer( name string, ctx context.Context, shouldStopFn func() (bool, error), - delegateInformer cache.SharedIndexInformer, + delegateInformer oauthinformersv1.OAuthClientInformer, resync time.Duration, informers []factory.Informer, recorder events.Recorder, @@ -78,7 +79,7 @@ func (s *InformerWithSwitch) Controller() factory.Controller { func (s *InformerWithSwitch) Informer() cache.SharedIndexInformer { return &alwaysSyncedInformer{ isRunning: func() bool { return s.runCtx != nil }, - SharedIndexInformer: s.delegateInformer, + SharedIndexInformer: s.delegateInformer.Informer(), } } @@ -97,7 +98,7 @@ func (s *InformerWithSwitch) ensureRunning() { klog.Infof("%s delegate informer starting", s.switchController.Name()) s.runCtx, s.stopFunc = context.WithCancel(s.parentCtx) - go s.delegateInformer.Run(s.runCtx.Done()) + go s.delegateInformer.Informer().Run(s.runCtx.Done()) } func (s *InformerWithSwitch) stop() { diff --git a/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller_test.go b/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller_test.go index 36f3c84fc..01526fc73 100644 --- a/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller_test.go +++ b/pkg/controllers/oauthclientsswitchedinformer/oauthclientsswitchedinformer_controller_test.go @@ -31,7 +31,7 @@ func TestSync(t *testing.T) { "TestInformerWithSwitchController", testCtx, shouldStopFn, - testInformer.Informer(), + testInformer, 0, nil, events.NewInMemoryRecorder("oauthclientscontroller_test"), From e4d21534ab8356a54299c86a666ba42690d9b976 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Tue, 3 Dec 2024 15:06:31 +0100 Subject: [PATCH 22/23] fixup! oauthclientscontroller: use a switched informer for oauthclients --- .../oauthclientscontroller/oauthclientscontroller_test.go | 2 +- pkg/operator/starter.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go b/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go index 667fe6584..891b2eebc 100644 --- a/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go +++ b/pkg/controllers/oauthclientscontroller/oauthclientscontroller_test.go @@ -126,7 +126,7 @@ func newTestOAuthsClientsController(t *testing.T) *oauthsClientsController { "TestOAuthClientsInformerWithSwitchController", context.TODO(), func() (bool, error) { return false, nil }, - oauthinformers.NewSharedInformerFactoryWithOptions(oauthClientset, 1*time.Minute).Oauth().V1().OAuthClients().Informer(), + oauthinformers.NewSharedInformerFactoryWithOptions(oauthClientset, 1*time.Minute).Oauth().V1().OAuthClients(), 0, nil, events.NewInMemoryRecorder("oauthclientscontroller_test"), diff --git a/pkg/operator/starter.go b/pkg/operator/starter.go index 21912b288..f99aa1c30 100644 --- a/pkg/operator/starter.go +++ b/pkg/operator/starter.go @@ -262,7 +262,7 @@ func prepareOauthOperator( func() (bool, error) { return common.ExternalOIDCConfigAvailable(authLister, kasLister, kasConfigMapLister) }, - oauthinformers.NewSharedInformerFactoryWithOptions(authOperatorInput.oauthClient, 1*time.Minute).Oauth().V1().OAuthClients().Informer(), + oauthinformers.NewSharedInformerFactoryWithOptions(authOperatorInput.oauthClient, 1*time.Minute).Oauth().V1().OAuthClients(), 0, []factory.Informer{ informerFactories.operatorInformer.Operator().V1().KubeAPIServers().Informer(), From a1cf995e5006067642ff64be1b595132fc236321 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Tue, 10 Dec 2024 11:59:56 +0100 Subject: [PATCH 23/23] fixup! common: add helper func to determine whether OIDC is enabled on KAS pods --- pkg/controllers/common/external_oidc.go | 4 ++++ pkg/controllers/common/external_oidc_test.go | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/pkg/controllers/common/external_oidc.go b/pkg/controllers/common/external_oidc.go index 39978abcd..0e1f3755b 100644 --- a/pkg/controllers/common/external_oidc.go +++ b/pkg/controllers/common/external_oidc.go @@ -37,6 +37,10 @@ func ExternalOIDCConfigAvailable(authLister configv1listers.AuthenticationLister observedRevisions.Insert(nodeStatus.CurrentRevision) } + if observedRevisions.Len() == 0 { + return false, nil + } + for _, revision := range observedRevisions.UnsortedList() { // ensure every observed revision includes an auth-config revisioned configmap _, err := cmLister.ConfigMaps("openshift-kube-apiserver").Get(fmt.Sprintf("auth-config-%d", revision)) diff --git a/pkg/controllers/common/external_oidc_test.go b/pkg/controllers/common/external_oidc_test.go index cd9635aa0..bd1aa7c10 100644 --- a/pkg/controllers/common/external_oidc_test.go +++ b/pkg/controllers/common/external_oidc_test.go @@ -28,6 +28,12 @@ func TestExternalOIDCConfigAvailable(t *testing.T) { expectAvailable bool expectError bool }{ + { + name: "no node statuses observed", + authType: configv1.AuthenticationTypeOIDC, + expectAvailable: false, + expectError: false, + }, { name: "oidc disabled, no rollout", configMaps: []*corev1.ConfigMap{cm("config-10", "config.yaml", kasConfigJSONWithoutOIDC)},