Skip to content

Commit

Permalink
Merge pull request #1534 from shipwright-io/fix/issue-1531
Browse files Browse the repository at this point in the history
Fix unknown strategy kind reconcile issue
  • Loading branch information
openshift-merge-bot[bot] authored Mar 13, 2024
2 parents 6bb032d + 1a6eb01 commit 8e46bc8
Show file tree
Hide file tree
Showing 8 changed files with 293 additions and 57 deletions.
2 changes: 2 additions & 0 deletions pkg/apis/build/v1beta1/build_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
36 changes: 30 additions & 6 deletions pkg/reconciler/build/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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() {
Expand Down Expand Up @@ -438,6 +461,7 @@ var _ = Describe("Reconcile Build", func() {
return nil
})
})

It("fails when the name is blank", func() {
buildSample.Spec.Env = []corev1.EnvVar{
{
Expand Down
44 changes: 43 additions & 1 deletion pkg/reconciler/buildrun/buildrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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())
})
})
})

Expand Down
94 changes: 47 additions & 47 deletions pkg/validate/strategies.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
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
}
}

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)
buildStrategy := &build.BuildStrategy{}
strategyExists, err = s.validateBuildStrategy(ctx, s.Build.Spec.Strategy.Name, buildStrategy)
builderStrategy = buildStrategy
return build.NamespacedBuildStrategyKind
}

if err != nil {
return err
return *s.Build.Spec.Strategy.Kind
}

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 strategyExists {
s.validateBuildParams(builderStrategy.GetParameters())
s.validateBuildVolumes(builderStrategy.GetVolumes())
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", strategyName, s.Build.Namespace))
return nil
}

return nil
return err
}

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) {
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
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
}
return true, nil
}

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) {
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
s.Build.Status.Message = pointer.String(fmt.Sprintf("clusterBuildStrategy %s does not exist", strategyName))
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)
Expand All @@ -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)
Expand Down
Loading

0 comments on commit 8e46bc8

Please sign in to comment.