Merge pull request openshift#7413 from patrickdillon/fg-val

CORS-2877: Install Config Feature Gate Validation

Author: openshift-merge-bot[bot]

pkg/asset/manifests/featuregate.go

@@ -2,8 +2,6 @@ package manifests
 
 import (
 	"path/filepath"
-	"strconv"
-	"strings"
 
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -12,6 +10,7 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/installer/pkg/asset"
 	"github.com/openshift/installer/pkg/asset/installconfig"
+	"github.com/openshift/installer/pkg/types/featuregates"
 )
 
 var fgFileName = filepath.Join(openshiftManifestDir, "99_feature-gate.yaml")
@@ -62,10 +61,7 @@ func (f *FeatureGate) Generate(dependencies asset.Parents) error {
 			return errors.Errorf("custom features can only be used with the CustomNoUpgrade feature set")
 		}
 
-		customFeatures, err := generateCustomFeatures(installConfig.Config.FeatureGates)
-		if err != nil {
-			return errors.Wrapf(err, "failed to generate custom features")
-		}
+		customFeatures := featuregates.GenerateCustomFeatures(installConfig.Config.FeatureGates)
 		f.Config.Spec.CustomNoUpgrade = customFeatures
 	}
 
@@ -93,45 +89,3 @@ func (f *FeatureGate) Files() []*asset.File {
 func (f *FeatureGate) Load(ff asset.FileFetcher) (bool, error) {
 	return false, nil
 }
-
-// generateCustomFeatures generates the custom feature gates from the install config.
-func generateCustomFeatures(features []string) (*configv1.CustomFeatureGates, error) {
-	customFeatures := &configv1.CustomFeatureGates{}
-
-	for _, feature := range features {
-		featureName, enabled, err := parseCustomFeatureGate(feature)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to parse custom feature %s", feature)
-		}
-
-		if enabled {
-			customFeatures.Enabled = append(customFeatures.Enabled, featureName)
-		} else {
-			customFeatures.Disabled = append(customFeatures.Disabled, featureName)
-		}
-	}
-
-	return customFeatures, nil
-}
-
-// parseCustomFeatureGates parses the custom feature gate string into the feature name and whether it is enabled.
-// The expected format is <FeatureName>=<Enabled>.
-func parseCustomFeatureGate(rawFeature string) (configv1.FeatureGateName, bool, error) {
-	var featureName string
-	var enabled bool
-
-	featureParts := strings.Split(rawFeature, "=")
-	if len(featureParts) != 2 {
-		return "", false, errors.Errorf("feature not in expected format %s", rawFeature)
-	}
-
-	featureName = featureParts[0]
-
-	var err error
-	enabled, err = strconv.ParseBool(featureParts[1])
-	if err != nil {
-		return "", false, errors.Wrapf(err, "feature not in expected format %s, could not parse boolean value", rawFeature)
-	}
-
-	return configv1.FeatureGateName(featureName), enabled, nil
-}

pkg/types/featuregates/featuregate.go

