Skip to content

Commit

Permalink
Insist on exactly one source spec
Browse files Browse the repository at this point in the history 
Previously, an if-then-else checked for the presence of source
specifications (GitSource and FluxSource), and this made it possible to
supply a stack object with both kinds of source (though only the first
checked -- GitSource -- would be used).

This commit makes that bit of code enforce the mutual exclusion of
sources.

Signed-off-by: Michael Bridgen <[email protected]>
  • Loading branch information
@squaremo
squaremo committed Oct 11, 2022
1 parent ed67b42 commit fc4f6fc
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 33 deletions.
30 changes: 16 additions & 14 deletions pkg/controller/stack/stack_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func isStalledError(e error) bool {
}

var errNamespaceIsolation = newStallErrorf(`refs are constrained to the object's namespace unless %s is set`, EnvInsecureNoNamespaceIsolation)
var errNoSourceSpecified = newStallErrorf(`a source for the stack must be specified`)
var errOtherThanOneSourceSpecified = newStallErrorf(`exactly one source (.spec.fluxSource or .spec.projectRepo) for the stack must be given`)

// Reconcile reads that state of the cluster for a Stack object and makes changes based on the state read
// and what is in the Stack.Spec
Expand Down Expand Up @@ -261,16 +261,18 @@ func (r *ReconcileStack) Reconcile(ctx context.Context, request reconcile.Reques
return reconcile.Result{}, err
}

// these are used in status, and are set from some property of the source, whether it's the
// actual commit, or some analogue.
// This value is reported in .status, and is set from some property of the source -- whether
// it's the actual commit, or some analogue.
var currentCommit string

// Step 1. Set up the workdir, select the right stack and populate config if supplied.

