From 900432997fd86b32a5464b745a143c3ad9e721a0 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 17 Dec 2025 12:18:58 +0000 Subject: [PATCH 01/12] Update library-go to include feature gating manifest-inclusion --- go.mod | 2 + go.sum | 4 +- .../library-go/pkg/manifest/manifest.go | 49 +++++++++++++++++-- vendor/modules.txt | 3 +- 4 files changed, 52 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 05ee80bb23..64cc4eec5d 100644 --- a/go.mod +++ b/go.mod @@ -33,6 +33,8 @@ require ( k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 ) +replace github.com/openshift/library-go => github.com/JoelSpeed/library-go v0.0.0-20251217102247-bcd739ac7ffa + require ( cel.dev/expr v0.24.0 // indirect github.com/antlr4-go/antlr/v4 v4.13.0 // indirect diff --git a/go.sum b/go.sum index 359398b7a8..03b9562c77 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ cel.dev/expr v0.24.0 h1:56OvJKSH3hDGL0ml5uSxZmz3/3Pq4tJ+fb1unVLAFcY= cel.dev/expr v0.24.0/go.mod h1:hLPLo1W4QUmuYdA72RBX06QTs6MXw941piREPl3Yfiw= +github.com/JoelSpeed/library-go v0.0.0-20251217102247-bcd739ac7ffa h1:XI0l1W9wZLiVI/VV8nDtuX3RN7vzAKH9HnoEnCgJF/s= +github.com/JoelSpeed/library-go v0.0.0-20251217102247-bcd739ac7ffa/go.mod h1:ErDfiIrPHH+menTP/B4LKd0nxFDdvCbTamAc6SWMIh8= github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI= github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= @@ -90,8 +92,6 @@ github.com/openshift/api v0.0.0-20251127005036-0e3c378fdedc h1:p83VYAk7mlqYZrMaK github.com/openshift/api v0.0.0-20251127005036-0e3c378fdedc/go.mod h1:d5uzF0YN2nQQFA0jIEWzzOZ+edmo6wzlGLvx5Fhz4uY= github.com/openshift/client-go v0.0.0-20251201171210-333716c1124a h1:iJYjd+rxyjMa3Sk6Vg55secJ4yMrabr/ulnTiy+vDH0= github.com/openshift/client-go v0.0.0-20251201171210-333716c1124a/go.mod h1:WD7m8ADeqiAKTHWx/mBoE/1MFMtnt9MYTyBOnf0L3LI= -github.com/openshift/library-go v0.0.0-20251120164824-14a789e09884 h1:6512TMT14gnXQ4vyshzAQGjkctU0PO9G+y0tcBjw6Vk= -github.com/openshift/library-go v0.0.0-20251120164824-14a789e09884/go.mod h1:ErDfiIrPHH+menTP/B4LKd0nxFDdvCbTamAc6SWMIh8= github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20241205171354-8006f302fd12 h1:AKx/w1qpS8We43bsRgf8Nll3CGlDHpr/WAXvuedTNZI= github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20241205171354-8006f302fd12/go.mod h1:7Du3c42kxCUegi0IImZ1wUQzMBVecgIHjR1C+NkhLQo= github.com/operator-framework/api v0.17.1 h1:J/6+Xj4IEV8C7hcirqUFwOiZAU3PbnJhWvB0/bB51c4= diff --git a/vendor/github.com/openshift/library-go/pkg/manifest/manifest.go b/vendor/github.com/openshift/library-go/pkg/manifest/manifest.go index d4082ce86b..4096f95070 100644 --- a/vendor/github.com/openshift/library-go/pkg/manifest/manifest.go +++ b/vendor/github.com/openshift/library-go/pkg/manifest/manifest.go @@ -26,6 +26,7 @@ const ( CapabilityAnnotation = "capability.openshift.io/name" DefaultClusterProfile = "self-managed-high-availability" featureSetAnnotation = "release.openshift.io/feature-set" + featureGateAnnotation = "release.openshift.io/feature-gates" ) var knownFeatureSets = sets.Set[string]{} @@ -187,12 +188,46 @@ func checkFeatureSets(requiredFeatureSet string, annotations map[string]string) return nil } +// checkFeatureGates validates if manifest should be included based on feature gate requirements +func checkFeatureGates(enabledGates sets.Set[string], annotations map[string]string) error { + if annotations == nil { + return nil // No annotations, include by default + } + gateRequirements, ok := annotations[featureGateAnnotation] + if !ok { + return nil // No requirements, include by default + } + + requirements := strings.Split(gateRequirements, ",") + for _, req := range requirements { + req = strings.TrimSpace(req) + if req == "" { + continue + } + + if strings.HasPrefix(req, "-") { + // Exclusion: gate must NOT be enabled + gate := req[1:] + if enabledGates.Has(gate) { + return fmt.Errorf("feature gate %s is enabled but manifest requires it to be disabled", gate) + } + } else { + // Inclusion: gate must be enabled + if !enabledGates.Has(req) { + return fmt.Errorf("feature gate %s is required but not enabled", req) + } + } + } + + return nil +} + // Include returns an error if the manifest fails an inclusion filter and should be excluded from further // processing by cluster version operator. Pointer arguments can be set nil to avoid excluding based on that // filter. For example, setting profile non-nil and capabilities nil will return an error if the manifest's // profile does not match, but will never return an error about capability issues. -func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string, profile *string, capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride) error { - return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, overrides, false) +func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string, profile *string, capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, enabledFeatureGates sets.Set[string]) error { + return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, overrides, enabledFeatureGates, false) } // IncludeAllowUnknownCapabilities returns an error if the manifest fails an inclusion filter and should be excluded from @@ -202,7 +237,7 @@ func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string // to capabilities filtering. When set to true a manifest will not be excluded simply because it contains an unknown // capability. This is necessary to allow updates to an OCP version containing newly defined capabilities. func (m *Manifest) IncludeAllowUnknownCapabilities(excludeIdentifier *string, requiredFeatureSet *string, profile *string, - capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, allowUnknownCapabilities bool) error { + capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, enabledFeatureGates sets.Set[string], allowUnknownCapabilities bool) error { annotations := m.Obj.GetAnnotations() if annotations == nil { @@ -223,6 +258,14 @@ func (m *Manifest) IncludeAllowUnknownCapabilities(excludeIdentifier *string, re } } + // Feature gate filtering + if enabledFeatureGates != nil { + err := checkFeatureGates(enabledFeatureGates, annotations) + if err != nil { + return err + } + } + if profile != nil { profileAnnotation := fmt.Sprintf("include.release.openshift.io/%s", *profile) if val, ok := annotations[profileAnnotation]; ok && val != "true" { diff --git a/vendor/modules.txt b/vendor/modules.txt index c657ef53fb..379278e5fd 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -236,7 +236,7 @@ github.com/openshift/client-go/security/clientset/versioned/fake github.com/openshift/client-go/security/clientset/versioned/scheme github.com/openshift/client-go/security/clientset/versioned/typed/security/v1 github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake -# github.com/openshift/library-go v0.0.0-20251120164824-14a789e09884 +# github.com/openshift/library-go v0.0.0-20251120164824-14a789e09884 => github.com/JoelSpeed/library-go v0.0.0-20251217102247-bcd739ac7ffa ## explicit; go 1.24.0 github.com/openshift/library-go/pkg/apiserver/jsonpatch github.com/openshift/library-go/pkg/config/clusterstatus @@ -1001,4 +1001,5 @@ sigs.k8s.io/structured-merge-diff/v6/value # sigs.k8s.io/yaml v1.6.0 ## explicit; go 1.22 sigs.k8s.io/yaml +# github.com/openshift/library-go => github.com/JoelSpeed/library-go v0.0.0-20251217102247-bcd739ac7ffa # github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20241205171354-8006f302fd12 From 8163e7d95102b324d6e98d537fb95bde328783ad Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 17 Dec 2025 15:57:14 +0000 Subject: [PATCH 02/12] Pass feature gating information through to sync workers --- hack/cluster-version-util/task_graph.go | 3 +- lib/manifest/manifest.go | 3 + pkg/cvo/cvo.go | 104 ++++++++- pkg/cvo/cvo_featuregates_test.go | 185 ++++++++++++++++ pkg/cvo/cvo_scenarios_test.go | 64 ++++++ pkg/cvo/featuregate_integration_test.go | 277 ++++++++++++++++++++++++ pkg/cvo/sync_test.go | 3 +- pkg/cvo/sync_worker.go | 76 ++++--- pkg/payload/payload.go | 7 +- pkg/payload/payload_test.go | 6 +- pkg/payload/render.go | 2 +- pkg/payload/task_graph_test.go | 3 +- pkg/start/start.go | 1 + 13 files changed, 693 insertions(+), 41 deletions(-) create mode 100644 pkg/cvo/cvo_featuregates_test.go create mode 100644 pkg/cvo/featuregate_integration_test.go diff --git a/hack/cluster-version-util/task_graph.go b/hack/cluster-version-util/task_graph.go index 122ccd7812..8f9c93baeb 100644 --- a/hack/cluster-version-util/task_graph.go +++ b/hack/cluster-version-util/task_graph.go @@ -6,6 +6,7 @@ import ( "time" "github.com/spf13/cobra" + "k8s.io/apimachinery/pkg/util/sets" "github.com/openshift/cluster-version-operator/pkg/payload" ) @@ -30,7 +31,7 @@ func newTaskGraphCmd() *cobra.Command { func runTaskGraphCmd(cmd *cobra.Command, args []string) error { manifestDir := args[0] - release, err := payload.LoadUpdate(manifestDir, "", "", "", payload.DefaultClusterProfile, nil) + release, err := payload.LoadUpdate(manifestDir, "", "", "", payload.DefaultClusterProfile, nil, sets.Set[string]{}) if err != nil { return err } diff --git a/lib/manifest/manifest.go b/lib/manifest/manifest.go index d5ad682331..0c7faff843 100644 --- a/lib/manifest/manifest.go +++ b/lib/manifest/manifest.go @@ -48,6 +48,7 @@ func GetImplicitlyEnabledCapabilities( currentPayloadManifests []manifest.Manifest, manifestInclusionConfiguration InclusionConfiguration, currentImplicitlyEnabled sets.Set[configv1.ClusterVersionCapability], + enabledFeatureGates sets.Set[string], ) sets.Set[configv1.ClusterVersionCapability] { ret := currentImplicitlyEnabled.Clone() for _, updateManifest := range updatePayloadManifests { @@ -57,6 +58,7 @@ func GetImplicitlyEnabledCapabilities( manifestInclusionConfiguration.Profile, manifestInclusionConfiguration.Capabilities, manifestInclusionConfiguration.Overrides, + enabledFeatureGates, true, ) // update manifest is enabled, no need to check @@ -74,6 +76,7 @@ func GetImplicitlyEnabledCapabilities( manifestInclusionConfiguration.Profile, manifestInclusionConfiguration.Capabilities, manifestInclusionConfiguration.Overrides, + enabledFeatureGates, true, ); err != nil { continue diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index 59b1da6ce1..cdf653ee18 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -13,6 +13,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" informerscorev1 "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" @@ -120,6 +121,7 @@ type Operator struct { cmConfigLister listerscorev1.ConfigMapNamespaceLister cmConfigManagedLister listerscorev1.ConfigMapNamespaceLister proxyLister configlistersv1.ProxyLister + featureGateLister configlistersv1.FeatureGateLister cacheSynced []cache.InformerSynced // queue tracks applying updates to a cluster. @@ -189,6 +191,8 @@ type Operator struct { // featurechangestopper controller will detect when cluster feature gate config changes and shutdown the CVO. enabledFeatureGates featuregates.CvoGateChecker + enabledManifestFeatureGates sets.Set[string] + clusterProfile string uid types.UID @@ -213,6 +217,7 @@ func New( cmConfigManagedInformer informerscorev1.ConfigMapInformer, proxyInformer configinformersv1.ProxyInformer, operatorInformerFactory operatorexternalversions.SharedInformerFactory, + featureGateInformer configinformersv1.FeatureGateInformer, client clientset.Interface, kubeClient kubernetes.Interface, operatorClient operatorclientset.Interface, @@ -248,9 +253,9 @@ func New( kubeClient: kubeClient, operatorClient: operatorClient, eventRecorder: eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: namespace}), - queue: workqueue.NewTypedRateLimitingQueueWithConfig[any](workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "clusterversion"}), - availableUpdatesQueue: workqueue.NewTypedRateLimitingQueueWithConfig[any](workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "availableupdates"}), - upgradeableQueue: workqueue.NewTypedRateLimitingQueueWithConfig[any](workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "upgradeable"}), + queue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "clusterversion"}), + availableUpdatesQueue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "availableupdates"}), + upgradeableQueue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "upgradeable"}), hypershift: hypershift, exclude: exclude, @@ -276,6 +281,9 @@ func New( if _, err := coInformer.Informer().AddEventHandler(optr.clusterOperatorEventHandler()); err != nil { return nil, err } + if _, err := featureGateInformer.Informer().AddEventHandler(optr.featureGateEventHandler()); err != nil { + return nil, err + } optr.coLister = coInformer.Lister() optr.cacheSynced = append(optr.cacheSynced, coInformer.Informer().HasSynced) @@ -287,6 +295,12 @@ func New( optr.cmConfigLister = cmConfigInformer.Lister().ConfigMaps(internal.ConfigNamespace) optr.cmConfigManagedLister = cmConfigManagedInformer.Lister().ConfigMaps(internal.ConfigManagedNamespace) + optr.featureGateLister = featureGateInformer.Lister() + optr.cacheSynced = append(optr.cacheSynced, featureGateInformer.Informer().HasSynced) + + // Initialize cluster feature gates + optr.initializeFeatureGates() + // make sure this is initialized after all the listers are initialized optr.upgradeableChecks = optr.defaultUpgradeableChecks() @@ -318,7 +332,7 @@ func (optr *Operator) LoadInitialPayload(ctx context.Context, restConfig *rest.C } update, err := payload.LoadUpdate(optr.defaultPayloadDir(), optr.release.Image, optr.exclude, string(optr.requiredFeatureSet), - optr.clusterProfile, configv1.KnownClusterVersionCapabilities) + optr.clusterProfile, configv1.KnownClusterVersionCapabilities, optr.getEnabledFeatureGates()) if err != nil { return nil, fmt.Errorf("the local release contents are invalid - no current version can be determined from disk: %v", err) @@ -779,7 +793,7 @@ func (optr *Operator) sync(ctx context.Context, key string) error { } // inform the config sync loop about our desired state - status := optr.configSync.Update(ctx, config.Generation, desired, config, state) + status := optr.configSync.Update(ctx, config.Generation, desired, config, state, optr.getEnabledFeatureGates()) // write cluster version status return optr.syncStatus(ctx, original, config, status, errs) @@ -1084,6 +1098,86 @@ func (optr *Operator) HTTPClient() (*http.Client, error) { }, nil } +// featureGateEventHandler handles changes to FeatureGate objects and updates the cluster feature gates +func (optr *Operator) featureGateEventHandler() cache.ResourceEventHandler { + workQueueKey := optr.queueKey() + return cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + optr.updateEnabledFeatureGates(obj) + optr.queue.Add(workQueueKey) + }, + UpdateFunc: func(old, new interface{}) { + optr.updateEnabledFeatureGates(new) + optr.queue.Add(workQueueKey) + }, + } +} + +// initializeFeatureGates initializes the cluster feature gates from the current FeatureGate object +func (optr *Operator) initializeFeatureGates() { + optr.enabledManifestFeatureGates = sets.Set[string]{} + + // Try to load initial state from the cluster FeatureGate object + if optr.featureGateLister != nil { + if featureGate, err := optr.featureGateLister.Get("cluster"); err == nil { + optr.enabledManifestFeatureGates = optr.extractEnabledGates(featureGate) + } + } +} + +// updateEnabledFeatureGates updates the cluster feature gates based on a FeatureGate object +func (optr *Operator) updateEnabledFeatureGates(obj interface{}) { + featureGate, ok := obj.(*configv1.FeatureGate) + if !ok { + klog.Warningf("Expected FeatureGate object but got %T", obj) + return + } + + newGates := optr.extractEnabledGates(featureGate) + + // Check if gates actually changed to avoid unnecessary work + if !optr.enabledManifestFeatureGates.Equal(newGates) { + klog.V(2).Infof("Cluster feature gates changed from %v to %v", + sets.List(optr.enabledManifestFeatureGates), sets.List(newGates)) + optr.enabledManifestFeatureGates = newGates + } +} + +// getEnabledFeatureGates returns a copy of the current cluster feature gates for safe consumption +func (optr *Operator) getEnabledFeatureGates() sets.Set[string] { + // Return a copy to prevent external modification + result := sets.Set[string]{} + for gate := range optr.enabledManifestFeatureGates { + result.Insert(gate) + } + return result +} + +// extractEnabledGates extracts the list of enabled feature gates for the current cluster version +func (optr *Operator) extractEnabledGates(featureGate *configv1.FeatureGate) sets.Set[string] { + enabledGates := sets.Set[string]{} + + // Find the feature gate details for the current cluster version + currentVersion := optr.release.Version + for _, details := range featureGate.Status.FeatureGates { + if details.Version == currentVersion { + for _, enabled := range details.Enabled { + enabledGates.Insert(string(enabled.Name)) + } + klog.V(4).Infof("Found %d enabled feature gates for version %s: %v", + enabledGates.Len(), currentVersion, sets.List(enabledGates)) + break + } + } + + // If no matching version found, log a warning but continue with empty set + if enabledGates.Len() == 0 { + klog.V(2).Infof("No feature gates found for current version %s, using empty set", currentVersion) + } + + return enabledGates +} + // shouldReconcileCVOConfiguration returns whether the CVO should reconcile its configuration using the API server. // // enabledFeatureGates must be initialized before the function is called. diff --git a/pkg/cvo/cvo_featuregates_test.go b/pkg/cvo/cvo_featuregates_test.go new file mode 100644 index 0000000000..aad0c559da --- /dev/null +++ b/pkg/cvo/cvo_featuregates_test.go @@ -0,0 +1,185 @@ +package cvo + +import ( + "testing" + + configv1 "github.com/openshift/api/config/v1" + "k8s.io/apimachinery/pkg/util/sets" +) + +func TestOperator_extractEnabledGates(t *testing.T) { + tests := []struct { + name string + featureGate *configv1.FeatureGate + release configv1.Release + expected sets.Set[string] + }{ + { + name: "extract gates for matching version", + featureGate: &configv1.FeatureGate{ + Status: configv1.FeatureGateStatus{ + FeatureGates: []configv1.FeatureGateDetails{ + { + Version: "4.14.0", + Enabled: []configv1.FeatureGateAttributes{ + {Name: "TechPreviewFeatureGate"}, + {Name: "ExperimentalFeature"}, + }, + }, + { + Version: "4.13.0", + Enabled: []configv1.FeatureGateAttributes{ + {Name: "OldFeature"}, + }, + }, + }, + }, + }, + release: configv1.Release{Version: "4.14.0"}, + expected: sets.New[string]("TechPreviewFeatureGate", "ExperimentalFeature"), + }, + { + name: "no matching version - return empty", + featureGate: &configv1.FeatureGate{ + Status: configv1.FeatureGateStatus{ + FeatureGates: []configv1.FeatureGateDetails{ + { + Version: "4.13.0", + Enabled: []configv1.FeatureGateAttributes{ + {Name: "OldFeature"}, + }, + }, + }, + }, + }, + release: configv1.Release{Version: "4.14.0"}, + expected: sets.Set[string]{}, + }, + { + name: "empty enabled gates", + featureGate: &configv1.FeatureGate{ + Status: configv1.FeatureGateStatus{ + FeatureGates: []configv1.FeatureGateDetails{ + { + Version: "4.14.0", + Enabled: []configv1.FeatureGateAttributes{}, + }, + }, + }, + }, + release: configv1.Release{Version: "4.14.0"}, + expected: sets.Set[string]{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + optr := &Operator{ + release: tt.release, + } + + result := optr.extractEnabledGates(tt.featureGate) + + if !result.Equal(tt.expected) { + t.Errorf("extractEnabledGates() = %v, expected %v", sets.List(result), sets.List(tt.expected)) + } + }) + } +} + +func TestOperator_getEnabledFeatureGates(t *testing.T) { + optr := &Operator{ + enabledManifestFeatureGates: sets.New[string]("gate1", "gate2"), + } + + result := optr.getEnabledFeatureGates() + expected := sets.New[string]("gate1", "gate2") + + if !result.Equal(expected) { + t.Errorf("getEnabledFeatureGates() = %v, expected %v", sets.List(result), sets.List(expected)) + } + + // Verify it returns a copy by modifying the result + result.Insert("gate3") + result2 := optr.getEnabledFeatureGates() + + if result2.Has("gate3") { + t.Error("getEnabledFeatureGates() should return a copy, but original was modified") + } +} + +func TestOperator_updateEnabledFeatureGates(t *testing.T) { + tests := []struct { + name string + obj interface{} + expectUpdate bool + }{ + { + name: "valid FeatureGate object", + obj: &configv1.FeatureGate{ + Status: configv1.FeatureGateStatus{ + FeatureGates: []configv1.FeatureGateDetails{ + { + Version: "4.14.0", + Enabled: []configv1.FeatureGateAttributes{ + {Name: "NewGate"}, + }, + }, + }, + }, + }, + expectUpdate: true, + }, + { + name: "invalid object type", + obj: "not-a-feature-gate", + expectUpdate: false, + }, + { + name: "nil object", + obj: nil, + expectUpdate: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + optr := &Operator{ + enabledManifestFeatureGates: sets.New[string]("oldgate"), + release: configv1.Release{Version: "4.14.0"}, + } + + originalGates := optr.getEnabledFeatureGates() + optr.updateEnabledFeatureGates(tt.obj) + newGates := optr.getEnabledFeatureGates() + + if tt.expectUpdate { + if newGates.Equal(originalGates) { + t.Error("updateEnabledFeatureGates() expected gates to be updated") + } + if !newGates.Has("NewGate") { + t.Error("updateEnabledFeatureGates() expected NewGate to be enabled") + } + } else { + if !newGates.Equal(originalGates) { + t.Error("updateEnabledFeatureGates() should not update gates for invalid object") + } + } + }) + } +} + +func TestOperator_initializeFeatureGates(t *testing.T) { + // This test would require mocking the featureGateLister + // For now, test that the method doesn't panic and initializes empty set + optr := &Operator{ + enabledManifestFeatureGates: sets.New[string]("should-be-cleared"), + } + + optr.initializeFeatureGates() + + result := optr.getEnabledFeatureGates() + if result.Len() != 0 { + t.Error("initializeFeatureGates() should initialize with empty set when lister not available") + } +} diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index 80ab8653a1..801ada4c2d 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -18,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" apiruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" dynamicfake "k8s.io/client-go/dynamic/fake" clientgotesting "k8s.io/client-go/testing" @@ -255,6 +256,7 @@ func TestCVO_StartupAndSync(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Generation: 1, @@ -266,6 +268,7 @@ func TestCVO_StartupAndSync(t *testing.T) { LastTransitionTime: time.Unix(2, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Generation: 1, @@ -291,6 +294,7 @@ func TestCVO_StartupAndSync(t *testing.T) { KnownCapabilities: sortedKnownCaps, }, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Generation: 1, @@ -317,6 +321,7 @@ func TestCVO_StartupAndSync(t *testing.T) { EnabledCapabilities: sortedCaps, }, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Generation: 1, @@ -343,6 +348,7 @@ func TestCVO_StartupAndSync(t *testing.T) { KnownCapabilities: sortedKnownCaps, }, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Generation: 1, @@ -370,6 +376,7 @@ func TestCVO_StartupAndSync(t *testing.T) { KnownCapabilities: sortedKnownCaps, }, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -452,6 +459,7 @@ func TestCVO_StartupAndSync(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Generation: 1, @@ -478,6 +486,7 @@ func TestCVO_StartupAndSync(t *testing.T) { LastTransitionTime: time.Unix(2, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Generation: 1, @@ -504,6 +513,7 @@ func TestCVO_StartupAndSync(t *testing.T) { LastTransitionTime: time.Unix(3, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Generation: 1, @@ -531,6 +541,7 @@ func TestCVO_StartupAndSync(t *testing.T) { LastTransitionTime: time.Unix(4, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -672,6 +683,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Actual: configv1.Release{ @@ -688,6 +700,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Total: 3, @@ -715,6 +728,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Done: 1, @@ -743,6 +757,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Initial: true, @@ -771,6 +786,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -860,6 +876,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -888,6 +905,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -916,6 +934,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -945,6 +964,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -1076,6 +1096,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Actual: configv1.Release{ @@ -1091,6 +1112,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Total: 3, @@ -1117,6 +1139,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { KnownCapabilities: sortedKnownCaps, }, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Done: 1, @@ -1144,6 +1167,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { KnownCapabilities: sortedKnownCaps, }, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Done: 2, @@ -1171,6 +1195,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -1258,6 +1283,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -1285,6 +1311,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -1312,6 +1339,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -1340,6 +1368,7 @@ func TestCVO_StartupAndSyncPreconditionFailing(t *testing.T) { Local: true, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -1429,6 +1458,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Actual: configv1.Release{Version: "1.0.1-abc", Image: "image/image:1"}, @@ -1441,6 +1471,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, Failure: payloadErr, }, + EnabledFeatureGates: sets.New[string](), }, ) actions = client.Actions() @@ -1557,6 +1588,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -1688,6 +1720,7 @@ func TestCVO_ResetPayloadLoadStatus(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Actual: configv1.Release{Version: "1.0.1-abc", Image: "image/image:1"}, @@ -1700,6 +1733,7 @@ func TestCVO_ResetPayloadLoadStatus(t *testing.T) { Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, Failure: payloadErr, }, + EnabledFeatureGates: sets.New[string](), }, ) actions = client.Actions() @@ -1817,6 +1851,7 @@ func TestCVO_ResetPayloadLoadStatus(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:0"}, }, + EnabledFeatureGates: sets.New[string](), }, ) actions = client.Actions() @@ -2095,6 +2130,7 @@ func TestCVO_InitImplicitlyEnabledCaps(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) actions := client.Actions() @@ -2222,6 +2258,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Actual: configv1.Release{Version: "1.0.1-abc", Image: "image/image:1"}, @@ -2234,6 +2271,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, Failure: payloadErr, }, + EnabledFeatureGates: sets.New[string](), }, ) actions = client.Actions() @@ -2351,6 +2389,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -2436,6 +2475,7 @@ func TestCVO_UpgradeUnverifiedPayloadRetrieveOnce(t *testing.T) { LastTransitionTime: time.Unix(4, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, }, + EnabledFeatureGates: sets.New[string](), }, finalStatusIndicatorCompleted, ) @@ -2510,6 +2550,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Actual: configv1.Release{Version: "1.0.1-abc", Image: "image/image:1"}, @@ -2522,6 +2563,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, Failure: &payload.UpdateError{Reason: "UpgradePreconditionCheckFailed", Message: "Precondition \"TestPrecondition SuccessAfter: 3\" failed because of \"CheckFailure\": failing, attempt: 1 will succeed after 3 attempt", Name: "PreconditionCheck"}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -2611,6 +2653,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Done: 1, @@ -2635,6 +2678,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { LastTransitionTime: time.Unix(2, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Done: 2, @@ -2659,6 +2703,7 @@ func TestCVO_UpgradePreconditionFailing(t *testing.T) { LastTransitionTime: time.Unix(3, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -2785,6 +2830,7 @@ func TestCVO_UpgradePreconditionFailingAcceptedRisks(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Actual: configv1.Release{Version: "1.0.1-abc", Image: "image/image:1"}, @@ -2797,6 +2843,7 @@ func TestCVO_UpgradePreconditionFailingAcceptedRisks(t *testing.T) { Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1"}, Failure: &payload.UpdateError{Reason: "UpgradePreconditionCheckFailed", Message: "Multiple precondition checks failed:\n* Precondition \"PreCondition1\" failed because of \"CheckFailure\": PreCondition1 will always fail.\n* Precondition \"PreCondition2\" failed because of \"CheckFailure\": PreCondition2 will always fail.", Name: "PreconditionCheck"}, }, + EnabledFeatureGates: sets.New[string](), }, ) @@ -2847,6 +2894,7 @@ func TestCVO_UpgradePreconditionFailingAcceptedRisks(t *testing.T) { LastTransitionTime: time.Unix(3, 0), Update: configv1.Update{Version: "1.0.1-abc", Image: "image/image:1", Force: true}, }, + EnabledFeatureGates: sets.New[string](), }, acceptedRisksPopulated, ) @@ -3151,6 +3199,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -3165,6 +3214,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(2, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -3189,6 +3239,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(3, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -3214,6 +3265,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(4, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -3239,6 +3291,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(5, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) client.ClearActions() @@ -3282,6 +3335,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -3306,6 +3360,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(2, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -3331,6 +3386,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(3, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -3356,6 +3412,7 @@ func TestCVO_RestartAndReconcile(t *testing.T) { LastTransitionTime: time.Unix(4, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) client.ClearActions() @@ -3453,6 +3510,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, SyncWorkerStatus{ Reconciling: true, @@ -3467,6 +3525,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { LastTransitionTime: time.Unix(2, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) // verify we haven't observed any other events @@ -3515,6 +3574,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) clearAllStatus(t, worker.StatusCh()) @@ -3551,6 +3611,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) clearAllStatus(t, worker.StatusCh()) @@ -3602,6 +3663,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { LastTransitionTime: time.Unix(1, 0), Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }, ) client.ClearActions() @@ -3771,6 +3833,7 @@ func TestCVO_ParallelError(t *testing.T) { LastTransitionTime: status.loadPayloadStatus.LastTransitionTime, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }) { t.Fatalf("unexpected status: %v", status) } @@ -3807,6 +3870,7 @@ func TestCVO_ParallelError(t *testing.T) { LastTransitionTime: status.loadPayloadStatus.LastTransitionTime, Update: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, }, + EnabledFeatureGates: sets.New[string](), }) { t.Fatalf("unexpected final: %v", status) } diff --git a/pkg/cvo/featuregate_integration_test.go b/pkg/cvo/featuregate_integration_test.go new file mode 100644 index 0000000000..37b4e8e622 --- /dev/null +++ b/pkg/cvo/featuregate_integration_test.go @@ -0,0 +1,277 @@ +package cvo + +import ( + "testing" + + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/library-go/pkg/manifest" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" +) + +// TestFeatureGateManifestFiltering tests the end-to-end feature gate filtering pipeline +func TestFeatureGateManifestFiltering(t *testing.T) { + tests := []struct { + name string + enabledGates sets.Set[string] + manifestAnnotations map[string]string + shouldInclude bool + expectedError string + }{ + { + name: "include manifest with matching feature gate", + enabledGates: sets.New[string]("TechPreviewFeatureGate"), + manifestAnnotations: map[string]string{ + "release.openshift.io/feature-gates": "TechPreviewFeatureGate", + }, + shouldInclude: true, + }, + { + name: "exclude manifest with disabled feature gate", + enabledGates: sets.New[string]("SomeOtherGate"), + manifestAnnotations: map[string]string{ + "release.openshift.io/feature-gates": "TechPreviewFeatureGate", + }, + shouldInclude: false, + expectedError: "feature gate TechPreviewFeatureGate is required but not enabled", + }, + { + name: "include manifest when exclusion gate is disabled", + enabledGates: sets.New[string]("TechPreviewFeatureGate"), + manifestAnnotations: map[string]string{ + "release.openshift.io/feature-gates": "-DisabledFeature", + }, + shouldInclude: true, + }, + { + name: "exclude manifest when exclusion gate is enabled", + enabledGates: sets.New[string]("DisabledFeature"), + manifestAnnotations: map[string]string{ + "release.openshift.io/feature-gates": "-DisabledFeature", + }, + shouldInclude: false, + expectedError: "feature gate DisabledFeature is enabled but manifest requires it to be disabled", + }, + { + name: "complex filtering - AND logic", + enabledGates: sets.New[string]("FeatureA"), + manifestAnnotations: map[string]string{ + "release.openshift.io/feature-gates": "FeatureA,-FeatureB", + }, + shouldInclude: true, + }, + { + name: "complex filtering - failed AND logic", + enabledGates: sets.New[string]("FeatureA", "FeatureB"), + manifestAnnotations: map[string]string{ + "release.openshift.io/feature-gates": "FeatureA,-FeatureB", + }, + shouldInclude: false, + expectedError: "feature gate FeatureB is enabled but manifest requires it to be disabled", + }, + { + name: "manifest with no feature gate annotation", + enabledGates: sets.New[string]("AnyGate"), + manifestAnnotations: map[string]string{ + "some.other.annotation": "value", + }, + shouldInclude: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a mock manifest with the test annotations + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-configmap", + "namespace": "test-namespace", + }, + }, + } + // Use SetAnnotations to ensure proper annotation handling + obj.SetAnnotations(tt.manifestAnnotations) + + manifest := &manifest.Manifest{ + Obj: obj, + } + + // Test the manifest inclusion logic + err := manifest.Include(nil, nil, nil, nil, nil, tt.enabledGates) + + if tt.shouldInclude { + if err != nil { + t.Errorf("Expected manifest to be included, but got error: %v", err) + } + } else { + if err == nil { + t.Error("Expected manifest to be excluded, but no error occurred") + } else if tt.expectedError != "" && err.Error() != tt.expectedError { + t.Errorf("Expected error %q, got %q", tt.expectedError, err.Error()) + } + } + }) + } +} + +// TestSyncWorkIntegration tests that feature gates are properly passed through the SyncWork pipeline +func TestSyncWorkIntegration(t *testing.T) { + work := &SyncWork{ + Generation: 1, + Desired: configv1.Update{Image: "test-image"}, + EnabledFeatureGates: sets.New[string]("TestGate1", "TestGate2"), + } + + // Test that the SyncWork can be used for manifest filtering + testObj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-configmap", + }, + }, + } + testObj.SetAnnotations(map[string]string{ + "release.openshift.io/feature-gates": "TestGate1", + }) + + manifest := &manifest.Manifest{ + Obj: testObj, + } + + err := manifest.Include(nil, nil, nil, nil, nil, work.EnabledFeatureGates) + if err != nil { + t.Errorf("Expected manifest to be included with TestGate1 enabled, got error: %v", err) + } + + // Test with a gate that's not enabled + manifest.Obj.SetAnnotations(map[string]string{ + "release.openshift.io/feature-gates": "DisabledGate", + }) + + err = manifest.Include(nil, nil, nil, nil, nil, work.EnabledFeatureGates) + if err == nil { + t.Error("Expected manifest to be excluded with DisabledGate not enabled, but no error occurred") + } +} + +// TestFeatureGateEventHandling tests the feature gate event handler +func TestFeatureGateEventHandling(t *testing.T) { + // Create a simple operator with feature gate management capabilities + optr := &Operator{ + release: configv1.Release{Version: "4.14.0"}, + } + + // Initialize feature gates + optr.initializeFeatureGates() + + // Test that initial state is empty + gates := optr.getEnabledFeatureGates() + if gates.Len() != 0 { + t.Errorf("Expected empty initial feature gates, got %v", sets.List(gates)) + } + + // Test updating with a FeatureGate object + featureGate := &configv1.FeatureGate{ + Status: configv1.FeatureGateStatus{ + FeatureGates: []configv1.FeatureGateDetails{ + { + Version: "4.14.0", + Enabled: []configv1.FeatureGateAttributes{ + {Name: "NewFeature"}, + {Name: "ExperimentalFeature"}, + }, + }, + }, + }, + } + + optr.updateEnabledFeatureGates(featureGate) + + // Verify gates were updated + gates = optr.getEnabledFeatureGates() + expected := sets.New[string]("NewFeature", "ExperimentalFeature") + if !gates.Equal(expected) { + t.Errorf("After update, feature gates = %v, expected %v", sets.List(gates), sets.List(expected)) + } +} + +// TestManifestFilteringExamples tests real-world usage examples +func TestManifestFilteringExamples(t *testing.T) { + examples := []struct { + name string + description string + enabledGates sets.Set[string] + manifestAnnotation string + shouldInclude bool + }{ + { + name: "TechPreview feature deployment", + description: "Deploy experimental ConfigMap only in TechPreviewFeatureGate enabled clusters", + enabledGates: sets.New("TechPreviewFeatureGate"), + manifestAnnotation: "TechPreviewFeatureGate", + shouldInclude: true, + }, + { + name: "Production cluster excludes TechPreview", + description: "Production cluster should exclude TechPreviewFeatureGate enabled manifests", + enabledGates: sets.New[string](), + manifestAnnotation: "TechPreviewFeatureGate", + shouldInclude: false, + }, + { + name: "Alternative implementation selection", + description: "Use new storage API when enabled, exclude legacy API", + enabledGates: sets.New("NewStorageAPI"), + manifestAnnotation: "NewStorageAPI,-LegacyStorageAPI", + shouldInclude: true, + }, + { + name: "Legacy implementation when new API disabled", + description: "Use legacy implementation when new API is not enabled", + enabledGates: sets.New[string](), + manifestAnnotation: "-NewStorageAPI", + shouldInclude: true, + }, + } + + for _, example := range examples { + t.Run(example.name, func(t *testing.T) { + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "example-manifest", + }, + }, + } + // Use SetAnnotations to ensure proper annotation handling + obj.SetAnnotations(map[string]string{ + "release.openshift.io/feature-gates": example.manifestAnnotation, + }) + + manifest := &manifest.Manifest{ + Obj: obj, + } + + err := manifest.Include(nil, nil, nil, nil, nil, example.enabledGates) + + if example.shouldInclude { + if err != nil { + t.Errorf("%s: Expected manifest to be included, but got error: %v", + example.description, err) + } + } else { + if err == nil { + t.Errorf("%s: Expected manifest to be excluded, but no error occurred", + example.description) + } + } + }) + } +} diff --git a/pkg/cvo/sync_test.go b/pkg/cvo/sync_test.go index 36c259db84..e660f4257f 100644 --- a/pkg/cvo/sync_test.go +++ b/pkg/cvo/sync_test.go @@ -20,6 +20,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" dynamicfake "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/rest" clientgotesting "k8s.io/client-go/testing" @@ -493,7 +494,7 @@ func (r *fakeSyncRecorder) NotifyAboutManagedResourceActivity(message string) { func (r *fakeSyncRecorder) Start(ctx context.Context, maxWorkers int) { } -func (r *fakeSyncRecorder) Update(ctx context.Context, generation int64, desired configv1.Update, config *configv1.ClusterVersion, state payload.State) *SyncWorkerStatus { +func (r *fakeSyncRecorder) Update(ctx context.Context, generation int64, desired configv1.Update, config *configv1.ClusterVersion, state payload.State, enabledFeatureGates sets.Set[string]) *SyncWorkerStatus { r.Updates = append(r.Updates, desired) return r.Returns } diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index b0a6d756da..f054e43d04 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -33,7 +33,7 @@ import ( // ConfigSyncWorker abstracts how the image is synchronized to the server. Introduced for testing. type ConfigSyncWorker interface { Start(ctx context.Context, maxWorkers int) - Update(ctx context.Context, generation int64, desired configv1.Update, config *configv1.ClusterVersion, state payload.State) *SyncWorkerStatus + Update(ctx context.Context, generation int64, desired configv1.Update, config *configv1.ClusterVersion, state payload.State, enabledFeatureGates sets.Set[string]) *SyncWorkerStatus StatusCh() <-chan SyncWorkerStatus // NotifyAboutManagedResourceActivity informs the sync worker about activity for a managed resource. @@ -78,6 +78,10 @@ type SyncWork struct { Attempt int Capabilities capability.ClusterCapabilities + + // EnabledFeatureGates contains the set of feature gate names that are currently enabled + // This is derived from the cluster's FeatureGate resource and used for manifest filtering + EnabledFeatureGates sets.Set[string] } // Empty returns true if the image is empty for this work. @@ -126,11 +130,20 @@ type SyncWorkerStatus struct { loadPayloadStatus LoadPayloadStatus CapabilitiesStatus CapabilityStatus + + // EnabledFeatureGates contains the set of feature gate names that are currently enabled + // and being used for manifest filtering during sync operations + EnabledFeatureGates sets.Set[string] } // DeepCopy copies the worker status. func (w SyncWorkerStatus) DeepCopy() *SyncWorkerStatus { - return &w + copy := w + + // Provide a proper deep copy for feature gates since this is a list. + copy.EnabledFeatureGates = w.EnabledFeatureGates.Clone() + + return © } // SyncWorker retrieves and applies the desired image, tracking the status for the parent to @@ -339,7 +352,7 @@ func (w *SyncWorker) syncPayload(ctx context.Context, work *SyncWork) ([]configv // Capability filtering is not done here since unknown capabilities are allowed // during updated payload load and enablement checking only occurs during apply. - payloadUpdate, err := payload.LoadUpdate(info.Directory, desired.Image, w.exclude, string(w.requiredFeatureSet), w.clusterProfile, nil) + payloadUpdate, err := payload.LoadUpdate(info.Directory, desired.Image, w.exclude, string(w.requiredFeatureSet), w.clusterProfile, nil, work.EnabledFeatureGates) if err != nil { msg := fmt.Sprintf("Loading payload failed version=%q image=%q failure=%v", desired.Version, desired.Image, err) @@ -415,7 +428,7 @@ func (w *SyncWorker) syncPayload(ctx context.Context, work *SyncWork) ([]configv } if w.payload != nil { implicitlyEnabledCaps = capability.SortedList(payload.GetImplicitlyEnabledCapabilities(payloadUpdate.Manifests, w.payload.Manifests, - work.Capabilities)) + work.Capabilities, work.EnabledFeatureGates)) } w.payload = payloadUpdate msg = fmt.Sprintf("Payload loaded version=%q image=%q architecture=%q", desired.Version, desired.Image, @@ -458,15 +471,16 @@ func (w *SyncWorker) loadUpdatedPayload(ctx context.Context, work *SyncWork) ([] // // Acquires the SyncWorker lock, so it must not be locked when Update is called func (w *SyncWorker) Update(ctx context.Context, generation int64, desired configv1.Update, config *configv1.ClusterVersion, - state payload.State) *SyncWorkerStatus { + state payload.State, enabledFeatureGates sets.Set[string]) *SyncWorkerStatus { w.lock.Lock() defer w.lock.Unlock() work := &SyncWork{ - Generation: generation, - Desired: desired, - Overrides: config.Spec.Overrides, + Generation: generation, + Desired: desired, + Overrides: config.Spec.Overrides, + EnabledFeatureGates: enabledFeatureGates, } var priorCaps sets.Set[configv1.ClusterVersionCapability] @@ -490,13 +504,13 @@ func (w *SyncWorker) Update(ctx context.Context, generation int64, desired confi ensureEnabledCapabilities := append(slices.Collect(maps.Keys(priorCaps)), w.alwaysEnableCapabilities...) work.Capabilities = capability.SetCapabilities(config, ensureEnabledCapabilities) - versionEqual, overridesEqual, capabilitiesEqual := + versionEqual, overridesEqual, capabilitiesEqual, featureGatesEqual := equalSyncWork(w.work, work, fmt.Sprintf("considering cluster version generation %d", generation)) // needs to be set here since changes in implicitly enabled capabilities are not considered a "capabilities change" w.status.CapabilitiesStatus.ImplicitlyEnabledCaps = capability.SortedList(work.Capabilities.ImplicitlyEnabled) - if versionEqual && overridesEqual && capabilitiesEqual { + if versionEqual && overridesEqual && capabilitiesEqual && featureGatesEqual { klog.V(2).Info("Update work is equal to current target; no change required") if !equalUpdate(w.work.Desired, w.status.loadPayloadStatus.Update) { @@ -520,6 +534,8 @@ func (w *SyncWorker) Update(ctx context.Context, generation int64, desired confi Version: work.Desired.Version, Image: work.Desired.Image, } + // Initialize feature gates in status + w.status.EnabledFeatureGates = work.EnabledFeatureGates.Clone() } else { oldDesired = &w.work.Desired } @@ -539,7 +555,9 @@ func (w *SyncWorker) Update(ctx context.Context, generation int64, desired confi if w.work != nil { w.work.Overrides = config.Spec.Overrides w.work.Capabilities = work.Capabilities + w.work.EnabledFeatureGates = work.EnabledFeatureGates w.status.CapabilitiesStatus.Status = capability.GetCapabilitiesStatus(w.work.Capabilities) + w.status.EnabledFeatureGates = work.EnabledFeatureGates.Clone() } return w.status.DeepCopy() } @@ -557,7 +575,8 @@ func (w *SyncWorker) Update(ctx context.Context, generation int64, desired confi w.status.CapabilitiesStatus.ImplicitlyEnabledCaps = capability.SortedList(w.work.Capabilities.ImplicitlyEnabled) w.status.CapabilitiesStatus.Status = capability.GetCapabilitiesStatus(w.work.Capabilities) - // Update syncWorker status with architecture of newly loaded payload. + // Update syncWorker status with feature gates and architecture of newly loaded payload. + w.status.EnabledFeatureGates = w.work.EnabledFeatureGates.Clone() w.status.Architecture = w.payload.Architecture // notify the sync loop that we changed config @@ -763,8 +782,8 @@ func (w *statusWrapper) Report(status SyncWorkerStatus) { // time work transitions from empty to not empty (as a result of someone invoking // Update). func (w *SyncWork) calculateNextFrom(desired *SyncWork) bool { - sameVersion, sameOverrides, sameCapabilities := equalSyncWork(w, desired, "calculating next work") - changed := !(sameVersion && sameOverrides && sameCapabilities) + sameVersion, sameOverrides, sameCapabilities, sameFeatureGates := equalSyncWork(w, desired, "calculating next work") + changed := !(sameVersion && sameOverrides && sameCapabilities && sameFeatureGates) // if this is the first time through the loop, initialize reconciling to // the state Update() calculated (to allow us to start in reconciling) @@ -820,20 +839,21 @@ func splitDigest(pullspec string) string { } // equalSyncWork returns indications of whether release version has changed, whether overrides have changed, -// and whether capabilities have changed. -func equalSyncWork(a, b *SyncWork, context string) (equalVersion, equalOverrides, equalCapabilities bool) { +// whether capabilities have changed, and whether enabled feature gates have changed. +func equalSyncWork(a, b *SyncWork, context string) (equalVersion, equalOverrides, equalCapabilities, equalFeatureGates bool) { // if both `a` and `b` are the same then simply return true if a == b { - return true, true, true + return true, true, true, true } // if either `a` or `b` are nil then return false if a == nil || b == nil { - return false, false, false + return false, false, false, false } sameVersion := equalUpdate(a.Desired, b.Desired) sameOverrides := reflect.DeepEqual(a.Overrides, b.Overrides) capabilitiesError := a.Capabilities.Equal(&b.Capabilities) + sameFeatureGates := a.EnabledFeatureGates.Equal(b.EnabledFeatureGates) var msgs []string if !sameVersion { @@ -845,10 +865,14 @@ func equalSyncWork(a, b *SyncWork, context string) (equalVersion, equalOverrides if capabilitiesError != nil { msgs = append(msgs, fmt.Sprintf("capabilities changed (%v)", capabilitiesError)) } + if !sameFeatureGates { + msgs = append(msgs, fmt.Sprintf("enabled feature gates changed (from %v to %v)", + sets.List(a.EnabledFeatureGates), sets.List(b.EnabledFeatureGates))) + } if len(msgs) > 0 { klog.V(2).Infof("Detected while %s: %s", context, strings.Join(msgs, ", ")) } - return sameVersion, sameOverrides, capabilitiesError == nil + return sameVersion, sameOverrides, capabilitiesError == nil, sameFeatureGates } // updateApplyStatus records the current status of the payload apply sync action for @@ -1020,7 +1044,7 @@ func (w *SyncWorker) apply(ctx context.Context, work *SyncWork, maxWorkers int, if task.Manifest.GVK != configv1.GroupVersion.WithKind("ClusterOperator") { continue } - if err := task.Manifest.Include(nil, nil, nil, &capabilities, work.Overrides); err != nil { + if err := task.Manifest.Include(nil, nil, nil, &capabilities, work.Overrides, work.EnabledFeatureGates); err != nil { klog.V(manifestVerbosity).Infof("Skipping precreation of %s: %s", task, err) continue } @@ -1040,7 +1064,7 @@ func (w *SyncWorker) apply(ctx context.Context, work *SyncWork, maxWorkers int, klog.V(manifestVerbosity).Infof("Running sync for %s", task) - if err := task.Manifest.Include(nil, nil, nil, &capabilities, work.Overrides); err != nil { + if err := task.Manifest.Include(nil, nil, nil, &capabilities, work.Overrides, work.EnabledFeatureGates); err != nil { klog.V(manifestVerbosity).Infof("Skipping %s: %s", task, err) continue } @@ -1117,10 +1141,10 @@ func (r *consistentReporter) Update() { defer r.lock.Unlock() metricPayload.WithLabelValues(r.version, "pending").Set(float64(r.total - r.done)) metricPayload.WithLabelValues(r.version, "applied").Set(float64(r.done)) - copied := r.status + copied := r.status.DeepCopy() copied.Done = r.done copied.Total = r.total - r.reporter.Report(copied) + r.reporter.Report(*copied) } // Errors updates the status based on the current state of the graph runner. @@ -1131,13 +1155,13 @@ func (r *consistentReporter) Errors(errs []error) error { r.lock.Lock() defer r.lock.Unlock() - copied := r.status + copied := r.status.DeepCopy() copied.Done = r.done copied.Total = r.total if err != nil { copied.Failure = err } - r.reporter.Report(copied) + r.reporter.Report(*copied) return err } @@ -1155,13 +1179,13 @@ func (r *consistentReporter) Complete() { defer r.lock.Unlock() metricPayload.WithLabelValues(r.version, "pending").Set(float64(r.total - r.done)) metricPayload.WithLabelValues(r.version, "applied").Set(float64(r.done)) - copied := r.status + copied := r.status.DeepCopy() copied.Completed = r.completed + 1 copied.Initial = false copied.Reconciling = true copied.Done = r.done copied.Total = r.total - r.reporter.Report(copied) + r.reporter.Report(*copied) } func isContextError(err error) bool { diff --git a/pkg/payload/payload.go b/pkg/payload/payload.go index 71c66f3bda..7b72ce4b5f 100644 --- a/pkg/payload/payload.go +++ b/pkg/payload/payload.go @@ -136,7 +136,7 @@ type metadata struct { } func LoadUpdate(dir, releaseImage, excludeIdentifier string, requiredFeatureSet string, profile string, - knownCapabilities []configv1.ClusterVersionCapability) (*Update, error) { + knownCapabilities []configv1.ClusterVersionCapability, enabledFeatureGates sets.Set[string]) (*Update, error) { klog.V(2).Infof("Loading updatepayload from %q", dir) if err := ValidateDirectory(dir); err != nil { return nil, err @@ -211,7 +211,7 @@ func LoadUpdate(dir, releaseImage, excludeIdentifier string, requiredFeatureSet filteredMs := []manifest.Manifest{} for _, manifest := range ms { - if err := manifest.Include(&excludeIdentifier, &requiredFeatureSet, &profile, onlyKnownCaps, nil); err != nil { + if err := manifest.Include(&excludeIdentifier, &requiredFeatureSet, &profile, onlyKnownCaps, nil, enabledFeatureGates); err != nil { klog.V(2).Infof("excluding %s: %v\n", manifest.String(), err) continue } @@ -243,13 +243,14 @@ func LoadUpdate(dir, releaseImage, excludeIdentifier string, requiredFeatureSet // the current payload the updated manifest's capabilities are checked to see if any must be implicitly enabled. // All capabilities requiring implicit enablement are returned. func GetImplicitlyEnabledCapabilities(updatePayloadManifests []manifest.Manifest, currentPayloadManifests []manifest.Manifest, - capabilities capability.ClusterCapabilities) sets.Set[configv1.ClusterVersionCapability] { + capabilities capability.ClusterCapabilities, enabledFeatureGates sets.Set[string]) sets.Set[configv1.ClusterVersionCapability] { clusterCaps := capability.GetCapabilitiesStatus(capabilities) return localmanifest.GetImplicitlyEnabledCapabilities( updatePayloadManifests, currentPayloadManifests, localmanifest.InclusionConfiguration{Capabilities: &clusterCaps}, capabilities.ImplicitlyEnabled, + enabledFeatureGates, ) } diff --git a/pkg/payload/payload_test.go b/pkg/payload/payload_test.go index d828d02b72..21730fd23f 100644 --- a/pkg/payload/payload_test.go +++ b/pkg/payload/payload_test.go @@ -128,7 +128,7 @@ func TestLoadUpdate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := LoadUpdate(tt.args.dir, tt.args.releaseImage, "exclude-test", "", DefaultClusterProfile, nil) + got, err := LoadUpdate(tt.args.dir, tt.args.releaseImage, "exclude-test", "", DefaultClusterProfile, nil, sets.Set[string]{}) if (err != nil) != tt.wantErr { t.Errorf("loadUpdatePayload() error = %v, wantErr %v", err, tt.wantErr) return @@ -203,7 +203,7 @@ func TestLoadUpdateArchitecture(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := LoadUpdate(tt.args.dir, tt.args.releaseImage, "exclude-test", "", DefaultClusterProfile, nil) + got, err := LoadUpdate(tt.args.dir, tt.args.releaseImage, "exclude-test", "", DefaultClusterProfile, nil, sets.Set[string]{}) if (err != nil) != tt.wantErr { t.Errorf("loadUpdatePayload() error = %v, wantErr %v", err, tt.wantErr) return @@ -365,7 +365,7 @@ func TestGetImplicitlyEnabledCapabilities(t *testing.T) { if tt.pathExt == "test10" { updateMans = append(updateMans, updateMans[0]) } - caps := GetImplicitlyEnabledCapabilities(updateMans, currentMans, tt.capabilities) + caps := GetImplicitlyEnabledCapabilities(updateMans, currentMans, tt.capabilities, sets.Set[string]{}) if diff := cmp.Diff(tt.wantImplicit, caps); diff != "" { t.Errorf("%s: wantImplicit differs from expected:\n%s", tt.name, diff) } diff --git a/pkg/payload/render.go b/pkg/payload/render.go index 283c4dca72..79fa2aefcb 100644 --- a/pkg/payload/render.go +++ b/pkg/payload/render.go @@ -163,7 +163,7 @@ func renderDir(renderConfig manifestRenderConfig, idir, odir string, requiredFea for _, manifest := range manifests { if !filterGroupKind.Has(manifest.GVK.GroupKind()) { klog.Infof("excluding %s because we do not render that group/kind", manifest.String()) - } else if err := manifest.Include(nil, requiredFeatureSet, clusterProfile, nil, nil); err != nil { + } else if err := manifest.Include(nil, requiredFeatureSet, clusterProfile, nil, nil, sets.Set[string]{} /* Empty, assuming any gated manifest will be applied later by the real CVO */); err != nil { klog.Infof("excluding %s: %v", manifest.String(), err) } else { filteredManifests = append(filteredManifests, string(manifest.Raw)) diff --git a/pkg/payload/task_graph_test.go b/pkg/payload/task_graph_test.go index 048bd93399..2e0d2208f9 100644 --- a/pkg/payload/task_graph_test.go +++ b/pkg/payload/task_graph_test.go @@ -16,6 +16,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/openshift/library-go/pkg/manifest" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" ) func Test_TaskGraph_Split(t *testing.T) { @@ -487,7 +488,7 @@ func Test_TaskGraph_real(t *testing.T) { if len(path) == 0 { t.Skip("TEST_GRAPH_PATH unset") } - p, err := LoadUpdate(path, "arbitrary/image:1", "", "", DefaultClusterProfile, nil) + p, err := LoadUpdate(path, "arbitrary/image:1", "", "", DefaultClusterProfile, nil, sets.Set[string]{}) if err != nil { t.Fatal(err) } diff --git a/pkg/start/start.go b/pkg/start/start.go index 9568a39c97..070e796365 100644 --- a/pkg/start/start.go +++ b/pkg/start/start.go @@ -608,6 +608,7 @@ func (o *Options) NewControllerContext( openshiftConfigManagedInformerFactory.Core().V1().ConfigMaps(), configInformerFactory.Config().V1().Proxies(), operatorInformerFactory, + configInformerFactory.Config().V1().FeatureGates(), cb.ClientOrDie(o.Namespace), cvoKubeClient, operatorClient, From dad68cee087ffcaf8d616bb4341082db263b6d20 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 17 Dec 2025 16:44:43 +0000 Subject: [PATCH 03/12] DNM - Add manifests to test inclusion --- ...luster-version-operator_03_configmaps.yaml | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 install/0000_90_cluster-version-operator_03_configmaps.yaml diff --git a/install/0000_90_cluster-version-operator_03_configmaps.yaml b/install/0000_90_cluster-version-operator_03_configmaps.yaml new file mode 100644 index 0000000000..eb47ed52e0 --- /dev/null +++ b/install/0000_90_cluster-version-operator_03_configmaps.yaml @@ -0,0 +1,32 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: dev-preview-created + namespace: openshift-cluster-version + annotations: + include.release.openshift.io/self-managed-high-availability: "true" + release.openshift.io/feature-gates: "Example,Example2" +data: + example: "this should be created on dev-preview-only as it's annotated with Example and Example2" +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: tech-preview-created + namespace: openshift-cluster-version + annotations: + include.release.openshift.io/self-managed-high-availability: "true" + release.openshift.io/feature-gates: "Example,-Example2" +data: + example: "this should be created on tech-preview-only as it's annotated with Example and not Example2" +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: default-created + namespace: openshift-cluster-version + annotations: + include.release.openshift.io/self-managed-high-availability: "true" + release.openshift.io/feature-gates: "-Example,-Example2" +data: + example: "this should be created on default-only as it's annotated with negation for Example and Example2" From ed06ae420a3ec1f0ce0134cd54b562686d3d7a08 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Dec 2025 10:25:39 +0000 Subject: [PATCH 04/12] Add mutex around operator feature gates read/write --- pkg/cvo/cvo.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index cdf653ee18..cd8ec2dc34 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -191,6 +191,8 @@ type Operator struct { // featurechangestopper controller will detect when cluster feature gate config changes and shutdown the CVO. enabledFeatureGates featuregates.CvoGateChecker + // featureGatesMutex protects access to enabledManifestFeatureGates + featureGatesMutex sync.RWMutex enabledManifestFeatureGates sets.Set[string] clusterProfile string @@ -1115,6 +1117,9 @@ func (optr *Operator) featureGateEventHandler() cache.ResourceEventHandler { // initializeFeatureGates initializes the cluster feature gates from the current FeatureGate object func (optr *Operator) initializeFeatureGates() { + optr.featureGatesMutex.Lock() + defer optr.featureGatesMutex.Unlock() + optr.enabledManifestFeatureGates = sets.Set[string]{} // Try to load initial state from the cluster FeatureGate object @@ -1135,6 +1140,9 @@ func (optr *Operator) updateEnabledFeatureGates(obj interface{}) { newGates := optr.extractEnabledGates(featureGate) + optr.featureGatesMutex.Lock() + defer optr.featureGatesMutex.Unlock() + // Check if gates actually changed to avoid unnecessary work if !optr.enabledManifestFeatureGates.Equal(newGates) { klog.V(2).Infof("Cluster feature gates changed from %v to %v", @@ -1145,6 +1153,9 @@ func (optr *Operator) updateEnabledFeatureGates(obj interface{}) { // getEnabledFeatureGates returns a copy of the current cluster feature gates for safe consumption func (optr *Operator) getEnabledFeatureGates() sets.Set[string] { + optr.featureGatesMutex.RLock() + defer optr.featureGatesMutex.RUnlock() + // Return a copy to prevent external modification result := sets.Set[string]{} for gate := range optr.enabledManifestFeatureGates { From d206d706f245c2311f1f75dc3961c5d6018d8c18 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Dec 2025 13:10:24 +0000 Subject: [PATCH 05/12] Fix feature gate extraction versioning --- pkg/cvo/cvo.go | 16 ++++++---------- pkg/cvo/cvo_featuregates_test.go | 6 ++++++ pkg/cvo/featuregate_integration_test.go | 3 +++ pkg/cvo/status_test.go | 8 +++++++- pkg/featuregates/featuregates.go | 8 ++++++++ 5 files changed, 30 insertions(+), 11 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index cd8ec2dc34..06e03040c7 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -1117,15 +1117,10 @@ func (optr *Operator) featureGateEventHandler() cache.ResourceEventHandler { // initializeFeatureGates initializes the cluster feature gates from the current FeatureGate object func (optr *Operator) initializeFeatureGates() { - optr.featureGatesMutex.Lock() - defer optr.featureGatesMutex.Unlock() - - optr.enabledManifestFeatureGates = sets.Set[string]{} - // Try to load initial state from the cluster FeatureGate object if optr.featureGateLister != nil { if featureGate, err := optr.featureGateLister.Get("cluster"); err == nil { - optr.enabledManifestFeatureGates = optr.extractEnabledGates(featureGate) + optr.updateEnabledFeatureGates(featureGate) } } } @@ -1140,13 +1135,14 @@ func (optr *Operator) updateEnabledFeatureGates(obj interface{}) { newGates := optr.extractEnabledGates(featureGate) - optr.featureGatesMutex.Lock() - defer optr.featureGatesMutex.Unlock() - // Check if gates actually changed to avoid unnecessary work if !optr.enabledManifestFeatureGates.Equal(newGates) { + optr.featureGatesMutex.Lock() + defer optr.featureGatesMutex.Unlock() + klog.V(2).Infof("Cluster feature gates changed from %v to %v", sets.List(optr.enabledManifestFeatureGates), sets.List(newGates)) + optr.enabledManifestFeatureGates = newGates } } @@ -1169,7 +1165,7 @@ func (optr *Operator) extractEnabledGates(featureGate *configv1.FeatureGate) set enabledGates := sets.Set[string]{} // Find the feature gate details for the current cluster version - currentVersion := optr.release.Version + currentVersion := optr.enabledFeatureGates.DesiredVersion() for _, details := range featureGate.Status.FeatureGates { if details.Version == currentVersion { for _, enabled := range details.Enabled { diff --git a/pkg/cvo/cvo_featuregates_test.go b/pkg/cvo/cvo_featuregates_test.go index aad0c559da..be4b43f419 100644 --- a/pkg/cvo/cvo_featuregates_test.go +++ b/pkg/cvo/cvo_featuregates_test.go @@ -76,6 +76,9 @@ func TestOperator_extractEnabledGates(t *testing.T) { t.Run(tt.name, func(t *testing.T) { optr := &Operator{ release: tt.release, + enabledFeatureGates: fakeRiFlags{ + desiredVersion: tt.release.Version, + }, } result := optr.extractEnabledGates(tt.featureGate) @@ -147,6 +150,9 @@ func TestOperator_updateEnabledFeatureGates(t *testing.T) { optr := &Operator{ enabledManifestFeatureGates: sets.New[string]("oldgate"), release: configv1.Release{Version: "4.14.0"}, + enabledFeatureGates: fakeRiFlags{ + desiredVersion: "4.14.0", + }, } originalGates := optr.getEnabledFeatureGates() diff --git a/pkg/cvo/featuregate_integration_test.go b/pkg/cvo/featuregate_integration_test.go index 37b4e8e622..4d280634ce 100644 --- a/pkg/cvo/featuregate_integration_test.go +++ b/pkg/cvo/featuregate_integration_test.go @@ -164,6 +164,9 @@ func TestFeatureGateEventHandling(t *testing.T) { // Create a simple operator with feature gate management capabilities optr := &Operator{ release: configv1.Release{Version: "4.14.0"}, + enabledFeatureGates: fakeRiFlags{ + desiredVersion: "4.14.0", + }, } // Initialize feature gates diff --git a/pkg/cvo/status_test.go b/pkg/cvo/status_test.go index 1e1711c3c6..0d32740a0c 100644 --- a/pkg/cvo/status_test.go +++ b/pkg/cvo/status_test.go @@ -3,11 +3,12 @@ package cvo import ( "context" "fmt" - "github.com/openshift/cluster-version-operator/pkg/internal" "reflect" "testing" "time" + "github.com/openshift/cluster-version-operator/pkg/internal" + "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/openshift/cluster-version-operator/pkg/payload" @@ -200,11 +201,16 @@ func TestOperator_syncFailingStatus(t *testing.T) { } type fakeRiFlags struct { + desiredVersion string unknownVersion bool statusReleaseArchitecture bool cvoConfiguration bool } +func (f fakeRiFlags) DesiredVersion() string { + return f.desiredVersion +} + func (f fakeRiFlags) UnknownVersion() bool { return f.unknownVersion } diff --git a/pkg/featuregates/featuregates.go b/pkg/featuregates/featuregates.go index 27fd627c3f..5768370f8b 100644 --- a/pkg/featuregates/featuregates.go +++ b/pkg/featuregates/featuregates.go @@ -17,6 +17,10 @@ const StubOpenShiftVersion = "0.0.1-snapshot" // CvoGateChecker allows CVO code to check which feature gates are enabled type CvoGateChecker interface { + // DesiredVersion returns the version of the CVO that is currently executing. This is used to determine + // the feature gates that are relevant for the current version of the CVO. + DesiredVersion() string + // UnknownVersion flag is set to true if CVO did not find a matching version in the FeatureGate // status resource, meaning the current set of enabled and disabled feature gates is unknown for // this version. This should be a temporary state (config-operator should eventually add the @@ -51,6 +55,10 @@ type CvoGates struct { cvoConfiguration bool } +func (c CvoGates) DesiredVersion() string { + return c.desiredVersion +} + func (c CvoGates) StatusReleaseArchitecture() bool { return c.statusReleaseArchitecture } From cc44477ad03ffa66faafee96d06e85f0291da8d3 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Dec 2025 13:59:18 +0000 Subject: [PATCH 06/12] Only reconcile when feature gates actually change --- pkg/cvo/cvo.go | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index 06e03040c7..ed54162c58 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -1105,12 +1105,14 @@ func (optr *Operator) featureGateEventHandler() cache.ResourceEventHandler { workQueueKey := optr.queueKey() return cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - optr.updateEnabledFeatureGates(obj) - optr.queue.Add(workQueueKey) + if optr.updateEnabledFeatureGates(obj) { + optr.queue.Add(workQueueKey) + } }, UpdateFunc: func(old, new interface{}) { - optr.updateEnabledFeatureGates(new) - optr.queue.Add(workQueueKey) + if optr.updateEnabledFeatureGates(new) { + optr.queue.Add(workQueueKey) + } }, } } @@ -1126,31 +1128,35 @@ func (optr *Operator) initializeFeatureGates() { } // updateEnabledFeatureGates updates the cluster feature gates based on a FeatureGate object -func (optr *Operator) updateEnabledFeatureGates(obj interface{}) { +func (optr *Operator) updateEnabledFeatureGates(obj interface{}) bool { featureGate, ok := obj.(*configv1.FeatureGate) if !ok { klog.Warningf("Expected FeatureGate object but got %T", obj) - return + return false } newGates := optr.extractEnabledGates(featureGate) // Check if gates actually changed to avoid unnecessary work if !optr.enabledManifestFeatureGates.Equal(newGates) { - optr.featureGatesMutex.Lock() - defer optr.featureGatesMutex.Unlock() + // optr.featureGatesMutex.Lock() + // defer optr.featureGatesMutex.Unlock() klog.V(2).Infof("Cluster feature gates changed from %v to %v", sets.List(optr.enabledManifestFeatureGates), sets.List(newGates)) optr.enabledManifestFeatureGates = newGates + + return true } + + return false } // getEnabledFeatureGates returns a copy of the current cluster feature gates for safe consumption func (optr *Operator) getEnabledFeatureGates() sets.Set[string] { - optr.featureGatesMutex.RLock() - defer optr.featureGatesMutex.RUnlock() + // optr.featureGatesMutex.RLock() + // defer optr.featureGatesMutex.RUnlock() // Return a copy to prevent external modification result := sets.Set[string]{} From 714d49f407831107b9523acf8f035bc7797da087 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Dec 2025 15:35:45 +0000 Subject: [PATCH 07/12] Should be using clone on the featuregates --- pkg/cvo/sync_worker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index f054e43d04..0d98b7a265 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -555,7 +555,7 @@ func (w *SyncWorker) Update(ctx context.Context, generation int64, desired confi if w.work != nil { w.work.Overrides = config.Spec.Overrides w.work.Capabilities = work.Capabilities - w.work.EnabledFeatureGates = work.EnabledFeatureGates + w.work.EnabledFeatureGates = work.EnabledFeatureGates.Clone() w.status.CapabilitiesStatus.Status = capability.GetCapabilitiesStatus(w.work.Capabilities) w.status.EnabledFeatureGates = work.EnabledFeatureGates.Clone() } From 309f0be252b3afc1f74f544c360d29c35e2b1a0a Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Dec 2025 16:08:13 +0000 Subject: [PATCH 08/12] Oops --- pkg/cvo/cvo.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index ed54162c58..df1b7de953 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -1139,8 +1139,8 @@ func (optr *Operator) updateEnabledFeatureGates(obj interface{}) bool { // Check if gates actually changed to avoid unnecessary work if !optr.enabledManifestFeatureGates.Equal(newGates) { - // optr.featureGatesMutex.Lock() - // defer optr.featureGatesMutex.Unlock() + optr.featureGatesMutex.Lock() + defer optr.featureGatesMutex.Unlock() klog.V(2).Infof("Cluster feature gates changed from %v to %v", sets.List(optr.enabledManifestFeatureGates), sets.List(newGates)) @@ -1155,8 +1155,8 @@ func (optr *Operator) updateEnabledFeatureGates(obj interface{}) bool { // getEnabledFeatureGates returns a copy of the current cluster feature gates for safe consumption func (optr *Operator) getEnabledFeatureGates() sets.Set[string] { - // optr.featureGatesMutex.RLock() - // defer optr.featureGatesMutex.RUnlock() + optr.featureGatesMutex.RLock() + defer optr.featureGatesMutex.RUnlock() // Return a copy to prevent external modification result := sets.Set[string]{} From f46468a4ce5a9e32b2481722ef64bd598ad0d400 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Dec 2025 18:13:57 +0000 Subject: [PATCH 09/12] Enable sync worker to refresh payload when a change occurs --- pkg/cvo/sync_worker.go | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index 0d98b7a265..9442466d03 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -647,8 +647,27 @@ func (w *SyncWorker) Start(ctx context.Context, maxWorkers int) { // determine whether we need to do work w.lock.Lock() - changed := work.calculateNextFrom(w.work) + changed, featureGatesChanged := work.calculateNextFrom(w.work) + + if featureGatesChanged { + // When the feature gates change, we must reload the payload. + // Loading the payload fiters out files that didn't match the previous set of feature gates, + // this means now, additional files may match the new set of feature gates and need to be included. + // Some files in the current payload may no longer match the new set of feature gates and need to be excluded, + // though these ones are already excluded when apply calls Include on the manifests. + klog.V(2).Infof("Feature gates changed, loading updated payload") + + // Clear the payload to force a reload. + w.payload = nil + + _, err := w.loadUpdatedPayload(ctx, w.work) + if err != nil { + klog.Warningf("Error when attempting to load updated payload: %v.", err) + } + } + w.lock.Unlock() + if !changed && waitingToReconcile { klog.V(2).Infof("No change, waiting") continue @@ -781,7 +800,7 @@ func (w *statusWrapper) Report(status SyncWorkerStatus) { // returns true if any changes were made. The reconciling flag is set the first // time work transitions from empty to not empty (as a result of someone invoking // Update). -func (w *SyncWork) calculateNextFrom(desired *SyncWork) bool { +func (w *SyncWork) calculateNextFrom(desired *SyncWork) (bool, bool) { sameVersion, sameOverrides, sameCapabilities, sameFeatureGates := equalSyncWork(w, desired, "calculating next work") changed := !(sameVersion && sameOverrides && sameCapabilities && sameFeatureGates) @@ -803,8 +822,9 @@ func (w *SyncWork) calculateNextFrom(desired *SyncWork) bool { } w.Generation = desired.Generation + w.EnabledFeatureGates = desired.EnabledFeatureGates.Clone() - return changed + return changed, !sameFeatureGates } // equalUpdate returns true if two updates are semantically equivalent. From 0f4973fc719e598ca60d6eb608b77f77e6598b0d Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Dec 2025 18:16:33 +0000 Subject: [PATCH 10/12] Remove nonsense test --- pkg/cvo/cvo_featuregates_test.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/pkg/cvo/cvo_featuregates_test.go b/pkg/cvo/cvo_featuregates_test.go index be4b43f419..d4d5e648fa 100644 --- a/pkg/cvo/cvo_featuregates_test.go +++ b/pkg/cvo/cvo_featuregates_test.go @@ -174,18 +174,3 @@ func TestOperator_updateEnabledFeatureGates(t *testing.T) { }) } } - -func TestOperator_initializeFeatureGates(t *testing.T) { - // This test would require mocking the featureGateLister - // For now, test that the method doesn't panic and initializes empty set - optr := &Operator{ - enabledManifestFeatureGates: sets.New[string]("should-be-cleared"), - } - - optr.initializeFeatureGates() - - result := optr.getEnabledFeatureGates() - if result.Len() != 0 { - t.Error("initializeFeatureGates() should initialize with empty set when lister not available") - } -} From 4becfc93466fec7f2a5dac8388e45ac23671d85a Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 19 Dec 2025 13:24:52 +0000 Subject: [PATCH 11/12] Filter all manifests at render based on featureset, featuregates and cluster profile --- pkg/payload/render.go | 145 ++++++++++++++++++++++++------------------ 1 file changed, 84 insertions(+), 61 deletions(-) diff --git a/pkg/payload/render.go b/pkg/payload/render.go index 79fa2aefcb..e1c22e625d 100644 --- a/pkg/payload/render.go +++ b/pkg/payload/render.go @@ -12,7 +12,7 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/library-go/pkg/manifest" "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" @@ -35,33 +35,9 @@ func Render(outputDir, releaseImage, featureGateManifestPath, clusterProfile str } ) - var requiredFeatureSet *string - if featureGateManifestPath != "" { - manifests, err := manifest.ManifestsFromFiles([]string{featureGateManifestPath}) - if err != nil { - return fmt.Errorf("loading FeatureGate manifest: %w", err) - } - if len(manifests) != 1 { - return fmt.Errorf("FeatureGate manifest %s contains %d manifests, but expected only one", featureGateManifestPath, len(manifests)) - } - featureGateManifest := manifests[0] - expectedGVK := schema.GroupVersionKind{Kind: "FeatureGate", Version: configv1.GroupVersion.Version, Group: config.GroupName} - if featureGateManifest.GVK != expectedGVK { - return fmt.Errorf("FeatureGate manifest %s GroupVersionKind %v, but expected %v", featureGateManifest.OriginalFilename, featureGateManifest.GVK, expectedGVK) - } - featureSet, found, err := unstructured.NestedString(featureGateManifest.Obj.Object, "spec", "featureSet") - if err != nil { - return fmt.Errorf("%s spec.featureSet lookup was not a string: %w", featureGateManifest.String(), err) - } else if found { - requiredFeatureSet = &featureSet - klog.Infof("--feature-gate-manifest-path=%s results in feature set %q", featureGateManifest.OriginalFilename, featureSet) - } else { - requiredFeatureSet = ptr.To[string]("") - klog.Infof("--feature-gate-manifest-path=%s does not set spec.featureSet, using the default feature set", featureGateManifest.OriginalFilename) - } - } else { - requiredFeatureSet = ptr.To[string]("") - klog.Info("--feature-gate-manifest-path is unset, using the default feature set") + requiredFeatureSet, enabledFeatureGates, err := parseFeatureGateManifest(featureGateManifestPath) + if err != nil { + return fmt.Errorf("error parsing feature gate manifest: %w", err) } tasks := []struct { @@ -94,7 +70,7 @@ func Render(outputDir, releaseImage, featureGateManifestPath, clusterProfile str }} var errs []error for _, task := range tasks { - if err := renderDir(renderConfig, task.idir, task.odir, requiredFeatureSet, &clusterProfile, task.processTemplate, task.skipFiles, task.filterGroupKind); err != nil { + if err := renderDir(renderConfig, task.idir, task.odir, requiredFeatureSet, enabledFeatureGates, &clusterProfile, task.processTemplate, task.skipFiles, task.filterGroupKind); err != nil { errs = append(errs, err) } } @@ -106,7 +82,9 @@ func Render(outputDir, releaseImage, featureGateManifestPath, clusterProfile str return nil } -func renderDir(renderConfig manifestRenderConfig, idir, odir string, requiredFeatureSet *string, clusterProfile *string, processTemplate bool, skipFiles sets.Set[string], filterGroupKind sets.Set[schema.GroupKind]) error { +func renderDir(renderConfig manifestRenderConfig, idir, odir string, requiredFeatureSet *string, enabledFeatureGates sets.Set[string], clusterProfile *string, processTemplate bool, skipFiles sets.Set[string], filterGroupKind sets.Set[schema.GroupKind]) error { + klog.Infof("Filtering manifests in %s for feature set %v, cluster profile %v and enabled feature gates %v", idir, *requiredFeatureSet, *clusterProfile, enabledFeatureGates.UnsortedList()) + if err := os.MkdirAll(odir, 0666); err != nil { return err } @@ -126,13 +104,6 @@ func renderDir(renderConfig manifestRenderConfig, idir, odir string, requiredFea if skipFiles.Has(file.Name()) { continue } - if strings.Contains(file.Name(), "CustomNoUpgrade") || strings.Contains(file.Name(), "TechPreviewNoUpgrade") || strings.Contains(file.Name(), "DevPreviewNoUpgrade") { - // CustomNoUpgrade, TechPreviewNoUpgrade and DevPreviewNoUpgrade may add features to manifests like the ClusterVersion CRD, - // but we do not need those features during bootstrap-render time. In those clusters, the production - // CVO will be along shortly to update the manifests and deliver the gated features. - // fixme: now that we have requiredFeatureSet, use it to do Manifest.Include() filtering here instead of making filename assumptions - continue - } ipath := filepath.Join(idir, file.Name()) iraw, err := os.ReadFile(ipath) @@ -152,34 +123,32 @@ func renderDir(renderConfig manifestRenderConfig, idir, odir string, requiredFea rraw = iraw } - if len(filterGroupKind) > 0 { - manifests, err := manifest.ParseManifests(bytes.NewReader(rraw)) - if err != nil { - errs = append(errs, fmt.Errorf("parse manifest %s from %s: %w", file.Name(), idir, err)) - continue - } + manifests, err := manifest.ParseManifests(bytes.NewReader(rraw)) + if err != nil { + errs = append(errs, fmt.Errorf("parse manifest %s from %s: %w", file.Name(), idir, err)) + continue + } - filteredManifests := make([]string, 0, len(manifests)) - for _, manifest := range manifests { - if !filterGroupKind.Has(manifest.GVK.GroupKind()) { - klog.Infof("excluding %s because we do not render that group/kind", manifest.String()) - } else if err := manifest.Include(nil, requiredFeatureSet, clusterProfile, nil, nil, sets.Set[string]{} /* Empty, assuming any gated manifest will be applied later by the real CVO */); err != nil { - klog.Infof("excluding %s: %v", manifest.String(), err) - } else { - filteredManifests = append(filteredManifests, string(manifest.Raw)) - klog.Infof("including %s filtered by feature set %v and cluster profile %v", manifest.String(), requiredFeatureSet, clusterProfile) - } + filteredManifests := make([]string, 0, len(manifests)) + for _, manifest := range manifests { + if len(filterGroupKind) > 0 && !filterGroupKind.Has(manifest.GVK.GroupKind()) { + klog.Infof("excluding %s because we do not render that group/kind", manifest.String()) + } else if err := manifest.Include(nil, requiredFeatureSet, clusterProfile, nil, nil, enabledFeatureGates); err != nil { + klog.Infof("excluding %s: %v", manifest.String(), err) + } else { + filteredManifests = append(filteredManifests, string(manifest.Raw)) + klog.Infof("including %s", manifest.String()) } + } - if len(filteredManifests) == 0 { - continue - } + if len(filteredManifests) == 0 { + continue + } - if len(filteredManifests) == 1 { - rraw = []byte(filteredManifests[0]) - } else { - rraw = []byte(strings.Join(filteredManifests, "\n---\n")) - } + if len(filteredManifests) == 1 { + rraw = []byte(filteredManifests[0]) + } else { + rraw = []byte(strings.Join(filteredManifests, "\n---\n")) } opath := filepath.Join(odir, file.Name()) @@ -215,3 +184,57 @@ func renderManifest(config manifestRenderConfig, manifestBytes []byte) ([]byte, return buf.Bytes(), nil } + +func parseFeatureGateManifest(featureGateManifestPath string) (*string, sets.Set[string], error) { + if featureGateManifestPath == "" { + return ptr.To(""), sets.Set[string]{}, nil + } + + manifests, err := manifest.ManifestsFromFiles([]string{featureGateManifestPath}) + if err != nil { + return nil, nil, fmt.Errorf("loading FeatureGate manifest: %w", err) + } + + if len(manifests) != 1 { + return nil, nil, fmt.Errorf("FeatureGate manifest %s contains %d manifests, but expected only one", featureGateManifestPath, len(manifests)) + } + + featureGateManifest := manifests[0] + expectedGVK := schema.GroupVersionKind{Kind: "FeatureGate", Version: configv1.GroupVersion.Version, Group: config.GroupName} + if featureGateManifest.GVK != expectedGVK { + return nil, nil, fmt.Errorf("FeatureGate manifest %s GroupVersionKind %v, but expected %v", featureGateManifest.OriginalFilename, featureGateManifest.GVK, expectedGVK) + } + + // Convert unstructured object to structured FeatureGate + var featureGate configv1.FeatureGate + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(featureGateManifest.Obj.Object, &featureGate); err != nil { + return nil, nil, fmt.Errorf("failed to convert FeatureGate manifest %s to structured object: %w", featureGateManifest.OriginalFilename, err) + } + + var requiredFeatureSet *string + if featureGate.Spec.FeatureSet != "" { + featureSetString := string(featureGate.Spec.FeatureSet) + requiredFeatureSet = &featureSetString + klog.Infof("--feature-gate-manifest-path=%s results in feature set %q", featureGateManifest.OriginalFilename, featureGate.Spec.FeatureSet) + } else { + requiredFeatureSet = ptr.To("") + klog.Infof("--feature-gate-manifest-path=%s does not set spec.featureSet, using the default feature set", featureGateManifest.OriginalFilename) + } + + if len(featureGate.Status.FeatureGates) == 0 { + // In case there are no feature gates, fall back to feature set only behaviour. + return requiredFeatureSet, sets.Set[string]{}, nil + } + + // A rendered manifest should only include 1 version of the enabled feature gates. + if len(featureGate.Status.FeatureGates) > 1 { + return nil, nil, fmt.Errorf("FeatureGate manifest %s contains %d feature gates, but expected exactly one", featureGateManifest.OriginalFilename, len(featureGate.Status.FeatureGates)) + } + + enabledFeatureGates := sets.Set[string]{} + for _, feature := range featureGate.Status.FeatureGates[0].Enabled { + enabledFeatureGates.Insert(string(feature.Name)) + } + + return requiredFeatureSet, enabledFeatureGates, nil +} From f723c0d80865dfd24b017c5111ad2f8075ee4f8b Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 19 Dec 2025 14:54:38 +0000 Subject: [PATCH 12/12] Ensure bootstrap pod is included in render --- bootstrap/bootstrap-pod.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bootstrap/bootstrap-pod.yaml b/bootstrap/bootstrap-pod.yaml index 1a885ea801..580eebd49b 100644 --- a/bootstrap/bootstrap-pod.yaml +++ b/bootstrap/bootstrap-pod.yaml @@ -5,6 +5,8 @@ metadata: namespace: openshift-cluster-version labels: k8s-app: cluster-version-operator + annotations: + include.release.openshift.io/{{ .ClusterProfile }}: "true" spec: containers: - name: cluster-version-operator