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

Add iam_groups to vault_aws_secret_backend_role #826

Merged
merged 4 commits into from
Aug 19, 2020

Conversation

lawliet89
Copy link
Contributor

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

Release note for CHANGELOG:

Add `iam_groups` to `vault_aws_secret_backend_role`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSSecretBackendRole'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSSecretBackendRole -timeout 120m
?       github.com/terraform-providers/terraform-provider-vault [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/coverage    [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/generate    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/codegen (cached) [no tests to run]
?       github.com/terraform-providers/terraform-provider-vault/generated       [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/datasources/transform/decode  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/datasources/transform/encode  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/alphabet  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/role      (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/template  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/transformation    (cached) [no tests to run]
?       github.com/terraform-providers/terraform-provider-vault/schema  [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.41s)
=== RUN   TestAccAWSSecretBackendRole_import
--- PASS: TestAccAWSSecretBackendRole_import (0.32s)
=== RUN   TestAccAWSSecretBackendRole_nested
--- PASS: TestAccAWSSecretBackendRole_nested (0.33s)
PASS
ok      github.com/terraform-providers/terraform-provider-vault/vault   (cached)

...

Copy link
Member

@tvoran tvoran 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 adding this! It's looking ok, though the spacing/tabs seems a bit off in the tests?

vault/resource_aws_secret_backend_role_test.go Outdated Show resolved Hide resolved
vault/resource_aws_secret_backend_role_test.go Outdated Show resolved Hide resolved
vault/resource_aws_secret_backend_role_test.go Outdated Show resolved Hide resolved
@lawliet89
Copy link
Contributor Author

Thanks. The spacing/tabs is hard to differentiate in my VSCode.

Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

So I noticed while trying this out that removing the iam_groups from a role doesn't work quite right. Turns out it's because of the check for iamGroups of length 0 before setting that parameter in the data. I left a couple suggestions to fix this, and I think this will need another test step in TestAccAWSSecretBackendRole_basic() that tries removing the iam_groups.

(It looks like this is also an issue with policy_arns, so it's not surprising you ran into it.)

vault/resource_aws_secret_backend_role.go Outdated Show resolved Hide resolved
vault/resource_aws_secret_backend_role.go Outdated Show resolved Hide resolved
@lawliet89
Copy link
Contributor Author

lawliet89 commented Aug 19, 2020

Thanks for the suggestion.

I asked a member of the Vault team before and they said that if you send a parameter to a version of Vault that does not support it (in this case < 1.4), Vault would ignore it.

I think we might be able to simplify this by always sending the parameter whether the list is empty or not.

$ VAULT_ADDR=http://127.0.0.1:8200 VAULT_TOKEN=12345 AWS_ACCESS_KEY_ID=12345 AWS_SECRET_ACCESS_KEY=12345 make testacc TESTARGS='-run=TestAccAWSSecretBackendRole_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSSecretBackendRole_basic -timeout 120m
?       github.com/terraform-providers/terraform-provider-vault [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/coverage    [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/generate    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/codegen (cached) [no tests to run]
?       github.com/terraform-providers/terraform-provider-vault/generated       [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/datasources/transform/decode  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/datasources/transform/encode  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/alphabet  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/role      (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/template  (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/generated/resources/transform/transformation    (cached) [no tests to run]
?       github.com/terraform-providers/terraform-provider-vault/schema  [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.55s)
PASS
ok      github.com/terraform-providers/terraform-provider-vault/vault   0.577s

Test failures seem to be in

=== RUN   TestAccIdentityGroupMemberEntityIdsExclusive

--- FAIL: TestAccIdentityGroupMemberEntityIdsExclusive (0.23s)

    testing.go:640: Step 1 error: Check failed: Check 3/3 error: vault_identity_group_member_entity_ids.member_entity_ids: Attribute 'member_entity_ids.#' expected "1", got "2"

=== RUN   TestAccIdentityGroupMemberEntityIdsNonExclusiveEmpty

--- PASS: TestAccIdentityGroupMemberEntityIdsNonExclusiveEmpty (0.37s)

=== RUN   TestAccIdentityGroupMemberEntityIdsNonExclusive

--- FAIL: TestAccIdentityGroupMemberEntityIdsNonExclusive (0.23s)

    testing.go:640: Step 1 error: Check failed: Check 4/5 error: vault_identity_group_member_entity_ids.test: Attribute 'member_entity_ids.#' found when not expected

These tests seem to be quite flaky.

@lawliet89 lawliet89 requested a review from tvoran August 19, 2020 02:23
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Yep, I'm seeing flakiness with those tests too, though I did have all of them pass locally (along with the AWS tests) so I think we're good.

@tvoran tvoran merged commit 43e03cf into hashicorp:master Aug 19, 2020
@lawliet89 lawliet89 deleted the aws-groups branch August 20, 2020 02:03
@catsby
Copy link
Contributor

catsby commented Aug 28, 2020

Hello all - this was released in v2.13.0 , thanks for the contribution and your patience!

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.

4 participants