Skip to content

Commit d1191ad

Browse files
fmountfmount
committed
Move to VerifySecretFields to reject invalid passwords
Service passwords are usually defined in the osp-secret and propagated to their config. This patch moves from verifyServiceSecret to the new lib-common VerifySecretFields that is able to call a validator to accept or reject the password field and set the appropriate errorMsg to the InputReady Condition. This patch also reworks the associated conditions to be consistent with the other operators. Signed-off-by: Francesco Pantano <fpantano@redhat.com>
1 parent c583d43 commit d1191ad

7 files changed

Lines changed: 313 additions & 145 deletions

File tree

‎internal/controller/ironic_controller.go‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -376,28 +376,41 @@ func (r *IronicReconciler) reconcileNormal(ctx context.Context, instance *ironic
376376
//
377377
// check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map
378378
//
379-
ospSecret, hash, err := oko_secret.GetSecret(ctx, helper, instance.Spec.Secret, instance.Namespace)
379+
// Associate to PasswordSelectors.Service field a password validator to
380+
// ensure pwd invalid detected patterns are rejected.
381+
validateFields := map[string]oko_secret.Validator{
382+
instance.Spec.PasswordSelectors.Service: oko_secret.PasswordValidator{},
383+
}
384+
hash, ctrlResult, err := oko_secret.VerifySecretFields(
385+
ctx,
386+
types.NamespacedName{
387+
Namespace: instance.Namespace,
388+
Name: instance.Spec.Secret,
389+
},
390+
validateFields,
391+
helper.GetClient(),
392+
time.Duration(10)*time.Second,
393+
)
380394
if err != nil {
381-
if k8s_errors.IsNotFound(err) {
382-
// Since the OpenStack secret should have been manually created by the user and referenced in the spec,
383-
// we treat this as a warning because it means that the service will not be able to start.
384-
Log.Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret))
385-
instance.Status.Conditions.Set(condition.FalseCondition(
386-
condition.InputReadyCondition,
387-
condition.ErrorReason,
388-
condition.SeverityWarning,
389-
condition.InputReadyWaitingMessage))
390-
return ctrl.Result{}, nil
391-
}
392395
instance.Status.Conditions.Set(condition.FalseCondition(
393396
condition.InputReadyCondition,
394397
condition.ErrorReason,
395398
condition.SeverityWarning,
396399
condition.InputReadyErrorMessage,
397400
err.Error()))
398-
return ctrl.Result{}, err
401+
return ctrlResult, err
402+
} else if (ctrlResult != ctrl.Result{}) {
403+
// Since the service secret should have been manually created by the user and referenced in the spec,
404+
// we treat this as a warning because it means that the service will not be able to start.
405+
log.FromContext(ctx).Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret))
406+
instance.Status.Conditions.Set(condition.FalseCondition(
407+
condition.InputReadyCondition,
408+
condition.ErrorReason,
409+
condition.SeverityWarning,
410+
condition.InputReadyWaitingMessage))
411+
return ctrlResult, err
399412
}
400-
configMapVars[ospSecret.Name] = env.SetValue(hash)
413+
configMapVars[instance.Spec.Secret] = env.SetValue(hash)
401414

402415
instance.Status.Conditions.MarkTrue(condition.InputReadyCondition, condition.InputReadyMessage)
403416
// run check OpenStack secret - end
@@ -480,7 +493,7 @@ func (r *IronicReconciler) reconcileNormal(ctx context.Context, instance *ironic
480493
}
481494

