-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: Support for GKE private clusters without default node pool #2408
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?
feat: Support for GKE private clusters without default node pool #2408
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@ciroclover, I think, in such an arrangement, it would never be accepted. First of all, you have to use autogen to generate all submodules automatically. Also, don't forget to generate docs and perform proper linting... I would recommend you make yourself familiar with the contribution guide |
390dcc1 to
d985e40
Compare
|
cbab4aa to
3800ac7
Compare
3800ac7 to
eeaf95d
Compare
|
Rebased with the main branch, fixed the template conflict, and ran |
|
/gcbrun |
|
Hi, I would also benefit from this PR. Is there anything that is stopping it from further review? |
|
/gemini review |
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.
Code Review
This pull request introduces support for creating GKE clusters without a default node pool by making the node_pool block dynamic and adjusting related logic. The changes correctly handle the case where no node pools are defined. However, I've identified a critical pre-existing issue in how the inline default node pool is configured, which this PR touches. It incorrectly uses configuration from the first user-defined node pool (var.node_pools[0]), which can lead to misconfigurations. I've provided a detailed comment on this with recommendations for a safer approach. Additionally, I've suggested a simplification for computing the cluster name, which you also hinted at in your description.
|
/gcbrun |
|
@apeabody Do you plan to merge this one? We also trying to migrate onto computeclasses and now it's not possible to create empty GKE public cluster |
|
/gemini review |
|
/gcbrun |
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.
Code Review
This pull request aims to add support for GKE private clusters without a default node pool. The changes involve making the node_pool block within the google_container_cluster resource dynamic, adjusting initial_node_count logic, and simplifying how the cluster name is derived for outputs. While the changes are mostly correct and improve the module's flexibility, I've found a critical issue in the condition for the dynamic node_pool block. This issue would cause Terraform to fail in scenarios where remove_default_node_pool is set to true while node_pools is not empty. My review includes suggestions to fix this logic across all affected files.
|
/gemini review |
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.
Code Review
The pull request successfully implements support for GKE private clusters without a default node pool. The changes correctly make the initial_node_count and the default node_pool block conditional based on whether node pools are explicitly defined or if the default node pool is to be removed. Additionally, the cluster_name_computed logic has been simplified to directly use var.name, which is a more robust approach when the existence of a default node pool is not guaranteed. The changes align well with the stated objective of the pull request.
|
@apeabody I think it's good to go! 🚀 |
|
/gcbrun |
Thanks @ciroclover - From the test: |
Fixes #2407
The diff shows a massive difference due to the "dynamic" block indentation.
The changes are:
initial_node_countwhen is not defined in the node-pool [ref].node_pools = [][ref]main.tf. I honestly believe that we could just usecluster_name_computed = var.name, however, more tests should be needed. [ref]