diff --git a/internal/featuregates/mapper.go b/internal/featuregates/mapper.go index 2b2cdb2e2..46ef18940 100644 --- a/internal/featuregates/mapper.go +++ b/internal/featuregates/mapper.go @@ -38,38 +38,46 @@ type Mapper struct { featureGates gateMapFunc } +func enableFeature(v *helmvalues.HelmValues, addList, removeList, feature string) error { + var errs []error + errs = append(errs, v.RemoveListValue(removeList, feature)) + errs = append(errs, v.AddListValue(addList, feature)) + return errors.Join(errs...) +} +func enableCatalogdFeature(v *helmvalues.HelmValues, enabled bool, feature string) error { + if enabled { + return enableFeature(v, helmvalues.EnableCatalogd, helmvalues.DisableCatalogd, feature) + } + return enableFeature(v, helmvalues.DisableCatalogd, helmvalues.EnableCatalogd, feature) +} + +func enableOperatorControllerFeature(v *helmvalues.HelmValues, enabled bool, feature string) error { + if enabled { + return enableFeature(v, helmvalues.EnableOperatorController, helmvalues.DisableOperatorController, feature) + } + return enableFeature(v, helmvalues.DisableOperatorController, helmvalues.EnableOperatorController, feature) +} + func NewMapper() *Mapper { // Add your downstream to upstream mapping here featureGates := gateMapFunc{ // features.FeatureGateNewOLMMyDownstreamFeature: functon that returns a list of enabled and disabled gates features.FeatureGateNewOLMPreflightPermissionChecks: func(v *helmvalues.HelmValues, enabled bool) error { - if enabled { - return v.AddListValue(helmvalues.EnableOperatorController, PreflightPermissions) - } - return v.AddListValue(helmvalues.DisableOperatorController, PreflightPermissions) + return enableOperatorControllerFeature(v, enabled, PreflightPermissions) }, features.FeatureGateNewOLMOwnSingleNamespace: func(v *helmvalues.HelmValues, enabled bool) error { - if enabled { - return v.AddListValue(helmvalues.EnableOperatorController, SingleOwnNamespaceInstallSupport) - } - return v.AddListValue(helmvalues.DisableOperatorController, SingleOwnNamespaceInstallSupport) + return enableOperatorControllerFeature(v, enabled, SingleOwnNamespaceInstallSupport) }, features.FeatureGateNewOLMWebhookProviderOpenshiftServiceCA: func(v *helmvalues.HelmValues, enabled bool) error { var errs []error - if enabled { - errs = append(errs, v.AddListValue(helmvalues.EnableOperatorController, WebhookProviderOpenshiftServiceCA)) - } else { - errs = append(errs, v.AddListValue(helmvalues.DisableOperatorController, WebhookProviderOpenshiftServiceCA)) - } - errs = append(errs, v.AddListValue(helmvalues.DisableOperatorController, WebhookProviderCertManager)) + errs = append(errs, enableOperatorControllerFeature(v, enabled, WebhookProviderOpenshiftServiceCA)) + // Always disable WebhookProviderCertManager + errs = append(errs, enableOperatorControllerFeature(v, false, WebhookProviderCertManager)) return errors.Join(errs...) }, features.FeatureGateNewOLMCatalogdAPIV1Metas: func(v *helmvalues.HelmValues, enabled bool) error { - if enabled { - return v.AddListValue(helmvalues.EnableCatalogd, APIV1MetasHandler) - } - return v.AddListValue(helmvalues.DisableCatalogd, APIV1MetasHandler) + return enableCatalogdFeature(v, enabled, APIV1MetasHandler) }, } diff --git a/internal/featuregates/mapper_test.go b/internal/featuregates/mapper_test.go index e0cc7b1f5..ed1588bf1 100644 --- a/internal/featuregates/mapper_test.go +++ b/internal/featuregates/mapper_test.go @@ -298,3 +298,317 @@ func TestMapper_Constants(t *testing.T) { t.Error("APIV1MetasHandler and PreflightPermissions should be different") } } + +func TestEnableFeature(t *testing.T) { + tests := []struct { + name string + addList string + removeList string + feature string + initialVals map[string]interface{} + expectedVals map[string]interface{} + }{ + { + name: "enable feature in empty values", + addList: helmvalues.EnableOperatorController, + removeList: helmvalues.DisableOperatorController, + feature: PreflightPermissions, + initialVals: make(map[string]interface{}), + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{PreflightPermissions}, + }, + }, + }, + }, + }, + { + name: "enable feature and remove from disabled list", + addList: helmvalues.EnableOperatorController, + removeList: helmvalues.DisableOperatorController, + feature: PreflightPermissions, + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{PreflightPermissions}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{}, + "enabled": []interface{}{PreflightPermissions}, + }, + }, + }, + }, + }, + { + name: "enable catalogd feature", + addList: helmvalues.EnableCatalogd, + removeList: helmvalues.DisableCatalogd, + feature: APIV1MetasHandler, + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{APIV1MetasHandler}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{}, + "enabled": []interface{}{APIV1MetasHandler}, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hv := helmvalues.NewHelmValues() + hv.GetValues() + for k, v := range tt.initialVals { + hv.GetValues()[k] = v + } + + err := enableFeature(hv, tt.addList, tt.removeList, tt.feature) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + actual := hv.GetValues() + if !reflect.DeepEqual(actual, tt.expectedVals) { + t.Errorf("Expected values %v, got %v", tt.expectedVals, actual) + } + }) + } +} + +func TestEnableOperatorControllerFeature(t *testing.T) { + tests := []struct { + name string + enabled bool + feature string + initialVals map[string]interface{} + expectedVals map[string]interface{} + }{ + { + name: "enable feature", + enabled: true, + feature: PreflightPermissions, + initialVals: make(map[string]interface{}), + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{PreflightPermissions}, + }, + }, + }, + }, + }, + { + name: "disable feature", + enabled: false, + feature: PreflightPermissions, + initialVals: make(map[string]interface{}), + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{PreflightPermissions}, + }, + }, + }, + }, + }, + { + name: "enable feature removes from disabled", + enabled: true, + feature: SingleOwnNamespaceInstallSupport, + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{SingleOwnNamespaceInstallSupport}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{}, + "enabled": []interface{}{SingleOwnNamespaceInstallSupport}, + }, + }, + }, + }, + }, + { + name: "disable feature removes from enabled", + enabled: false, + feature: SingleOwnNamespaceInstallSupport, + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{SingleOwnNamespaceInstallSupport}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "operatorController": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + "disabled": []interface{}{SingleOwnNamespaceInstallSupport}, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hv := helmvalues.NewHelmValues() + for k, v := range tt.initialVals { + hv.GetValues()[k] = v + } + + err := enableOperatorControllerFeature(hv, tt.enabled, tt.feature) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + actual := hv.GetValues() + if !reflect.DeepEqual(actual, tt.expectedVals) { + t.Errorf("Expected values %v, got %v", tt.expectedVals, actual) + } + }) + } +} + +func TestEnableCatalogdFeature(t *testing.T) { + tests := []struct { + name string + enabled bool + feature string + initialVals map[string]interface{} + expectedVals map[string]interface{} + }{ + { + name: "enable catalogd feature", + enabled: true, + feature: APIV1MetasHandler, + initialVals: make(map[string]interface{}), + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{APIV1MetasHandler}, + }, + }, + }, + }, + }, + { + name: "disable catalogd feature", + enabled: false, + feature: APIV1MetasHandler, + initialVals: make(map[string]interface{}), + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{APIV1MetasHandler}, + }, + }, + }, + }, + }, + { + name: "enable catalogd feature removes from disabled", + enabled: true, + feature: APIV1MetasHandler, + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{APIV1MetasHandler}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{}, + "enabled": []interface{}{APIV1MetasHandler}, + }, + }, + }, + }, + }, + { + name: "disable catalogd feature removes from enabled", + enabled: false, + feature: APIV1MetasHandler, + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{APIV1MetasHandler}, + }, + }, + }, + }, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "enabled": []interface{}{}, + "disabled": []interface{}{APIV1MetasHandler}, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hv := helmvalues.NewHelmValues() + for k, v := range tt.initialVals { + hv.GetValues()[k] = v + } + + err := enableCatalogdFeature(hv, tt.enabled, tt.feature) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + actual := hv.GetValues() + if !reflect.DeepEqual(actual, tt.expectedVals) { + t.Errorf("Expected values %v, got %v", tt.expectedVals, actual) + } + }) + } +} diff --git a/pkg/controller/featuregates_hook.go b/pkg/controller/featuregates_hook.go index 23f3b02da..bd965673c 100644 --- a/pkg/controller/featuregates_hook.go +++ b/pkg/controller/featuregates_hook.go @@ -11,12 +11,12 @@ import ( // upstreamFeatureGates builds a set of helm values for the downsteeam feature-gates that are // mapped to upstream feature-gates func upstreamFeatureGates( + values *helmvalues.HelmValues, clusterGatesConfig featuregates.FeatureGate, downstreamGates []configv1.FeatureGateName, downstreamToUpstreamFunc func(configv1.FeatureGateName) func(*helmvalues.HelmValues, bool) error, ) (*helmvalues.HelmValues, error) { errs := make([]error, 0, len(downstreamGates)) - values := helmvalues.NewHelmValues() for _, downstreamGate := range downstreamGates { f := downstreamToUpstreamFunc(downstreamGate) diff --git a/pkg/controller/featuregates_hook_test.go b/pkg/controller/featuregates_hook_test.go index 703afe342..6b7749685 100644 --- a/pkg/controller/featuregates_hook_test.go +++ b/pkg/controller/featuregates_hook_test.go @@ -276,6 +276,7 @@ func TestUpstreamFeatureGates(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result, err := upstreamFeatureGates( + helmvalues.NewHelmValues(), tt.clusterGatesConfig, tt.downstreamGates, tt.downstreamToUpstreamFunc, @@ -319,7 +320,7 @@ func TestUpstreamFeatureGates_NilMappingFunction(t *testing.T) { return nil // Return nil function } - _, _ = upstreamFeatureGates(clusterGatesConfig, downstreamGates, downstreamToUpstreamFunc) + _, _ = upstreamFeatureGates(helmvalues.NewHelmValues(), clusterGatesConfig, downstreamGates, downstreamToUpstreamFunc) } func TestUpstreamFeatureGates_EdgeCases(t *testing.T) { @@ -332,6 +333,7 @@ func TestUpstreamFeatureGates_EdgeCases(t *testing.T) { downstreamGates := []configv1.FeatureGateName{features.FeatureGateNewOLMPreflightPermissionChecks} result, err := upstreamFeatureGates( + helmvalues.NewHelmValues(), clusterGatesConfig, downstreamGates, func(_ configv1.FeatureGateName) func(*helmvalues.HelmValues, bool) error { diff --git a/pkg/controller/helm.go b/pkg/controller/helm.go index 0b69099af..0ef4c30fb 100644 --- a/pkg/controller/helm.go +++ b/pkg/controller/helm.go @@ -34,7 +34,11 @@ func (b *Builder) renderHelmTemplate(helmPath, manifestDir string) error { return fmt.Errorf("CurrentFeatureGates failed: %w", err) } - featureGateValues, err := upstreamFeatureGates(clusterGatesConfig, + // Gather feature-gate configuration first, to see if we want to use + // the experimental values file. + featureGateValues, err := upstreamFeatureGates( + helmvalues.NewHelmValues(), + clusterGatesConfig, b.Clients.FeatureGateMapper.DownstreamFeatureGates(), b.Clients.FeatureGateMapper.UpstreamForDownstream) if err != nil { @@ -61,10 +65,18 @@ func (b *Builder) renderHelmTemplate(helmPath, manifestDir string) error { return fmt.Errorf("error from GatherHelmValuesFromFiles: %w", err) } - // Add the featureGateValues - if err := values.AddValues(featureGateValues); err != nil { - return fmt.Errorf("error from AddValues: %w", err) + // Add the upstreamFeatureGates - this is run a second time to update the existing + // list rather than create a new list. Using AddValues() to merge featureGateValues + // from above would not allow overriding of enabled/disabled features. + values, err = upstreamFeatureGates( + values, + clusterGatesConfig, + b.Clients.FeatureGateMapper.DownstreamFeatureGates(), + b.Clients.FeatureGateMapper.UpstreamForDownstream) + if err != nil { + return err } + // Log verbosity and proxies are dynamic, so they are not included here. // Feature flags are listed here, and if they change cluster-olm-operator // will resart, as the manifest needs to be regenerated diff --git a/pkg/helmvalues/helmvalues.go b/pkg/helmvalues/helmvalues.go index 523f6c214..88e1bd624 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,29 @@ 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{}) } diff --git a/pkg/helmvalues/helmvalues_test.go b/pkg/helmvalues/helmvalues_test.go index 0e2a2fbbf..4cd410166 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,155 @@ 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"}, + }, + }, + }, + }, + { + name: "remove from catalogd features", + initialVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{"APIV1MetasHandler", "OtherFeature"}, + }, + }, + }, + }, + location: "options.catalogd.features.disabled", + value: "APIV1MetasHandler", + expectError: false, + expectedVals: map[string]interface{}{ + "options": map[string]interface{}{ + "catalogd": map[string]interface{}{ + "features": map[string]interface{}{ + "disabled": []interface{}{"OtherFeature"}, + }, + }, + }, + }, + }, + } + + 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