Add iam_tags to vault_aws_secret_backend_role resource #2087

pporee · 2023-11-15T13:45:46Z

Description

Relates OR Closes #0000

Checklist

Added CHANGELOG entry (only for user-facing changes)
Acceptance tests where run against all supported Vault Versions

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

This reverts commit b2223e6.

hashicorp-cla · 2023-11-15T13:45:50Z

All committers have signed the CLA.

thyton

Hi @pporee, this PR seems to break some of the acceptance tests (see below for an example). Local test setup guides can be found at https://github.com/hashicorp/terraform-provider-vault/blob/main/README.md#developing-the-provider. Could you take a look and drop your acceptance tests' output in the description like #781's?

~/go/src/github.com/community/pporee/terraform-provider-vault (main) $ make testacc TESTARGS='-run=TestAccAWSSecretBackendRole'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test -run=TestAccAWSSecretBackendRole -timeout 30m ./...
?   	github.com/hashicorp/terraform-provider-vault	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/coverage	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/generate	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/helper	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/consts	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/identity/group	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/identity/mfa	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/pki	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/codegen	(cached) [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/internal/identity/entity	(cached) [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/schema	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/internal/provider	(cached) [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/testutil	(cached) [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/util	(cached) [no tests to run]
--- FAIL: TestAccAWSSecretBackendRole_import (2.63s)
    resource_aws_secret_backend_role_test.go:61: Step 2/6 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.
        
          map[string]string{
        - 	"iam_tags.%":    "2",
        - 	"iam_tags.key1": "value1",
        - 	"iam_tags.key2": "value2",
          }
FAIL
FAIL	github.com/hashicorp/terraform-provider-vault/vault	12.007s
FAIL
make: *** [testacc] Error 1

Please let us know if you need any help.

Thanks,
Thy

thyton · 2023-11-17T01:08:52Z

vault/resource_aws_secret_backend_role_test.go

@@ -145,6 +145,8 @@ func testAccAWSSecretBackendRoleCheckBasicAttributes(name, backend string) resou
 		resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "backend", backend),
 		testutil.TestCheckResourceAttrJSON("vault_aws_secret_backend_role.test_policy_inline", "policy_document", testAccAWSSecretBackendRolePolicyInline_basic),
 		resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_groups.#", "0"),


I think another test for the map's key length can ensure the map value.

Suggested change

resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_groups.#", "0"),

resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_groups.#", "0"),

resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_tags.%", "2"),

thyton · 2023-11-17T01:11:33Z

vault/resource_aws_secret_backend_role_test.go

@@ -177,6 +179,8 @@ func testAccAWSSecretBackendRoleCheckUpdatedAttributes(name, backend string) res
 		resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "default_sts_ttl", "3600"),
 		resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "max_sts_ttl", "21600"),
 		resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_groups.#", "2"),


Suggested change

resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_groups.#", "2"),

resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_groups.#", "2"),

resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_tags.%", "2"),

thyton · 2023-11-17T01:29:15Z

vault/resource_aws_secret_backend_role.go

@@ -123,6 +131,8 @@ func awsSecretBackendRoleWrite(d *schema.ResourceData, meta interface{}) error {

 	iamGroups := d.Get("iam_groups").(*schema.Set).List()

+	iamTags, iamTagsOk := d.GetOk("iam_tags")


Get will return the data for the given key, or nil if the key doesn't exist in the schema.

Suggested change

iamTags, iamTagsOk := d.GetOk("iam_tags")

iamTags := d.Get("iam_tags")

thyton · 2023-11-17T01:31:47Z

vault/resource_aws_secret_backend_role.go

@@ -155,6 +165,9 @@ func awsSecretBackendRoleWrite(d *schema.ResourceData, meta interface{}) error {
 	if d.HasChange("iam_groups") {
 		data["iam_groups"] = iamGroups
 	}
+	if iamTagsOk {


We should also check if the value has changed, following the current practice from other fields/keys.

Suggested change

if iamTagsOk {

if d.HasChange("iam_tags") {

thyton · 2023-11-17T02:24:59Z

vault/resource_aws_secret_backend_role_test.go

@@ -233,6 +241,10 @@ resource "vault_aws_secret_backend_role" "test_policy_arns" {
  policy_arns = ["%s"]
  credential_type = "iam_user"
  backend = vault_aws_secret_backend.test.path
+  iam_tags = {


Looks like resource.TestCheckResourceAttr(...) tests for iam_tags of resource "vault_aws_secret_backend_role.test_policy_arns" are missing.

Could you please enhance its test coverage like how you did for resource "vault_aws_secret_backend_role.test_policy_inline"?

thyton · 2023-11-17T02:35:16Z

vault/resource_aws_secret_backend_role_test.go

+  iam_tags = {
+    "key1" = "value1"
+    "key2" = "value2"
+  }


Similar comment to the above to enhance test coverage.

thyton · 2023-11-17T02:35:37Z

vault/resource_aws_secret_backend_role_test.go

+  iam_tags = {
+    "key1" = "value1"
+    "key2" = "value2"
+  }


Similar comment to the above to enhance test coverage.

thyton · 2023-11-17T02:36:03Z

vault/resource_aws_secret_backend_role_test.go

+  iam_tags = {
+    "key1" = "value1"
+    "key2" = "value2"
+  }


Similar comment to the above to enhance test coverage.

thyton · 2023-11-17T05:09:02Z

vault/resource_aws_secret_backend_role_test.go

+	iam_tags = {
+		"key1" = "value1"
+		"key2" = "value2"
+	  }


Suggested change

iam_tags = {

"key1" = "value1"

"key2" = "value2"

}

iam_tags = {

"key1" = "value1"

"key2" = "value2"

}

thyton · 2023-11-17T05:09:51Z

vault/resource_aws_secret_backend_role_test.go

+	iam_tags = {
+		"key1" = "value1"
+		"key2" = "value2"
+	  }


Suggested change

iam_tags = {

"key1" = "value1"

"key2" = "value2"

}

iam_tags = {

"key1" = "value1"

"key2" = "value2"

}

Shocktrooper · 2024-04-14T22:47:12Z

Hello! I am interested in this because it will allow us to use AWS policy variables in our IAM policies. Without this we cannot do that and thus cannot use AWS Policy variables based off of tags

https://docs.aws.amazon.com/IAM/latest/UserGuide/access_tags.html

Shocktrooper · 2024-04-14T22:50:09Z

@thyton Can you check this code again and see if there are any other changes needed other than what you have reviewed since its a few months old now. if @pporee cannot complete this I can go ahead and try to finish it with a new MR

thyton · 2024-04-30T19:12:10Z

@Shocktrooper Thank you for your patience! I've created a WIP #2231 to finish the remaining work and close this PR.
I'm expecting to close #2231 within 2 weeks.

Shocktrooper · 2024-05-01T01:31:57Z

Thank you for picking this up! @thyton Let me know if there is anything I can do

thyton · 2024-05-01T21:23:47Z

@pporee @Shocktrooper This feature will be out in release 4.3.0. Thank you again for your patience!

pporee added 4 commits November 15, 2023 14:39

add iam_tags

b2223e6

Revert "add iam_tags"

7409ef9

This reverts commit b2223e6.

add iam_tags

32457e1

update tests

cc1b54e

github-actions bot added the size/M label Nov 15, 2023

thyton self-requested a review November 15, 2023 22:32

thyton requested changes Nov 17, 2023

View reviewed changes

thyton closed this Apr 30, 2024

thyton mentioned this pull request May 1, 2024

add iam_tags to vault_aws_secret_backend_role #2231

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add iam_tags to vault_aws_secret_backend_role resource #2087

Add iam_tags to vault_aws_secret_backend_role resource #2087

pporee commented Nov 15, 2023 •

edited

Loading

hashicorp-cla commented Nov 15, 2023 •

edited

Loading

thyton left a comment •

edited

Loading

thyton Nov 17, 2023

thyton Nov 17, 2023

thyton Nov 17, 2023

thyton Nov 17, 2023

thyton Nov 17, 2023

thyton Nov 17, 2023

thyton Nov 17, 2023

thyton Nov 17, 2023

thyton Nov 17, 2023

thyton Nov 17, 2023

Shocktrooper commented Apr 14, 2024

Shocktrooper commented Apr 14, 2024

thyton commented Apr 30, 2024

Shocktrooper commented May 1, 2024

thyton commented May 1, 2024

	resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_groups.#", "0"),
	resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_groups.#", "0"),
	resource.TestCheckResourceAttr("vault_aws_secret_backend_role.test_policy_inline", "iam_tags.%", "2"),

		@@ -123,6 +131,8 @@ func awsSecretBackendRoleWrite(d *schema.ResourceData, meta interface{}) error {

		iamGroups := d.Get("iam_groups").(*schema.Set).List()

		iamTags, iamTagsOk := d.GetOk("iam_tags")

	iamTags, iamTagsOk := d.GetOk("iam_tags")
	iamTags := d.Get("iam_tags")

Add iam_tags to vault_aws_secret_backend_role resource #2087

Add iam_tags to vault_aws_secret_backend_role resource #2087

Conversation

pporee commented Nov 15, 2023 • edited Loading

Description

Checklist

Output from acceptance testing:

Community Note

hashicorp-cla commented Nov 15, 2023 • edited Loading

thyton left a comment • edited Loading

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

Choose a reason for hiding this comment

Shocktrooper commented Apr 14, 2024

Shocktrooper commented Apr 14, 2024

thyton commented Apr 30, 2024

Shocktrooper commented May 1, 2024

thyton commented May 1, 2024

pporee commented Nov 15, 2023 •

edited

Loading

hashicorp-cla commented Nov 15, 2023 •

edited

Loading

thyton left a comment •

edited

Loading