From 088daf472b7d1ed4502aef211efca6653b05ff59 Mon Sep 17 00:00:00 2001 From: yongruilin Date: Fri, 27 Dec 2024 11:13:14 -0800 Subject: [PATCH] feat: Refactors featuregate lifecycle management script - rename featuregate_linter to compatibility_lifecycle - add feature removal verify to follow N+3 rule - remove unversioned related operation - rename yaml folder name to "reference" --- hack/update-featuregates.sh | 6 +- hack/verify-featuregates.sh | 6 +- .../OWNERS | 0 test/compatibility_lifecycle/README.md | 10 + .../cmd/feature_gates.go | 171 +++--- .../cmd/feature_gates_test.go | 547 ++++++------------ .../cmd/root.go | 6 +- .../cmd/util.go | 0 .../main.go | 2 +- .../reference}/OWNERS | 0 .../reference}/unversioned_feature_list.yaml | 0 .../reference}/versioned_feature_list.yaml | 3 + test/featuregates_linter/README.md | 8 - 13 files changed, 303 insertions(+), 456 deletions(-) rename test/{featuregates_linter => compatibility_lifecycle}/OWNERS (100%) create mode 100644 test/compatibility_lifecycle/README.md rename test/{featuregates_linter => compatibility_lifecycle}/cmd/feature_gates.go (72%) rename test/{featuregates_linter => compatibility_lifecycle}/cmd/feature_gates_test.go (65%) rename test/{featuregates_linter => compatibility_lifecycle}/cmd/root.go (84%) rename test/{featuregates_linter => compatibility_lifecycle}/cmd/util.go (100%) rename test/{featuregates_linter => compatibility_lifecycle}/main.go (91%) rename test/{featuregates_linter/test_data => compatibility_lifecycle/reference}/OWNERS (100%) rename test/{featuregates_linter/test_data => compatibility_lifecycle/reference}/unversioned_feature_list.yaml (100%) rename test/{featuregates_linter/test_data => compatibility_lifecycle/reference}/versioned_feature_list.yaml (99%) delete mode 100644 test/featuregates_linter/README.md diff --git a/hack/update-featuregates.sh b/hack/update-featuregates.sh index 7e72b575806..792c6e05bc2 100755 --- a/hack/update-featuregates.sh +++ b/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 diff --git a/hack/verify-featuregates.sh b/hack/verify-featuregates.sh index 42c3a5acb57..1122d62d795 100755 --- a/hack/verify-featuregates.sh +++ b/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 diff --git a/test/featuregates_linter/OWNERS b/test/compatibility_lifecycle/OWNERS similarity index 100% rename from test/featuregates_linter/OWNERS rename to test/compatibility_lifecycle/OWNERS diff --git a/test/compatibility_lifecycle/README.md b/test/compatibility_lifecycle/README.md new file mode 100644 index 00000000000..93c045bdfe8 --- /dev/null +++ b/test/compatibility_lifecycle/README.md @@ -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 +``` diff --git a/test/featuregates_linter/cmd/feature_gates.go b/test/compatibility_lifecycle/cmd/feature_gates.go similarity index 72% rename from test/featuregates_linter/cmd/feature_gates.go rename to test/compatibility_lifecycle/cmd/feature_gates.go index 0a4f56abd4e..955750367f1 100644 --- a/test/featuregates_linter/cmd/feature_gates.go +++ b/test/compatibility_lifecycle/cmd/feature_gates.go @@ -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 +// 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) } - 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)) + 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,7 +386,7 @@ 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 { @@ -352,7 +394,7 @@ func extractFeatureInfoList(filePath string, v ast.Expr, aliasMap map[string]str if !ok { continue } - info, err := parseFeatureInfo(variables, kv, versioned) + info, err := parseFeatureInfo(variables, kv) if err != nil { return features, err } @@ -368,11 +410,8 @@ 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 @@ -380,19 +419,15 @@ func isFeatureSpecType(v ast.Expr, aliasMap map[string]string, versioned bool) b 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) diff --git a/test/featuregates_linter/cmd/feature_gates_test.go b/test/compatibility_lifecycle/cmd/feature_gates_test.go similarity index 65% rename from test/featuregates_linter/cmd/feature_gates_test.go rename to test/compatibility_lifecycle/cmd/feature_gates_test.go index 14fb8097a8e..eabc287a86b 100644 --- a/test/featuregates_linter/cmd/feature_gates_test.go +++ b/test/compatibility_lifecycle/cmd/feature_gates_test.go @@ -17,18 +17,26 @@ limitations under the License. package cmd import ( - "fmt" "go/ast" "go/token" "log" "os" "path/filepath" "reflect" + "strings" "testing" "github.com/google/go-cmp/cmp" + "k8s.io/apimachinery/pkg/util/version" ) +const expectedHeader = `# This file is generated by compatibility_lifecycle tool. +# Do not edit manually. Run hack/update-featuregates.sh to regenerate. + +` + +var testCurrentVersion = version.MustParse("1.32") + func TestVerifyAlphabeticOrder(t *testing.T) { tests := []struct { name string @@ -87,210 +95,8 @@ func TestVerifyAlphabeticOrder(t *testing.T) { } } -func TestVerifyOrUpdateFeatureListUnversioned(t *testing.T) { - featureListFileContent := `- name: AppArmorFields - versionedSpecs: - - default: true - lockToDefault: false - preRelease: Beta - version: "" -- name: ClusterTrustBundleProjection - versionedSpecs: - - default: false - lockToDefault: false - preRelease: Alpha - version: "" -- name: CPUCFSQuotaPeriod - versionedSpecs: - - default: false - lockToDefault: false - preRelease: Alpha - version: "" -` - tests := []struct { - name string - goFileContent string - featureListFileContent string - updatedFeatureListFileContent string - expectVerifyErr bool - expectUpdateErr bool - }{ - { - name: "no change", - goFileContent: ` -package features - -import ( - "k8s.io/component-base/featuregate" -) -var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, - CPUCFSQuotaPeriod: {Default: false, PreRelease: featuregate.Alpha}, - ClusterTrustBundleProjection: {Default: false, PreRelease: featuregate.Alpha}, -} -var otherFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, -} -`, - featureListFileContent: featureListFileContent, - updatedFeatureListFileContent: featureListFileContent, - }, - { - name: "semantically equivalent, formatting wrong", - goFileContent: ` -package features - -import ( - "k8s.io/component-base/featuregate" -) -var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, - CPUCFSQuotaPeriod: {Default: false, PreRelease: featuregate.Alpha}, - ClusterTrustBundleProjection: {Default: false, PreRelease: featuregate.Alpha}, -} -var otherFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, -} -`, - featureListFileContent: `- name: AppArmorFields - versionedSpecs: - - default: true - lockToDefault: false - preRelease: Beta - version: "" -- name: ClusterTrustBundleProjection - versionedSpecs: - - default: false - lockToDefault: false - preRelease: Alpha - version: "" -- name: CPUCFSQuotaPeriod - versionedSpecs: - - default: false - lockToDefault: false - preRelease: Alpha - version: "" -`, - updatedFeatureListFileContent: featureListFileContent, - expectVerifyErr: true, - }, - { - name: "same feature added twice with different lifecycle", - goFileContent: ` -package features - -import ( - "k8s.io/component-base/featuregate" -) -var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, - CPUCFSQuotaPeriod: {Default: false, PreRelease: featuregate.Alpha}, - ClusterTrustBundleProjection: {Default: false, PreRelease: featuregate.Alpha}, -} - var otherFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Alpha}, -} -`, - featureListFileContent: featureListFileContent, - expectVerifyErr: true, - expectUpdateErr: true, - }, - { - name: "new feature added", - goFileContent: ` -package features - -import ( - "k8s.io/component-base/featuregate" -) -var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, - CPUCFSQuotaPeriod: {Default: false, PreRelease: featuregate.Alpha}, - ClusterTrustBundleProjection: {Default: false, PreRelease: featuregate.Alpha}, - SELinuxMount: {Default: false, PreRelease: featuregate.Alpha}, -} -`, - featureListFileContent: featureListFileContent, - expectVerifyErr: true, - expectUpdateErr: true, - }, - { - name: "delete feature", - goFileContent: ` -package features - -import ( - "k8s.io/component-base/featuregate" -) -var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, - ClusterTrustBundleProjection: {Default: false, PreRelease: featuregate.Alpha}, -} -`, - featureListFileContent: featureListFileContent, - expectVerifyErr: true, - updatedFeatureListFileContent: `- name: AppArmorFields - versionedSpecs: - - default: true - lockToDefault: false - preRelease: Beta - version: "" -- name: ClusterTrustBundleProjection - versionedSpecs: - - default: false - lockToDefault: false - preRelease: Alpha - version: "" -`, - }, - { - name: "update feature", - goFileContent: ` -package features - -import ( - "k8s.io/component-base/featuregate" -) -var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.GA}, - CPUCFSQuotaPeriod: {Default: false, PreRelease: featuregate.Alpha}, - ClusterTrustBundleProjection: {Default: false, PreRelease: featuregate.Alpha}, -} - `, - featureListFileContent: featureListFileContent, - expectVerifyErr: true, - expectUpdateErr: true, - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - featureListFile := writeContentToTmpFile(t, "", "feature_list.yaml", tc.featureListFileContent) - tmpDir := filepath.Dir(featureListFile.Name()) - _ = writeContentToTmpFile(t, tmpDir, "pkg/new_features.go", tc.goFileContent) - err := verifyOrUpdateFeatureList(tmpDir, filepath.Base(featureListFile.Name()), false, false) - if tc.expectVerifyErr != (err != nil) { - t.Errorf("expectVerifyErr=%v, got err: %s", tc.expectVerifyErr, err) - } - err = verifyOrUpdateFeatureList(tmpDir, filepath.Base(featureListFile.Name()), true, false) - if tc.expectUpdateErr != (err != nil) { - t.Errorf("expectUpdateErr=%v, got err: %s", tc.expectUpdateErr, err) - } - if tc.expectUpdateErr { - return - } - updatedFeatureListFileContent, err := os.ReadFile(featureListFile.Name()) - if err != nil { - t.Fatal(err) - } - if diff := cmp.Diff(string(updatedFeatureListFileContent), tc.updatedFeatureListFileContent); diff != "" { - t.Errorf("updatedFeatureListFileContent does not match expected, diff=%s", diff) - } - }) - } -} - func TestVerifyOrUpdateFeatureListVersioned(t *testing.T) { - featureListFileContent := `- name: APIListChunking + featureListFileContent := expectedHeader + `- name: APIListChunking versionedSpecs: - default: true lockToDefault: true @@ -378,7 +184,7 @@ var otherFeatureGates = map[featuregate.Feature]featuregate.VersionedSpecs{ }, } `, - featureListFileContent: `- name: APIListChunking + featureListFileContent: expectedHeader + `- name: APIListChunking versionedSpecs: - default: true lockToDefault: true @@ -488,7 +294,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate `, expectVerifyErr: true, featureListFileContent: featureListFileContent, - updatedFeatureListFileContent: `- name: APIListChunking + updatedFeatureListFileContent: expectedHeader + `- name: APIListChunking versionedSpecs: - default: true lockToDefault: true @@ -519,7 +325,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate `, }, { - name: "remove feature", + name: "not allowed to remove feature", goFileContent: ` package features @@ -538,8 +344,9 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate } `, expectVerifyErr: true, + expectUpdateErr: true, featureListFileContent: featureListFileContent, - updatedFeatureListFileContent: `- name: APIListChunking + updatedFeatureListFileContent: expectedHeader + `- name: APIListChunking versionedSpecs: - default: true lockToDefault: true @@ -582,7 +389,7 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate `, expectVerifyErr: true, featureListFileContent: featureListFileContent, - updatedFeatureListFileContent: `- name: APIListChunking + updatedFeatureListFileContent: expectedHeader + `- name: APIListChunking versionedSpecs: - default: true lockToDefault: true @@ -616,13 +423,13 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate featureListFile := writeContentToTmpFile(t, "", "feature_list.yaml", tc.featureListFileContent) tmpDir := filepath.Dir(featureListFile.Name()) _ = writeContentToTmpFile(t, tmpDir, "pkg/new_features.go", tc.goFileContent) - err := verifyOrUpdateFeatureList(tmpDir, filepath.Base(featureListFile.Name()), false, true) + err := verifyOrUpdateFeatureList(tmpDir, filepath.Base(featureListFile.Name()), testCurrentVersion, false) if tc.expectVerifyErr != (err != nil) { t.Errorf("expectVerifyErr=%v, got err: %s", tc.expectVerifyErr, err) } - err = verifyOrUpdateFeatureList(tmpDir, filepath.Base(featureListFile.Name()), true, true) + err = verifyOrUpdateFeatureList(tmpDir, filepath.Base(featureListFile.Name()), testCurrentVersion, true) if tc.expectUpdateErr != (err != nil) { - t.Errorf("expectVerifyErr=%v, got err: %s", tc.expectVerifyErr, err) + t.Errorf("expectUpdateErr=%v, got err: %s", tc.expectUpdateErr, err) } if tc.expectUpdateErr { return @@ -638,162 +445,6 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate } } -func TestExtractFeatureInfoListFromFile(t *testing.T) { - tests := []struct { - name string - fileContent string - expectedFeatures []featureInfo - }{ - { - name: "map in var", - fileContent: ` -package features - -import ( - "k8s.io/apimachinery/pkg/util/version" - genericfeatures "k8s.io/apiserver/pkg/features" - "k8s.io/component-base/featuregate" -) -var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: featuregate.Beta}, - CPUCFSQuotaPeriod: {Default: false, PreRelease: featuregate.Alpha}, - genericfeatures.AggregatedDiscoveryEndpoint: {Default: false, PreRelease: featuregate.Alpha}, -} - `, - expectedFeatures: []featureInfo{ - { - Name: "AppArmorFields", - FullName: "AppArmorFields", - VersionedSpecs: []featureSpec{ - {Default: true, PreRelease: "Beta"}, - }, - }, - { - Name: "CPUCFSQuotaPeriod", - FullName: "CPUCFSQuotaPeriod", - VersionedSpecs: []featureSpec{ - {Default: false, PreRelease: "Alpha"}, - }, - }, - { - Name: "AggregatedDiscoveryEndpoint", - FullName: "genericfeatures.AggregatedDiscoveryEndpoint", - VersionedSpecs: []featureSpec{ - {Default: false, PreRelease: "Alpha"}, - }, - }, - }, - }, - { - name: "map in var with alias", - fileContent: ` -package features - -import ( - "k8s.io/apimachinery/pkg/util/version" - genericfeatures "k8s.io/apiserver/pkg/features" - fg "k8s.io/component-base/featuregate" -) -const ( - CPUCFSQuotaPeriodDefault = false -) -var defaultVersionedKubernetesFeatureGates = map[fg.Feature]fg.FeatureSpec{ - AppArmorFields: {Default: true, PreRelease: fg.Beta}, - CPUCFSQuotaPeriod: {Default: CPUCFSQuotaPeriodDefault, PreRelease: fg.Alpha}, - genericfeatures.AggregatedDiscoveryEndpoint: {Default: false, PreRelease: fg.Alpha}, -} - `, - expectedFeatures: []featureInfo{ - { - Name: "AppArmorFields", - FullName: "AppArmorFields", - VersionedSpecs: []featureSpec{ - {Default: true, PreRelease: "Beta"}, - }, - }, - { - Name: "CPUCFSQuotaPeriod", - FullName: "CPUCFSQuotaPeriod", - VersionedSpecs: []featureSpec{ - {Default: false, PreRelease: "Alpha"}, - }, - }, - { - Name: "AggregatedDiscoveryEndpoint", - FullName: "genericfeatures.AggregatedDiscoveryEndpoint", - VersionedSpecs: []featureSpec{ - {Default: false, PreRelease: "Alpha"}, - }, - }, - }, - }, - { - name: "map in function return statement", - fileContent: ` -package features - -import ( - "k8s.io/component-base/featuregate" -) - -const ( - ComponentSLIs featuregate.Feature = "ComponentSLIs" -) - -func featureGates() map[featuregate.Feature]featuregate.FeatureSpec { - return map[featuregate.Feature]featuregate.FeatureSpec{ - ComponentSLIs: {Default: true, PreRelease: featuregate.Beta}, - } -} - `, - expectedFeatures: []featureInfo{ - { - Name: "ComponentSLIs", - FullName: "ComponentSLIs", - VersionedSpecs: []featureSpec{ - {Default: true, PreRelease: "Beta"}, - }, - }, - }, - }, - // { - // name: "map in function call", - // fileContent: ` - // package features - - // import ( - // "k8s.io/component-base/featuregate" - // ) - - // const ( - // ComponentSLIs featuregate.Feature = "ComponentSLIs" - // ) - - // func featureGates() featuregate.FeatureGate { - // featureGate := featuregate.NewFeatureGate() - // _ = featureGate.Add(map[featuregate.Feature]featuregate.FeatureSpec{ - // ComponentSLIs: { - // Default: true, PreRelease: featuregate.Beta}}) - // return featureGate - // } - // `, - // }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - newFile := writeContentToTmpFile(t, "", "new_features.go", tc.fileContent) - fset := token.NewFileSet() - features, err := extractFeatureInfoListFromFile(fset, newFile.Name(), false) - if err != nil { - t.Fatal(err) - } - if diff := cmp.Diff(features, tc.expectedFeatures); diff != "" { - t.Errorf("File contents: got=%v, want=%v, diff=%s", features, tc.expectedFeatures, diff) - } - }) - } -} - func TestExtractFeatureInfoListFromFileVersioned(t *testing.T) { tests := []struct { name string @@ -961,7 +612,7 @@ func featureGates() map[featuregate.Feature]featuregate.VersionedSpecs { t.Run(tc.name, func(t *testing.T) { newFile := writeContentToTmpFile(t, "", "new_features.go", tc.fileContent) fset := token.NewFileSet() - features, err := extractFeatureInfoListFromFile(fset, newFile.Name(), true) + features, err := extractFeatureInfoListFromFile(fset, newFile.Name()) if tc.expectErr { if err == nil { t.Fatal("expect err") @@ -1003,7 +654,6 @@ func writeContentToTmpFile(t *testing.T, tmpDir, fileName, fileContent string) * if err != nil { t.Fatal(err) } - fmt.Printf("sizhangDebug: Written tmp file %s\n", tmpfile.Name()) return tmpfile } @@ -1087,3 +737,160 @@ func TestParseFeatureSpec(t *testing.T) { }) } } +func TestVerifyFeatureRemoval(t *testing.T) { + tests := []struct { + name string + featureList []featureInfo + baseFeatureList []featureInfo + currentVersion *version.Version + thresholdVersion *version.Version + expectErr bool + expectedErrMsg string + }{ + { + name: "no features removed", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureB", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Beta"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureB", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Beta"}}}, + }, + currentVersion: version.MustParse("1.1"), + expectErr: false, + }, + { + name: "alpha feature removed", + featureList: []featureInfo{ + {Name: "FeatureB", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Beta"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureB", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Beta"}}}, + }, + currentVersion: version.MustParse("1.1"), + expectErr: false, + }, + { + name: "beta feature removed", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureB", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Beta"}}}, + }, + currentVersion: version.MustParse("1.1"), + expectErr: true, + expectedErrMsg: "feature FeatureB cannot be removed while in beta", + }, + { + name: "GA feature removed before allowed version", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureC", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "GA", LockToDefault: true}}}, + }, + currentVersion: version.MustParse("1.2"), + expectErr: true, + expectedErrMsg: "feature FeatureC cannot be removed until version 1.3 (required for emulation support)", + }, + { + name: "GA feature removed after allowed version", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureC", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "GA", LockToDefault: true}}}, + }, + currentVersion: version.MustParse("1.4"), + expectErr: false, + }, + { + name: "feature with no version specs", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureD", VersionedSpecs: []featureSpec{}}, + }, + currentVersion: version.MustParse("1.1"), + expectErr: true, + expectedErrMsg: "feature FeatureD has no version specs", + }, + { + name: "feature with invalid version", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureE", VersionedSpecs: []featureSpec{{Version: "invalid", PreRelease: "GA", LockToDefault: true}}}, + }, + currentVersion: version.MustParse("1.1"), + expectErr: true, + expectedErrMsg: "invalid version \"invalid\" for feature FeatureE", + }, + { + name: "GA feature not locked to default", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureC", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "GA"}}}, + }, + currentVersion: version.MustParse("1.4"), + expectErr: true, + expectedErrMsg: "feature FeatureC cannot be removed because it is in GA or Deprecated state and is not locked to default", + }, + { + name: "Deprecated feature not locked to default", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureD", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Deprecated"}}}, + }, + currentVersion: version.MustParse("1.4"), + expectErr: true, + expectedErrMsg: "feature FeatureD cannot be removed because it is in GA or Deprecated state and is not locked to default", + }, + { + name: "GA feature removed at threshold version", + featureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + }, + baseFeatureList: []featureInfo{ + {Name: "FeatureA", VersionedSpecs: []featureSpec{{Version: "1.0", PreRelease: "Alpha"}}}, + {Name: "FeatureC", VersionedSpecs: []featureSpec{{Version: "1.4", PreRelease: "GA", LockToDefault: true}}}, + }, + currentVersion: version.MustParse("1.5"), + thresholdVersion: version.MustParse("1.4"), + expectErr: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := verifyFeatureRemoval(tc.featureList, tc.baseFeatureList, tc.currentVersion, tc.thresholdVersion) + if tc.expectErr { + if err == nil { + t.Fatalf("expected error, got nil") + } + if !strings.Contains(err.Error(), tc.expectedErrMsg) { + t.Fatalf("expected error message to contain %q, got %q", tc.expectedErrMsg, err.Error()) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} diff --git a/test/featuregates_linter/cmd/root.go b/test/compatibility_lifecycle/cmd/root.go similarity index 84% rename from test/featuregates_linter/cmd/root.go rename to test/compatibility_lifecycle/cmd/root.go index 38ddd97dfa3..8156aea00ef 100644 --- a/test/featuregates_linter/cmd/root.go +++ b/test/compatibility_lifecycle/cmd/root.go @@ -23,9 +23,9 @@ import ( ) var rootCmd = &cobra.Command{ - Use: "static-analysis", - Short: "static-analysis", - Long: `static-analysis runs static analysis of go code.`, + Use: "compatibility-lifecycle", + Short: "compatibility-lifecycle", + Long: `compatibility-lifecycle runs verification for compatibility lifecycle.`, } func Execute() { diff --git a/test/featuregates_linter/cmd/util.go b/test/compatibility_lifecycle/cmd/util.go similarity index 100% rename from test/featuregates_linter/cmd/util.go rename to test/compatibility_lifecycle/cmd/util.go diff --git a/test/featuregates_linter/main.go b/test/compatibility_lifecycle/main.go similarity index 91% rename from test/featuregates_linter/main.go rename to test/compatibility_lifecycle/main.go index f6b79cbe303..1a967750d59 100644 --- a/test/featuregates_linter/main.go +++ b/test/compatibility_lifecycle/main.go @@ -16,7 +16,7 @@ limitations under the License. package main -import "k8s.io/kubernetes/test/featuregates_linter/cmd" +import "k8s.io/kubernetes/test/compatibility_lifecycle/cmd" func main() { cmd.Execute() diff --git a/test/featuregates_linter/test_data/OWNERS b/test/compatibility_lifecycle/reference/OWNERS similarity index 100% rename from test/featuregates_linter/test_data/OWNERS rename to test/compatibility_lifecycle/reference/OWNERS diff --git a/test/featuregates_linter/test_data/unversioned_feature_list.yaml b/test/compatibility_lifecycle/reference/unversioned_feature_list.yaml similarity index 100% rename from test/featuregates_linter/test_data/unversioned_feature_list.yaml rename to test/compatibility_lifecycle/reference/unversioned_feature_list.yaml diff --git a/test/featuregates_linter/test_data/versioned_feature_list.yaml b/test/compatibility_lifecycle/reference/versioned_feature_list.yaml similarity index 99% rename from test/featuregates_linter/test_data/versioned_feature_list.yaml rename to test/compatibility_lifecycle/reference/versioned_feature_list.yaml index 2a0ab439f1d..0c9e20cc924 100644 --- a/test/featuregates_linter/test_data/versioned_feature_list.yaml +++ b/test/compatibility_lifecycle/reference/versioned_feature_list.yaml @@ -1,3 +1,6 @@ +# This file is generated by compatibility_lifecycle tool. +# Do not edit manually. Run hack/update-featuregates.sh to regenerate. + - name: AllowDNSOnlyNodeCSR versionedSpecs: - default: true diff --git a/test/featuregates_linter/README.md b/test/featuregates_linter/README.md deleted file mode 100644 index 8e1af3b0ba6..00000000000 --- a/test/featuregates_linter/README.md +++ /dev/null @@ -1,8 +0,0 @@ -This directory contains static analysis scripts for verify functions. - -Currently, the following commands are implemented: -``` -go run test/featuregates_linter/main.go feature-gates verify-no-new-unversioned --new-features-file="${new_features_file}" --old-features-file="${old_features_file}" - -go run test/featuregates_linter/main.go feature-gates verify-alphabetic-order --features-file="${features_file}" -```