diff --git a/pkg/api/types.go b/pkg/api/types.go index 4cecbd6a62..7004dd0a17 100644 --- a/pkg/api/types.go +++ b/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 diff --git a/pkg/api/zz_generated.deepcopy.go b/pkg/api/zz_generated.deepcopy.go index fe4829ffe5..ded4b9ac7d 100644 --- a/pkg/api/zz_generated.deepcopy.go +++ b/pkg/api/zz_generated.deepcopy.go @@ -872,6 +872,11 @@ func (in *MultiStageTestConfiguration) DeepCopyInto(out *MultiStageTestConfigura (*out)[key] = val } } + if in.AllowPrePostStepOverrides != nil { + in, out := &in.AllowPrePostStepOverrides, &out.AllowPrePostStepOverrides + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MultiStageTestConfiguration. diff --git a/pkg/registry/resolver.go b/pkg/registry/resolver.go index 5d89b4d2d8..53934ae560 100644 --- a/pkg/registry/resolver.go +++ b/pkg/registry/resolver.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 diff --git a/pkg/validation/config.go b/pkg/validation/config.go index 3d5f19de52..89355c5535 100644 --- a/pkg/validation/config.go +++ b/pkg/validation/config.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{} } diff --git a/pkg/validation/test.go b/pkg/validation/test.go index fa9fe873c2..311e01eb6a 100644 --- a/pkg/validation/test.go +++ b/pkg/validation/test.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)...) diff --git a/pkg/validation/test_workflow_overrides_test.go b/pkg/validation/test_workflow_overrides_test.go new file mode 100644 index 0000000000..52955b619e --- /dev/null +++ b/pkg/validation/test_workflow_overrides_test.go @@ -0,0 +1,366 @@ +package validation + +import ( + "strings" + "testing" + + "k8s.io/apimachinery/pkg/util/sets" + + "github.com/openshift/ci-tools/pkg/api" +) + +func TestValidateWorkflowOverrides(t *testing.T) { + truePtr := func(b bool) *bool { return &b } + + // Define test workflows with pre and post steps + workflowWithPrePost := "workflow-with-pre-post" + workflowWithOnlyPre := "workflow-with-only-pre" + workflowWithOnlyPost := "workflow-with-only-post" + workflowEmpty := "workflow-empty" + + workflows := map[string]api.MultiStageTestConfiguration{ + workflowWithPrePost: { + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "pre-setup", + From: "cli", + Commands: "setup commands", + }, + }}, + Post: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "post-cleanup", + From: "cli", + Commands: "cleanup commands", + }, + }}, + }, + workflowWithOnlyPre: { + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "pre-setup", + From: "cli", + Commands: "setup commands", + }, + }}, + }, + workflowWithOnlyPost: { + Post: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "post-cleanup", + From: "cli", + Commands: "cleanup commands", + }, + }}, + }, + workflowEmpty: {}, + } + + validator := NewValidatorWithWorkflows(nil, nil, workflows) + + testCases := []struct { + name string + config api.MultiStageTestConfiguration + expectError bool + expectedErrorMsg string + }{ + { + name: "no override - should pass", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithPrePost, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + }, + expectError: false, + }, + { + name: "override pre without flag - should fail", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithPrePost, + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-pre", + From: "cli", + Commands: "my pre commands", + }, + }}, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + }, + expectError: true, + expectedErrorMsg: "test configuration overrides pre steps from workflow \"workflow-with-pre-post\" but does not have 'allow_pre_post_step_overrides: true' set", + }, + { + name: "override post without flag - should fail", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithPrePost, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + Post: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-post", + From: "cli", + Commands: "my post commands", + }, + }}, + }, + expectError: true, + expectedErrorMsg: "test configuration overrides post steps from workflow \"workflow-with-pre-post\" but does not have 'allow_pre_post_step_overrides: true' set", + }, + { + name: "override both pre and post without flag - should fail", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithPrePost, + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-pre", + From: "cli", + Commands: "my pre commands", + }, + }}, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + Post: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-post", + From: "cli", + Commands: "my post commands", + }, + }}, + }, + expectError: true, + expectedErrorMsg: "test configuration overrides pre and post steps from workflow \"workflow-with-pre-post\" but does not have 'allow_pre_post_step_overrides: true' set", + }, + { + name: "override pre with flag - should pass", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithPrePost, + AllowPrePostStepOverrides: truePtr(true), + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-pre", + From: "cli", + Commands: "my pre commands", + }, + }}, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + }, + expectError: false, + }, + { + name: "override post with flag - should pass", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithPrePost, + AllowPrePostStepOverrides: truePtr(true), + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + Post: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-post", + From: "cli", + Commands: "my post commands", + }, + }}, + }, + expectError: false, + }, + { + name: "override both pre and post with flag - should pass", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithPrePost, + AllowPrePostStepOverrides: truePtr(true), + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-pre", + From: "cli", + Commands: "my pre commands", + }, + }}, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + Post: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-post", + From: "cli", + Commands: "my post commands", + }, + }}, + }, + expectError: false, + }, + { + name: "override with flag set to false - should fail", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithPrePost, + AllowPrePostStepOverrides: truePtr(false), + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-pre", + From: "cli", + Commands: "my pre commands", + }, + }}, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + }, + expectError: true, + expectedErrorMsg: "test configuration overrides pre steps from workflow \"workflow-with-pre-post\" but does not have 'allow_pre_post_step_overrides: true' set", + }, + { + name: "empty workflow - no validation needed", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowEmpty, + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-pre", + From: "cli", + Commands: "my pre commands", + }, + }}, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + Post: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-post", + From: "cli", + Commands: "my post commands", + }, + }}, + }, + expectError: false, + }, + { + name: "override only pre from workflow with only pre - should fail", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithOnlyPre, + Pre: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-pre", + From: "cli", + Commands: "my pre commands", + }, + }}, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + }, + expectError: true, + expectedErrorMsg: "test configuration overrides pre steps from workflow \"workflow-with-only-pre\" but does not have 'allow_pre_post_step_overrides: true' set", + }, + { + name: "override only post from workflow with only post - should fail", + config: api.MultiStageTestConfiguration{ + Workflow: &workflowWithOnlyPost, + Test: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "test-step", + From: "cli", + Commands: "test commands", + }, + }}, + Post: []api.TestStep{{ + LiteralTestStep: &api.LiteralTestStep{ + As: "my-post", + From: "cli", + Commands: "my post commands", + }, + }}, + }, + expectError: true, + expectedErrorMsg: "test configuration overrides post steps from workflow \"workflow-with-only-post\" but does not have 'allow_pre_post_step_overrides: true' set", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Test the validation by creating a test configuration with the MultiStageTestConfiguration + testConfig := api.ReleaseBuildConfiguration{ + Tests: []api.TestStepConfiguration{ + { + As: "test-name", + MultiStageTestConfiguration: &tc.config, + }, + }, + } + + errs := validator.validateTestStepConfiguration(NewConfigContext(), "tests", testConfig.Tests, nil, nil, sets.New[string](), sets.New[string](), false) + + if tc.expectError { + if len(errs) == 0 { + t.Fatalf("Expected error but got none") + } + found := false + for _, err := range errs { + if err != nil && strings.Contains(err.Error(), tc.expectedErrorMsg) { + found = true + break + } + } + if !found { + t.Errorf("Expected error message to contain %q, got errors: %v", tc.expectedErrorMsg, errs) + } + } else { + // Filter out non-workflow validation errors and only look for workflow override errors + var workflowErrs []string + for _, err := range errs { + if err != nil && (strings.Contains(err.Error(), "allow_pre_post_step_overrides") || strings.Contains(err.Error(), "overrides") && strings.Contains(err.Error(), "workflow")) { + workflowErrs = append(workflowErrs, err.Error()) + } + } + if len(workflowErrs) > 0 { + t.Fatalf("Expected no workflow override error but got: %v", workflowErrs) + } + } + }) + } +} diff --git a/pkg/webreg/zz_generated.ci_operator_reference.go b/pkg/webreg/zz_generated.ci_operator_reference.go index 97a8001e87..7bab1d951d 100644 --- a/pkg/webreg/zz_generated.ci_operator_reference.go +++ b/pkg/webreg/zz_generated.ci_operator_reference.go @@ -1072,6 +1072,10 @@ const ciOperatorReferenceYaml = "# The list of base images describe\n" + " # they fail. The given step must explicitly ask for being ignored by setting\n" + " # the OptionalOnSuccess flag to true.\n" + " allow_best_effort_post_steps: false\n" + + " # AllowPrePostStepOverrides must be set to true when a test configuration overrides the pre or post steps\n" + + " # from a workflow. This is a safety mechanism to prevent accidental overriding of critical setup and\n" + + " # teardown steps that could cause resource leaks.\n" + + " allow_pre_post_step_overrides: false\n" + " # AllowSkipOnSuccess defines if any steps can be skipped when\n" + " # all previous `pre` and `test` steps were successful. The given step must explicitly\n" + " # ask for being skipped by setting the OptionalOnSuccess flag to true.\n" + @@ -1970,6 +1974,10 @@ const ciOperatorReferenceYaml = "# The list of base images describe\n" + " # they fail. The given step must explicitly ask for being ignored by setting\n" + " # the OptionalOnSuccess flag to true.\n" + " allow_best_effort_post_steps: false\n" + + " # AllowPrePostStepOverrides must be set to true when a test configuration overrides the pre or post steps\n" + + " # from a workflow. This is a safety mechanism to prevent accidental overriding of critical setup and\n" + + " # teardown steps that could cause resource leaks.\n" + + " allow_pre_post_step_overrides: false\n" + " # AllowSkipOnSuccess defines if any steps can be skipped when\n" + " # all previous `pre` and `test` steps were successful. The given step must explicitly\n" + " # ask for being skipped by setting the OptionalOnSuccess flag to true.\n" +