feat: ✨ Add support for `assume-role` block in S3 backend for Terraform #74

erikpaasonen · 2024-07-12T18:41:20Z

Proposed changes

Adds support for the assume_role = {} block that is the current syntax for assuming a role for an S3 backend in Terraform.

Issues for these changes

#73

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (changes to code, which do not change application behavior)

Checklist

I have filled out this PR template
I have read the CONTRIBUTING doc
I have added automated tests that prove my fix is effective or that my feature works
I have added necessary documentation (README.md, CHANGELOG.md, etc. - if appropriate)

Dependencies and Blockers

Relevant Links

Further comments

Note: this will break compatibility with Terraform v1.1 and below. (The assume-role block was added in v1.2.) For reference, the current version of Terraform is 1.9.

CLAassistant · 2024-07-12T18:41:25Z

All committers have signed the CLA.

erikpaasonen · 2024-07-12T19:09:08Z

All tests passing locally for me.

plugins/terraform/stepper.go

mmraz · 2024-11-05T14:36:01Z

plugins/terraform/stepper.go

+		if interpolatedRoleArn != roleArn {
+			roleArn = interpolatedRoleArn
+			b["assume_role"] = fmt.Sprintf("{\"role_arn\"=\"%s\"}", roleArn)
+		}


Dead assignment to roleArn as it's local scope ends at the close of line 374. We always want the interpolated value anyhow.

Suggested change

if interpolatedRoleArn != roleArn {

roleArn = interpolatedRoleArn

b["assume_role"] = fmt.Sprintf("{\"role_arn\"=\"%s\"}", roleArn)

}

if interpolatedRoleArn != roleArn {

b["assume_role"] = fmt.Sprintf("{\"role_arn\"=\"%s\"}", interpolatedRoleArn)

}

if this suggestion is taken, then line 373 will log the wrong i.e. non-interpolated value. the suggestion is the way I originally coded it.

line 370 is what preserves the correct value outside the local scope, and that is being set correctly. so this is really just semantics of how to also log the chosen value (since we want the log entry regardless of the if condition on line 369).

mmraz · 2024-11-05T14:40:22Z

plugins/terraform/stepper.go

