From 118775119cab5130896857f1bcb534432a05f2bb Mon Sep 17 00:00:00 2001 From: mumesan Date: Mon, 16 Jun 2025 17:33:34 +0100 Subject: [PATCH] Fix customServiceConfig bug --- controllers/ironic_controller.go | 60 ++++++++++- controllers/ironicapi_controller.go | 99 +++++++++-------- controllers/ironicconductor_controller.go | 100 ++++++++++-------- pkg/ironicapi/volumes.go | 7 +- pkg/ironicconductor/volumes.go | 5 +- templates/common/bin/common.sh | 6 +- templates/ironic/bin/dbsync.sh | 7 +- templates/ironic/config/db-sync-config.json | 4 +- .../ironicapi/config/ironic-api-config.json | 10 +- .../config/ironic-conductor-config.json | 10 +- 10 files changed, 204 insertions(+), 104 deletions(-) diff --git a/controllers/ironic_controller.go b/controllers/ironic_controller.go index 19fdf8eb..aae8d992 100644 --- a/controllers/ironic_controller.go +++ b/controllers/ironic_controller.go @@ -49,6 +49,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" batchv1 "k8s.io/api/batch/v1" @@ -218,6 +219,52 @@ func (r *IronicReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res // SetupWithManager sets up the controller with the Manager. func (r *IronicReconciler) SetupWithManager(mgr ctrl.Manager) error { + // watch for configmap where the CM owner label AND the CR.Spec.ManagingCrName label matches + configMapFn := func(ctx context.Context, o client.Object) []reconcile.Request { + Log := r.GetLogger(ctx) + + result := []reconcile.Request{} + + // get all API CRs + apis := &ironicv1.IronicInspectorList{} + listOpts := []client.ListOption{ + client.InNamespace(o.GetNamespace()), + } + if err := r.Client.List( + ctx, + apis, + listOpts...); err != nil { + + Log.Error(err, "Unable to retrieve API CRs %v") + return nil + } + + label := o.GetLabels() + // TODO: Just trying to verify that the CM is owned by this CR's managing CR + if l, ok := label[labels.GetOwnerNameLabelSelector( + labels.GetGroupLabel(ironic.ServiceName))]; ok { + for _, cr := range apis.Items { + // return reconcil event for the CR where the CM owner label + // AND the parentIronicName matches + if l == ironicv1.GetOwningIronicName(&cr) { + // return namespace and Name of CR + name := client.ObjectKey{ + Namespace: o.GetNamespace(), + Name: cr.Name, + } + Log.Info(fmt.Sprintf( + "ConfigMap object %s and CR %s marked with label: %s", + o.GetName(), cr.Name, l)) + result = append( + result, reconcile.Request{NamespacedName: name}) + } + } + } + if len(result) > 0 { + return result + } + return nil + } return ctrl.NewControllerManagedBy(mgr). For(&ironicv1.Ironic{}). Owns(&ironicv1.IronicConductor{}). @@ -228,10 +275,13 @@ func (r *IronicReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&mariadbv1.MariaDBAccount{}). Owns(&batchv1.Job{}). Owns(&corev1.Secret{}). - Owns(&corev1.ConfigMap{}). Owns(&corev1.ServiceAccount{}). Owns(&rbacv1.Role{}). Owns(&rbacv1.RoleBinding{}). + Watches( + &corev1.ConfigMap{}, + handler.EnqueueRequestsFromMapFunc(configMapFn), + builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})). Watches(&keystonev1.KeystoneAPI{}, handler.EnqueueRequestsFromMapFunc(r.findObjectForSrc), builder.WithPredicates(keystonev1.KeystoneAPIStatusChangedPredicate)). @@ -539,6 +589,10 @@ func (r *IronicReconciler) reconcileNormal(ctx context.Context, instance *ironic } } + if instance.Spec.IronicAPI.CustomServiceConfig == "" { + instance.Spec.IronicAPI.CustomServiceConfig = instance.Spec.IronicSpecCore.CustomServiceConfig + } + // deploy ironic-api ironicAPI, op, err := r.apiDeploymentCreateOrUpdate(instance, &keystoneEndpoints) if err != nil { @@ -935,8 +989,8 @@ func (r *IronicReconciler) generateServiceConfigMaps( // all other files get placed into /etc/ironic to allow overwrite of e.g. policy.json // TODO: make sure custom.conf can not be overwritten customData := map[string]string{ - common.CustomServiceConfigFileName: instance.Spec.CustomServiceConfig, - "my.cnf": db.GetDatabaseClientConfig(tlsCfg), //(mschuppert) for now just get the default my.cnf + "01-ironic-custom.conf": instance.Spec.CustomServiceConfig, + "my.cnf": db.GetDatabaseClientConfig(tlsCfg), //(mschuppert) for now just get the default my.cnf } for key, data := range instance.Spec.DefaultConfigOverwrite { diff --git a/controllers/ironicapi_controller.go b/controllers/ironicapi_controller.go index dd3fb346..f22ad676 100644 --- a/controllers/ironicapi_controller.go +++ b/controllers/ironicapi_controller.go @@ -224,43 +224,6 @@ func (r *IronicAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // SetupWithManager sets up the controller with the Manager. func (r *IronicAPIReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error { - // watch for configmap where the CM owner label AND the CR.Spec.ManagingCrName label matches - configMapFn := func(ctx context.Context, o client.Object) []reconcile.Request { - Log := r.GetLogger(ctx) - - result := []reconcile.Request{} - - // get all API CRs - apis := &ironicv1.IronicAPIList{} - listOpts := []client.ListOption{ - client.InNamespace(o.GetNamespace()), - } - if err := r.Client.List(ctx, apis, listOpts...); err != nil { - Log.Error(err, "Unable to retrieve API CRs %v") - return nil - } - - label := o.GetLabels() - // TODO: Just trying to verify that the CM is owned by this CR's managing CR - if l, ok := label[labels.GetOwnerNameLabelSelector(labels.GetGroupLabel(ironic.ServiceName))]; ok { - for _, cr := range apis.Items { - // return reconcil event for the CR where the CM owner label AND the parentIronicName matches - if l == ironicv1.GetOwningIronicName(&cr) { - // return namespace and Name of CR - name := client.ObjectKey{ - Namespace: o.GetNamespace(), - Name: cr.Name, - } - Log.Info(fmt.Sprintf("ConfigMap object %s and CR %s marked with label: %s", o.GetName(), cr.Name, l)) - result = append(result, reconcile.Request{NamespacedName: name}) - } - } - } - if len(result) > 0 { - return result - } - return nil - } // index passwordSecretField if err := mgr.GetFieldIndexer().IndexField(ctx, &ironicv1.IronicAPI{}, passwordSecretField, func(rawObj client.Object) []string { @@ -332,9 +295,6 @@ func (r *IronicAPIReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Man Owns(&corev1.ServiceAccount{}). Owns(&rbacv1.Role{}). Owns(&rbacv1.RoleBinding{}). - // watch the config CMs we don't own - Watches(&corev1.ConfigMap{}, - handler.EnqueueRequestsFromMapFunc(configMapFn)). Watches( &corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.findObjectsForSrc), @@ -351,8 +311,36 @@ func (r *IronicAPIReconciler) findObjectsForSrc(ctx context.Context, src client. l := log.FromContext(ctx).WithName("Controllers").WithName("IronicAPI") + crList := &ironicv1.IronicAPIList{} + listOpts := []client.ListOption{client.InNamespace(src.GetNamespace())} + + if err := r.List(ctx, crList, listOpts...); err != nil { + l.Error(err, "Unable to retrieve Conductor CRs %v") + } else { + label := src.GetLabels() + // TODO: Just trying to verify that the Secret is owned by this CR's managing CR + if lbl, ok := label[labels.GetOwnerNameLabelSelector(labels.GetGroupLabel(ironic.ServiceName))]; ok { + for _, item := range crList.Items { + // return reconcile event for the CR where the Secret owner label AND the parentIronicName matches + if lbl == ironicv1.GetOwningIronicName(&item) { + // return Namespace and Name of CR + l.Info(fmt.Sprintf("input source %s changed, reconcile: %s - %s", src.GetName(), item.GetName(), item.GetNamespace())) + + requests = append(requests, + reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: item.GetName(), + Namespace: item.GetNamespace(), + }, + }, + ) + + } + } + } + } + for _, field := range ironicAPIWatchFields { - crList := &ironicv1.IronicAPIList{} listOps := &client.ListOptions{ FieldSelector: fields.OneTermEqualSelector(field, src.GetName()), Namespace: src.GetNamespace(), @@ -662,6 +650,31 @@ func (r *IronicAPIReconciler) reconcileNormal(ctx context.Context, instance *iro // // check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map // + + // Get secrets by Namespace and Label that we need to hash + labelSelectorMap := map[string]string{} + lbl := labels.GetOwnerNameLabelSelector(labels.GetGroupLabel(ironic.ServiceName)) + labelSelectorMap[lbl] = ironicv1.GetOwningIronicName(instance) + secrets, err := secret.GetSecrets(ctx, helper, instance.Namespace, labelSelectorMap) + if err != nil { + Log.Info(fmt.Sprintf("No secrets with label %s found", lbl)) + } else { + for _, s := range secrets.Items { + hash, err := secret.Hash(&s) + if err != nil { + Log.Info(fmt.Sprintf("Not able to has secret %s found", s.Name)) + instance.Status.Conditions.Set(condition.FalseCondition( + condition.InputReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.InputReadyErrorMessage, + err.Error())) + return ctrl.Result{}, err + } + configMapVars[s.Name] = env.SetValue(hash) + } + } + ospSecret, hash, err := secret.GetSecret(ctx, helper, instance.Spec.Secret, instance.Namespace) if err != nil { if k8s_errors.IsNotFound(err) { @@ -1034,8 +1047,8 @@ func (r *IronicAPIReconciler) generateServiceConfigMaps( // custom.conf is going to be merged into /etc/ironic/ironic.conf // TODO: make sure custom.conf can not be overwritten customData := map[string]string{ - common.CustomServiceConfigFileName: instance.Spec.CustomServiceConfig, - "my.cnf": db.GetDatabaseClientConfig(tlsCfg), //(mschuppert) for now just get the default my.cnf + "02-api-custom.conf": instance.Spec.CustomServiceConfig, + "my.cnf": db.GetDatabaseClientConfig(tlsCfg), //(mschuppert) for now just get the default my.cnf } for key, data := range instance.Spec.DefaultConfigOverwrite { diff --git a/controllers/ironicconductor_controller.go b/controllers/ironicconductor_controller.go index f526908c..621a880f 100644 --- a/controllers/ironicconductor_controller.go +++ b/controllers/ironicconductor_controller.go @@ -207,43 +207,6 @@ func (r *IronicConductorReconciler) Reconcile(ctx context.Context, req ctrl.Requ // SetupWithManager sets up the controller with the Manager. func (r *IronicConductorReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error { - // watch for configmap where the CM owner label AND the CR.Spec.ManagingCrName label matches - configMapFn := func(ctx context.Context, o client.Object) []reconcile.Request { - result := []reconcile.Request{} - Log := r.GetLogger(ctx) - - // get all API CRs - apis := &ironicv1.IronicConductorList{} - listOpts := []client.ListOption{ - client.InNamespace(o.GetNamespace()), - } - if err := r.Client.List(ctx, apis, listOpts...); err != nil { - Log.Error(err, "Unable to retrieve API CRs %v") - return nil - } - - label := o.GetLabels() - // TODO: Just trying to verify that the CM is owned by this CR's managing CR - if l, ok := label[labels.GetOwnerNameLabelSelector(labels.GetGroupLabel(ironic.ServiceName))]; ok { - for _, cr := range apis.Items { - // return reconcil event for the CR where the CM owner label AND the parentIronicName matches - if l == ironicv1.GetOwningIronicName(&cr) { - // return namespace and Name of CR - name := client.ObjectKey{ - Namespace: o.GetNamespace(), - Name: cr.Name, - } - Log.Info(fmt.Sprintf("ConfigMap object %s and CR %s marked with label: %s", o.GetName(), cr.Name, l)) - result = append(result, reconcile.Request{NamespacedName: name}) - } - } - } - if len(result) > 0 { - return result - } - return nil - } - // index passwordSecretField if err := mgr.GetFieldIndexer().IndexField(ctx, &ironicv1.IronicConductor{}, passwordSecretField, func(rawObj client.Object) []string { // Extract the secret name from the spec, if one is provided @@ -290,9 +253,6 @@ func (r *IronicConductorReconciler) SetupWithManager(ctx context.Context, mgr ct Owns(&corev1.ServiceAccount{}). Owns(&rbacv1.Role{}). Owns(&rbacv1.RoleBinding{}). - // watch the config CMs we don't own - Watches(&corev1.ConfigMap{}, - handler.EnqueueRequestsFromMapFunc(configMapFn)). Watches( &corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.findObjectsForSrc), @@ -309,8 +269,38 @@ func (r *IronicConductorReconciler) findObjectsForSrc(ctx context.Context, src c l := log.FromContext(ctx).WithName("Controllers").WithName("IronicConductor") + crList := &ironicv1.IronicConductorList{} + namespace := src.GetNamespace() + listOpts := []client.ListOption{client.InNamespace(namespace)} + + if err := r.List(ctx, crList, listOpts...); err != nil { + l.Error(err, "Unable to retrieve Conductor CRs %v") + } else { + label := src.GetLabels() + // TODO: Just trying to verify that the Secret is owned by this CR's managing CR + if lbl, ok := label[labels.GetOwnerNameLabelSelector(labels.GetGroupLabel(ironic.ServiceName))]; ok { + for _, item := range crList.Items { + // return reconcil event for the CR where the Secret owner label AND the parentIronicName matches + if lbl == ironicv1.GetOwningIronicName(&item) { + // return Namespace and Name of CR + l.Info(fmt.Sprintf("input source %s changed, reconcile: %s - %s", src.GetName(), item.GetName(), item.GetNamespace())) + + requests = append( + requests, + reconcile.Request{ + NamespacedName: k8s_types.NamespacedName{ + Name: item.GetName(), + Namespace: item.GetNamespace(), + }, + }, + ) + + } + } + } + } + for _, field := range ironicConductorWatchFields { - crList := &ironicv1.IronicConductorList{} listOps := &client.ListOptions{ FieldSelector: fields.OneTermEqualSelector(field, src.GetName()), Namespace: src.GetNamespace(), @@ -500,6 +490,30 @@ func (r *IronicConductorReconciler) reconcileNormal(ctx context.Context, instanc // // check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map // + + // Get secrets by Namespace and Label that we need to hash ... + labelSelectorMap := map[string]string{} + lbl := labels.GetOwnerNameLabelSelector(labels.GetGroupLabel(ironic.ServiceName)) + labelSelectorMap[lbl] = ironicv1.GetOwningIronicName(instance) + secrets, err := secret.GetSecrets(ctx, helper, instance.Namespace, labelSelectorMap) + if err != nil { + Log.Info(fmt.Sprintf("No secrets with label %s found", lbl)) + } else { + for _, s := range secrets.Items { + hash, err := secret.Hash(&s) + if err != nil { + Log.Info(fmt.Sprintf("Not able to has secret %s not found", s.Name)) + instance.Status.Conditions.Set(condition.FalseCondition( + condition.InputReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.InputReadyErrorMessage, + err.Error())) + return ctrl.Result{}, err + } + configMapVars[s.Name] = env.SetValue(hash) + } + } ospSecret, hash, err := secret.GetSecret(ctx, helper, instance.Spec.Secret, instance.Namespace) if err != nil { if k8s_errors.IsNotFound(err) { @@ -852,8 +866,8 @@ func (r *IronicConductorReconciler) generateServiceConfigMaps( // custom.conf is going to be merged into /etc/ironic/ironic.conf // TODO: make sure custom.conf can not be overwritten customData := map[string]string{ - common.CustomServiceConfigFileName: instance.Spec.CustomServiceConfig, - "my.cnf": db.GetDatabaseClientConfig(tlsCfg), //(mschuppert) for now just get the default my.cnf + "02-conductor-custom.conf": instance.Spec.CustomServiceConfig, + "my.cnf": db.GetDatabaseClientConfig(tlsCfg), //(mschuppert) for now just get the default my.cnf } for key, data := range instance.Spec.DefaultConfigOverwrite { diff --git a/pkg/ironicapi/volumes.go b/pkg/ironicapi/volumes.go index bc4d8e52..8b7a4a28 100644 --- a/pkg/ironicapi/volumes.go +++ b/pkg/ironicapi/volumes.go @@ -1,6 +1,9 @@ package ironicapi import ( + "fmt" + "strings" + "github.com/openstack-k8s-operators/ironic-operator/pkg/ironic" corev1 "k8s.io/api/core/v1" ) @@ -8,14 +11,14 @@ import ( // GetVolumes - func GetVolumes(name string) []corev1.Volume { var config0640AccessMode int32 = 0640 - + parentName := strings.Replace(name, "-api", "", 1) apiVolumes := []corev1.Volume{ { Name: "config-data-custom", VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ DefaultMode: &config0640AccessMode, - SecretName: name + "-config-data", + SecretName: fmt.Sprintf("%s-config-data", parentName), }, }, }, diff --git a/pkg/ironicconductor/volumes.go b/pkg/ironicconductor/volumes.go index c6d7fd2b..ea74b099 100644 --- a/pkg/ironicconductor/volumes.go +++ b/pkg/ironicconductor/volumes.go @@ -2,6 +2,7 @@ package ironicconductor import ( "fmt" + "strings" ironicv1 "github.com/openstack-k8s-operators/ironic-operator/api/v1beta1" "github.com/openstack-k8s-operators/ironic-operator/pkg/ironic" @@ -11,13 +12,15 @@ import ( // GetVolumes - func GetVolumes(instance *ironicv1.IronicConductor) []corev1.Volume { var config0640AccessMode int32 = 0640 + parentName := strings.Replace(instance.Name, "-conductor", "", 1) + conductorVolumes := []corev1.Volume{ { Name: "config-data-custom", VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ DefaultMode: &config0640AccessMode, - SecretName: fmt.Sprintf("%s-config-data", instance.Name), + SecretName: fmt.Sprintf("%s-config-data", parentName), }, }, }, diff --git a/templates/common/bin/common.sh b/templates/common/bin/common.sh index 075db52a..2b929830 100755 --- a/templates/common/bin/common.sh +++ b/templates/common/bin/common.sh @@ -44,7 +44,7 @@ function common_ironic_config { export CUSTOMCONF=${CustomConf:-""} SVC_CFG=/etc/ironic/ironic.conf - SVC_CFG_MERGED=/var/lib/config-data/merged/ironic.conf + SVC_CFG_MERGED=/var/lib/config-data/merged/01-ironic-custom.conf # Copy default service config from container image as base cp -a ${SVC_CFG} ${SVC_CFG_MERGED} @@ -65,8 +65,8 @@ function common_ironic_config { # Can we just put custom.conf in something like /etc/ironic/ironic.conf.d/custom.conf # and have it automatically detected, or would we have to somehow change the call # to the ironic binary to tell it to use that custom conf dir? - echo merging /var/lib/config-data/default/custom.conf into ${SVC_CFG_MERGED} - crudini --merge ${SVC_CFG_MERGED} < /var/lib/config-data/default/custom.conf + echo merging /var/lib/config-data/default/01-ironic-custom.conf into ${SVC_CFG_MERGED} + crudini --merge ${SVC_CFG_MERGED} < /var/lib/config-data/default/01-ironic-custom.conf # TODO: a cleaner way to handle this? # There might be service-specific extra custom conf that needs to be merged diff --git a/templates/ironic/bin/dbsync.sh b/templates/ironic/bin/dbsync.sh index 33911b5e..4aec0ff7 100755 --- a/templates/ironic/bin/dbsync.sh +++ b/templates/ironic/bin/dbsync.sh @@ -33,13 +33,14 @@ if [ $ret_val -gt 1 ] ; then # returned as greater than 1 which means there is a major upgrade # stopping issue which needs to be addressed. echo "WARNING: Status check failed, we're going to attempt to apply the schema update and then re-evaluate." - ironic-dbsync --config-file=/etc/ironic/ironic.conf upgrade + ironic-dbsync --config-file /etc/ironic/ironic.conf --config-dir /etc/ironic/ironic.conf.d/ upgrade ironic-status upgrade check && ret_val=$? || ret_val=$? if [ $ret_val -gt 1 ] ; then echo $LINENO "Ironic DB Status check failed, returned: $ret_val" exit $ret_val fi fi -ironic-dbsync --config-file /etc/ironic/ironic.conf -ironic-dbsync --config-file /etc/ironic/ironic.conf online_data_migrations +ironic-dbsync --config-file /etc/ironic/ironic.conf --config-dir /etc/ironic/ironic.conf.d/ + +ironic-dbsync --config-file /etc/ironic/ironic.conf --config-dir /etc/ironic/ironic.conf.d/ online_data_migrations diff --git a/templates/ironic/config/db-sync-config.json b/templates/ironic/config/db-sync-config.json index 920e0365..80a8c6e7 100644 --- a/templates/ironic/config/db-sync-config.json +++ b/templates/ironic/config/db-sync-config.json @@ -8,8 +8,8 @@ "perm": "0600" }, { - "source": "/var/lib/config-data/merged/custom.conf", - "dest": "/etc/ironic/ironic.conf.d/custom.conf", + "source": "/var/lib/config-data/merged/01-ironic-custom.conf", + "dest": "/etc/ironic/ironic.conf.d/01-ironic-custom.conf", "owner": "ironic", "perm": "0600" }, diff --git a/templates/ironicapi/config/ironic-api-config.json b/templates/ironicapi/config/ironic-api-config.json index 6e8699c2..d9f25fe1 100644 --- a/templates/ironicapi/config/ironic-api-config.json +++ b/templates/ironicapi/config/ironic-api-config.json @@ -8,8 +8,14 @@ "perm": "0600" }, { - "source": "/var/lib/config-data/merged/custom.conf", - "dest": "/etc/ironic/ironic.conf.d/custom.conf", + "source": "/var/lib/config-data/merged/01-ironic-custom.conf", + "dest": "/etc/ironic/ironic.conf.d/01-ironic-custom.conf", + "owner": "ironic", + "perm": "0600" + }, + { + "source": "/var/lib/config-data/merged/02-api-custom.conf", + "dest": "/etc/ironic/ironic.conf.d/02-api-custom.conf", "owner": "ironic", "perm": "0600" }, diff --git a/templates/ironicconductor/config/ironic-conductor-config.json b/templates/ironicconductor/config/ironic-conductor-config.json index e389d9c6..fcf98623 100644 --- a/templates/ironicconductor/config/ironic-conductor-config.json +++ b/templates/ironicconductor/config/ironic-conductor-config.json @@ -8,8 +8,14 @@ "perm": "0600" }, { - "source": "/var/lib/config-data/merged/custom.conf", - "dest": "/etc/ironic/ironic.conf.d/custom.conf", + "source": "/var/lib/config-data/merged/01-ironic-custom.conf", + "dest": "/etc/ironic/ironic.conf.d/01-ironic-custom.conf", + "owner": "ironic", + "perm": "0600" + }, + { + "source": "/var/lib/config-data/merged/02-conductor-custom.conf", + "dest": "/etc/ironic/ironic.conf.d/02-conductor-custom.conf", "owner": "ironic", "perm": "0600" },