Restrict/warn overridding of pre/post definitions in tests and requires a manual flag

Author: deepsm007

pkg/api/types.go

@@ -1232,6 +1232,10 @@ type MultiStageTestConfiguration struct {
 	// NodeArchitectureOverrides is a map that allows overriding the node architecture for specific steps
 	// that exist in the Pre, Test and Post steps. The key is the name of the step and the value is the architecture.
 	NodeArchitectureOverrides NodeArchitectureOverrides `json:"node_architecture_overrides,omitempty"`
+	// AllowPrePostStepOverrides must be set to true when a test configuration overrides the pre or post steps
+	// from a workflow. This is a safety mechanism to prevent accidental overriding of critical setup and
+	// teardown steps that could cause resource leaks.
+	AllowPrePostStepOverrides *bool `json:"allow_pre_post_step_overrides,omitempty"`
 }
 
 type NodeArchitectureOverrides map[string]NodeArchitecture

pkg/api/zz_generated.deepcopy.go

@@ -68,6 +68,7 @@ func NewResolver(stepsByName ReferenceByName, chainsByName ChainByName, workflow
 
 func (r *registry) Resolve(name string, config api.MultiStageTestConfiguration) (api.MultiStageTestConfigurationLiteral, error) {
 	var overridden [][]api.TestStep
+
 	if config.Workflow != nil {
 		var errs []error
 		overridden, errs = r.mergeWorkflow(&config)
@@ -103,6 +104,7 @@ func (r *registry) mergeWorkflow(config *api.MultiStageTestConfiguration) ([][]a
 	} else {
 		overridden = append(overridden, workflow.Post)
 	}
+
 	config.Environment = mergeEnvironments(workflow.Environment, config.Environment)
 	config.Dependencies = mergeDependencies(workflow.Dependencies, config.Dependencies)
 	config.DependencyOverrides = mergeDependencyOverrides(workflow.DependencyOverrides, config.DependencyOverrides)
@@ -410,7 +412,7 @@ func ResolveConfig(resolver Resolver, config api.ReleaseBuildConfiguration) (api
 
 		resolvedConfig, err := resolver.Resolve(step.As, *step.MultiStageTestConfiguration)
 		if err != nil {
-			return api.ReleaseBuildConfiguration{}, fmt.Errorf("Failed resolve MultiStageTestConfiguration: %w", err)
+			return api.ReleaseBuildConfiguration{}, fmt.Errorf("failed to resolve MultiStageTestConfiguration: %w", err)
 		}
 		step.MultiStageTestConfigurationLiteral = &resolvedConfig
 		// remove old multi stage config

pkg/registry/resolver.go

@@ -20,6 +20,8 @@ type Validator struct {
 	validClusterClaimOwners api.ClusterClaimOwnersMap
 	// hasTrapCache avoids redundant regexp searches on step commands.
 	hasTrapCache map[string]bool
+	// workflowsByName holds workflows for pre/post override validation
+	workflowsByName map[string]api.MultiStageTestConfiguration
 }
 
 // NewValidator creates an object that optimizes bulk validations.
@@ -36,6 +38,13 @@ func NewValidator(profiles api.ClusterProfilesMap, clusterClaimOwners api.Cluste
 	return ret
 }
 
+// NewValidatorWithWorkflows creates a validator with workflow information for pre/post override validation.
+func NewValidatorWithWorkflows(profiles api.ClusterProfilesMap, clusterClaimOwners api.ClusterClaimOwnersMap, workflowsByName map[string]api.MultiStageTestConfiguration) Validator {
+	ret := NewValidator(profiles, clusterClaimOwners)
+	ret.workflowsByName = workflowsByName
+	return ret
+}
+
 func newSingleUseValidator() Validator {
 	return Validator{}
 }

pkg/validation/config.go

@@ -428,6 +428,35 @@ func validateTestStepDependencies(config *api.ReleaseBuildConfiguration) []error
 	return errs
 }
 
+// validateWorkflowOverrides validates that tests properly acknowledge when they override workflow pre/post steps
+func (v *Validator) validateWorkflowOverrides(config api.MultiStageTestConfiguration) error {
+	if config.Workflow == nil {
+		return nil
+	}
+
+	workflow, ok := v.workflowsByName[*config.Workflow]
+	if !ok {
+		// This will be caught by mergeWorkflow, no need to duplicate the error
+		return nil
+	}
+
+	hasPreOverride := len(config.Pre) > 0 && len(workflow.Pre) > 0
+	hasPostOverride := len(config.Post) > 0 && len(workflow.Post) > 0
+
+	if (hasPreOverride || hasPostOverride) && (config.AllowPrePostStepOverrides == nil || !*config.AllowPrePostStepOverrides) {
+		var overriddenSections []string
+		if hasPreOverride {
+			overriddenSections = append(overriddenSections, "pre")
+		}
+		if hasPostOverride {
+			overriddenSections = append(overriddenSections, "post")
+		}
+		return fmt.Errorf("test configuration overrides %s steps from workflow %q but does not have 'allow_pre_post_step_overrides: true' set. This flag is required to prevent accidental overriding of critical setup and teardown steps that could cause resource leaks", strings.Join(overriddenSections, " and "), *config.Workflow)
+	}
+
+	return nil
+}
+
 func (v *Validator) validateClusterProfile(fieldRoot string, p api.ClusterProfile, metadata *api.Metadata) []error {
 	if v.validClusterProfiles != nil {
 		if _, ok := v.validClusterProfiles[p]; ok {
@@ -602,6 +631,13 @@ func (v *Validator) validateTestConfigurationType(
 		if testConfig.NodeArchitecture != nil {
 			validationErrors = append(validationErrors, validateNodeArchitecture(fieldRoot, *testConfig.NodeArchitecture))
 		}
+
+		// Validate workflow overrides if workflow registry is available
+		if v.workflowsByName != nil {
+			if err := v.validateWorkflowOverrides(*testConfig); err != nil {
+				validationErrors = append(validationErrors, err)
+			}
+		}
 		validationErrors = append(validationErrors, v.validateTestSteps(context.addField("pre"), testStagePre, testConfig.Pre, claimRelease)...)
 		validationErrors = append(validationErrors, v.validateTestSteps(context.addField("test"), testStageTest, testConfig.Test, claimRelease)...)
 		validationErrors = append(validationErrors, v.validateTestSteps(context.addField("post"), testStagePost, testConfig.Post, claimRelease)...)