Fix unknown strategy kind reconcile issue

Ref: #1531

Set status when strategy kind is not one of the supported ones.

Add test cases to strategy validation.

Refactor strategy validation code to be more compact and readable.

Signed-off-by: Matthias Diester <[email protected]>

pkg/apis/build/v1beta1/build_types.go

@@ -16,6 +16,8 @@ type BuildReason string
 const (
 	// SucceedStatus indicates that all validations Succeeded
 	SucceedStatus BuildReason = "Succeeded"
+	// UnknownBuildStrategyKind indicates that neither namespace-scope or cluster-scope strategy kind was used
+	UnknownBuildStrategyKind BuildReason = "UnknownBuildStrategyKind"
 	// 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

pkg/reconciler/build/build_test.go

@@ -134,6 +134,7 @@ var _ = Describe("Reconcile Build", func() {
 				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
 			})
+
 			It("succeed when the secret exists", func() {
 				// Fake some client Get calls and ensure we populate all
 				// different resources we could get during reconciliation
@@ -176,7 +177,6 @@ var _ = Describe("Reconcile Build", func() {
 
 		Context("when spec strategy ClusterBuildStrategy is specified", func() {
 			It("fails when the strategy does not exists", func() {
-
 				// Fake some client Get calls and ensure we populate all
 				// different resources we could get during reconciliation
 				client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error {
@@ -199,8 +199,8 @@ var _ = Describe("Reconcile Build", func() {
 				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
 			})
-			It("succeed when the strategy exists", func() {
 
+			It("succeed when the strategy exists", func() {
 				// Fake some client Get calls and ensure we populate all
 				// different resources we could get during reconciliation
 				client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error {
@@ -235,16 +235,15 @@ var _ = Describe("Reconcile Build", func() {
 			})
 
 			It("fails when the strategy does not exists", func() {
-
 				statusCall := ctl.StubFunc(corev1.ConditionFalse, build.BuildStrategyNotFound, fmt.Sprintf("buildStrategy %s does not exist in namespace %s", buildStrategyName, namespace))
 				statusWriter.UpdateCalls(statusCall)
 
 				_, err := reconciler.Reconcile(context.TODO(), request)
 				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
 			})
-			It("succeed when the strategy exists", func() {
 
+			It("succeed when the strategy exists", func() {
 				// Fake some client Get calls and ensure we populate all
 				// different resources we could get during reconciliation
 				client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error {
@@ -275,16 +274,16 @@ var _ = Describe("Reconcile Build", func() {
 				// Override the buildSample to use a BuildStrategy instead of the Cluster one, although the build strategy kind is nil
 				buildSample = ctl.BuildWithNilBuildStrategyKind(buildName, namespace, buildStrategyName)
 			})
-			It("default to BuildStrategy and fails when the strategy does not exists", func() {
 
+			It("default to BuildStrategy and fails when the strategy does not exists", func() {
 				statusCall := ctl.StubFunc(corev1.ConditionFalse, build.BuildStrategyNotFound, fmt.Sprintf("buildStrategy %s does not exist in namespace %s", buildStrategyName, namespace))
 				statusWriter.UpdateCalls(statusCall)
 
 				_, err := reconciler.Reconcile(context.TODO(), request)
 				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
-
 			})
+
 			It("default to BuildStrategy and succeed if the strategy exists", func() {
 				// Fake some client Get calls and ensure we populate all
 				// different resources we could get during reconciliation
@@ -309,6 +308,30 @@ var _ = Describe("Reconcile Build", func() {
 			})
 		})
 
+		Context("when spec strategy kind is unknown", func() {
+			JustBeforeEach(func() {
+				buildStrategyKind := build.BuildStrategyKind("abc")
+				buildStrategyName = "xyz"
+				buildName = "build-name"
+				buildSample = ctl.BuildWithNilBuildStrategyKind(buildName, namespace, buildStrategyName)
+				buildSample.Spec.Strategy.Kind = &buildStrategyKind
+			})
+
+			It("should fail validation and update the status to indicate that the strategy kind is unknown", func() {
+				statusWriter.UpdateCalls(func(ctx context.Context, o crc.Object, sruo ...crc.SubResourceUpdateOption) error {
+					Expect(o).To(BeAssignableToTypeOf(&build.Build{}))
+					b := o.(*build.Build)
+					Expect(b.Status.Reason).ToNot(BeNil())
+					Expect(*b.Status.Reason).To(Equal(build.UnknownBuildStrategyKind))
+					return nil
+				})
+
+				_, err := reconciler.Reconcile(context.TODO(), request)
+				Expect(err).To(BeNil())
+				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
+			})
+		})
+
 		Context("when source URL is specified", func() {
 			// validate file protocol
 			It("fails when source URL is invalid", func() {
@@ -438,6 +461,7 @@ var _ = Describe("Reconcile Build", func() {
 					return nil
 				})
 			})
+
 			It("fails when the name is blank", func() {
 				buildSample.Spec.Env = []corev1.EnvVar{
 					{

pkg/reconciler/buildrun/buildrun_test.go

@@ -1036,7 +1036,7 @@ var _ = Describe("Reconcile BuildRun", func() {
 					emptyTaskRunName,
 					build.Condition{
 						Type:   build.Succeeded,
-						Reason: "BuildRegistrationFailed",
+						Reason: resources.ConditionBuildRegistrationFailed,
 						Status: corev1.ConditionFalse,
 					},
 					corev1.ConditionFalse,
@@ -1492,6 +1492,48 @@ var _ = Describe("Reconcile BuildRun", func() {
 					Expect(statusWriter.UpdateCallCount()).To(Equal(2))
 					Expect(taskRunCreates).To(Equal(1))
 				})
+
+				It("should validate the embedded BuildSpec to identify that the strategy kind is unknown", func() {
+					client.GetCalls(func(_ context.Context, nn types.NamespacedName, o crc.Object, _ ...crc.GetOption) error {
+						switch object := o.(type) {
+						case *build.BuildRun:
+							(&build.BuildRun{
+								ObjectMeta: metav1.ObjectMeta{Name: buildRunName},
+								Spec: build.BuildRunSpec{
+									Build: build.ReferencedBuild{
+										Spec: &build.BuildSpec{
+											Source: &build.Source{
+												Type: build.GitType,
+												Git:  &build.Git{URL: "https://github.com/shipwright-io/sample-go.git"},
+											},
+											Strategy: build.Strategy{
+												Kind: (*build.BuildStrategyKind)(pointer.String("foo")), // problematic value
+												Name: strategyName,
+											},
+											Output: build.Image{Image: "foo/bar:latest"},
+										},
+									},
+								},
+							}).DeepCopyInto(object)
+							return nil
+						}
+
+						return k8serrors.NewNotFound(schema.GroupResource{}, nn.Name)
+					})
+
+					statusWriter.UpdateCalls(func(ctx context.Context, o crc.Object, sruo ...crc.SubResourceUpdateOption) error {
+						Expect(o).To(BeAssignableToTypeOf(&build.BuildRun{}))
+						condition := o.(*build.BuildRun).Status.GetCondition(build.Succeeded)
+						Expect(condition.Status).To(Equal(corev1.ConditionFalse))
+						Expect(condition.Reason).To(Equal(resources.ConditionBuildRegistrationFailed))
+						Expect(condition.Message).To(ContainSubstring("unknown strategy kind"))
+						return nil
+					})
+
+					_, err := reconciler.Reconcile(context.TODO(), buildRunRequest)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(statusWriter.UpdateCallCount()).ToNot(BeZero())
+				})
 			})
 		})
 

pkg/validate/strategies.go

@@ -32,69 +32,70 @@ func NewStrategies(client client.Client, build *build.Build) *Strategy {
 // that the referenced strategy exists. This applies to both
 // namespaced or cluster scoped strategies
 func (s Strategy) ValidatePath(ctx context.Context) error {
-	var (
-		builderStrategy build.BuilderStrategy
-		strategyExists  bool
-		err             error
-	)
-
-	if s.Build.Spec.Strategy.Kind != nil {
-		switch *s.Build.Spec.Strategy.Kind {
-		case build.NamespacedBuildStrategyKind:
-			buildStrategy := &build.BuildStrategy{}
-			strategyExists, err = s.validateBuildStrategy(ctx, s.Build.Spec.Strategy.Name, buildStrategy)
-			builderStrategy = buildStrategy
-		case build.ClusterBuildStrategyKind:
-			clusterBuildStrategy := &build.ClusterBuildStrategy{}
-			strategyExists, err = s.validateClusterBuildStrategy(ctx, s.Build.Spec.Strategy.Name, clusterBuildStrategy)
-			builderStrategy = clusterBuildStrategy
-		default:
-			return fmt.Errorf("unknown strategy kind: %v", *s.Build.Spec.Strategy.Kind)
-		}
-	} else {
-		ctxlog.Info(ctx, "buildStrategy kind is nil, use default NamespacedBuildStrategyKind", namespace, s.Build.Namespace, name, s.Build.Name)
-		buildStrategy := &build.BuildStrategy{}
-		strategyExists, err = s.validateBuildStrategy(ctx, s.Build.Spec.Strategy.Name, buildStrategy)
-		builderStrategy = buildStrategy
-	}
-
-	if err != nil {
-		return err
+	switch s.kind(ctx) {
+	case build.NamespacedBuildStrategyKind:
+		return s.validateBuildStrategy(ctx, s.Build.Spec.Strategy.Name)
+
+	case build.ClusterBuildStrategyKind:
+		return s.validateClusterBuildStrategy(ctx, s.Build.Spec.Strategy.Name)
+
+	default:
+		s.Build.Status.Reason = build.BuildReasonPtr(build.UnknownBuildStrategyKind)
+		s.Build.Status.Message = pointer.String(fmt.Sprintf("unknown strategy kind %s used, must be one of %s, or %s",
+			*s.Build.Spec.Strategy.Kind,
+			build.NamespacedBuildStrategyKind,
+			build.ClusterBuildStrategyKind))
+		return nil
 	}
+}
 
-	if strategyExists {
-		s.validateBuildParams(builderStrategy.GetParameters())
-		s.validateBuildVolumes(builderStrategy.GetVolumes())
+func (s Strategy) kind(ctx context.Context) build.BuildStrategyKind {
+	if s.Build.Spec.Strategy.Kind == nil {
+		ctxlog.Info(ctx, "buildStrategy kind is nil, use default NamespacedBuildStrategyKind", namespace, s.Build.Namespace, name, s.Build.Name)
+		return build.NamespacedBuildStrategyKind
 	}
 
-	return nil
+	return *s.Build.Spec.Strategy.Kind
 }
 
-func (s Strategy) validateBuildStrategy(ctx context.Context, strategyName string, buildStrategy *build.BuildStrategy) (bool, error) {
-	if err := s.Client.Get(ctx, types.NamespacedName{Name: strategyName, Namespace: s.Build.Namespace}, buildStrategy); err != nil && !apierrors.IsNotFound(err) {
-		return false, err
-	} else if apierrors.IsNotFound(err) {
+func (s Strategy) validateBuildStrategy(ctx context.Context, strategyName string) error {
+	buildStrategy := &build.BuildStrategy{}
+	err := s.Client.Get(ctx, types.NamespacedName{Name: strategyName, Namespace: s.Build.Namespace}, buildStrategy)
+	if err == nil {
+		s.validateBuildParams(buildStrategy.GetParameters())
+		s.validateBuildVolumes(buildStrategy.GetVolumes())
+		return nil
+	}
+
+	if apierrors.IsNotFound(err) {
 		s.Build.Status.Reason = build.BuildReasonPtr(build.BuildStrategyNotFound)
 		s.Build.Status.Message = pointer.String(fmt.Sprintf("buildStrategy %s does not exist in namespace %s", s.Build.Spec.Strategy.Name, s.Build.Namespace))
-		return false, nil
+		return nil
 	}
-	return true, nil
+
+	return err
 }
 
-func (s Strategy) validateClusterBuildStrategy(ctx context.Context, strategyName string, clusterBuildStrategy *build.ClusterBuildStrategy) (bool, error) {
-	if err := s.Client.Get(ctx, types.NamespacedName{Name: strategyName}, clusterBuildStrategy); err != nil && !apierrors.IsNotFound(err) {
-		return false, err
-	} else if apierrors.IsNotFound(err) {
+func (s Strategy) validateClusterBuildStrategy(ctx context.Context, strategyName string) error {
+	clusterBuildStrategy := &build.ClusterBuildStrategy{}
+	err := s.Client.Get(ctx, types.NamespacedName{Name: strategyName}, clusterBuildStrategy)
+	if err == nil {
+		s.validateBuildParams(clusterBuildStrategy.GetParameters())
+		s.validateBuildVolumes(clusterBuildStrategy.GetVolumes())
+		return nil
+	}
+
+	if apierrors.IsNotFound(err) {
 		s.Build.Status.Reason = build.BuildReasonPtr(build.ClusterBuildStrategyNotFound)
 		s.Build.Status.Message = pointer.String(fmt.Sprintf("clusterBuildStrategy %s does not exist", s.Build.Spec.Strategy.Name))
-		return false, nil
+		return nil
 	}
-	return true, nil
+
+	return err
 }
 
 func (s Strategy) validateBuildParams(parameterDefinitions []build.Parameter) {
 	valid, reason, message := BuildParameters(parameterDefinitions, s.Build.Spec.ParamValues)
-
 	if !valid {
 		s.Build.Status.Reason = build.BuildReasonPtr(reason)
 		s.Build.Status.Message = pointer.String(message)
@@ -103,7 +104,6 @@ func (s Strategy) validateBuildParams(parameterDefinitions []build.Parameter) {
 
 func (s Strategy) validateBuildVolumes(strategyVolumes []build.BuildStrategyVolume) {
 	valid, reason, message := BuildVolumes(strategyVolumes, s.Build.Spec.Volumes)
-
 	if !valid {
 		s.Build.Status.Reason = build.BuildReasonPtr(reason)
 		s.Build.Status.Message = pointer.String(message)