Read featuregates from cluster state

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
+}

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 {

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)
 			}