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

Adding private_cluster #1250

Merged
merged 5 commits into from
Mar 30, 2018
Merged

Adding private_cluster #1250

merged 5 commits into from
Mar 30, 2018

Conversation

lenartj
Copy link
Contributor

@lenartj lenartj commented Mar 24, 2018

This is to implement #1174, support for Private Clusters on Google Kubernetes Engine (Container Engine; GKE).

I am not familiar with the codebase, please be patient :-)

@danawillow, can you look at this please?

@danawillow danawillow self-requested a review March 29, 2018 23:24
@danawillow danawillow self-assigned this Mar 29, 2018
Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Thanks @lenartj, this looks great!

I ran the tests and they failed because of the interpolation typo I commented on. Fixing that led to a different bug I found. If you wouldn't mind just adding this:

	if len(c.CidrBlocks) == 0 {
		return nil
	}

to the beginning of flattenMasterAuthorizedNetworksConfig that'll fix it.

Also, feel free to run the tests yourself- there are some instructions at https://github.com/terraform-providers/terraform-provider-google/blob/master/.github/CONTRIBUTING.md#tests. Let me know if you have any questions!

Default: false,
},

"master_ipv4_cidr": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's actually call this master_ipv4_cider_block to be consistent with the cidr_blocks attribute from master_authorized_networks_config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, cider sounds good :-)

private_cluster = true
master_ipv4_cidr = "10.42.0.0/28"
ip_allocation_policy {
cluster_secondary_range_name = "${google_compute_subnetwork.container_network.secondary_ip_range.0.range_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

think you meant container_subnetwork :)
(and the line below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

network = "${google_compute_network.container_network.name}"
ip_cidr_range = "10.0.36.0/24"
region = "us-central1"

Copy link
Contributor

Choose a reason for hiding this comment

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

I ran the tests, and it looks like private_ip_google_access is getting enabled automatically on the subnetwork when the cluster is created in it. Mind just setting that to true from the beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

ResourceName: "google_container_cluster.with_private_cluster",
ImportStateIdPrefix: "us-central1-a/",
ImportState: true,
ImportStateVerify: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

The import tests won't actually work for this, because we don't have import support for beta attributes yet. You can just set ImportStateVerifyIgnore for the new fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@lenartj
Copy link
Contributor Author

lenartj commented Mar 30, 2018

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withPrivateCluster'                           
==> Checking that code complies with gofmt requirements...                                                                                                                                                              
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_withPrivateCluster -timeout 120m                                                                                                                      
=== RUN   TestAccContainerCluster_withPrivateCluster                                                                                           
=== PAUSE TestAccContainerCluster_withPrivateCluster                                                                                                                         
=== CONT  TestAccContainerCluster_withPrivateCluster                                                                                                                            
--- PASS: TestAccContainerCluster_withPrivateCluster (423.94s)
PASS                                                                          
ok      github.com/terraform-providers/terraform-provider-google/google 423.951s

@lenartj
Copy link
Contributor Author

lenartj commented Mar 30, 2018

Thanks for the comments @danawillow, fixed it up. Apologies for not running the tests, I had a really bad internet connection at sea ⛴ when I wrote this

@danawillow
Copy link
Contributor

Thanks @lenartj, looks good! And lol good catch about the cider thing, thanks for not taking my suggestion too literally :)

@danawillow danawillow merged commit 1840363 into hashicorp:master Mar 30, 2018
@lenartj
Copy link
Contributor Author

lenartj commented Mar 30, 2018

Resolved #1174

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
* Updated google.golang.org/api/container/v1beta1

* Added support for private_cluster and master_ipv4_cidr

This is to implement hashicorp#1174. See
https://groups.google.com/forum/#!topic/google-cloud-sdk-announce/GGW3SQSANIc

* Added simple test for private_cluster and master_ipv4_cidr

* Review replies

* Added some documentation for private_cluster
@ghost
Copy link

ghost commented Nov 19, 2018

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants