Skip to content

Commit 91ca3c0

Browse files
committed
fix(iac/gcp): drop attribute.account so both WIF onboarding paths agree
The Terraform module maps three attributes on the AWS branch (google.subject, attribute.aws_role, attribute.account) while arm/CUDly-CrossSubscription/setup-gcp-wif.sh maps only the first two. Nothing reads attribute.account: its only two references in the tree were the mapping line itself and the comment above it. It is not named by the attribute_condition, by the impersonation grant's principalSet, or by any test. Today that divergence is inert, because the script compares only the provider's attributeCondition. Once the script also compares attributeMapping as an exact set, it stops being inert: both paths default to pool 'cudly-pool' and provider 'cudly-provider', so a customer onboarded via the Terraform bundle who then runs the script against the same project lands on this module's provider, fails the set comparison on the one extra key, and is told to delete the provider and re-run. That detaches every live federated session, and the next terraform apply then fights the recreated provider. The verdict would also be wrong: the mapping is not unsafe, it just carries one extra unused key. Removing the key rather than teaching the script to expect it keeps the comparison an exact set, which is what stops an unexpected attribute from being introduced later and keyed on by a pool-wide principalSet://.../attribute.X/... grant. Account pinning is unaffected: the provider's aws.account_id block and the account number inside the pinned role ARN both still constrain it. Also corrects the comment above the mapping. It specifically anticipated this attribute and concluded it was inert because the script "compares conditions, not mappings" - true when written, false as soon as the script compares mappings, and a future reader would have trusted it. Migration: attribute_mapping is not ForceNew in the google provider (the field is Optional with no ForceNew, and Update appends "attributeMapping" to the PATCH updateMask), so dropping a key is an in-place update. It rides along with the change this branch already makes to the same field and adds no replacement risk. TestGCPTargetMappingMatchesSetupScript locks the two paths to the same mapping set, parsing both files rather than restating either, and fails if a key is ever added on one side alone. Refs #1667
1 parent bdcc8c8 commit 91ca3c0

2 files changed

Lines changed: 106 additions & 8 deletions

File tree

‎iac/federation/gcp-target/terraform/main.tf‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -123,17 +123,24 @@ resource "google_iam_workload_identity_pool_provider" "cudly" {
123123
# refused. STS drops the path from assumed-role ARNs, so the same trick does
124124
# not work through an IAM role.
125125
#
126-
# This expression, the attribute_condition below, and the impersonation grant's
127-
# member are byte-identical to their counterparts in
128-
# arm/CUDly-CrossSubscription/setup-gcp-wif.sh, so the two customer-facing
129-
# onboarding paths pin the same identity and the script's condition check
130-
# accepts a provider this module created. (The script does not map
131-
# attribute.account, which is mapped here and unused by the condition; it
132-
# compares conditions, not mappings, so the extra attribute is inert there.)
126+
# This expression, the attribute_condition below, the impersonation grant's
127+
# member, and the set of keys mapped here are byte-identical to their
128+
# counterparts in arm/CUDly-CrossSubscription/setup-gcp-wif.sh, so the two
129+
# customer-facing onboarding paths pin the same identity and configure the
130+
# same provider. The two paths do converge on one provider rather than staying
131+
# independent: both default to pool 'cudly-pool' and provider
132+
# 'cudly-provider', so a customer who applies this module and then runs the
133+
# script lands on the provider this resource created.
134+
#
135+
# Do not map an attribute here without mapping it in the script too. The
136+
# script refuses to reuse a provider whose configuration differs from the one
137+
# it would have written, and the only remedy it offers is deleting the
138+
# provider, which detaches every live federated session. An extra key on one
139+
# side alone is enough to trigger that, even when nothing reads its value.
140+
# TestGCPTargetMappingMatchesSetupScript guards the parity.
133141
attribute_mapping = var.provider_type == "aws" ? {
134142
"google.subject" = "assertion.arn"
135143
"attribute.aws_role" = "assertion.arn.contains('assumed-role') ? assertion.arn.extract('{account_arn}assumed-role/') + 'assumed-role/' + assertion.arn.extract('assumed-role/{role_name}/') : assertion.arn"
136-
"attribute.account" = "assertion.account"
137144
} : var.oidc_attribute_mapping
138145

139146
# Both branches are non-null: aws_role_name and oidc_subject are validated

‎iac/gcp_target_wif_test.go‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package iac
22

33
import (
4+
"os"
45
"regexp"
6+
"slices"
57
"strings"
68
"testing"
79
)
@@ -119,6 +121,95 @@ func TestGCPTargetUpgradeOrderingIsPinned(t *testing.T) {
119121
}
120122
}
121123

