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

Consider deprecating / removing remove_default_node_pool in google_container_cluster #4963

Open
nicktrav opened this issue Nov 21, 2019 · 3 comments

Comments

@nicktrav
Copy link

nicktrav commented Nov 21, 2019

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment. If the issue is assigned to the "modular-magician" user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If the issue is assigned to a user, that user is claiming responsibility for the issue. If the issue is assigned to "hashibot", a community member has claimed the issue already.

Description

When creating a cluster for the first time, the initial_node_count can be set along with remove_default_node_pool to remove the default node pool (this is mentioned in the official documentation for the resource).

In the case that a resource is manually created, say with gcloud, and then imported into the state file, subsequent plans will attempt to re-create the cluster. For example:

      ~ initial_node_count          = 0 -> 1 # forces replacement

It appears this is due gcloud leaving this field unset (the API documentation states that the field is
deprecated on the cluster object).

Given this field is deprecated on the underlying GCP API that Terraform is calling, I'd like to propose the following:

  • prevent a difference in initial_node_count from re-creating the cluster
  • remove / deprecate the field in the Terraform resource

Relevant code for the first point is here:
https://github.com/terraform-providers/terraform-provider-google/blob/2eb04684ddf7de1b2f860b922168d20727caeba1/google/resource_container_cluster.go#L292-L296

New or Affected Resource(s)

  • google_container_cluster

References

b/374161595

@ghost ghost added the enhancement label Nov 21, 2019
@rileykarson
Copy link
Collaborator

rileykarson commented Nov 21, 2019

Hey @nicktrav! This was one of the deprecations (and related to several of the deprecations) that I was considering for 3.0.0, however I ultimately decided against it. While creating a GKE cluster isn't a great experience and I wanted to improve it, there wasn't a clear enough benefit from removing the field to justify the impact on existing users.

In the underlying API, all of the "special" fields for the default node pool are deprecated. They can be specified by creating a node_pool with instead. My most feasible proposal involved keeping remove_default_node_pool but moving configuration of the default pool entirely into the node_pool block:

resource "google_container_cluster" "primary" {
  name     = "my-gke-cluster"
  location = "us-central1"

  # We can't create a cluster with no node pool defined, but we want to only use
  # separately managed node pools. So we create the smallest possible default
  # node pool and immediately delete it.
  remove_default_node_pool = true
-  initial_node_count = 1

  # Because we've deleted the default Compute Engine service account, even the
  # temporary node pool needs a service account defined.
-  node_config {
-    service_account = "terraform@my-project.iam.gserviceaccount.com"
-  }
+  node_pool {
+    name = "default-pool"
+    initial_node_count = 1
+    node_config {
+      service_account = "terraform@my-project.iam.gserviceaccount.com"
+    }
+  }
  
  # stop a diff based on the default pool being gone
  lifecycle {
-    ignore_changes = ["node_config"]
+    ignore_changes = ["node_pool"]
  } 
}

resource "google_container_node_pool" "primary" {
  name       = "my-only-node-pool"
  location   = "us-central1"
  cluster    = "${google_container_cluster.primary.name}"
  node_count = 1
  
  node_config {
    service_account = "terraform@my-project.iam.gserviceaccount.com"
  }
}

This would get around the difficulties in importing clusters, but it's a large refactor for many users.

I think the thing that would make a deprecation like this worth it is if we could create pool-less clusters, and remove inline configuration of node pools altogether. Removing that option would considerably simplify the UX in Terraform, and be well worth it in a future major release. I've opened an issue against the underlying GKE API (b/132685471, for my own reference), however it hasn't had any uptake unfortunately.


To solve your issue at import time, lifecycle.ignore_changes is the right approach today. initial_ fields are expected to recreate the resource if modified, because they're controls for how a resource was created. Unfortunately, we can't get the config value into state at import time.

I'll tag this as a 4.0.0 issue to track changes to this field / flow over the next year or so.

@rileykarson rileykarson added this to the 4.0.0 milestone Nov 21, 2019
@nikhilk
Copy link

nikhilk commented Dec 4, 2019

+1 to removing the inline config of node pools to support node-pool-less clusters, and faster creation of clusters (avoiding creation/deleting of default node pool).

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this issue Jul 12, 2021
Signed-off-by: Modular Magician <magic-modules@google.com>
modular-magician added a commit that referenced this issue Jul 12, 2021
Signed-off-by: Modular Magician <magic-modules@google.com>
@roaks3 roaks3 added the forward/review In review; remove label to forward label Aug 17, 2023
@roaks3
Copy link
Collaborator

roaks3 commented Aug 17, 2023

Adding review label until we determine an appropriate type that can be forwarded

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

No branches or pull requests

9 participants