482495
// Handle service upgrade
483-
ctrlResult, err := r.reconcileUpgrade(ctx, instance, helper, serviceLabels)
496+
ctrlResult, err = r.reconcileUpgrade(ctx, instance, helper, serviceLabels)
484497
if err != nil {
485498
return ctrlResult, err
486499
} else if (ctrlResult != ctrl.Result{}) {

‎internal/controller/ironicapi_controller.go‎

Lines changed: 54 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -676,57 +676,81 @@ func (r *IronicAPIReconciler) reconcileNormal(ctx context.Context, instance *iro
676676
//
677677
// check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map
678678
//
679-
ospSecret, hash, err := secret.GetSecret(ctx, helper, instance.Spec.Secret, instance.Namespace)
679+
// Associate to PasswordSelectors.Service field a password validator to
680+
// ensure pwd invalid detected patterns are rejected.
681+
validateFields := map[string]secret.Validator{
682+
instance.Spec.PasswordSelectors.Service: secret.PasswordValidator{},
683+
}
684+
hash, ctrlResult, err := secret.VerifySecretFields(
685+
ctx,
686+
types.NamespacedName{
687+
Namespace: instance.Namespace,
688+
Name: instance.Spec.Secret,
689+
},
690+
validateFields,
691+
helper.GetClient(),
692+
time.Duration(10)*time.Second,
693+
)
680694
if err != nil {
681-
if k8s_errors.IsNotFound(err) {
682-
// Since the OpenStack secret should have been manually created by the user and referenced in the spec,
683-
// we treat this as a warning because it means that the service will not be able to start.
684-
Log.Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret))
685-
instance.Status.Conditions.Set(condition.FalseCondition(
686-
condition.InputReadyCondition,
687-
condition.ErrorReason,
688-
condition.SeverityWarning,
689-
condition.InputReadyWaitingMessage))
690-
return ctrl.Result{RequeueAfter: time.Second * 10}, nil
691-
}
692695
instance.Status.Conditions.Set(condition.FalseCondition(
693696
condition.InputReadyCondition,
694697
condition.ErrorReason,
695698
condition.SeverityWarning,
696699
condition.InputReadyErrorMessage,
697700
err.Error()))
698-
return ctrl.Result{}, err
701+
return ctrlResult, err
702+
} else if (ctrlResult != ctrl.Result{}) {
703+
// Since the service secret should have been manually created by the user and referenced in the spec,
704+
// we treat this as a warning because it means that the service will not be able to start.
705+
log.FromContext(ctx).Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret))
706+
instance.Status.Conditions.Set(condition.FalseCondition(
707+
condition.InputReadyCondition,
708+
condition.ErrorReason,
709+
condition.SeverityWarning,
710+
condition.InputReadyWaitingMessage))
711+
return ctrlResult, err
699712
}
700-
configMapVars[ospSecret.Name] = env.SetValue(hash)
713+
configMapVars[instance.Spec.Secret] = env.SetValue(hash)
701714
// run check OpenStack secret - end
702715

