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

Fix external group policies getting cleared on group updates #1256

Closed
wants to merge 4 commits into from

Conversation

lawliet89
Copy link
Contributor

@lawliet89 lawliet89 commented Dec 9, 2021

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 is a follow up to #1061

That PR introduced a behaviour where any update to the group will clear the entire list of policies that's managed externally on the group. Sending nothing to Vault for policies in an API call seems to clear all the policies on the group. This PR proposes a change to simply read from the API and set policies to whatever the API returns.

Release note for CHANGELOG:

- Fix externally managed policies on `vault_identity_group` being cleared on a group update

Output from acceptance testing:

$ VAULT_ADDR=http://127.0.0.1:8200 VAULT_TOKEN=12345 TESTARGS="--run TestAccIdentityGroup" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test --run TestAccIdentityGroup -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]
ok      github.com/hashicorp/terraform-provider-vault/codegen   (cached) [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/generated [no test files]
ok      github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/decode    0.017s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/encode    0.017s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/alphabet    0.019s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/role        0.017s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/template    0.018s [no tests to run]
ok      github.com/hashicorp/terraform-provider-vault/generated/resources/transform/transformation      0.017s [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/helper    [no test files]
?       github.com/hashicorp/terraform-provider-vault/internal/consts   [no test files]
ok      github.com/hashicorp/terraform-provider-vault/internal/identity/entity  (cached) [no tests to run]
?       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/internal/provider (cached) [no tests to run]
?       github.com/hashicorp/terraform-provider-vault/schema    [no test files]
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]
ok      github.com/hashicorp/terraform-provider-vault/vault     24.702s

@github-actions github-actions bot added the size/M label Dec 9, 2021
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@lawliet89
Copy link
Contributor Author

lawliet89 commented Jan 26, 2023

Fixed conflicts and test. Can I get someone to review please.

@fairclothjm
Copy link
Contributor

@lawliet89 I apologize for the delay! If you are willing to resolve the merge conflicts I am happy to review. Thanks!

@lawliet89
Copy link
Contributor Author

@fairclothjm It has been years since I made this PR and I am not keen to do further testing. I think I've resolved the conflict correctly, but please verify.

@fairclothjm
Copy link
Contributor

Thanks @lawliet89 ! Looks like this is already fixed by #2084

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants