From 7f0c780eab60a3558df0e4aa9cbf6322f9d034f6 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 18 Mar 2024 16:19:42 +0100 Subject: [PATCH 1/3] Use common variable for durations and requeue We have a number of places where we are using 5 seconds durations, 10 seconds duration, and requeue after 10 seconds. These are all "magic numbers" spread all over the operator code. This patch creates 2 constants and 1 variable (because golang doesn't allow it to be defined as a constant) for these so that they can be used throughout the code having defined the "magic numbers" in a single location. --- controllers/cinder_controller.go | 15 ++++++++------- controllers/cinderapi_controller.go | 19 +++++++------------ controllers/cinderbackup_controller.go | 15 +++++---------- controllers/cinderscheduler_controller.go | 15 +++++---------- controllers/cindervolume_controller.go | 15 +++++---------- pkg/cinder/const.go | 9 +++++++++ 6 files changed, 39 insertions(+), 49 deletions(-) diff --git a/controllers/cinder_controller.go b/controllers/cinder_controller.go index 1f5748e6..0bde0302 100644 --- a/controllers/cinder_controller.go +++ b/controllers/cinder_controller.go @@ -396,7 +396,7 @@ func (r *CinderReconciler) reconcileInit( jobDef, cinderv1beta1.DbSyncHash, instance.Spec.PreserveJobs, - time.Duration(5)*time.Second, + cinder.ShortDuration, dbSyncHash, ) ctrlResult, err := dbSyncjob.DoJob( @@ -492,7 +492,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder condition.RequestedReason, condition.SeverityInfo, condition.RabbitMqTransportURLReadyRunningMessage)) - return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + return cinder.ResultRequeue, nil } instance.Status.Conditions.MarkTrue(condition.RabbitMqTransportURLReadyCondition, condition.RabbitMqTransportURLReadyMessage) @@ -504,6 +504,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder // memcached, err := memcachedv1.GetMemcachedByName(ctx, helper, instance.Spec.MemcachedInstance, instance.Namespace) if err != nil { + Log.Info(fmt.Sprintf("%s... requeueing", condition.MemcachedReadyWaitingMessage)) if k8s_errors.IsNotFound(err) { Log.Info(fmt.Sprintf("memcached %s not found", instance.Spec.MemcachedInstance)) instance.Status.Conditions.Set(condition.FalseCondition( @@ -511,7 +512,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder condition.RequestedReason, condition.SeverityInfo, condition.MemcachedReadyWaitingMessage)) - return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + return cinder.ResultRequeue, nil } instance.Status.Conditions.Set(condition.FalseCondition( condition.MemcachedReadyCondition, @@ -523,13 +524,13 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder } if !memcached.IsReady() { - Log.Info(fmt.Sprintf("memcached %s is not ready", memcached.Name)) + Log.Info(fmt.Sprintf("%s... requeueing", condition.MemcachedReadyWaitingMessage)) instance.Status.Conditions.Set(condition.FalseCondition( condition.MemcachedReadyCondition, condition.RequestedReason, condition.SeverityInfo, condition.MemcachedReadyWaitingMessage)) - return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + return cinder.ResultRequeue, nil } // Mark the Memcached Service as Ready if we get to this point with no errors instance.Status.Conditions.MarkTrue( @@ -548,7 +549,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder condition.RequestedReason, condition.SeverityInfo, condition.InputReadyWaitingMessage)) - return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + return cinder.ResultRequeue, fmt.Errorf("OpenStack secret %s not found", instance.Spec.Secret) } instance.Status.Conditions.Set(condition.FalseCondition( condition.InputReadyCondition, @@ -629,7 +630,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder condition.SeverityInfo, condition.NetworkAttachmentsReadyWaitingMessage, netAtt)) - return ctrl.Result{RequeueAfter: time.Second * 10}, nil + return cinder.ResultRequeue, fmt.Errorf(condition.NetworkAttachmentsReadyWaitingMessage, netAtt) } instance.Status.Conditions.Set(condition.FalseCondition( condition.NetworkAttachmentsReadyCondition, diff --git a/controllers/cinderapi_controller.go b/controllers/cinderapi_controller.go index f3cf7e35..ca4f28d2 100644 --- a/controllers/cinderapi_controller.go +++ b/controllers/cinderapi_controller.go @@ -19,7 +19,6 @@ package controllers import ( "context" "fmt" - "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -569,7 +568,7 @@ func (r *CinderAPIReconciler) reconcileInit( PasswordSelector: instance.Spec.PasswordSelectors.Service, } - ksSvcObj := keystonev1.NewKeystoneService(ksSvcSpec, instance.Namespace, serviceLabels, time.Duration(10)*time.Second) + ksSvcObj := keystonev1.NewKeystoneService(ksSvcSpec, instance.Namespace, serviceLabels, cinder.NormalDuration) ctrlResult, err := ksSvcObj.CreateOrPatch(ctx, helper) if err != nil { instance.Status.Conditions.MarkFalse( @@ -604,7 +603,7 @@ func (r *CinderAPIReconciler) reconcileInit( instance.Namespace, ksEndptSpec, serviceLabels, - time.Duration(10)*time.Second) + cinder.NormalDuration) ctrlResult, err = ksEndptObj.CreateOrPatch(ctx, helper) if err != nil { instance.Status.Conditions.MarkFalse( @@ -781,7 +780,7 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin condition.SeverityInfo, condition.NetworkAttachmentsReadyWaitingMessage, netAtt)) - return ctrl.Result{RequeueAfter: time.Second * 10}, nil + return cinder.ResultRequeue, fmt.Errorf("network-attachment-definition %s not found", netAtt) } instance.Status.Conditions.Set(condition.FalseCondition( condition.NetworkAttachmentsReadyCondition, @@ -869,10 +868,7 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin err.Error())) return ctrl.Result{}, err } - ss := statefulset.NewStatefulSet( - ssDef, - time.Duration(5)*time.Second, - ) + ss := statefulset.NewStatefulSet(ssDef, cinder.ShortDuration) var ssData appsv1.StatefulSet ctrlResult, err = ss.CreateOrPatch(ctx, helper) @@ -889,9 +885,8 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin // Wait until the data in the StatefulSet is for the current generation ssData = ss.GetStatefulSet() if ssData.Generation != ssData.Status.ObservedGeneration { - ctrlResult = ctrl.Result{RequeueAfter: time.Duration(10) * time.Second} - Log.Info(fmt.Sprintf("waiting for Statefulset %s to start reconciling", ssData.Name)) - err = nil + ctrlResult = cinder.ResultRequeue + err = fmt.Errorf("waiting for Statefulset %s to start reconciling", ssData.Name) } } @@ -1021,7 +1016,7 @@ func (r *CinderAPIReconciler) getSecret( condition.RequestedReason, condition.SeverityInfo, condition.InputReadyWaitingMessage)) - return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + return cinder.ResultRequeue, fmt.Errorf("Secret %s not found", secretName) } instance.Status.Conditions.Set(condition.FalseCondition( condition.InputReadyCondition, diff --git a/controllers/cinderbackup_controller.go b/controllers/cinderbackup_controller.go index 4afa8551..e289d4d0 100644 --- a/controllers/cinderbackup_controller.go +++ b/controllers/cinderbackup_controller.go @@ -19,7 +19,6 @@ package controllers import ( "context" "fmt" - "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -485,7 +484,7 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance * condition.SeverityInfo, condition.NetworkAttachmentsReadyWaitingMessage, netAtt)) - return ctrl.Result{RequeueAfter: time.Second * 10}, nil + return cinder.ResultRequeue, fmt.Errorf("network-attachment-definition %s not found", netAtt) } instance.Status.Conditions.Set(condition.FalseCondition( condition.NetworkAttachmentsReadyCondition, @@ -539,10 +538,7 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance * // Deploy a statefulset ssDef := cinderbackup.StatefulSet(instance, inputHash, serviceLabels, serviceAnnotations) - ss := statefulset.NewStatefulSet( - ssDef, - time.Duration(5)*time.Second, - ) + ss := statefulset.NewStatefulSet(ssDef, cinder.ShortDuration) var ssData appsv1.StatefulSet ctrlResult, err = ss.CreateOrPatch(ctx, helper) @@ -559,9 +555,8 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance * // Wait until the data in the StatefulSet is for the current generation ssData = ss.GetStatefulSet() if ssData.Generation != ssData.Status.ObservedGeneration { - ctrlResult = ctrl.Result{RequeueAfter: time.Duration(10) * time.Second} - Log.Info(fmt.Sprintf("waiting for Statefulset %s to start reconciling", ssData.Name)) - err = nil + ctrlResult = cinder.ResultRequeue + err = fmt.Errorf("waiting for Statefulset %s to start reconciling", ssData.Name) } } @@ -690,7 +685,7 @@ func (r *CinderBackupReconciler) getSecret( condition.RequestedReason, condition.SeverityInfo, condition.InputReadyWaitingMessage)) - return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + return cinder.ResultRequeue, fmt.Errorf("Secret %s not found", secretName) } instance.Status.Conditions.Set(condition.FalseCondition( condition.InputReadyCondition, diff --git a/controllers/cinderscheduler_controller.go b/controllers/cinderscheduler_controller.go index 8dc2c21e..89bdfa17 100644 --- a/controllers/cinderscheduler_controller.go +++ b/controllers/cinderscheduler_controller.go @@ -19,7 +19,6 @@ package controllers import ( "context" "fmt" - "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -484,7 +483,7 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc condition.SeverityInfo, condition.NetworkAttachmentsReadyWaitingMessage, netAtt)) - return ctrl.Result{RequeueAfter: time.Second * 10}, nil + return cinder.ResultRequeue, fmt.Errorf("network-attachment-definition %s not found", netAtt) } instance.Status.Conditions.Set(condition.FalseCondition( condition.NetworkAttachmentsReadyCondition, @@ -538,10 +537,7 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc // Deploy a statefulset ssDef := cinderscheduler.StatefulSet(instance, inputHash, serviceLabels, serviceAnnotations) - ss := statefulset.NewStatefulSet( - ssDef, - time.Duration(5)*time.Second, - ) + ss := statefulset.NewStatefulSet(ssDef, cinder.ShortDuration) var ssData appsv1.StatefulSet ctrlResult, err = ss.CreateOrPatch(ctx, helper) @@ -558,9 +554,8 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc // Wait until the data in the StatefulSet is for the current generation ssData = ss.GetStatefulSet() if ssData.Generation != ssData.Status.ObservedGeneration { - ctrlResult = ctrl.Result{RequeueAfter: time.Duration(10) * time.Second} - Log.Info(fmt.Sprintf("waiting for Statefulset %s to start reconciling", ssData.Name)) - err = nil + ctrlResult = cinder.ResultRequeue + err = fmt.Errorf("waiting for Statefulset %s to start reconciling", ssData.Name) } } @@ -689,7 +684,7 @@ func (r *CinderSchedulerReconciler) getSecret( condition.RequestedReason, condition.SeverityInfo, condition.InputReadyWaitingMessage)) - return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + return cinder.ResultRequeue, fmt.Errorf("Secret %s not found", secretName) } instance.Status.Conditions.Set(condition.FalseCondition( condition.InputReadyCondition, diff --git a/controllers/cindervolume_controller.go b/controllers/cindervolume_controller.go index b2342f10..ff3a2eb9 100644 --- a/controllers/cindervolume_controller.go +++ b/controllers/cindervolume_controller.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "strings" - "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -487,7 +486,7 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance * condition.SeverityInfo, condition.NetworkAttachmentsReadyWaitingMessage, netAtt)) - return ctrl.Result{RequeueAfter: time.Second * 10}, nil + return cinder.ResultRequeue, fmt.Errorf("network-attachment-definition %s not found", netAtt) } instance.Status.Conditions.Set(condition.FalseCondition( condition.NetworkAttachmentsReadyCondition, @@ -541,10 +540,7 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance * // Deploy a statefulset ssDef := cindervolume.StatefulSet(instance, inputHash, serviceLabels, serviceAnnotations, usesLVM) - ss := statefulset.NewStatefulSet( - ssDef, - time.Duration(5)*time.Second, - ) + ss := statefulset.NewStatefulSet(ssDef, cinder.ShortDuration) var ssData appsv1.StatefulSet ctrlResult, err = ss.CreateOrPatch(ctx, helper) @@ -561,9 +557,8 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance * // Wait until the data in the StatefulSet is for the current generation ssData = ss.GetStatefulSet() if ssData.Generation != ssData.Status.ObservedGeneration { - ctrlResult = ctrl.Result{RequeueAfter: time.Duration(10) * time.Second} - Log.Info(fmt.Sprintf("waiting for Statefulset %s to start reconciling", ssData.Name)) - err = nil + ctrlResult = cinder.ResultRequeue + err = fmt.Errorf("waiting for Statefulset %s to start reconciling", ssData.Name) } } @@ -692,7 +687,7 @@ func (r *CinderVolumeReconciler) getSecret( condition.RequestedReason, condition.SeverityInfo, condition.InputReadyWaitingMessage)) - return ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, nil + return cinder.ResultRequeue, fmt.Errorf("Secret %s not found", secretName) } instance.Status.Conditions.Set(condition.FalseCondition( condition.InputReadyCondition, diff --git a/pkg/cinder/const.go b/pkg/cinder/const.go index 560b39d1..3ac5a2c8 100644 --- a/pkg/cinder/const.go +++ b/pkg/cinder/const.go @@ -16,6 +16,10 @@ limitations under the License. package cinder import ( + "time" + + ctrl "sigs.k8s.io/controller-runtime" + "github.com/openstack-k8s-operators/lib-common/modules/storage" ) @@ -66,8 +70,13 @@ const ( // Cinder is the global ServiceType that refers to all the components deployed // by the cinder operator Cinder storage.PropagationType = "Cinder" + + ShortDuration = time.Duration(5) * time.Second + NormalDuration = time.Duration(10) * time.Second ) +var ResultRequeue = ctrl.Result{RequeueAfter: NormalDuration} + // DbsyncPropagation keeps track of the DBSync Service Propagation Type var DbsyncPropagation = []storage.PropagationType{storage.DBSync} From 1bec453824d18cd1250183f925a4f9b61398c100 Mon Sep 17 00:00:00 2001 From: Alan Bishop Date: Wed, 4 Sep 2024 12:20:25 -0700 Subject: [PATCH 2/3] Move to VerifySecret when checking the ctlplane secret Instead of recording a hash of the entire ctlplane secret, a new function is introduced that uses lib-common's VerifySecret() to verify and return the hash of only the cinder service's portion of the secret. This avoids restarting all cinder services when an unrelated portion of the ctlplane secret changes. This change follows a similar manila PR [1]. [1] https://github.com/openstack-k8s-operators/manila-operator/pull/324 Jira: OSPRH-8192 --- controllers/cinder_common.go | 71 +++++++++++++++++++++++ controllers/cinder_controller.go | 36 +++++------- controllers/cinderapi_controller.go | 15 ++++- controllers/cinderbackup_controller.go | 15 ++++- controllers/cinderscheduler_controller.go | 15 ++++- controllers/cindervolume_controller.go | 15 ++++- 6 files changed, 143 insertions(+), 24 deletions(-) create mode 100644 controllers/cinder_common.go diff --git a/controllers/cinder_common.go b/controllers/cinder_common.go new file mode 100644 index 00000000..dd43d96e --- /dev/null +++ b/controllers/cinder_common.go @@ -0,0 +1,71 @@ +/* +Copyright 2024. + +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 ( + "context" + "fmt" + "github.com/openstack-k8s-operators/lib-common/modules/common/condition" + "github.com/openstack-k8s-operators/lib-common/modules/common/secret" + "k8s.io/apimachinery/pkg/types" + "time" + + "github.com/openstack-k8s-operators/lib-common/modules/common/env" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" +) + +type conditionUpdater interface { + Set(c *condition.Condition) + MarkTrue(t condition.Type, messageFormat string, messageArgs ...interface{}) +} + +// verifyServiceSecret - ensures that the Secret object exists and the expected +// fields are in the Secret. It also sets a hash of the values of the expected +// fields passed as input. +func verifyServiceSecret( + ctx context.Context, + secretName types.NamespacedName, + expectedFields []string, + reader client.Reader, + conditionUpdater conditionUpdater, + requeueTimeout time.Duration, + envVars *map[string]env.Setter, +) (ctrl.Result, error) { + + hash, res, err := secret.VerifySecret(ctx, secretName, expectedFields, reader, requeueTimeout) + if err != nil { + conditionUpdater.Set(condition.FalseCondition( + condition.InputReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.InputReadyErrorMessage, + err.Error())) + return res, err + } else if (res != ctrl.Result{}) { + log.FromContext(ctx).Info(fmt.Sprintf("OpenStack secret %s not found", secretName)) + conditionUpdater.Set(condition.FalseCondition( + condition.InputReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + condition.InputReadyWaitingMessage)) + return res, nil + } + (*envVars)[secretName.Name] = env.SetValue(hash) + return ctrl.Result{}, nil +} diff --git a/controllers/cinder_controller.go b/controllers/cinder_controller.go index 0bde0302..5dfe21b6 100644 --- a/controllers/cinder_controller.go +++ b/controllers/cinder_controller.go @@ -23,6 +23,7 @@ import ( k8s_errors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -540,28 +541,23 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder // // check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map // - ospSecret, hash, err := secret.GetSecret(ctx, helper, instance.Spec.Secret, instance.Namespace) + + result, err := verifyServiceSecret( + ctx, + types.NamespacedName{Namespace: instance.Namespace, Name: instance.Spec.Secret}, + []string{ + instance.Spec.PasswordSelectors.Service, + }, + helper.GetClient(), + &instance.Status.Conditions, + cinder.NormalDuration, + &configVars, + ) if err != nil { - if k8s_errors.IsNotFound(err) { - Log.Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret)) - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.RequestedReason, - condition.SeverityInfo, - condition.InputReadyWaitingMessage)) - return cinder.ResultRequeue, fmt.Errorf("OpenStack secret %s not found", instance.Spec.Secret) - } - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.ErrorReason, - condition.SeverityWarning, - condition.InputReadyErrorMessage, - err.Error())) - return ctrl.Result{}, err + return result, err + } else if (result != ctrl.Result{}) { + return result, nil } - // Add a prefix to the var name to avoid accidental collision with other non-secret vars. - configVars["secret-"+ospSecret.Name] = env.SetValue(hash) - instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage) // run check OpenStack secret - end diff --git a/controllers/cinderapi_controller.go b/controllers/cinderapi_controller.go index ca4f28d2..9c23e190 100644 --- a/controllers/cinderapi_controller.go +++ b/controllers/cinderapi_controller.go @@ -641,9 +641,22 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin // // check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map // - ctrlResult, err := r.getSecret(ctx, helper, instance, instance.Spec.Secret, &configVars) + + ctrlResult, err := verifyServiceSecret( + ctx, + types.NamespacedName{Namespace: instance.Namespace, Name: instance.Spec.Secret}, + []string{ + instance.Spec.PasswordSelectors.Service, + }, + helper.GetClient(), + &instance.Status.Conditions, + cinder.NormalDuration, + &configVars, + ) if err != nil { return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil } // diff --git a/controllers/cinderbackup_controller.go b/controllers/cinderbackup_controller.go index e289d4d0..209e172b 100644 --- a/controllers/cinderbackup_controller.go +++ b/controllers/cinderbackup_controller.go @@ -342,9 +342,22 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance * // // check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map // - ctrlResult, err := r.getSecret(ctx, helper, instance, instance.Spec.Secret, &configVars) + + ctrlResult, err := verifyServiceSecret( + ctx, + types.NamespacedName{Namespace: instance.Namespace, Name: instance.Spec.Secret}, + []string{ + instance.Spec.PasswordSelectors.Service, + }, + helper.GetClient(), + &instance.Status.Conditions, + cinder.NormalDuration, + &configVars, + ) if err != nil { return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil } // diff --git a/controllers/cinderscheduler_controller.go b/controllers/cinderscheduler_controller.go index 89bdfa17..8bdcc576 100644 --- a/controllers/cinderscheduler_controller.go +++ b/controllers/cinderscheduler_controller.go @@ -341,9 +341,22 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc // // check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map // - ctrlResult, err := r.getSecret(ctx, helper, instance, instance.Spec.Secret, &configVars) + + ctrlResult, err := verifyServiceSecret( + ctx, + types.NamespacedName{Namespace: instance.Namespace, Name: instance.Spec.Secret}, + []string{ + instance.Spec.PasswordSelectors.Service, + }, + helper.GetClient(), + &instance.Status.Conditions, + cinder.NormalDuration, + &configVars, + ) if err != nil { return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil } // diff --git a/controllers/cindervolume_controller.go b/controllers/cindervolume_controller.go index ff3a2eb9..da5ca3fb 100644 --- a/controllers/cindervolume_controller.go +++ b/controllers/cindervolume_controller.go @@ -343,9 +343,22 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance * // // check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map // - ctrlResult, err := r.getSecret(ctx, helper, instance, instance.Spec.Secret, &configVars) + + ctrlResult, err := verifyServiceSecret( + ctx, + types.NamespacedName{Namespace: instance.Namespace, Name: instance.Spec.Secret}, + []string{ + instance.Spec.PasswordSelectors.Service, + }, + helper.GetClient(), + &instance.Status.Conditions, + cinder.NormalDuration, + &configVars, + ) if err != nil { return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil } // From 8509ddcabcee143d2393e24802587fa62d029f2c Mon Sep 17 00:00:00 2001 From: Alan Bishop Date: Wed, 4 Sep 2024 12:55:32 -0700 Subject: [PATCH 3/3] Consolidate hashing TransportURL and config secrets Replace multiple calls to collect hashes of the Transport URL secret and other service configuration secrets with a new verifyConfigSecrets() function that can be shared by all cinder services. This also eliminates the need for each service to provide its own getSecret function. This change continues to mimic code in manila's PR [1]. [1] https://github.com/openstack-k8s-operators/manila-operator/pull/324 --- controllers/cinder_common.go | 43 ++++++++++++ controllers/cinderapi_controller.go | 80 ++++++----------------- controllers/cinderbackup_controller.go | 80 ++++++----------------- controllers/cinderscheduler_controller.go | 80 ++++++----------------- controllers/cindervolume_controller.go | 80 ++++++----------------- 5 files changed, 119 insertions(+), 244 deletions(-) diff --git a/controllers/cinder_common.go b/controllers/cinder_common.go index dd43d96e..ea8446ee 100644 --- a/controllers/cinder_common.go +++ b/controllers/cinder_common.go @@ -24,7 +24,10 @@ import ( "k8s.io/apimachinery/pkg/types" "time" + "github.com/openstack-k8s-operators/cinder-operator/pkg/cinder" "github.com/openstack-k8s-operators/lib-common/modules/common/env" + "github.com/openstack-k8s-operators/lib-common/modules/common/helper" + k8s_errors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -69,3 +72,43 @@ func verifyServiceSecret( (*envVars)[secretName.Name] = env.SetValue(hash) return ctrl.Result{}, nil } + +// verifyConfigSecrets - It iterates over the secretNames passed as input and +// sets the hash of values in the envVars map. +func verifyConfigSecrets( + ctx context.Context, + h *helper.Helper, + conditionUpdater conditionUpdater, + secretNames []string, + namespace string, + envVars *map[string]env.Setter, +) (ctrl.Result, error) { + var hash string + var err error + for _, secretName := range secretNames { + _, hash, err = secret.GetSecret(ctx, h, secretName, namespace) + if err != nil { + if k8s_errors.IsNotFound(err) { + log.FromContext(ctx).Info(fmt.Sprintf("Secret %s not found", secretName)) + conditionUpdater.Set(condition.FalseCondition( + condition.InputReadyCondition, + condition.RequestedReason, + condition.SeverityInfo, + condition.InputReadyWaitingMessage)) + return cinder.ResultRequeue, nil + } + conditionUpdater.Set(condition.FalseCondition( + condition.InputReadyCondition, + condition.ErrorReason, + condition.SeverityWarning, + condition.InputReadyErrorMessage, + err.Error())) + return ctrl.Result{}, err + } + // Add a prefix to the var name to avoid accidental collision with other non-secret + // vars. The secret names themselves will be unique. + (*envVars)["secret-"+secretName] = env.SetValue(hash) + } + + return ctrl.Result{}, nil +} diff --git a/controllers/cinderapi_controller.go b/controllers/cinderapi_controller.go index 9c23e190..55130abe 100644 --- a/controllers/cinderapi_controller.go +++ b/controllers/cinderapi_controller.go @@ -660,37 +660,30 @@ func (r *CinderAPIReconciler) reconcileNormal(ctx context.Context, instance *cin } // - // check for required TransportURL secret holding transport URL string + // check for required Transport URL and config secrets // - ctrlResult, err = r.getSecret(ctx, helper, instance, instance.Spec.TransportURLSecret, &configVars) - if err != nil { - return ctrlResult, err - } - - // - // check for required service secrets - // - for _, secretName := range instance.Spec.CustomServiceConfigSecrets { - ctrlResult, err = r.getSecret(ctx, helper, instance, secretName, &configVars) - if err != nil { - return ctrlResult, err - } - } - // - // check for required Cinder secrets that should have been created by parent Cinder CR - // parentCinderName := cinder.GetOwningCinderName(instance) - parentSecrets := []string{ - fmt.Sprintf("%s-scripts", parentCinderName), - fmt.Sprintf("%s-config-data", parentCinderName), + secretNames := []string{ + instance.Spec.TransportURLSecret, // TransportURLSecret + fmt.Sprintf("%s-scripts", parentCinderName), // ScriptsSecret + fmt.Sprintf("%s-config-data", parentCinderName), // ConfigSecret } + // Append CustomServiceConfigSecrets that should be checked + secretNames = append(secretNames, instance.Spec.CustomServiceConfigSecrets...) - for _, parentSecret := range parentSecrets { - ctrlResult, err = r.getSecret(ctx, helper, instance, parentSecret, &configVars) - if err != nil { - return ctrlResult, err - } + ctrlResult, err = verifyConfigSecrets( + ctx, + helper, + &instance.Status.Conditions, + secretNames, + instance.Namespace, + &configVars, + ) + if err != nil { + return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil } instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage) @@ -1012,41 +1005,6 @@ func (r *CinderAPIReconciler) reconcileUpgrade(ctx context.Context, instance *ci return ctrl.Result{}, nil } -// getSecret - get the specified secret, and add its hash to envVars -func (r *CinderAPIReconciler) getSecret( - ctx context.Context, - h *helper.Helper, - instance *cinderv1beta1.CinderAPI, - secretName string, - envVars *map[string]env.Setter, -) (ctrl.Result, error) { - secret, hash, err := secret.GetSecret(ctx, h, secretName, instance.Namespace) - if err != nil { - if k8s_errors.IsNotFound(err) { - h.GetLogger().Info(fmt.Sprintf("Secret %s not found", secretName)) - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.RequestedReason, - condition.SeverityInfo, - condition.InputReadyWaitingMessage)) - return cinder.ResultRequeue, fmt.Errorf("Secret %s not found", secretName) - } - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.ErrorReason, - condition.SeverityWarning, - condition.InputReadyErrorMessage, - err.Error())) - return ctrl.Result{}, err - } - - // Add a prefix to the var name to avoid accidental collision with other non-secret - // vars. The secret names themselves will be unique. - (*envVars)["secret-"+secret.Name] = env.SetValue(hash) - - return ctrl.Result{}, nil -} - // generateServiceConfigs - create Secret which holds the service configuration func (r *CinderAPIReconciler) generateServiceConfigs( ctx context.Context, diff --git a/controllers/cinderbackup_controller.go b/controllers/cinderbackup_controller.go index 209e172b..5aba831a 100644 --- a/controllers/cinderbackup_controller.go +++ b/controllers/cinderbackup_controller.go @@ -361,37 +361,30 @@ func (r *CinderBackupReconciler) reconcileNormal(ctx context.Context, instance * } // - // check for required TransportURL secret holding transport URL string + // check for required Transport URL and config secrets // - ctrlResult, err = r.getSecret(ctx, helper, instance, instance.Spec.TransportURLSecret, &configVars) - if err != nil { - return ctrlResult, err - } - - // - // check for required service secrets - // - for _, secretName := range instance.Spec.CustomServiceConfigSecrets { - ctrlResult, err = r.getSecret(ctx, helper, instance, secretName, &configVars) - if err != nil { - return ctrlResult, err - } - } - // - // check for required Cinder secrets that should have been created by parent Cinder CR - // parentCinderName := cinder.GetOwningCinderName(instance) - parentSecrets := []string{ - fmt.Sprintf("%s-scripts", parentCinderName), - fmt.Sprintf("%s-config-data", parentCinderName), + secretNames := []string{ + instance.Spec.TransportURLSecret, // TransportURLSecret + fmt.Sprintf("%s-scripts", parentCinderName), // ScriptsSecret + fmt.Sprintf("%s-config-data", parentCinderName), // ConfigSecret } + // Append CustomServiceConfigSecrets that should be checked + secretNames = append(secretNames, instance.Spec.CustomServiceConfigSecrets...) - for _, parentSecret := range parentSecrets { - ctrlResult, err = r.getSecret(ctx, helper, instance, parentSecret, &configVars) - if err != nil { - return ctrlResult, err - } + ctrlResult, err = verifyConfigSecrets( + ctx, + helper, + &instance.Status.Conditions, + secretNames, + instance.Namespace, + &configVars, + ) + if err != nil { + return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil } instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage) @@ -681,41 +674,6 @@ func (r *CinderBackupReconciler) reconcileUpgrade(ctx context.Context, instance return ctrl.Result{}, nil } -// getSecret - get the specified secret, and add its hash to envVars -func (r *CinderBackupReconciler) getSecret( - ctx context.Context, - h *helper.Helper, - instance *cinderv1beta1.CinderBackup, - secretName string, - envVars *map[string]env.Setter, -) (ctrl.Result, error) { - secret, hash, err := secret.GetSecret(ctx, h, secretName, instance.Namespace) - if err != nil { - if k8s_errors.IsNotFound(err) { - h.GetLogger().Info(fmt.Sprintf("Secret %s not found", secretName)) - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.RequestedReason, - condition.SeverityInfo, - condition.InputReadyWaitingMessage)) - return cinder.ResultRequeue, fmt.Errorf("Secret %s not found", secretName) - } - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.ErrorReason, - condition.SeverityWarning, - condition.InputReadyErrorMessage, - err.Error())) - return ctrl.Result{}, err - } - - // Add a prefix to the var name to avoid accidental collision with other non-secret - // vars. The secret names themselves will be unique. - (*envVars)["secret-"+secret.Name] = env.SetValue(hash) - - return ctrl.Result{}, nil -} - // generateServiceConfigs - create Secret which holds the service configuration func (r *CinderBackupReconciler) generateServiceConfigs( ctx context.Context, diff --git a/controllers/cinderscheduler_controller.go b/controllers/cinderscheduler_controller.go index 8bdcc576..55cf79f9 100644 --- a/controllers/cinderscheduler_controller.go +++ b/controllers/cinderscheduler_controller.go @@ -360,37 +360,30 @@ func (r *CinderSchedulerReconciler) reconcileNormal(ctx context.Context, instanc } // - // check for required TransportURL secret holding transport URL string + // check for required Transport URL and config secrets // - ctrlResult, err = r.getSecret(ctx, helper, instance, instance.Spec.TransportURLSecret, &configVars) - if err != nil { - return ctrlResult, err - } - - // - // check for required service secrets - // - for _, secretName := range instance.Spec.CustomServiceConfigSecrets { - ctrlResult, err = r.getSecret(ctx, helper, instance, secretName, &configVars) - if err != nil { - return ctrlResult, err - } - } - // - // check for required Cinder secrets that should have been created by parent Cinder CR - // parentCinderName := cinder.GetOwningCinderName(instance) - parentSecrets := []string{ - fmt.Sprintf("%s-scripts", parentCinderName), - fmt.Sprintf("%s-config-data", parentCinderName), + secretNames := []string{ + instance.Spec.TransportURLSecret, // TransportURLSecret + fmt.Sprintf("%s-scripts", parentCinderName), // ScriptsSecret + fmt.Sprintf("%s-config-data", parentCinderName), // ConfigSecret } + // Append CustomServiceConfigSecrets that should be checked + secretNames = append(secretNames, instance.Spec.CustomServiceConfigSecrets...) - for _, parentSecret := range parentSecrets { - ctrlResult, err = r.getSecret(ctx, helper, instance, parentSecret, &configVars) - if err != nil { - return ctrlResult, err - } + ctrlResult, err = verifyConfigSecrets( + ctx, + helper, + &instance.Status.Conditions, + secretNames, + instance.Namespace, + &configVars, + ) + if err != nil { + return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil } instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage) @@ -680,41 +673,6 @@ func (r *CinderSchedulerReconciler) reconcileUpgrade(ctx context.Context, instan return ctrl.Result{}, nil } -// getSecret - get the specified secret, and add its hash to envVars -func (r *CinderSchedulerReconciler) getSecret( - ctx context.Context, - h *helper.Helper, - instance *cinderv1beta1.CinderScheduler, - secretName string, - envVars *map[string]env.Setter, -) (ctrl.Result, error) { - secret, hash, err := secret.GetSecret(ctx, h, secretName, instance.Namespace) - if err != nil { - if k8s_errors.IsNotFound(err) { - h.GetLogger().Info(fmt.Sprintf("Secret %s not found", secretName)) - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.RequestedReason, - condition.SeverityInfo, - condition.InputReadyWaitingMessage)) - return cinder.ResultRequeue, fmt.Errorf("Secret %s not found", secretName) - } - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.ErrorReason, - condition.SeverityWarning, - condition.InputReadyErrorMessage, - err.Error())) - return ctrl.Result{}, err - } - - // Add a prefix to the var name to avoid accidental collision with other non-secret - // vars. The secret names themselves will be unique. - (*envVars)["secret-"+secret.Name] = env.SetValue(hash) - - return ctrl.Result{}, nil -} - // generateServiceConfigs - create Secret which holds the service configuration func (r *CinderSchedulerReconciler) generateServiceConfigs( ctx context.Context, diff --git a/controllers/cindervolume_controller.go b/controllers/cindervolume_controller.go index da5ca3fb..bf31bb49 100644 --- a/controllers/cindervolume_controller.go +++ b/controllers/cindervolume_controller.go @@ -362,37 +362,30 @@ func (r *CinderVolumeReconciler) reconcileNormal(ctx context.Context, instance * } // - // check for required TransportURL secret holding transport URL string + // check for required Transport URL and config secrets // - ctrlResult, err = r.getSecret(ctx, helper, instance, instance.Spec.TransportURLSecret, &configVars) - if err != nil { - return ctrlResult, err - } - - // - // check for required service secrets - // - for _, secretName := range instance.Spec.CustomServiceConfigSecrets { - ctrlResult, err = r.getSecret(ctx, helper, instance, secretName, &configVars) - if err != nil { - return ctrlResult, err - } - } - // - // check for required Cinder secrets that should have been created by parent Cinder CR - // parentCinderName := cinder.GetOwningCinderName(instance) - parentSecrets := []string{ - fmt.Sprintf("%s-scripts", parentCinderName), - fmt.Sprintf("%s-config-data", parentCinderName), + secretNames := []string{ + instance.Spec.TransportURLSecret, // TransportURLSecret + fmt.Sprintf("%s-scripts", parentCinderName), // ScriptsSecret + fmt.Sprintf("%s-config-data", parentCinderName), // ConfigSecret } + // Append CustomServiceConfigSecrets that should be checked + secretNames = append(secretNames, instance.Spec.CustomServiceConfigSecrets...) - for _, parentSecret := range parentSecrets { - ctrlResult, err = r.getSecret(ctx, helper, instance, parentSecret, &configVars) - if err != nil { - return ctrlResult, err - } + ctrlResult, err = verifyConfigSecrets( + ctx, + helper, + &instance.Status.Conditions, + secretNames, + instance.Namespace, + &configVars, + ) + if err != nil { + return ctrlResult, err + } else if (ctrlResult != ctrl.Result{}) { + return ctrlResult, nil } instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage) @@ -683,41 +676,6 @@ func (r *CinderVolumeReconciler) reconcileUpgrade(ctx context.Context, instance return ctrl.Result{}, nil } -// getSecret - get the specified secret, and add its hash to envVars -func (r *CinderVolumeReconciler) getSecret( - ctx context.Context, - h *helper.Helper, - instance *cinderv1beta1.CinderVolume, - secretName string, - envVars *map[string]env.Setter, -) (ctrl.Result, error) { - secret, hash, err := secret.GetSecret(ctx, h, secretName, instance.Namespace) - if err != nil { - if k8s_errors.IsNotFound(err) { - h.GetLogger().Info(fmt.Sprintf("Secret %s not found", secretName)) - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.RequestedReason, - condition.SeverityInfo, - condition.InputReadyWaitingMessage)) - return cinder.ResultRequeue, fmt.Errorf("Secret %s not found", secretName) - } - instance.Status.Conditions.Set(condition.FalseCondition( - condition.InputReadyCondition, - condition.ErrorReason, - condition.SeverityWarning, - condition.InputReadyErrorMessage, - err.Error())) - return ctrl.Result{}, err - } - - // Add a prefix to the var name to avoid accidental collision with other non-secret - // vars. The secret names themselves will be unique. - (*envVars)["secret-"+secret.Name] = env.SetValue(hash) - - return ctrl.Result{}, nil -} - // generateServiceConfigs - create Secret which holds the service configuration and check if it's using LVM func (r *CinderVolumeReconciler) generateServiceConfigs( ctx context.Context,