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 Private cluster and master ipv4 configuration #13

Conversation

pratikmallya
Copy link
Contributor

@pratikmallya pratikmallya commented Sep 24, 2018

Fixes #8

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 for this, just a few notes.

main.tf Show resolved Hide resolved
variables.tf Outdated
@@ -170,3 +170,13 @@ variable "monitoring_service" {
description = "The monitoring service that the cluster should write metrics to. Automatically send metrics from pods in the cluster to the Google Cloud Monitoring API. VM metrics will be collected by Google Compute Engine regardless of this setting Available options include monitoring.googleapis.com, monitoring.googleapis.com/kubernetes (beta) and none"
default = "monitoring.googleapis.com"
}

variable "private_cluster" {
description = "(Optional, Beta) If true, a private cluster will be created, meaning nodes do not get public IP addresses. It is mandatory to specify master_ipv4_cidr_block and ip_allocation_policy with this option."
Copy link
Contributor

Choose a reason for hiding this comment

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

We can leave off the note about ip_allocation_policy since this that field isn't exposed to module users.

@pratikmallya pratikmallya force-pushed the add_more_params branch 2 times, most recently from 917937c to 23579fe Compare September 26, 2018 21:15
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.

This LGTM, will merge once tests pass.

@qvallance
Copy link

Before merging this, I would recommend refactoring to utilize the new private_cluster_config config block. It depricates the standalone master_ipv4_cidr_block and private_cluster fields and adds new private_cluster_config.master_ipv4_cidr_block, private_cluster_config.enable_private_nodes, and private_cluster_config.enable_private_endpoint fields instead.

@Jberlinsky
Copy link
Contributor

@pratikmallya, thank you very much for your contribution!

Unfortunately, we can't merge this until the integration tests are passing. When I checked out your branch and tried to run it on my local machine, I get numerous errors. Could you please get the output of make test_integration to pass, so we can merge this in?

@morgante
Copy link
Contributor

@qvallance Thanks for the suggestion.

@Jberlinsky Could you file a new PR which takes care of adding this functionality using the new private_cluster_config field? (And making sure tests pass.)

@Jberlinsky
Copy link
Contributor

@morgante Sure, I'd be happy to!

@Jberlinsky
Copy link
Contributor

@morgante Just to confirm, making this change would require moving from the google provider to google-beta. This would force consumers to update their code, as we don't specify a provider explicitly in the module code itself. Is this the direction we wish to move in?

@morgante
Copy link
Contributor

morgante commented Oct 10, 2018

@Jberlinsky I believe we're likely already using some beta fields, we're going to have to address that overall anyways so for now you can ignore the provider split.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants