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

Identity improvements #321

Merged

Conversation

nazarewk
Copy link
Contributor

@nazarewk nazarewk commented Feb 25, 2019

This PR implements #320 ideas, except for group membership management.
Fixes #343
Fixes #320

Major change

Adds vault_identity_group_policies resource and vault_identity_group.external_policies attribute.
It solves chicken & egg problem of using vault_identity_group.id inside templated vault_policy.

Bug fixes

  1. adds missing error checks in Go d.Set(...)
  2. as a result fixes vault_identity_entity.disabled not working at all (used TypeString instead of expected TypeBool)
  3. fixes error messages being about AppRole instead of specific Identity* in vault_identity_* resources

Minor changes

  1. enables passthrough importers on vault_identity_* resources
  2. enables name autogeneration (by default Vault assigns group-<UUID> or entity-<UUID> names) by Required: true -> Optional: true
  3. enables renaming vault_identity_entity and vault_identity_group resources (ForceNew: false was enough),

@ghost ghost added the size/XL label Feb 25, 2019
@nazarewk nazarewk force-pushed the feature/identity-improvements branch from 2fe2243 to 3c4a5a6 Compare February 25, 2019 11:11
@nazarewk
Copy link
Contributor Author

I guess the test was a random failure, closing and reopening

@nazarewk nazarewk closed this Feb 25, 2019
@nazarewk nazarewk reopened this Feb 25, 2019
Copy link
Contributor

@lawliet89 lawliet89 left a comment

Choose a reason for hiding this comment

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

I would like to see this merged in. Would be useful to manage policies for entities and groups.

If you would like, I could help work on the suggestions next week.

vault/resource_identity_entity.go Show resolved Hide resolved
@@ -53,6 +55,13 @@ func identityGroupResource() *schema.Resource {
Description: "Policies to be tied to the group.",
},

"external_policies": {
Copy link
Contributor

@lawliet89 lawliet89 Mar 8, 2019

Choose a reason for hiding this comment

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

This field seems a bit counter-intuitive if you compare it with the google_xxx_iam_* or other similar resources.

May I suggest that:

  • if policies is not provided, do a DiffSuppressFunc to suppress a diff on policies
  • Likewise, do not set policies while writing the entity.

Then, users can use the new resources you have added to manage policies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is possible to tell whether the value was set by user or read from the Vault.

vault/resource_identity_group_policies.go Show resolved Hide resolved
@nazarewk nazarewk force-pushed the feature/identity-improvements branch from 3c4a5a6 to 1f484ac Compare March 8, 2019 10:02
nazarewk added 3 commits March 8, 2019 11:43
…pplicable

Signed-off-by: Krzysztof Nazarewski <3494992+nazarewk@users.noreply.github.com>
…ment

Signed-off-by: Krzysztof Nazarewski <3494992+nazarewk@users.noreply.github.com>
…lusive

Signed-off-by: Krzysztof Nazarewski <3494992+nazarewk@users.noreply.github.com>
@cvbarros
Copy link
Contributor

@nazarewk since you got this underway, would you mind breaking a few more eggs and addressing the problem described in #343?
Apparently the reserved id field in the resource schema for these resources is causing problems with Terraform 0.12. AFAIK, I've never implmented any resourced that had an id field set in the schema, just always used SetId() to make the attribute available for output.

@nazarewk
Copy link
Contributor Author

@cvbarros i'm not sure what is the interaction of id attribute, tests are passing after simply removing the field, so i will stick with it.

Signed-off-by: Krzysztof Nazarewski <3494992+nazarewk@users.noreply.github.com>
@cvbarros
Copy link
Contributor

@nazarewk with previous Terraform versions, one could get away of having id attribute in the resource schema with just a warning.

What happens is that it is a common mistake to assume that in order to have a resource's id attribute exported, one would have to add it to the schema. This is not the case, since Terraform exposes it by default. It's safe (and recommended, since TF 0.12 is being more strict) to remove it.

@nazarewk
Copy link
Contributor Author

Is there some maintainer participating in conversation? It is ready for merging and I would like to proceed.

@cvbarros
Copy link
Contributor

@nazarewk I think repo is undergoing a codefreeze at the moment for #356. It's a pretty big change, and probably all PRs will have to be rebased once that biggie is done.

@tyrannosaurus-becks
Copy link
Contributor

Thanks @cvbarros and @nazarewk ! The fix for #343 would indeed be nice to have but can also be done separately, so I will go ahead and take this one in as-is. Much appreciated!

@tyrannosaurus-becks tyrannosaurus-becks merged commit fc8908e into hashicorp:master Mar 29, 2019
@AndresPineros
Copy link

AndresPineros commented May 23, 2019

Hello, I just downloaded Terraform 0.12 (latest release, not beta or rc) and I'm getting this issue:
Error: Failed to instantiate provider "vault" to obtain schema: Incompatible API version with plugin. Plugin version: 4, Client versions: [5]

I don't really know the src of Terraform of the provider, can anyone explain why this happen? Is there a solution?

Thanks 👍

@nazarewk nazarewk deleted the feature/identity-improvements branch December 19, 2019 10:07
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with 0.12-beta dev build Identity secret backend improvements
5 participants