diff --git a/internal/directives/simple_engine.go b/internal/directives/simple_engine.go index 4c3eaeb43..abeb3cfc4 100644 --- a/internal/directives/simple_engine.go +++ b/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, @@ -110,9 +130,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, diff --git a/internal/webhook/stage/webhook.go b/internal/webhook/stage/webhook.go index 2c3f1ecbc..a08afd358 100644 --- a/internal/webhook/stage/webhook.go +++ b/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 +} diff --git a/internal/webhook/stage/webhook_test.go b/internal/webhook/stage/webhook_test.go index 08c1f4c10..026fd63b9 100644 --- a/internal/webhook/stage/webhook_test.go +++ b/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, + ), + ) + }) + } +}