-
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 all 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 |
---|---|---|
|
@@ -81,6 +81,7 @@ resource "google_compute_firewall" "master_webhooks" { | |
direction = "INGRESS" | ||
|
||
source_ranges = [local.cluster_endpoint_for_nodes] | ||
source_tags = [""] | ||
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. CI seems to be failing due to this. IIRC we added this due hashicorp/terraform-provider-google#10494. Maybe we should do
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 thought the "correct" (?) fix for that was covered by this: GoogleCloudPlatform/magic-modules#5526 so we may still see the CI failing until that (or something better) is merged. On a more personal note, I left my previous employer at the end of last year, so it may be hard for me to take this much further, as the CLA etc. was signed with that email. I'm in touch with a former coworker who I'm going to try and persuade to finish this off for me; I'll let you know how that goes. 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.
Thanks, we can probably follow through if necessary as well. |
||
target_tags = [local.cluster_network_tag] | ||
|
||
allow { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,7 +114,7 @@ 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 | ||
} | ||
|
||
|
@@ -123,23 +123,23 @@ output "service_account" { | |
value = local.service_account | ||
} | ||
|
||
output "instance_group_urls" { | ||
description = "List of GKE generated instance groups" | ||
value = distinct(flatten([for np in google_container_node_pool.pools : np.managed_instance_group_urls])) | ||
} | ||
|
||
output "release_channel" { | ||
description = "The release channel of this cluster" | ||
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 | ||
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 |
---|---|---|
@@ -0,0 +1,65 @@ | ||
# Upgrading to v18.0 | ||
|
||
The v18.0 release of *kubernetes-engine* is a backwards incompatible release. | ||
|
||
### Google Cloud Platform Provider upgrade | ||
The Terraform Kubernetes Engine Module now requires version 4.0 or higher of | ||
the Google Cloud Platform Provider. | ||
|
||
```diff | ||
terraform { | ||
required_providers { | ||
google = { | ||
source = "hashicorp/google" | ||
- version = "~> 3.0" | ||
+ version = "~> 4.0" | ||
} | ||
google-beta = { | ||
source = "hashicorp/google-beta" | ||
- version = "~> 3.0" | ||
+ version = "~> 4.0" | ||
} | ||
|
||
} | ||
} | ||
``` | ||
|
||
### 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!" | ||
} | ||
``` | ||
|
||
### Acceptable values for node_metadata modified | ||
The `node_metadata` variable should now be set to one of `GKE_METADATA`, | ||
`GCE_METADATA` or `UNSPECIFIED`. `GKE_METADATA` replaces the previous | ||
`GKE_METADATA_SERVER` value, `GCE_METADATA` should be used in place of | ||
`EXPOSE`. The `SECURE` option, previously deprecated, has now been removed. | ||
|
||
```diff | ||
module "gke" { | ||
source = "../../modules/safer-cluster" | ||
|
||
node_pools = [ | ||
{ | ||
|
||
- node_metadata = "GKE_METADATA_SERVER" | ||
+ node_metadata = "GKE_METADATA" | ||
} | ||
] | ||
} | ||
``` | ||
|
||
### 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. |
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.