From d61cf6701cb349165745fd977de9598f23b05db0 Mon Sep 17 00:00:00 2001 From: Marek Aufart Date: Wed, 12 Aug 2026 08:44:53 +0200 Subject: [PATCH 1/4] Undeprecate plugin optionals There was a deprecated transform flag plugin optionals allowing pass args to plugins, this is now needed for e.g. PVC mapping or more powerful plugins. This PR 1. un-deprecates this flag and 2. adds its full support for multistage transformations including instructions file update. Fixes: https://github.com/migtools/crane/issues/791 Signed-off-by: Marek Aufart --- cmd/transform/optionals/optionals.go | 5 +- cmd/transform/transform.go | 57 ++++++- cmd/transform/transform_test.go | 121 ++++++++++++++ e2e-tests/framework/crane.go | 4 + internal/transform/instructions.go | 94 ++++++++++- internal/transform/instructions_test.go | 207 ++++++++++++++++++++++-- internal/transform/orchestrator.go | 12 +- internal/transform/orchestrator_test.go | 68 ++++++++ 8 files changed, 536 insertions(+), 32 deletions(-) diff --git a/cmd/transform/optionals/optionals.go b/cmd/transform/optionals/optionals.go index 9003a5d1..15bca035 100644 --- a/cmd/transform/optionals/optionals.go +++ b/cmd/transform/optionals/optionals.go @@ -47,9 +47,8 @@ func NewOptionalsCommand(f *flags.GlobalFlags) *cobra.Command { cobraGlobalFlags: f, } cmd := &cobra.Command{ - Use: "optionals", - Short: "Return a list of optional fields accepted by configured plugins", - Deprecated: "use custom stages with kustomization patches instead. Optional flags apply globally to all stages and will be removed in a future version.", + Use: "optionals", + Short: "Return a list of optional fields accepted by configured plugins", RunE: func(c *cobra.Command, args []string) error { if err := o.Complete(c, args); err != nil { return err diff --git a/cmd/transform/transform.go b/cmd/transform/transform.go index 05830c37..67b31c3a 100644 --- a/cmd/transform/transform.go +++ b/cmd/transform/transform.go @@ -45,6 +45,7 @@ type Flags struct { TransformDir string `mapstructure:"transform-dir"` SkipPlugins []string `mapstructure:"skip-plugins"` OptionalFlags string `mapstructure:"optional-flags"` + StageOptionals []string `mapstructure:"stage-optionals"` Overwrite bool `mapstructure:"overwrite"` // Kustomize arguments KustomizeArgs string `mapstructure:"kustomize-args"` @@ -159,9 +160,8 @@ func addFlagsForOptions(o *Flags, cmd *cobra.Command) { cmd.Flags().StringVar(&o.InstructionsFile, "instructions-file", "", "Path to the transform instructions file") cmd.Flags().BoolVar(&o.Overwrite, "overwrite", false, "Overwrite existing stage directories even if they contain user modifications") - // Deprecated: optional-flags will be removed in a future version - cmd.Flags().StringVar(&o.OptionalFlags, "optional-flags", "", "(DEPRECATED) JSON string holding flag value pairs to be passed to all plugins. Use custom stages with kustomization instead. (ie. '{\"foo-flag\": \"foo-a=/data,foo-b=/data\", \"bar-flag\": \"bar-value\"}')") - cmd.Flags().MarkDeprecated("optional-flags", "use custom stages with kustomization patches instead. This flag applies globally to all stages and will be removed in a future version.") + cmd.Flags().StringVar(&o.OptionalFlags, "optional-flags", "", "JSON string holding flag value pairs to be passed to all plugins (e.g. '{\"registry-replacement\": \"docker.io=quay.io\"}')") + cmd.Flags().StringSliceVar(&o.StageOptionals, "stage-optionals", nil, "Per-stage optional flags as StageName=JSON, repeatable (e.g. --stage-optionals 'KubernetesPlugin={\"registry-replacement\":\"docker.io=quay.io\"}')") // Kustomize arguments cmd.Flags().StringVar(&o.KustomizeArgs, "kustomize-args", "", "Additional arguments for kustomize (e.g., '--enable-helm --helm-command=helm3')") @@ -190,10 +190,15 @@ func (o *Options) run() error { return err } - if o.InstructionsFile != "" && len(o.RequestedStages) > 0 { // instructions file and positional args are mutually exclusive + if o.InstructionsFile != "" && len(o.RequestedStages) > 0 { return fmt.Errorf("use either --instructions-file or positional stage arguments, not both") } + if o.InstructionsFile != "" && len(o.StageOptionals) > 0 { + return fmt.Errorf("use either --instructions-file or --stage-optionals, not both") + } + var instructionStages []string + var instructionStageOptionals map[string]map[string]string if o.InstructionsFile != "" { instructionsFilePath, err := filepath.Abs(o.InstructionsFile) if err != nil { @@ -203,7 +208,8 @@ func (o *Options) run() error { if err != nil { return err } - instructionStages = internalTransform.GenerateStageDirNames(cfg.Stages) + instructionStages = internalTransform.GenerateStageDirNames(cfg.StageNames()) + instructionStageOptionals = cfg.StageOptionals() } // Parse optional flags var optionalFlags map[string]string @@ -215,6 +221,20 @@ func (o *Options) run() error { optionalFlags = optionalFlagsToLower(optionalFlags) } + // Parse per-stage optional flags from CLI + var stageOptionalFlags map[string]map[string]string + if len(o.StageOptionals) > 0 { + stageOptionalFlags, err = parseStageOptionals(o.StageOptionals) + if err != nil { + return err + } + } + + // Use instruction file per-stage optionals if present, otherwise CLI + if instructionStageOptionals != nil { + stageOptionalFlags = instructionStageOptionals + } + // Parse and validate kustomize arguments kustomizeArgs, err := kustomize.ParseAndValidateArgs(o.KustomizeArgs) if err != nil { @@ -229,6 +249,7 @@ func (o *Options) run() error { PluginDir: pluginDir, SkipPlugins: o.SkipPlugins, OptionalFlags: optionalFlags, + StageOptionalFlags: stageOptionalFlags, Overwrite: o.Overwrite, CraneVersion: "v1.0.0", // TODO: Get from build version NewlyCreatedStages: make(map[string]bool), @@ -321,6 +342,32 @@ func (o *Options) run() error { return orchestrator.RunMultiStage(selector) } +// parseStageOptionals parses --stage-optionals values from "StageName=JSON" format +// into a map of stage name to optional flags. +func parseStageOptionals(values []string) (map[string]map[string]string, error) { + result := make(map[string]map[string]string, len(values)) + for _, v := range values { + stageName, jsonStr, found := strings.Cut(v, "=") + if !found { + return nil, fmt.Errorf("invalid --stage-optionals value %q: expected format StageName=JSON", v) + } + + if stageName == "" { + return nil, fmt.Errorf("invalid --stage-optionals value %q: stage name is empty", v) + } + if _, exists := result[stageName]; exists { + return nil, fmt.Errorf("duplicate --stage-optionals for stage %q", stageName) + } + + var flags map[string]string + if err := json.Unmarshal([]byte(jsonStr), &flags); err != nil { + return nil, fmt.Errorf("invalid JSON in --stage-optionals for stage %q: %w", stageName, err) + } + result[stageName] = optionalFlagsToLower(flags) + } + return result, nil +} + // Returns an extras map with lowercased keys, since any keys coming from the config file // are lower-cased by viper func optionalFlagsToLower(inFlags map[string]string) map[string]string { diff --git a/cmd/transform/transform_test.go b/cmd/transform/transform_test.go index d6cf3b1e..c3c780ce 100644 --- a/cmd/transform/transform_test.go +++ b/cmd/transform/transform_test.go @@ -1088,6 +1088,127 @@ func TestValidate_ExportDir(t *testing.T) { } } +func TestParseStageOptionals(t *testing.T) { + tests := []struct { + name string + values []string + wantErr bool + errMsg string + expected map[string]map[string]string + }{ + { + name: "single stage", + values: []string{`KubernetesPlugin={"registry-replacement": "docker.io=quay.io"}`}, + expected: map[string]map[string]string{ + "KubernetesPlugin": {"registry-replacement": "docker.io=quay.io"}, + }, + }, + { + name: "multiple stages", + values: []string{ + `KubernetesPlugin={"registry-replacement": "docker.io=quay.io"}`, + `RegistryPlugin={"registry-replacement": "quay.io=ghcr.io"}`, + }, + expected: map[string]map[string]string{ + "KubernetesPlugin": {"registry-replacement": "docker.io=quay.io"}, + "RegistryPlugin": {"registry-replacement": "quay.io=ghcr.io"}, + }, + }, + { + name: "missing equals sign", + values: []string{"KubernetesPlugin"}, + wantErr: true, + errMsg: "expected format StageName=JSON", + }, + { + name: "empty stage name", + values: []string{`={"key": "value"}`}, + wantErr: true, + errMsg: "stage name is empty", + }, + { + name: "malformed JSON", + values: []string{`KubernetesPlugin=not-json`}, + wantErr: true, + errMsg: "invalid JSON", + }, + { + name: "duplicate stage name", + values: []string{ + `KubernetesPlugin={"key": "val1"}`, + `KubernetesPlugin={"key": "val2"}`, + }, + wantErr: true, + errMsg: "duplicate", + }, + { + name: "keys are lowercased", + values: []string{`MyPlugin={"Registry-Replacement": "docker.io=quay.io"}`}, + expected: map[string]map[string]string{ + "MyPlugin": {"registry-replacement": "docker.io=quay.io"}, + }, + }, + { + name: "JSON value containing equals sign", + values: []string{`MyPlugin={"registry-replacement": "docker.io=quay.io,gcr.io=ghcr.io"}`}, + expected: map[string]map[string]string{ + "MyPlugin": {"registry-replacement": "docker.io=quay.io,gcr.io=ghcr.io"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := parseStageOptionals(tt.values) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error, got nil") + } + if !strings.Contains(err.Error(), tt.errMsg) { + t.Fatalf("expected error containing %q, got %v", tt.errMsg, err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(result) != len(tt.expected) { + t.Fatalf("expected %d stages, got %d", len(tt.expected), len(result)) + } + for stage, expectedFlags := range tt.expected { + actualFlags, ok := result[stage] + if !ok { + t.Errorf("missing stage %q", stage) + continue + } + for k, v := range expectedFlags { + if actualFlags[k] != v { + t.Errorf("stage %q key %q: expected %q, got %q", stage, k, v, actualFlags[k]) + } + } + } + }) + } +} + +func TestRun_InstructionsFileAndStageOptionalsConflict(t *testing.T) { + o := &Options{ + globalFlags: &flags.GlobalFlags{}, + Flags: Flags{ + InstructionsFile: "instructions.yaml", + StageOptionals: []string{`KubernetesPlugin={"key": "val"}`}, + }, + } + + err := o.run() + if err == nil { + t.Fatalf("expected conflict error, got nil") + } + if !strings.Contains(err.Error(), "use either --instructions-file or --stage-optionals, not both") { + t.Fatalf("unexpected error message: %v", err) + } +} + func TestValidate_MissingExportDir_FailsBeforeRun(t *testing.T) { tmpDir := t.TempDir() transformDir := filepath.Join(tmpDir, "transform") diff --git a/e2e-tests/framework/crane.go b/e2e-tests/framework/crane.go index 05da2389..ac02f3d1 100644 --- a/e2e-tests/framework/crane.go +++ b/e2e-tests/framework/crane.go @@ -49,6 +49,7 @@ type TransformOptions struct { PluginDir string SkipPlugins []string OptionalFlags string + StageOptionals []string Overwrite bool KustomizeArgs string InstructionsFile string @@ -122,6 +123,9 @@ func (c CraneRunner) Transform(opts TransformOptions) error { if opts.OptionalFlags != "" { args = append(args, "--optional-flags", opts.OptionalFlags) } + for _, so := range opts.StageOptionals { + args = append(args, "--stage-optionals", so) + } if opts.Overwrite { args = append(args, "--overwrite") } diff --git a/internal/transform/instructions.go b/internal/transform/instructions.go index 28d81f10..b08c91c0 100644 --- a/internal/transform/instructions.go +++ b/internal/transform/instructions.go @@ -9,7 +9,7 @@ import ( "regexp" "strings" - "gopkg.in/yaml.v3" + yamlv3 "gopkg.in/yaml.v3" ) var stageTokenRegex = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`) @@ -22,8 +22,65 @@ var unknownInstructionsFieldRegex = regexp.MustCompile(`line ([0-9]+): field ([^ // instead of an object containing top-level "stages". var rootSequenceInstructionsRegex = regexp.MustCompile(`line ([0-9]+): cannot unmarshal !!seq into .*InstructionsFile`) +// StageEntry represents a single stage in the instructions file. +// It can be specified as either a plain string (just the name) or an object +// with name and optional per-stage flags. +type StageEntry struct { + Name string `yaml:"name"` + Optionals map[string]string `yaml:"optionals,omitempty"` +} + type InstructionsFile struct { - Stages []string `yaml:"stages"` + Stages []StageEntry `yaml:"-"` +} + +// rawInstructionsFile is used for initial YAML decoding before the mixed-list +// stages field is processed. +type rawInstructionsFile struct { + Stages []yamlv3.Node `yaml:"stages"` +} + +func (f *InstructionsFile) UnmarshalYAML(value *yamlv3.Node) error { + // First pass: decode known top-level keys using a raw struct so we can + // handle the mixed-format stages list ourselves. + var raw rawInstructionsFile + if err := value.Decode(&raw); err != nil { + return err + } + + // Check for unknown top-level keys + if value.Kind == yamlv3.MappingNode { + for i := 0; i+1 < len(value.Content); i += 2 { + key := value.Content[i].Value + if key != "stages" { + return fmt.Errorf("line %d: field %s not found in type transform.InstructionsFile", value.Content[i].Line, key) + } + } + } + + for i, node := range raw.Stages { + switch node.Kind { + case yamlv3.ScalarNode: + f.Stages = append(f.Stages, StageEntry{Name: node.Value}) + case yamlv3.MappingNode: + var entry StageEntry + if err := node.Decode(&entry); err != nil { + return fmt.Errorf("stage at index %d: %w", i, err) + } + // Check for unknown keys in the stage entry + for j := 0; j+1 < len(node.Content); j += 2 { + key := node.Content[j].Value + if key != "name" && key != "optionals" { + return fmt.Errorf("stage at index %d: unknown field %q (supported fields: name, optionals)", i, key) + } + } + f.Stages = append(f.Stages, entry) + default: + return fmt.Errorf("stage at index %d: expected a string or mapping, got %v", i, node.Kind) + } + } + + return nil } // LoadInstructions reads a transform instructions file from disk, parses YAML, and validates @@ -39,7 +96,7 @@ func LoadInstructions(path string) (*InstructionsFile, error) { } cfg := &InstructionsFile{} - decoder := yaml.NewDecoder(bytes.NewReader(data)) + decoder := yamlv3.NewDecoder(bytes.NewReader(data)) decoder.KnownFields(true) if err := decoder.Decode(cfg); err != nil { return nil, fmt.Errorf("failed to parse instructions file %q: %s: %w", path, friendlyInstructionsDecodeError(err), err) @@ -86,8 +143,8 @@ func ValidateInstructions(cfg *InstructionsFile) error { seen := make(map[string]struct{}, len(cfg.Stages)) - for i, s := range cfg.Stages { - stage := strings.TrimSpace(s) + for i := range cfg.Stages { + stage := strings.TrimSpace(cfg.Stages[i].Name) if stage == "" { return fmt.Errorf("stage at index %d is empty", i) } @@ -102,11 +159,36 @@ func ValidateInstructions(cfg *InstructionsFile) error { seen[stage] = struct{}{} - cfg.Stages[i] = stage + cfg.Stages[i].Name = stage } return nil } +// StageNames returns the stage names from the instructions file as a string slice. +func (f *InstructionsFile) StageNames() []string { + names := make([]string, len(f.Stages)) + for i, s := range f.Stages { + names[i] = s.Name + } + return names +} + +// StageOptionals returns a map of stage name to optional flags for stages that +// have per-stage optionals defined. Stages without optionals are omitted. +func (f *InstructionsFile) StageOptionals() map[string]map[string]string { + result := make(map[string]map[string]string) + for _, s := range f.Stages { + if len(s.Optionals) > 0 { + lower := make(map[string]string, len(s.Optionals)) + for k, v := range s.Optionals { + lower[strings.ToLower(k)] = v + } + result[s.Name] = lower + } + } + return result +} + // GenerateStageDirNames converts ordered stage tokens into deterministic stage // directory names using 10-step numeric prefixes (10_, 20_, 30_, ...). func GenerateStageDirNames(stageTokens []string) []string { diff --git a/internal/transform/instructions_test.go b/internal/transform/instructions_test.go index 5f4a6be4..1cbf26d0 100644 --- a/internal/transform/instructions_test.go +++ b/internal/transform/instructions_test.go @@ -14,50 +14,51 @@ func TestValidateInstructions(t *testing.T) { wantErr bool wantStages []string }{ - // Valid instructions remains unchanged. { name: "valid instructions", - cfg: &InstructionsFile{Stages: []string{"KubernetesPlugin", "CustomStage"}}, + cfg: &InstructionsFile{Stages: []StageEntry{{Name: "KubernetesPlugin"}, {Name: "CustomStage"}}}, wantErr: false, wantStages: []string{"KubernetesPlugin", "CustomStage"}, }, - // Whitespace around stage names is trimmed. { name: "valid instructions file trims stage names", - cfg: &InstructionsFile{Stages: []string{" KubernetesPlugin ", " CustomStage\t"}}, + cfg: &InstructionsFile{Stages: []StageEntry{{Name: " KubernetesPlugin "}, {Name: " CustomStage\t"}}}, wantErr: false, wantStages: []string{"KubernetesPlugin", "CustomStage"}, }, - // Duplicate stage names are rejected. { name: "duplicate stages in instructions file", - cfg: &InstructionsFile{Stages: []string{"KubernetesPlugin", "KubernetesPlugin"}}, + cfg: &InstructionsFile{Stages: []StageEntry{{Name: "KubernetesPlugin"}, {Name: "KubernetesPlugin"}}}, wantErr: true, }, - // Unsafe characters are rejected. { name: "invalid characters in instructions file", - cfg: &InstructionsFile{Stages: []string{"KubernetesPlugin", "../bad"}}, + cfg: &InstructionsFile{Stages: []StageEntry{{Name: "KubernetesPlugin"}, {Name: "../bad"}}}, wantErr: true, }, - // At least one stage is required. { name: "empty stages list in instructions file", - cfg: &InstructionsFile{Stages: []string{}}, + cfg: &InstructionsFile{Stages: []StageEntry{}}, wantErr: true, }, - // Nil config pointer is invalid. { name: "nil instructions file", cfg: nil, wantErr: true, }, - // Blank stage entries are invalid. { name: "empty stage entry in instructions file", - cfg: &InstructionsFile{Stages: []string{"KubernetesPlugin", " "}}, + cfg: &InstructionsFile{Stages: []StageEntry{{Name: "KubernetesPlugin"}, {Name: " "}}}, wantErr: true, }, + { + name: "valid stage with optionals", + cfg: &InstructionsFile{Stages: []StageEntry{ + {Name: "KubernetesPlugin", Optionals: map[string]string{"registry-replacement": "docker.io=quay.io"}}, + }}, + wantErr: false, + wantStages: []string{"KubernetesPlugin"}, + }, } for _, tt := range tests { @@ -75,12 +76,13 @@ func TestValidateInstructions(t *testing.T) { } if len(tt.wantStages) > 0 { - if len(tt.cfg.Stages) != len(tt.wantStages) { - t.Fatalf("stages length mismatch: got %d want %d", len(tt.cfg.Stages), len(tt.wantStages)) + names := tt.cfg.StageNames() + if len(names) != len(tt.wantStages) { + t.Fatalf("stages length mismatch: got %d want %d", len(names), len(tt.wantStages)) } for i := range tt.wantStages { - if tt.cfg.Stages[i] != tt.wantStages[i] { - t.Fatalf("at index %d: got %q want %q", i, tt.cfg.Stages[i], tt.wantStages[i]) + if names[i] != tt.wantStages[i] { + t.Fatalf("at index %d: got %q want %q", i, names[i], tt.wantStages[i]) } } } @@ -152,6 +154,177 @@ stages: } } +// Mixed-list YAML format: plain strings and objects can be freely intermixed. +func TestLoadInstructions_MixedListFormat(t *testing.T) { + tmpDir := t.TempDir() + instructionsFilePath := filepath.Join(tmpDir, "mixed-instructions.yaml") + + content := []byte(`stages: + - KubernetesPlugin + - name: RegistryPlugin + optionals: + registry-replacement: "docker.io=quay.io" + - CustomEdits +`) + if err := os.WriteFile(instructionsFilePath, content, 0o600); err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + cfg, err := LoadInstructions(instructionsFilePath) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if len(cfg.Stages) != 3 { + t.Fatalf("expected 3 stages, got %d", len(cfg.Stages)) + } + + // First stage: plain string + if cfg.Stages[0].Name != "KubernetesPlugin" { + t.Errorf("stage 0 name: expected %q, got %q", "KubernetesPlugin", cfg.Stages[0].Name) + } + if len(cfg.Stages[0].Optionals) != 0 { + t.Errorf("stage 0 optionals: expected empty, got %v", cfg.Stages[0].Optionals) + } + + // Second stage: object with optionals + if cfg.Stages[1].Name != "RegistryPlugin" { + t.Errorf("stage 1 name: expected %q, got %q", "RegistryPlugin", cfg.Stages[1].Name) + } + if cfg.Stages[1].Optionals["registry-replacement"] != "docker.io=quay.io" { + t.Errorf("stage 1 optionals: expected registry-replacement=docker.io=quay.io, got %v", cfg.Stages[1].Optionals) + } + + // Third stage: plain string + if cfg.Stages[2].Name != "CustomEdits" { + t.Errorf("stage 2 name: expected %q, got %q", "CustomEdits", cfg.Stages[2].Name) + } +} + +// Object-only stages list should also work. +func TestLoadInstructions_AllObjectFormat(t *testing.T) { + tmpDir := t.TempDir() + instructionsFilePath := filepath.Join(tmpDir, "all-object-instructions.yaml") + + content := []byte(`stages: + - name: KubernetesPlugin + optionals: + strip-default-rbac: "false" + - name: CustomEdits +`) + if err := os.WriteFile(instructionsFilePath, content, 0o600); err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + cfg, err := LoadInstructions(instructionsFilePath) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if len(cfg.Stages) != 2 { + t.Fatalf("expected 2 stages, got %d", len(cfg.Stages)) + } + + if cfg.Stages[0].Optionals["strip-default-rbac"] != "false" { + t.Errorf("stage 0 optionals: expected strip-default-rbac=false, got %v", cfg.Stages[0].Optionals) + } + if len(cfg.Stages[1].Optionals) != 0 { + t.Errorf("stage 1 optionals: expected empty, got %v", cfg.Stages[1].Optionals) + } +} + +// Backward compatibility: plain string list should still work. +func TestLoadInstructions_BackwardCompatibleStringList(t *testing.T) { + tmpDir := t.TempDir() + instructionsFilePath := filepath.Join(tmpDir, "string-instructions.yaml") + + content := []byte(`stages: + - KubernetesPlugin + - CustomStage +`) + if err := os.WriteFile(instructionsFilePath, content, 0o600); err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + cfg, err := LoadInstructions(instructionsFilePath) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + names := cfg.StageNames() + if len(names) != 2 { + t.Fatalf("expected 2 stages, got %d", len(names)) + } + if names[0] != "KubernetesPlugin" || names[1] != "CustomStage" { + t.Fatalf("unexpected stage names: %v", names) + } +} + +// Unknown fields in stage objects should fail. +func TestLoadInstructions_UnknownStageFieldFails(t *testing.T) { + tmpDir := t.TempDir() + instructionsFilePath := filepath.Join(tmpDir, "bad-stage-field.yaml") + + content := []byte(`stages: + - name: KubernetesPlugin + unknown-field: value +`) + if err := os.WriteFile(instructionsFilePath, content, 0o600); err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + _, err := LoadInstructions(instructionsFilePath) + if err == nil { + t.Fatalf("expected error for unknown stage field, got nil") + } + if !strings.Contains(err.Error(), "unknown field") { + t.Fatalf("expected unknown field error, got %v", err) + } +} + +// StageOptionals helper should return only stages with optionals. +func TestInstructionsFile_StageOptionals(t *testing.T) { + cfg := &InstructionsFile{ + Stages: []StageEntry{ + {Name: "KubernetesPlugin"}, + {Name: "RegistryPlugin", Optionals: map[string]string{"registry-replacement": "docker.io=quay.io"}}, + {Name: "CustomEdits"}, + }, + } + + optionals := cfg.StageOptionals() + if len(optionals) != 1 { + t.Fatalf("expected 1 entry in StageOptionals, got %d", len(optionals)) + } + if optionals["RegistryPlugin"]["registry-replacement"] != "docker.io=quay.io" { + t.Errorf("unexpected optionals for RegistryPlugin: %v", optionals["RegistryPlugin"]) + } +} + +// StageOptionals should lowercase keys for viper compatibility. +func TestInstructionsFile_StageOptionals_LowercasesKeys(t *testing.T) { + cfg := &InstructionsFile{ + Stages: []StageEntry{ + {Name: "RegistryPlugin", Optionals: map[string]string{ + "Registry-Replacement": "docker.io=quay.io", + "Strip-Default-RBAC": "true", + }}, + }, + } + + optionals := cfg.StageOptionals() + got := optionals["RegistryPlugin"] + if got["registry-replacement"] != "docker.io=quay.io" { + t.Errorf("expected lowercased key registry-replacement, got keys: %v", got) + } + if got["strip-default-rbac"] != "true" { + t.Errorf("expected lowercased key strip-default-rbac, got keys: %v", got) + } + if _, exists := got["Registry-Replacement"]; exists { + t.Errorf("original mixed-case key should not be present after lowercasing") + } +} + // Root YAML must be a mapping with top-level stages key, not a sequence. func TestLoadInstructions_RootSequenceFailsWithFriendlyMessage(t *testing.T) { tmpDir := t.TempDir() diff --git a/internal/transform/orchestrator.go b/internal/transform/orchestrator.go index 4cf166df..0656123f 100644 --- a/internal/transform/orchestrator.go +++ b/internal/transform/orchestrator.go @@ -27,6 +27,7 @@ type Orchestrator struct { PluginDir string SkipPlugins []string OptionalFlags map[string]string + StageOptionalFlags map[string]map[string]string Overwrite bool CraneVersion string // NewlyCreatedStages tracks stages created in this run that can be overwritten @@ -36,6 +37,15 @@ type Orchestrator struct { KustomizeArgs []string } +func (o *Orchestrator) resolveOptionalFlags(stage Stage) map[string]string { + if o.StageOptionalFlags != nil { + if stageFlags, ok := o.StageOptionalFlags[stage.PluginName]; ok { + return stageFlags + } + } + return o.OptionalFlags +} + // RunMultiStage executes transform with multi-stage pipeline // Each stage runs on the fully applied output of the previous stage func (o *Orchestrator) RunMultiStage(stageSelector StageSelector) error { @@ -199,7 +209,7 @@ func (o *Orchestrator) transformResources(stage Stage, stagePlugin cranelib.Plug runner := cranelib.Runner{ Log: o.Log, PluginPriorities: nil, // No priorities needed - max 1 plugin per stage - OptionalFlags: o.OptionalFlags, + OptionalFlags: o.resolveOptionalFlags(stage), } var artifacts []cranelib.TransformArtifact diff --git a/internal/transform/orchestrator_test.go b/internal/transform/orchestrator_test.go index 5e88a9d1..70ad730a 100644 --- a/internal/transform/orchestrator_test.go +++ b/internal/transform/orchestrator_test.go @@ -1825,6 +1825,74 @@ resources: t.Log("✓ Resource content: ClusterRole rules survived two pipeline stages") } +func TestResolveOptionalFlags(t *testing.T) { + globalFlags := map[string]string{ + "registry-replacement": "docker.io=quay.io", + "strip-default-rbac": "false", + } + stageFlags := map[string]map[string]string{ + "RegistryPlugin": { + "registry-replacement": "docker.io=ghcr.io", + }, + } + + tests := []struct { + name string + optionalFlags map[string]string + stageOptionals map[string]map[string]string + stage Stage + expected map[string]string + }{ + { + name: "per-stage flags override global", + optionalFlags: globalFlags, + stageOptionals: stageFlags, + stage: Stage{PluginName: "RegistryPlugin"}, + expected: stageFlags["RegistryPlugin"], + }, + { + name: "falls back to global when no per-stage", + optionalFlags: globalFlags, + stageOptionals: stageFlags, + stage: Stage{PluginName: "KubernetesPlugin"}, + expected: globalFlags, + }, + { + name: "returns nil when neither set", + optionalFlags: nil, + stageOptionals: nil, + stage: Stage{PluginName: "KubernetesPlugin"}, + expected: nil, + }, + { + name: "returns global when stageOptionals map is empty", + optionalFlags: globalFlags, + stageOptionals: map[string]map[string]string{}, + stage: Stage{PluginName: "KubernetesPlugin"}, + expected: globalFlags, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o := &Orchestrator{ + OptionalFlags: tt.optionalFlags, + StageOptionalFlags: tt.stageOptionals, + } + result := o.resolveOptionalFlags(tt.stage) + if len(result) != len(tt.expected) { + t.Errorf("expected %d flags, got %d", len(tt.expected), len(result)) + return + } + for k, v := range tt.expected { + if result[k] != v { + t.Errorf("key %q: expected %q, got %q", k, v, result[k]) + } + } + }) + } +} + // TestEmptyStageErrorMessage verifies that when a stage has no resources, // a helpful error message is displayed instead of the generic kustomize error func TestEmptyStageErrorMessage(t *testing.T) { From 2fd903382a309a0a66a5473dea28a42cbed74e88 Mon Sep 17 00:00:00 2001 From: Marek Aufart Date: Wed, 12 Aug 2026 10:15:46 +0200 Subject: [PATCH 2/4] Address coderabbitai comments Signed-off-by: Marek Aufart --- cmd/transform/transform.go | 28 ++++++++++++++++-- cmd/transform/transform_test.go | 39 +++++++++++++++++++++++++ internal/transform/instructions.go | 10 +++++-- internal/transform/instructions_test.go | 30 +++++++++++++++++-- 4 files changed, 99 insertions(+), 8 deletions(-) diff --git a/cmd/transform/transform.go b/cmd/transform/transform.go index 67b31c3a..e5dca2df 100644 --- a/cmd/transform/transform.go +++ b/cmd/transform/transform.go @@ -161,7 +161,7 @@ func addFlagsForOptions(o *Flags, cmd *cobra.Command) { cmd.Flags().BoolVar(&o.Overwrite, "overwrite", false, "Overwrite existing stage directories even if they contain user modifications") cmd.Flags().StringVar(&o.OptionalFlags, "optional-flags", "", "JSON string holding flag value pairs to be passed to all plugins (e.g. '{\"registry-replacement\": \"docker.io=quay.io\"}')") - cmd.Flags().StringSliceVar(&o.StageOptionals, "stage-optionals", nil, "Per-stage optional flags as StageName=JSON, repeatable (e.g. --stage-optionals 'KubernetesPlugin={\"registry-replacement\":\"docker.io=quay.io\"}')") + cmd.Flags().StringArrayVar(&o.StageOptionals, "stage-optionals", nil, "Per-stage optional flags as StageName=JSON, repeatable (e.g. --stage-optionals 'KubernetesPlugin={\"registry-replacement\":\"docker.io=quay.io\"}')") // Kustomize arguments cmd.Flags().StringVar(&o.KustomizeArgs, "kustomize-args", "", "Additional arguments for kustomize (e.g., '--enable-helm --helm-command=helm3')") @@ -209,7 +209,10 @@ func (o *Options) run() error { return err } instructionStages = internalTransform.GenerateStageDirNames(cfg.StageNames()) - instructionStageOptionals = cfg.StageOptionals() + instructionStageOptionals, err = cfg.StageOptionals() + if err != nil { + return fmt.Errorf("invalid instructions file %q: %w", instructionsFilePath, err) + } } // Parse optional flags var optionalFlags map[string]string @@ -363,7 +366,14 @@ func parseStageOptionals(values []string) (map[string]map[string]string, error) if err := json.Unmarshal([]byte(jsonStr), &flags); err != nil { return nil, fmt.Errorf("invalid JSON in --stage-optionals for stage %q: %w", stageName, err) } - result[stageName] = optionalFlagsToLower(flags) + if flags == nil { + return nil, fmt.Errorf("invalid JSON in --stage-optionals for stage %q: expected a JSON object, got null", stageName) + } + lower, err := optionalFlagsToLowerChecked(flags) + if err != nil { + return nil, fmt.Errorf("invalid --stage-optionals for stage %q: %w", stageName, err) + } + result[stageName] = lower } return result, nil } @@ -378,6 +388,18 @@ func optionalFlagsToLower(inFlags map[string]string) map[string]string { return lowerMap } +func optionalFlagsToLowerChecked(inFlags map[string]string) (map[string]string, error) { + lowerMap := make(map[string]string, len(inFlags)) + for key, val := range inFlags { + lk := strings.ToLower(key) + if _, exists := lowerMap[lk]; exists { + return nil, fmt.Errorf("duplicate optional key %q (case-insensitive collision)", lk) + } + lowerMap[lk] = val + } + return lowerMap, nil +} + // runStageWithCleanup runs a single stage and optionally cleans up on error. // This is used by the instructions file code path which runs stages one at a time. func (o *Options) runStageWithCleanup(orchestrator *internalTransform.Orchestrator, selector internalTransform.StageSelector, stageDir string, cleanupOnError bool, log *logrus.Logger) error { diff --git a/cmd/transform/transform_test.go b/cmd/transform/transform_test.go index c3c780ce..c225d858 100644 --- a/cmd/transform/transform_test.go +++ b/cmd/transform/transform_test.go @@ -1148,6 +1148,12 @@ func TestParseStageOptionals(t *testing.T) { "MyPlugin": {"registry-replacement": "docker.io=quay.io"}, }, }, + { + name: "null JSON value", + values: []string{`KubernetesPlugin=null`}, + wantErr: true, + errMsg: "expected a JSON object", + }, { name: "JSON value containing equals sign", values: []string{`MyPlugin={"registry-replacement": "docker.io=quay.io,gcr.io=ghcr.io"}`}, @@ -1191,6 +1197,39 @@ func TestParseStageOptionals(t *testing.T) { } } +// Reject case-insensitive duplicate keys in --stage-optionals JSON. +func TestParseStageOptionals_CaseInsensitiveDuplicateKey(t *testing.T) { + values := []string{ + `MyPlugin={"Registry-Replacement": "docker.io=quay.io", "registry-replacement": "gcr.io=ghcr.io"}`, + } + _, err := parseStageOptionals(values) + if err == nil { + t.Fatalf("expected error for case-insensitive duplicate key, got nil") + } + if !strings.Contains(err.Error(), "duplicate optional key") { + t.Fatalf("expected duplicate key error, got %v", err) + } +} + +// Regression: multi-field JSON with commas must not be split by the flag parser. +// StringArrayVar preserves each value as-is; StringSliceVar would split on commas. +func TestParseStageOptionals_MultiFieldJSON(t *testing.T) { + values := []string{ + `KubernetesPlugin={"registry-replacement": "docker.io=quay.io", "strip-default-rbac": "false"}`, + } + result, err := parseStageOptionals(values) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + got := result["KubernetesPlugin"] + if got["registry-replacement"] != "docker.io=quay.io" { + t.Errorf("registry-replacement: expected %q, got %q", "docker.io=quay.io", got["registry-replacement"]) + } + if got["strip-default-rbac"] != "false" { + t.Errorf("strip-default-rbac: expected %q, got %q", "false", got["strip-default-rbac"]) + } +} + func TestRun_InstructionsFileAndStageOptionalsConflict(t *testing.T) { o := &Options{ globalFlags: &flags.GlobalFlags{}, diff --git a/internal/transform/instructions.go b/internal/transform/instructions.go index b08c91c0..566b905c 100644 --- a/internal/transform/instructions.go +++ b/internal/transform/instructions.go @@ -175,18 +175,22 @@ func (f *InstructionsFile) StageNames() []string { // StageOptionals returns a map of stage name to optional flags for stages that // have per-stage optionals defined. Stages without optionals are omitted. -func (f *InstructionsFile) StageOptionals() map[string]map[string]string { +func (f *InstructionsFile) StageOptionals() (map[string]map[string]string, error) { result := make(map[string]map[string]string) for _, s := range f.Stages { if len(s.Optionals) > 0 { lower := make(map[string]string, len(s.Optionals)) for k, v := range s.Optionals { - lower[strings.ToLower(k)] = v + lk := strings.ToLower(k) + if _, exists := lower[lk]; exists { + return nil, fmt.Errorf("stage %q: duplicate optional key %q (case-insensitive collision)", s.Name, lk) + } + lower[lk] = v } result[s.Name] = lower } } - return result + return result, nil } // GenerateStageDirNames converts ordered stage tokens into deterministic stage diff --git a/internal/transform/instructions_test.go b/internal/transform/instructions_test.go index 1cbf26d0..5f51d794 100644 --- a/internal/transform/instructions_test.go +++ b/internal/transform/instructions_test.go @@ -292,7 +292,10 @@ func TestInstructionsFile_StageOptionals(t *testing.T) { }, } - optionals := cfg.StageOptionals() + optionals, err := cfg.StageOptionals() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } if len(optionals) != 1 { t.Fatalf("expected 1 entry in StageOptionals, got %d", len(optionals)) } @@ -312,7 +315,10 @@ func TestInstructionsFile_StageOptionals_LowercasesKeys(t *testing.T) { }, } - optionals := cfg.StageOptionals() + optionals, err := cfg.StageOptionals() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } got := optionals["RegistryPlugin"] if got["registry-replacement"] != "docker.io=quay.io" { t.Errorf("expected lowercased key registry-replacement, got keys: %v", got) @@ -325,6 +331,26 @@ func TestInstructionsFile_StageOptionals_LowercasesKeys(t *testing.T) { } } +// StageOptionals should reject case-insensitive duplicate keys. +func TestInstructionsFile_StageOptionals_RejectsCaseCollision(t *testing.T) { + cfg := &InstructionsFile{ + Stages: []StageEntry{ + {Name: "RegistryPlugin", Optionals: map[string]string{ + "Registry-Replacement": "docker.io=quay.io", + "registry-replacement": "gcr.io=ghcr.io", + }}, + }, + } + + _, err := cfg.StageOptionals() + if err == nil { + t.Fatalf("expected error for case-insensitive duplicate key, got nil") + } + if !strings.Contains(err.Error(), "duplicate optional key") { + t.Fatalf("expected duplicate key error, got %v", err) + } +} + // Root YAML must be a mapping with top-level stages key, not a sequence. func TestLoadInstructions_RootSequenceFailsWithFriendlyMessage(t *testing.T) { tmpDir := t.TempDir() From c9f2df0913745247df2ad6e82909022a801bc1f0 Mon Sep 17 00:00:00 2001 From: Marek Aufart Date: Wed, 12 Aug 2026 10:37:28 +0200 Subject: [PATCH 3/4] Minor cleanup Signed-off-by: Marek Aufart --- cmd/transform/optionals_test.go | 5 ++++- cmd/transform/transform.go | 13 ++++--------- internal/transform/instructions.go | 3 +-- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/cmd/transform/optionals_test.go b/cmd/transform/optionals_test.go index eb9be12e..bbad7937 100644 --- a/cmd/transform/optionals_test.go +++ b/cmd/transform/optionals_test.go @@ -149,7 +149,10 @@ func TestOptionalFlagsToLower(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := optionalFlagsToLower(tt.input) + result, err := optionalFlagsToLowerChecked(tt.input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } assertMapEquals(t, tt.expected, result) }) } diff --git a/cmd/transform/transform.go b/cmd/transform/transform.go index e5dca2df..93c41869 100644 --- a/cmd/transform/transform.go +++ b/cmd/transform/transform.go @@ -221,7 +221,10 @@ func (o *Options) run() error { if err != nil { return err } - optionalFlags = optionalFlagsToLower(optionalFlags) + optionalFlags, err = optionalFlagsToLowerChecked(optionalFlags) + if err != nil { + return fmt.Errorf("invalid --optional-flags: %w", err) + } } // Parse per-stage optional flags from CLI @@ -380,14 +383,6 @@ func parseStageOptionals(values []string) (map[string]map[string]string, error) // Returns an extras map with lowercased keys, since any keys coming from the config file // are lower-cased by viper -func optionalFlagsToLower(inFlags map[string]string) map[string]string { - lowerMap := make(map[string]string) - for key, val := range inFlags { - lowerMap[strings.ToLower(key)] = val - } - return lowerMap -} - func optionalFlagsToLowerChecked(inFlags map[string]string) (map[string]string, error) { lowerMap := make(map[string]string, len(inFlags)) for key, val := range inFlags { diff --git a/internal/transform/instructions.go b/internal/transform/instructions.go index 566b905c..f75b6702 100644 --- a/internal/transform/instructions.go +++ b/internal/transform/instructions.go @@ -48,7 +48,7 @@ func (f *InstructionsFile) UnmarshalYAML(value *yamlv3.Node) error { return err } - // Check for unknown top-level keys + // Replaces KnownFields(true) which has no effect with custom UnmarshalYAML. if value.Kind == yamlv3.MappingNode { for i := 0; i+1 < len(value.Content); i += 2 { key := value.Content[i].Value @@ -97,7 +97,6 @@ func LoadInstructions(path string) (*InstructionsFile, error) { cfg := &InstructionsFile{} decoder := yamlv3.NewDecoder(bytes.NewReader(data)) - decoder.KnownFields(true) if err := decoder.Decode(cfg); err != nil { return nil, fmt.Errorf("failed to parse instructions file %q: %s: %w", path, friendlyInstructionsDecodeError(err), err) } From 512677e615fac4734012ef2ac3e1770b51eebfc3 Mon Sep 17 00:00:00 2001 From: Marek Aufart Date: Wed, 12 Aug 2026 14:17:06 +0200 Subject: [PATCH 4/4] Address Sachin feedback, validation, global and stage merged Signed-off-by: Marek Aufart --- internal/transform/orchestrator.go | 46 ++++++++++++-- internal/transform/orchestrator_test.go | 84 ++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 7 deletions(-) diff --git a/internal/transform/orchestrator.go b/internal/transform/orchestrator.go index 0656123f..28382b80 100644 --- a/internal/transform/orchestrator.go +++ b/internal/transform/orchestrator.go @@ -37,13 +37,45 @@ type Orchestrator struct { KustomizeArgs []string } -func (o *Orchestrator) resolveOptionalFlags(stage Stage) map[string]string { - if o.StageOptionalFlags != nil { - if stageFlags, ok := o.StageOptionalFlags[stage.PluginName]; ok { - return stageFlags +func (o *Orchestrator) validateStageOptionalFlags(stages []Stage) error { + if len(o.StageOptionalFlags) == 0 { + return nil + } + known := make(map[string]bool, len(stages)) + for _, s := range stages { + known[s.PluginName] = true + } + for name := range o.StageOptionalFlags { + if !known[name] { + names := make([]string, len(stages)) + for i, s := range stages { + names[i] = s.PluginName + } + return fmt.Errorf("per-stage optionals reference unknown stage %q (known stages: %s)", name, strings.Join(names, ", ")) } } - return o.OptionalFlags + return nil +} + +func (o *Orchestrator) resolveOptionalFlags(stage Stage) map[string]string { + if o.StageOptionalFlags == nil { + return o.OptionalFlags + } + stageFlags, ok := o.StageOptionalFlags[stage.PluginName] + if !ok { + return o.OptionalFlags + } + if len(o.OptionalFlags) == 0 { + return stageFlags + } + merged := make(map[string]string, len(o.OptionalFlags)+len(stageFlags)) + for k, v := range o.OptionalFlags { + merged[k] = v + } + for k, v := range stageFlags { + merged[k] = v + } + return merged } // RunMultiStage executes transform with multi-stage pipeline @@ -69,6 +101,10 @@ func (o *Orchestrator) RunMultiStage(stageSelector StageSelector) error { return fmt.Errorf("no stages found matching selector") } + if err := o.validateStageOptionalFlags(selectedStages); err != nil { + return err + } + opts := file.PathOpts{ TransformDir: o.TransformDir, ExportDir: o.ExportDir, diff --git a/internal/transform/orchestrator_test.go b/internal/transform/orchestrator_test.go index 70ad730a..0b16459c 100644 --- a/internal/transform/orchestrator_test.go +++ b/internal/transform/orchestrator_test.go @@ -1844,11 +1844,14 @@ func TestResolveOptionalFlags(t *testing.T) { expected map[string]string }{ { - name: "per-stage flags override global", + name: "per-stage flags merged with global, stage wins on conflict", optionalFlags: globalFlags, stageOptionals: stageFlags, stage: Stage{PluginName: "RegistryPlugin"}, - expected: stageFlags["RegistryPlugin"], + expected: map[string]string{ + "registry-replacement": "docker.io=ghcr.io", + "strip-default-rbac": "false", + }, }, { name: "falls back to global when no per-stage", @@ -1871,6 +1874,13 @@ func TestResolveOptionalFlags(t *testing.T) { stage: Stage{PluginName: "KubernetesPlugin"}, expected: globalFlags, }, + { + name: "stage optionals with no global flags", + optionalFlags: nil, + stageOptionals: stageFlags, + stage: Stage{PluginName: "RegistryPlugin"}, + expected: stageFlags["RegistryPlugin"], + }, } for _, tt := range tests { @@ -1893,6 +1903,76 @@ func TestResolveOptionalFlags(t *testing.T) { } } +func TestValidateStageOptionalFlags(t *testing.T) { + stages := []Stage{ + {PluginName: "KubernetesPlugin", DirName: "10_KubernetesPlugin"}, + {PluginName: "CustomEdits", DirName: "20_CustomEdits"}, + } + + tests := []struct { + name string + stageOptionals map[string]map[string]string + wantErr bool + errContains string + }{ + { + name: "nil map is valid", + stageOptionals: nil, + wantErr: false, + }, + { + name: "empty map is valid", + stageOptionals: map[string]map[string]string{}, + wantErr: false, + }, + { + name: "known stage is valid", + stageOptionals: map[string]map[string]string{ + "KubernetesPlugin": {"key": "val"}, + }, + wantErr: false, + }, + { + name: "unknown stage is rejected", + stageOptionals: map[string]map[string]string{ + "NonExistentPlugin": {"key": "val"}, + }, + wantErr: true, + errContains: "unknown stage", + }, + { + name: "typo in stage name is rejected", + stageOptionals: map[string]map[string]string{ + "KubernetesPlugin": {"key": "val"}, + "Typo": {"key": "val"}, + }, + wantErr: true, + errContains: "unknown stage", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o := &Orchestrator{ + StageOptionalFlags: tt.stageOptionals, + } + err := o.validateStageOptionalFlags(stages) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error, got nil") + } + if !strings.Contains(err.Error(), tt.errContains) { + t.Fatalf("expected error containing %q, got %v", tt.errContains, err) + } + } else { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + } + }) + } +} + // TestEmptyStageErrorMessage verifies that when a stage has no resources, // a helpful error message is displayed instead of the generic kustomize error func TestEmptyStageErrorMessage(t *testing.T) {