diff --git a/internal/controller/bootstrap/config_bridge.go b/internal/controller/bootstrap/config_bridge.go index 85484ca70..1149374d1 100644 --- a/internal/controller/bootstrap/config_bridge.go +++ b/internal/controller/bootstrap/config_bridge.go @@ -24,19 +24,16 @@ import ( // This allows bootstrap to use controller merge logic without import cycles type ConfigMergerFunc func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) -// configMerger holds the merger function set by the reconciler -var configMerger ConfigMergerFunc - -// SetConfigMerger sets the configuration merger function -// Called during reconciler initialization to inject the merge logic -func SetConfigMerger(merger ConfigMergerFunc) { - configMerger = merger +// SetConfigMerger sets the configuration merger function on the Bootstrap instance. +// Called during reconciler initialization to inject the merge logic. +func (b *Bootstrap) SetConfigMerger(merger ConfigMergerFunc) { + b.configMerger = merger } // mergeConfigs calls the injected merger function if available func (b *Bootstrap) mergeConfigs(baseConfig string, cs *apiv3.CommonService) (string, error) { - if configMerger != nil { - return configMerger(baseConfig, cs, b.CSData.ServicesNs) + if b.configMerger != nil { + return b.configMerger(baseConfig, cs, b.CSData.ServicesNs) } // If no merger is set, return base config unchanged return baseConfig, nil diff --git a/internal/controller/bootstrap/config_bridge_test.go b/internal/controller/bootstrap/config_bridge_test.go index 060c9334c..4a739b93e 100644 --- a/internal/controller/bootstrap/config_bridge_test.go +++ b/internal/controller/bootstrap/config_bridge_test.go @@ -26,20 +26,17 @@ import ( apiv3 "github.com/IBM/ibm-common-service-operator/v4/api/v3" ) -// resetConfigMerger clears the package-level configMerger between tests to -// prevent state leaking across test cases. -func resetConfigMerger() { - configMerger = nil +// newTestBootstrap creates a Bootstrap instance for testing with the given ServicesNs. +func newTestBootstrap(servicesNs string) *Bootstrap { + return &Bootstrap{ + CSData: apiv3.CSData{ServicesNs: servicesNs}, + } } // TestSetConfigMerger_NilByDefault verifies that before SetConfigMerger is called -// the package-level merger is nil and mergeConfigs returns the base config unchanged. +// the merger is nil and mergeConfigs returns the base config unchanged. func TestSetConfigMerger_NilByDefault(t *testing.T) { - resetConfigMerger() - - b := &Bootstrap{ - CSData: apiv3.CSData{ServicesNs: "ibm-common-services"}, - } + b := newTestBootstrap("ibm-common-services") cs := &apiv3.CommonService{} result, err := b.mergeConfigs("base-config", cs) @@ -51,18 +48,13 @@ func TestSetConfigMerger_NilByDefault(t *testing.T) { // TestSetConfigMerger_InjectsFunction verifies that SetConfigMerger stores the // provided function and that mergeConfigs delegates to it. func TestSetConfigMerger_InjectsFunction(t *testing.T) { - resetConfigMerger() - defer resetConfigMerger() - called := false - SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { + b := newTestBootstrap("ibm-common-services") + b.SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { called = true return "merged-" + baseConfig, nil }) - b := &Bootstrap{ - CSData: apiv3.CSData{ServicesNs: "ibm-common-services"}, - } cs := &apiv3.CommonService{} result, err := b.mergeConfigs("base", cs) @@ -74,18 +66,13 @@ func TestSetConfigMerger_InjectsFunction(t *testing.T) { // TestSetConfigMerger_PassesServicesNs verifies that mergeConfigs forwards the // Bootstrap's ServicesNs to the injected merger function. func TestSetConfigMerger_PassesServicesNs(t *testing.T) { - resetConfigMerger() - defer resetConfigMerger() - var capturedNs string - SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { + b := newTestBootstrap("my-services-ns") + b.SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { capturedNs = servicesNs return baseConfig, nil }) - b := &Bootstrap{ - CSData: apiv3.CSData{ServicesNs: "my-services-ns"}, - } cs := &apiv3.CommonService{} _, err := b.mergeConfigs("config", cs) @@ -97,18 +84,13 @@ func TestSetConfigMerger_PassesServicesNs(t *testing.T) { // TestSetConfigMerger_PassesCSInstance verifies that mergeConfigs forwards the // CommonService instance to the injected merger function. func TestSetConfigMerger_PassesCSInstance(t *testing.T) { - resetConfigMerger() - defer resetConfigMerger() - var capturedCS *apiv3.CommonService - SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { + b := newTestBootstrap("ibm-common-services") + b.SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { capturedCS = cs return baseConfig, nil }) - b := &Bootstrap{ - CSData: apiv3.CSData{ServicesNs: "ibm-common-services"}, - } cs := &apiv3.CommonService{} cs.Name = "common-service" cs.Namespace = "ibm-common-services" @@ -123,17 +105,12 @@ func TestSetConfigMerger_PassesCSInstance(t *testing.T) { // TestSetConfigMerger_PropagatesError verifies that an error returned by the // injected merger function is propagated back to the caller of mergeConfigs. func TestSetConfigMerger_PropagatesError(t *testing.T) { - resetConfigMerger() - defer resetConfigMerger() - mergeErr := errors.New("merge failed") - SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { + b := newTestBootstrap("ibm-common-services") + b.SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { return "", mergeErr }) - b := &Bootstrap{ - CSData: apiv3.CSData{ServicesNs: "ibm-common-services"}, - } cs := &apiv3.CommonService{} result, err := b.mergeConfigs("config", cs) @@ -145,19 +122,14 @@ func TestSetConfigMerger_PropagatesError(t *testing.T) { // TestSetConfigMerger_Overwrite verifies that calling SetConfigMerger a second // time replaces the previously registered function. func TestSetConfigMerger_Overwrite(t *testing.T) { - resetConfigMerger() - defer resetConfigMerger() - - SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { + b := newTestBootstrap("ibm-common-services") + b.SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { return "first", nil }) - SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { + b.SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { return "second", nil }) - b := &Bootstrap{ - CSData: apiv3.CSData{ServicesNs: "ibm-common-services"}, - } cs := &apiv3.CommonService{} result, err := b.mergeConfigs("config", cs) @@ -171,21 +143,16 @@ func TestSetConfigMerger_Overwrite(t *testing.T) { // produces a complete merged result — there is no intermediate state where the // base config is returned before CS values are applied. func TestMergeConfigs_SingleStageNoRaceCondition(t *testing.T) { - resetConfigMerger() - defer resetConfigMerger() - // Simulate the real merger: it always returns a "complete" config that // includes both base and CS values in one shot. - SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { + b := newTestBootstrap("ibm-common-services") + b.SetConfigMerger(func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { // In production this would be MergeConfigs(); here we just verify the // contract: the result must differ from the bare base config, proving // CS values were applied in the same call. return baseConfig + "+cs-values", nil }) - b := &Bootstrap{ - CSData: apiv3.CSData{ServicesNs: "ibm-common-services"}, - } cs := &apiv3.CommonService{} result, err := b.mergeConfigs("base-opcon", cs) diff --git a/internal/controller/bootstrap/init.go b/internal/controller/bootstrap/init.go index a3dccdc29..b08146811 100644 --- a/internal/controller/bootstrap/init.go +++ b/internal/controller/bootstrap/init.go @@ -82,6 +82,7 @@ type Bootstrap struct { MultiInstancesEnable bool CSOperators []CSOperator CSData apiv3.CSData + configMerger ConfigMergerFunc } // CanI performs a SelfSubjectAccessReview (SSAR) to check whether the operator service account diff --git a/internal/controller/commonservice_controller.go b/internal/controller/commonservice_controller.go index 30c0cf94f..75dd546cd 100644 --- a/internal/controller/commonservice_controller.go +++ b/internal/controller/commonservice_controller.go @@ -248,10 +248,6 @@ func (r *CommonServiceReconciler) ReconcileMasterCR(ctx context.Context, instanc return ctrl.Result{}, statusErr } - // OperandConfig already created with complete configuration - // in InitResources, no need for second updateOperandConfig call - klog.Info("OperandConfig created with complete configuration") - var isEqual bool if isEqual, statusErr = r.updateOperatorConfig(ctx, instance.Spec.OperatorConfigs); statusErr != nil { if statusErr := r.updatePhase(ctx, instance, apiv3.CRFailed); statusErr != nil { @@ -529,7 +525,7 @@ func isNonNoopOperandReconcile(operandRegistry *odlm.OperandRegistry) bool { func (r *CommonServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { // Set up configuration merger for single-stage OperandConfig creation // This injects the merge logic into bootstrap without creating import cycles - bootstrap.SetConfigMerger(CreateMergerFunc(r)) + r.Bootstrap.SetConfigMerger(CreateMergerFunc(r)) klog.Info("Configuration merger initialized for single-stage OperandConfig creation") controller := ctrl.NewControllerManagedBy(mgr). diff --git a/internal/controller/config_extractor.go b/internal/controller/config_extractor.go index 4015f191d..323a0be14 100644 --- a/internal/controller/config_extractor.go +++ b/internal/controller/config_extractor.go @@ -221,48 +221,58 @@ func extractSizeConfigs( return sizeConfigs, serviceControllerMapping, nil } -// extractCustomSizeConfigs extracts custom size configurations from services field -func extractCustomSizeConfigs(cs *apiv3.CommonService, serviceControllerMapping map[string]string) ([]interface{}, map[string]string) { - var dest []interface{} - - if cs.Spec.Services != nil { - for _, service := range cs.Spec.Services { - serviceMap := make(map[string]interface{}) - - // Convert service name - serviceMap["name"] = service.Name - - // Convert service spec - if service.Spec != nil { - specMap := make(map[string]interface{}) - for key, val := range service.Spec { - var rawValue interface{} - if err := json.Unmarshal(val.Raw, &rawValue); err == nil { - specMap[key] = rawValue - } +// convertServiceConfigs converts CommonService service specs to []interface{} maps. +// This is the shared conversion logic used by both extractCustomSizeConfigs and extractSizeTemplate. +func convertServiceConfigs(services []apiv3.ServiceConfig) []interface{} { + if services == nil { + return nil + } + var result []interface{} + for _, service := range services { + serviceMap := make(map[string]interface{}) + serviceMap["name"] = service.Name + + if service.Spec != nil { + specMap := make(map[string]interface{}) + for key, val := range service.Spec { + var rawValue interface{} + if err := json.Unmarshal(val.Raw, &rawValue); err == nil { + specMap[key] = rawValue } - serviceMap["spec"] = specMap } + serviceMap["spec"] = specMap + } - // Convert service resources - if service.Resources != nil { - var resources []interface{} - for _, res := range service.Resources { - var rawResource interface{} - if err := json.Unmarshal(res.Raw, &rawResource); err == nil { - resources = append(resources, rawResource) - } + if service.Resources != nil { + var resources []interface{} + for _, res := range service.Resources { + var rawResource interface{} + if err := json.Unmarshal(res.Raw, &rawResource); err == nil { + resources = append(resources, rawResource) } - serviceMap["resources"] = resources } + serviceMap["resources"] = resources + } - // Handle management strategy + if service.ManagementStrategy != "" { + serviceMap["managementStrategy"] = service.ManagementStrategy + } + + result = append(result, serviceMap) + } + return result +} + +// extractCustomSizeConfigs extracts custom size configurations from services field +func extractCustomSizeConfigs(cs *apiv3.CommonService, serviceControllerMapping map[string]string) ([]interface{}, map[string]string) { + dest := convertServiceConfigs(cs.Spec.Services) + + // Populate serviceControllerMapping from management strategies + if cs.Spec.Services != nil { + for _, service := range cs.Spec.Services { if service.ManagementStrategy != "" { - serviceMap["managementStrategy"] = service.ManagementStrategy serviceControllerMapping[service.Name] = service.ManagementStrategy } - - dest = append(dest, serviceMap) } } @@ -272,41 +282,7 @@ func extractCustomSizeConfigs(cs *apiv3.CommonService, serviceControllerMapping // extractSizeTemplate applies a size template and merges with custom services func extractSizeTemplate(cs *apiv3.CommonService, sizeTemplate string, serviceControllerMapping map[string]string, opconNs string) ([]interface{}, map[string]string, error) { // Convert custom services to []interface{} - var src []interface{} - if cs.Spec.Services != nil { - for _, service := range cs.Spec.Services { - serviceMap := make(map[string]interface{}) - serviceMap["name"] = service.Name - - if service.Spec != nil { - specMap := make(map[string]interface{}) - for key, val := range service.Spec { - var rawValue interface{} - if err := json.Unmarshal(val.Raw, &rawValue); err == nil { - specMap[key] = rawValue - } - } - serviceMap["spec"] = specMap - } - - if service.Resources != nil { - var resources []interface{} - for _, res := range service.Resources { - var rawResource interface{} - if err := json.Unmarshal(res.Raw, &rawResource); err == nil { - resources = append(resources, rawResource) - } - } - serviceMap["resources"] = resources - } - - if service.ManagementStrategy != "" { - serviceMap["managementStrategy"] = service.ManagementStrategy - } - - src = append(src, serviceMap) - } - } + src := convertServiceConfigs(cs.Spec.Services) // Convert size template string to slice sizes, err := convertStringToSlice(sizeTemplate) @@ -367,5 +343,3 @@ func extractSizeTemplate(cs *apiv3.CommonService, sizeTemplate string, serviceCo return sizes, serviceControllerMapping, nil } - -// Made with Bob diff --git a/internal/controller/config_merger.go b/internal/controller/config_merger.go index 1c3e361fd..0e68efc00 100644 --- a/internal/controller/config_merger.go +++ b/internal/controller/config_merger.go @@ -157,12 +157,12 @@ func validateMergedConfig(config *odlm.OperandConfig) error { // MergeConfigs is a convenience function that combines extraction and merging // Used by bootstrap to create complete OperandConfig in single stage func MergeConfigs( - r *CommonServiceReconciler, baseConfig string, cs *apiv3.CommonService, + servicesNs string, ) (string, error) { // Extract CommonService configurations - csConfigs, serviceControllerMapping, err := ExtractCommonServiceConfigs(cs, r.Bootstrap.CSData.ServicesNs) + csConfigs, serviceControllerMapping, err := ExtractCommonServiceConfigs(cs, servicesNs) if err != nil { return "", fmt.Errorf("failed to extract CommonService configs: %v", err) } @@ -172,7 +172,7 @@ func MergeConfigs( baseConfig, csConfigs, serviceControllerMapping, - r.Bootstrap.CSData.ServicesNs, + servicesNs, ) if err != nil { return "", fmt.Errorf("failed to merge configurations: %v", err) @@ -185,6 +185,6 @@ func MergeConfigs( // This is used to inject the merge logic into bootstrap without import cycles func CreateMergerFunc(r *CommonServiceReconciler) func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { return func(baseConfig string, cs *apiv3.CommonService, servicesNs string) (string, error) { - return MergeConfigs(r, baseConfig, cs) + return MergeConfigs(baseConfig, cs, servicesNs) } } diff --git a/internal/controller/config_merger_test.go b/internal/controller/config_merger_test.go index 9c70a1850..0ed08862e 100644 --- a/internal/controller/config_merger_test.go +++ b/internal/controller/config_merger_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apiv3 "github.com/IBM/ibm-common-service-operator/v4/api/v3" odlm "github.com/IBM/operand-deployment-lifecycle-manager/v4/api/v1alpha1" ) @@ -373,3 +374,65 @@ spec: assert.False(t, strings.Contains(result, `"replicas":1`) || strings.Contains(result, "replicas: 1"), "merged result must not retain the base replica count of 1 for ibm-iam-operator") } + +// TestMergeConfigs_EmptyCS verifies that MergeConfigs handles an empty CommonService +// gracefully and returns the base config unchanged. +func TestMergeConfigs_EmptyCS(t *testing.T) { + cs := &apiv3.CommonService{} + result, err := MergeConfigs(minimalBaseOpconYAML, cs, "ibm-common-services") + require.NoError(t, err) + assert.NotEmpty(t, result) + + // Verify the result still contains the original services + opcon, err := parseOperandConfig(result) + require.NoError(t, err) + assert.Len(t, opcon.Spec.Services, 2) +} + +// TestMergeConfigs_WithCSFeatures verifies that MergeConfigs properly extracts +// CommonService configurations and merges them with the base config. +func TestMergeConfigs_WithCSFeatures(t *testing.T) { + cs := &apiv3.CommonService{} + cs.Spec.StorageClass = "my-storage" + cs.Spec.RouteHost = "example.com" + + result, err := MergeConfigs(minimalBaseOpconYAML, cs, "ibm-common-services") + require.NoError(t, err) + assert.NotEmpty(t, result) + + opcon, err := parseOperandConfig(result) + require.NoError(t, err) + assert.NotEmpty(t, opcon.Spec.Services) +} + +// TestMergeConfigs_InvalidBaseConfig verifies that an invalid base config +// returns an error through MergeConfigs. +func TestMergeConfigs_InvalidBaseConfig(t *testing.T) { + cs := &apiv3.CommonService{} + _, err := MergeConfigs("{ not valid yaml: [", cs, "ibm-common-services") + assert.Error(t, err) +} + +// TestMergeConfigs_PassesServicesNs verifies that MergeConfigs passes servicesNs +// through to the extraction and merging functions, not using a hardcoded value. +func TestMergeConfigs_PassesServicesNs(t *testing.T) { + cs := &apiv3.CommonService{} + cs.Spec.Size = "small" + + customNsYAML := ` +apiVersion: operator.ibm.com/v1alpha1 +kind: OperandConfig +metadata: + name: common-service + namespace: custom-ns +spec: + services: + - name: ibm-iam-operator + spec: + authentication: + replicas: 1 +` + result, err := MergeConfigs(customNsYAML, cs, "custom-ns") + require.NoError(t, err) + assert.NotEmpty(t, result) +} diff --git a/internal/controller/no_olm_commonservice_controller.go b/internal/controller/no_olm_commonservice_controller.go index cfdee3fae..9da592576 100644 --- a/internal/controller/no_olm_commonservice_controller.go +++ b/internal/controller/no_olm_commonservice_controller.go @@ -184,10 +184,6 @@ func (r *CommonServiceReconciler) ReconcileNoOLMMasterCR(ctx context.Context, in klog.Error("ODLM CRD not ready, waiting for it to be ready") } - // OperandConfig already created with complete configuration - // in InstallOrUpdateOpcon, no need for second updateOperandConfig call - klog.Info("OperandConfig created with complete configuration via single-stage creation") - var isEqual bool if isEqual, statusErr = r.updateOperatorConfig(ctx, instance.Spec.OperatorConfigs); statusErr != nil { diff --git a/internal/controller/operandconfig.go b/internal/controller/operandconfig.go index bc155cf74..e900eec3e 100644 --- a/internal/controller/operandconfig.go +++ b/internal/controller/operandconfig.go @@ -134,49 +134,152 @@ func mergeCSCRs(csSummary, csCR, ruleSlice []interface{}, serviceControllerMappi csSummary = setSpecByName(csSummary, operator.(map[string]interface{})["name"].(string), summaryCR.(map[string]interface{})["spec"]) } - // check if operator.(map[string]interface{})["resources"] is nil - if operator.(map[string]interface{})["resources"] != nil { - for i, opResource := range operator.(map[string]interface{})["resources"].([]interface{}) { - var apiVersion, kind, name, namespace string - if opResource.(map[string]interface{})["apiVersion"] != nil { - apiVersion = opResource.(map[string]interface{})["apiVersion"].(string) - } - if opResource.(map[string]interface{})["kind"] != nil { - kind = opResource.(map[string]interface{})["kind"].(string) - } - if opResource.(map[string]interface{})["name"] != nil { - name = opResource.(map[string]interface{})["name"].(string) - } - if opResource.(map[string]interface{})["namespace"] != nil { - namespace = opResource.(map[string]interface{})["namespace"].(string) - } - // check if above 4 fields are all set - if apiVersion == "" || kind == "" || name == "" { - klog.Warningf("Skipping merging resource %s/%s/%s/%s, because apiVersion, kind or name is not set", apiVersion, kind, name, namespace) - continue - } - // check if namespace is set, if not, set it to OperandConfig namespace - if namespace == "" { - namespace = opconNs - } - if summaryCR == nil || summaryCR.(map[string]interface{})["resources"] == nil { - continue - } - newResource := getItemByGVKNameNamespace(summaryCR.(map[string]interface{})["resources"].([]interface{}), opconNs, apiVersion, kind, name, namespace) - if newResource != nil { - if _, ok := nonDefaultProfileController[serviceController]; ok { - if isOpResourceExists(newResource) { - klog.Info("### DEBUG: deleting key") - newResource.(map[string]interface{})["data"].(map[string]interface{})["spec"].(map[string]interface{})["resources"].(map[string]interface{})["limits"].(map[string]interface{})["cpu"] = struct{}{} + // Merge resources: preserve base resources and merge/add CS resources + // This fixes the bug where base-only resources (like Certificates) were lost + if operator.(map[string]interface{})["resources"] != nil || (summaryCR != nil && summaryCR.(map[string]interface{})["resources"] != nil) { + // Get base resources from summary (these must be preserved) + baseResources := []interface{}{} + if summaryCR != nil && summaryCR.(map[string]interface{})["resources"] != nil { + baseResources = summaryCR.(map[string]interface{})["resources"].([]interface{}) + } + + // Get CS resources to merge + csResources := []interface{}{} + if operator.(map[string]interface{})["resources"] != nil { + csResources = operator.(map[string]interface{})["resources"].([]interface{}) + } + + // Merge resources: update base with CS overrides, preserve base-only resources + mergedResources := mergeResourceArrays(baseResources, csResources, opconNs, serviceController) + + // Set the merged resources + csSummary = setResByName(csSummary, operator.(map[string]interface{})["name"].(string), mergedResources) + } + } + return csSummary +} + +// toStringMap safely converts an interface{} to map[string]interface{}. +// Returns nil and false if the conversion fails. +func toStringMap(v interface{}) (map[string]interface{}, bool) { + m, ok := v.(map[string]interface{}) + return m, ok +} + +// getStringField safely extracts a string field from a resource map. +func getStringField(m map[string]interface{}, key string) string { + if v, ok := m[key]; ok { + if s, ok := v.(string); ok { + return s + } + } + return "" +} + +// mergeResourceArrays merges base resources with CS resources, preserving base-only resources +// and allowing CS resources to override matching base resources. +// Base resource ordering is preserved: matched base resources are replaced in-place, +// and new CS-only resources are appended at the end. +func mergeResourceArrays(baseResources, csResources []interface{}, opconNs string, serviceController string) []interface{} { + // Map from base index → merged result (populated during CS resource iteration) + mergedByBaseIndex := make(map[int]interface{}) + // Track which CS resources matched a base resource + matchedCSIndices := make(map[int]bool) + + // First pass: find matches between CS and base resources, merge them + for csIdx, csResource := range csResources { + csMap, ok := toStringMap(csResource) + if !ok { + klog.Warningf("Skipping CS resource: unexpected type %T", csResource) + matchedCSIndices[csIdx] = true // mark as handled (skipped) + continue + } + + csApiVersion := getStringField(csMap, "apiVersion") + csKind := getStringField(csMap, "kind") + csName := getStringField(csMap, "name") + csNamespace := getStringField(csMap, "namespace") + + // Validate required fields + if csApiVersion == "" || csKind == "" || csName == "" { + klog.Warningf("Skipping CS resource %s/%s/%s/%s: missing required fields", csApiVersion, csKind, csName, csNamespace) + matchedCSIndices[csIdx] = true // mark as handled (skipped) + continue + } + + // Default namespace to opconNs if not set + if csNamespace == "" { + csNamespace = opconNs + } + + // Find matching base resource + for baseIdx, baseResource := range baseResources { + if _, alreadyMerged := mergedByBaseIndex[baseIdx]; alreadyMerged { + continue // Already matched by another CS resource + } + + baseMap, ok := toStringMap(baseResource) + if !ok { + continue + } + + baseApiVersion := getStringField(baseMap, "apiVersion") + baseKind := getStringField(baseMap, "kind") + baseName := getStringField(baseMap, "name") + baseNamespace := getStringField(baseMap, "namespace") + if baseNamespace == "" { + baseNamespace = opconNs + } + + // Check if resources match by GVK+Name+Namespace + if baseApiVersion == csApiVersion && baseKind == csKind && baseName == csName && baseNamespace == csNamespace { + // Merge CS resource into base resource + mergedResource := mergeCRsIntoOperandConfigWithDefaultRules(csMap, baseMap, false) + + // Apply profile controller cleanup if needed + if _, ok := nonDefaultProfileController[serviceController]; ok { + if isOpResourceExists(mergedResource) { + klog.V(2).Info("Applying profile controller cleanup to merged resource") + if dataMap, ok := toStringMap(mergedResource["data"]); ok { + if specMap, ok := toStringMap(dataMap["spec"]); ok { + if resources, ok := toStringMap(specMap["resources"]); ok { + if limits, ok := toStringMap(resources["limits"]); ok { + limits["cpu"] = struct{}{} + } + } + } } } - operator.(map[string]interface{})["resources"].([]interface{})[i] = mergeCRsIntoOperandConfigWithDefaultRules(opResource.(map[string]interface{}), newResource.(map[string]interface{}), false) } + + mergedByBaseIndex[baseIdx] = mergedResource + matchedCSIndices[csIdx] = true + break } - csSummary = setResByName(csSummary, operator.(map[string]interface{})["name"].(string), operator.(map[string]interface{})["resources"].([]interface{})) } } - return csSummary + + // Second pass: build result in base order, replacing matched base resources with merged results + mergedResources := make([]interface{}, 0, len(baseResources)+len(csResources)) + for i, baseResource := range baseResources { + if merged, ok := mergedByBaseIndex[i]; ok { + mergedResources = append(mergedResources, merged) + } else { + mergedResources = append(mergedResources, baseResource) + } + } + + // Third pass: append new CS-only resources (those that didn't match any base resource) + for csIdx, csResource := range csResources { + if !matchedCSIndices[csIdx] { + mergedResources = append(mergedResources, csResource) + } + } + + klog.V(2).Infof("Merged resources: %d base + %d CS = %d total (preserved %d base-only)", + len(baseResources), len(csResources), len(mergedResources), len(baseResources)-len(mergedByBaseIndex)) + + return mergedResources } // mergeCRsIntoOperandConfig merges CRs by specific rules @@ -469,7 +572,7 @@ func (r *CommonServiceReconciler) updateOperandConfig(ctx context.Context, newCo if newResource != nil { if _, ok := nonDefaultProfileController[serviceController]; ok { if isOpResourceExists(newResource) { - klog.Info("### DEBUG: deleting key") + klog.V(2).Info("Clearing CPU limits for non-default profile controller") newResource.(map[string]interface{})["data"].(map[string]interface{})["spec"].(map[string]interface{})["resources"].(map[string]interface{})["limits"].(map[string]interface{})["cpu"] = struct{}{} } } @@ -517,16 +620,23 @@ func (r *CommonServiceReconciler) updateOperandConfig(ctx context.Context, newCo } func isOpResourceExists(opResource interface{}) bool { - if opResource.(map[string]interface{})["data"] == nil { - klog.Info("### DEBUG: data not exists") + resMap, ok := toStringMap(opResource) + if !ok { + klog.V(2).Info("Resource is not a map") + return false + } + dataMap, ok := toStringMap(resMap["data"]) + if !ok { + klog.V(2).Info("Resource has no data field") return false } - if opResource.(map[string]interface{})["data"].(map[string]interface{})["spec"] == nil { - klog.Info("### DEBUG: data not exists") + specMap, ok := toStringMap(dataMap["spec"]) + if !ok { + klog.V(2).Info("Resource data has no spec field") return false } - if opResource.(map[string]interface{})["data"].(map[string]interface{})["spec"].(map[string]interface{})["resources"] == nil { - klog.Info("### DEBUG: resources not exists") + if specMap["resources"] == nil { + klog.V(2).Info("Resource spec has no resources field") return false } return true @@ -544,25 +654,22 @@ func (r *CommonServiceReconciler) getExtremeizes(ctx context.Context, opconServi }); err != nil { return []interface{}{}, err } - csList, err := util.ObjectListToNewUnstructuredList(csObjectList) - if err != nil { - return []interface{}{}, err - } + var configSummary []interface{} tmpConfigsSlice := make(map[int][]interface{}) serviceControllerMappingSummary := make(map[string]string) - for _, cs := range csList.Items { + for i, cs := range csObjectList.Items { if cs.GetDeletionTimestamp() != nil { continue } - csConfigs, serviceControllerMapping, err := r.getNewConfigs(&cs) + csConfigs, serviceControllerMapping, err := ExtractCommonServiceConfigs(&cs, r.CSData.ServicesNs) if err != nil { return []interface{}{}, err } serviceControllerMappingSummary = mergeProfileController(serviceControllerMappingSummary, serviceControllerMapping) - tmpConfigsSlice[len(tmpConfigsSlice)] = csConfigs + tmpConfigsSlice[i] = csConfigs } for _, csConfigs := range tmpConfigsSlice { configSummary = mergeCSCRs(configSummary, csConfigs, ruleSlice, serviceControllerMappingSummary, r.CSData.ServicesNs) @@ -626,7 +733,7 @@ func (r *CommonServiceReconciler) getExtremeizes(ctx context.Context, opconServi if summarizedRes != nil { if _, ok := nonDefaultProfileController[serviceController]; ok { if isOpResourceExists(summarizedRes) { - klog.Info("### DEBUG: deleting key") + klog.V(2).Info("Clearing CPU limits for non-default profile controller in summarized resource") summarizedRes.(map[string]interface{})["data"].(map[string]interface{})["spec"].(map[string]interface{})["resources"].(map[string]interface{})["limits"].(map[string]interface{})["cpu"] = struct{}{} } } diff --git a/internal/controller/operandconfig_test.go b/internal/controller/operandconfig_test.go new file mode 100644 index 000000000..b6531a9a8 --- /dev/null +++ b/internal/controller/operandconfig_test.go @@ -0,0 +1,423 @@ +// +// Copyright 2022 IBM Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package controllers + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMergeResourceArrays_PreservesBaseOnlyResources verifies that base resources +// not present in CS resources are preserved in the merged result. +// This is the critical test for the bug fix. +func TestMergeResourceArrays_PreservesBaseOnlyResources(t *testing.T) { + baseResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "common-service-db-tls-cert", + }, + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "common-service-db-replica-tls-cert", + }, + map[string]interface{}{ + "apiVersion": "postgresql.k8s.enterprisedb.io/v1", + "kind": "Cluster", + "name": "common-service-db", + }, + } + + // CS resources only contain the Cluster, not the Certificates + csResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "postgresql.k8s.enterprisedb.io/v1", + "kind": "Cluster", + "name": "common-service-db", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "instances": float64(2), + }, + }, + }, + } + + merged := mergeResourceArrays(baseResources, csResources, "cs4-data", "default") + + // All 3 resources must be present + require.Len(t, merged, 3, "Merged result must contain all base resources plus CS resources") + + // Verify Certificates are preserved + certCount := 0 + clusterCount := 0 + for _, res := range merged { + resMap := res.(map[string]interface{}) + if resMap["kind"] == "Certificate" { + certCount++ + } + if resMap["kind"] == "Cluster" { + clusterCount++ + } + } + + assert.Equal(t, 2, certCount, "Both Certificate resources must be preserved") + assert.Equal(t, 1, clusterCount, "Cluster resource must be present") +} + +// TestMergeResourceArrays_MergesMatchingResources verifies that when a resource +// exists in both base and CS, they are properly merged. +func TestMergeResourceArrays_MergesMatchingResources(t *testing.T) { + baseResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "postgresql.k8s.enterprisedb.io/v1", + "kind": "Cluster", + "name": "common-service-db", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "instances": float64(1), + "storage": map[string]interface{}{ + "size": "10Gi", + }, + }, + }, + }, + } + + csResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "postgresql.k8s.enterprisedb.io/v1", + "kind": "Cluster", + "name": "common-service-db", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "instances": float64(3), + }, + }, + }, + } + + merged := mergeResourceArrays(baseResources, csResources, "cs4-data", "default") + + require.Len(t, merged, 1, "Should have one merged resource") + + mergedRes := merged[0].(map[string]interface{}) + assert.Equal(t, "Cluster", mergedRes["kind"]) + assert.Equal(t, "common-service-db", mergedRes["name"]) + + // Verify the merge happened (instances should be updated) + data := mergedRes["data"].(map[string]interface{}) + spec := data["spec"].(map[string]interface{}) + assert.Equal(t, float64(3), spec["instances"], "CS value should override base value") +} + +// TestMergeResourceArrays_AddsNewCSResources verifies that resources only in CS +// are added to the result. +func TestMergeResourceArrays_AddsNewCSResources(t *testing.T) { + baseResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "base-cert", + }, + } + + csResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "name": "new-configmap", + }, + } + + merged := mergeResourceArrays(baseResources, csResources, "cs4-data", "default") + + require.Len(t, merged, 2, "Should have both base and new CS resources") + + names := make([]string, 0, 2) + for _, res := range merged { + names = append(names, res.(map[string]interface{})["name"].(string)) + } + + assert.Contains(t, names, "base-cert", "Base resource must be preserved") + assert.Contains(t, names, "new-configmap", "New CS resource must be added") +} + +// TestMergeResourceArrays_EmptyBaseResources verifies behavior when base has no resources. +func TestMergeResourceArrays_EmptyBaseResources(t *testing.T) { + baseResources := []interface{}{} + + csResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "name": "cs-configmap", + }, + } + + merged := mergeResourceArrays(baseResources, csResources, "cs4-data", "default") + + require.Len(t, merged, 1, "Should have CS resource") + assert.Equal(t, "cs-configmap", merged[0].(map[string]interface{})["name"]) +} + +// TestMergeResourceArrays_EmptyCSResources verifies that all base resources are +// preserved when CS has no resources. +func TestMergeResourceArrays_EmptyCSResources(t *testing.T) { + baseResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "base-cert-1", + }, + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "base-cert-2", + }, + } + + csResources := []interface{}{} + + merged := mergeResourceArrays(baseResources, csResources, "cs4-data", "default") + + require.Len(t, merged, 2, "All base resources must be preserved") + assert.Equal(t, "base-cert-1", merged[0].(map[string]interface{})["name"]) + assert.Equal(t, "base-cert-2", merged[1].(map[string]interface{})["name"]) +} + +// TestMergeResourceArrays_MatchesByGVKNameNamespace verifies that resources are +// matched correctly by GroupVersionKind + Name + Namespace. +func TestMergeResourceArrays_MatchesByGVKNameNamespace(t *testing.T) { + baseResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "name": "my-config", + "namespace": "ns1", + }, + map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "name": "my-config", + "namespace": "ns2", + }, + } + + // CS resource matches only the ns1 ConfigMap + csResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "name": "my-config", + "namespace": "ns1", + "data": map[string]interface{}{ + "key": "updated-value", + }, + }, + } + + merged := mergeResourceArrays(baseResources, csResources, "cs4-data", "default") + + require.Len(t, merged, 2, "Both ConfigMaps should be present") + + // Find the ns1 ConfigMap and verify it was merged + var ns1Config map[string]interface{} + var ns2Config map[string]interface{} + for _, res := range merged { + resMap := res.(map[string]interface{}) + if resMap["namespace"] == "ns1" { + ns1Config = resMap + } else if resMap["namespace"] == "ns2" { + ns2Config = resMap + } + } + + require.NotNil(t, ns1Config, "ns1 ConfigMap must be present") + require.NotNil(t, ns2Config, "ns2 ConfigMap must be present") + + // ns1 should have the merged data + assert.NotNil(t, ns1Config["data"], "ns1 ConfigMap should have merged data") + + // ns2 should be unchanged (no data field) + assert.Nil(t, ns2Config["data"], "ns2 ConfigMap should be unchanged") +} + +// TestMergeResourceArrays_HandlesInvalidCSResources verifies that CS resources +// with missing required fields are skipped with a warning. +func TestMergeResourceArrays_HandlesInvalidCSResources(t *testing.T) { + baseResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "name": "valid-config", + }, + } + + csResources := []interface{}{ + map[string]interface{}{ + // Missing apiVersion + "kind": "ConfigMap", + "name": "invalid-config", + }, + map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "name": "another-valid-config", + }, + } + + merged := mergeResourceArrays(baseResources, csResources, "cs4-data", "default") + + // Should have base + 1 valid CS resource (invalid one skipped) + require.Len(t, merged, 2, "Should have base resource and one valid CS resource") + + names := make([]string, 0, 2) + for _, res := range merged { + names = append(names, res.(map[string]interface{})["name"].(string)) + } + + assert.Contains(t, names, "valid-config") + assert.Contains(t, names, "another-valid-config") + assert.NotContains(t, names, "invalid-config", "Invalid resource should be skipped") +} + +// TestMergeResourceArrays_PostgreSQLCertificatesScenario tests the exact scenario +// from the bug: PostgreSQL service with Certificates in base, only Cluster in CS. +func TestMergeResourceArrays_PostgreSQLCertificatesScenario(t *testing.T) { + // This replicates the actual bug scenario + baseResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "common-service-db-replica-tls-cert", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "commonName": "streaming_replica", + "secretName": "common-service-db-replica-tls-secret", + }, + }, + }, + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "common-service-db-tls-cert", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "secretName": "common-service-db-tls-secret", + "dnsNames": []interface{}{ + "common-service-db", + "common-service-db.cs4-data", + }, + }, + }, + }, + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "common-service-db-im-tls-cert", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "commonName": "im_user", + "secretName": "common-service-db-im-tls-secret", + }, + }, + }, + map[string]interface{}{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "name": "common-service-db-zen-tls-cert", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "commonName": "zen_user", + "secretName": "common-service-db-zen-tls-secret", + }, + }, + }, + map[string]interface{}{ + "apiVersion": "postgresql.k8s.enterprisedb.io/v1", + "kind": "Cluster", + "name": "common-service-db", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "instances": float64(1), + }, + }, + }, + } + + // CS resources from size profile - only has Cluster + csResources := []interface{}{ + map[string]interface{}{ + "apiVersion": "postgresql.k8s.enterprisedb.io/v1", + "kind": "Cluster", + "name": "common-service-db", + "data": map[string]interface{}{ + "spec": map[string]interface{}{ + "instances": float64(1), + "resources": map[string]interface{}{ + "limits": map[string]interface{}{ + "cpu": "200m", + "memory": "512Mi", + }, + }, + }, + }, + }, + } + + merged := mergeResourceArrays(baseResources, csResources, "cs4-data", "default") + + // CRITICAL: All 5 resources must be present (4 Certificates + 1 Cluster) + require.Len(t, merged, 5, "All base resources must be preserved: 4 Certificates + 1 Cluster") + + // Verify all Certificates are present + certNames := []string{ + "common-service-db-replica-tls-cert", + "common-service-db-tls-cert", + "common-service-db-im-tls-cert", + "common-service-db-zen-tls-cert", + } + + foundCerts := make(map[string]bool) + var clusterResource map[string]interface{} + + for _, res := range merged { + resMap := res.(map[string]interface{}) + if resMap["kind"] == "Certificate" { + foundCerts[resMap["name"].(string)] = true + } else if resMap["kind"] == "Cluster" { + clusterResource = resMap + } + } + + // Verify all Certificates are present + for _, certName := range certNames { + assert.True(t, foundCerts[certName], "Certificate %s must be preserved", certName) + } + + // Verify Cluster was merged with CS values + require.NotNil(t, clusterResource, "Cluster resource must be present") + assert.Equal(t, "common-service-db", clusterResource["name"]) + + // Verify Cluster has merged spec from CS + data := clusterResource["data"].(map[string]interface{}) + spec := data["spec"].(map[string]interface{}) + assert.NotNil(t, spec["resources"], "Cluster should have resources from CS config") +}