Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
hashivault_kv auth_path moved from metadata to inputs #7991
hashivault_kv auth_path moved from metadata to inputs #7991
Changes from 1 commit
00fc5f6
7c8e5ac
cf5d1a2
16fdf0e
e8b54ab
08c9219
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more than one auth path can be available to a given AppRole role_id and AppRole secret_id, which may be the reason why the auth path was originally set as a metadata variable instead of an input.
However, I do see the rationale for grouping the auth path with the inputs instead because it is probably more convenient to just make a separate hashi credential for each auth_login path than it would be to provide the same login paths every time you link the hashi credential to an input field.
Based on my understanding of https://learn.hashicorp.com/tutorials/vault/approle#policy-requirements, I think this pr is probably the way to go. An app may have multiple approle auth paths it can use, but it doesn't seem like you'd typically use more than one at a time for most situations.
That said, I may be overlooking a use case that depends on the current configuration.
cc @kawsark @ryanpetrello Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth mentioning that changing this is likely going to be a breaking change and potentially require a messy data migration for people who've already set these up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with Ryan. Also often larger customers have 100s of AppRole Auth methods setup (typically one per application) and it might be cumbersome to create a new hashi credential for each auth_login path. Usually the auth login path can be determined by the CI/CD Pipeline based on variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great info, thank you @kawsark. This is a compelling reason to leave the current variable as-is.
I think this implies linking input fields to external credentials dynamically in a ci job run? (cool!)
Do you feel that there is also a (separate) use case for keeping an auth path scoped to the whole hashi credential? Perhaps we can accommodate both use cases with a default approle auth path that lives in the inputs. The default will be overridden by any metadata auth paths that are provided.
Pros:
Cons:
cc @ryanpetrello @wenottingham
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your reviews
The way approle works is:
approle
auth method at a specific path in Vault. Using this path you will be able to interact with this approlerole_id
and you can create severalsecret_id
role_id
andsecret_id
to login using the path of the approle auth methodThe tuple
role_id
andsecret_id
depend on only one approle (you don't need to specify the role name during login as arole_id
match only one role)As you can see a tuple
role_id
andsecret_id
match only one approle auth method, only one pathI don't think it's possible to use an approle to auth against several auth methods as the role_id is generated by Vault for each role, that's why I made this change
@jakemcdermott
That's right but the app will need a
role_id
and asecret_id
for each approle auth path (and also for different roles at the same approle auth path)@ryanpetrello
That's completely right but having the approle auth path elsewhere than in the credential is only data duplication
@kawsark
I can't see the use case you're talking about as, from my point of view, there is already different credentials for each auth_login path
To add more context, we are using Vault since several years now and we're using several approles/roles
But we started the AWX setup recently so maybe I'm missing something on a pure AWX point of view ? (on the way credentials are stored/used ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great conversation and there is some excellent information here. Thanks.
problem: 1:
problem: 2:
problem 3:
Adding a default auth_path to the input fields and overriding it w/ the metadata path if both are provided will address all of these problems and concerns. We can explain the override behavior with appropriate variable naming and informative help text on the fields. Thoughts? Other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the right way to do it 👍
I'll work on it then submit a new commit for review
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbayszczak
Sounds good - thank you.