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 support to set default issuers configuration for PKI Secrets Engine #1937

Merged
merged 6 commits into from Jul 12, 2023

Conversation

vinay-gopalan
Copy link
Contributor

Adds a new resource with relevant tests and documentation that allows users to set the default issuer under a mount.

@fairclothjm fairclothjm self-assigned this Jul 12, 2023
var pkiSecretBackendFromConfigIssuersPathRegex = regexp.MustCompile("^(.+)/config/issuers")

const (
fieldDefaultFollowsLatestIssuer = "default_follows_latest_issuer"
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 thought this might be a good spot for an example to showcase that if a schema field seems very specific to only a single resource, we need not export it out of the package and needlessly bulk up our consts package. Instead, we can define an unexported constant within the same file. However, if this feels a bit unclean, I'm happy to move it to the constants package

leading or trailing `/`s.

* `default` - (Required) Specifies the default issuer using the issuer ID.
**NOTE:** It is recommended to only set the default issuer using the ID.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Vault, one can provide both the issuer name and issuer ID to set this field. However, since the Vault response only ever returns the issuer ID for this field, there can be a drift in the TF state if we provide an issuer name but Vault returns an issuer ID. I opted to document for now that we recommend only using the ID (since that is fairly easy to do in TF), but in the future if users feel like they specifically want to be able to set this parameter by the issuer name we can always add an additional read-only schema field that will store the ID separately (thereby removing any drifts)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! thanks for the context!

Copy link
Member

@robmonte robmonte left a comment

Choose a reason for hiding this comment

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

Looks good! Let me know if my question doesn't apply in this context.

vault/resource_pki_secret_backend_config_issuers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

LGTM! Just a few suggestions

vinay-gopalan and others added 2 commits July 12, 2023 13:19
Co-authored-by: John-Michael Faircloth <fairclothjm@users.noreply.github.com>
@vinay-gopalan vinay-gopalan merged commit 43da33d into main Jul 12, 2023
10 checks passed
@vinay-gopalan vinay-gopalan added this to the 3.18.0 milestone Jul 12, 2023
@vinay-gopalan vinay-gopalan deleted the VAULT-17755/add-pki-config-issuers branch July 12, 2023 21:00
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.

None yet

4 participants