chore(CI): add insecure_kubelet_readonly_port_enabled

[apeabody]

No description provided.

[apeabody]

Hi @wyardley - FYI, I happened to discover that insecure_kubelet_readonly_port_enabled isn't a bool but actually a string with accepted values of "FALSE" or "TRUE":

https://github.com/hashicorp/terraform-provider-google/blob/main/google/services/container/node_config.go#L82

I think our insecure_kubelet_readonly_port_enabled variable should still be a bool, so an option might be to use:
insecure_kubelet_readonly_port_enabled = var.insecure_kubelet_readonly_port_enabled != null ? upper(tostring(var.insecure_kubelet_readonly_port_enabled)) : null

[wyardley]

Yes, discussed / mentioned here:
#2082 (comment)

I added that feature, so definitely aware that it's an enum at the provider level (for Reasons(TM)). We had decided to make it an optional bool here; you're right that probably it should be coerced to the correct value in the code, and it's possible I just never updated it, but it's interesting to me that the tests ever passed with the example added in #2082 with the existing code - I'd think even the validation should fail because of the provider requiring the enum values.

[apeabody]

Yes, discussed / mentioned here: #2082 (comment)

I added that feature, so definitely aware that it's an enum at the provider level (for Reasons(TM)). We had decided to make it an optional bool here; you're right that probably it should be coerced to the correct value in the code, and it's possible I just never updated it, but it's interesting to me that the tests ever passed with the example added in #2082 with the existing code - I'd think even the validation should fail because of the provider requiring the enum values.

Thanks @wyardley!

Looks like the #2082 tests are both for node_pools (not cluster), which doesn't enforce the TF bool type (as it's a lookup) and so was actually defined using a string: insecure_kubelet_readonly_port_enabled = "FALSE". The issue I identified is when set at the cluster level TF enforces the bool type, which isn't accepted by the API (if only because the string is the wrong case).

I'll update this PR to fix and include a cluster test.

[wyardley]

Oh, I see, interesting, looks like I'd left the wrong code in the tests. I guess the module didn't complain because some of those settings are inside a data structure so the type isn't getting validated? I'll make a PR to update, and incorporate the update to convert from bool to the expected strings (from a quick test, doesn't seem like terraform will allow anything other than the exact value of true or false as a bool, right, i.e., someone couldn't use 1 or t as a truthy value?)

[wyardley]

I'll update this PR to fix and include a cluster test.

I have something close to done if you want (#2145), otherwise, feel free to adjust this -- whatever's easier.

[apeabody]

I'll update this PR to fix and include a cluster test.

I have something close to done if you want (#2145), otherwise, feel free to adjust this -- whatever's easier.

Thanks @wyardley! Let's see if we can use yours as I can approve it!

[apeabody]

Oh, I see, interesting, looks like I'd left the wrong code in the tests. I guess the module didn't complain because some of those settings are inside a data structure so the type isn't getting validated? I'll make a PR to update, and incorporate the update to convert from bool to the expected strings (from a quick test, doesn't seem like terraform will allow anything other than the exact value of true or false as a bool, right, i.e., someone couldn't use 1 or t as a truthy value?)

Unfortunately "node_pools" is (currently) defined as list(map(any)), so it doesn't get the same type validation as the cluster level variable. With the TF v1.3's introduction of optionals it should likely be possible to type define "node_pools", but this greatly pre-dates that release.

Yeah, bool can only be true, false, or the variable null (which we already check).

[apeabody]

Close in favor of #2145