703716
//
704717
// check for required TransportURL secret holding transport URL string
705718
//
706719
if instance.Spec.RPCTransport == "oslo" {
707-
transportURLSecret, hash, err := secret.GetSecret(ctx, helper, instance.Spec.TransportURLSecret, instance.Namespace)
720+
// transportURLFields are not pure password fields. We do not associate a
721+
// password validator and we only verify that the entry exists in the
722+
// secret
723+
transportValidateFields := map[string]secret.Validator{
724+
"transport_url": secret.NoOpValidator{},
725+
}
726+
hash, ctrlResult, err = secret.VerifySecretFields(
727+
ctx,
728+
types.NamespacedName{
729+
Namespace: instance.Namespace,
730+
Name: instance.Spec.TransportURLSecret,
731+
},
732+
transportValidateFields,
733+
helper.GetClient(),
734+
time.Duration(10)*time.Second,
735+
)
708736
if err != nil {
709-
if k8s_errors.IsNotFound(err) {
710-
// Since the TransportURL secret should have been previously automatically created by the parent
711-
// Ironic CR and then referenced in this instance's spec, we treat this as a warning because it
712-
// means that the service will not be able to start.
713-
Log.Info(fmt.Sprintf("TransportURL secret %s not found", instance.Spec.TransportURLSecret))
714-
instance.Status.Conditions.Set(condition.FalseCondition(
715-
condition.InputReadyCondition,
716-
condition.ErrorReason,
717-
condition.SeverityWarning,
718-
condition.InputReadyWaitingMessage))
719-
return ctrl.Result{RequeueAfter: time.Second * 10}, nil
720-
}
721737
instance.Status.Conditions.Set(condition.FalseCondition(
722738
condition.InputReadyCondition,
723739
condition.ErrorReason,
724740
condition.SeverityWarning,
725741
condition.InputReadyErrorMessage,
726742
err.Error()))
727-
return ctrl.Result{}, err
743+
return ctrlResult, err
744+
} else if (ctrlResult != ctrl.Result{}) {
745+
Log.Info(fmt.Sprintf("TransportURL secret %s not found", instance.Spec.TransportURLSecret))
746+
instance.Status.Conditions.Set(condition.FalseCondition(
747+
condition.InputReadyCondition,
748+
condition.RequestedReason,
749+
condition.SeverityInfo,
750+
condition.InputReadyWaitingMessage))
751+
return ctrlResult, err
728752
}
729-
configMapVars[transportURLSecret.Name] = env.SetValue(hash)
753+
configMapVars[instance.Spec.TransportURLSecret] = env.SetValue(hash)
730754
}
731755
// run check TransportURL secret - end
732756

@@ -877,7 +901,7 @@ func (r *IronicAPIReconciler) reconcileNormal(ctx context.Context, instance *iro
877901
}
878902

879903
// Handle service init
880-
ctrlResult, err := r.reconcileInit(ctx, instance, helper, serviceLabels)
904+
ctrlResult, err = r.reconcileInit(ctx, instance, helper, serviceLabels)
881905
if err != nil {
882906
return ctrlResult, err
883907
} else if (ctrlResult != ctrl.Result{}) {

‎internal/controller/ironicconductor_controller.go‎

Lines changed: 55 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -550,57 +550,82 @@ func (r *IronicConductorReconciler) reconcileNormal(ctx context.Context, instanc
550550
//
551551
// check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map
552552
//
553-
ospSecret, hash, err := secret.GetSecret(ctx, helper, instance.Spec.Secret, instance.Namespace)
553+
// Associate to PasswordSelectors.Service field a password validator to
554+
// ensure pwd invalid detected patterns are rejected.
555+
validateFields := map[string]secret.Validator{
556+
instance.Spec.PasswordSelectors.Service: secret.PasswordValidator{},
557+
}
558+
hash, ctrlResult, err := secret.VerifySecretFields(
559+
ctx,
560+
types.NamespacedName{
561+
Namespace: instance.Namespace,
562+
Name: instance.Spec.Secret,
563+
},
564+
validateFields,
565+
helper.GetClient(),
566+
time.Duration(10)*time.Second,
567+
)
554568
if err != nil {
555-
if k8s_errors.IsNotFound(err) {
556-
// Since the OpenStack secret should have been manually created by the user and referenced in the spec,
557-
// we treat this as a warning because it means that the service will not be able to start.
558-
Log.Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret))
559-
instance.Status.Conditions.Set(condition.FalseCondition(
560-
condition.InputReadyCondition,
561-
condition.ErrorReason,
562-
condition.SeverityWarning,
563-
condition.InputReadyWaitingMessage))
564-
return ctrl.Result{RequeueAfter: time.Second * 10}, nil
565-
}
566569
instance.Status.Conditions.Set(condition.FalseCondition(
567570
condition.InputReadyCondition,
568571
condition.ErrorReason,
569572
condition.SeverityWarning,
570573
condition.InputReadyErrorMessage,
571574
err.Error()))
572-
return ctrl.Result{}, err
575+
return ctrlResult, err
576+
} else if (ctrlResult != ctrl.Result{}) {
577+
// Since the service secret should have been manually created by the user and referenced in the spec,
578+
// we treat this as a warning because it means that the service will not be able to start.
579+
log.FromContext(ctx).Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret))
580+
instance.Status.Conditions.Set(condition.FalseCondition(
581+
condition.InputReadyCondition,
582+
condition.ErrorReason,
583+
condition.SeverityWarning,
584+
condition.InputReadyWaitingMessage))
585+
return ctrlResult, err
573586
}
574-
configMapVars[ospSecret.Name] = env.SetValue(hash)
587+
configMapVars[instance.Spec.Secret] = env.SetValue(hash)
575588
// run check OpenStack secret - end
576589