@@ -0,0 +1,57 @@
+package featuregates
+
+// source: https://github.com/openshift/library-go/blob/c515269de16e5e239bd6e93e1f9821a976bb460b/pkg/operator/configobserver/featuregates/featuregate.go#L23-L28
+
+import (
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	configv1 "github.com/openshift/api/config/v1"
+)
+
+// FeatureGate indicates whether a given feature is enabled or not
+// This interface is heavily influenced by k8s.io/component-base, but not exactly compatible.
+type FeatureGate interface {
+	// Enabled returns true if the key is enabled.
+	Enabled(key configv1.FeatureGateName) bool
+}
+
+type featureGate struct {
+	enabled  sets.Set[configv1.FeatureGateName]
+	disabled sets.Set[configv1.FeatureGateName]
+}
+
+// GatedInstallConfigFeature contains fields that will be used to validate
+// that required feature gates are enabled when gated install config fields
+// are used.
+// FeatureGateName: openshift/api feature gate required to enable the use of Field
+// Condition: the check which determines whether the install config field is used,
+// if Condition evaluates to True, FeatureGateName must be enabled  to pass validation.
+// Field: the gated install config field, Field is used in the error message.
+type GatedInstallConfigFeature struct {
+	FeatureGateName configv1.FeatureGateName
+	Condition       bool
+	Field           *field.Path
+}
+
+func newFeatureGate(enabled, disabled []configv1.FeatureGateName) FeatureGate {
+	return &featureGate{
+		enabled:  sets.New[configv1.FeatureGateName](enabled...),
+		disabled: sets.New[configv1.FeatureGateName](disabled...),
+	}
+}
+
+// Enabled returns true if the provided feature gate is enabled
+// in the active feature sets.
+func (f *featureGate) Enabled(key configv1.FeatureGateName) bool {
+	if f.enabled.Has(key) {
+		return true
+	}
+	if f.disabled.Has(key) {
+		return false
+	}
+
+	panic(fmt.Errorf("feature %q is not registered in FeatureGates", key))
+}

pkg/types/featuregates/featuregates.go

@@ -0,0 +1,93 @@
+package featuregates
+
+// source: https://github.com/openshift/cluster-config-operator/blob/636a2dc303037e2561a243ae1ab5c5b953ddad04/pkg/cmd/render/render.go#L153
+
+import (
+	"strconv"
+	"strings"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+
+	configv1 "github.com/openshift/api/config/v1"
+)
+
+func toFeatureGateNames(in []configv1.FeatureGateDescription) []configv1.FeatureGateName {
+	out := []configv1.FeatureGateName{}
+	for _, curr := range in {
+		out = append(out, curr.FeatureGateAttributes.Name)
+	}
+
+	return out
+}
+
+// completeFeatureGates identifies every known feature and ensures that is explicitly on or explicitly off.
+func completeFeatureGates(knownFeatureSets map[configv1.FeatureSet]*configv1.FeatureGateEnabledDisabled, enabled, disabled []configv1.FeatureGateName) ([]configv1.FeatureGateName, []configv1.FeatureGateName) {
+	specificallyEnabledFeatureGates := sets.New[configv1.FeatureGateName]()
+	specificallyEnabledFeatureGates.Insert(enabled...)
+
+	knownFeatureGates := sets.New[configv1.FeatureGateName]()
+	knownFeatureGates.Insert(enabled...)
+	knownFeatureGates.Insert(disabled...)
+	for _, known := range knownFeatureSets {
+		for _, curr := range known.Disabled {
+			knownFeatureGates.Insert(curr.FeatureGateAttributes.Name)
+		}
+		for _, curr := range known.Enabled {
+			knownFeatureGates.Insert(curr.FeatureGateAttributes.Name)
+		}
+	}
+
+	return enabled, knownFeatureGates.Difference(specificallyEnabledFeatureGates).UnsortedList()
+}
+
+// FeatureGateFromFeatureSets creates a FeatureGate from the active feature sets.
+func FeatureGateFromFeatureSets(knownFeatureSets map[configv1.FeatureSet]*configv1.FeatureGateEnabledDisabled, fs configv1.FeatureSet, customFS *configv1.CustomFeatureGates) FeatureGate {
+	if customFS != nil {
+		completeEnabled, completeDisabled := completeFeatureGates(knownFeatureSets, customFS.Enabled, customFS.Disabled)
+		return newFeatureGate(completeEnabled, completeDisabled)
+	}
+
+	featureSet := knownFeatureSets[fs]
+
+	completeEnabled, completeDisabled := completeFeatureGates(knownFeatureSets, toFeatureGateNames(featureSet.Enabled), toFeatureGateNames(featureSet.Disabled))
+	return newFeatureGate(completeEnabled, completeDisabled)
+}
+
+// GenerateCustomFeatures generates the custom feature gates from the install config.
+func GenerateCustomFeatures(features []string) *configv1.CustomFeatureGates {
+	customFeatures := &configv1.CustomFeatureGates{}
+
+	for _, feature := range features {
+		featureName, enabled := parseCustomFeatureGate(feature)
+
+		if enabled {
+			customFeatures.Enabled = append(customFeatures.Enabled, featureName)
+		} else {
+			customFeatures.Disabled = append(customFeatures.Disabled, featureName)
+		}
+	}
+
+	return customFeatures
+}
+
+// parseCustomFeatureGates parses the custom feature gate string into the feature name and whether it is enabled.
+// The expected format is <FeatureName>=<Enabled>.
+func parseCustomFeatureGate(rawFeature string) (configv1.FeatureGateName, bool) {
+	var featureName string
+	var enabled bool
+
+	featureParts := strings.Split(rawFeature, "=")
+	if len(featureParts) != 2 {
+		return "", false
+	}
+
+	featureName = featureParts[0]
+
+	var err error
+	enabled, err = strconv.ParseBool(featureParts[1])
+	if err != nil {
+		return "", false
+	}
+
+	return configv1.FeatureGateName(featureName), enabled
+}

