Add support for master authorized networks in `google_container_cluster` #626

davidquarles · 2017-10-24T11:09:29Z

Fixes #619.

Update container/v1 API (handled independently in Update container/v1 API #624)
Add support for master_authorized_networks in google_container_cluster

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

davidquarles · 2017-10-24T11:39:55Z

google/resource_container_cluster.go

+					Schema: map[string]*schema.Schema{
+						"enabled": {
+							Type:     schema.TypeBool,
+							Required: true,


I wasn't entirely sure what made sense here for Optional / Computed / Required / Default. In the end, Required: true made the most sense to me, but only marginally so. I'll test upstream for implicit requirements/defaults tomorrow.

I'd actually recommend not having an enabled field at all, and instead just relying on the presence of any master_authorized_networks_configs to determine what that value should be, similar to how we do autoscaling. The reason for this is that when enabled is false, GCP won't return anything for cidrBlocks:

"masterAuthorizedNetworksConfig": {},

This means that a config like this:

master_authorized_networks_config { enabled = false cidr_blocks = ["172.16.0.0/12", "10.0.0.0/8"] }

is equivalent to:

master_authorized_networks_config { enabled = false }

Because we want to set the state based on what's actually true in GCP, the state will always resemble the second config above, and thus planning immediately after applying with the first config will show a diff.

"But wait!" you might ask, "I added a test for the enabled=false case and it passed!" Ah yes, and this tripped me up for a while too. I'll explain in a comment below.

Totally, I almost did that, but wasn't sure and opted for mimicking the API. I'll follow that pattern elsewhere.

davidquarles · 2017-10-31T04:17:05Z

the two failures below happen on master as well. I'm guessing this is a known issue?

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster -timeout 120m
=== RUN   TestAccContainerCluster_import
=== RUN   TestAccContainerCluster_basic
=== RUN   TestAccContainerCluster_withTimeout
=== RUN   TestAccContainerCluster_withAddons
=== RUN   TestAccContainerCluster_withMasterAuth
=== RUN   TestAccContainerCluster_withMasterAuthorizedNetworksConfig
=== RUN   TestAccContainerCluster_withAdditionalZones
=== RUN   TestAccContainerCluster_withLegacyAbac
=== RUN   TestAccContainerCluster_withVersion
=== RUN   TestAccContainerCluster_updateVersion
=== RUN   TestAccContainerCluster_withNodeConfig
=== RUN   TestAccContainerCluster_withNodeConfigScopeAlias
=== RUN   TestAccContainerCluster_network
=== RUN   TestAccContainerCluster_backend
=== RUN   TestAccContainerCluster_withLogging
=== RUN   TestAccContainerCluster_withMonitoring
=== RUN   TestAccContainerCluster_withNodePoolBasic
=== RUN   TestAccContainerCluster_withNodePoolResize
=== RUN   TestAccContainerCluster_withNodePoolAutoscaling
=== RUN   TestAccContainerCluster_withNodePoolNamePrefix
=== RUN   TestAccContainerCluster_withNodePoolMultiple
=== RUN   TestAccContainerCluster_withNodePoolConflictingNameFields
=== RUN   TestAccContainerCluster_withNodePoolNodeConfig
--- PASS: TestAccContainerCluster_withVersion (358.25s)
--- PASS: TestAccContainerCluster_withNodePoolNodeConfig (406.36s)
--- PASS: TestAccContainerCluster_withNodeConfigScopeAlias (408.32s)
--- PASS: TestAccContainerCluster_import (478.31s)
--- PASS: TestAccContainerCluster_withAdditionalZones (483.29s)
--- PASS: TestAccContainerCluster_withLegacyAbac (625.03s)
--- FAIL: TestAccContainerCluster_network (714.98s)
	testing.go:494: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.

		Error: Error applying: 1 error(s) occurred:

		* google_compute_network.container_network (destroy): 1 error(s) occurred:

		* google_compute_network.container_network: Error waiting for Deleting Network: timeout while waiting for state to become 'DONE' (last state: 'RUNNING', timeout: 4m0s)

		State: google_compute_network.container_network:
		  ID = container-net-izw2yl82rx
		  auto_create_subnetworks = true
		  gateway_ipv4 =
		  name = container-net-izw2yl82rx
		  self_link = https://www.googleapis.com/compute/v1/projects/nordstrom-k8s/global/networks/container-net-izw2yl82rx
--- PASS: TestAccContainerCluster_withNodePoolConflictingNameFields (559.19s)
--- PASS: TestAccContainerCluster_updateVersion (933.94s)
--- PASS: TestAccContainerCluster_withNodePoolNamePrefix (526.78s)
--- FAIL: TestAccContainerCluster_withNodePoolMultiple (529.43s)
	testing.go:434: Step 0 error: Check failed: Check 1/1 error: Cluster has mismatched node_pool.1.node_config.0.oauth_scopes[0].
		TF State:
		GCP State: https://www.googleapis.com/auth/logging.write
--- PASS: TestAccContainerCluster_withNodeConfig (345.72s)
--- PASS: TestAccContainerCluster_withNodePoolBasic (417.79s)
--- PASS: TestAccContainerCluster_withNodePoolResize (663.22s)
--- PASS: TestAccContainerCluster_withNodePoolAutoscaling (834.72s)
--- PASS: TestAccContainerCluster_basic (356.24s)
--- PASS: TestAccContainerCluster_backend (392.66s)
--- PASS: TestAccContainerCluster_withTimeout (395.95s)
--- PASS: TestAccContainerCluster_withMasterAuth (406.47s)
--- PASS: TestAccContainerCluster_withLogging (615.11s)
--- PASS: TestAccContainerCluster_withMonitoring (645.08s)
--- PASS: TestAccContainerCluster_withMasterAuthorizedNetworksConfig (418.33s)
--- PASS: TestAccContainerCluster_withAddons (584.49s)
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-google/google	1897.568s
make: *** [testacc] Error 1

danawillow · 2017-10-31T16:17:01Z

google/resource_container_cluster.go

+				Optional:     true,
+				Computed:     true,
+				ForceNew:     true,
+				ValidateFunc: validation.CIDRNetwork(11, 19),


Where did the numbers 11 and 19 come from? (I want to make sure we don't break any existing configs with this change)

Good call – I was extrapolating API-level constraints based on my experience with the console/UI, which requires a mask in that range, but now that I'm hitting the API directly, you're right -- it doesn't seem to be a thing! RFC1918 is a strict requirement here, though, and I added logic specifically to validate this attribute against those ranges, then lost it on rebase. I'll add it back + remove this constraint.

danawillow · 2017-10-31T16:18:15Z

google/resource_container_cluster.go

+							Optional: true,
+							Computed: true,
+							MaxItems: 10,
+							Elem: &schema.Schema{


Do you think anyone would want to set the displayName field inside of cidr block?

For sure, I'll add that now.

danawillow · 2017-10-31T20:16:21Z

google/resource_container_cluster.go

+					Schema: map[string]*schema.Schema{
+						"enabled": {
+							Type:     schema.TypeBool,
+							Required: true,


I'd actually recommend not having an enabled field at all, and instead just relying on the presence of any master_authorized_networks_configs to determine what that value should be, similar to how we do autoscaling. The reason for this is that when enabled is false, GCP won't return anything for cidrBlocks:

"masterAuthorizedNetworksConfig": {},

This means that a config like this:

master_authorized_networks_config { enabled = false cidr_blocks = ["172.16.0.0/12", "10.0.0.0/8"] }

is equivalent to:

master_authorized_networks_config { enabled = false }

Because we want to set the state based on what's actually true in GCP, the state will always resemble the second config above, and thus planning immediately after applying with the first config will show a diff.

"But wait!" you might ask, "I added a test for the enabled=false case and it passed!" Ah yes, and this tripped me up for a while too. I'll explain in a comment below.

danawillow · 2017-10-31T20:19:31Z

google/resource_container_cluster.go

+	result := []map[string]interface{}{
+		map[string]interface{}{
+			"enabled":     c.Enabled,
+			"cidr_blocks": cidrBlocks,


Turns out, if flattenMasterAuthorizedNetworksConfig returns something that looks like this:

result: [map[enabled:false cidr_blocks:[]]]

d.Set will actually return an error: Invalid address to set: []string{"master_authorized_networks_config", "0", "cidr_blocks"} This is fairly cryptic, but it basically just means that you can't set an empty slice in config, you have to just set no slice at all.

Hrmmmmm, alright. I'll add that as a test case, too.

danawillow · 2017-10-31T20:20:58Z

google/resource_container_cluster.go

@@ -514,6 +536,30 @@ func resourceContainerClusterUpdate(d *schema.ResourceData, meta interface{}) er

 	d.Partial(true)

+	if d.HasChange("master_authorized_networks_config") {
+		if c, ok := d.GetOk("master_authorized_networks_config"); ok {


This means that if a user removes the master_authorized_networks_config block from their config, nothing actually changes. Is that intentional? I'd assume it would remove the configuration.

That was definitely not intentional, and is now fixed.

davidquarles · 2017-11-01T09:35:48Z

@danawillow I worked through your comments, and left this second round of changes is in a separate commit for now. I can squash/rebase once everything's ready to merge.

Do you think it makes sense to collapse the schema a bit, as below? I've basically translated the upstream structure, and the result feels a bit verbose to me. I know how painful backwards compatibility issues can be, though. I'll defer to you.

resource "google_container_cluster" "foo" {
    master_authorized_network {
        cidr_block = "1.2.3.4/32"
    }
    master_authorized_network {
        cidr_block = "5.6.7.8/32"
    }
}

Test output:

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

danawillow · 2017-11-01T22:57:34Z

google/validation.go

@@ -31,3 +39,25 @@ func validateRegexp(re string) schema.SchemaValidateFunc {
 		return
 	}
 }
+
+func validateRFC1918Network(min, max int) schema.SchemaValidateFunc {


This validation has enough logic in it that I wouldn't mind seeing a unit test.

danawillow · 2017-11-01T23:11:41Z

google/resource_container_cluster_test.go

+
+	master_authorized_networks_config {
+		%s
+    }


nit: indentation

danawillow · 2017-11-01T23:11:56Z

google/resource_container_cluster_test.go

+				Config: testAccContainerCluster_withMasterAuthorizedNetworksConfig(clusterName, []string{"8.8.8.8/32"}),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckContainerCluster("google_container_cluster.with_master_authorized_networks"),
+					resource.TestCheckNoResourceAttr("google_container_cluster.with_master_authorized_networks",


I think you want to check that the block is there though, right?

Right. Fixed.

danawillow · 2017-11-01T23:12:53Z

google/resource_container_cluster.go

+				"display_name": v.DisplayName,
+			})
+		}
+		// now does this adhere to the schema?


I'm not quite sure what this comment is for

Oh, that definitely wasn't supposed to make it in. 😺

danawillow · 2017-11-01T23:14:37Z

I'm fine with the schema as-is.

Also feel free to just always do separate commits- I default to "squash and merge" for the final merge :)

- remove `google_container_cluster.master_authorized_networks_config.enabled` - add `display_name` and restructure schema as follows: master_authorized_networks_config { cidr_blocks { cidr_block = "0.0.0.0/0" display_name = "foo" } } - amend tests

danawillow · 2017-11-02T17:36:53Z

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

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

Thanks @davidquarles! Looks good!

…er` (hashicorp#626) * Add support for master authorized networks in `google_container_cluster` * [review] remove enabled flag / restructure schema - remove `google_container_cluster.master_authorized_networks_config.enabled` - add `display_name` and restructure schema as follows: master_authorized_networks_config { cidr_blocks { cidr_block = "0.0.0.0/0" display_name = "foo" } } - amend tests * [review] add test for validateRFC1918Network, fix acc test

Signed-off-by: Modular Magician <[email protected]>

ghost · 2020-03-30T14:22:29Z

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 👉 [email protected]. Thanks!

davidquarles force-pushed the master-authorized-networks branch from b09279c to f299bbf Compare October 24, 2017 11:13

davidquarles commented Oct 24, 2017

View reviewed changes

danawillow self-requested a review October 26, 2017 22:12

davidquarles force-pushed the master-authorized-networks branch 2 times, most recently from 17745c1 to 49c51bc Compare October 31, 2017 00:05

danawillow suggested changes Oct 31, 2017

View reviewed changes

davidquarles force-pushed the master-authorized-networks branch 2 times, most recently from af411e0 to b362224 Compare November 1, 2017 09:03

danawillow reviewed Nov 1, 2017

View reviewed changes

davidquarles added 3 commits November 1, 2017 16:58

Add support for master authorized networks in google_container_cluster

382be19

[review] add test for validateRFC1918Network, fix acc test

edd19a7

davidquarles force-pushed the master-authorized-networks branch from b362224 to edd19a7 Compare November 2, 2017 08:43

danawillow approved these changes Nov 2, 2017

View reviewed changes

danawillow merged commit ca7551c into hashicorp:master Nov 2, 2017

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Add pagination to google projects data source (hashicorp#626)

bf41c48

Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for master authorized networks in `google_container_cluster` #626

Add support for master authorized networks in `google_container_cluster` #626

davidquarles commented Oct 24, 2017 •

edited

Loading

davidquarles Oct 24, 2017

danawillow Oct 31, 2017

davidquarles Nov 1, 2017

davidquarles commented Oct 31, 2017

danawillow Oct 31, 2017

davidquarles Nov 1, 2017

davidquarles Nov 1, 2017

danawillow Oct 31, 2017

davidquarles Nov 1, 2017

davidquarles Nov 1, 2017

danawillow Oct 31, 2017

danawillow Oct 31, 2017

davidquarles Nov 1, 2017

davidquarles Nov 1, 2017

danawillow Oct 31, 2017

davidquarles Nov 1, 2017

davidquarles commented Nov 1, 2017

danawillow Nov 1, 2017

davidquarles Nov 2, 2017

danawillow Nov 1, 2017

danawillow Nov 1, 2017

davidquarles Nov 2, 2017

danawillow Nov 1, 2017

davidquarles Nov 1, 2017

danawillow commented Nov 1, 2017

danawillow commented Nov 2, 2017

ghost commented Mar 30, 2020

Add support for master authorized networks in google_container_cluster #626

Add support for master authorized networks in google_container_cluster #626

Conversation

davidquarles commented Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidquarles commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidquarles commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow commented Nov 1, 2017

danawillow commented Nov 2, 2017

ghost commented Mar 30, 2020

Add support for master authorized networks in `google_container_cluster` #626

Add support for master authorized networks in `google_container_cluster` #626

davidquarles commented Oct 24, 2017 •

edited

Loading