diff --git a/pkg/workflow/secret_extraction.go b/pkg/workflow/secret_extraction.go index 750c4c5bb32..ca651c7d649 100644 --- a/pkg/workflow/secret_extraction.go +++ b/pkg/workflow/secret_extraction.go @@ -121,7 +121,13 @@ func ExtractEnvExpressionsFromMap(values map[string]string) map[string]string { // The backslash is used to escape the ${} for proper JSON rendering in Copilot configs func ReplaceSecretsWithEnvVars(value string, secrets map[string]string) string { result := value - for varName, secretExpr := range secrets { + // Sort keys for deterministic output. When multiple secret names share the same + // expression (e.g. "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}" maps + // to both "DD_APPLICATION_KEY" and "DD_APP_KEY"), the alphabetically first key is + // processed first and its replacement wins; subsequent keys find the expression + // already replaced and are no-ops. This ensures stable lock-file output across runs. + for _, varName := range sortedMapKeys(secrets) { + secretExpr := secrets[varName] // Replace ${{ secrets.VAR }} with \${VAR} (backslash-escaped for copilot JSON config) result = strings.ReplaceAll(result, secretExpr, "\\${"+varName+"}") } @@ -137,7 +143,9 @@ func ReplaceSecretsWithEnvVars(value string, secrets map[string]string) string { func ReplaceSecretsWithBashVars(value string) string { result := value secrets := ExtractSecretsFromValue(value) - for varName, secretExpr := range secrets { + // Sort keys for deterministic output; see ReplaceSecretsWithEnvVars for rationale. + for _, varName := range sortedMapKeys(secrets) { + secretExpr := secrets[varName] result = strings.ReplaceAll(result, secretExpr, "${"+varName+"}") } return result @@ -321,15 +329,18 @@ func formatInputNameAsEnvVar(inputName string) string { func ReplaceTemplateExpressionsWithEnvVars(value string) string { result := value - // Extract and replace secrets + // Extract and replace secrets — sort keys for deterministic output; see + // ReplaceSecretsWithEnvVars for rationale. secrets := ExtractSecretsFromValue(value) - for varName, secretExpr := range secrets { + for _, varName := range sortedMapKeys(secrets) { + secretExpr := secrets[varName] result = strings.ReplaceAll(result, secretExpr, "\\${"+varName+"}") } - // Extract and replace env vars + // Extract and replace env vars — sort keys for deterministic output. envVars := ExtractEnvExpressionsFromValue(value) - for varName, envExpr := range envVars { + for _, varName := range sortedMapKeys(envVars) { + envExpr := envVars[varName] result = strings.ReplaceAll(result, envExpr, "\\${"+varName+"}") } diff --git a/pkg/workflow/secret_extraction_test.go b/pkg/workflow/secret_extraction_test.go index 0c20a75dd57..368b0d72b7b 100644 --- a/pkg/workflow/secret_extraction_test.go +++ b/pkg/workflow/secret_extraction_test.go @@ -333,6 +333,71 @@ func TestSharedReplaceSecretsWithEnvVars(t *testing.T) { } } +// TestReplaceSecretsWithEnvVars_FallbackExpressionDeterminism verifies that when a single +// GitHub Actions expression references two secrets (e.g. a fallback pattern like +// "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}"), the replacement is +// deterministic across repeated calls. Previously, Go map iteration produced either +// "\${DD_APP_KEY}" or "\${DD_APPLICATION_KEY}" non-deterministically, causing +// smoke-otel-backends.lock.yml to differ between compiler runs. +func TestReplaceSecretsWithEnvVars_FallbackExpressionDeterminism(t *testing.T) { + // Both secrets map to the same expression — this is what ExtractSecretsFromMap + // produces for "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}". + sharedExpr := "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}" + secrets := map[string]string{ + "DD_APPLICATION_KEY": sharedExpr, + "DD_APP_KEY": sharedExpr, + } + + // Run many times to surface any non-determinism from Go map iteration. + const runs = 50 + var first string + for i := range runs { + got := ReplaceSecretsWithEnvVars(sharedExpr, secrets) + if i == 0 { + first = got + continue + } + if got != first { + t.Errorf("non-deterministic output: run 0 produced %q, run %d produced %q", first, i, got) + } + } + + // The alphabetically first key ("DD_APPLICATION_KEY") should win because it is processed + // first in sorted order and performs the replacement; the second key finds nothing + // to replace. Note: '_' (ASCII 95) > 'L' (ASCII 76), so "DD_APPLICATION_KEY" sorts + // before "DD_APP_KEY" (comparison diverges at position 6: 'L' < '_'). + want := "\\${DD_APPLICATION_KEY}" + if first != want { + t.Errorf("ReplaceSecretsWithEnvVars(%q, secrets) = %q, want %q", sharedExpr, first, want) + } +} + +// TestReplaceSecretsWithBashVars_FallbackExpressionDeterminism verifies that +// ReplaceSecretsWithBashVars produces deterministic output for fallback expressions +// with two secret references. Mirrors TestReplaceSecretsWithEnvVars_FallbackExpressionDeterminism. +func TestReplaceSecretsWithBashVars_FallbackExpressionDeterminism(t *testing.T) { + value := "${{ secrets.DD_APPLICATION_KEY || secrets.DD_APP_KEY }}" + + const runs = 50 + var first string + for i := range runs { + got := ReplaceSecretsWithBashVars(value) + if i == 0 { + first = got + continue + } + if got != first { + t.Errorf("non-deterministic output: run 0 produced %q, run %d produced %q", first, i, got) + } + } + + // "DD_APPLICATION_KEY" sorts before "DD_APP_KEY" ('L' < '_'), so it wins. + want := "${DD_APPLICATION_KEY}" + if first != want { + t.Errorf("ReplaceSecretsWithBashVars(%q) = %q, want %q", value, first, want) + } +} + // TestSharedExtractSecretsFromValueEdgeCases tests edge cases for the shared ExtractSecretsFromValue utility function func TestSharedExtractSecretsFromValueEdgeCases(t *testing.T) { tests := []struct {