124+
// setupScript is the shell onboarding path. It is the second way a customer can
125+
// configure the same pool and provider; both paths default to pool 'cudly-pool'
126+
// and provider 'cudly-provider', so a customer who uses one and then the other
127+
// lands on the same provider rather than on two independent ones.
128+
const setupScript = "../arm/CUDly-CrossSubscription/setup-gcp-wif.sh"
129+
130+
// awsMappingBlock captures the body of the AWS branch of attribute_mapping in
131+
// main.tf, i.e. everything between the ternary's opening brace and the OIDC
132+
// alternative.
133+
var awsMappingBlock = regexp.MustCompile(`(?s)attribute_mapping = var\.provider_type == "aws" \? \{(.*?)\n\s*\} : var\.oidc_attribute_mapping`)
134+
135+
// hclMappingPair matches one `"key" = "value"` entry inside that block.
136+
var hclMappingPair = regexp.MustCompile(`(?m)^\s*"([^"]+)"\s*=\s*"(.*)"\s*$`)
137+
138+
// scriptMappingLiteral captures the quoted, comma-delimited mapping dict the
139+
// script hands to gcloud's --attribute-mapping. It is anchored on the
140+
// attribute.aws_role key so it selects the AWS dict and not the OIDC one.
141+
var scriptMappingLiteral = regexp.MustCompile(`"([^"]*attribute\.aws_role=[^"]*)"`)
142+
143+
// tfAWSMapping returns main.tf's AWS attribute_mapping as sorted "key=value"
144+
// entries, the form both sides are compared in.
145+
func tfAWSMapping(t *testing.T) []string {
146+
t.Helper()
147+
148+
block := awsMappingBlock.FindStringSubmatch(readMainTF(t))
149+
if block == nil {
150+
t.Fatalf("%s: AWS branch of attribute_mapping not found", gcpTargetMainTF)
151+
}
152+
pairs := hclMappingPair.FindAllStringSubmatch(block[1], -1)
153+
if len(pairs) == 0 {
154+
t.Fatalf("%s: AWS attribute_mapping has no entries", gcpTargetMainTF)
155+
}
156+
157+
got := make([]string, 0, len(pairs))
158+
for _, p := range pairs {
159+
got = append(got, p[1]+"="+p[2])
160+
}
161+
slices.Sort(got)
162+
return got
163+
}
164+
165+
// scriptAWSMapping returns the script's AWS attribute mapping as sorted
166+
// "key=value" entries. Every literal carrying the mapping must agree, so the
167+
// script cannot create a provider with one mapping and reconcile against
168+
// another.
169+
func scriptAWSMapping(t *testing.T) []string {
170+
t.Helper()
171+
172+
raw, err := os.ReadFile(setupScript)
173+
if err != nil {
174+
t.Fatalf("read %s: %v", setupScript, err)
175+
}
176+
literals := scriptMappingLiteral.FindAllStringSubmatch(string(raw), -1)
177+
if len(literals) == 0 {
178+
t.Fatalf("%s: no --attribute-mapping dict containing attribute.aws_role found", setupScript)
179+
}
180+
for _, l := range literals[1:] {
181+
if l[1] != literals[0][1] {
182+
t.Fatalf("%s: AWS attribute mapping is spelled two different ways:\n%s\n%s", setupScript, literals[0][1], l[1])
183+
}
184+
}
185+
186+
got := strings.Split(literals[0][1], ",")
187+
slices.Sort(got)
188+
return got
189+
}
190+
191+
// TestGCPTargetMappingMatchesSetupScript pins the two onboarding paths to the
192+
// same attribute mapping.
193+
//
194+
// The script refuses to reuse a provider whose configuration differs from the
195+
// one it would have written, so a mapping this module writes but the script
196+
// does not expect is not a cosmetic divergence: a customer onboarded via the
197+
// Terraform bundle who later runs the script against the same project is told
198+
// to delete the provider and start over, which detaches every live federated
199+
// session and which the next terraform apply then fights. An extra key does
200+
// that even when nothing reads its value, so the whole set is compared here,
201+
// not just the keys the attribute_condition and the impersonation grant consume.
202+
func TestGCPTargetMappingMatchesSetupScript(t *testing.T) {
203+
tfMapping := tfAWSMapping(t)
204+
scriptMapping := scriptAWSMapping(t)
205+
206+
if !slices.Equal(tfMapping, scriptMapping) {
207+
t.Errorf("AWS attribute mapping differs between the two onboarding paths; a provider created by one is unusable by the other.\n%s:\n %s\n%s:\n %s",
208+
gcpTargetMainTF, strings.Join(tfMapping, "\n "),
209+
setupScript, strings.Join(scriptMapping, "\n "))
210+
}
211+
}
212+
122213
// extractTemplate models the extract() function available in GCP's workload
123214
// identity attribute-mapping CEL. The template is a literal containing exactly
124215
// one {placeholder}; the result is the text between the literal prefix and

0 commit comments

Comments
 (0)