Skip to content

OCPBUGS-71236: Align featureGate validations with openshift #6138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
copejon wants to merge 1 commit into
base: main
Choose a base branch
from
+103 −165
Open

OCPBUGS-71236: Align featureGate validations with openshift #6138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ GO_BUILD_FLAGS :=-tags 'include_gcs include_oss containers_image_openpgp gssapi

# Set variables for test-unit target
GO_TEST_FLAGS=$(GO_BUILD_FLAGS)
GO_TEST_ARGS=$(GO_LD_FLAGS)
GO_TEST_PACKAGES=./cmd/... ./pkg/...

# Enable CGO when building microshift binary for access to local libraries.
Expand Down
78 changes: 20 additions & 58 deletions pkg/admin/prerun/featuregate_lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"os"
"path/filepath"
"reflect"

"github.com/openshift/microshift/pkg/config"
"github.com/openshift/microshift/pkg/util"
Expand Down Expand Up @@ -45,58 +44,27 @@ func featureGateLockManagement(cfg *config.Config) error {
if err != nil {
return fmt.Errorf("failed to check if lock file exists: %w", err)
}

// Lock file exists - validate configuration
if lockExists {
return validateFeatureGateLockFile(cfg)
return runValidationsChecks(cfg)
}

// No lock file exists yet and custom feature gates are configured, so this is the first time configuring custom feature gates
if hasCustomFeatureGates(cfg.ApiServer.FeatureGates) {
klog.InfoS("Custom feature gates detected", "featureSet", cfg.ApiServer.FeatureGates.FeatureSet)
if cfg.ApiServer.FeatureGates.FeatureSet != "" {
return createFeatureGateLockFile(cfg)
}

// No lock file and no custom feature gates - normal operation
klog.InfoS("No custom feature gates configured - skipping lock file management")
return nil
}

// hasCustomFeatureGates checks if any custom feature gates are configured
func hasCustomFeatureGates(fg config.FeatureGates) bool {
// Empty feature set means no custom feature gates
if fg.FeatureSet == "" {
return false
}

// TechPreviewNoUpgrade and DevPreviewNoUpgrade are considered custom
if fg.FeatureSet == config.FeatureSetTechPreviewNoUpgrade ||
fg.FeatureSet == config.FeatureSetDevPreviewNoUpgrade {
return true
}

// CustomNoUpgrade requires actual enabled or disabled features
if fg.FeatureSet == config.FeatureSetCustomNoUpgrade {
return len(fg.CustomNoUpgrade.Enabled) > 0 || len(fg.CustomNoUpgrade.Disabled) > 0
}

return false
}

// createFeatureGateLockFile creates the lock file with current configuration
func createFeatureGateLockFile(cfg *config.Config) error {
klog.InfoS("Creating feature gate lock file - this cluster can no longer be upgraded",
"path", featureGateLockFilePath)

// Get current version from version file
currentVersion, err := getVersionOfData()
currentVersion, err := GetVersionOfExecutable()
if err != nil {
// If version file doesn't exist yet, get executable version
klog.InfoS("Version file does not exist yet, using executable version")
currentVersion, err = GetVersionOfExecutable()
if err != nil {
return fmt.Errorf("failed to get version: %w", err)
}
return fmt.Errorf("failed to get version: %w", err)
}

lockFile := featureGateLockFile{
Expand All @@ -116,9 +84,9 @@ func createFeatureGateLockFile(cfg *config.Config) error {
return nil
}

// validateFeatureGateLockFile validates that the current configuration matches the lock file
// and that no version upgrade has occurred
func validateFeatureGateLockFile(cfg *config.Config) error {
// runValidationsChecks validates the feature gate lock file and the current configuration
// It returns an error if the configuration is invalid or if an x or y stream version upgrade has occurred.
func runValidationsChecks(cfg *config.Config) error {
klog.InfoS("Validating feature gate lock file", "path", featureGateLockFilePath)

lockFile, err := readFeatureGateLockFile(featureGateLockFilePath)
Expand All @@ -127,22 +95,21 @@ func validateFeatureGateLockFile(cfg *config.Config) error {
}

// Check if feature gate configuration has changed
if err := compareFeatureGates(lockFile, cfg.ApiServer.FeatureGates); err != nil {
return fmt.Errorf("feature gate configuration has changed: %w\n\n"+
"Custom feature gates cannot be modified or reverted once applied.\n"+
if err := configValidationChecksPass(lockFile, cfg.ApiServer.FeatureGates); err != nil {
return fmt.Errorf("detected invalid changes in feature gate configuration: %w\n\n"+
"To restore MicroShift to a supported state, you must:\n"+
"1. Run: sudo microshift-cleanup-data --all\n"+
"2. Remove custom feature gates from /etc/microshift/config.yaml\n"+
"3. Restart MicroShift: sudo systemctl restart microshift", err)
}

// Check if version has changed (upgrade attempted)
currentVersion, err := getVersionOfData()
currentExecutableVersion, err := getVersionOfData()
if err != nil {
return fmt.Errorf("failed to get current version: %w", err)
}

if lockFile.Version != currentVersion {
if lockFile.Version != currentExecutableVersion {
return fmt.Errorf("version upgrade detected with custom feature gates: locked version %s, current version %s\n\n"+
"Upgrades are not supported when custom feature gates are configured.\n"+
"Custom feature gates (%s) were configured in version %s.\n"+
Expand All @@ -151,29 +118,24 @@ func validateFeatureGateLockFile(cfg *config.Config) error {
"2. Run: sudo microshift-cleanup-data --all\n"+
"3. Remove custom feature gates from /etc/microshift/config.yaml\n"+
"4. Restart MicroShift: sudo systemctl restart microshift",
lockFile.Version.String(), currentVersion.String(),
lockFile.Version.String(), currentExecutableVersion.String(),
lockFile.FeatureSet, lockFile.Version.String(), lockFile.Version.String())
}

klog.InfoS("Feature gate lock file validation successful")
return nil
}

// compareFeatureGates compares the lock file with current configuration
func compareFeatureGates(lockFile featureGateLockFile, current config.FeatureGates) error {
var errs []error

if lockFile.FeatureSet != current.FeatureSet {
errs = append(errs, fmt.Errorf("feature set changed: locked config has %q, current config has %q",
lockFile.FeatureSet, current.FeatureSet))
func configValidationChecksPass(prev featureGateLockFile, current config.FeatureGates) error {
if prev.FeatureSet != "" && current.FeatureSet == "" {
// Disallow changing from feature set to no feature set
return fmt.Errorf("cannot unset feature set. Previous config had feature set %q, current config has no feature set configured", prev.FeatureSet)
}

if !reflect.DeepEqual(lockFile.CustomNoUpgrade, current.CustomNoUpgrade) {
errs = append(errs, fmt.Errorf("custom feature gates changed: locked config has %#v, current config has %#v",
lockFile.CustomNoUpgrade, current.CustomNoUpgrade))
if prev.FeatureSet == config.FeatureSetCustomNoUpgrade && current.FeatureSet != config.FeatureSetCustomNoUpgrade {
// Disallow changing from custom feature gates to any other feature set
return fmt.Errorf("cannot change CustomNoUpgrade feature set. Previous feature set was %q, current feature set is %q", prev.FeatureSet, current.FeatureSet)
}

return errors.Join(errs...)
return nil
}

// writeFeatureGateLockFile writes the lock file to disk in YAML format
Expand Down
92 changes: 38 additions & 54 deletions pkg/admin/prerun/featuregate_lock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,15 @@ func TestIsCustomFeatureGatesConfigured(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := hasCustomFeatureGates(tt.fg)
if got != tt.want {
t.Errorf("isCustomFeatureGatesConfigured() = %v, want %v", got, tt.want)
err := configValidationChecksPass(featureGateLockFile{
FeatureSet: tt.fg.FeatureSet,
CustomNoUpgrade: tt.fg.CustomNoUpgrade,
}, tt.fg)
if err != nil {
t.Errorf("featureValidationsPass() error = %v", err)
}
if err != nil {
t.Errorf("featureValidationsPass() error = %v", err)
}
})
}
Expand Down Expand Up @@ -206,75 +212,40 @@ func TestFeatureGateLockFile_ReadNonExistent(t *testing.T) {
}
}

func TestCompareFeatureGates(t *testing.T) {
func TestConfigValidationChecksPass(t *testing.T) {
tests := []struct {
name string
lockFile featureGateLockFile
current config.FeatureGates
wantMatch bool
name string
lockFile featureGateLockFile
current config.FeatureGates
wantErr bool
}{
{
name: "identical custom feature gates",
name: "unset any feature set",
lockFile: featureGateLockFile{
FeatureSet: config.FeatureSetCustomNoUpgrade,
CustomNoUpgrade: config.CustomNoUpgrade{
Enabled: []string{"FeatureA", "FeatureB"},
Disabled: []string{"FeatureC"},
},
},
current: config.FeatureGates{
FeatureSet: config.FeatureSetCustomNoUpgrade,
CustomNoUpgrade: config.CustomNoUpgrade{
Enabled: []string{"FeatureA", "FeatureB"},
Disabled: []string{"FeatureC"},
},
FeatureSet: "",
},
wantMatch: true,
wantErr: true,
},
{
name: "different enabled features",
name: "change CustomNoUpgrade to any other feature set",
lockFile: featureGateLockFile{
FeatureSet: config.FeatureSetCustomNoUpgrade,
CustomNoUpgrade: config.CustomNoUpgrade{
Enabled: []string{"FeatureA"},
},
},
current: config.FeatureGates{
FeatureSet: config.FeatureSetCustomNoUpgrade,
CustomNoUpgrade: config.CustomNoUpgrade{
Enabled: []string{"FeatureB"},
},
},
wantMatch: false,
},
{
name: "different feature sets",
lockFile: featureGateLockFile{
FeatureSet: config.FeatureSetTechPreviewNoUpgrade,
},
current: config.FeatureGates{
FeatureSet: config.FeatureSetDevPreviewNoUpgrade,
},
wantMatch: false,
},
{
name: "identical TechPreviewNoUpgrade",
lockFile: featureGateLockFile{
FeatureSet: config.FeatureSetTechPreviewNoUpgrade,
},
current: config.FeatureGates{
FeatureSet: config.FeatureSetTechPreviewNoUpgrade,
},
wantMatch: true,
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := compareFeatureGates(tt.lockFile, tt.current)
gotMatch := err == nil
if gotMatch != tt.wantMatch {
t.Errorf("compareFeatureGates() match = %v, want %v, error = %v", gotMatch, tt.wantMatch, err)
err := configValidationChecksPass(tt.lockFile, tt.current)
if (err != nil) != tt.wantErr {
t.Errorf("configValidationChecksPass() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
Expand All @@ -299,8 +270,13 @@ func TestFeatureGateLockManagement_FirstRun(t *testing.T) {
defer func() { versionFilePath = originalVersionPath }()

// Create a version file to simulate existing data (version file uses JSON format)
version, err := GetVersionOfExecutable()
if err != nil {
t.Fatal(err)
}
t.Logf("version: %s", version.String())
versionData := versionFile{
Version: versionMetadata{Major: 4, Minor: 18, Patch: 0},
Version: version,
BootID: "test-boot",
}
versionJSON, _ := json.Marshal(versionData)
Expand Down Expand Up @@ -354,8 +330,12 @@ func TestFeatureGateLockManagement_ConfigChange(t *testing.T) {
defer func() { versionFilePath = originalVersionPath }()

// Create a version file to simulate existing data (version file uses JSON format)
version, err := GetVersionOfExecutable()
if err != nil {
t.Fatal(err)
}
versionData := versionFile{
Version: versionMetadata{Major: 4, Minor: 18, Patch: 0},
Version: version,
BootID: "test-boot",
}
versionJSON, _ := json.Marshal(versionData)
Expand Down Expand Up @@ -411,8 +391,12 @@ func TestFeatureGateLockManagement_VersionChange(t *testing.T) {
defer func() { versionFilePath = originalVersionPath }()

// Create a version file with NEW version (simulating upgrade) (version file uses JSON format)
version, err := GetVersionOfExecutable()
if err != nil {
t.Fatal(err)
}
versionData := versionFile{
Version: versionMetadata{Major: 4, Minor: 19, Patch: 0}, // Newer version
Version: version, // TODO should be newer version
BootID: "test-boot",
}
versionJSON, _ := json.Marshal(versionData)
Expand Down
Loading