diff --git a/internal/featuregates/mapper.go b/internal/featuregates/mapper.go index 2b2cdb2e2..26d88705c 100644 --- a/internal/featuregates/mapper.go +++ b/internal/featuregates/mapper.go @@ -62,6 +62,7 @@ func NewMapper() *Mapper { } else { errs = append(errs, v.AddListValue(helmvalues.DisableOperatorController, WebhookProviderOpenshiftServiceCA)) } + // Always disable WebhookProviderCertManager errs = append(errs, v.AddListValue(helmvalues.DisableOperatorController, WebhookProviderCertManager)) return errors.Join(errs...) }, diff --git a/pkg/controller/helm.go b/pkg/controller/helm.go index 0b69099af..6aaa8a3d8 100644 --- a/pkg/controller/helm.go +++ b/pkg/controller/helm.go @@ -61,6 +61,12 @@ func (b *Builder) renderHelmTemplate(helmPath, manifestDir string) error { return fmt.Errorf("error from GatherHelmValuesFromFiles: %w", err) } + // Clear any feature gate settings from helm values files + // This ensures cluster feature gate configuration takes precedence + if err := values.ClearFeatureGates(); err != nil { + return fmt.Errorf("error from ClearFeatureGates: %w", err) + } + // Add the featureGateValues if err := values.AddValues(featureGateValues); err != nil { return fmt.Errorf("error from AddValues: %w", err) diff --git a/pkg/helmvalues/helmvalues.go b/pkg/helmvalues/helmvalues.go index 523f6c214..9e47f6f4d 100644 --- a/pkg/helmvalues/helmvalues.go +++ b/pkg/helmvalues/helmvalues.go @@ -2,7 +2,6 @@ package helmvalues import ( "errors" - "fmt" "os" "slices" "strings" @@ -94,7 +93,8 @@ func (v *HelmValues) AddListValue(location string, newValue string) error { } if found { if slices.Index(values, newValue) != -1 { - return fmt.Errorf("newValue=%q already present in values=%v", newValue, values) + // if newValue is already there, then it's been "added" + return nil } values = append(values, newValue) slices.Sort(values) @@ -103,6 +103,49 @@ func (v *HelmValues) AddListValue(location string, newValue string) error { return unstructured.SetNestedStringSlice(v.values, []string{newValue}, ss...) } +func (v *HelmValues) RemoveListValue(location string, rmValue string) error { + if location == "" { + return errors.New("location string has no locations") + } + ss := strings.Split(location, ".") + values, found, err := unstructured.NestedStringSlice(v.values, ss...) + if err != nil { + return err + } + if !found { + // slice doesn't exist, so rmValue value doesn't exist + return nil + } + idx := slices.Index(values, rmValue) + if idx == -1 { + // if rmValue is not already there, then it's been "removed" + return nil + } + + values = append(values[:idx], values[idx+1:]...) + return unstructured.SetNestedStringSlice(v.values, values, ss...) +} + func (v *HelmValues) AddValues(newValues *HelmValues) error { return deepmerge.DeepMerge(v.values, newValues.values, deepmerge.Config{}) } + +// ClearFeatureGates removes all feature gate settings from the helm values +// This is used to ensure cluster feature gate configuration takes precedence +// over any feature gates defined in helm values files +func (v *HelmValues) ClearFeatureGates() error { + locationsToClear := []string{ + EnableOperatorController, + DisableOperatorController, + EnableCatalogd, + DisableCatalogd, + } + for _, location := range locationsToClear { + if _, found, _ := unstructured.NestedStringSlice(v.values, strings.Split(location, ".")...); found { + if err := unstructured.SetNestedStringSlice(v.values, []string{}, strings.Split(location, ".")...); err != nil { + return err + } + } + } + return nil +} diff --git a/pkg/helmvalues/helmvalues_test.go b/pkg/helmvalues/helmvalues_test.go index 0e2a2fbbf..4f3986515 100644 --- a/pkg/helmvalues/helmvalues_test.go +++ b/pkg/helmvalues/helmvalues_test.go @@ -365,7 +365,7 @@ func TestAddListValue(t *testing.T) { }, }, { - name: "add duplicate value", + name: "add duplicate value - idempotent", initialVals: map[string]interface{}{ "options": map[string]interface{}{ "features": map[string]interface{}{ @@ -375,7 +375,14 @@ func TestAddListValue(t *testing.T) { }, location: "options.features.enabled", value: "feature1", - expectError: true, + expectError: false, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1"}, + }, + }, + }, }, { name: "add with sorting", @@ -419,6 +426,131 @@ func TestAddListValue(t *testing.T) { } } +func TestRemoveListValue(t *testing.T) { + tests := []struct { + name string + initialVals map[string]interface{} + location string + value string + expectError bool + expectedVals map[string]interface{} + }{ + { + name: "empty location", + location: "", + value: "test", + expectError: true, + }, + { + name: "remove from non-existent location - idempotent", + initialVals: make(map[string]interface{}), + location: "options.features.enabled", + value: "feature1", + expectError: false, + expectedVals: map[string]interface{}{}, + }, + { + name: "remove existing value", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1", "feature2"}, + }, + }, + }, + location: "options.features.enabled", + value: "feature1", + expectError: false, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature2"}, + }, + }, + }, + }, + { + name: "remove last value from list", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1"}, + }, + }, + }, + location: "options.features.enabled", + value: "feature1", + expectError: false, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + }, + }, + }, + }, + { + name: "remove non-existent value - idempotent", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1", "feature2"}, + }, + }, + }, + location: "options.features.enabled", + value: "feature3", + expectError: false, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1", "feature2"}, + }, + }, + }, + }, + { + name: "remove middle value from list", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1", "feature2", "feature3"}, + }, + }, + }, + location: "options.features.enabled", + value: "feature2", + expectError: false, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1", "feature3"}, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hv := NewHelmValues() + hv.values = tt.initialVals + + err := hv.RemoveListValue(tt.location, tt.value) + + if tt.expectError && err == nil { + t.Errorf("Expected error, got nil") + } + if !tt.expectError && err != nil { + t.Errorf("Unexpected error: %v", err) + } + if !tt.expectError && !reflect.DeepEqual(hv.values, tt.expectedVals) { + t.Errorf("Expected %v, got %v", tt.expectedVals, hv.values) + } + }) + } +} + func TestAddValues(t *testing.T) { tests := []struct { name string @@ -499,3 +631,153 @@ func TestAddValues(t *testing.T) { }) } } + +func TestClearFeatureGates(t *testing.T) { + tests := []struct { + name string + initialVals map[string]interface{} + expectedVals map[string]interface{} + }{ + { + name: "empty values", + initialVals: make(map[string]interface{}), + expectedVals: map[string]interface{}{}, + }, + { + name: "clear operator controller feature gates", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1"}, + "disabled": []interface{}{"feature2"}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + "disabled": []interface{}{}, + }, + }, + }, + }, + }, + { + name: "clear catalogd feature gates", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"APIV1MetasHandler"}, + "disabled": []interface{}{}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + "disabled": []interface{}{}, + }, + }, + }, + }, + }, + { + name: "clear both operator controller and catalogd feature gates", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1"}, + "disabled": []interface{}{"feature2"}, + }, + }, + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"APIV1MetasHandler"}, + "disabled": []interface{}{}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + "disabled": []interface{}{}, + }, + }, + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + "disabled": []interface{}{}, + }, + }, + }, + }, + }, + { + name: "preserve other values", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"feature1"}, + }, + "image": "test-image", + }, + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{"APIV1MetasHandler"}, + }, + "deployment": map[string]interface{}{ + "replicas": 1, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + }, + "image": "test-image", + }, + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + }, + "deployment": map[string]interface{}{ + "replicas": 1, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hv := NewHelmValues() + hv.values = tt.initialVals + + err := hv.ClearFeatureGates() + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + if !reflect.DeepEqual(hv.values, tt.expectedVals) { + t.Errorf("Expected %v, got %v", tt.expectedVals, hv.values) + } + }) + } +}