577590
//
578591
// check for required TransportURL secret holding transport URL string
579592
//
580593
if instance.Spec.RPCTransport == "oslo" {
581-
transportURLSecret, hash, err := secret.GetSecret(ctx, helper, instance.Spec.TransportURLSecret, instance.Namespace)
594+
// transportURLFields are not pure password fields. We do not associate a
595+
// password validator and we only verify that the entry exists in the
596+
// secret
597+
transportValidateFields := map[string]secret.Validator{
598+
"transport_url": secret.NoOpValidator{},
599+
}
600+
hash, ctrlResult, err = secret.VerifySecretFields(
601+
ctx,
602+
types.NamespacedName{
603+
Namespace: instance.Namespace,
604+
Name: instance.Spec.TransportURLSecret,
605+
},
606+
transportValidateFields,
607+
helper.GetClient(),
608+
time.Duration(10)*time.Second,
609+
)
610+
// NOTE: This should be moved to TransportURLSecretReadyCondition
582611
if err != nil {
583-
if k8s_errors.IsNotFound(err) {
584-
// Since the TransportURL secret should have been previously automatically created by the parent
585-
// Ironic CR and then referenced in this instance's spec, we treat this as a warning because it
586-
// means that the service will not be able to start.
587-
Log.Info(fmt.Sprintf("TransportURL secret %s not found", instance.Spec.TransportURLSecret))
588-
instance.Status.Conditions.Set(condition.FalseCondition(
589-
condition.InputReadyCondition,
590-
condition.ErrorReason,
591-
condition.SeverityWarning,
592-
condition.InputReadyWaitingMessage))
593-
return ctrl.Result{RequeueAfter: time.Second * 10}, nil
594-
}
595612
instance.Status.Conditions.Set(condition.FalseCondition(
596613
condition.InputReadyCondition,
597614
condition.ErrorReason,
598615
condition.SeverityWarning,
599616
condition.InputReadyErrorMessage,
600617
err.Error()))
601-
return ctrl.Result{}, err
618+
return ctrlResult, err
619+
} else if (ctrlResult != ctrl.Result{}) {
620+
Log.Info(fmt.Sprintf("TransportURL secret %s not found", instance.Spec.TransportURLSecret))
621+
instance.Status.Conditions.Set(condition.FalseCondition(
622+
condition.InputReadyCondition,
623+
condition.RequestedReason,
624+
condition.SeverityInfo,
625+
condition.InputReadyWaitingMessage))
626+
return ctrlResult, err
602627
}
603-
configMapVars[transportURLSecret.Name] = env.SetValue(hash)
628+
configMapVars[instance.Spec.TransportURLSecret] = env.SetValue(hash)
604629
}
605630
// run check TransportURL secret - end
606631

@@ -772,7 +797,7 @@ func (r *IronicConductorReconciler) reconcileNormal(ctx context.Context, instanc
772797
time.Duration(5)*time.Second,
773798
)
774799

775-
ctrlResult, err := ss.CreateOrPatch(ctx, helper)
800+
ctrlResult, err = ss.CreateOrPatch(ctx, helper)
776801
if err != nil {
777802
instance.Status.Conditions.Set(condition.FalseCondition(
778803
condition.DeploymentReadyCondition,

0 commit comments

Comments
 (0)