diff --git a/internal/operator-controller/applier/provider.go b/internal/operator-controller/applier/provider.go index 49e5a8df0..e82d17ba4 100644 --- a/internal/operator-controller/applier/provider.go +++ b/internal/operator-controller/applier/provider.go @@ -112,13 +112,13 @@ func (r *RegistryV1ManifestProvider) extractBundleConfigOptions(rv1 *bundle.Regi opts = append(opts, render.WithTargetNamespaces(*watchNS)) } - // Extract and convert deploymentConfig if present and the feature gate is enabled. + // Extract deploymentConfig if present and the feature gate is enabled. if r.IsDeploymentConfigEnabled { - if deploymentConfigMap := bundleConfig.GetDeploymentConfig(); deploymentConfigMap != nil { - deploymentConfig, err := convertToDeploymentConfig(deploymentConfigMap) - if err != nil { - return nil, errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("invalid deploymentConfig: %w", err)) - } + deploymentConfig, err := bundleConfig.GetDeploymentConfig() + if err != nil { + return nil, errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("invalid deploymentConfig: %w", err)) + } + if deploymentConfig != nil { opts = append(opts, render.WithDeploymentConfig(deploymentConfig)) } } @@ -187,29 +187,6 @@ func extensionConfigBytes(ext *ocv1.ClusterExtension) []byte { return nil } -// convertToDeploymentConfig converts a map[string]any (from validated bundle config) -// to a *config.DeploymentConfig struct that can be passed to the renderer. -// Returns nil if the map is empty. -func convertToDeploymentConfig(deploymentConfigMap map[string]any) (*config.DeploymentConfig, error) { - if len(deploymentConfigMap) == 0 { - return nil, nil - } - - // Marshal the map to JSON - data, err := json.Marshal(deploymentConfigMap) - if err != nil { - return nil, fmt.Errorf("failed to marshal deploymentConfig: %w", err) - } - - // Unmarshal into the DeploymentConfig struct - var deploymentConfig config.DeploymentConfig - if err := json.Unmarshal(data, &deploymentConfig); err != nil { - return nil, fmt.Errorf("failed to unmarshal deploymentConfig: %w", err) - } - - return &deploymentConfig, nil -} - func getBundleAnnotations(bundleFS fs.FS) (map[string]string, error) { // The need to get the underlying bundle in order to extract its annotations // will go away once we have a bundle interface that can surface the annotations independently of the diff --git a/internal/operator-controller/applier/provider_test.go b/internal/operator-controller/applier/provider_test.go index 71b1ab3bd..5f6913f43 100644 --- a/internal/operator-controller/applier/provider_test.go +++ b/internal/operator-controller/applier/provider_test.go @@ -17,6 +17,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/applier" + "github.com/operator-framework/operator-controller/internal/operator-controller/config" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1" @@ -599,8 +600,8 @@ func Test_RegistryV1ManifestProvider_DeploymentConfig(t *testing.T) { BundleRenderer: render.BundleRenderer{ ResourceGenerators: []render.ResourceGenerator{ func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) { - t.Log("ensure deploymentConfig is nil for empty config object") - require.Nil(t, opts.DeploymentConfig) + t.Log("ensure deploymentConfig is empty for empty config object") + require.Equal(t, &config.DeploymentConfig{}, opts.DeploymentConfig) return nil, nil }, }, diff --git a/internal/operator-controller/config/config.go b/internal/operator-controller/config/config.go index afb89dff5..a814b5025 100644 --- a/internal/operator-controller/config/config.go +++ b/internal/operator-controller/config/config.go @@ -106,44 +106,29 @@ func (c *Config) GetWatchNamespace() *string { } // GetDeploymentConfig returns the deploymentConfig value if present in the configuration. -// Returns nil if deploymentConfig is not set or is explicitly set to null. -// The returned value is a generic map[string]any that can be marshaled to JSON -// for validation or conversion to specific types (like v1alpha1.SubscriptionConfig). -// -// Returns a defensive deep copy so callers can't mutate the internal Config state. -func (c *Config) GetDeploymentConfig() map[string]any { +// Returns (nil, nil) if deploymentConfig is not set or is explicitly set to null. +// Returns a non-nil error if the value cannot be marshaled or unmarshaled into a DeploymentConfig. +func (c *Config) GetDeploymentConfig() (*DeploymentConfig, error) { if c == nil || *c == nil { - return nil + return nil, nil } val, exists := (*c)["deploymentConfig"] if !exists { - return nil + return nil, nil } // User set deploymentConfig: null - treat as "not configured" if val == nil { - return nil - } - // Schema validation ensures this is an object (map) - dcMap, ok := val.(map[string]any) - if !ok { - return nil + return nil, nil } - - // Return a defensive deep copy so callers can't mutate the internal Config state. - // We use JSON marshal/unmarshal because the data is already JSON-compatible and - // this handles nested structures correctly. - data, err := json.Marshal(dcMap) + data, err := json.Marshal(val) if err != nil { - // This should never happen since the map came from validated JSON/YAML, - // but return nil as a safe fallback - return nil + return nil, fmt.Errorf("failed to marshal deploymentConfig: %w", err) } - var copied map[string]any - if err := json.Unmarshal(data, &copied); err != nil { - // This should never happen for valid JSON - return nil + var dc DeploymentConfig + if err := json.Unmarshal(data, &dc); err != nil { + return nil, fmt.Errorf("failed to unmarshal deploymentConfig: %w", err) } - return copied + return &dc, nil } // UnmarshalConfig takes user configuration, validates it, and creates a Config object. diff --git a/internal/operator-controller/config/config_test.go b/internal/operator-controller/config/config_test.go index 1e451cf5c..e7429eeba 100644 --- a/internal/operator-controller/config/config_test.go +++ b/internal/operator-controller/config/config_test.go @@ -5,6 +5,8 @@ import ( "testing" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/utils/ptr" "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -596,7 +598,7 @@ func Test_GetDeploymentConfig(t *testing.T) { tests := []struct { name string rawConfig []byte - expectedDeploymentConfig map[string]any + expectedDeploymentConfig *config.DeploymentConfig expectedDeploymentConfigNil bool }{ { @@ -628,13 +630,13 @@ func Test_GetDeploymentConfig(t *testing.T) { } } }`), - expectedDeploymentConfig: map[string]any{ - "nodeSelector": map[string]any{ + expectedDeploymentConfig: &config.DeploymentConfig{ + NodeSelector: map[string]string{ "kubernetes.io/os": "linux", }, - "resources": map[string]any{ - "requests": map[string]any{ - "memory": "128Mi", + Resources: &corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("128Mi"), }, }, }, @@ -650,7 +652,8 @@ func Test_GetDeploymentConfig(t *testing.T) { cfg, err := config.UnmarshalConfig(tt.rawConfig, schema, "") require.NoError(t, err) - result := cfg.GetDeploymentConfig() + result, err := cfg.GetDeploymentConfig() + require.NoError(t, err) if tt.expectedDeploymentConfigNil { require.Nil(t, result) } else { @@ -663,12 +666,13 @@ func Test_GetDeploymentConfig(t *testing.T) { // Test nil config separately t.Run("nil config returns nil", func(t *testing.T) { var cfg *config.Config - result := cfg.GetDeploymentConfig() + result, err := cfg.GetDeploymentConfig() + require.NoError(t, err) require.Nil(t, result) }) - // Test that returned map is a defensive copy (mutations don't affect original) - t.Run("returned map is defensive copy - mutations don't affect original", func(t *testing.T) { + // Test that returned struct is a separate instance (mutations don't affect original) + t.Run("returned struct is independent copy - mutations don't affect original", func(t *testing.T) { rawConfig := []byte(`{ "deploymentConfig": { "nodeSelector": { @@ -684,31 +688,29 @@ func Test_GetDeploymentConfig(t *testing.T) { require.NoError(t, err) // Get the deploymentConfig - result1 := cfg.GetDeploymentConfig() + result1, err := cfg.GetDeploymentConfig() + require.NoError(t, err) require.NotNil(t, result1) - // Mutate the returned map - result1["nodeSelector"] = map[string]any{ - "mutated": "value", - } - result1["newField"] = "added" + // Mutate the returned struct + result1.NodeSelector["mutated"] = "value" // Get deploymentConfig again - should be unaffected by mutations - result2 := cfg.GetDeploymentConfig() + result2, err := cfg.GetDeploymentConfig() + require.NoError(t, err) require.NotNil(t, result2) // Original values should be intact - require.Equal(t, map[string]any{ - "nodeSelector": map[string]any{ - "kubernetes.io/os": "linux", - }, - }, result2) - - // New field should not exist - _, exists := result2["newField"] - require.False(t, exists) + require.Equal(t, map[string]string{ + "kubernetes.io/os": "linux", + }, result2.NodeSelector) + }) - // result1 should have the mutations - require.Equal(t, "added", result1["newField"]) + // Test that invalid deploymentConfig type returns an error + t.Run("invalid deploymentConfig type returns error", func(t *testing.T) { + cfg := config.Config{"deploymentConfig": "not-an-object"} + result, err := cfg.GetDeploymentConfig() + require.Error(t, err) + require.Nil(t, result) }) }