Merge pull request kubernetes#129414 from yongruilin/compatibility-version-featuregate

feat: Refactors featuregate lifecycle management script

Author: k8s-ci-robot

hack/update-featuregates.sh

@@ -14,8 +14,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# This script updates  test/featuregates_linter/test_data/unversioned_feature_list.yaml and
-# test/featuregates_linter/test_data/versioned_feature_list.yaml with all the feature gate features.
+# This script updates test/compatibility_lifecycle/reference/versioned_feature_list.yaml 
+# with all the feature gate features.
 # Usage: `hack/update-featuregates.sh`.
 
 set -o errexit
@@ -27,4 +27,4 @@ source "${KUBE_ROOT}/hack/lib/init.sh"
 
 cd "${KUBE_ROOT}"
 
-go run test/featuregates_linter/main.go feature-gates update
+go run test/compatibility_lifecycle/main.go feature-gates update

hack/verify-featuregates.sh

@@ -14,8 +14,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# This script checks test/featuregates_linter/test_data/unversioned_feature_list.yaml and
-# test/featuregates_linter/test_data/versioned_feature_list.yaml are up to date with all the feature gate features.
+# This script checks test/compatibility_lifecycle/reference/versioned_feature_list.yaml
+# are up to date with all the feature gate features, and verifies no feature is removed before 3 versions post `lockedToDefault:true`.
 # We should run `hack/update-featuregates.sh` if the list is out of date.
 # Usage: `hack/verify-featuregates.sh`.
 
@@ -30,7 +30,7 @@ kube::golang::setup_env
 
 cd "${KUBE_ROOT}"
 
-if ! go run test/featuregates_linter/main.go feature-gates verify; then
+if ! go run test/compatibility_lifecycle/main.go feature-gates verify; then
   echo "Please run 'hack/update-featuregates.sh' to update the feature list."
   exit 1
 fi

test/featuregates_linter/OWNERS renamed to test/compatibility_lifecycle/OWNERS