pkg/types/gcp/validation/featuregates.go

@@ -0,0 +1,27 @@
+package validation
+
+import (
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	configv1 "github.com/openshift/api/config/v1"
+	"github.com/openshift/installer/pkg/types"
+	"github.com/openshift/installer/pkg/types/featuregates"
+)
+
+// GatedFeatures determines all of the install config fields that should
+// be validated to ensure that the proper featuregate is enabled when the field is used.
+func GatedFeatures(c *types.InstallConfig) []featuregates.GatedInstallConfigFeature {
+	g := c.GCP
+	return []featuregates.GatedInstallConfigFeature{
+		{
+			FeatureGateName: configv1.FeatureGateGCPLabelsTags,
+			Condition:       len(g.UserLabels) > 0,
+			Field:           field.NewPath("platform", "gcp", "userLabels"),
+		},
+		{
+			FeatureGateName: configv1.FeatureGateGCPLabelsTags,
+			Condition:       len(g.UserTags) > 0,
+			Field:           field.NewPath("platform", "gcp", "userTags"),
+		},
+	}
+}

pkg/types/installconfig.go

@@ -13,6 +13,7 @@ import (
 	"github.com/openshift/installer/pkg/types/azure"
 	"github.com/openshift/installer/pkg/types/baremetal"
 	"github.com/openshift/installer/pkg/types/external"
+	"github.com/openshift/installer/pkg/types/featuregates"
 	"github.com/openshift/installer/pkg/types/gcp"
 	"github.com/openshift/installer/pkg/types/ibmcloud"
 	"github.com/openshift/installer/pkg/types/libvirt"
@@ -516,3 +517,17 @@ func (c *InstallConfig) WorkerMachinePool() *MachinePool {
 
 	return nil
 }
+
+// EnabledFeatureGates returns a FeatureGate that can be checked (using the Enabled function)
+// to determine if a feature gate is enabled in the current feature sets.
+func (c *InstallConfig) EnabledFeatureGates() featuregates.FeatureGate {
+	var customFS *configv1.CustomFeatureGates
+
+	if c.FeatureSet == configv1.CustomNoUpgrade {
+		customFS = featuregates.GenerateCustomFeatures(c.FeatureGates)
+	}
+
+	fg := featuregates.FeatureGateFromFeatureSets(configv1.FeatureSets, c.FeatureSet, customFS)
+
+	return fg
+}

pkg/types/validation/featuregate_test.go

@@ -0,0 +1,115 @@
+package validation
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	v1 "github.com/openshift/api/config/v1"
+	"github.com/openshift/installer/pkg/types"
+	"github.com/openshift/installer/pkg/types/gcp"
+	"github.com/openshift/installer/pkg/types/vsphere"
+)
+
+func TestFeatureGates(t *testing.T) {
+	cases := []struct {
+		name          string
+		installConfig *types.InstallConfig
+		expected      string
+	}{
+		{
+			name: "GCP UserTags is allowed with Feature Gates enabled",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.FeatureSet = v1.TechPreviewNoUpgrade
+				c.GCP = validGCPPlatform()
+				c.GCP.UserTags = []gcp.UserTag{{ParentID: "a", Key: "b", Value: "c"}}
+				return c
+			}(),
+		},
+		{
+			name: "GCP UserTags is not allowed without Feature Gates",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.GCP = validGCPPlatform()
+				c.GCP.UserTags = []gcp.UserTag{{ParentID: "a", Key: "b", Value: "c"}}
+				return c
+			}(),
+			expected: `^platform.gcp.userTags: Forbidden: this field is protected by the GCPLabelsTags feature gate which must be enabled through either the TechPreviewNoUpgrade or CustomNoUpgrade feature set$`,
+		},
+		{
+			name: "GCP UserLabels is allowed with Feature Gates enabled",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.FeatureSet = v1.TechPreviewNoUpgrade
+				c.GCP = validGCPPlatform()
+				c.GCP.UserLabels = []gcp.UserLabel{{Key: "a", Value: "b"}}
+				return c
+			}(),
+		},
+		{
+			name: "GCP UserLabels is not allowed without Feature Gates",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.GCP = validGCPPlatform()
+				c.GCP.UserLabels = []gcp.UserLabel{{Key: "a", Value: "b"}}
+				return c
+			}(),
+			expected: `^platform.gcp.userLabels: Forbidden: this field is protected by the GCPLabelsTags feature gate which must be enabled through either the TechPreviewNoUpgrade or CustomNoUpgrade feature set$`,
+		},
+		{
+			name: "vSphere hosts is allowed with Feature Gates enabled",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.FeatureSet = v1.TechPreviewNoUpgrade
+				c.VSphere = validVSpherePlatform()
+				c.VSphere.Hosts = []*vsphere.Host{{Role: "test"}}
+				return c
+			}(),
+		},
+		{
+			name: "vSphere hosts is not allowed without Feature Gates",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.VSphere = validVSpherePlatform()
+				c.VSphere.Hosts = []*vsphere.Host{{Role: "test"}}
+				return c
+			}(),
+			expected: `^platform.vsphere.hosts: Forbidden: this field is protected by the VSphereStaticIPs feature gate which must be enabled through either the TechPreviewNoUpgrade or CustomNoUpgrade feature set$`,
+		},
+		{
+			name: "vSphere hosts is allowed with custom Feature Gates",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.FeatureSet = v1.CustomNoUpgrade
+				c.FeatureGates = []string{"VSphereStaticIPs=true"}
+				c.VSphere = validVSpherePlatform()
+				c.VSphere.Hosts = []*vsphere.Host{{Role: "test"}}
+				return c
+			}(),
+		},
+		{
+			name: "vSphere hosts is not allowed with custom Feature Gate disabled",
+			installConfig: func() *types.InstallConfig {
+				c := validInstallConfig()
+				c.FeatureSet = v1.CustomNoUpgrade
+				c.FeatureGates = []string{"VSphereStaticIPs=false"}
+				c.VSphere = validVSpherePlatform()
+				c.VSphere.Hosts = []*vsphere.Host{{Role: "test"}}
+				return c
+			}(),
+			expected: `^platform.vsphere.hosts: Forbidden: this field is protected by the VSphereStaticIPs feature gate which must be enabled through either the TechPreviewNoUpgrade or CustomNoUpgrade feature set$`,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := ValidateFeatureSet(tc.installConfig).ToAggregate()
+			if tc.expected == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.Regexp(t, tc.expected, err)
+			}
+		})
+	}
+}