-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Authenticator Groups #217
Conversation
} | ||
|
||
variable "authenticator_groups_config" { | ||
type = list(map(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.
Instead of embedding the map, could we directly accept a list of security groups?
type = list(map(string)) | |
type = list(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.
I've changed it to list(object({ security_group = string }))
which is a more accurate depiction of the usage and made it clearer in the docs on the object format (similar to
The initial reasoning in using a map/object was being consistent with how other variables are done when using dynamic blocks e.g. pod security. However on second thought it should be simple to make these variables a string/bool to the user and internally have a local variable/conditonal that converts it into a list (the list format being required for dynamic blocks)
I take a crack at doing the latter in this PR if that sounds reasonable @morgante
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.
In this case, I'd rather go straight to list(string)
so we don't introduce an unnecessary breaking change in the future. Updating the others can be a separate PR though.
Does that work?
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.
Moving to a single item string list works, but you still get the awkward API for users by forcing a list even though its going to be a single item at max. It also assumes the underlying API structure won't change. Although if that ever does change then i guess a breaking change in this module is fair game.
These are the options we can go with, personally my preference as a end user would be string
over any list()
. If we go a list()
based type then i don't think it makes sense to move to string
in the future since that would be a breaking change.
# list(object) attempting to mimic the actual provider/gke api structure
authenticator_groups_config = [{
security_group = "group@yourdomain.com"
}]
workload_identity_config = [{
identity_namespace = "{your-project-id}.svc.id.goog"
}]
# Moving to list(string)
authenticator_groups_config = ["group@yourdomain.com"]
workload_identity_namespace = ["your-project-id}.svc.id.goog"]
# Simplified, convert to list inside the module using conditional. Default is ""
authenticator_groups_config = "group@yourdomain.com"
workload_identity_namespace = "{your-project-id}.svc.id.goog"
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, I'm in favor of moving straight to a 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.
PR updated to just include these changes for authenticator groups since Workload Identity has already been merged to master, ready for review @morgante
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! 1 small question/nit.
Also needs conflicts resolved.
Add Authenticator Groups
Follow up PR after #216 gets merged, will rebase then.
Related PR: #196