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 support for GKE Autopilot in google_container_cluster resource #8632

Merged
merged 5 commits into from
Mar 30, 2021

Conversation

paulwilljones
Copy link
Contributor

@paulwilljones paulwilljones commented Mar 9, 2021

Adds support for GKE Autopilot to google_container_cluster resource.

Due to the defaults set in GKE Autopilot, some features of GKE aren't available when in Autopilot mode.

@ghost ghost added the size/m label Mar 9, 2021
@ghost ghost requested a review from ScottSuarez March 9, 2021 18:13
@ghost ghost added size/l and removed size/m labels Mar 9, 2021
@ghost ghost added size/m size/l and removed size/l size/m labels Mar 9, 2021
@ScottSuarez ScottSuarez requested a review from c2thorn March 9, 2021 20:22
Copy link
Collaborator

@c2thorn c2thorn left a comment

Choose a reason for hiding this comment

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

@paulwilljones Thanks for starting this process. With the amount of differences between autopilot and standard GKE, this is not a straightforward change. We want to be cautious and ensure that standard GKE UX is not disrupted.

In addition to the comments already suggested, I would request more extensive testing:

google/resource_container_cluster.go Outdated Show resolved Hide resolved
google/resource_container_cluster.go Outdated Show resolved Hide resolved
google/resource_container_cluster.go Show resolved Hide resolved
@paulwilljones
Copy link
Contributor Author

@paulwilljones Thanks for starting this process. With the amount of differences between autopilot and standard GKE, this is not a straightforward change. We want to be cautious and ensure that standard GKE UX is not disrupted.

In addition to the comments already suggested, I would request more extensive testing:

Thanks both. I'll take a look at those changes.
One thing I've noticed is certain features will need enabling when enable_autopilot: true, such as shielded nodes in order for the API to accept the request:

Error: googleapi: Error 400: Autopilot clusters requires shielded nodes to be enabled., badRequest                                            

@c2thorn
Copy link
Collaborator

c2thorn commented Mar 10, 2021

One thing I've noticed is certain features will need enabling when enable_autopilot: true, such as shielded nodes in order for the API to accept the request:

Error: googleapi: Error 400: Autopilot clusters requires shielded nodes to be enabled., badRequest                                            

Omitting shielded nodes from the request altogether works correct?

@ghost ghost added size/l and removed size/m labels Mar 11, 2021
@glerchundi
Copy link

This is absolutely fantastic! Cannot wait to see this merged and ready to be consumed :D

Copy link
Collaborator

@c2thorn c2thorn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

I'll work on getting this upstreamed to https://github.com/GoogleCloudPlatform/magic-modules where there'll be more testing and another review.

@cmcga1125
Copy link

not trying to be pesky - any idea when this will hit the provider?

@c2thorn
Copy link
Collaborator

c2thorn commented Mar 17, 2021

@cmcga1125 My estimate is version v3.62 scheduled for March 29th. Running the full test suite for google_container_cluster showed that this implementation introduces new test failures that need to be addressed before releasing. The cutoff for the v3.61 release was last night, so this looks like it will be released in v3.62.

@c2thorn
Copy link
Collaborator

c2thorn commented Mar 17, 2021

@paulwilljones If you are interested in taking a stab at the failing tests, I have found there are a few types of failures:

  • enable_shielded_nodes shows a diff after applying - seen in TestAccContainerCluster_withMasterAuthConfig
  • enable_autopilot shows a diff after importing - seen in TestAccContainerCluster_withReleaseChannelEnabled
  • A container cluster datasource tests are also failing: see TestAccContainerClusterDatasource_zonal

Otherwise, I plan to address them in GoogleCloudPlatform/magic-modules#4591

@ghost ghost added size/xl and removed size/l labels Mar 17, 2021
@paulwilljones paulwilljones force-pushed the autopilot branch 3 times, most recently from af508a6 to 8ea195f Compare March 18, 2021 08:52
@paulwilljones
Copy link
Contributor Author

paulwilljones commented Mar 18, 2021

@paulwilljones If you are interested in taking a stab at the failing tests, I have found there are a few types of failures:

  • enable_shielded_nodes shows a diff after applying - seen in TestAccContainerCluster_withMasterAuthConfig
  • enable_autopilot shows a diff after importing - seen in TestAccContainerCluster_withReleaseChannelEnabled
  • A container cluster datasource tests are also failing: see TestAccContainerClusterDatasource_zonal

Otherwise, I plan to address them in GoogleCloudPlatform/magic-modules#4591

Addressed in 8ea195f

@@ -364,10 +367,19 @@ func resourceContainerCluster() *schema.Resource {
},

"enable_shielded_nodes": {
Type: schema.TypeBool,
Optional: true,
Default: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@c2thorn Enabling Shielded Nodes by default is in alignment with GKE 1.18.

I'm seeing test failures (for example TestAccContainerCluster_withMasterAuthConfig) when shielded nodes is false:

# google_container_cluster.with_master_auth_no_cert will be updated in-place
          ~ resource "google_container_cluster" "with_master_auth_no_cert" {
              ~ enable_shielded_nodes       = true -> false

@c2thorn
Copy link
Collaborator

c2thorn commented Mar 24, 2021

@paulwilljones I've made a few changes to the upstream PR in GoogleCloudPlatform/magic-modules#4591 and believe I have a working iteration. Syncing this PR isn't required, since the upstream PR will cover the differences.

@ghost
Copy link

ghost commented Apr 30, 2021

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 as resolved and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants