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.

Sign up for GitHub

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

Jump to bottom

Add support for setting gke intranode visibility #747

Merged
merged 1 commit into from
Jun 4, 2019
Merged

Add support for setting gke intranode visibility #747

merged 1 commit into from
Jun 4, 2019

Conversation

luis-silva
Copy link
Contributor

This PR adds the option to set GKE intranode visibility.
Intranode Visibility is a beta option available on GKE clusters versions >= 1.11 and set to false by default.

Both tests report a pass and I also confirmed the behaviour on the GCP console while the tests were running as well as via the GCP Audit Logs.

$ make testacc TEST=./google-beta TESTARGS='-run=TestAccContainerCluster_withDefaultIntraNodeVisibility'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccContainerCluster_withDefaultIntraNodeVisibility -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccContainerCluster_withDefaultIntraNodeVisibility
=== PAUSE TestAccContainerCluster_withDefaultIntraNodeVisibility
=== CONT  TestAccContainerCluster_withDefaultIntraNodeVisibility
--- PASS: TestAccContainerCluster_withDefaultIntraNodeVisibility (525.36s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google-beta/google-beta	525.390s

$ make testacc TEST=./google-beta TESTARGS='-run=TestAccContainerCluster_withIntraNodeVisibility'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccContainerCluster_withIntraNodeVisibility -timeout 240m -ldflags="-X=github.com/terraform-providers/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccContainerCluster_withIntraNodeVisibility
=== PAUSE TestAccContainerCluster_withIntraNodeVisibility
=== CONT  TestAccContainerCluster_withIntraNodeVisibility
--- PASS: TestAccContainerCluster_withIntraNodeVisibility (868.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google-beta/google-beta	868.542s

@ghost ghost added the size/m label May 21, 2019
@luis-silva
Copy link
Contributor Author

@chrisst Can you take a look at this PR and merge it if you're happy with it?

@chrisst chrisst self-assigned this Jun 3, 2019
@chrisst chrisst mentioned this pull request Jun 3, 2019
Merged
chrisst
chrisst approved these changes Jun 3, 2019
View reviewed changes
Copy link

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

I have a couple quick comments, but overall looks good! No need to address the comments here as I've had to copy this PR into our upstream code generator at GoogleCloudPlatform/magic-modules#1871, so I applied the comments there already.
Once the upstream PR is ready I'll merge this followed by the upstream which will iron out the differences between the two PRs.

google-beta/resource_container_cluster.go
@@ -738,6 +744,10 @@ func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) er
Autoscaling: expandClusterAutoscaling(d.Get("cluster_autoscaling"), d),
MasterAuth: expandMasterAuth(d.Get("master_auth")),
ResourceLabels: expandStringMap(d, "resource_labels"),
NetworkConfig: &containerBeta.NetworkConfig{
EnableIntraNodeVisibility: d.Get("enable_intranode_visibility").(bool),
ForceSendFields: []string{"Enabled"},
Copy link

Choose a reason for hiding this comment

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

I don't think this ForceSendFields is working as intended. There is no Enabled in the EnableIntraNodeVisibility object so it is ignored. However since this is in Create and false is the default there is no need to force send a falsey value.

google-beta/resource_container_cluster_test.go
Since GKE disables Intra Node Visibility by default, this test will ensure that Intra Node Visibility is disabled by default to be
more consistent with default settings in the Cloud Console
*/
func TestAccContainerCluster_withDefaultIntraNodeVisibility(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

Since this field is marked as Default: false I don't think we need to assert that the default is actually sent to GCP. This is more of a test of Terraform than the provider implementation here. Since GKE tests are somewhat expensive I'll be taking this out.

@chrisst chrisst merged commit 50691ea into hashicorp:master Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisst chrisst chrisst approved these changes

Assignees

@chrisst chrisst

Labels
size/m
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luis-silva @chrisst