From 1d335e75046feef0ceffd8f99939a48e265a1adc Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Tue, 15 Apr 2025 19:04:34 -0400 Subject: [PATCH 1/4] status: Omit istios namespace in relatedObjects Omit the namespace for the istios resource in relatedObjects as the istios resource is cluster-scoped. * pkg/operator/controller/status/controller.go (Reconcile): Remove the namespace for istios. --- pkg/operator/controller/status/controller.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/operator/controller/status/controller.go b/pkg/operator/controller/status/controller.go index 5bd956da9d..731363c6bd 100644 --- a/pkg/operator/controller/status/controller.go +++ b/pkg/operator/controller/status/controller.go @@ -233,12 +233,11 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( Namespace: subscriptionName.Namespace, Name: subscriptionName.Name, }) + related = append(related, configv1.ObjectReference{ + Group: sailv1.GroupVersion.Group, + Resource: "istios", + }) if state.IngressNamespace != nil { - related = append(related, configv1.ObjectReference{ - Group: sailv1.GroupVersion.Group, - Resource: "istios", - Namespace: state.IngressNamespace.Name, - }) related = append(related, configv1.ObjectReference{ Group: gatewayapiv1.GroupName, Resource: "gateways", From fb548fc901b620a904af46192f0ca5a8ad52415d Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Tue, 15 Apr 2025 19:07:21 -0400 Subject: [PATCH 2/4] status: Omit gateways namespace in relatedObjects Omit the namespace for the gateways resource in relatedObjects as Istio manages gateways in all namespaces. * pkg/operator/controller/status/controller.go (Reconcile): Remove the namespace for gateways. --- pkg/operator/controller/status/controller.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/operator/controller/status/controller.go b/pkg/operator/controller/status/controller.go index 731363c6bd..181a5ee61a 100644 --- a/pkg/operator/controller/status/controller.go +++ b/pkg/operator/controller/status/controller.go @@ -237,13 +237,11 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( Group: sailv1.GroupVersion.Group, Resource: "istios", }) - if state.IngressNamespace != nil { - related = append(related, configv1.ObjectReference{ - Group: gatewayapiv1.GroupName, - Resource: "gateways", - Namespace: state.IngressNamespace.Name, - }) - } + related = append(related, configv1.ObjectReference{ + Group: gatewayapiv1.GroupName, + Resource: "gateways", + Namespace: "", // Include all namespaces. + }) } } From 751bb767bf997ec46e4049a32af6065269a599bd Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Tue, 8 Apr 2025 18:51:54 -0400 Subject: [PATCH 3/4] getOperatorState: Add context parameter * pkg/operator/controller/status/controller.go (Reconcile): Pass ctx to getOperatorState. * pkg/operator/controller/status/controller.go (getOperatorState): Add a parameter for ctx, and use it instead of context.TODO(). --- pkg/operator/controller/status/controller.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/operator/controller/status/controller.go b/pkg/operator/controller/status/controller.go index 181a5ee61a..b09ca841d2 100644 --- a/pkg/operator/controller/status/controller.go +++ b/pkg/operator/controller/status/controller.go @@ -183,7 +183,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( } oldStatus := co.Status.DeepCopy() - state, err := r.getOperatorState(ingressNamespace, canaryNamespace, co) + state, err := r.getOperatorState(ctx, ingressNamespace, canaryNamespace, co) if err != nil { return reconcile.Result{}, fmt.Errorf("failed to get operator state: %v", err) } @@ -320,11 +320,11 @@ type operatorState struct { // getOperatorState gets and returns the resources necessary to compute the // operator's current state. -func (r *reconciler) getOperatorState(ingressNamespace, canaryNamespace string, co *configv1.ClusterOperator) (operatorState, error) { +func (r *reconciler) getOperatorState(ctx context.Context, ingressNamespace, canaryNamespace string, co *configv1.ClusterOperator) (operatorState, error) { state := operatorState{} ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ingressNamespace}} - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: ingressNamespace}, ns); err != nil { + if err := r.client.Get(ctx, types.NamespacedName{Name: ingressNamespace}, ns); err != nil { if !errors.IsNotFound(err) { return state, fmt.Errorf("failed to get namespace %q: %v", ingressNamespace, err) } @@ -333,7 +333,7 @@ func (r *reconciler) getOperatorState(ingressNamespace, canaryNamespace string, } ns = &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: canaryNamespace}} - if err := r.client.Get(context.TODO(), types.NamespacedName{Name: canaryNamespace}, ns); err != nil { + if err := r.client.Get(ctx, types.NamespacedName{Name: canaryNamespace}, ns); err != nil { if !errors.IsNotFound(err) { return state, fmt.Errorf("failed to get namespace %q: %v", canaryNamespace, err) } @@ -342,7 +342,7 @@ func (r *reconciler) getOperatorState(ingressNamespace, canaryNamespace string, } ingressList := &operatorv1.IngressControllerList{} - if err := r.cache.List(context.TODO(), ingressList, client.InNamespace(r.config.Namespace)); err != nil { + if err := r.cache.List(ctx, ingressList, client.InNamespace(r.config.Namespace)); err != nil { return state, fmt.Errorf("failed to list ingresscontrollers in %q: %v", r.config.Namespace, err) } else { state.IngressControllers = ingressList.Items @@ -352,7 +352,7 @@ func (r *reconciler) getOperatorState(ingressNamespace, canaryNamespace string, if r.config.MarketplaceEnabled && r.config.OperatorLifecycleManagerEnabled { var subscription operatorsv1alpha1.Subscription subscriptionName := operatorcontroller.ServiceMeshOperatorSubscriptionName() - if err := r.cache.Get(context.TODO(), subscriptionName, &subscription); err != nil { + if err := r.cache.Get(ctx, subscriptionName, &subscription); err != nil { if !errors.IsNotFound(err) { return state, fmt.Errorf("failed to get subscription %q: %v", subscriptionName, err) } From 6f42089c113be8c5286f35aefa9d52130753face Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Tue, 8 Apr 2025 19:08:13 -0400 Subject: [PATCH 4/4] status: Conditionally add CRDs to relatedObjects Check whether the gatewayclasses, gateways, and istios CRDs actually exist before adding them to relatedObjects. Watch customresourcedefinitions in the status controller so that it updates relatedObjects as these CRDs are created. Check the "GatewayAPIController" featuregate to determine whether to add the gatewayclasses, gateways, istios, and subscriptions resources to relatedObjects, in addition to checking the "GatewayAPI" featuregate. Before this change, the operator could add istios to relatedObjects even if the OSSM subscription failed to install. By convention, an operator should only add resources to relatedObjects if those resources exist. This commit fixes OCPBUGS-54745. https://issues.redhat.com/browse/OCPBUGS-54745 * pkg/operator/controller/status/controller.go (gatewaysResourceName, gatewayclassesResourceName, istiosResourceName): New consts for the CRD names. (relatedObjectsCRDs): New var for a string set that contains gatewaysResourceName, gatewayclassesResourceName, and istiosResourceName. (New): Check the GatewayAPIControllerEnabled field in the controller config in addition to checking GatewayAPIEnabled to determine whether to watch subscriptions and customresourcedefinitions. Add a watch on customresourcedefinitions, with a predicate for CRDs with names that are in relatedObjectsCRDs. (Config): Add GatewayAPIControllerEnabled. (Reconcile): Check the GatewayAPIControllerEnabled field in the controller config as well as the haveIstiosResource, haveGatewayclassesResource, and haveGatewaysResource fields in the operatorState object, and conditionally add the corresponding resources to relatedObjects. (operatorState): Add haveIstiosResource, haveGatewaysResource, and haveGatewayclassesResource fields. (getOperatorState): Check GatewayAPIControllerEnabled in addition to GatewayAPIEnabled before checking for the OSSM subscription. Set haveGatewaysResource, haveGatewayclassesResource, and haveIstiosResource. * pkg/operator/operator.go (New): Specify GatewayAPIControllerEnabled in the status controller config. * test/e2e/operator_test.go (TestClusterOperatorStatusRelatedObjects): Expect to see "gateways" and "gatewayclasses" in relatedObjects if the "GatewayAPI" and "GatewayAPIController" featuregates are enabled. --- pkg/operator/controller/status/controller.go | 129 +++++++++++++++---- pkg/operator/operator.go | 1 + test/e2e/operator_test.go | 23 +++- 3 files changed, 122 insertions(+), 31 deletions(-) diff --git a/pkg/operator/controller/status/controller.go b/pkg/operator/controller/status/controller.go index b09ca841d2..be802cca64 100644 --- a/pkg/operator/controller/status/controller.go +++ b/pkg/operator/controller/status/controller.go @@ -24,9 +24,11 @@ import ( corev1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" utilclock "k8s.io/utils/clock" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -50,13 +52,27 @@ const ( ingressesEqualConditionMessage = "desired and current number of IngressControllers are equal" + // gatewaysResourceName is the name of the Gateway API gateways CRD. + gatewaysResourceName = "gateways.gateway.networking.k8s.io" + // gatewayclassesResourceName is the name of the Gateway API + // gatewayclasses CRD. + gatewayclassesResourceName = "gatewayclasses.gateway.networking.k8s.io" + // istiosResourceName is the name of the Sail Operator istios CRD. + istiosResourceName = "istios.sailoperator.io" + controllerName = "status_controller" ) -var log = logf.Logger.WithName(controllerName) +var ( + log = logf.Logger.WithName(controllerName) + + // clock is to enable unit testing + clock utilclock.Clock = utilclock.RealClock{} -// clock is to enable unit testing -var clock utilclock.Clock = utilclock.RealClock{} + // relatedObjectsCRDs is a set of names of CRDs that we add to + // relatedObjects if they exist. + relatedObjectsCRDs = sets.New[string](gatewaysResourceName, gatewayclassesResourceName, istiosResourceName) +) // New creates the status controller. This is the controller that handles all // the logic for creating the ClusterOperator operator and updating its status. @@ -99,15 +115,15 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) { return nil, err } - // If the "GatewayAPI" controller featuregate is enabled, watch - // subscriptions so that this controller can update status when the OSSM - // subscription is created or updated. Note that the subscriptions - // resource only exists if the "OperatorLifecycleManager" capability is - // enabled, so we cannot watch it if the capability is not enabled. - // Additionally, the default catalog only exists if the "marketplace" - // capability is enabled, so we cannot install OSSM without that - // capability. - if config.GatewayAPIEnabled && config.MarketplaceEnabled && config.OperatorLifecycleManagerEnabled { + // If the "GatewayAPI" and "GatewayAPIController" featuregates are + // enabled, watch subscriptions so that this controller can update + // status when the OSSM subscription is created or updated. Note that + // the subscriptions resource only exists if the + // "OperatorLifecycleManager" capability is enabled, so we cannot watch + // it if the capability is not enabled. Additionally, the default + // catalog only exists if the "marketplace" capability is enabled, so we + // cannot install OSSM without that capability. + if config.GatewayAPIEnabled && config.GatewayAPIControllerEnabled && config.MarketplaceEnabled && config.OperatorLifecycleManagerEnabled { if err := c.Watch(source.Kind[client.Object](operatorCache, &operatorsv1alpha1.Subscription{}, handler.EnqueueRequestsFromMapFunc(toDefaultIngressController), predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { return e.Object.GetNamespace() == operatorcontroller.OpenshiftOperatorNamespace @@ -124,6 +140,22 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) { })); err != nil { return nil, err } + if err := c.Watch(source.Kind[client.Object](operatorCache, &apiextensionsv1.CustomResourceDefinition{}, handler.EnqueueRequestsFromMapFunc(toDefaultIngressController), predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + return relatedObjectsCRDs.Has(e.Object.GetName()) + }, + UpdateFunc: func(e event.UpdateEvent) bool { + return false + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return relatedObjectsCRDs.Has(e.Object.GetName()) + }, + GenericFunc: func(e event.GenericEvent) bool { + return false + }, + })); err != nil { + return nil, err + } } return c, nil @@ -133,6 +165,9 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) { type Config struct { // GatewayAPIEnabled indicates that the "GatewayAPI" featuregate is enabled. GatewayAPIEnabled bool + // GatewayAPIControllerEnabled indicates that the "GatewayAPIController" + // featuregate is enabled. + GatewayAPIControllerEnabled bool // MarketplaceEnabled indicates whether the "marketplace" capability is // enabled. MarketplaceEnabled bool @@ -220,11 +255,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( Name: state.CanaryNamespace.Name, }) } - if r.config.GatewayAPIEnabled { - related = append(related, configv1.ObjectReference{ - Group: gatewayapiv1.GroupName, - Resource: "gatewayclasses", - }) + if r.config.GatewayAPIEnabled && r.config.GatewayAPIControllerEnabled { if state.haveOSSMSubscription { subscriptionName := operatorcontroller.ServiceMeshOperatorSubscriptionName() related = append(related, configv1.ObjectReference{ @@ -233,10 +264,20 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( Namespace: subscriptionName.Namespace, Name: subscriptionName.Name, }) + } + if state.haveIstiosResource { related = append(related, configv1.ObjectReference{ Group: sailv1.GroupVersion.Group, Resource: "istios", }) + } + if state.haveGatewayclassesResource { + related = append(related, configv1.ObjectReference{ + Group: gatewayapiv1.GroupName, + Resource: "gatewayclasses", + }) + } + if state.haveGatewaysResource { related = append(related, configv1.ObjectReference{ Group: gatewayapiv1.GroupName, Resource: "gateways", @@ -314,8 +355,17 @@ type operatorState struct { IngressControllers []operatorv1.IngressController DNSRecords []iov1.DNSRecord - haveOSSMSubscription bool unmanagedGatewayAPICRDNames string + // haveOSSMSubscription means that the subscription for OSSM 3 exists. + haveOSSMSubscription bool + // haveIstiosResource means that the "istios.sailproject.io" CRD exists. + haveIstiosResource bool + // haveGatewaysResource means that the + // "gateways.gateway.networking.k8s.io" CRD exists. + haveGatewaysResource bool + // haveGatewayclassesResource means that the + // "gatewayclasses.gateway.networking.k8s.io" CRD exists. + haveGatewayclassesResource bool } // getOperatorState gets and returns the resources necessary to compute the @@ -349,7 +399,15 @@ func (r *reconciler) getOperatorState(ctx context.Context, ingressNamespace, can } if r.config.GatewayAPIEnabled { - if r.config.MarketplaceEnabled && r.config.OperatorLifecycleManagerEnabled { + if len(co.Status.Extension.Raw) > 0 { + extension := &IngressOperatorStatusExtension{} + if err := json.Unmarshal(co.Status.Extension.Raw, extension); err != nil { + return state, fmt.Errorf("failed to unmarshal status extension of cluster operator %q: %w", co.Name, err) + } + state.unmanagedGatewayAPICRDNames = extension.UnmanagedGatewayAPICRDNames + } + + if r.config.GatewayAPIControllerEnabled && r.config.MarketplaceEnabled && r.config.OperatorLifecycleManagerEnabled { var subscription operatorsv1alpha1.Subscription subscriptionName := operatorcontroller.ServiceMeshOperatorSubscriptionName() if err := r.cache.Get(ctx, subscriptionName, &subscription); err != nil { @@ -360,14 +418,35 @@ func (r *reconciler) getOperatorState(ctx context.Context, ingressNamespace, can state.haveOSSMSubscription = true } - } - if len(co.Status.Extension.Raw) > 0 { - extension := &IngressOperatorStatusExtension{} - if err := json.Unmarshal(co.Status.Extension.Raw, extension); err != nil { - return state, fmt.Errorf("failed to unmarshal status extension of cluster operator %q: %w", co.Name, err) + var ( + crd apiextensionsv1.CustomResourceDefinition + gatewaysResourceNamespacedName = types.NamespacedName{Name: gatewaysResourceName} + gatewayclassesResourceNamespacedName = types.NamespacedName{Name: gatewayclassesResourceName} + istiosResourceNamespacedName = types.NamespacedName{Name: istiosResourceName} + ) + + if err := r.cache.Get(ctx, gatewaysResourceNamespacedName, &crd); err != nil { + if !errors.IsNotFound(err) { + return state, fmt.Errorf("failed to get CRD %q: %v", gatewaysResourceName, err) + } + } else { + state.haveGatewaysResource = true + } + if err := r.cache.Get(ctx, gatewayclassesResourceNamespacedName, &crd); err != nil { + if !errors.IsNotFound(err) { + return state, fmt.Errorf("failed to get CRD %q: %v", gatewayclassesResourceName, err) + } + } else { + state.haveGatewayclassesResource = true + } + if err := r.cache.Get(ctx, istiosResourceNamespacedName, &crd); err != nil { + if !errors.IsNotFound(err) { + return state, fmt.Errorf("failed to get CRD %q: %v", istiosResourceName, err) + } + } else { + state.haveIstiosResource = true } - state.unmanagedGatewayAPICRDNames = extension.UnmanagedGatewayAPICRDNames } } diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index f9d9ea3306..bf749eb849 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -225,6 +225,7 @@ func New(config operatorconfig.Config, kubeConfig *rest.Config) (*Operator, erro CanaryImage: config.CanaryImage, OperatorReleaseVersion: config.OperatorReleaseVersion, GatewayAPIEnabled: gatewayAPIEnabled, + GatewayAPIControllerEnabled: gatewayAPIControllerEnabled, MarketplaceEnabled: marketplaceEnabled, OperatorLifecycleManagerEnabled: olmEnabled, }); err != nil { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 09d997de97..eb5ad72328 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -233,12 +233,23 @@ func TestClusterOperatorStatusRelatedObjects(t *testing.T) { if gatewayAPIEnabled, err := isFeatureGateEnabled(features.FeatureGateGatewayAPI); err != nil { t.Fatalf("Failed to look up %q featuregate: %v", features.FeatureGateGatewayAPI, err) } else if gatewayAPIEnabled { - expected = append(expected, configv1.ObjectReference{ - Group: "gateway.networking.k8s.io", - Resource: "gatewayclasses", - }) - // This test runs before TestGatewayAPI, so we do *not* expect - // to see subscriptions, istios, or gateways in relatedObjects. + if gatewayAPIControllerEnabled, err := isFeatureGateEnabled(features.FeatureGateGatewayAPIController); err != nil { + t.Fatalf("Failed to look up %q featuregate: %v", features.FeatureGateGatewayAPIController, err) + } else if gatewayAPIControllerEnabled { + // This test runs before TestGatewayAPI creates the + // subscription to install OSSM, so we do *not* expect + // to see subscriptions or istios in relatedObjects. + // However, we *do* expect to see gatewayclasses and + // gateways whenever the GatewayAPI and + // GatewayAPIController featuregates are enabled. + expected = append(expected, configv1.ObjectReference{ + Group: "gateway.networking.k8s.io", + Resource: "gatewayclasses", + }, configv1.ObjectReference{ + Group: "gateway.networking.k8s.io", + Resource: "gateways", + }) + } } coName := controller.IngressClusterOperatorName()