+		// only track override config if interpolated is different from what user declared
+		if interpolatedBucket != bucket {
+			b["bucket"] = interpolatedBucket
+			bucket = interpolatedBucket


Dead assignment of bucket. Scope ends at line 387.

Suggested change

bucket = interpolatedBucket

ditto previous comment; the var bucket is logged before the local scope ends.

mmraz · 2024-11-05T14:41:11Z

plugins/terraform/stepper.go

+		// only track override config if interpolated is different from what user declared
+		if interpolatedBucket != bucket {
+			b["bucket"] = interpolatedBucket
+			bucket = interpolatedBucket


Dead assignment of bucket. Scope ends at line 400.

Suggested change

bucket = interpolatedBucket

ditto previous comment; the var bucket is logged before the local scope ends.

mmraz · 2024-11-05T14:42:28Z

plugins/terraform/stepper.go

+		// only track override config if interpolated is different from what user declared
+		if interpolatedPrefix != prefix {
+			b["prefix"] = interpolatedPrefix
+			prefix = interpolatedPrefix


Dead assignment of prefix. Scope ends at line 413

Suggested change

prefix = interpolatedPrefix

ditto previous comment; the var prefix is logged before the local scope ends.

mmraz · 2024-11-05T15:14:48Z

plugins/terraform/stepper_test.go

@@ -59,7 +59,7 @@ func TestGetBackendConfig_ShouldParseAssumeRoleCoreAccountIDMapCorrectly(t *test
 		}}, ParseTFBackend)

 	require.Equal(t, S3Backend, mockResult.Type)
-	require.Equal(t, fmt.Sprintf("arn:aws:iam::%s:role/OrganizationAccountAccessRole", DefaultStubAccountID), mockResult.Config["role_arn"])
+	require.Contains(t, mockResult.Config["assume_role"], fmt.Sprintf("arn:aws:iam::%s:role/OrganizationAccountAccessRole", DefaultStubAccountID))


This test is passing, but only because the paramater happens to have the same name for its key at both the deprecated depth and the newer, nested depth. The mock backend uses the deprecated syntax but never tests the newer syntax. It also appears that the parser is not extracting the role arn value from a specific depth during initial ingestion. The regex catches and parses both cases of a role_arn value at

runiac/plugins/terraform/terraform.go

Lines 113 to 119 in 2a60147

// RoleArn

rRegex, _ := regexp.Compile(`role_arn\s*=\s*"(.+)"`)

roleMatch := rRegex.FindStringSubmatch(s)

if len(roleMatch) > 0 {

backend.S3RoleArn = roleMatch[1]

}

I would suggest mocking both the deprecated syntax and the newer assume_role block as separate tests to validate behavior when encountering either one. This is also more likely to catch regressions for either case if they diverge in the future.

mmraz · 2024-11-05T15:50:15Z

plugins/terraform/stepper_test.go

@@ -502,7 +504,7 @@ func TestParseBackend_ShouldParseRoleArnWhenSet(t *testing.T) {
 	mockResult := ParseTFBackend(fs, logger, "testbackend.tf")

 	require.Equal(t, S3Backend, mockResult.Type)
-	require.Equal(t, "stubrolearn", mockResult.S3RoleArn)
+	require.Equal(t, "stubrolearn", mockResult.AssumeRole.RoleArn)


The mock input is only using the deprecated syntax without interpolation. It does not validate the newer backend.assume_role.role_arn syntax.

erikpaasonen added 3 commits July 12, 2024 14:30

refactor: 🎨 terraform fmt -recursive

0553011

chore: ⬆️ go.mod/go.sum updates

464c06e

feat: ✨ use non-deprecated assume-role block syntax for S3 TF backend

0481f4e

erikpaasonen added 3 commits July 12, 2024 15:03

🐛 some tests require 'key' always present

b409620

🐛 order-of-ops

f1bd823

test: ✅ tweaks to tests to reflect changes

d23d141

mmraz reviewed Nov 4, 2024

View reviewed changes

plugins/terraform/stepper.go Outdated Show resolved Hide resolved

fix: 🐛 misspelling in var name

01e179c

mmraz reviewed Nov 5, 2024

View reviewed changes

mmraz approved these changes Nov 8, 2024

View reviewed changes

mbpolan approved these changes Nov 11, 2024

View reviewed changes

mbpolan merged commit 75d8663 into Optum:main Nov 11, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ✨ Add support for `assume-role` block in S3 backend for Terraform #74

feat: ✨ Add support for `assume-role` block in S3 backend for Terraform #74

erikpaasonen commented Jul 12, 2024 •

edited

Loading

CLAassistant commented Jul 12, 2024 •

edited

Loading

erikpaasonen commented Jul 12, 2024

mmraz Nov 5, 2024

erikpaasonen Nov 8, 2024

mmraz Nov 5, 2024

erikpaasonen Nov 8, 2024

mmraz Nov 5, 2024

erikpaasonen Nov 8, 2024

mmraz Nov 5, 2024

erikpaasonen Nov 8, 2024

mmraz Nov 5, 2024

mmraz Nov 5, 2024

	// RoleArn
	rRegex, _ := regexp.Compile(`role_arn\s=\s"(.+)"`)
	roleMatch := rRegex.FindStringSubmatch(s)

	if len(roleMatch) > 0 {
	backend.S3RoleArn = roleMatch[1]
	}

feat: ✨ Add support for assume-role block in S3 backend for Terraform #74

feat: ✨ Add support for assume-role block in S3 backend for Terraform #74

Conversation

erikpaasonen commented Jul 12, 2024 • edited Loading

Proposed changes

Issues for these changes

Types of changes

Checklist

Dependencies and Blockers

Relevant Links

Further comments

CLAassistant commented Jul 12, 2024 • edited Loading

erikpaasonen commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: ✨ Add support for `assume-role` block in S3 backend for Terraform #74

feat: ✨ Add support for `assume-role` block in S3 backend for Terraform #74

erikpaasonen commented Jul 12, 2024 •

edited

Loading

CLAassistant commented Jul 12, 2024 •

edited

Loading