-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[WIP] Require Google provider 4.0.0 #1071
Conversation
Thanks for the PR! 🚀 |
This is a bit fiddly, so I'm not surprised to see there are some failures, although at least the validation succeeds now. Can somebody (@bharathkkb?) paste the Cloud Build logs for the int trigger failure here? It'd be really handy if we could have Hmm, on the subject of |
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.
@jackwhelpton Thanks for working on this!
We will need to update https://github.com/terraform-google-modules/terraform-google-gcloud for 4.0 which is used by some modules here
* addresses warning about multiple provider blocks
Thanks for the heads up, I'll look at that bit next. |
Looks like the same problem over there: there's an automated PR that's passing the lint checks, but raising testing errors that aren't visible to us mere mortals. Any chance you could relay those? The PR in that case is terraform-google-modules/terraform-google-gcloud#108. |
@jackwhelpton I have updated gcloud to allow 4.0 and will cut a release in a bit. You can use main to iterate on this PR |
* fetches main branch by default?
…google-kubernetes-engine into feature/provider-upgrade
…terraform-google-kubernetes-engine into feature/provider-upgrade # Conflicts: # examples/node_pool/main.tf
There's quite a dependency chain here, next up is https://github.com/terraform-google-modules/terraform-google-vm/blob/master/modules/compute_disk_snapshot/versions.tf. That one doesn't seem to have an automated PR, I'll raise one... terraform-google-modules/terraform-google-vm#215 if anybody wants to help get that released. |
Thanks for the continued support on this. Looks like the |
@jackwhelpton thanks for working on these. For those modules we use in examples(like bastion host), we can leave the example at 3.x and not block this PR on that. If another module is used within a module(like gcloud), then we will need to fix those first. |
If the examples have dependencies that have dependencies that have ... that eventually rely on google < 4, doesn't that cause the conflicts we're still seeing above? |
I forgot in this case we have to constrain GKE module to 4.0+ due to breaking changes, so you are right |
Great, thanks... so I guess this guy is next? |
@@ -610,9 +607,10 @@ resource "google_container_node_pool" "pools" { | |||
for_each = local.cluster_node_metadata_config | |||
|
|||
content { | |||
node_metadata = lookup(each.value, "node_metadata", workload_metadata_config.value.node_metadata) | |||
mode = lookup(each.value, "node_metadata", workload_metadata_config.value.mode) |
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.
I believe this was done as a workaround for cases where the list was rebuilt. It's possible the bug has disappeared from Terraform Core so we should be okay with not including an empty key in maps. |
depends_on = [ | ||
google_container_cluster.primary | ||
] | ||
} | ||
|
||
output "instance_group_urls" { |
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'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 comment
The reason will be displayed to describe this comment to others. Learn more.
By all means: so you'd keep the new node_pools_
outputs but also include this?
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.
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 instance_group_urls
output is currently consumed: it's obviously possible to keep it as a single flattened list, but now the property has migrated to the node pool level within the provider I worried about the loss of information that would result from doing that.
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.
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.
autogen/main/outputs.tf.tmpl
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add a new output unless needed (see below coment).
autogen/main/outputs.tf.tmpl
Outdated
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 comment
The 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.
autogen/main/variables.tf.tmpl
Outdated
@@ -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 comment
The 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 workload_pool
).
I think I've addressed those comments, let me know what the build failure is now. Owing to our organizational setup it's unfortunately pretty tricky for me to run these integration tests locally. |
@jackwhelpton PFA logs. I didn't find anything off at a quick glance but saw hashicorp/terraform-provider-google#10494
|
Thanks for the repro on that linked ticket; it does indeed look like a provider bug. Damnit. |
* this should be removed once hashicorp/terraform-provider-google#10494 is addressed
I'm trying to set up an environment where I can run the integration tests locally... I actually made some progress towards this for some earlier work I did on the Workload Identity module. I've got a point where I can prepare the test environment using Looking in the I've tried walking through https://codelabs.developers.google.com/codelabs/cft-onboarding/#7 and running in interactive mode (executing a single example), but the results are the same.
|
Oh boo. @bharathkkb , could you share the reason for this recent failure so I can look into it? As far as I'm aware all I did was update to use the newly published versions of a couple of dependencies. I'm assuming it's just down to the known firewall bug which we're now discussing here: GoogleCloudPlatform/magic-modules#5526 |
…terraform-google-kubernetes-engine into feature/provider-upgrade # Conflicts: # autogen/main/versions.tf.tmpl # examples/node_pool_update_variant_beta/main.tf # examples/node_pool_update_variant_public_beta/main.tf # examples/regional_private_node_pool_oauth_scopes/provider.tf # examples/safer_cluster/main.tf # examples/safer_cluster_iap_bastion/provider.tf # examples/simple_regional_beta/main.tf # examples/simple_regional_private_beta/main.tf # examples/simple_zonal_with_asm/main.tf # examples/workload_metadata_config/main.tf # modules/beta-private-cluster-update-variant/versions.tf # modules/beta-private-cluster/versions.tf # modules/beta-public-cluster-update-variant/versions.tf # modules/beta-public-cluster/versions.tf
@jackwhelpton looks like the latest error is from
|
That makes sense, as SECURE has been deprecated now... fixed that (hopefully?) |
Thanks for the work so far on this @jackwhelpton & @bharathkkb. We have been eagerly waiting on this module to support Google provider version > 4.0.0. |
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.
Mostly LGTM with one comment below. Looks like we also need to rebase this @jackwhelpton
@@ -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 comment
The 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 source_tags = []
as a workaround
Error: Error creating Firewall: googleapi: Error 400: Invalid value for field 'resource.sourceTags[0]': ''. Must be a match of regex '(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?)', invalid
with module.example.module.gke.google_compute_firewall.master_webhooks[0],
on ../../../firewall.tf line 63, in resource "google_compute_firewall" "master_webhooks":
63: resource "google_compute_firewall" "master_webhooks" {
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thanks, we can probably follow through if necessary as well.
superseded by #1129 |
No description provided.