// Check which kind of source we have.
if stack.GitSource != nil {
gitSource, fluxSource := stack.GitSource, stack.FluxSource
switch {
case gitSource != nil && fluxSource == nil:
// Validate that there is enough specified to be able to clone the git repo.
if stack.GitSource.ProjectRepo == "" || (stack.GitSource.Commit == "" && stack.GitSource.Branch == "") {
if gitSource.ProjectRepo == "" || (gitSource.Commit == "" && gitSource.Branch == "") {

msg := "Stack git source needs to specify 'projectRepo' and either 'branch' or 'commit'"
r.emitEvent(instance, pulumiv1.StackConfigInvalidEvent(), msg)
Expand All @@ -282,7 +284,7 @@ func (r *ReconcileStack) Reconcile(ctx context.Context, request reconcile.Reques
return reconcile.Result{}, nil
}

gitAuth, err := sess.SetupGitAuth(ctx)
gitAuth, err := sess.SetupGitAuth(ctx) // TODO be more explicit about what's being fed in here
if err != nil {
r.emitEvent(instance, pulumiv1.StackGitAuthFailureEvent(), "Failed to setup git authentication: %v", err.Error())
reqLogger.Error(err, "Failed to setup git authentication", "Stack.Name", stack.Stack)
Expand All @@ -297,7 +299,7 @@ func (r *ReconcileStack) Reconcile(ctx context.Context, request reconcile.Reques
sess.addSSHKeysToKnownHosts(sess.stack.ProjectRepo)
}

if currentCommit, err = sess.SetupWorkdirFromGitSource(ctx, gitAuth, stack.GitSource); err != nil {
if currentCommit, err = sess.SetupWorkdirFromGitSource(ctx, gitAuth, gitSource); err != nil {
r.emitEvent(instance, pulumiv1.StackInitializationFailureEvent(), "Failed to initialize stack: %v", err.Error())
reqLogger.Error(err, "Failed to setup Pulumi workdir", "Stack.Name", stack.Stack)
r.markStackFailed(sess, instance, err, "", "")
Expand All @@ -309,12 +311,12 @@ func (r *ReconcileStack) Reconcile(ctx context.Context, request reconcile.Reques
// this can fail for reasons which might go away without intervention; so, retry explicitly
return reconcile.Result{Requeue: true}, nil
}
} else if source := stack.FluxSource; source != nil {
case gitSource == nil && fluxSource != nil:
var sourceObject unstructured.Unstructured
sourceObject.SetAPIVersion(source.SourceRef.APIVersion)
sourceObject.SetKind(source.SourceRef.Kind)
sourceObject.SetAPIVersion(fluxSource.SourceRef.APIVersion)
sourceObject.SetKind(fluxSource.SourceRef.Kind)
if err := r.client.Get(ctx, client.ObjectKey{
Name: source.SourceRef.Name,
Name: fluxSource.SourceRef.Name,
Namespace: request.Namespace,
}, &sourceObject); err != nil {
r.markStackFailed(sess, instance, err, "", "")
Expand All @@ -331,7 +333,7 @@ func (r *ReconcileStack) Reconcile(ctx context.Context, request reconcile.Reques
return reconcile.Result{Requeue: true}, nil
}

currentCommit, err = sess.SetupWorkdirFromFluxSource(ctx, sourceObject, stack.FluxSource)
currentCommit, err = sess.SetupWorkdirFromFluxSource(ctx, sourceObject, fluxSource)
if err != nil {
r.emitEvent(instance, pulumiv1.StackInitializationFailureEvent(), "Failed to initialize stack: %v", err.Error())
reqLogger.Error(err, "Failed to setup Pulumi workdir", "Stack.Name", stack.Stack)
Expand All @@ -344,8 +346,8 @@ func (r *ReconcileStack) Reconcile(ctx context.Context, request reconcile.Reques
// this can fail for reasons which might go away without intervention; so, retry explicitly
return reconcile.Result{Requeue: true}, nil
}
} else { // no source specified
err := errNoSourceSpecified
default:
err := errOtherThanOneSourceSpecified
r.markStackFailed(sess, instance, err, "", "")
instance.Status.MarkStalledCondition(pulumiv1.StalledSpecInvalidReason, err.Error())
return reconcile.Result{}, nil
Expand Down
60 changes: 41 additions & 19 deletions test/invalid_spec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ package tests
import (
"context"

// "github.com/pulumi/pulumi-kubernetes-operator/pkg/apis/pulumi/shared"
"github.com/pulumi/pulumi-kubernetes-operator/pkg/apis/pulumi/shared"
pulumiv1 "github.com/pulumi/pulumi-kubernetes-operator/pkg/apis/pulumi/v1"

. "github.com/onsi/ginkgo"
Expand All @@ -15,28 +15,19 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

var _ = Describe("Stacks that should stall", func() {

var (
stack pulumiv1.Stack
)

AfterEach(func() {
if stack.Name != "" { // assume that if it's been named, it was created in the cluster
Expect(k8sClient.Delete(context.TODO(), &stack)).To(Succeed())
}
})

When("there is no source specified", func() {
func checkInvalidSpecStalls(when string, setup func(stack *pulumiv1.Stack)) {
When(when, func() {
var stack pulumiv1.Stack

BeforeEach(func() {
stack = pulumiv1.Stack{}
stack.Name = randString()
stack.Namespace = "default"
setup(&stack)
Expect(k8sClient.Create(context.TODO(), &stack)).To(Succeed())
})

It("should mark a stack as stalled", func() {
It("should mark the stack as stalled", func() {
// wait until the controller has seen the stack object and completed processing it
var s pulumiv1.Stack
Eventually(func() bool {
Expand All @@ -49,12 +40,43 @@ var _ = Describe("Stacks that should stall", func() {
}
return s.Status.ObservedGeneration == s.Generation
}, "20s", "1s").Should(BeTrue())

Expect(apimeta.IsStatusConditionTrue(s.Status.Conditions, pulumiv1.ReadyCondition)).To(BeFalse())
Expect(apimeta.FindStatusCondition(s.Status.Conditions, pulumiv1.ReconcilingCondition)).To(BeNil())
Expect(s.Status.LastUpdate).ToNot(BeNil(), ".status.lastUpdate is recorded")
Expect(s.Status.LastUpdate.State).To(Equal(shared.FailedStackStateMessage))
stalledCondition := apimeta.FindStatusCondition(s.Status.Conditions, pulumiv1.StalledCondition)
Expect(stalledCondition).ToNot(BeNil())
Expect(stalledCondition).ToNot(BeNil(), "stalled condition is present")
Expect(stalledCondition.Reason).To(Equal(pulumiv1.StalledSpecInvalidReason))
// not ready, and not in progress
Expect(apimeta.IsStatusConditionTrue(s.Status.Conditions, pulumiv1.ReadyCondition)).To(BeFalse(), "ready condition is false")
Expect(apimeta.FindStatusCondition(s.Status.Conditions, pulumiv1.ReconcilingCondition)).To(BeNil(), "reconciling condition is absent")
})

AfterEach(func() {
if stack.Name != "" { // assume that if it's been named, it was created in the cluster
Expect(k8sClient.Delete(context.TODO(), &stack)).To(Succeed())
}
})
})
}

var _ = Describe("Stacks that should stall because of an invalid spec", func() {

checkInvalidSpecStalls("there is no source specified", func(*pulumiv1.Stack) {})

checkInvalidSpecStalls("a git source with no branch and no commit hash is given", func(s *pulumiv1.Stack) {
s.Spec.GitSource = &shared.GitSource{
ProjectRepo: "https://github.com/pulumi/pulumi-kubernetes-operator",
}
})

checkInvalidSpecStalls("more than one source is given", func(s *pulumiv1.Stack) {
s.Spec.GitSource = &shared.GitSource{
ProjectRepo: "https://github.com/pulumi/pulumi-kubernetes-operator",
Branch: "default",
}
s.Spec.FluxSource = &shared.FluxSource{
SourceRef: shared.FluxSourceReference{
Name: "foo",
},
}
})
})

0 comments on commit fc4f6fc

Please sign in to comment.