Skip to content

OCPBUGS-63617: Fix Prevent conflicting feature gate settings in catalogd deployment #147

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

Closed
jianzhangbjz wants to merge 1 commit into from
Closed

OCPBUGS-63617: Fix Prevent conflicting feature gate settings in catalogd deployment #147

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 internal/featuregates/mapper.go
View file
Open in desktop
Copy link
Contributor

@tmshort tmshort Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we do want to explicitly disable (regardless of Kubernetes "best practices"), as this allows us to have features GA'd upstream and not downstream. That was the whole point of #144.

We need to explicitly disable WebHookCertManager, otherwise, it takes precedence over WebHookOpenshiftServiceCA (although you did leave that disablement in).

Please revert mapper.go and mapper_test.go.

Original file line number Diff line number Diff line change
Expand Up @@ -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...)
},
Expand Down
6 changes: 6 additions & 0 deletions pkg/controller/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
47 changes: 45 additions & 2 deletions pkg/helmvalues/helmvalues.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package helmvalues

import (
"errors"
"fmt"
"os"
"slices"
"strings"
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
286 changes: 284 additions & 2 deletions pkg/helmvalues/helmvalues_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}{
Expand All @@ -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",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
})
}
}