From aef125662c3b36b09b75c20d3d8b74a5ba3a009c Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 08:42:54 +0000 Subject: [PATCH] chore(iac): remove the unused roleAssignmentGuidPrefix ARM parameter arm/CUDly-CrossSubscription/template.json declared roleAssignmentGuidPrefix with a [newGuid()] default and described it as the base GUID for deterministic role assignment names, but nothing in the template read it: the custom role name and all three assignment names are guid() seeds over the principal, a literal and subscription().subscriptionId. Delete it rather than wire it in. Wiring the newGuid() default into the seeds would rename every assignment on each redeploy, and Azure refuses a second assignment of the same role to the same principal at the same scope (RoleAssignmentExists), so existing customers could no longer redeploy. No shipped command passes the parameter (setup.sh and the known-issues.md redeploy commands pass only servicePrincipalObjectId), so removal breaks none of them. The new iac tests fail if a declared parameter goes unread, if a parameter defaults to newGuid(), or if a shipped deploy command for the template passes a parameter the template no longer declares. Closes #143 Co-Authored-By: Claude Opus 5.5 --- arm/CUDly-CrossSubscription/template.json | 7 -- iac/arm_cross_subscription_template_test.go | 132 ++++++++++++++++++++ 2 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 iac/arm_cross_subscription_template_test.go diff --git a/arm/CUDly-CrossSubscription/template.json b/arm/CUDly-CrossSubscription/template.json index 7a7a0c33e..b607ab07c 100644 --- a/arm/CUDly-CrossSubscription/template.json +++ b/arm/CUDly-CrossSubscription/template.json @@ -13,13 +13,6 @@ "metadata": { "description": "Object ID of the CUDly Azure AD service principal. Obtain with: az ad sp show --id --query id -o tsv" } - }, - "roleAssignmentGuidPrefix": { - "type": "string", - "defaultValue": "[newGuid()]", - "metadata": { - "description": "Base GUID used to derive deterministic role assignment names. Leave at default to auto-generate." - } } }, diff --git a/iac/arm_cross_subscription_template_test.go b/iac/arm_cross_subscription_template_test.go new file mode 100644 index 000000000..c070a066d --- /dev/null +++ b/iac/arm_cross_subscription_template_test.go @@ -0,0 +1,132 @@ +package iac + +import ( + "encoding/json" + "os" + "regexp" + "strings" + "testing" +) + +// These tests read the ARM template customers deploy into every onboarded +// Azure subscription, and the shipped commands that deploy it, from the repo +// tree (arm/ is not embedded; setup.sh and known-issues.md point customers at +// the file itself). + +const crossSubscriptionTemplate = "../arm/CUDly-CrossSubscription/template.json" + +// crossSubscriptionDeployCommandSources are the shipped files that tell a +// customer how to run `az deployment sub create` against the template. +var crossSubscriptionDeployCommandSources = []string{ + "../arm/CUDly-CrossSubscription/setup.sh", + "../known-issues.md", +} + +// readCrossSubscriptionTemplate returns the template's declared parameters and +// the rest of the template (everything but the parameters section) as JSON +// text, so a parameter's own declaration cannot count as a use of it. +func readCrossSubscriptionTemplate(t *testing.T) (map[string]json.RawMessage, string) { + t.Helper() + raw, err := os.ReadFile(crossSubscriptionTemplate) + if err != nil { + t.Fatalf("read %s: %v", crossSubscriptionTemplate, err) + } + var doc map[string]json.RawMessage + if err = json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("parse %s: %v", crossSubscriptionTemplate, err) + } + var params map[string]json.RawMessage + if err = json.Unmarshal(doc["parameters"], ¶ms); err != nil { + t.Fatalf("parse %s parameters: %v", crossSubscriptionTemplate, err) + } + if len(params) == 0 { + t.Fatalf("%s declares no parameters; expected at least servicePrincipalObjectId", crossSubscriptionTemplate) + } + delete(doc, "parameters") + body, err := json.Marshal(doc) + if err != nil { + t.Fatalf("re-marshal %s: %v", crossSubscriptionTemplate, err) + } + return params, string(body) +} + +// TestCrossSubscriptionTemplateReadsEveryParameter guards issue #143: the +// template declared roleAssignmentGuidPrefix, described as the base GUID for +// deterministic role assignment names, and nothing read it. A parameter the +// template ignores promises the customer a control that does nothing. +func TestCrossSubscriptionTemplateReadsEveryParameter(t *testing.T) { + params, body := readCrossSubscriptionTemplate(t) + for name := range params { + // ARM parameter names are case-insensitive, as is the function name. + ref := regexp.MustCompile(`(?i)parameters\(\s*'` + regexp.QuoteMeta(name) + `'\s*\)`) + if !ref.MatchString(body) { + t.Errorf("parameter %q is declared but no variable, resource or output reads parameters('%s')", name, name) + } + } +} + +// TestCrossSubscriptionTemplateHasNoNewGuidDefault keeps role assignment and +// role definition names deterministic. A [newGuid()] default yields a fresh +// value on every deployment, so feeding it into a guid(...) name seed would +// make each redeploy try to create a second assignment of the same role to the +// same principal at the same scope, which Azure refuses (RoleAssignmentExists). +func TestCrossSubscriptionTemplateHasNoNewGuidDefault(t *testing.T) { + params, _ := readCrossSubscriptionTemplate(t) + newGUID := regexp.MustCompile(`(?i)newGuid\s*\(`) + for name, decl := range params { + if newGUID.Match(decl) { + t.Errorf("parameter %q has a newGuid() default: %s", name, decl) + } + } +} + +// TestCrossSubscriptionDeployCommandsPassOnlyDeclaredParameters checks the +// other side of removing a parameter: ARM fails a deployment that supplies a +// parameter the template does not declare, so every `--parameters key=value` +// in a shipped deploy command for this template must name a declared one. +func TestCrossSubscriptionDeployCommandsPassOnlyDeclaredParameters(t *testing.T) { + params, _ := readCrossSubscriptionTemplate(t) + declared := make(map[string]bool, len(params)) + for name := range params { + declared[strings.ToLower(name)] = true + } + paramArg := regexp.MustCompile(`--parameters\s+"?([A-Za-z0-9_]+)=`) + + for _, src := range crossSubscriptionDeployCommandSources { + cmds := crossSubscriptionDeployCommands(t, src) + if len(cmds) == 0 { + t.Errorf("%s: found no `az deployment sub create` command for the cross-subscription template; update crossSubscriptionDeployCommandSources", src) + continue + } + for _, cmd := range cmds { + for _, m := range paramArg.FindAllStringSubmatch(cmd, -1) { + if !declared[strings.ToLower(m[1])] { + t.Errorf("%s: deploy command passes undeclared parameter %q, which ARM rejects:\n%s", src, m[1], cmd) + } + } + } + } +} + +// crossSubscriptionDeployCommands returns each `az deployment sub create` +// command in path that deploys the cross-subscription template, with its +// backslash continuation lines joined. +func crossSubscriptionDeployCommands(t *testing.T, path string) []string { + t.Helper() + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + joined := regexp.MustCompile(`\\\r?\n`).ReplaceAllString(string(raw), " ") + var cmds []string + for _, line := range strings.Split(joined, "\n") { + if !strings.Contains(line, "az deployment sub create") { + continue + } + if strings.Contains(line, "CUDly-CrossSubscription/template.json") || + strings.Contains(line, "${SCRIPT_DIR}/template.json") { + cmds = append(cmds, line) + } + } + return cmds +}