-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: additional_ip_ranges_config - again #2458
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
base: main
Are you sure you want to change the base?
fix: additional_ip_ranges_config - again #2458
Conversation
/gcbrun |
autogen/main/cluster.tf.tmpl
Outdated
subnetwork = var.additional_ip_ranges_config.value.subnetwork | ||
pod_ipv4_range_names = var.additional_ip_ranges_config.value.pod_ipv4_range_names |
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.
Shouldn't this be like this instead?
subnetwork = var.additional_ip_ranges_config.value.subnetwork | |
pod_ipv4_range_names = var.additional_ip_ranges_config.value.pod_ipv4_range_names | |
subnetwork = additional_ip_ranges_config.value.subnetwork | |
pod_ipv4_range_names = additional_ip_ranges_config.value.pod_ipv4_range_names |
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.
Based on the docs at https://developer.hashicorp.com/terraform/language/expressions/dynamic-blocks
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.
+1
@DrFaust92 - Can you please add a usage to the existing node_pool example and test data so we have sufficient test coverage. Thanks!
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.
Ok, I think i added the changes and test. can we trigger a run?
1c84b6c
to
8960c33
Compare
8960c33
to
a82e717
Compare
/gcbrun |
1 similar comment
/gcbrun |
/gcbrun |
Hi @DrFaust92 - I'm going to run the test again, but this is the only PR I'm currently seeing this error:
|
/gcbrun |
Signed-off-by: drfaust92 <[email protected]>
Signed-off-by: drfaust92 <[email protected]>
Signed-off-by: drfaust92 <[email protected]>
Signed-off-by: drfaust92 <[email protected]>
Thanks @DrFaust92, still seeing:
|
I don't think the additional_ip_ranges_config variable needs to be defined as a list... see my comment here -> #2437 (comment) |
db2480a
to
faed61b
Compare
Signed-off-by: drfaust92 <[email protected]>
examples/node_pool/main.tf
Outdated
additional_ip_ranges_config = [ | ||
{ | ||
subnetwork = var.subnetwork | ||
subnetwork = "projects/${var.project_id}/regions/${var.region}/subnetworks/${var.subnetwork}" |
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.
@apeabody I think this was the issue, plan passes either way. but error is strange
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.
additional_ip_ranges_config is not a list/array as defined in the google_container_cluster documentation. See https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/container_cluster#additional_ip_ranges_config-1
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.
yes it is, according to provider code and google container api
https://github.com/hashicorp/terraform-provider-google/blob/4003211203eb953bab681ae9a2972eff1a90ef09/google/services/container/resource_container_cluster.go#L1695
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.
Well that's interesting. So as a data point, I was able to update the module locally so that it was an 'object' and NOT a 'list' and it then the terraform plan handled this section for me correctly. As a list it complained, as detailed in the comment I referenced above.
I'm not sure why the discrepancy..?
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.
It looks like in the Google console UI it is a List so I can try again and see what comes out of the changes here
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.
@DrFaust92 sorry for the spam, just want to let you know you are right and the changes you have here I tested locally in my environment and fix my issue, so for what it's worth LGTM! Thanks.
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.
Thanks @DrFaust92 - Looks like we are close
Error: Error updating cluster for []: googleapi: Error 400: The cluster's default subnetwork cannot be used as an additional subnetwork.
If needed, this is where the current one is defined: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/main/test/fixtures/node_pool/network.tf
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.
lets try once more @apeabody , thanks for bearing with me on 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.
Thanks @DrFaust92!
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.
Hi @DrFaust92 - Looks like the docs need to be regenerated after the change. Thanks!
Checking submodule's files generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=.git' /workspace/examples/node_pool/README.md /tmp/tmp.VdIZFvHmj1/workspace/examples/node_pool/README.md
10a11
> | additional\_ip\_pod\_range\_subnetwork | The subnetwork to host the additional pod range in | `any` | n/a | yes |
Error: submodule's files generation has not been run, please run the
'make build' command and commit changes
Checking for documentation generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=autogen' '--exclude=*.tfvars' '--exclude=*metadata.yaml' /workspace/examples/node_pool/README.md /tmp/tmp.QlBKcRfkkq/generate_docs/workspace/examples/node_pool/README.md
10a11
> | additional\_ip\_pod\_range\_subnetwork | The subnetwork to host the additional pod range in | `any` | n/a | yes |
Error: Documentation generation has not been run, please run the
'make docker_generate_docs' command and commit the above changes.
/gcbrun |
Signed-off-by: drfaust92 <[email protected]>
/gcbrun |
apologies, missed this again
Fixes: #2437