Skip to content
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

feat(TPG>=5.44.2)!: add standard cluster support for insecureKubeletReadonlyPortEnabled #2082

Merged

Conversation

wyardley
Copy link
Contributor

@wyardley wyardley commented Sep 11, 2024

This adds support for insecure_kubelet_readonly_port_enabled, now that it's supported at the provider level.
This will need either > 5.44.2 (cherrypicked update) or > 6.7.0, so will be a breaking change; happy to tweak the title and / or commit type as desired.

Fixes #2013

@wyardley wyardley requested review from ericyz, gtsorbo and a team as code owners September 11, 2024 05:57
@wyardley wyardley changed the title fix(TPG>=5.44)!: add support for insecureKubeletReadonlyPortEnabled feat(TPG>=5.44)!: add support for insecureKubeletReadonlyPortEnabled Sep 11, 2024
@wyardley wyardley force-pushed the wyardley/issues/2013 branch from 2faae5f to 1c97a95 Compare September 11, 2024 15:59
@apeabody
Copy link
Collaborator

/gcbrun

@wyardley
Copy link
Contributor Author

Thanks for triggering @apeabody
Would you mind letting me know what the failed tests are? If it gets super complicated, I can get the test setup working locally, but if it's something simple, I can try to fix.

@apeabody
Copy link
Collaborator

Thanks for triggering @apeabody Would you mind letting me know what the failed tests are? If it gets super complicated, I can get the test setup working locally, but if it's something simple, I can try to fix.

I think this might have been a flaky run, re-triggered the test.

@apeabody
Copy link
Collaborator

/gcbrun

@wyardley
Copy link
Contributor Author

Thanks, yeah, passed that time. Any other adjustments you all want? presumably this will have to wait for another major release?

@wyardley
Copy link
Contributor Author

Not sure if there's anything special that needs to be done to support the situation where the default changes down the road. I think leaving the default null should already handle that Ok?
When I applied 33.x version module without this change and 6.2.0 provider, it behaved somewhat as I'd expect, reflecting the new setting in the state, but not triggering any changes in the plan.

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody
Copy link
Collaborator

Thanks, yeah, passed that time. Any other adjustments you all want? presumably this will have to wait for another major release?

Correct - This is a breaking change, so will need to wait till we start assembling the next major version.

@apeabody
Copy link
Collaborator

Not sure if there's anything special that needs to be done to support the situation where the default changes down the road. I think leaving the default null should already handle that Ok? When I applied 33.x version module without this change and 6.2.0 provider, it behaved somewhat as I'd expect, reflecting the new setting in the state, but not triggering any changes in the plan.

It's hard to predict, but as this is a field (and not a block), leaving as null is probably a good option.

@apeabody apeabody self-assigned this Sep 13, 2024
@wyardley wyardley closed this Sep 13, 2024
@wyardley wyardley deleted the wyardley/issues/2013 branch September 13, 2024 19:20
@wyardley wyardley restored the wyardley/issues/2013 branch September 13, 2024 19:21
@wyardley wyardley reopened this Sep 13, 2024
@apeabody
Copy link
Collaborator

/gcbrun

1 similar comment
@apeabody
Copy link
Collaborator

/gcbrun

Copy link
Contributor Author

