From 900432997fd86b32a5464b745a143c3ad9e721a0 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 17 Dec 2025 12:18:58 +0000 Subject: [PATCH 01/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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/14] 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 From 8d82576263cb92463305b17e7e9604f2387b4348 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 19 Dec 2025 14:59:58 +0000 Subject: [PATCH 13/14] Update updateEnabledFeatureGates locking --- pkg/cvo/cvo.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index df1b7de953..d3959e2422 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -1137,8 +1137,11 @@ func (optr *Operator) updateEnabledFeatureGates(obj interface{}) bool { newGates := optr.extractEnabledGates(featureGate) + optr.featureGatesMutex.RLock() + // Check if gates actually changed to avoid unnecessary work if !optr.enabledManifestFeatureGates.Equal(newGates) { + optr.featureGatesMutex.RUnlock() optr.featureGatesMutex.Lock() defer optr.featureGatesMutex.Unlock() @@ -1150,6 +1153,7 @@ func (optr *Operator) updateEnabledFeatureGates(obj interface{}) bool { return true } + optr.featureGatesMutex.RUnlock() return false } From e98f804673ff2977e9484bb4e26fbe5dd0fae361 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 23 Dec 2025 13:15:59 +0000 Subject: [PATCH 14/14] Add major version manifest filtering --- go.mod | 2 +- go.sum | 4 +- lib/manifest/manifest.go | 3 + pkg/cvo/featuregate_integration_test.go | 8 +- pkg/cvo/sync_worker.go | 6 +- pkg/payload/payload.go | 13 +- pkg/payload/payload_test.go | 5 +- pkg/payload/render.go | 39 ++- pkg/payload/render_test.go | 242 ++++++++++++++++++ .../library-go/pkg/manifest/manifest.go | 76 +++++- vendor/modules.txt | 4 +- 11 files changed, 376 insertions(+), 26 deletions(-) diff --git a/go.mod b/go.mod index 64cc4eec5d..703b2ff827 100644 --- a/go.mod +++ b/go.mod @@ -33,7 +33,7 @@ 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 +replace github.com/openshift/library-go => github.com/JoelSpeed/library-go v0.0.0-20251223143639-e409b040c48b require ( cel.dev/expr v0.24.0 // indirect diff --git a/go.sum b/go.sum index 03b9562c77..8547d066fa 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +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/JoelSpeed/library-go v0.0.0-20251223143639-e409b040c48b h1:3db1Pe8LH/Z+oYZJ5XC41S9WAvd5kZJJqGaH33B1xn0= +github.com/JoelSpeed/library-go v0.0.0-20251223143639-e409b040c48b/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= diff --git a/lib/manifest/manifest.go b/lib/manifest/manifest.go index 0c7faff843..946997c7f8 100644 --- a/lib/manifest/manifest.go +++ b/lib/manifest/manifest.go @@ -49,6 +49,7 @@ func GetImplicitlyEnabledCapabilities( manifestInclusionConfiguration InclusionConfiguration, currentImplicitlyEnabled sets.Set[configv1.ClusterVersionCapability], enabledFeatureGates sets.Set[string], + majorVersion *uint64, ) sets.Set[configv1.ClusterVersionCapability] { ret := currentImplicitlyEnabled.Clone() for _, updateManifest := range updatePayloadManifests { @@ -59,6 +60,7 @@ func GetImplicitlyEnabledCapabilities( manifestInclusionConfiguration.Capabilities, manifestInclusionConfiguration.Overrides, enabledFeatureGates, + majorVersion, true, ) // update manifest is enabled, no need to check @@ -77,6 +79,7 @@ func GetImplicitlyEnabledCapabilities( manifestInclusionConfiguration.Capabilities, manifestInclusionConfiguration.Overrides, enabledFeatureGates, + majorVersion, true, ); err != nil { continue diff --git a/pkg/cvo/featuregate_integration_test.go b/pkg/cvo/featuregate_integration_test.go index 4d280634ce..d1ac98435e 100644 --- a/pkg/cvo/featuregate_integration_test.go +++ b/pkg/cvo/featuregate_integration_test.go @@ -100,7 +100,7 @@ func TestFeatureGateManifestFiltering(t *testing.T) { } // Test the manifest inclusion logic - err := manifest.Include(nil, nil, nil, nil, nil, tt.enabledGates) + err := manifest.Include(nil, nil, nil, nil, nil, tt.enabledGates, nil) if tt.shouldInclude { if err != nil { @@ -143,7 +143,7 @@ func TestSyncWorkIntegration(t *testing.T) { Obj: testObj, } - err := manifest.Include(nil, nil, nil, nil, nil, work.EnabledFeatureGates) + err := manifest.Include(nil, nil, nil, nil, nil, work.EnabledFeatureGates, nil) if err != nil { t.Errorf("Expected manifest to be included with TestGate1 enabled, got error: %v", err) } @@ -153,7 +153,7 @@ func TestSyncWorkIntegration(t *testing.T) { "release.openshift.io/feature-gates": "DisabledGate", }) - err = manifest.Include(nil, nil, nil, nil, nil, work.EnabledFeatureGates) + err = manifest.Include(nil, nil, nil, nil, nil, work.EnabledFeatureGates, nil) if err == nil { t.Error("Expected manifest to be excluded with DisabledGate not enabled, but no error occurred") } @@ -262,7 +262,7 @@ func TestManifestFilteringExamples(t *testing.T) { Obj: obj, } - err := manifest.Include(nil, nil, nil, nil, nil, example.enabledGates) + err := manifest.Include(nil, nil, nil, nil, nil, example.enabledGates, nil) if example.shouldInclude { if err != nil { diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index 9442466d03..a28f16f99e 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -428,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.EnabledFeatureGates)) + work.Capabilities, work.EnabledFeatureGates, payloadUpdate.MajorVersion)) } w.payload = payloadUpdate msg = fmt.Sprintf("Payload loaded version=%q image=%q architecture=%q", desired.Version, desired.Image, @@ -1064,7 +1064,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, work.EnabledFeatureGates); err != nil { + if err := task.Manifest.Include(nil, nil, nil, &capabilities, work.Overrides, work.EnabledFeatureGates, payloadUpdate.MajorVersion); err != nil { klog.V(manifestVerbosity).Infof("Skipping precreation of %s: %s", task, err) continue } @@ -1084,7 +1084,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, work.EnabledFeatureGates); err != nil { + if err := task.Manifest.Include(nil, nil, nil, &capabilities, work.Overrides, work.EnabledFeatureGates, payloadUpdate.MajorVersion); err != nil { klog.V(manifestVerbosity).Infof("Skipping %s: %s", task, err) continue } diff --git a/pkg/payload/payload.go b/pkg/payload/payload.go index 7b72ce4b5f..977687b41f 100644 --- a/pkg/payload/payload.go +++ b/pkg/payload/payload.go @@ -13,6 +13,7 @@ import ( "time" "k8s.io/klog/v2" + "k8s.io/utils/ptr" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" @@ -110,6 +111,7 @@ type Update struct { ImageRef *imagev1.ImageStream Architecture string + MajorVersion *uint64 // manifestHash is a hash of the manifests included in this payload ManifestHash string @@ -211,7 +213,7 @@ func LoadUpdate(dir, releaseImage, excludeIdentifier string, requiredFeatureSet filteredMs := []manifest.Manifest{} for _, manifest := range ms { - if err := manifest.Include(&excludeIdentifier, &requiredFeatureSet, &profile, onlyKnownCaps, nil, enabledFeatureGates); err != nil { + if err := manifest.Include(&excludeIdentifier, &requiredFeatureSet, &profile, onlyKnownCaps, nil, enabledFeatureGates, payload.MajorVersion); err != nil { klog.V(2).Infof("excluding %s: %v\n", manifest.String(), err) continue } @@ -243,7 +245,7 @@ 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, enabledFeatureGates sets.Set[string]) sets.Set[configv1.ClusterVersionCapability] { + capabilities capability.ClusterCapabilities, enabledFeatureGates sets.Set[string], majorVersion *uint64) sets.Set[configv1.ClusterVersionCapability] { clusterCaps := capability.GetCapabilitiesStatus(capabilities) return localmanifest.GetImplicitlyEnabledCapabilities( updatePayloadManifests, @@ -251,6 +253,7 @@ func GetImplicitlyEnabledCapabilities(updatePayloadManifests []manifest.Manifest localmanifest.InclusionConfiguration{Capabilities: &clusterCaps}, capabilities.ImplicitlyEnabled, enabledFeatureGates, + majorVersion, ) } @@ -285,6 +288,11 @@ func loadPayloadMetadata(releaseDir, releaseImage string) (*Update, error) { klog.V(2).Infof("Architecture from %s (%s) retrieved from runtime: %q", cincinnatiJSONFile, release.Version, arch) } + parsedVersion, err := semver.Parse(release.Version) + if err != nil { + return nil, fmt.Errorf("failed to parse release version %s: %w", release.Version, err) + } + release.Image = releaseImage imageRef, err := loadImageReferences(releaseDir) @@ -300,6 +308,7 @@ func loadPayloadMetadata(releaseDir, releaseImage string) (*Update, error) { Release: release, ImageRef: imageRef, Architecture: arch, + MajorVersion: ptr.To(parsedVersion.Major), }, nil } diff --git a/pkg/payload/payload_test.go b/pkg/payload/payload_test.go index 21730fd23f..0e49f0ecf6 100644 --- a/pkg/payload/payload_test.go +++ b/pkg/payload/payload_test.go @@ -15,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" + "k8s.io/utils/ptr" configv1 "github.com/openshift/api/config/v1" imagev1 "github.com/openshift/api/image/v1" @@ -66,6 +67,7 @@ func TestLoadUpdate(t *testing.T) { }, }, Architecture: architecture, + MajorVersion: ptr.To(uint64(1)), ManifestHash: "DL-FFQ2Uem8=", Manifests: []manifest.Manifest{ { @@ -177,6 +179,7 @@ func TestLoadUpdateArchitecture(t *testing.T) { }, }, Architecture: "Multi", + MajorVersion: ptr.To(uint64(1)), ManifestHash: "fvnVhLw92pE=", Manifests: []manifest.Manifest{ { @@ -365,7 +368,7 @@ func TestGetImplicitlyEnabledCapabilities(t *testing.T) { if tt.pathExt == "test10" { updateMans = append(updateMans, updateMans[0]) } - caps := GetImplicitlyEnabledCapabilities(updateMans, currentMans, tt.capabilities, sets.Set[string]{}) + caps := GetImplicitlyEnabledCapabilities(updateMans, currentMans, tt.capabilities, sets.Set[string]{}, nil) 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 e1c22e625d..6902fa6b32 100644 --- a/pkg/payload/render.go +++ b/pkg/payload/render.go @@ -2,12 +2,14 @@ package payload import ( "bytes" + "encoding/json" "fmt" "os" "path/filepath" "strings" "text/template" + "github.com/blang/semver/v4" "github.com/openshift/api/config" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/library-go/pkg/manifest" @@ -40,6 +42,11 @@ func Render(outputDir, releaseImage, featureGateManifestPath, clusterProfile str return fmt.Errorf("error parsing feature gate manifest: %w", err) } + payloadVersion, err := loadReleaseVersion(releaseManifestsDir) + if err != nil { + return fmt.Errorf("error loading release version: %w", err) + } + tasks := []struct { idir string odir string @@ -70,7 +77,7 @@ func Render(outputDir, releaseImage, featureGateManifestPath, clusterProfile str }} var errs []error for _, task := range tasks { - if err := renderDir(renderConfig, task.idir, task.odir, requiredFeatureSet, enabledFeatureGates, &clusterProfile, task.processTemplate, task.skipFiles, task.filterGroupKind); err != nil { + if err := renderDir(renderConfig, task.idir, task.odir, requiredFeatureSet, enabledFeatureGates, &clusterProfile, ptr.To(payloadVersion.Major), task.processTemplate, task.skipFiles, task.filterGroupKind); err != nil { errs = append(errs, err) } } @@ -82,8 +89,8 @@ func Render(outputDir, releaseImage, featureGateManifestPath, clusterProfile str return nil } -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()) +func renderDir(renderConfig manifestRenderConfig, idir, odir string, requiredFeatureSet *string, enabledFeatureGates sets.Set[string], clusterProfile *string, majorVersion *uint64, 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, enabled feature gates %v, and major version %v", idir, *requiredFeatureSet, *clusterProfile, enabledFeatureGates.UnsortedList(), ptr.Deref(majorVersion, 0)) if err := os.MkdirAll(odir, 0666); err != nil { return err @@ -133,7 +140,7 @@ func renderDir(renderConfig manifestRenderConfig, idir, odir string, requiredFea 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 { + } else if err := manifest.Include(nil, requiredFeatureSet, clusterProfile, nil, nil, enabledFeatureGates, majorVersion); err != nil { klog.Infof("excluding %s: %v", manifest.String(), err) } else { filteredManifests = append(filteredManifests, string(manifest.Raw)) @@ -238,3 +245,27 @@ func parseFeatureGateManifest(featureGateManifestPath string) (*string, sets.Set return requiredFeatureSet, enabledFeatureGates, nil } + +func loadReleaseVersion(releaseDir string) (semver.Version, error) { + path := filepath.Join(releaseDir, cincinnatiJSONFile) + data, err := os.ReadFile(path) + if err != nil { + return semver.Version{}, err + } + + var metadata metadata + if err := json.Unmarshal(data, &metadata); err != nil { + return semver.Version{}, fmt.Errorf("unmarshal Cincinnati metadata: %w", err) + } + + if metadata.Version == "" { + return semver.Version{}, errors.New("missing required Cincinnati metadata version") + } + + parsedVersion, err := semver.Parse(metadata.Version) + if err != nil { + return semver.Version{}, fmt.Errorf("Cincinnati metadata version %q is not a valid semantic version: %v", metadata.Version, err) + } + + return parsedVersion, nil +} diff --git a/pkg/payload/render_test.go b/pkg/payload/render_test.go index efaafc427a..0d9727e164 100644 --- a/pkg/payload/render_test.go +++ b/pkg/payload/render_test.go @@ -6,6 +6,9 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/utils/ptr" ) func TestRenderManifest(t *testing.T) { @@ -58,3 +61,242 @@ func TestRenderManifest(t *testing.T) { }) } } + +func TestRenderDirWithMajorVersionFiltering(t *testing.T) { + tests := []struct { + name string + manifests []testManifest + majorVersion *uint64 + expectedInclusions []string // Names of manifests that should be included + expectedExclusions []string // Names of manifests that should be excluded + }{ + { + name: "major version 4 includes version 4 manifests", + manifests: []testManifest{ + { + name: "version4-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "4", + }, + }, + { + name: "version5-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "5", + }, + }, + { + name: "no-version-manifest", + annotations: map[string]string{}, + }, + }, + majorVersion: ptr.To(uint64(4)), + expectedInclusions: []string{"version4-manifest", "no-version-manifest"}, + expectedExclusions: []string{"version5-manifest"}, + }, + { + name: "major version 5 includes version 5 manifests", + manifests: []testManifest{ + { + name: "version4-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "4", + }, + }, + { + name: "version5-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "5", + }, + }, + }, + majorVersion: ptr.To(uint64(5)), + expectedInclusions: []string{"version5-manifest"}, + expectedExclusions: []string{"version4-manifest"}, + }, + { + name: "nil major version includes all manifests", + manifests: []testManifest{ + { + name: "version4-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "4", + }, + }, + { + name: "version5-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "5", + }, + }, + }, + majorVersion: nil, + expectedInclusions: []string{"version4-manifest", "version5-manifest"}, + expectedExclusions: []string{}, + }, + { + name: "exclusion filtering with major version", + manifests: []testManifest{ + { + name: "not-for-version3-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "-3", + }, + }, + { + name: "only-for-version4-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "4", + }, + }, + }, + majorVersion: ptr.To(uint64(4)), + expectedInclusions: []string{"not-for-version3-manifest", "only-for-version4-manifest"}, + expectedExclusions: []string{}, + }, + { + name: "complex major version filtering", + manifests: []testManifest{ + { + name: "version4-or-5-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "4,5", + }, + }, + { + name: "exclude-version3-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "-3", + }, + }, + { + name: "version6-only-manifest", + annotations: map[string]string{ + "release.openshift.io/major-version": "6", + }, + }, + }, + majorVersion: ptr.To(uint64(4)), + expectedInclusions: []string{"version4-or-5-manifest", "exclude-version3-manifest"}, + expectedExclusions: []string{"version6-only-manifest"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create temporary directory structure + tempDir := t.TempDir() + inputDir := tempDir + "/input" + outputDir := tempDir + "/output" + + if err := os.MkdirAll(inputDir, 0755); err != nil { + t.Fatalf("failed to create input directory: %v", err) + } + if err := os.MkdirAll(outputDir, 0755); err != nil { + t.Fatalf("failed to create output directory: %v", err) + } + + // Write test manifests to input directory + for _, manifest := range tt.manifests { + manifestContent := createTestManifestYAML(manifest.name, manifest.annotations) + manifestPath := inputDir + "/" + manifest.name + ".yaml" + if err := os.WriteFile(manifestPath, []byte(manifestContent), 0644); err != nil { + t.Fatalf("failed to write manifest %s: %v", manifest.name, err) + } + } + + // Setup render configuration + renderConfig := manifestRenderConfig{ + ReleaseImage: "test-image:latest", + ClusterProfile: "test-profile", + } + + requiredFeatureSet := "test" + enabledFeatureGates := sets.Set[string]{} + clusterProfile := "test-profile" + + // Call renderDir with major version + err := renderDir( + renderConfig, + inputDir, + outputDir, + &requiredFeatureSet, + enabledFeatureGates, + &clusterProfile, + tt.majorVersion, + false, // processTemplate + sets.Set[string]{}, // skipFiles + sets.Set[schema.GroupKind]{}, // filterGroupKind + ) + + if err != nil { + t.Fatalf("renderDir failed: %v", err) + } + + // Check which manifests were included in output + outputFiles, err := os.ReadDir(outputDir) + if err != nil { + t.Fatalf("failed to read output directory: %v", err) + } + + includedManifests := make(map[string]bool) + for _, file := range outputFiles { + if strings.HasSuffix(file.Name(), ".yaml") { + manifestName := strings.TrimSuffix(file.Name(), ".yaml") + includedManifests[manifestName] = true + } + } + + // Verify expected inclusions + for _, expectedName := range tt.expectedInclusions { + if !includedManifests[expectedName] { + t.Errorf("Expected manifest %s to be included, but it was not found in output", expectedName) + } + } + + // Verify expected exclusions + for _, expectedExcludedName := range tt.expectedExclusions { + if includedManifests[expectedExcludedName] { + t.Errorf("Expected manifest %s to be excluded, but it was found in output", expectedExcludedName) + } + } + }) + } +} + +// testManifest represents a test manifest with its annotations +type testManifest struct { + name string + annotations map[string]string +} + +// createTestManifestYAML creates a YAML representation of a test manifest +func createTestManifestYAML(name string, annotations map[string]string) string { + yaml := `apiVersion: v1 +kind: ConfigMap +metadata: + name: ` + name + ` + namespace: test-namespace` + + // Always add required cluster profile annotation + allAnnotations := make(map[string]string) + allAnnotations["include.release.openshift.io/test-profile"] = "true" + + // Add provided annotations + for k, v := range annotations { + allAnnotations[k] = v + } + + if len(allAnnotations) > 0 { + yaml += "\n annotations:" + for k, v := range allAnnotations { + yaml += "\n " + k + ": \"" + v + "\"" + } + } + + yaml += ` +data: + test: "data"` + + return yaml +} 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 4096f95070..c7a6d7236b 100644 --- a/vendor/github.com/openshift/library-go/pkg/manifest/manifest.go +++ b/vendor/github.com/openshift/library-go/pkg/manifest/manifest.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "reflect" + "strconv" "strings" "k8s.io/apimachinery/pkg/util/sets" @@ -23,10 +24,11 @@ import ( ) const ( - CapabilityAnnotation = "capability.openshift.io/name" - DefaultClusterProfile = "self-managed-high-availability" - featureSetAnnotation = "release.openshift.io/feature-set" - featureGateAnnotation = "release.openshift.io/feature-gates" + CapabilityAnnotation = "capability.openshift.io/name" + DefaultClusterProfile = "self-managed-high-availability" + featureSetAnnotation = "release.openshift.io/feature-set" + featureGateAnnotation = "release.openshift.io/feature-gates" + majorVersionAnnotation = "release.openshift.io/major-version" ) var knownFeatureSets = sets.Set[string]{} @@ -222,12 +224,64 @@ func checkFeatureGates(enabledGates sets.Set[string], annotations map[string]str return nil } +// checkMajorVersion validates if manifest should be included based on major version requirements +func checkMajorVersion(clusterMajorVersion uint64, annotations map[string]string) error { + if annotations == nil { + return nil // No annotations, include by default + } + majorVersionRequirements, ok := annotations[majorVersionAnnotation] + if !ok { + return nil // No requirements, include by default + } + + requirements := strings.Split(majorVersionRequirements, ",") + includedVersions := sets.New[uint64]() + excludedVersions := sets.New[uint64]() + + for _, req := range requirements { + req = strings.TrimSpace(req) + if req == "" { + continue + } + + if strings.HasPrefix(req, "-") { + excludedVersionStr := req[1:] + excludedVersion, err := strconv.ParseUint(excludedVersionStr, 10, 64) + if err != nil { + return fmt.Errorf("invalid excluded major version %q in annotation: %v", excludedVersionStr, err) + } + excludedVersions.Insert(excludedVersion) + } else { + includedVersionStr := req + includedVersion, err := strconv.ParseUint(includedVersionStr, 10, 64) + if err != nil { + return fmt.Errorf("invalid included major version %q in annotation: %v", includedVersionStr, err) + } + includedVersions.Insert(includedVersion) + } + } + + switch { + case includedVersions.Intersection(excludedVersions).Len() > 0: + return fmt.Errorf("inclusion and exclusion requirements overlap: %v", includedVersions.Intersection(excludedVersions).UnsortedList()) + case includedVersions.Has(clusterMajorVersion): + return nil // Found matching version, include this manifest + case excludedVersions.Has(clusterMajorVersion): + return fmt.Errorf("major version %d matches excluded version %d", clusterMajorVersion, clusterMajorVersion) + case len(includedVersions) > 0: + return fmt.Errorf("major version %d does not match any required versions", clusterMajorVersion) + default: + // No positive requirement and did not match any excluded versions + 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, enabledFeatureGates sets.Set[string]) error { - return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, overrides, enabledFeatureGates, false) +func (m *Manifest) Include(excludeIdentifier *string, requiredFeatureSet *string, profile *string, capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, enabledFeatureGates sets.Set[string], majorVersion *uint64) error { + return m.IncludeAllowUnknownCapabilities(excludeIdentifier, requiredFeatureSet, profile, capabilities, overrides, enabledFeatureGates, majorVersion, false) } // IncludeAllowUnknownCapabilities returns an error if the manifest fails an inclusion filter and should be excluded from @@ -237,7 +291,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, enabledFeatureGates sets.Set[string], allowUnknownCapabilities bool) error { + capabilities *configv1.ClusterVersionCapabilitiesStatus, overrides []configv1.ComponentOverride, enabledFeatureGates sets.Set[string], majorVersion *uint64, allowUnknownCapabilities bool) error { annotations := m.Obj.GetAnnotations() if annotations == nil { @@ -266,6 +320,14 @@ func (m *Manifest) IncludeAllowUnknownCapabilities(excludeIdentifier *string, re } } + // Major version filtering + if majorVersion != nil { + err := checkMajorVersion(*majorVersion, 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 379278e5fd..7582157023 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/JoelSpeed/library-go v0.0.0-20251217102247-bcd739ac7ffa +# github.com/openshift/library-go v0.0.0-20251120164824-14a789e09884 => github.com/JoelSpeed/library-go v0.0.0-20251223143639-e409b040c48b ## explicit; go 1.24.0 github.com/openshift/library-go/pkg/apiserver/jsonpatch github.com/openshift/library-go/pkg/config/clusterstatus @@ -1001,5 +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/openshift/library-go => github.com/JoelSpeed/library-go v0.0.0-20251223143639-e409b040c48b # github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20241205171354-8006f302fd12