Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws_secret_backend: allow role_arns with policy_arns #710

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

adongy
Copy link
Contributor

@adongy adongy commented Mar 17, 2020

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

Closes #709

Release note for CHANGELOG:

aws_secret_backend: when `credential_type = assumed_role`, allow `policy_arns`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSSecretBackend'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSSecretBackend -timeout 120m
?   	github.com/terraform-providers/terraform-provider-vault	[no test files]
?   	github.com/terraform-providers/terraform-provider-vault/cmd/coverage	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-vault/util	(cached) [no tests to run]
=== RUN   TestAccAWSSecretBackendRole_basic
--- PASS: TestAccAWSSecretBackendRole_basic (0.23s)
=== RUN   TestAccAWSSecretBackendRole_import
--- PASS: TestAccAWSSecretBackendRole_import (0.19s)
=== RUN   TestAccAWSSecretBackendRole_nested
--- PASS: TestAccAWSSecretBackendRole_nested (0.27s)
=== RUN   TestAccAWSSecretBackend_basic
--- PASS: TestAccAWSSecretBackend_basic (0.25s)
=== RUN   TestAccAWSSecretBackend_import
--- PASS: TestAccAWSSecretBackend_import (0.12s)
PASS
ok  	github.com/terraform-providers/terraform-provider-vault/vault	1.070s

Tests did not update. I let the Vault binary provide validation as done here: https://github.com/hashicorp/vault/blob/master/builtin/logical/aws/path_roles.go

I did not change the code for the deprecated attributes (as they have Removed). Website documentation has been copied from the Vault website and reordered.

The sample Terraform has been updated, as only specifying a policy_document with credential_type = assumed_role is illegal.

Thanks!

Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. I'm thinking we shouldn't remove most of the ConflictsWith fields, but am looking forward to hearing your thoughts about it.

@@ -36,10 +36,9 @@ func awsSecretBackendRoleResource() *schema.Resource {
Description: "The path of the AWS Secret Backend the role belongs to.",
},
"policy_arns": {
Type: schema.TypeSet,
Optional: true,
ConflictsWith: []string{"policy", "policy_arn", "role_arns"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I'd be fine with pulling role_arns out here. However, I'm not so sure we should pull out the rest. Adding these deprecated parameters to ConflictsWith here is a normal part of the best practices around deprecation located here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

That does seem reasonable as we're still not at the next major version bump.

@ghost ghost added size/XS and removed size/S labels Mar 31, 2020
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks @adongy !

@tyrannosaurus-becks tyrannosaurus-becks merged commit 3f89dde into hashicorp:master Mar 31, 2020
@adongy adongy deleted the adong/conflicts branch April 3, 2020 07:41
dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vault_aws_secret_backend_role: role_arns conflicts with policy_arns while it should not
2 participants