feat(controller)!: assign a default alias to promotion steps that hav…

…e none (akuity#2881)

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

internal/directives/simple_engine.go

@@ -5,6 +5,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"regexp"
+	"strings"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -13,6 +15,10 @@ import (
 	"github.com/akuity/kargo/internal/credentials"
 )
 
+// ReservedStepAliasRegex is a regular expression that matches step aliases that
+// are reserved for internal use.
+var ReservedStepAliasRegex = regexp.MustCompile(`^step-\d+$`)
+
 // SimpleEngine is a simple engine that executes a list of PromotionSteps in
 // sequence.
 type SimpleEngine struct {
@@ -87,6 +93,20 @@ func (e *SimpleEngine) Promote(
 
 		stateCopy := state.DeepCopy()
 
+		step.Alias = strings.TrimSpace(step.Alias)
+		if step.Alias == "" {
+			step.Alias = fmt.Sprintf("step-%d", i)
+		} else if ReservedStepAliasRegex.MatchString(step.Alias) {
+			// A webhook enforces this regex as well, but we're checking here to
+			// account for the possibility of EXISTING Stages with a promotionTemplate
+			// containing a step with a now-reserved alias.
+			return PromotionResult{
+				Status:      kargoapi.PromotionPhaseErrored,
+				CurrentStep: i,
+				State:       state,
+			}, fmt.Errorf("step alias %q is forbidden", step.Alias)
+		}
+
 		stepCtx := &PromotionStepContext{
 			UIBaseURL:       promoCtx.UIBaseURL,
 			WorkDir:         workDir,
@@ -111,9 +131,7 @@ func (e *SimpleEngine) Promote(
 		}
 
 		result, err := reg.Runner.RunPromotionStep(ctx, stepCtx)
-		if step.Alias != "" {
-			state[step.Alias] = result.Output
-		}
+		state[step.Alias] = result.Output
 		if err != nil {
 			return PromotionResult{
 					Status:      kargoapi.PromotionPhaseErrored,

internal/webhook/stage/webhook.go

@@ -3,6 +3,7 @@ package stage
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	admissionv1 "k8s.io/api/admission/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -14,6 +15,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
+	"github.com/akuity/kargo/internal/directives"
 	libWebhook "github.com/akuity/kargo/internal/webhook"
 )
 
@@ -194,7 +196,11 @@ func (w *webhook) validateSpec(
 	if spec == nil { // nil spec is caught by declarative validations
 		return nil
 	}
-	return w.validateRequestedFreight(f.Child("requestedFreight"), spec.RequestedFreight)
+	errs := w.validateRequestedFreight(f.Child("requestedFreight"), spec.RequestedFreight)
+	return append(
+		errs,
+		w.ValidatePromotionTemplate(f.Child("promotionTemplate"), spec.PromotionTemplate)...,
+	)
 }
 
 func (w *webhook) validateRequestedFreight(
@@ -221,3 +227,24 @@ func (w *webhook) validateRequestedFreight(
 	}
 	return nil
 }
+
+func (w *webhook) ValidatePromotionTemplate(
+	f *field.Path,
+	promoTemplate *kargoapi.PromotionTemplate,
+) field.ErrorList {
+	if promoTemplate == nil {
+		return nil
+	}
+	errs := field.ErrorList{}
+	for i, step := range promoTemplate.Spec.Steps {
+		stepAlias := strings.TrimSpace(step.As)
+		if directives.ReservedStepAliasRegex.MatchString(stepAlias) {
+			errs = append(errs, field.Invalid(
+				f.Child("spec", "steps").Index(i).Child("as"),
+				stepAlias,
+				"step alias is reserved",
+			))
+		}
+	}
+	return errs
+}

internal/webhook/stage/webhook_test.go

@@ -1033,6 +1033,12 @@ func TestValidateSpec(t *testing.T) {
 					testFreightRequest,
 					testFreightRequest,
 				},
+				PromotionTemplate: &kargoapi.PromotionTemplate{
+					Spec: kargoapi.PromotionTemplateSpec{
+						// This step alias matches a reserved pattern
+						Steps: []kargoapi.PromotionStep{{As: "step-42"}},
+					},
+				},
 			},
 			assertions: func(t *testing.T, spec *kargoapi.StageSpec, errs field.ErrorList) {
 				// We really want to see that all underlying errors have been bubbled up
@@ -1047,6 +1053,12 @@ func TestValidateSpec(t *testing.T) {
 							Detail: `freight with origin Warehouse/test-warehouse requested multiple ` +
 								"times in spec.requestedFreight",
 						},
+						{
+							Type:     field.ErrorTypeInvalid,
+							Field:    "spec.promotionTemplate.spec.steps[0].as",
+							BadValue: "step-42",
+							Detail:   "step alias is reserved",
+						},
 					},
 					errs,
 				)
@@ -1139,3 +1151,69 @@ func TestValidateRequestedFreight(t *testing.T) {
 		})
 	}
 }
+
+func TestValidatePromotionTemplate(t *testing.T) {
+	testCases := []struct {
+		name          string
+		promoTemplate *kargoapi.PromotionTemplate
+		assertions    func(*testing.T, field.ErrorList)
+	}{
+		{
+			name: "promotionTemplate is nil",
+			assertions: func(t *testing.T, errs field.ErrorList) {
+				require.Empty(t, errs)
+			},
+		},
+		{
+			name: "promotionTemplate is valid",
+			promoTemplate: &kargoapi.PromotionTemplate{
+				Spec: kargoapi.PromotionTemplateSpec{
+					Steps: []kargoapi.PromotionStep{
+						{},
+						{As: "fake-step"},
+					},
+				},
+			},
+			assertions: func(t *testing.T, errs field.ErrorList) {
+				require.Empty(t, errs)
+			},
+		},
+		{
+			name: "promotionTemplate is invalid",
+			promoTemplate: &kargoapi.PromotionTemplate{
+				Spec: kargoapi.PromotionTemplateSpec{
+					Steps: []kargoapi.PromotionStep{
+						{},
+						{As: "step-42"}, // This step alias matches a reserved pattern
+					},
+				},
+			},
+			assertions: func(t *testing.T, errs field.ErrorList) {
+				require.Equal(
+					t,
+					field.ErrorList{
+						{
+							Type:     field.ErrorTypeInvalid,
+							Field:    "promotionTemplate.spec.steps[1].as",
+							BadValue: "step-42",
+							Detail:   "step alias is reserved",
+						},
+					},
+					errs,
+				)
+			},
+		},
+	}
+	w := &webhook{}
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			testCase.assertions(
+				t,
+				w.ValidatePromotionTemplate(
+					field.NewPath("promotionTemplate"),
+					testCase.promoTemplate,
+				),
+			)
+		})
+	}
+}