From fc4f6fce787390d90af098facdad90083e7c4484 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Tue, 11 Oct 2022 11:30:04 +0100 Subject: [PATCH] Insist on exactly one source spec 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 --- pkg/controller/stack/stack_controller.go | 30 ++++++------ test/invalid_spec_test.go | 60 ++++++++++++++++-------- 2 files changed, 57 insertions(+), 33 deletions(-) diff --git a/pkg/controller/stack/stack_controller.go b/pkg/controller/stack/stack_controller.go index 5261d443..25915767 100644 --- a/pkg/controller/stack/stack_controller.go +++ b/pkg/controller/stack/stack_controller.go @@ -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 @@ -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) @@ -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) @@ -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, "", "") @@ -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, "", "") @@ -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) @@ -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 diff --git a/test/invalid_spec_test.go b/test/invalid_spec_test.go index 27b0b04d..3e47a6a9 100644 --- a/test/invalid_spec_test.go +++ b/test/invalid_spec_test.go @@ -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" @@ -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 { @@ -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", + }, + } + }) })