Refactor Build Controller logic

Adding support for secrets differenciation errors in a Build
Definition of one-word camelCase constants for Build validations
Adding support for Status.Reason and Status.Message fields
Ensure usage of Build constants to populate the Status.Reason
Ensure appropiate message for the Status.Message

Signed-off-by: Zoe <[email protected]>

Author: qu1queee

pkg/apis/build/v1alpha1/build_types.go

@@ -9,6 +9,35 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+// BuildReason is a type used for populating the
+// Build Status.Reason field
+type BuildReason string
+
+const (
+	// SucceedStatus indicates that all validations Succeeded
+	SucceedStatus BuildReason = "Succeeded"
+	// BuildStrategyNotFound indicates that a namespaced-scope strategy was not found in the namespace
+	BuildStrategyNotFound BuildReason = "BuildStrategyNotFound"
+	// ClusterBuildStrategyNotFound indicates that a cluster-scope strategy was not found
+	ClusterBuildStrategyNotFound BuildReason = "ClusterBuildStrategyNotFound"
+	// SetOwnerReferenceFailed indicates that setting ownerReferences between a Build and a BuildRun failed
+	SetOwnerReferenceFailed BuildReason = "SetOwnerReferenceFailed"
+	// SpecSourceSecretRefNotFound indicates the referenced secret in source is missing
+	SpecSourceSecretRefNotFound BuildReason = "SpecSourceSecretRefNotFound"
+	// SpecOutputSecretRefNotFound indicates the referenced secret in output is missing
+	SpecOutputSecretRefNotFound BuildReason = "SpecOutputSecretRefNotFound"
+	// SpecRuntimeSecretRefNotFound indicates the referenced secret in runtime is missing
+	SpecRuntimeSecretRefNotFound BuildReason = "SpecRuntimeSecretRefNotFound"
+	// MultipleSecretRefNotFound indicates that multiple secrets are missing
+	MultipleSecretRefNotFound BuildReason = "MultipleSecretRefNotFound"
+	// RuntimePathsCanNotBeEmpty indicates that the spec.runtime feature is used but the paths were not specified
+	RuntimePathsCanNotBeEmpty BuildReason = "RuntimePathsCanNotBeEmpty"
+	// RemoteRepositoryUnreachable indicates the referenced repository is unreachable
+	RemoteRepositoryUnreachable BuildReason = "RemoteRepositoryUnreachable"
+	// AllValidationsSucceeded indicates a Build was successfully validated
+	AllValidationsSucceeded = "all validations succeeded"
+)
+
 const (
 	// BuildDomain is the domain used for all labels and annotations for this resource
 	BuildDomain = "build.build.dev"

pkg/controller/build/build_controller.go

@@ -10,8 +10,6 @@ import (
 	"reflect"
 	"strings"
 
-	"github.com/pkg/errors"
-
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -36,10 +34,10 @@ import (
 	buildmetrics "github.com/shipwright-io/build/pkg/metrics"
 )
 
-// succeedStatus default status for the Build CRD
-const succeedStatus string = "Succeeded"
-const namespace string = "namespace"
-const name string = "name"
+const (
+	namespace string = "namespace"
+	name      string = "name"
+)
 
 type setOwnerReferenceFunc func(owner, object metav1.Object, scheme *runtime.Scheme) error
 
@@ -235,67 +233,60 @@ func (r *ReconcileBuild) Reconcile(request reconcile.Request) (reconcile.Result,
 
 	// Populate the status struct with default values
 	b.Status.Registered = corev1.ConditionFalse
-	b.Status.Reason = succeedStatus
+	b.Status.Reason = build.SucceedStatus
 
 	// Validate if remote repository exists
 	if b.Spec.Source.SecretRef == nil {
 		if err := r.validateSourceURL(ctx, b, b.Spec.Source.URL); err != nil {
-			b.Status.Reason = err.Error()
-			if updateErr := r.client.Status().Update(ctx, b); updateErr != nil {
-				return reconcile.Result{}, updateErr
-			}
-			return reconcile.Result{}, nil
+			MarkBuildStatus(b, build.RemoteRepositoryUnreachable, err.Error())
+			return r.UpdateBuildStatusAndRetreat(ctx, b)
 		}
 	}
 
 	// Validate if the referenced secrets exist in the namespace
-	var secretNames []string
+	secretRefMap := map[string]build.BuildReason{}
 	if b.Spec.Output.SecretRef != nil && b.Spec.Output.SecretRef.Name != "" {
-		secretNames = append(secretNames, b.Spec.Output.SecretRef.Name)
+		secretRefMap[b.Spec.Output.SecretRef.Name] = build.SpecOutputSecretRefNotFound
 	}
 	if b.Spec.Source.SecretRef != nil && b.Spec.Source.SecretRef.Name != "" {
-		secretNames = append(secretNames, b.Spec.Source.SecretRef.Name)
+		secretRefMap[b.Spec.Source.SecretRef.Name] = build.SpecSourceSecretRefNotFound
 	}
 	if b.Spec.BuilderImage != nil && b.Spec.BuilderImage.SecretRef != nil && b.Spec.BuilderImage.SecretRef.Name != "" {
-		secretNames = append(secretNames, b.Spec.BuilderImage.SecretRef.Name)
+		secretRefMap[b.Spec.BuilderImage.SecretRef.Name] = build.SpecRuntimeSecretRefNotFound
 	}
 
-	if len(secretNames) > 0 {
-		if err := r.validateSecrets(ctx, secretNames, b.Namespace); err != nil {
-			b.Status.Reason = err.Error()
-			if updateErr := r.client.Status().Update(ctx, b); updateErr != nil {
-				// return an error in case of transient failure, and expect the next
-				// reconciliation to be able to update the Status of the object
-				return reconcile.Result{}, fmt.Errorf("errors: %v %v", err, updateErr)
-			}
-			// The Secret Resource watcher will Reconcile again once the missing
-			// secret is created, therefore no need to return an error and enter on an infinite
-			// reconciliation
-			return reconcile.Result{}, nil
+	// Validate if the referenced secrets exist
+	if len(secretRefMap) > 0 {
+		if err := r.validateSecrets(ctx, secretRefMap, b); err != nil {
+			return reconcile.Result{}, err
+		}
+
+		if b.Status.Reason != build.SucceedStatus {
+			return r.UpdateBuildStatusAndRetreat(ctx, b)
 		}
 	}
 
-	// Validate if the build strategy is defined
+	// Validate if the referenced strategy exists
 	if b.Spec.StrategyRef != nil {
-		if err := r.validateStrategyRef(ctx, b.Spec.StrategyRef, b.Namespace); err != nil {
-			b.Status.Reason = err.Error()
-			updateErr := r.client.Status().Update(ctx, b)
-			return reconcile.Result{}, fmt.Errorf("errors: %v %v", err, updateErr)
+		if err := r.validateStrategyRef(ctx, b); err != nil {
+			return reconcile.Result{}, err
+		}
+
+		if b.Status.Reason != build.SucceedStatus {
+			return r.UpdateBuildStatusAndRetreat(ctx, b)
 		}
 		ctxlog.Info(ctx, "buildStrategy found", namespace, b.Namespace, name, b.Name, "strategy", b.Spec.StrategyRef.Name)
 	}
 
-	// validate if "spec.runtime" attributes are valid
+	// Validate "spec.runtime" attributes
 	if utils.IsRuntimeDefined(b) {
-		if err := r.validateRuntime(b.Spec.Runtime); err != nil {
-			ctxlog.Error(ctx, err, "failed validating runtime attributes", "Build", b.Name)
-			b.Status.Reason = err.Error()
-			updateErr := r.client.Status().Update(ctx, b)
-			return reconcile.Result{}, fmt.Errorf("errors: %v %v", err, updateErr)
+		if r.validateRuntimeFailed(b) {
+			return r.UpdateBuildStatusAndRetreat(ctx, b)
 		}
 	}
 
 	b.Status.Registered = corev1.ConditionTrue
+	b.Status.Message = build.AllValidationsSucceeded
 	err = r.client.Status().Update(ctx, b)
 	if err != nil {
 		return reconcile.Result{}, err
@@ -308,114 +299,85 @@ func (r *ReconcileBuild) Reconcile(request reconcile.Request) (reconcile.Result,
 	return reconcile.Result{}, nil
 }
 
-func (r *ReconcileBuild) validateRuntime(runtime *build.Runtime) error {
-	if len(runtime.Paths) == 0 {
-		return fmt.Errorf("the property 'spec.runtime.paths' must not be empty")
+// UpdateBuildStatusAndRetreat returns an error if an update fails, this should force
+// a new reconcile until the API call succeeds. If return is nil, no further reconciliations
+// will take place
+func (r *ReconcileBuild) UpdateBuildStatusAndRetreat(ctx context.Context, b *build.Build) (reconcile.Result, error) {
+	if err := r.client.Status().Update(ctx, b); err != nil {
+		return reconcile.Result{}, err
 	}
-	return nil
+	return reconcile.Result{}, nil
 }
 
-func (r *ReconcileBuild) validateStrategyRef(ctx context.Context, s *build.StrategyRef, ns string) error {
-	if s.Kind != nil {
-		switch *s.Kind {
+func (r *ReconcileBuild) validateRuntimeFailed(b *build.Build) bool {
+	if len(b.Spec.Runtime.Paths) == 0 {
+		MarkBuildStatus(b, build.RuntimePathsCanNotBeEmpty, "the property 'spec.runtime.paths' must not be empty")
+		return true
+	}
+	return false
+}
+
+func (r *ReconcileBuild) validateStrategyRef(ctx context.Context, b *build.Build) error {
+
+	if b.Spec.StrategyRef.Kind != nil {
+		switch *b.Spec.StrategyRef.Kind {
 		case build.NamespacedBuildStrategyKind:
-			if err := r.validateBuildStrategy(ctx, s.Name, ns); err != nil {
+			if err := r.validateBuildStrategy(ctx, b.Spec.StrategyRef.Name, b); err != nil {
 				return err
 			}
 		case build.ClusterBuildStrategyKind:
-			if err := r.validateClusterBuildStrategy(ctx, s.Name); err != nil {
+			if err := r.validateClusterBuildStrategy(ctx, b.Spec.StrategyRef.Name, b); err != nil {
 				return err
 			}
 		default:
-			return fmt.Errorf("unknown strategy %v", *s.Kind)
+			return fmt.Errorf("unknown strategy kind: %v", *b.Spec.StrategyRef.Kind)
 		}
 	} else {
 		ctxlog.Info(ctx, "buildStrategy kind is nil, use default NamespacedBuildStrategyKind")
-		if err := r.validateBuildStrategy(ctx, s.Name, ns); err != nil {
+		if err := r.validateBuildStrategy(ctx, b.Spec.StrategyRef.Name, b); err != nil {
 			return err
 		}
 	}
+
 	return nil
 }
 
-func (r *ReconcileBuild) validateBuildStrategy(ctx context.Context, n string, ns string) error {
-	list := &build.BuildStrategyList{}
-
-	if err := r.client.List(ctx, list, &client.ListOptions{Namespace: ns}); err != nil {
-		return errors.Wrapf(err, "listing BuildStrategies in ns %s failed", ns)
-	}
-
-	if len(list.Items) == 0 {
-		return errors.Errorf("none BuildStrategies found in namespace %s", ns)
+func (r *ReconcileBuild) validateBuildStrategy(ctx context.Context, strategyName string, b *build.Build) error {
+	buildStrategy := &build.BuildStrategy{}
+	if err := r.client.Get(ctx, types.NamespacedName{Name: strategyName, Namespace: b.Namespace}, buildStrategy); err != nil && !apierrors.IsNotFound(err) {
+		return err
+	} else if apierrors.IsNotFound(err) {
+		MarkBuildStatus(b, build.BuildStrategyNotFound, fmt.Sprintf("buildStrategy %s does not exist in namespace %s", b.Spec.StrategyRef.Name, b.Namespace))
 	}
 
-	if len(list.Items) > 0 {
-		for _, s := range list.Items {
-			if s.Name == n {
-				return nil
-			}
-		}
-		return fmt.Errorf("buildStrategy %s does not exist in namespace %s", n, ns)
-	}
 	return nil
 }
 
-func (r *ReconcileBuild) validateClusterBuildStrategy(ctx context.Context, n string) error {
-	list := &build.ClusterBuildStrategyList{}
-
-	if err := r.client.List(ctx, list); err != nil {
-		return errors.Wrapf(err, "listing ClusterBuildStrategies failed")
-	}
-
-	if len(list.Items) == 0 {
-		return errors.Errorf("no ClusterBuildStrategies found")
-	}
-
-	if len(list.Items) > 0 {
-		for _, s := range list.Items {
-			if s.Name == n {
-				return nil
-			}
-		}
-		return fmt.Errorf("clusterBuildStrategy %s does not exist", n)
+func (r *ReconcileBuild) validateClusterBuildStrategy(ctx context.Context, strategyName string, b *build.Build) error {
+	clusterBuildStrategy := &build.ClusterBuildStrategy{}
+	if err := r.client.Get(ctx, types.NamespacedName{Name: strategyName}, clusterBuildStrategy); err != nil && !apierrors.IsNotFound(err) {
+		return err
+	} else if apierrors.IsNotFound(err) {
+		MarkBuildStatus(b, build.ClusterBuildStrategyNotFound, fmt.Sprintf("clusterBuildStrategy %s does not exist", b.Spec.StrategyRef.Name))
 	}
 	return nil
 }
-func (r *ReconcileBuild) validateSecrets(ctx context.Context, secretNames []string, ns string) error {
-	list := &corev1.SecretList{}
-
-	if err := r.client.List(
-		ctx,
-		list,
-		&client.ListOptions{
-			Namespace: ns,
-		},
-	); err != nil {
-		return errors.Wrapf(err, "listing secrets in namespace %s failed", ns)
-	}
 
-	if len(list.Items) == 0 {
-		return errors.Errorf("there are no secrets in namespace %s", ns)
-	}
+func (r *ReconcileBuild) validateSecrets(ctx context.Context, secretNames map[string]build.BuildReason, b *build.Build) error {
 
-	var lookUp = map[string]bool{}
-	for _, secretName := range secretNames {
-		lookUp[secretName] = false
-	}
-	for _, secret := range list.Items {
-		lookUp[secret.Name] = true
-	}
 	var missingSecrets []string
-	for name, found := range lookUp {
-		if !found {
-			missingSecrets = append(missingSecrets, name)
+	secret := &corev1.Secret{}
+	for refSecret, secretType := range secretNames {
+		if err := r.client.Get(ctx, types.NamespacedName{Name: refSecret, Namespace: b.Namespace}, secret); err != nil && !apierrors.IsNotFound(err) {
+			return err
+		} else if apierrors.IsNotFound(err) {
+			MarkBuildStatus(b, secretType, fmt.Sprintf("referenced secret %s not found", refSecret))
+			missingSecrets = append(missingSecrets, refSecret)
 		}
 	}
 
 	if len(missingSecrets) > 1 {
-		return fmt.Errorf("secrets %s do not exist", strings.Join(missingSecrets, ", "))
-	} else if len(missingSecrets) > 0 {
-		return fmt.Errorf("secret %s does not exist", missingSecrets[0])
+		MarkBuildStatus(b, build.MultipleSecretRefNotFound, fmt.Sprintf("missing secrets are %s", strings.Join(missingSecrets, ",")))
 	}
 
 	return nil
@@ -436,7 +398,7 @@ func (r *ReconcileBuild) validateBuildRunOwnerReferences(ctx context.Context, b
 		for _, buildRun := range buildRunList.Items {
 			if index := r.validateBuildOwnerReference(buildRun.OwnerReferences, b); index == -1 {
 				if err := r.setOwnerReferenceFunc(b, &buildRun, r.scheme); err != nil {
-					b.Status.Reason = fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err)
+					MarkBuildStatus(b, build.SetOwnerReferenceFailed, fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err))
 					if err := r.client.Status().Update(ctx, b); err != nil {
 						return err
 					}
@@ -520,3 +482,9 @@ func buildSecretRefAnnotationExist(annotation map[string]string) (string, bool)
 	}
 	return "", false
 }
+
+// MarkBuildStatus sets the Build Status fields
+func MarkBuildStatus(b *build.Build, reason build.BuildReason, msg string) {
+	b.Status.Reason = reason
+	b.Status.Message = msg
+}

pkg/controller/buildrun/buildrun_controller.go

@@ -285,7 +285,8 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 			// Set OwnerReference for Build and BuildRun only when build.build.dev/build-run-deletion is set "true"
 			if build.GetAnnotations()[buildv1alpha1.AnnotationBuildRunDeletion] == "true" && !isOwnedByBuild(build, buildRun.OwnerReferences) {
 				if err := r.setOwnerReferenceFunc(build, buildRun, r.scheme); err != nil {
-					build.Status.Reason = fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err)
+					build.Status.Reason = buildv1alpha1.SetOwnerReferenceFailed
+					build.Status.Message = fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err)
 					if err := r.client.Status().Update(ctx, build); err != nil {
 						return reconcile.Result{}, err
 					}