diff --git a/pkg/apis/build/v1beta1/build_types.go b/pkg/apis/build/v1beta1/build_types.go index 0d233f0c50..2f7e95f87c 100644 --- a/pkg/apis/build/v1beta1/build_types.go +++ b/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 diff --git a/pkg/reconciler/build/build_test.go b/pkg/reconciler/build/build_test.go index bd4a14ac15..6222f3e83c 100644 --- a/pkg/reconciler/build/build_test.go +++ b/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,7 +235,6 @@ 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) @@ -243,8 +242,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 { @@ -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{ { diff --git a/pkg/validate/strategies.go b/pkg/validate/strategies.go index 3ecbcc2706..5bbc99541d 100644 --- a/pkg/validate/strategies.go +++ b/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) diff --git a/pkg/validate/strategies_test.go b/pkg/validate/strategies_test.go new file mode 100644 index 0000000000..0ad9ad651e --- /dev/null +++ b/pkg/validate/strategies_test.go @@ -0,0 +1,167 @@ +// Copyright The Shipwright Contributors +// +// SPDX-License-Identifier: Apache-2.0 + +package validate_test + +import ( + "context" + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/shipwright-io/build/pkg/controller/fakes" + . "github.com/shipwright-io/build/pkg/validate" + + build "github.com/shipwright-io/build/pkg/apis/build/v1beta1" + + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + crc "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ = Describe("BuildStrategy", func() { + var ctx context.Context + var client *fakes.FakeClient + + BeforeEach(func() { + ctx = context.TODO() + client = &fakes.FakeClient{} + }) + + var sampleBuild = func(kind build.BuildStrategyKind, name string) *build.Build { + return &build.Build{ + Spec: build.BuildSpec{ + Strategy: build.Strategy{ + Kind: &kind, + Name: name, + }, + }, + } + } + + Context("namespaced build strategy is used", func() { + It("should pass when the referenced build strategy exists", func() { + sample := sampleBuild(build.NamespacedBuildStrategyKind, "buildkit") + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error { + switch object := object.(type) { + case *build.BuildStrategy: + (&build.BuildStrategy{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: nn.Namespace, + Name: nn.Name}, + }).DeepCopyInto(object) + return nil + } + + return errors.NewNotFound(schema.GroupResource{}, "schema not found") + }) + + Expect(NewStrategies(client, sample).ValidatePath(ctx)).To(Succeed()) + Expect(sample.Status.Reason).To(BeNil()) + }) + + It("should fail when the referenced build strategy does not exists", func() { + sample := sampleBuild(build.NamespacedBuildStrategyKind, "buildkit") + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error { + return errors.NewNotFound(schema.GroupResource{}, "schema not found") + }) + + Expect(NewStrategies(client, sample).ValidatePath(ctx)).To(Succeed()) + Expect(*sample.Status.Reason).To(Equal(build.BuildStrategyNotFound)) + }) + + It("should error when there is an unexpected result", func() { + sample := sampleBuild(build.NamespacedBuildStrategyKind, "buildkit") + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error { + return errors.NewInternalError(fmt.Errorf("monkey wrench")) + }) + + Expect(NewStrategies(client, sample).ValidatePath(ctx)).ToNot(Succeed()) + }) + }) + + Context("cluster build strategy is used", func() { + It("should pass when the referenced build strategy exists", func() { + sample := sampleBuild(build.ClusterBuildStrategyKind, "buildkit") + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error { + switch object := object.(type) { + case *build.ClusterBuildStrategy: + (&build.ClusterBuildStrategy{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: nn.Namespace, + Name: nn.Name}, + }).DeepCopyInto(object) + return nil + } + + return errors.NewNotFound(schema.GroupResource{}, "schema not found") + }) + + Expect(NewStrategies(client, sample).ValidatePath(ctx)).To(Succeed()) + Expect(sample.Status.Reason).To(BeNil()) + }) + + It("should fail when the referenced build strategy does not exists", func() { + sample := sampleBuild(build.ClusterBuildStrategyKind, "buildkit") + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error { + return errors.NewNotFound(schema.GroupResource{}, "schema not found") + }) + + Expect(NewStrategies(client, sample).ValidatePath(ctx)).To(Succeed()) + Expect(*sample.Status.Reason).To(Equal(build.ClusterBuildStrategyNotFound)) + }) + + It("should error when there is an unexpected result", func() { + sample := sampleBuild(build.ClusterBuildStrategyKind, "buildkit") + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error { + return errors.NewInternalError(fmt.Errorf("monkey wrench")) + }) + + Expect(NewStrategies(client, sample).ValidatePath(ctx)).ToNot(Succeed()) + }) + }) + + Context("edge cases", func() { + It("should default to namespace build strategy when kind is nil", func() { + sample := &build.Build{ + Spec: build.BuildSpec{ + Strategy: build.Strategy{ + Kind: nil, + Name: "foobar", + }, + }, + } + + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error { + switch object := object.(type) { + case *build.BuildStrategy: + (&build.BuildStrategy{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: nn.Namespace, + Name: nn.Name}, + }).DeepCopyInto(object) + return nil + } + + return errors.NewNotFound(schema.GroupResource{}, "schema not found") + }) + + Expect(NewStrategies(client, sample).ValidatePath(ctx)).To(Succeed()) + Expect(sample.Status.Reason).To(BeNil()) + }) + + It("should fail validation if the strategy kind is unknown", func() { + sample := sampleBuild("abc", "xyz") + client.GetCalls(func(_ context.Context, nn types.NamespacedName, object crc.Object, getOptions ...crc.GetOption) error { + return errors.NewNotFound(schema.GroupResource{}, "schema not found") + }) + + Expect(NewStrategies(client, sample).ValidatePath(ctx)).To(Succeed()) + Expect(*sample.Status.Reason).To(Equal(build.UnknownBuildStrategyKind)) + }) + }) +})