diff --git a/internal/controller/bootstrap/config_bridge.go b/internal/controller/bootstrap/config_bridge.go new file mode 100644 index 000000000..85484ca70 --- /dev/null +++ b/internal/controller/bootstrap/config_bridge.go @@ -0,0 +1,43 @@ +// +// 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 bootstrap + +import ( + apiv3 "github.com/IBM/ibm-common-service-operator/v4/api/v3" +) + +// ConfigMergerFunc is a function type that merges base config with CommonService configs +// 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 +} + +// 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 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 new file mode 100644 index 000000000..060c9334c --- /dev/null +++ b/internal/controller/bootstrap/config_bridge_test.go @@ -0,0 +1,199 @@ +// +// 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 bootstrap + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + 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 +} + +// TestSetConfigMerger_NilByDefault verifies that before SetConfigMerger is called +// the package-level 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"}, + } + cs := &apiv3.CommonService{} + + result, err := b.mergeConfigs("base-config", cs) + require.NoError(t, err) + assert.Equal(t, "base-config", result, + "when no merger is set, mergeConfigs must return the base config unchanged") +} + +// 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) { + called = true + return "merged-" + baseConfig, nil + }) + + b := &Bootstrap{ + CSData: apiv3.CSData{ServicesNs: "ibm-common-services"}, + } + cs := &apiv3.CommonService{} + + result, err := b.mergeConfigs("base", cs) + require.NoError(t, err) + assert.True(t, called, "the injected merger function must be called") + assert.Equal(t, "merged-base", result) +} + +// 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) { + capturedNs = servicesNs + return baseConfig, nil + }) + + b := &Bootstrap{ + CSData: apiv3.CSData{ServicesNs: "my-services-ns"}, + } + cs := &apiv3.CommonService{} + + _, err := b.mergeConfigs("config", cs) + require.NoError(t, err) + assert.Equal(t, "my-services-ns", capturedNs, + "mergeConfigs must pass Bootstrap.CSData.ServicesNs to the merger function") +} + +// 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) { + 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" + + _, err := b.mergeConfigs("config", cs) + require.NoError(t, err) + require.NotNil(t, capturedCS) + assert.Equal(t, "common-service", capturedCS.Name) + assert.Equal(t, "ibm-common-services", capturedCS.Namespace) +} + +// 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) { + return "", mergeErr + }) + + b := &Bootstrap{ + CSData: apiv3.CSData{ServicesNs: "ibm-common-services"}, + } + cs := &apiv3.CommonService{} + + result, err := b.mergeConfigs("config", cs) + assert.Error(t, err) + assert.Equal(t, mergeErr, err) + assert.Empty(t, result) +} + +// 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) { + return "first", nil + }) + 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) + require.NoError(t, err) + assert.Equal(t, "second", result, + "the second SetConfigMerger call must overwrite the first") +} + +// TestMergeConfigs_SingleStageNoRaceCondition is the key integration test for the +// Single-Stage Creation feature. It verifies that a single mergeConfigs call +// 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) { + // 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) + require.NoError(t, err) + + // The result must already contain CS values — no second call needed. + assert.Equal(t, "base-opcon+cs-values", result, + "single mergeConfigs call must return complete config with CS values applied") + assert.NotEqual(t, "base-opcon", result, + "result must not be the bare base config (that would indicate an incomplete intermediate state)") +} diff --git a/internal/controller/bootstrap/init.go b/internal/controller/bootstrap/init.go index fad8524e5..1015cda53 100644 --- a/internal/controller/bootstrap/init.go +++ b/internal/controller/bootstrap/init.go @@ -319,7 +319,7 @@ func (b *Bootstrap) InitResources(instance *apiv3.CommonService, forceUpdateODLM } klog.Info("Installing/Updating OperandConfig") - if err := b.InstallOrUpdateOpcon(forceUpdateODLMCRs); err != nil { + if err := b.InstallOrUpdateOpcon(forceUpdateODLMCRs, instance); err != nil { return err } } @@ -369,7 +369,7 @@ func (b *Bootstrap) InitResources(instance *apiv3.CommonService, forceUpdateODLM } klog.Info("Installing/Updating OperandConfig") - if err := b.InstallOrUpdateOpcon(forceUpdateODLMCRs); err != nil { + if err := b.InstallOrUpdateOpcon(forceUpdateODLMCRs, instance); err != nil { return err } } @@ -960,7 +960,8 @@ func (b *Bootstrap) InstallOrUpdateOpreg(ctx context.Context, installPlanApprova } // InstallOrUpdateOpcon will install or update OperandConfig when Opcon CRD is existent -func (b *Bootstrap) InstallOrUpdateOpcon(forceUpdateODLMCRs bool) error { +// Now accepts CommonService instance with merged configurations +func (b *Bootstrap) InstallOrUpdateOpcon(forceUpdateODLMCRs bool, csInstance *apiv3.CommonService) error { var baseCon string configs := []string{ @@ -983,7 +984,21 @@ func (b *Bootstrap) InstallOrUpdateOpcon(forceUpdateODLMCRs bool) error { return err } - if err := b.renderTemplate(concatenatedCon, b.CSData, forceUpdateODLMCRs); err != nil { + // Merge CommonService configurations with base templates + // before rendering to create complete OperandConfig in one step + finalConfig := concatenatedCon + if csInstance != nil { + klog.Info("Merging CommonService configurations with base OperandConfig") + mergedConfig, err := b.mergeConfigs(concatenatedCon, csInstance) + if err != nil { + klog.Errorf("Failed to merge configurations: %v", err) + return err + } + finalConfig = mergedConfig + klog.Info("Successfully merged configurations for single-stage OperandConfig creation") + } + + if err := b.renderTemplate(finalConfig, b.CSData, forceUpdateODLMCRs); err != nil { return err } return nil diff --git a/internal/controller/commonservice_controller.go b/internal/controller/commonservice_controller.go index fcb4b21e3..30c0cf94f 100644 --- a/internal/controller/commonservice_controller.go +++ b/internal/controller/commonservice_controller.go @@ -248,38 +248,11 @@ func (r *CommonServiceReconciler) ReconcileMasterCR(ctx context.Context, instanc return ctrl.Result{}, statusErr } - // Apply new configs to CommonService CR - cs := util.NewUnstructured("operator.ibm.com", "CommonService", "v3") - if statusErr = r.Bootstrap.Client.Get(ctx, types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, cs); statusErr != nil { - klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, statusErr) - return ctrl.Result{}, statusErr - } - // Set "Pending" condition and "Updating" for phase when config CS CR - instance.SetPendingCondition(constant.MasterCR, apiv3.ConditionTypeReconciling, corev1.ConditionTrue, apiv3.ConditionReasonConfig, apiv3.ConditionMessageConfig) - instance.Status.Phase = apiv3.CRUpdating - newConfigs, serviceControllerMapping, statusErr := r.getNewConfigs(cs) - if statusErr != nil { - klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, statusErr) - instance.SetErrorCondition(constant.MasterCR, apiv3.ConditionTypeError, corev1.ConditionTrue, apiv3.ConditionReasonError, statusErr.Error()) - instance.Status.Phase = apiv3.CRFailed - } - - if statusErr = r.Client.Status().Update(ctx, instance); statusErr != nil { - klog.Errorf("Fail to update %s/%s: %v", instance.Namespace, instance.Name, statusErr) - 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.updateOperandConfig(ctx, newConfigs, serviceControllerMapping); statusErr != nil { - if statusErr := r.updatePhase(ctx, instance, apiv3.CRFailed); statusErr != nil { - klog.Error(statusErr) - } - klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, statusErr) - return ctrl.Result{}, statusErr - } else if isEqual { - r.Recorder.Event(instance, corev1.EventTypeNormal, "Noeffect", fmt.Sprintf("No update, resource sizings in the OperandConfig %s/%s are larger than the profile from CommonService CR %s/%s", r.Bootstrap.CSData.OperatorNs, "common-service", instance.Namespace, instance.Name)) - } - if isEqual, statusErr = r.updateOperatorConfig(ctx, instance.Spec.OperatorConfigs); statusErr != nil { if statusErr := r.updatePhase(ctx, instance, apiv3.CRFailed); statusErr != nil { klog.Error(statusErr) @@ -371,11 +344,6 @@ func (r *CommonServiceReconciler) ReconcileGeneralCR(ctx context.Context, instan return ctrl.Result{}, err } - cs := util.NewUnstructured("operator.ibm.com", "CommonService", "v3") - if err := r.Bootstrap.Client.Get(ctx, types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, cs); err != nil { - klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, err) - return ctrl.Result{}, err - } // Generate Issuer and Certificate CR if err := r.Bootstrap.DeployCertManagerCR(); err != nil { klog.Errorf("Failed to deploy cert manager CRs: %v", err) @@ -386,7 +354,8 @@ func (r *CommonServiceReconciler) ReconcileGeneralCR(ctx context.Context, instan return ctrl.Result{}, err } - newConfigs, serviceControllerMapping, err := r.getNewConfigs(cs) + // Extract configurations using new extractor for subsequent updates + newConfigs, serviceControllerMapping, err := ExtractCommonServiceConfigs(instance, r.Bootstrap.CSData.ServicesNs) if err != nil { if err := r.updatePhase(ctx, instance, apiv3.CRFailed); err != nil { klog.Error(err) @@ -395,6 +364,7 @@ func (r *CommonServiceReconciler) ReconcileGeneralCR(ctx context.Context, instan return ctrl.Result{}, err } + // Update OperandConfig (single update for subsequent reconciliations) isEqual, err := r.updateOperandConfig(ctx, newConfigs, serviceControllerMapping) if err != nil { if err := r.updatePhase(ctx, instance, apiv3.CRFailed); err != nil { @@ -557,6 +527,10 @@ 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)) + klog.Info("Configuration merger initialized for single-stage OperandConfig creation") controller := ctrl.NewControllerManagedBy(mgr). // AnnotationChangedPredicate is intended to be used in conjunction with the GenerationChangedPredicate diff --git a/internal/controller/config_extractor.go b/internal/controller/config_extractor.go new file mode 100644 index 000000000..4015f191d --- /dev/null +++ b/internal/controller/config_extractor.go @@ -0,0 +1,371 @@ +// +// 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 ( + "bytes" + "encoding/json" + "fmt" + "strconv" + "strings" + "text/template" + + "k8s.io/klog" + + apiv3 "github.com/IBM/ibm-common-service-operator/v4/api/v3" + "github.com/IBM/ibm-common-service-operator/v4/internal/controller/constant" + "github.com/IBM/ibm-common-service-operator/v4/internal/controller/size" +) + +// ExtractCommonServiceConfigs extracts all configurations from CommonService CR +// This function is independent of reconciler context and can be used during bootstrap +func ExtractCommonServiceConfigs( + cs *apiv3.CommonService, + servicesNs string, +) ([]interface{}, map[string]string, error) { + var newConfigs []interface{} + + // Extract feature configurations + featureConfigs, err := extractFeatureConfigs(cs) + if err != nil { + return nil, nil, err + } + newConfigs = append(newConfigs, featureConfigs...) + + // Extract size configurations + sizeConfigs, serviceControllerMapping, err := extractSizeConfigs(cs, servicesNs) + if err != nil { + return nil, nil, err + } + newConfigs = append(newConfigs, sizeConfigs...) + + return newConfigs, serviceControllerMapping, nil +} + +// extractFeatureConfigs handles feature flag extraction +func extractFeatureConfigs(cs *apiv3.CommonService) ([]interface{}, error) { + var configs []interface{} + + // Extract storageClass configuration + if cs.Spec.StorageClass != "" { + klog.Info("Extracting storageClass configuration") + storageConfig, err := convertStringToSlice(strings.ReplaceAll(constant.StorageClassTemplate, "placeholder", cs.Spec.StorageClass)) + if err != nil { + return nil, err + } + configs = append(configs, storageConfig...) + } + + // Extract EnableInstanaMetricCollection configuration + if cs.Spec.EnableInstanaMetricCollection { + klog.Info("Extracting enableInstanaMetricCollection configuration") + t := template.Must(template.New("template InstanaEnable").Parse(constant.InstanaEnableTemplate)) + var tmplWriter bytes.Buffer + instanaEnable := struct { + InstanaEnable bool + }{ + InstanaEnable: cs.Spec.EnableInstanaMetricCollection, + } + if err := t.Execute(&tmplWriter, instanaEnable); err != nil { + return nil, err + } + instanaConfig, err := convertStringToSlice(tmplWriter.String()) + if err != nil { + return nil, err + } + configs = append(configs, instanaConfig...) + } + + // Extract AutoScaleConfig configuration + if cs.Spec.AutoScaleConfig { + klog.Info("Extracting autoScaleConfig configuration") + t := template.Must(template.New("template AutoScaleConfigTemplate").Parse(constant.AutoScaleConfigTemplate)) + var tmplWriter bytes.Buffer + autoScaleConfigEnable := struct { + AutoScaleConfigEnable bool + }{ + AutoScaleConfigEnable: cs.Spec.AutoScaleConfig, + } + if err := t.Execute(&tmplWriter, autoScaleConfigEnable); err != nil { + return nil, err + } + autoScaleConfig, err := convertStringToSlice(tmplWriter.String()) + if err != nil { + return nil, err + } + configs = append(configs, autoScaleConfig...) + } + + // Extract routeHost configuration + if cs.Spec.RouteHost != "" { + klog.Info("Extracting routeHost configuration") + routeHostConfig, err := convertStringToSlice(strings.ReplaceAll(constant.RouteHostTemplate, "placeholder", cs.Spec.RouteHost)) + if err != nil { + return nil, err + } + configs = append(configs, routeHostConfig...) + } + + // Extract default admin username configuration + if cs.Spec.DefaultAdminUser != "" { + klog.Info("Extracting default admin username configuration") + adminUsernameConfig, err := convertStringToSlice(strings.ReplaceAll(constant.DefaultAdminUserTemplate, "placeholder", cs.Spec.DefaultAdminUser)) + if err != nil { + return nil, err + } + configs = append(configs, adminUsernameConfig...) + } + + // Extract FIPS configuration + if cs.Spec.FipsEnabled { + klog.Info("Extracting fips configuration") + fipsEnabledConfig, err := convertStringToSlice(strings.ReplaceAll(constant.FipsEnabledTemplate, "placeholder", strconv.FormatBool(cs.Spec.FipsEnabled))) + if err != nil { + return nil, err + } + configs = append(configs, fipsEnabledConfig...) + } + + // Extract hugepages configuration + if cs.Spec.HugePages != nil && cs.Spec.HugePages.Enable { + klog.Info("Extracting hugepages configuration") + for size, allocation := range cs.Spec.HugePages.HugePagesSizes { + if !strings.HasPrefix(size, "hugepages-") { + return nil, fmt.Errorf("invalid hugepage size format: %s", size) + } + if allocation == "" { + allocation = constant.DefaultHugePageAllocation + } + replacer := strings.NewReplacer("placeholder1", size, "placeholder2", allocation) + hugePagesConfig, err := convertStringToSlice(replacer.Replace(constant.HugePagesTemplate)) + if err != nil { + return nil, err + } + configs = append(configs, hugePagesConfig...) + } + } + + // Extract API Catalog storageClass configuration + if cs.Spec.Features != nil && cs.Spec.Features.APICatalog != nil && cs.Spec.Features.APICatalog.StorageClass != "" { + klog.Info("Extracting API Catalog storageClass configuration") + storageConfig, err := convertStringToSlice(strings.ReplaceAll(constant.APICatalogTemplate, "placeholder", cs.Spec.Features.APICatalog.StorageClass)) + if err != nil { + return nil, err + } + configs = append(configs, storageConfig...) + } + + // Extract labels configuration + if cs.Spec.Labels != nil && len(cs.Spec.Labels) > 0 { + klog.Info("Extracting label configuration") + for key, value := range cs.Spec.Labels { + replacer := strings.NewReplacer("placeholder1", key, "placeholder2", value) + labelConfig, err := convertStringToSlice(replacer.Replace(constant.ServiceLabelTemplate)) + if err != nil { + return nil, err + } + configs = append(configs, labelConfig...) + } + } + + return configs, nil +} + +// extractSizeConfigs handles size profile extraction +func extractSizeConfigs( + cs *apiv3.CommonService, + servicesNs string, +) ([]interface{}, map[string]string, error) { + klog.Info("Extracting size configuration") + + serviceControllerMapping := make(map[string]string) + serviceControllerMapping["profileController"] = "default" + if cs.Spec.ProfileController != "" { + serviceControllerMapping["profileController"] = cs.Spec.ProfileController + } + + var sizeConfigs []interface{} + var err error + + switch cs.Spec.Size { + case "starterset", "starter": + sizeConfigs, serviceControllerMapping, err = extractSizeTemplate(cs, size.StarterSet, serviceControllerMapping, servicesNs) + case "small": + sizeConfigs, serviceControllerMapping, err = extractSizeTemplate(cs, size.Small, serviceControllerMapping, servicesNs) + case "medium": + sizeConfigs, serviceControllerMapping, err = extractSizeTemplate(cs, size.Medium, serviceControllerMapping, servicesNs) + case "large", "production": + sizeConfigs, serviceControllerMapping, err = extractSizeTemplate(cs, size.Large, serviceControllerMapping, servicesNs) + default: + sizeConfigs, serviceControllerMapping = extractCustomSizeConfigs(cs, serviceControllerMapping) + } + + if err != nil { + return nil, nil, err + } + + 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 + } + } + 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) + } + } + serviceMap["resources"] = resources + } + + // Handle management strategy + if service.ManagementStrategy != "" { + serviceMap["managementStrategy"] = service.ManagementStrategy + serviceControllerMapping[service.Name] = service.ManagementStrategy + } + + dest = append(dest, serviceMap) + } + } + + return dest, 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) + } + } + + // Convert size template string to slice + sizes, err := convertStringToSlice(sizeTemplate) + if err != nil { + klog.Errorf("convert size to interface slice: %v", err) + return nil, nil, err + } + + // Merge size template with custom services using the same logic as applySizeTemplate + for i, configSize := range sizes { + if configSize == nil { + continue + } + config := getItemByName(src, configSize.(map[string]interface{})["name"].(string)) + if config == nil { + continue + } + if controller, ok := config.(map[string]interface{})["managementStrategy"]; ok { + serviceControllerMapping[configSize.(map[string]interface{})["name"].(string)] = controller.(string) + } + // Merge spec + if configSize.(map[string]interface{})["spec"] != nil && config.(map[string]interface{})["spec"] != nil { + for cr, mergedSize := range mergeSizeProfile(configSize.(map[string]interface{})["spec"].(map[string]interface{}), config.(map[string]interface{})["spec"].(map[string]interface{})) { + configSize.(map[string]interface{})["spec"].(map[string]interface{})[cr] = mergedSize + } + } + // Merge resources + if configSize.(map[string]interface{})["resources"] != nil && config.(map[string]interface{})["resources"] != nil { + for j, res := range configSize.(map[string]interface{})["resources"].([]interface{}) { + var apiVersion, kind, name, namespace string + if res.(map[string]interface{})["apiVersion"] != nil { + apiVersion = res.(map[string]interface{})["apiVersion"].(string) + } + if res.(map[string]interface{})["kind"] != nil { + kind = res.(map[string]interface{})["kind"].(string) + } + if res.(map[string]interface{})["name"] != nil { + name = res.(map[string]interface{})["name"].(string) + } + if res.(map[string]interface{})["namespace"] != nil { + namespace = res.(map[string]interface{})["namespace"].(string) + } + 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 + } + if namespace == "" { + namespace = opconNs + } + newResource := getItemByGVKNameNamespace(config.(map[string]interface{})["resources"].([]interface{}), opconNs, apiVersion, kind, name, namespace) + if newResource != nil { + configSize.(map[string]interface{})["resources"].([]interface{})[j] = mergeSizeProfile(res.(map[string]interface{}), newResource.(map[string]interface{})) + } + } + } + sizes[i] = configSize + } + + return sizes, serviceControllerMapping, nil +} + +// Made with Bob diff --git a/internal/controller/config_extractor_test.go b/internal/controller/config_extractor_test.go new file mode 100644 index 000000000..1f636e47b --- /dev/null +++ b/internal/controller/config_extractor_test.go @@ -0,0 +1,266 @@ +// +// 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 ( + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + + apiv3 "github.com/IBM/ibm-common-service-operator/v4/api/v3" +) + +const testServicesNs = "ibm-common-services" + +// newCS is a helper that returns a minimal CommonService CR. +func newCS() *apiv3.CommonService { + return &apiv3.CommonService{} +} + +// rawJSON is a helper that wraps a JSON-encoded value in an ExtensionWithMarker. +func rawJSON(t *testing.T, v interface{}) apiv3.ExtensionWithMarker { + t.Helper() + b, err := json.Marshal(v) + require.NoError(t, err) + return apiv3.ExtensionWithMarker{RawExtension: runtime.RawExtension{Raw: b}} +} + +// TestExtractCommonServiceConfigs_EmptyCS verifies that an empty CommonService CR +// produces no configs and a default profileController mapping. +func TestExtractCommonServiceConfigs_EmptyCS(t *testing.T) { + cs := newCS() + configs, mapping, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + assert.Empty(t, configs, "empty CS should produce no configs") + assert.Equal(t, "default", mapping["profileController"]) +} + +// TestExtractCommonServiceConfigs_StorageClass verifies that a storageClass value +// in the CS spec is extracted into the configs slice. +func TestExtractCommonServiceConfigs_StorageClass(t *testing.T) { + cs := newCS() + cs.Spec.StorageClass = "my-storage-class" + + configs, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + require.NotEmpty(t, configs, "storageClass should produce at least one config entry") + + // At least one entry should reference the storage class value + found := false + for _, c := range configs { + b, _ := json.Marshal(c) + if strings.Contains(string(b), "my-storage-class") { + found = true + break + } + } + assert.True(t, found, "extracted configs should contain the storageClass value") +} + +// TestExtractCommonServiceConfigs_RouteHost verifies that a routeHost value is +// extracted into the configs slice. +func TestExtractCommonServiceConfigs_RouteHost(t *testing.T) { + cs := newCS() + cs.Spec.RouteHost = "cp-console.apps.example.com" + + configs, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + require.NotEmpty(t, configs) + + found := false + for _, c := range configs { + b, _ := json.Marshal(c) + if strings.Contains(string(b), "cp-console.apps.example.com") { + found = true + break + } + } + assert.True(t, found, "extracted configs should contain the routeHost value") +} + +// TestExtractCommonServiceConfigs_DefaultAdminUser verifies that a defaultAdminUser +// value is extracted into the configs slice. +func TestExtractCommonServiceConfigs_DefaultAdminUser(t *testing.T) { + cs := newCS() + cs.Spec.DefaultAdminUser = "myadmin" + + configs, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + require.NotEmpty(t, configs) + + found := false + for _, c := range configs { + b, _ := json.Marshal(c) + if strings.Contains(string(b), "myadmin") { + found = true + break + } + } + assert.True(t, found, "extracted configs should contain the defaultAdminUser value") +} + +// TestExtractCommonServiceConfigs_FipsEnabled verifies that fipsEnabled=true is +// extracted into the configs slice. +func TestExtractCommonServiceConfigs_FipsEnabled(t *testing.T) { + cs := newCS() + cs.Spec.FipsEnabled = true + + configs, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + require.NotEmpty(t, configs) + + found := false + for _, c := range configs { + b, _ := json.Marshal(c) + if strings.Contains(string(b), "true") { + found = true + break + } + } + assert.True(t, found, "extracted configs should contain fipsEnabled=true") +} + +// TestExtractCommonServiceConfigs_ProfileController verifies that a custom +// profileController is reflected in the serviceControllerMapping. +func TestExtractCommonServiceConfigs_ProfileController(t *testing.T) { + cs := newCS() + cs.Spec.ProfileController = "turbo" + + _, mapping, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + assert.Equal(t, "turbo", mapping["profileController"]) +} + +// TestExtractCommonServiceConfigs_SizeSmall verifies that the "small" size profile +// produces a non-empty configs slice. +func TestExtractCommonServiceConfigs_SizeSmall(t *testing.T) { + cs := newCS() + cs.Spec.Size = "small" + + configs, mapping, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + assert.NotEmpty(t, configs, "small size profile should produce configs") + assert.Equal(t, "default", mapping["profileController"]) +} + +// TestExtractCommonServiceConfigs_SizeMedium verifies that the "medium" size profile +// produces a non-empty configs slice. +func TestExtractCommonServiceConfigs_SizeMedium(t *testing.T) { + cs := newCS() + cs.Spec.Size = "medium" + + configs, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + assert.NotEmpty(t, configs, "medium size profile should produce configs") +} + +// TestExtractCommonServiceConfigs_SizeLarge verifies that the "large" size profile +// produces a non-empty configs slice. +func TestExtractCommonServiceConfigs_SizeLarge(t *testing.T) { + cs := newCS() + cs.Spec.Size = "large" + + configs, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + assert.NotEmpty(t, configs, "large size profile should produce configs") +} + +// TestExtractCommonServiceConfigs_CustomServices verifies that custom service +// entries in the CS spec are extracted and the managementStrategy is captured +// in the serviceControllerMapping. +func TestExtractCommonServiceConfigs_CustomServices(t *testing.T) { + cs := newCS() + cs.Spec.Services = []apiv3.ServiceConfig{ + { + Name: "ibm-iam-operator", + ManagementStrategy: "turbo", + Spec: map[string]apiv3.ExtensionWithMarker{ + "authentication": rawJSON(t, map[string]interface{}{"replicas": 2}), + }, + }, + } + + configs, mapping, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + require.NotEmpty(t, configs) + + // The management strategy should be captured + assert.Equal(t, "turbo", mapping["ibm-iam-operator"]) + + // The service name should appear in the configs + found := false + for _, c := range configs { + b, _ := json.Marshal(c) + if strings.Contains(string(b), "ibm-iam-operator") { + found = true + break + } + } + assert.True(t, found, "custom service name should appear in extracted configs") +} + +// TestExtractCommonServiceConfigs_HugePagesInvalidFormat verifies that an invalid +// hugepage size format returns an error. +func TestExtractCommonServiceConfigs_HugePagesInvalidFormat(t *testing.T) { + cs := newCS() + cs.Spec.HugePages = &apiv3.HugePages{ + Enable: true, + HugePagesSizes: map[string]string{ + "invalid-size": "1Gi", + }, + } + + _, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid hugepage size format") +} + +// TestExtractCommonServiceConfigs_HugePagesValid verifies that a valid hugepage +// configuration is extracted without error. +func TestExtractCommonServiceConfigs_HugePagesValid(t *testing.T) { + cs := newCS() + cs.Spec.HugePages = &apiv3.HugePages{ + Enable: true, + HugePagesSizes: map[string]string{ + "hugepages-2Mi": "1Gi", + }, + } + + configs, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + require.NotEmpty(t, configs) +} + +// TestExtractCommonServiceConfigs_MultipleFeatures verifies that multiple feature +// flags set simultaneously all produce entries in the configs slice. +func TestExtractCommonServiceConfigs_MultipleFeatures(t *testing.T) { + cs := newCS() + cs.Spec.StorageClass = "fast-storage" + cs.Spec.RouteHost = "cp-console.apps.example.com" + cs.Spec.DefaultAdminUser = "admin2" + + configs, _, err := ExtractCommonServiceConfigs(cs, testServicesNs) + require.NoError(t, err) + + // All three features should contribute entries + assert.GreaterOrEqual(t, len(configs), 3, + "three feature flags should produce at least 3 config entries") +} diff --git a/internal/controller/config_merger.go b/internal/controller/config_merger.go new file mode 100644 index 000000000..1c3e361fd --- /dev/null +++ b/internal/controller/config_merger.go @@ -0,0 +1,190 @@ +// +// 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 ( + "encoding/json" + "fmt" + + utilyaml "github.com/ghodss/yaml" + "k8s.io/klog" + + apiv3 "github.com/IBM/ibm-common-service-operator/v4/api/v3" + "github.com/IBM/ibm-common-service-operator/v4/internal/controller/rules" + odlm "github.com/IBM/operand-deployment-lifecycle-manager/v4/api/v1alpha1" +) + +// MergeBaseAndCSConfigs merges base OperandConfig templates with CommonService configurations +// OperandConfig is created complete on first reconciliation +func MergeBaseAndCSConfigs( + baseConfig string, + csConfigs []interface{}, + serviceControllerMapping map[string]string, + servicesNs string, +) (string, error) { + klog.Info("Merging base OperandConfig with CommonService configurations") + + // Parse base config YAML to OperandConfig object + baseOpcon, err := parseOperandConfig(baseConfig) + if err != nil { + return "", fmt.Errorf("failed to parse base OperandConfig: %v", err) + } + + // Get base services from OperandConfig + baseServices := baseOpcon.Spec.Services + if baseServices == nil { + baseServices = []odlm.ConfigService{} + } + + // Convert to interface slice for merging + baseServicesInterface := make([]interface{}, len(baseServices)) + for i, svc := range baseServices { + svcBytes, err := json.Marshal(svc) + if err != nil { + return "", fmt.Errorf("failed to marshal base service: %v", err) + } + var svcMap map[string]interface{} + if err := json.Unmarshal(svcBytes, &svcMap); err != nil { + return "", fmt.Errorf("failed to unmarshal base service: %v", err) + } + baseServicesInterface[i] = svcMap + } + + // Convert configuration rules to slice + ruleSlice, err := convertStringToSlice(rules.ConfigurationRules) + if err != nil { + return "", fmt.Errorf("failed to convert configuration rules: %v", err) + } + + // Merge CommonService configs into base services using existing merge logic + mergedServices := mergeCSCRs(baseServicesInterface, csConfigs, ruleSlice, serviceControllerMapping, servicesNs) + + // Convert merged services back to OperandConfig format + mergedServicesBytes, err := json.Marshal(mergedServices) + if err != nil { + return "", fmt.Errorf("failed to marshal merged services: %v", err) + } + + var configServices []odlm.ConfigService + if err := json.Unmarshal(mergedServicesBytes, &configServices); err != nil { + return "", fmt.Errorf("failed to unmarshal merged services: %v", err) + } + + // Update OperandConfig with merged services + baseOpcon.Spec.Services = configServices + + // Validate merged configuration + if err := validateMergedConfig(baseOpcon); err != nil { + klog.Warningf("Merged configuration validation warning: %v", err) + } + + // Convert back to YAML string + mergedYAML, err := convertOperandConfigToYAML(baseOpcon) + if err != nil { + return "", fmt.Errorf("failed to convert merged config to YAML: %v", err) + } + + klog.Info("Successfully merged CommonService configurations with base OperandConfig") + return mergedYAML, nil +} + +// parseOperandConfig parses YAML string to OperandConfig object +func parseOperandConfig(yamlStr string) (*odlm.OperandConfig, error) { + jsonBytes, err := utilyaml.YAMLToJSON([]byte(yamlStr)) + if err != nil { + return nil, fmt.Errorf("failed to convert YAML to JSON: %v", err) + } + + var opcon odlm.OperandConfig + if err := json.Unmarshal(jsonBytes, &opcon); err != nil { + return nil, fmt.Errorf("failed to unmarshal OperandConfig: %v", err) + } + + return &opcon, nil +} + +// convertOperandConfigToYAML converts OperandConfig object to YAML string +func convertOperandConfigToYAML(opcon *odlm.OperandConfig) (string, error) { + jsonBytes, err := json.Marshal(opcon) + if err != nil { + return "", fmt.Errorf("failed to marshal OperandConfig: %v", err) + } + + yamlBytes, err := utilyaml.JSONToYAML(jsonBytes) + if err != nil { + return "", fmt.Errorf("failed to convert JSON to YAML: %v", err) + } + + return string(yamlBytes), nil +} + +// validateMergedConfig validates the merged OperandConfig +// Returns error if critical validation fails, warning for non-critical issues +func validateMergedConfig(config *odlm.OperandConfig) error { + if config == nil { + return fmt.Errorf("config is nil") + } + + if config.Spec.Services == nil || len(config.Spec.Services) == 0 { + return fmt.Errorf("no services defined in OperandConfig") + } + + // Validate each service has required fields + for i, svc := range config.Spec.Services { + if svc.Name == "" { + return fmt.Errorf("service at index %d has empty name", i) + } + } + + klog.V(2).Infof("Validated merged OperandConfig with %d services", len(config.Spec.Services)) + return nil +} + +// 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, +) (string, error) { + // Extract CommonService configurations + csConfigs, serviceControllerMapping, err := ExtractCommonServiceConfigs(cs, r.Bootstrap.CSData.ServicesNs) + if err != nil { + return "", fmt.Errorf("failed to extract CommonService configs: %v", err) + } + + // Merge with base templates + mergedConfig, err := MergeBaseAndCSConfigs( + baseConfig, + csConfigs, + serviceControllerMapping, + r.Bootstrap.CSData.ServicesNs, + ) + if err != nil { + return "", fmt.Errorf("failed to merge configurations: %v", err) + } + + return mergedConfig, nil +} + +// CreateMergerFunc creates a ConfigMergerFunc for the given reconciler +// 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) + } +} diff --git a/internal/controller/config_merger_test.go b/internal/controller/config_merger_test.go new file mode 100644 index 000000000..9c70a1850 --- /dev/null +++ b/internal/controller/config_merger_test.go @@ -0,0 +1,375 @@ +// +// 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 ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + odlm "github.com/IBM/operand-deployment-lifecycle-manager/v4/api/v1alpha1" +) + +// minimalBaseOpconYAML is a minimal valid OperandConfig YAML used as base config in tests. +const minimalBaseOpconYAML = ` +apiVersion: operator.ibm.com/v1alpha1 +kind: OperandConfig +metadata: + name: common-service + namespace: ibm-common-services +spec: + services: + - name: ibm-iam-operator + spec: + authentication: + replicas: 1 + - name: ibm-management-ingress-operator + spec: + managementIngress: + replicas: 1 +` + +// TestParseOperandConfig verifies that a valid YAML string is correctly parsed +// into an OperandConfig object. +func TestParseOperandConfig(t *testing.T) { + t.Run("valid YAML parses successfully", func(t *testing.T) { + opcon, err := parseOperandConfig(minimalBaseOpconYAML) + require.NoError(t, err) + require.NotNil(t, opcon) + assert.Equal(t, "common-service", opcon.Name) + assert.Equal(t, "ibm-common-services", opcon.Namespace) + assert.Len(t, opcon.Spec.Services, 2) + assert.Equal(t, "ibm-iam-operator", opcon.Spec.Services[0].Name) + assert.Equal(t, "ibm-management-ingress-operator", opcon.Spec.Services[1].Name) + }) + + t.Run("invalid YAML returns error", func(t *testing.T) { + _, err := parseOperandConfig("{ invalid yaml: [") + assert.Error(t, err) + }) + + t.Run("empty string returns empty OperandConfig without error", func(t *testing.T) { + // The YAML library treats an empty string as a valid null document, + // so parseOperandConfig returns an empty OperandConfig rather than an error. + opcon, err := parseOperandConfig("") + assert.NoError(t, err) + assert.NotNil(t, opcon) + assert.Empty(t, opcon.Spec.Services) + }) +} + +// TestValidateMergedConfig verifies the validation logic for merged OperandConfig objects. +func TestValidateMergedConfig(t *testing.T) { + t.Run("nil config returns error", func(t *testing.T) { + err := validateMergedConfig(nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "config is nil") + }) + + t.Run("config with no services returns error", func(t *testing.T) { + config := &odlm.OperandConfig{} + err := validateMergedConfig(config) + assert.Error(t, err) + assert.Contains(t, err.Error(), "no services defined") + }) + + t.Run("config with service missing name returns error", func(t *testing.T) { + config := &odlm.OperandConfig{ + Spec: odlm.OperandConfigSpec{ + Services: []odlm.ConfigService{ + {Name: ""}, + }, + }, + } + err := validateMergedConfig(config) + assert.Error(t, err) + assert.Contains(t, err.Error(), "empty name") + }) + + t.Run("valid config passes validation", func(t *testing.T) { + config := &odlm.OperandConfig{ + Spec: odlm.OperandConfigSpec{ + Services: []odlm.ConfigService{ + {Name: "ibm-iam-operator"}, + {Name: "ibm-management-ingress-operator"}, + }, + }, + } + err := validateMergedConfig(config) + assert.NoError(t, err) + }) +} + +// TestMergeBaseAndCSConfigs_NoCSConfigs verifies that when no CommonService configs +// are provided, the base OperandConfig is returned unchanged. +func TestMergeBaseAndCSConfigs_NoCSConfigs(t *testing.T) { + result, err := MergeBaseAndCSConfigs( + minimalBaseOpconYAML, + nil, + map[string]string{"profileController": "default"}, + "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) + assert.Equal(t, "ibm-iam-operator", opcon.Spec.Services[0].Name) +} + +// TestMergeBaseAndCSConfigs_WithStorageClass verifies that a storageClass config +// from a CommonService CR is merged into the base OperandConfig. +func TestMergeBaseAndCSConfigs_WithStorageClass(t *testing.T) { + // Build a csConfig slice that mimics what ExtractCommonServiceConfigs produces + // for a storageClass setting — a list of service entries with spec overrides. + storageClassConfig := []interface{}{ + map[string]interface{}{ + "name": "ibm-iam-operator", + "spec": map[string]interface{}{ + "authentication": map[string]interface{}{ + "storageClass": "my-storage-class", + }, + }, + }, + } + + result, err := MergeBaseAndCSConfigs( + minimalBaseOpconYAML, + storageClassConfig, + map[string]string{"profileController": "default"}, + "ibm-common-services", + ) + require.NoError(t, err) + assert.NotEmpty(t, result) + + // The result should still be valid YAML parseable as OperandConfig + opcon, err := parseOperandConfig(result) + require.NoError(t, err) + assert.NotEmpty(t, opcon.Spec.Services) +} + +// TestMergeBaseAndCSConfigs_InvalidBaseConfig verifies that an invalid base config +// YAML returns an error rather than silently producing bad output. +func TestMergeBaseAndCSConfigs_InvalidBaseConfig(t *testing.T) { + _, err := MergeBaseAndCSConfigs( + "{ not valid yaml: [", + nil, + map[string]string{"profileController": "default"}, + "ibm-common-services", + ) + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to parse base OperandConfig") +} + +// TestMergeBaseAndCSConfigs_EmptyBaseServices verifies that a base config with no +// services is handled gracefully and CS configs can still be merged in. +func TestMergeBaseAndCSConfigs_EmptyBaseServices(t *testing.T) { + emptyServicesYAML := ` +apiVersion: operator.ibm.com/v1alpha1 +kind: OperandConfig +metadata: + name: common-service + namespace: ibm-common-services +spec: + services: [] +` + csConfigs := []interface{}{ + map[string]interface{}{ + "name": "ibm-iam-operator", + "spec": map[string]interface{}{ + "authentication": map[string]interface{}{ + "replicas": float64(2), + }, + }, + }, + } + + result, err := MergeBaseAndCSConfigs( + emptyServicesYAML, + csConfigs, + map[string]string{"profileController": "default"}, + "ibm-common-services", + ) + require.NoError(t, err) + assert.NotEmpty(t, result) + + // The CS config should have been merged in even though the base had no services + assert.True(t, strings.Contains(result, "ibm-iam-operator"), + "CS config service should appear in result even when base services list was empty") +} + +// TestMergeBaseAndCSConfigs_PreservesUnchangedServices verifies that services not +// referenced in csConfigs are preserved verbatim in the merged output. +func TestMergeBaseAndCSConfigs_PreservesUnchangedServices(t *testing.T) { + // Only provide config for ibm-iam-operator; ibm-management-ingress-operator should be untouched. + csConfigs := []interface{}{ + map[string]interface{}{ + "name": "ibm-iam-operator", + "spec": map[string]interface{}{ + "authentication": map[string]interface{}{ + "replicas": float64(3), + }, + }, + }, + } + + result, err := MergeBaseAndCSConfigs( + minimalBaseOpconYAML, + csConfigs, + map[string]string{"profileController": "default"}, + "ibm-common-services", + ) + require.NoError(t, err) + + opcon, err := parseOperandConfig(result) + require.NoError(t, err) + + // Both services must still be present + names := make([]string, 0, len(opcon.Spec.Services)) + for _, svc := range opcon.Spec.Services { + names = append(names, svc.Name) + } + assert.Contains(t, names, "ibm-iam-operator") + assert.Contains(t, names, "ibm-management-ingress-operator") +} + +// TestMergeBaseAndCSConfigs_OutputIsValidYAML verifies that the merged result is +// valid YAML that round-trips through JSON without data loss. +func TestMergeBaseAndCSConfigs_OutputIsValidYAML(t *testing.T) { + result, err := MergeBaseAndCSConfigs( + minimalBaseOpconYAML, + nil, + map[string]string{"profileController": "default"}, + "ibm-common-services", + ) + require.NoError(t, err) + assert.True(t, strings.Contains(result, "ibm-iam-operator"), "output YAML should contain service name") + + // Verify it round-trips through parseOperandConfig → convertOperandConfigToYAML + opcon, err := parseOperandConfig(result) + require.NoError(t, err) + + roundTripped, err := convertOperandConfigToYAML(opcon) + require.NoError(t, err) + assert.NotEmpty(t, roundTripped) +} + +// TestConvertOperandConfigToYAML verifies that an OperandConfig object is correctly +// serialised to a YAML string. +func TestConvertOperandConfigToYAML(t *testing.T) { + t.Run("valid config converts to YAML", func(t *testing.T) { + config := &odlm.OperandConfig{ + Spec: odlm.OperandConfigSpec{ + Services: []odlm.ConfigService{ + {Name: "ibm-iam-operator"}, + {Name: "ibm-management-ingress-operator"}, + }, + }, + } + + yaml, err := convertOperandConfigToYAML(config) + require.NoError(t, err) + assert.Contains(t, yaml, "ibm-iam-operator") + assert.Contains(t, yaml, "ibm-management-ingress-operator") + }) + + t.Run("empty config converts without error", func(t *testing.T) { + config := &odlm.OperandConfig{} + yaml, err := convertOperandConfigToYAML(config) + require.NoError(t, err) + assert.NotEmpty(t, yaml) + }) + + t.Run("round-trip parse then convert preserves service names", func(t *testing.T) { + opcon, err := parseOperandConfig(minimalBaseOpconYAML) + require.NoError(t, err) + + yaml, err := convertOperandConfigToYAML(opcon) + require.NoError(t, err) + assert.Contains(t, yaml, "ibm-iam-operator") + assert.Contains(t, yaml, "ibm-management-ingress-operator") + }) +} + +// TestMergeBaseAndCSConfigs_SingleStageCompleteness is the key regression test for +// the Single-Stage Creation feature. It verifies that after one call to +// MergeBaseAndCSConfigs the resulting OperandConfig already contains the values +// from the CommonService CR — i.e. there is no intermediate incomplete state. +func TestMergeBaseAndCSConfigs_SingleStageCompleteness(t *testing.T) { + baseYAML := ` +apiVersion: operator.ibm.com/v1alpha1 +kind: OperandConfig +metadata: + name: common-service + namespace: ibm-common-services +spec: + services: + - name: ibm-iam-operator + spec: + authentication: + replicas: 1 +` + + // Simulate what the CS CR contributes: a higher replica count. + csConfigs := []interface{}{ + map[string]interface{}{ + "name": "ibm-iam-operator", + "spec": map[string]interface{}{ + "authentication": map[string]interface{}{ + "replicas": float64(3), + }, + }, + }, + } + + // Single call — no intermediate state. + result, err := MergeBaseAndCSConfigs( + baseYAML, + csConfigs, + map[string]string{"profileController": "default"}, + "ibm-common-services", + ) + require.NoError(t, err, "single-stage merge must not return an error") + assert.NotEmpty(t, result, "merged result must not be empty") + + // The result must be a complete, parseable OperandConfig. + opcon, err := parseOperandConfig(result) + require.NoError(t, err, "merged result must be valid OperandConfig YAML") + require.NotEmpty(t, opcon.Spec.Services, "merged OperandConfig must contain services") + + // Verify the service is present and the CS replica value was applied in the + // single call — this is the core single-stage completeness assertion. + var iamSvc *odlm.ConfigService + for i := range opcon.Spec.Services { + if opcon.Spec.Services[i].Name == "ibm-iam-operator" { + iamSvc = &opcon.Spec.Services[i] + break + } + } + require.NotNil(t, iamSvc, "ibm-iam-operator must be present in the single-stage merged OperandConfig") + + // The merged YAML must contain the CS-supplied replica value (3), not the + // base value (1), proving CS values were applied in the same call. + assert.True(t, strings.Contains(result, "3"), + "merged result must contain the CS-supplied replica count (3), not the base value (1)") + 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") +} diff --git a/internal/controller/no_olm_commonservice_controller.go b/internal/controller/no_olm_commonservice_controller.go index 8da96e62a..cfdee3fae 100644 --- a/internal/controller/no_olm_commonservice_controller.go +++ b/internal/controller/no_olm_commonservice_controller.go @@ -176,7 +176,7 @@ func (r *CommonServiceReconciler) ReconcileNoOLMMasterCR(ctx context.Context, in } klog.Info("Installing/Updating OperandConfig") - if err := r.Bootstrap.InstallOrUpdateOpcon(forceUpdateODLMCRs); err != nil { + if err := r.Bootstrap.InstallOrUpdateOpcon(forceUpdateODLMCRs, instance); err != nil { klog.Errorf("Fail to Installing/Updating OperandConfig: %v", err) return ctrl.Result{}, err } @@ -184,58 +184,11 @@ func (r *CommonServiceReconciler) ReconcileNoOLMMasterCR(ctx context.Context, in klog.Error("ODLM CRD not ready, waiting for it to be ready") } - // Init common service bootstrap resource - // Including namespace-scope configmap - // Deploy OperandConfig and OperandRegistry - // if statusErr = r.Bootstrap.InitResources(instance, forceUpdateODLMCRs); statusErr != nil { - // if statusErr := r.updatePhase(ctx, instance, CRFailed); statusErr != nil { - // klog.Error(statusErr) - // } - // klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, statusErr) - // return ctrl.Result{}, statusErr - // } - - // Generate Issuer and Certificate CR - // if statusErr = r.Bootstrap.DeployCertManagerCR(); statusErr != nil { - // klog.Errorf("Failed to deploy cert manager CRs: %v", statusErr) - // if statusErr = r.updatePhase(ctx, instance, CRFailed); statusErr != nil { - // klog.Error(statusErr) - // } - // klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, statusErr) - // return ctrl.Result{}, statusErr - // } - - // Apply new configs to CommonService CR - cs := util.NewUnstructured("operator.ibm.com", "CommonService", "v3") - if statusErr = r.Bootstrap.Client.Get(ctx, types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, cs); statusErr != nil { - klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, statusErr) - return ctrl.Result{}, statusErr - } - // Set "Pengding" condition and "Updating" for phase when config CS CR - instance.SetPendingCondition(constant.MasterCR, apiv3.ConditionTypeReconciling, corev1.ConditionTrue, apiv3.ConditionReasonConfig, apiv3.ConditionMessageConfig) - instance.Status.Phase = apiv3.CRUpdating - newConfigs, serviceControllerMapping, statusErr := r.getNewConfigs(cs) - if statusErr != nil { - klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, statusErr) - instance.SetErrorCondition(constant.MasterCR, apiv3.ConditionTypeError, corev1.ConditionTrue, apiv3.ConditionReasonError, statusErr.Error()) - instance.Status.Phase = apiv3.CRFailed - } - - if statusErr = r.Client.Status().Update(ctx, instance); statusErr != nil { - klog.Errorf("Fail to update %s/%s: %v", instance.Namespace, instance.Name, statusErr) - return ctrl.Result{}, statusErr - } + // 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.updateOperandConfig(ctx, newConfigs, serviceControllerMapping); statusErr != nil { - if statusErr := r.updatePhase(ctx, instance, apiv3.CRFailed); statusErr != nil { - klog.Error(statusErr) - } - klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, statusErr) - return ctrl.Result{}, statusErr - } else if isEqual { - r.Recorder.Event(instance, corev1.EventTypeNormal, "Noeffect", fmt.Sprintf("No update, resource sizings in the OperandConfig %s/%s are larger than the profile from CommonService CR %s/%s", r.Bootstrap.CSData.OperatorNs, "common-service", instance.Namespace, instance.Name)) - } if isEqual, statusErr = r.updateOperatorConfig(ctx, instance.Spec.OperatorConfigs); statusErr != nil { if statusErr := r.updatePhase(ctx, instance, apiv3.CRFailed); statusErr != nil { @@ -320,11 +273,6 @@ func (r *CommonServiceReconciler) ReconcileNoOLMGeneralCR(ctx context.Context, i return ctrl.Result{}, err } - cs := util.NewUnstructured("operator.ibm.com", "CommonService", "v3") - if err := r.Bootstrap.Client.Get(ctx, types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, cs); err != nil { - klog.Errorf("Fail to reconcile %s/%s: %v", instance.Namespace, instance.Name, err) - return ctrl.Result{}, err - } // Generate Issuer and Certificate CR if err := r.Bootstrap.DeployCertManagerCR(); err != nil { klog.Errorf("Failed to deploy cert manager CRs: %v", err) @@ -335,7 +283,8 @@ func (r *CommonServiceReconciler) ReconcileNoOLMGeneralCR(ctx context.Context, i return ctrl.Result{}, err } - newConfigs, serviceControllerMapping, err := r.getNewConfigs(cs) + // Extract configurations using new extractor for subsequent updates + newConfigs, serviceControllerMapping, err := ExtractCommonServiceConfigs(instance, r.Bootstrap.CSData.ServicesNs) if err != nil { if err := r.updatePhase(ctx, instance, apiv3.CRFailed); err != nil { klog.Error(err) @@ -344,6 +293,7 @@ func (r *CommonServiceReconciler) ReconcileNoOLMGeneralCR(ctx context.Context, i return ctrl.Result{}, err } + // Update OperandConfig (single update for subsequent reconciliations) isEqual, err := r.updateOperandConfig(ctx, newConfigs, serviceControllerMapping) if err != nil { if err := r.updatePhase(ctx, instance, apiv3.CRFailed); err != nil {