-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP] Require Google provider 4.0.0 #1071
Changes from 25 commits
5f8e135
860f790
6e4b330
0027c3b
12a6834
6138b52
6f99d53
6531be7
586e6f2
2e3f3d1
a8f5dbd
73b3891
9f88922
bce89ed
a1799fd
7130a05
dc95ed0
7eaf1a5
ca74832
37210e3
5fa17c7
00f5691
2e7a687
e4d372d
b374965
2d41101
019182a
092ef7a
3bebdce
03ff18a
2493bd5
0d0a4c8
3f8f9fd
345fa58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,10 +114,15 @@ output "node_pools_names" { | |
} | ||
|
||
output "node_pools_versions" { | ||
description = "List of node pools versions" | ||
description = "Node pool versions by node pool name" | ||
morgante marked this conversation as resolved.
Show resolved
Hide resolved
|
||
value = local.cluster_node_pools_versions | ||
} | ||
|
||
output "node_pools_instance_group_urls" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not add a new output unless needed (see below coment). |
||
description = "Lists of GKE generated instance groups by node pool name" | ||
value = local.cluster_node_pools_instance_group_urls | ||
} | ||
|
||
output "service_account" { | ||
description = "The service account to default running nodes as if not overridden in `node_pools`." | ||
value = local.service_account | ||
|
@@ -128,18 +133,13 @@ output "release_channel" { | |
value = var.release_channel | ||
} | ||
|
||
output "identity_namespace" { | ||
description = "Workload Identity namespace" | ||
value = length(local.cluster_workload_identity_config) > 0 ? local.cluster_workload_identity_config[0].identity_namespace : null | ||
output "workload_pool" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's a real need to change this output name, since it's still pointing to the same value. |
||
description = "Workload Identity pool" | ||
value = length(local.cluster_workload_identity_config) > 0 ? local.cluster_workload_identity_config[0].workload_pool : null | ||
depends_on = [ | ||
google_container_cluster.primary | ||
] | ||
} | ||
|
||
output "instance_group_urls" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to keep this output value, as it is helpful for broadly addressing the cluster. Could we simply concat all the instance groups from the different node pools? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By all means: so you'd keep the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, just saw your next comment, perhaps I'll wait for you to finish the review :) I don't think I have enough knowledge about how the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my experience, it's most useful for addressing the cluster as a whole to apply networking changes. Let's leave it as-is—we can always add an additional output later if requests come in, but every output we add is an addition to the API surface. |
||
description = "List of GKE generated instance groups" | ||
value = google_container_cluster.primary.instance_group_urls | ||
} | ||
{% if private_cluster %} | ||
|
||
output "master_ipv4_cidr_block" { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -379,18 +379,6 @@ variable "service_account" { | |
default = "" | ||
} | ||
|
||
variable "basic_auth_username" { | ||
type = string | ||
description = "The username to be used with Basic Authentication. An empty value will disable Basic Authentication, which is the recommended configuration." | ||
default = "" | ||
} | ||
|
||
variable "basic_auth_password" { | ||
type = string | ||
description = "The password to be used with Basic Authentication." | ||
default = "" | ||
} | ||
|
||
variable "issue_client_certificate" { | ||
type = bool | ||
description = "Issues a client certificate to authenticate to the cluster endpoint. To maximize the security of your cluster, leave this option disabled. Client certificates don't automatically rotate and aren't easily revocable. WARNING: changing this after cluster creation is destructive!" | ||
|
@@ -548,8 +536,8 @@ variable "database_encryption" { | |
}] | ||
} | ||
|
||
variable "identity_namespace" { | ||
description = "Workload Identity namespace. (Default value of `enabled` automatically sets project based namespace `[project_id].svc.id.goog`)" | ||
variable "workload_pool" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to change this variable name (can add a note in the description that this is otherwise known as |
||
description = "The workload pool to attach all Kubernetes service accounts to. (Default value of `enabled` automatically sets project-based pool `[project_id].svc.id.goog`)" | ||
type = string | ||
default = "enabled" | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
# Upgrading to v18.0 | ||
|
||
The v18.0 release of *kubernetes-engine* is a backwards incompatible release. | ||
|
||
### Kubernetes Basic Authentication removed | ||
Basic authentication is deprecated and has been removed in GKE 1.19 and later. | ||
Owing to this, the `basic_auth_username` and `basic_auth_password` variables | ||
have been eliminated. | ||
|
||
```diff | ||
module "gke" { | ||
source = "terraform-google-modules/kubernetes-engine/google//modules/private-cluster" | ||
- version = "~> 17.0" | ||
+ version = "~> 18.0" | ||
|
||
- basic_auth_username = "admin" | ||
- basic_auth_password = "s3crets!" | ||
} | ||
``` | ||
|
||
### identity_namespace renamed to workload_pool | ||
The `identity_namespace` variable has been renamed for consistency with the | ||
Kubernetes API; the behavior (e.g. enabling Workload Identity by default) | ||
remains the same. | ||
|
||
```diff | ||
module "gke" { | ||
source = "terraform-google-modules/kubernetes-engine/google" | ||
- version = "~> 17.0" | ||
+ version = "~> 18.0" | ||
|
||
- identity_namespace = null | ||
+ workload_pool = null | ||
} | ||
``` | ||
|
||
### node_pools_versions is now keyed by node-pool name | ||
The `node_pools_versions` output is now an object keyed by node-pool name, | ||
rather than a list as previously. | ||
|
||
### instance_group_urls is now removed | ||
The `instance_group_urls` output has been removed in favor of a node-pool level | ||
output `node_pools_instance_group_urls`, keyed by node-pool name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to change the input value (ie. still look at
node_metadata
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to refresh my memory on this (and find a line reference), but I think I'm still using the original input value, but I've adjusted the
workload_metadata_config
object to match the names of the new properties, so it serves as an adapter between the two; at the time that seemed to make the most sense to me.