@wyardley wyardley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apeabody I realized this probably needs to also be tested in a couple more spots (and probably should use "FALSE" as the default, since even though "TRUE" may be a safer bet, we probably don't want to encourage people to set that by putting it in examples) , and probably setup for autopilot clusters too? Does it make sense to use a single parameter to handle

  • node_pool_auto_config.kubelet_config.insecure_kubelet_readonly_port_enabled for autopilot clusters (currently a bit stuck on how to handle that block)
  • node_pool_defaults.node_config_defaults.insecure_kubelet_readonly_port_enabled (I think node_pool_defaults.node_config_defaults needs to have its beta gate removed as well?
  • node pool settings (node_config.kubelet_config) at the node-pool level

I guess maybe we need to differentiate between the setting for a given node pool vs. the autopilot or node_pool_defaults cases, where there'd just be a parameter set at the top level, so maybe don't try to solve that problem as part of this PR?

Does it make sense to have the same parameter name (insecure_kubelet_readonly_port_enabled) as a module variable and as a value within the node_pools var as well?

@apeabody
Copy link
Collaborator

apeabody commented Sep 13, 2024

@apeabody I realized this probably needs to also be tested in a couple more spots (and probably should use "FALSE" as the default, since even though "TRUE" may be a safer bet, we probably don't want to encourage people to set that by putting it in examples) , and probably setup for autopilot clusters too? Does it make sense to use a single parameter to handle

  • node_pool_auto_config.kubelet_config.insecure_kubelet_readonly_port_enabled for autopilot clusters (currently a bit stuck on how to handle that block)
  • node_pool_defaults.node_config_defaults.insecure_kubelet_readonly_port_enabled (I think node_pool_defaults.node_config_defaults needs to have its beta gate removed as well?
  • node pool settings (node_config.kubelet_config) at the node-pool level

I guess maybe we need to differentiate between the setting for a given node pool vs. the autopilot or node_pool_defaults cases, where there'd just be a parameter set at the top level, so maybe don't try to solve that problem as part of this PR?

Does it make sense to have the same parameter name (insecure_kubelet_readonly_port_enabled) as a module variable and as a value within the node_pools var as well?

I haven't looked into these specifically, but generally we want to retain the ability for a separate cluster vs nodepool configuration - same parameter name is fine. Looking into node_pool_defaults.node_config_defaults, I would suggest it be best to do an separate/initial PR to promote that from beta clusters for clarity of the change. Fortunately node_config_defaults is already GA in 5.40, so it can be a normal Feat: PR. :)

@wyardley
Copy link
Contributor Author

but generally we want to retain the ability for a separate cluster vs nodepool configuration - same parameter name is fine

Ok, I may convert this to a draft, but just pushed up something for you to look at here.

While it would be nice to have it be a true bool, I'm thinking match the upstream behavior of "TRUE" / "FALSE" (look at that PR if you want more detail about why it got implemented that way), which also lets us have it be optional (vs. a boolean?)

@wyardley wyardley marked this pull request as draft September 13, 2024 23:41
@wyardley wyardley force-pushed the wyardley/issues/2013 branch from dbe36b1 to 0411c32 Compare September 13, 2024 23:42
@apeabody
Copy link
Collaborator

/gcbrun

@wyardley wyardley changed the title feat(TPG>=5.44.1)!: add standard cluster support for insecureKubeletReadonlyPortEnabled feat(TPG>=5.44.2)!: add standard cluster support for insecureKubeletReadonlyPortEnabled Oct 14, 2024
@wyardley wyardley force-pushed the wyardley/issues/2013 branch from c4db48a to b6c5fa6 Compare October 14, 2024 19:15
@wyardley
Copy link
Contributor Author

@apeabody updated if you want to see how things look with those latest provider updates reflected. Obviously if this is released in the next major, these ugly version specifications can be greatly simplified if #2126 then just updates the min version to 6.7.0.

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@apeabody apeabody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @wyardley!

@apeabody apeabody added the P1 highest priority issues label Oct 15, 2024
@wyardley
Copy link
Contributor Author

@apeabody is the test failure now due to this setting / expectations, or another transient failure?

@apeabody
Copy link
Collaborator

@apeabody is the test failure now due to this setting / expectations, or another transient failure?

node pool timed out, retriggered

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody apeabody merged commit 96626d5 into terraform-google-modules:master Oct 16, 2024
4 checks passed
@wyardley wyardley deleted the wyardley/issues/2013 branch October 17, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 highest priority issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

insecureKubeletReadonlyPortEnabled support
4 participants