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

Enabling two features in beta clusters #188

Conversation

chrislovecnm
Copy link
Contributor

@chrislovecnm chrislovecnm commented Jun 22, 2019

This allows pod security policies and binary authorization to be used
by both beta private and beta public clusters. Previously these two features
where limited to only private clusters, and this commit also removes
that functionality from private clusters.

Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

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

@chrislovecnm I believe the reason why those features were only added to the private cluster was because they require the beta provider. Fortunately, the template logic has been updated to account for beta clusters!

README.md Outdated
@@ -109,75 +109,6 @@ Version 1.0.0 of this module introduces a breaking change: adding the `disable-l
In either case, upgrading to module version `v1.0.0` will trigger a recreation of all node pools in the cluster.

[^]: (autogen_docs_start)

## Inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Please run make generate_docs to regenerate the tables after make generate regenerates the modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

autogen/cluster_regional.tf Show resolved Hide resolved
autogen/cluster_zonal.tf Show resolved Hide resolved
@chrislovecnm
Copy link
Contributor Author

I will update. Thanks for the review

@chrislovecnm chrislovecnm force-pushed the enable-features-in-public-clusters branch from 4e4119f to 3b23985 Compare June 25, 2019 20:03
@chrislovecnm chrislovecnm changed the title Enabling two features in public clusters Enabling two features in beta clusters Jun 25, 2019
@chrislovecnm
Copy link
Contributor Author

@aaron-lane @morgante PTAL - I updated the PR title and body to reflect the changes.

morgante
morgante previously approved these changes Jun 25, 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.

Please run make generate_docs, otherwise LGTM.

@chrislovecnm
Copy link
Contributor Author

#191 needs to be merged. I forgot that the README.md lives in the autogen directory as well.

@chrislovecnm chrislovecnm force-pushed the enable-features-in-public-clusters branch from c6c56ad to 13bf6dd Compare June 25, 2019 23:53
@chrislovecnm
Copy link
Contributor Author

chrislovecnm commented Jun 25, 2019

Updated with generate_docs.

@morgante / @aaron-lane I added 13bf6dd which can be dropped before merging. This is the same as changes as #191. This should allow this PR to pass CI now.

@chrislovecnm
Copy link
Contributor Author

 * module.example.module.gke.local.cluster_type_output_pod_security_policy_enabled: local.cluster_type_output_pod_security_policy_enabled: Resource 'google_container_cluster.zonal_primary' does not have attribute 'pod_security_policy_config.0.enabled' for variable 'google_container_cluster.zonal_primary.*.pod_security_policy_config.0.enabled'

Not a guru in terraform, so I am not quite sure where this is coming from. pod_security_policy_config is in a beta module, so it should not even impact this cluster. Looking into this

@chrislovecnm chrislovecnm force-pushed the enable-features-in-public-clusters branch 2 times, most recently from c6c77db to 70a4775 Compare June 26, 2019 18:40
@chrislovecnm chrislovecnm changed the title Enabling two features in beta clusters [WIP] Enabling two features in beta clusters Jun 26, 2019
@chrislovecnm
Copy link
Contributor Author

Please do not merge, I am working on what I think are some bugs.

@chrislovecnm
Copy link
Contributor Author

@morgante @aaron-lane the latest bug I am trying to figure out is:

* module.gke.local.cluster_type_output_binary_authorization_enabled: local.cluster_type_output_binary_authorization_enabled: Resource 'google_container_cluster.primary' does not have attribute 'enable_binary_authorization.0.enabled' for variable 'google_container_cluster.primary.*.enable_binary_authorization.0.enabled'

I am confused a bit as I have defined enable_binary_authorization in the main.tf file

  cluster_type_output_binary_authorization_enabled = {
    regional = "${element(concat(google_container_cluster.primary.*.enable_binary_authorization.0.enabled, list("")), 0)}"
    zonal    = "${element(concat(google_container_cluster.zonal_primary.*.enable_binary_authorization.0.enabled, list("")), 0)}"
  }

and then

  cluster_binary_authorization_enabled = "${local.cluster_type_output_binary_authorization_enabled[local.cluster_type] ? true : false}"

@chrislovecnm
Copy link
Contributor Author

So I cannot get the output to work, but really I do not care at this point. If we want to add the output for binary authorization later we can.

@chrislovecnm
Copy link
Contributor Author

@morgante @aaron-lane we are missing a bunch of other beta features. Do you want another PR or just to update this one? For example enable_tpu, vertical_pod_autoscaling, etc.

This allow pod security policies and binary authorization to be used
by both beta private and  beta public clusters. Previously these two features
where limited to only private clusters, and this commit also removes
that functionality from private clusters.
@chrislovecnm chrislovecnm force-pushed the enable-features-in-public-clusters branch from e6566ec to 48bfedf Compare June 27, 2019 15:57
@chrislovecnm chrislovecnm changed the title [WIP] Enabling two features in beta clusters Enabling two features in beta clusters Jun 27, 2019
@aaron-lane
Copy link
Contributor

@chrislovecnm let's handle the additional features in different pull requests to minimize the amount of content to review per branch.

@morgante
Copy link
Contributor

So I cannot get the output to work, but really I do not care at this point. If we want to add the output for binary authorization later we can.

Yeah, that's fine. Go ahead and drop it.

@itlinux
Copy link

itlinux commented Jun 27, 2019 via email

@chrislovecnm
Copy link
Contributor Author

@chrislovecnm
Copy link
Contributor Author

@morgante @aaron-lane PTAL ... CI is passing

@morgante
Copy link
Contributor

@chrislovecnm Thanks for the contribution.

@morgante morgante requested a review from aaron-lane June 27, 2019 16:39
@morgante morgante merged commit 76bb697 into terraform-google-modules:master Jun 27, 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

4 participants