@@ -0,0 +1,10 @@
+This directory contains commands for [compatibility lifecycle verification](https://github.com/kubernetes/enhancements/blob/master/keps/sig-architecture/4330-compatibility-versions/README.md)
+
+Currently, the following commands are implemented:
+```
+# Verify feature gate list is up to date
+go run test/compatibility_lifecycle/main.go feature-gates verify
+
+# Update feature gate list
+go run test/compatibility_lifecycle/main.go feature-gates update
+```

test/compatibility_lifecycle/README.md

@@ -31,18 +31,25 @@ import (
 	"github.com/spf13/cobra"
 
 	"k8s.io/apimachinery/pkg/util/version"
+	baseversion "k8s.io/component-base/version"
 	yaml "sigs.k8s.io/yaml/goyaml.v2"
 )
 
 var (
-	alphabeticalOrder          bool
-	k8RootPath                 string
-	unversionedFeatureListFile = "test/featuregates_linter/test_data/unversioned_feature_list.yaml"
-	versionedFeatureListFile   = "test/featuregates_linter/test_data/versioned_feature_list.yaml"
+	alphabeticalOrder        bool
+	k8RootPath               string
+	versionedFeatureListFile = "test/compatibility_lifecycle/reference/versioned_feature_list.yaml"
+	// thresholdVersion is the version after which we require emulation support for feature removal
+	// 1.31 is when we introduced emulation version support
+	thresholdVersion = version.MajorMinor(1, 31)
 )
 
 const (
-	featureGatePkg = "\"k8s.io/component-base/featuregate\""
+	featureGatePkg       = "\"k8s.io/component-base/featuregate\""
+	generatedFileWarning = `# This file is generated by compatibility_lifecycle tool.
+# Do not edit manually. Run hack/update-featuregates.sh to regenerate.
+
+`
 )
 
 type featureSpec struct {
@@ -95,39 +102,33 @@ func NewUpdateFeatureListCommand() *cobra.Command {
 }
 
 func verifyFeatureListFunc(cmd *cobra.Command, args []string) {
-	if err := verifyOrUpdateFeatureList(k8RootPath, unversionedFeatureListFile, false, false); err != nil {
-		fmt.Fprintf(os.Stderr, "Failed to verify feature list: \n%s", err)
-		os.Exit(1)
-	}
-	if err := verifyOrUpdateFeatureList(k8RootPath, versionedFeatureListFile, false, true); err != nil {
+	currentVersion := version.MustParse(baseversion.DefaultKubeBinaryVersion)
+	if err := verifyOrUpdateFeatureList(k8RootPath, versionedFeatureListFile, currentVersion, false); err != nil {
 		fmt.Fprintf(os.Stderr, "Failed to verify versioned feature list: \n%s", err)
 		os.Exit(1)
 	}
 }
 
 func updateFeatureListFunc(cmd *cobra.Command, args []string) {
-	if err := verifyOrUpdateFeatureList(k8RootPath, unversionedFeatureListFile, true, false); err != nil {
-		fmt.Fprintf(os.Stderr, "Failed to update feature list: \n%s", err)
-		os.Exit(1)
-	}
-	if err := verifyOrUpdateFeatureList(k8RootPath, versionedFeatureListFile, true, true); err != nil {
+	currentVersion := version.MustParse(baseversion.DefaultKubeBinaryVersion)
+	if err := verifyOrUpdateFeatureList(k8RootPath, versionedFeatureListFile, currentVersion, true); err != nil {
 		fmt.Fprintf(os.Stderr, "Failed to update versioned feature list: \n%s", err)
 		os.Exit(1)
 	}
 }
 
 // verifyOrUpdateFeatureList walks all the files under pkg/ and staging/ to find the list of all the features in
-// map[featuregate.Feature]featuregate.FeatureSpec or map[featuregate.Feature]featuregate.VersionedSpecs.
+// map[featuregate.Feature]featuregate.VersionedSpecs.
 // It will then update the feature list in featureListFile, or verifies there is no change from the existing list.
-func verifyOrUpdateFeatureList(rootPath, featureListFile string, update, versioned bool) error {
+func verifyOrUpdateFeatureList(rootPath, featureListFile string, currentVersion *version.Version, update bool) error {
 	featureList := []featureInfo{}
-	features, err := searchPathForFeatures(filepath.Join(rootPath, "pkg"), versioned)
+	features, err := searchPathForFeatures(filepath.Join(rootPath, "pkg"))
 	if err != nil {
 		return err
 	}
 	featureList = append(featureList, features...)
 
-	features, err = searchPathForFeatures(filepath.Join(rootPath, "staging"), versioned)
+	features, err = searchPathForFeatures(filepath.Join(rootPath, "staging"))
 	if err != nil {
 		return err
 	}
@@ -153,29 +154,22 @@ func verifyOrUpdateFeatureList(rootPath, featureListFile string, update, version
 		return err
 	}
 
-	// only feature deletion is allowed for unversioned features.
-	// all new features or feature updates should be migrated to versioned feature gate.
-	// https://github.com/kubernetes/kubernetes/issues/125031
-	if !versioned {
-		if err := verifyFeatureDeletionOnly(featureList, baseFeatureList); err != nil {
-			return err
-		}
+	if err := verifyFeatureRemoval(featureList, baseFeatureList, currentVersion, thresholdVersion); err != nil {
+		return err
 	}
 
 	featureListBytes, err := yaml.Marshal(featureList)
 	if err != nil {
 		return err
 	}
+	featureListBytes = []byte(generatedFileWarning + string(featureListBytes))
 	if update {
 		return os.WriteFile(filePath, featureListBytes, 0644)
 	}
 
 	if diff := cmp.Diff(featureListBytes, baseFeatureListBytes); diff != "" {
-		if versioned {
-			return fmt.Errorf("detected diff in versioned feature list (%s), diff: \n%s", versionedFeatureListFile, diff)
-		} else {
-			return fmt.Errorf("detected diff in unversioned feature list (%s), diff: \n%s", unversionedFeatureListFile, diff)
-		}
+		return fmt.Errorf("detected diff in versioned feature list (%s), diff: \n%s", versionedFeatureListFile, diff)
+
 	}
 	return nil
 }
@@ -205,27 +199,70 @@ func dedupeFeatureList(featureList []featureInfo) ([]featureInfo, error) {
 	return deduped, nil
 }
 
-func verifyFeatureDeletionOnly(newFeatureList []featureInfo, oldFeatureList []featureInfo) error {
-	oldFeatureSet := make(map[string]*featureInfo)
-	for _, f := range oldFeatureList {
-		oldFeatureSet[f.Name] = &f
-	}
-	newFeatures := []string{}
-	for _, f := range newFeatureList {
-		oldSpecs, found := oldFeatureSet[f.Name]
-		if !found {
-			newFeatures = append(newFeatures, f.Name)
-		} else if !reflect.DeepEqual(*oldSpecs, f) {
-			return fmt.Errorf("feature %s changed with diff: %s", f.Name, cmp.Diff(*oldSpecs, f))
+// verifyFeatureRemoval checks if removed features are allowed to be removed based on their lifecycle.
+// Alpha features can be removed anytime without error.
+// Returns error if:
+//   - Beta features are removed (not allowed)
+//   - GA/Deprecated features are removed without being locked to default
+//   - GA/Deprecated features locked after v1.31 are removed before 3 minor versions
+//     have passed (required for emulation support)
+func verifyFeatureRemoval(featureList []featureInfo, baseFeatureList []featureInfo,
+	currentVersion *version.Version, thresholdVersion *version.Version) error {
+	if thresholdVersion == nil {
+		thresholdVersion = version.MajorMinor(0, 0)
+	}
+	baseFeatures := make(map[string]featureInfo)
+	for _, f := range baseFeatureList {
+		baseFeatures[f.Name] = f
+	}
+	currentFeatures := make(map[string]featureInfo)
+	for _, f := range featureList {
+		currentFeatures[f.Name] = f
+	}
+
+	for name, baseFeature := range baseFeatures {
+		// Check if feature was removed
+		if _, found := currentFeatures[name]; found {
+			continue
+		}
+
+		// Feature was removed, check if allowed
+		specs := baseFeature.VersionedSpecs
+		if len(specs) == 0 {
+			return fmt.Errorf("feature %s has no version specs", name)
+		}
+
+		lastSpec := specs[len(specs)-1]
+		switch lastSpec.PreRelease {
+		case "Alpha":
+			continue // can remove alpha features anytime
+		case "Beta":
+			return fmt.Errorf("feature %s cannot be removed while in beta", name)
+		case "GA", "Deprecated":
+			if !lastSpec.LockToDefault {
+				return fmt.Errorf("feature %s cannot be removed because it is in GA or Deprecated state and is not locked to default", name)
+			}
+			specVer, err := version.Parse(lastSpec.Version)
+			if err != nil {
+				return fmt.Errorf("invalid version \"%s\" for feature %s: %w", lastSpec.Version, name, err)
+			}
+			// we do not require the 3 version retention for features locked before the thresholdVersion.
+			// TODO: remove after 1.34
+			if !specVer.GreaterThan(thresholdVersion) {
+				continue
+			}
+			minRemovalVer := specVer.AddMinor(3)
+			if currentVersion.LessThan(minRemovalVer) {
+				return fmt.Errorf("feature %s cannot be removed until version %s (required for emulation support)",
+					name, minRemovalVer)
+			}
+
 		}
-	}
-	if len(newFeatures) > 0 {
-		return fmt.Errorf("new features added to FeatureSpec map! %v\nPlease add new features through VersionedSpecs map ONLY! ", newFeatures)
 	}
 	return nil
 }
 
-func searchPathForFeatures(path string, versioned bool) ([]featureInfo, error) {
+func searchPathForFeatures(path string) ([]featureInfo, error) {
 	allFeatures := []featureInfo{}
 	// Create a FileSet to work with
 	fset := token.NewFileSet()
@@ -239,7 +276,12 @@ func searchPathForFeatures(path string, versioned bool) ([]featureInfo, error) {
 		if strings.HasSuffix(path, "_test.go") {
 			return nil
 		}
-		features, parseErr := extractFeatureInfoListFromFile(fset, path, versioned)
+		// exclude generated files
+		base := filepath.Base(path)
+		if strings.HasPrefix(base, "zz_generated") {
+			return nil
+		}
+		features, parseErr := extractFeatureInfoListFromFile(fset, path)
 		if parseErr != nil {
 			return parseErr
 		}
@@ -249,9 +291,9 @@ func searchPathForFeatures(path string, versioned bool) ([]featureInfo, error) {
 	return allFeatures, err
 }
 
-// extractFeatureInfoListFromFile extracts info all the the features from
-// map[featuregate.Feature]featuregate.FeatureSpec or map[featuregate.Feature]featuregate.VersionedSpecs from the given file.
-func extractFeatureInfoListFromFile(fset *token.FileSet, filePath string, versioned bool) (allFeatures []featureInfo, err error) {
+// extractFeatureInfoListFromFile extracts info of all the features from
+// map[featuregate.Feature]featuregate.VersionedSpecs in the given file.
+func extractFeatureInfoListFromFile(fset *token.FileSet, filePath string) (allFeatures []featureInfo, err error) {
 	// Parse the file and create an AST
 	absFilePath, err := filepath.Abs(filePath)
 	if err != nil {
@@ -274,7 +316,7 @@ func extractFeatureInfoListFromFile(fset *token.FileSet, filePath string, versio
 				if vspec, ok := spec.(*ast.ValueSpec); ok {
 					for _, name := range vspec.Names {
 						for _, value := range vspec.Values {
-							features, err := extractFeatureInfoList(filePath, value, aliasMap, variables, versioned)
+							features, err := extractFeatureInfoList(filePath, value, aliasMap, variables)
 							if err != nil {
 								return allFeatures, err
 							}
@@ -291,7 +333,7 @@ func extractFeatureInfoListFromFile(fset *token.FileSet, filePath string, versio
 			for _, stmt := range fd.Body.List {
 				if st, ok := stmt.(*ast.ReturnStmt); ok {
 					for _, value := range st.Results {
-						features, err := extractFeatureInfoList(filePath, value, aliasMap, variables, versioned)
+						features, err := extractFeatureInfoList(filePath, value, aliasMap, variables)
 						if err != nil {
 							return allFeatures, err
 						}
@@ -332,8 +374,8 @@ func verifyAlphabeticOrder(keys []string, path string) error {
 }
 
 // extractFeatureInfoList extracts the info all the the features from
-// map[featuregate.Feature]featuregate.FeatureSpec or map[featuregate.Feature]featuregate.VersionedSpecs.
-func extractFeatureInfoList(filePath string, v ast.Expr, aliasMap map[string]string, variables map[string]ast.Expr, versioned bool) ([]featureInfo, error) {
+// map[featuregate.Feature]featuregate.VersionedSpecs.
+func extractFeatureInfoList(filePath string, v ast.Expr, aliasMap map[string]string, variables map[string]ast.Expr) ([]featureInfo, error) {
 	keys := []string{}
 	features := []featureInfo{}
 	cl, ok := v.(*ast.CompositeLit)
@@ -344,15 +386,15 @@ func extractFeatureInfoList(filePath string, v ast.Expr, aliasMap map[string]str
 	if !ok {
 		return features, nil
 	}
-	if !isFeatureSpecType(mt.Value, aliasMap, versioned) {
+	if !isFeatureSpecType(mt.Value, aliasMap) {
 		return features, nil
 	}
 	for _, elt := range cl.Elts {
 		kv, ok := elt.(*ast.KeyValueExpr)
 		if !ok {
 			continue
 		}
-		info, err := parseFeatureInfo(variables, kv, versioned)
+		info, err := parseFeatureInfo(variables, kv)
 		if err != nil {
 			return features, err
 		}
@@ -368,31 +410,24 @@ func extractFeatureInfoList(filePath string, v ast.Expr, aliasMap map[string]str
 	return features, nil
 }
 
-func isFeatureSpecType(v ast.Expr, aliasMap map[string]string, versioned bool) bool {
-	typeName := "FeatureSpec"
-	if versioned {
-		typeName = "VersionedSpecs"
-	}
+func isFeatureSpecType(v ast.Expr, aliasMap map[string]string) bool {
+	typeName := "VersionedSpecs"
 	alias, ok := aliasMap[featureGatePkg]
 	if ok {
 		typeName = alias + "." + typeName
 	}
 	return identifierName(v, false) == typeName
 }
 
-func parseFeatureInfo(variables map[string]ast.Expr, kv *ast.KeyValueExpr, versioned bool) (featureInfo, error) {
+func parseFeatureInfo(variables map[string]ast.Expr, kv *ast.KeyValueExpr) (featureInfo, error) {
 	info := featureInfo{
 		Name:           identifierName(kv.Key, true),
 		FullName:       identifierName(kv.Key, false),
 		VersionedSpecs: []featureSpec{},
 	}
 	specExps := []ast.Expr{}
-	if versioned {
-		if cl, ok := kv.Value.(*ast.CompositeLit); ok {
-			specExps = append(specExps, cl.Elts...)
-		}
-	} else {
-		specExps = append(specExps, kv.Value)
+	if cl, ok := kv.Value.(*ast.CompositeLit); ok {
+		specExps = append(specExps, cl.Elts...)
 	}
 	for _, specExp := range specExps {
 		spec, err := parseFeatureSpec(variables, specExp)