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 Authenticator Groups #217

Merged
merged 3 commits into from
Aug 22, 2019
Merged

Conversation

Dev25
Copy link
Contributor

@Dev25 Dev25 commented Jul 25, 2019

Follow up PR after #216 gets merged, will rebase then.

Related PR: #196

autogen/main.tf Outdated Show resolved Hide resolved
}

variable "authenticator_groups_config" {
type = list(map(string))
Copy link
Contributor

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?

Suggested change
type = list(map(string))
type = list(string)

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'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

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morgante

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"

Copy link
Contributor

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.

Copy link
Contributor Author

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

@aaron-lane aaron-lane added the enhancement New feature or request label Aug 16, 2019
@Dev25 Dev25 changed the title Add Workload Identity and Authenticator Groups Add Authenticator Groups Aug 20, 2019
Copy link
Contributor

@morgante morgante left a 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.

autogen/variables.tf Outdated Show resolved Hide resolved
@morgante morgante merged commit 5c17b2e into terraform-google-modules:master Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants