diff --git a/pkg/operator/configobserver/featuregates/hardcoded_featuregate_reader.go b/pkg/operator/configobserver/featuregates/hardcoded_featuregate_reader.go new file mode 100644 index 0000000000..e8c6c3cc0c --- /dev/null +++ b/pkg/operator/configobserver/featuregates/hardcoded_featuregate_reader.go @@ -0,0 +1,59 @@ +package featuregates + +import ( + "context" + + configv1 "github.com/openshift/api/config/v1" +) + +type hardcodedFeatureGateAccess struct { + enabled []configv1.FeatureGateName + disabled []configv1.FeatureGateName + readErr error + + initialFeatureGatesObserved chan struct{} + featureGatesHaveChangedSinceFirstObserved chan struct{} +} + +// NewHardcodedFeatureGateAccess is useful for unit testing, potentially in other packages as well. +func NewHardcodedFeatureGateAccess(enabled, disabled []configv1.FeatureGateName) FeatureGateAccess { + initialFeatureGatesObserved := make(chan struct{}) + close(initialFeatureGatesObserved) + c := &hardcodedFeatureGateAccess{ + enabled: enabled, + disabled: disabled, + initialFeatureGatesObserved: initialFeatureGatesObserved, + featureGatesHaveChangedSinceFirstObserved: make(chan struct{}), + } + + return c +} + +func (c *hardcodedFeatureGateAccess) SetChangeHandler(featureGateChangeHandlerFn FeatureGateChangeHandlerFunc) { + // ignore +} + +func (c *hardcodedFeatureGateAccess) Run(ctx context.Context) { + // ignore +} + +func (c *hardcodedFeatureGateAccess) InitialFeatureGatesObserved() chan struct{} { + return c.initialFeatureGatesObserved +} + +func (c *hardcodedFeatureGateAccess) FeatureGatesHaveChangedSinceFirstObserved() chan struct{} { + return c.featureGatesHaveChangedSinceFirstObserved +} + +func (c *hardcodedFeatureGateAccess) AreInitialFeatureGatesObserved() bool { + select { + case <-c.InitialFeatureGatesObserved(): + return true + default: + return false + } +} + +func (c *hardcodedFeatureGateAccess) CurrentFeatureGates() ([]configv1.FeatureGateName, []configv1.FeatureGateName, error) { + return c.enabled, c.disabled, c.readErr +} diff --git a/pkg/operator/configobserver/featuregates/observe_featuregates.go b/pkg/operator/configobserver/featuregates/observe_featuregates.go index aece99858f..c004bf77ce 100644 --- a/pkg/operator/configobserver/featuregates/observe_featuregates.go +++ b/pkg/operator/configobserver/featuregates/observe_featuregates.go @@ -5,70 +5,59 @@ import ( "reflect" "strings" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/sets" configv1 "github.com/openshift/api/config/v1" - configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" "github.com/openshift/library-go/pkg/operator/configobserver" "github.com/openshift/library-go/pkg/operator/events" ) -type FeatureGateLister interface { - FeatureGateLister() configlistersv1.FeatureGateLister +type FeatureGateAccessor interface { + FeatureGates() FeatureGateAccess } // NewObserveFeatureFlagsFunc produces a configobserver for feature gates. If non-nil, the featureWhitelist filters // feature gates to a known subset (instead of everything). The featureBlacklist will stop certain features from making // it through the list. The featureBlacklist should be empty, but for a brief time, some featuregates may need to skipped. // @smarterclayton will live forever in shame for being the first to require this for "IPv6DualStack". -func NewObserveFeatureFlagsFunc(featureWhitelist sets.String, featureBlacklist sets.String, configPath []string) configobserver.ObserveConfigFunc { +func NewObserveFeatureFlagsFunc(featureWhitelist sets.Set[configv1.FeatureGateName], featureBlacklist sets.Set[configv1.FeatureGateName], configPath []string, featureGateAccess FeatureGateAccess) configobserver.ObserveConfigFunc { return (&featureFlags{ - allowAll: len(featureWhitelist) == 0, - featureWhitelist: featureWhitelist, - featureBlacklist: featureBlacklist, - configPath: configPath, + allowAll: len(featureWhitelist) == 0, + featureWhitelist: featureWhitelist, + featureBlacklist: featureBlacklist, + configPath: configPath, + featureGateAccess: featureGateAccess, }).ObserveFeatureFlags } type featureFlags struct { allowAll bool - featureWhitelist sets.String + featureWhitelist sets.Set[configv1.FeatureGateName] // we add a forceDisableFeature list because we've now had bad featuregates break individual operators. Awesome. - featureBlacklist sets.String - configPath []string + featureBlacklist sets.Set[configv1.FeatureGateName] + configPath []string + featureGateAccess FeatureGateAccess } // ObserveFeatureFlags fills in --feature-flags for the kube-apiserver -func (f *featureFlags) ObserveFeatureFlags(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (ret map[string]interface{}, _ []error) { - defer func() { - ret = configobserver.Pruned(ret, f.configPath) - }() +func (f *featureFlags) ObserveFeatureFlags(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (map[string]interface{}, []error) { + prunedExistingConfig := configobserver.Pruned(existingConfig, f.configPath) - listers := genericListers.(FeatureGateLister) errs := []error{} - observedConfig := map[string]interface{}{} - configResource, err := listers.FeatureGateLister().Get("cluster") - // if we have no featuregate, then the installer and MCO probably still have way to reconcile certain custom resources - // we will assume that this means the same as default and hope for the best - if apierrors.IsNotFound(err) { - configResource = &configv1.FeatureGate{ - Spec: configv1.FeatureGateSpec{ - FeatureGateSelection: configv1.FeatureGateSelection{ - FeatureSet: configv1.Default, - }, - }, - } - } else if err != nil { - return existingConfig, append(errs, err) + if !f.featureGateAccess.AreInitialFeatureGatesObserved() { + // if we haven't observed featuregates yet, return the existing + return prunedExistingConfig, nil } - newConfigValue, err := f.getWhitelistedFeatureNames(configResource) + enabledFeatures, disabledFeatures, err := f.featureGateAccess.CurrentFeatureGates() if err != nil { - return existingConfig, append(errs, err) + return prunedExistingConfig, append(errs, err) } + observedConfig := map[string]interface{}{} + newConfigValue := f.getWhitelistedFeatureNames(enabledFeatures, disabledFeatures) + currentConfigValue, _, err := unstructured.NestedStringSlice(existingConfig, f.configPath...) if err != nil { errs = append(errs, err) @@ -80,27 +69,19 @@ func (f *featureFlags) ObserveFeatureFlags(genericListers configobserver.Listers if err := unstructured.SetNestedStringSlice(observedConfig, newConfigValue, f.configPath...); err != nil { recorder.Warningf("ObserveFeatureFlags", "Failed setting %v: %v", strings.Join(f.configPath, "."), err) - return existingConfig, append(errs, err) + return prunedExistingConfig, append(errs, err) } - return observedConfig, errs + return configobserver.Pruned(observedConfig, f.configPath), errs } -func (f *featureFlags) getWhitelistedFeatureNames(fg *configv1.FeatureGate) ([]string, error) { - var err error +func (f *featureFlags) getWhitelistedFeatureNames(enabledFeatures, disabledFeatures []configv1.FeatureGateName) []string { newConfigValue := []string{} - enabledFeatures := []string{} - disabledFeatures := []string{} - formatEnabledFunc := func(fs string) string { - return fmt.Sprintf("%s=true", fs) + formatEnabledFunc := func(fs configv1.FeatureGateName) string { + return fmt.Sprintf("%v=true", fs) } - formatDisabledFunc := func(fs string) string { - return fmt.Sprintf("%s=false", fs) - } - - enabledFeatures, disabledFeatures, err = FeaturesGatesFromFeatureSets(fg) - if err != nil { - return nil, err + formatDisabledFunc := func(fs configv1.FeatureGateName) string { + return fmt.Sprintf("%v=false", fs) } for _, enable := range enabledFeatures { @@ -124,24 +105,7 @@ func (f *featureFlags) getWhitelistedFeatureNames(fg *configv1.FeatureGate) ([]s newConfigValue = append(newConfigValue, formatDisabledFunc(disable)) } - return newConfigValue, nil -} - -func FeaturesGatesFromFeatureSets(fg *configv1.FeatureGate) ([]string, []string, error) { - if fg.Spec.FeatureSet == configv1.CustomNoUpgrade { - if fg.Spec.FeatureGateSelection.CustomNoUpgrade != nil { - return FeatureGateNamesToStrings(fg.Spec.FeatureGateSelection.CustomNoUpgrade.Enabled), - FeatureGateNamesToStrings(fg.Spec.FeatureGateSelection.CustomNoUpgrade.Disabled), - nil - } - return []string{}, []string{}, nil - } - - featureSet, ok := configv1.FeatureSets[fg.Spec.FeatureSet] - if !ok { - return []string{}, []string{}, fmt.Errorf(".spec.featureSet %q not found", featureSet) - } - return featureSet.Enabled, featureSet.Disabled, nil + return newConfigValue } func StringsToFeatureGateNames(in []string) []configv1.FeatureGateName { diff --git a/pkg/operator/configobserver/featuregates/observe_featuregates_test.go b/pkg/operator/configobserver/featuregates/observe_featuregates_test.go index 8dbe522853..63487cc8b7 100644 --- a/pkg/operator/configobserver/featuregates/observe_featuregates_test.go +++ b/pkg/operator/configobserver/featuregates/observe_featuregates_test.go @@ -4,7 +4,6 @@ import ( "reflect" "testing" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/cache" @@ -31,6 +30,37 @@ func (l testLister) PreRunHasSynced() []cache.InformerSynced { return nil } +var testingFeatureSets = map[configv1.FeatureSet]configv1.FeatureGateEnabledDisabled{ + configv1.Default: { + Enabled: []string{ + "OpenShiftPodSecurityAdmission", // bz-auth, stlaz, OCP specific + }, + Disabled: []string{ + "RetroactiveDefaultStorageClass", // sig-storage, RomanBednar, Kubernetes feature gate + }, + }, + configv1.CustomNoUpgrade: { + Enabled: []string{}, + Disabled: []string{}, + }, + configv1.TechPreviewNoUpgrade: { + Enabled: []string{ + "OpenShiftPodSecurityAdmission", // bz-auth, stlaz, OCP specific + "ExternalCloudProvider", // sig-cloud-provider, jspeed, OCP specific + "CSIDriverSharedResource", // sig-build, adkaplan, OCP specific + "BuildCSIVolumes", // sig-build, adkaplan, OCP specific + "NodeSwap", // sig-node, ehashman, Kubernetes feature gate + "MachineAPIProviderOpenStack", // openstack, egarcia (#forum-openstack), OCP specific + "InsightsConfigAPI", // insights, tremes (#ccx), OCP specific + "MatchLabelKeysInPodTopologySpread", // sig-scheduling, ingvagabund (#forum-workloads), Kubernetes feature gate + "PDBUnhealthyPodEvictionPolicy", // sig-apps, atiratree (#forum-workloads), Kubernetes feature gate + }, + Disabled: []string{ + "RetroactiveDefaultStorageClass", // sig-storage, RomanBednar, Kubernetes feature gate + }, + }, +} + func TestObserveFeatureFlags(t *testing.T) { configPath := []string{"foo", "bar"} @@ -41,8 +71,8 @@ func TestObserveFeatureFlags(t *testing.T) { expectedResult []string expectError bool customNoUpgrade *configv1.CustomFeatureGates - knownFeatures sets.String - blacklistedFeatures sets.String + knownFeatures sets.Set[configv1.FeatureGateName] + blacklistedFeatures sets.Set[configv1.FeatureGateName] }{ { name: "default", @@ -64,11 +94,8 @@ func TestObserveFeatureFlags(t *testing.T) { "MachineAPIProviderOpenStack=true", "InsightsConfigAPI=true", "MatchLabelKeysInPodTopologySpread=true", - "RetroactiveDefaultStorageClass=true", "PDBUnhealthyPodEvictionPolicy=true", - "DynamicResourceAllocation=true", - "ValidatingAdmissionPolicy=true", - "AdmissionWebhookMatchConditions=true", + "RetroactiveDefaultStorageClass=false", }, }, { @@ -98,7 +125,7 @@ func TestObserveFeatureFlags(t *testing.T) { Enabled: []configv1.FeatureGateName{"CustomFeatureEnabled"}, Disabled: []configv1.FeatureGateName{"CustomFeatureDisabled"}, }, - knownFeatures: sets.NewString("CustomFeatureEnabled"), + knownFeatures: sets.New[configv1.FeatureGateName]("CustomFeatureEnabled"), }, { name: "custom no upgrade and blacklisted features", @@ -112,32 +139,28 @@ func TestObserveFeatureFlags(t *testing.T) { Enabled: []configv1.FeatureGateName{"CustomFeatureEnabled", "AnotherThing", "AThirdThing"}, Disabled: []configv1.FeatureGateName{"CustomFeatureDisabled", "DisabledThing"}, }, - blacklistedFeatures: sets.NewString("AnotherThing", "DisabledThing"), + blacklistedFeatures: sets.New[configv1.FeatureGateName]("AnotherThing", "DisabledThing"), }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) - indexer.Add(&configv1.FeatureGate{ - ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, - Spec: configv1.FeatureGateSpec{ - FeatureGateSelection: configv1.FeatureGateSelection{ - FeatureSet: tc.configValue, - CustomNoUpgrade: tc.customNoUpgrade, - }, - }, - }) - listers := testLister{ - lister: configlistersv1.NewFeatureGateLister(indexer), + apiEnabledDisabled := testingFeatureSets[tc.configValue] + enabled := []configv1.FeatureGateName{} + enabled = append(enabled, StringsToFeatureGateNames(apiEnabledDisabled.Enabled)...) + disabled := []configv1.FeatureGateName{} + disabled = append(disabled, StringsToFeatureGateNames(apiEnabledDisabled.Disabled)...) + + if tc.customNoUpgrade != nil { + enabled = append(enabled, tc.customNoUpgrade.Enabled...) + disabled = append(disabled, tc.customNoUpgrade.Disabled...) } + featureAccessor := NewHardcodedFeatureGateAccess(enabled, disabled) eventRecorder := events.NewInMemoryRecorder("") - initialExistingConfig := map[string]interface{}{} + observeFn := NewObserveFeatureFlagsFunc(tc.knownFeatures, tc.blacklistedFeatures, configPath, featureAccessor) - observeFn := NewObserveFeatureFlagsFunc(tc.knownFeatures, tc.blacklistedFeatures, configPath) - - observed, errs := observeFn(listers, eventRecorder, initialExistingConfig) + observed, errs := observeFn(nil, eventRecorder, initialExistingConfig) if len(errs) != 0 && !tc.expectError { t.Fatal(errs) } diff --git a/pkg/operator/configobserver/featuregates/simple_featuregate_reader.go b/pkg/operator/configobserver/featuregates/simple_featuregate_reader.go index 29770b55d9..d125f20580 100644 --- a/pkg/operator/configobserver/featuregates/simple_featuregate_reader.go +++ b/pkg/operator/configobserver/featuregates/simple_featuregate_reader.go @@ -8,6 +8,8 @@ import ( "sync" "time" + configv1 "github.com/openshift/api/config/v1" + v1 "github.com/openshift/client-go/config/informers/externalversions/config/v1" configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" "github.com/openshift/library-go/pkg/operator/events" @@ -21,18 +23,34 @@ import ( type FeatureGateChangeHandlerFunc func(featureChange FeatureChange) +// FeatureGateAccess is used to get a list of enabled and disabled featuregates. +// Create a new instance using NewFeatureGateAccess. +// To create one for unit testing, use NewHardcodedFeatureGateAccess. type FeatureGateAccess interface { + // SetChangeHandler can only be called before Run. + // The default change handler will exit 0 when the set of featuregates changes. + // That is usually the easiest and simplest thing for an *operator* to do. + // This also discourages direct operand reading since all operands restarting simultaneously is bad. + // This function allows changing that default behavior to something else (perhaps a channel notification for + // all impacted controllers in an operator. + // I doubt this will be worth the effort in the majority of cases. SetChangeHandler(featureGateChangeHandlerFn FeatureGateChangeHandlerFunc) + // Run starts a go func that continously watches the set of featuregates enabled in the cluster. Run(ctx context.Context) + // InitialFeatureGatesObserved returns a channel that is closed once the featuregates have been observed. + // Once closed, the CurrentFeatureGates method can be called successfully. InitialFeatureGatesObserved() chan struct{} - CurrentFeatureGates() (enabled []string, disabled []string, err error) + // CurrentFeatureGates returns the list of enabled and disabled featuregates. + // It returns an error if the current set of featuregates is not known. + CurrentFeatureGates() (enabled []configv1.FeatureGateName, disabled []configv1.FeatureGateName, err error) + // AreInitialFeatureGatesObserved returns true if the initial featuregates have been observed. AreInitialFeatureGatesObserved() bool } type Features struct { - Enabled []string - Disabled []string + Enabled []configv1.FeatureGateName + Disabled []configv1.FeatureGateName } type FeatureChange struct { @@ -182,14 +200,25 @@ func (c *defaultFeatureGateAccess) syncHandler(ctx context.Context) error { return err } + found := false features := Features{} - enabled, disabled, err := FeaturesGatesFromFeatureSets(featureGate) - if err != nil { - return err + for _, featureGateValues := range featureGate.Status.FeatureGates { + if featureGateValues.Version != desiredVersion { + continue + } + found = true + for _, enabled := range featureGateValues.Enabled { + features.Enabled = append(features.Enabled, enabled.Name) + } + for _, disabled := range featureGateValues.Disabled { + features.Disabled = append(features.Disabled, disabled.Name) + } + break } - features.Enabled = enabled - features.Disabled = disabled + if !found { + return fmt.Errorf("missing desired version %q in featuregates.config.openshift.io/cluster", desiredVersion) + } c.setFeatureGates(features) return nil @@ -238,15 +267,15 @@ func (c *defaultFeatureGateAccess) AreInitialFeatureGatesObserved() bool { } } -func (c *defaultFeatureGateAccess) CurrentFeatureGates() ([]string, []string, error) { +func (c *defaultFeatureGateAccess) CurrentFeatureGates() ([]configv1.FeatureGateName, []configv1.FeatureGateName, error) { c.lock.Lock() defer c.lock.Unlock() if !c.AreInitialFeatureGatesObserved() { return nil, nil, fmt.Errorf("featureGates not yet observed") } - retEnabled := make([]string, len(c.currentFeatures.Enabled)) - retDisabled := make([]string, len(c.currentFeatures.Disabled)) + retEnabled := make([]configv1.FeatureGateName, len(c.currentFeatures.Enabled)) + retDisabled := make([]configv1.FeatureGateName, len(c.currentFeatures.Disabled)) copy(retEnabled, c.currentFeatures.Enabled) copy(retDisabled, c.currentFeatures.Disabled) diff --git a/pkg/operator/configobserver/featuregates/simple_featuregate_reader_test.go b/pkg/operator/configobserver/featuregates/simple_featuregate_reader_test.go index 1aedc61689..e2e8f8dd95 100644 --- a/pkg/operator/configobserver/featuregates/simple_featuregate_reader_test.go +++ b/pkg/operator/configobserver/featuregates/simple_featuregate_reader_test.go @@ -14,7 +14,6 @@ import ( ) type testFeatureGateBuilder struct { - featureSetName string versionToFeatures map[string]Features } @@ -24,15 +23,9 @@ func featureGateBuilder() *testFeatureGateBuilder { } } -func (f *testFeatureGateBuilder) specFeatureSet(featureSetName string) *testFeatureGateBuilder { - f.featureSetName = featureSetName - - return f -} - func (f *testFeatureGateBuilder) enabled(version string, enabled ...string) *testFeatureGateBuilder { curr := f.versionToFeatures[version] - curr.Enabled = enabled + curr.Enabled = StringsToFeatureGateNames(enabled) f.versionToFeatures[version] = curr return f @@ -40,7 +33,7 @@ func (f *testFeatureGateBuilder) enabled(version string, enabled ...string) *tes func (f *testFeatureGateBuilder) disabled(version string, disabled ...string) *testFeatureGateBuilder { curr := f.versionToFeatures[version] - curr.Disabled = disabled + curr.Disabled = StringsToFeatureGateNames(disabled) f.versionToFeatures[version] = curr return f @@ -49,11 +42,18 @@ func (f *testFeatureGateBuilder) disabled(version string, disabled ...string) *t func (f *testFeatureGateBuilder) toFeatureGate() *configv1.FeatureGate { ret := &configv1.FeatureGate{ ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, - Spec: configv1.FeatureGateSpec{ - FeatureGateSelection: configv1.FeatureGateSelection{ - FeatureSet: configv1.FeatureSet(f.featureSetName), - }, - }, + } + for version, features := range f.versionToFeatures { + details := configv1.FeatureGateDetails{ + Version: version, + } + for _, curr := range features.Enabled { + details.Enabled = append(details.Enabled, configv1.FeatureGateAttributes{Name: curr}) + } + for _, curr := range features.Disabled { + details.Disabled = append(details.Disabled, configv1.FeatureGateAttributes{Name: curr}) + } + ret.Status.FeatureGates = append(ret.Status.FeatureGates, details) } return ret @@ -93,7 +93,6 @@ func Test_defaultFeatureGateAccess_syncHandler(t *testing.T) { name: "read-explicit-version", firstFeatureGate: featureGateBuilder(). - specFeatureSet("features-for-desired-version"). enabled("desired-version", "alpha", "bravo"). disabled("desired-version", "charlie", "delta"). toFeatureGate(), @@ -108,10 +107,10 @@ func Test_defaultFeatureGateAccess_syncHandler(t *testing.T) { if history[0].Previous != nil { t.Fatalf("bad changes: %v", history) } - if !reflect.DeepEqual([]string{"alpha", "bravo"}, history[0].New.Enabled) { + if !reflect.DeepEqual([]configv1.FeatureGateName{"alpha", "bravo"}, history[0].New.Enabled) { t.Fatal(history[0].New.Enabled) } - if !reflect.DeepEqual([]string{"charlie", "delta"}, history[0].New.Disabled) { + if !reflect.DeepEqual([]configv1.FeatureGateName{"charlie", "delta"}, history[0].New.Disabled) { t.Fatal(history[0].New.Enabled) } }, @@ -120,7 +119,6 @@ func Test_defaultFeatureGateAccess_syncHandler(t *testing.T) { name: "read-explicit-version-from-others", firstFeatureGate: featureGateBuilder(). - specFeatureSet("features-for-desired-version"). enabled("desired-version", "alpha", "bravo"). disabled("desired-version", "charlie", "delta"). enabled("other-version", "yankee", "zulu"). @@ -136,10 +134,10 @@ func Test_defaultFeatureGateAccess_syncHandler(t *testing.T) { if history[0].Previous != nil { t.Fatalf("bad changes: %v", history) } - if !reflect.DeepEqual([]string{"alpha", "bravo"}, history[0].New.Enabled) { + if !reflect.DeepEqual([]configv1.FeatureGateName{"alpha", "bravo"}, history[0].New.Enabled) { t.Fatal(history[0].New.Enabled) } - if !reflect.DeepEqual([]string{"charlie", "delta"}, history[0].New.Disabled) { + if !reflect.DeepEqual([]configv1.FeatureGateName{"charlie", "delta"}, history[0].New.Disabled) { t.Fatal(history[0].New.Enabled) } }, @@ -148,12 +146,10 @@ func Test_defaultFeatureGateAccess_syncHandler(t *testing.T) { name: "no-change-means-no-extra-watch-call", firstFeatureGate: featureGateBuilder(). - specFeatureSet("features-for-desired-version"). enabled("desired-version", "alpha", "bravo"). disabled("desired-version", "charlie", "delta"). toFeatureGate(), secondFeatureGate: featureGateBuilder(). - specFeatureSet("features-for-desired-version"). enabled("desired-version", "alpha", "bravo"). disabled("desired-version", "charlie", "delta"). toFeatureGate(), @@ -168,10 +164,76 @@ func Test_defaultFeatureGateAccess_syncHandler(t *testing.T) { if history[0].Previous != nil { t.Fatalf("bad changes: %v", history) } - if !reflect.DeepEqual([]string{"alpha", "bravo"}, history[0].New.Enabled) { + if !reflect.DeepEqual([]configv1.FeatureGateName{"alpha", "bravo"}, history[0].New.Enabled) { + t.Fatal(history[0].New.Enabled) + } + if !reflect.DeepEqual([]configv1.FeatureGateName{"charlie", "delta"}, history[0].New.Disabled) { + t.Fatal(history[0].New.Enabled) + } + }, + }, + { + name: "change-means-watch-call", + + firstFeatureGate: featureGateBuilder(). + enabled("desired-version", "alpha", "bravo"). + disabled("desired-version", "charlie", "delta"). + toFeatureGate(), + secondFeatureGate: featureGateBuilder(). + enabled("desired-version", "alpha", "bravo", "charlie", "delta"). + toFeatureGate(), + fields: fields{ + desiredVersion: "desired-version", + }, + + changeVerifier: func(t *testing.T, history []FeatureChange) { + if len(history) != 2 { + t.Fatalf("bad changes: %v", history) + } + if history[0].Previous != nil { + t.Fatalf("bad changes: %v", history) + } + if !reflect.DeepEqual([]configv1.FeatureGateName{"alpha", "bravo"}, history[0].New.Enabled) { + t.Fatal(history[0].New.Enabled) + } + if !reflect.DeepEqual([]configv1.FeatureGateName{"charlie", "delta"}, history[0].New.Disabled) { + t.Fatal(history[0].New.Enabled) + } + if !reflect.DeepEqual(*(history[1].Previous), history[0].New) { + t.Fatalf("bad changes: %v", history) + } + if !reflect.DeepEqual([]configv1.FeatureGateName{"alpha", "bravo", "charlie", "delta"}, history[1].New.Enabled) { + t.Fatal(history[1].New.Enabled) + } + if len(history[1].New.Disabled) != 0 { + t.Fatal(history[1].New.Disabled) + } + }, + }, + { + name: "missing-version-means-use-cluster-version", + + firstFeatureGate: featureGateBuilder(). + enabled("other-version", "alpha", "bravo"). + disabled("other-version", "charlie", "delta"). + toFeatureGate(), + fields: fields{ + desiredVersion: "missing-version", + missingVersionMarker: "missing-version", + }, + clusterVersion: "other-version", + + changeVerifier: func(t *testing.T, history []FeatureChange) { + if len(history) != 1 { + t.Fatalf("bad changes: %v", history) + } + if history[0].Previous != nil { + t.Fatalf("bad changes: %v", history) + } + if !reflect.DeepEqual([]configv1.FeatureGateName{"alpha", "bravo"}, history[0].New.Enabled) { t.Fatal(history[0].New.Enabled) } - if !reflect.DeepEqual([]string{"charlie", "delta"}, history[0].New.Disabled) { + if !reflect.DeepEqual([]configv1.FeatureGateName{"charlie", "delta"}, history[0].New.Disabled) { t.Fatal(history[0].New.Enabled) } }, @@ -180,34 +242,19 @@ func Test_defaultFeatureGateAccess_syncHandler(t *testing.T) { name: "missing desiredVersion", firstFeatureGate: featureGateBuilder(). - specFeatureSet("features-for-missing-version"). enabled("other-version", "alpha", "bravo"). disabled("other-version", "charlie", "delta"). toFeatureGate(), fields: fields{ - desiredVersion: "missing-version", + desiredVersion: "desired-version", }, wantErr: true, }, } - // set the map. This is very ugly, but will hopefully be replaced by https://github.com/openshift/library-go/pull/1468/files shortly - configv1.FeatureSets["features-for-desired-version"] = &configv1.FeatureGateEnabledDisabled{ - Enabled: []string{"alpha", "bravo"}, - Disabled: []string{"charlie", "delta"}, - } - configv1.FeatureSets["features-for-other-version"] = &configv1.FeatureGateEnabledDisabled{ - Enabled: []string{"alpha", "bravo"}, - Disabled: []string{"charlie", "delta"}, - } - defer func() { - delete(configv1.FeatureSets, "features-for-desired-version") - }() - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := context.Background() _, cancel := context.WithCancel(ctx) defer cancel()