-
Notifications
You must be signed in to change notification settings - Fork 25
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 alias types for IAM and GCE logins #89
Conversation
Resolves hashicorp/vault#8761 Allows users to specify an alias type field for IAM and GCE logins which will then switch between the current behavior (a unique ID for IAM, and the instance ID for GCE) and a newly created role_id field. The role_id is a UUID generated when the role is created. All existing roles without a role_id will have one generated and saved when the role is read or written.
plugin/path_role.go
Outdated
// role reads a gcpRole from storage. This assumes the caller has already obtained the role lock. | ||
func (b *GcpAuthBackend) role(ctx context.Context, s logical.Storage, name string) (*gcpRole, error) { | ||
// retrieveRole from storage. This assumes the caller has already obtained the role lock. | ||
func (b *GcpAuthBackend) retrieveRole(ctx context.Context, s logical.Storage, name string) (*gcpRole, error) { |
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.
Can you share the rational for renaming this method?
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.
Generally I ascribe to the idea that function names should be read like verbs.
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 don't necessarily disagree, however this breaks a common naming convention in most of our secret backends. I think Role
is pretty Go idiomatic as well:
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 agree with the naming convention for getters, however I don't agree that this is a getter as described. This is retrieving the role from the storage backend, not returning an existing, unexported field on the GcpAuthBackend
.
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.
That's fair, it's not a true getter as described in that link. I do feel that role
is consistent with most of our other secret backends as well as some other methods/functions in this plugin (Factory()
, Backend()
, IAMClient()
,ComputeClient()
)
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.
Changed back to role
for consistency across plugins that I wasn't aware of previously.
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 working on this @pcman312 , it looks great! I also black-box tested it in the command line, and it's working as expected. Just a couple of discussion questions around whether we should allow no selection, and how we should handle the performance impact that those selecting role_id
might see.
allowedGCEAliasesSlice = gceMapKeyToSlice(allowedGCEAliases) | ||
) | ||
|
||
func iamMapKeyToSlice(m map[string]iamAliaser) (s []string) { |
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.
These could probably be reduced into a single function that accepts an interface and is type switched. Same with many of the get..
functions below.
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.
Unfortunately it wouldn't really help here. The function would have the same code in it (including both versions of the for loop), but wrapped up into a single function rather than two. There isn't a generic map
type that can be used as a variable type that would allow the type coercion needed here. Additionally it's generally frowned upon to pass the empty interface around unless necessary.
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 have generally found the same thing as well - when I have tried that approach it read awkwardly, but just in a different way. I don't think there's any "perfect" answer here, but I do like the more direct approach taken here because type conversions can be expensive.
Thank you for your review, by the way, broamski!
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.
LGTM! Thanks for doing this while on the Sustaining team, I know you must be busy!
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.
Looks good. Is there a chance of a release including this being vendored into the 1.4.1 release of Vault?
@dustin-decker This change and another one to provide some customization of metadata storage (PR very soon) are targeted for 1.4.1. |
Resolves hashicorp/vault#8761 Allows users to specify an alias type field for IAM and GCE logins which will then switch between the current behavior (a unique ID for IAM, and the instance ID for GCE) and a newly created role_id field. The role_id is a UUID generated when the role is created. All existing roles without a role_id will have one generated and saved when the role is read or written.
Resolves hashicorp/vault#8761 Allows users to specify an alias type field for IAM and GCE logins which will then switch between the current behavior (a unique ID for IAM, and the instance ID for GCE) and a newly created role_id field. The role_id is a UUID generated when the role is created. All existing roles without a role_id will have one generated and saved when the role is read or written.
Resolves hashicorp/vault#8761 Allows users to specify an alias type field for IAM and GCE logins which will then switch between the current behavior (a unique ID for IAM, and the instance ID for GCE) and a newly created role_id field. The role_id is a UUID generated when the role is created. All existing roles without a role_id will have one generated and saved when the role is read or written.
Overview
Allows users to specify an alias type field for IAM and GCE logins which will then switch between the current behavior (a unique ID for IAM, and the instance ID for GCE) and a newly created
role_id
field. Therole_id
is a UUID generated when the role is created. All existing roles without a role_id will have one generated and saved when the role is read or written.Design of Change
Adds 3 fields to the role config:
role_id
(generated)iam_alias
-"unique_id"
(default) or"role_id"
gce_alias
-"instance_id"
(default) or"role_id"
If the alias field is specified as
"role_id"
, the Vault alias will be the generatedrole_id
rather than the current behavior of a unique ID (for IAM) or the instance ID (for GCE).I've also utilized go's mocking library to mock the
logical.SystemView
andlogical.Storage
interfaces to improve testing coverage of some new tests. To generate the mocks, runmake mocks
.The
gcpRole
struct and its associated functions have been pulled out into its own file:gcp_role.go
since thepath_role.go
file was getting very large (1000+ lines).Related Issues/Pull Requests
Test Output
Contributor Checklist