feat(safer-cluster): add some missing variables #2429

bergemalm · 2025-08-29T07:02:28Z

This PR adds some missing variables in the safer-cluster modules.

node_pools_resource_manager_tags
node_pools_linux_node_configs_sysctls
node_pools_cgroup_mode
node_pools_hugepage_size_2m
node_pools_hugepage_size_1g
enable_secret_manager_addon

apeabody

Thanks for the contribution @bergemalm

Can you please build the templates and add to the commit: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/main/CONTRIBUTING.md#templating

bergemalm · 2025-09-03T06:48:01Z

Thanks for the contribution @bergemalm

Can you please build the templates and add to the commit: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/main/CONTRIBUTING.md#templating

Done. @apeabody

apeabody · 2025-09-03T20:06:43Z

/gcbrun

bergemalm · 2025-10-13T09:30:11Z

Checking in on this one. Any reason for not adding this one on last release? @apeabody

apeabody · 2025-10-31T15:46:12Z

/gemini review

gemini-code-assist

Code Review

This pull request adds several new variables to the safer-cluster modules, exposing more configuration options for GKE node pools and enabling the Secret Manager addon. The changes are mostly boilerplate and have been applied across multiple generated files. I've identified a few minor issues related to formatting, ordering, and a description that could be improved for consistency and maintainability.

autogen/safer-cluster/variables.tf.tmpl

modules/safer-cluster-update-variant/metadata.yaml

modules/safer-cluster/metadata.yaml

apeabody · 2025-10-31T15:57:50Z

/gcbrun

bergemalm · 2025-12-01T13:21:59Z

Is there anything waiting on me to release this PR @apeabody ?

apeabody

Thanks for the contribution @bergemalm!

I've checked with the team, and to maintain the "safer-than-default configuration" of the safer-cluster, please drop sysctls and the 1g hugepage. Otherwise this looks, good. Thanks!

autogen/safer-cluster/main.tf.tmpl

- node_pools_resource_manager_tags - node_pools_cgroup_mode - node_pools_hugepage_size_2m - enable_secret_manager_addon

bergemalm · 2025-12-02T10:14:40Z

I removed node_pools_linux_node_configs_sysctls and node_pools_hugepage_size_1g and ran the make build - which then also added "display" yaml files (more than last run in August) where some still have the removed vars.
As mentioned I have to migrate away from this module so feel free to merge this or not.

apeabody · 2025-12-02T18:53:35Z

/gcbrun

apeabody · 2025-12-02T21:40:39Z

/gemini review

gemini-code-assist

Code Review

This pull request adds several new variables to the safer-cluster modules. While the changes are mostly straightforward, there are a few areas that need attention. Some variables mentioned in the metadata files (node_pools_hugepage_size_1g, node_pools_linux_node_configs_sysctls) appear to be incompletely implemented as they are missing from the Terraform variable definitions and module calls. Additionally, there are some formatting inconsistencies, such as unaligned code in .tf files and unsorted lists in documentation and metadata, which should be addressed to improve maintainability. I've also noticed some misleading comments in variable definitions and an unrelated change for monitoring_auto_monitoring_config_scope that would be better in a separate PR. My detailed comments below provide specific suggestions for these points.

gemini-code-assist · 2025-12-02T21:42:46Z

modules/safer-cluster-update-variant/metadata.display.yaml

+        node_pools_cgroup_mode:
+          name: node_pools_cgroup_mode
+          title: Node Pools Cgroup Mode
+        node_pools_hugepage_size_1g:
+          name: node_pools_hugepage_size_1g
+          title: Node Pools Hugepage Size 1G
+        node_pools_hugepage_size_2m:
+          name: node_pools_hugepage_size_2m
+          title: Node Pools Hugepage Size 2M
        node_pools_labels:
          name: node_pools_labels
          title: Node Pools Labels
+        node_pools_linux_node_configs_sysctls:
+          name: node_pools_linux_node_configs_sysctls
+          title: Node Pools Linux Node Configs Sysctls


There are a couple of issues here:

The variables node_pools_hugepage_size_1g and node_pools_linux_node_configs_sysctls are added to this metadata file, but they don't seem to be defined in variables.tf or used in main.tf. This looks like an incomplete implementation. Please either implement them fully or remove them for now.

The variables in this section are not sorted alphabetically. Please sort them to improve readability and maintainability.

gemini-code-assist · 2025-12-02T21:42:46Z

modules/safer-cluster/metadata.display.yaml

+        node_pools_cgroup_mode:
+          name: node_pools_cgroup_mode
+          title: Node Pools Cgroup Mode
+        node_pools_hugepage_size_1g:
+          name: node_pools_hugepage_size_1g
+          title: Node Pools Hugepage Size 1G
+        node_pools_hugepage_size_2m:
+          name: node_pools_hugepage_size_2m
+          title: Node Pools Hugepage Size 2M
        node_pools_labels:
          name: node_pools_labels
          title: Node Pools Labels
+        node_pools_linux_node_configs_sysctls:
+          name: node_pools_linux_node_configs_sysctls
+          title: Node Pools Linux Node Configs Sysctls


There are a couple of issues here:

The variables node_pools_hugepage_size_1g and node_pools_linux_node_configs_sysctls are added to this metadata file, but they don't seem to be defined in variables.tf or used in main.tf. This looks like an incomplete implementation. Please either implement them fully or remove them for now.

The variables in this section are not sorted alphabetically. Please sort them to improve readability and maintainability.

gemini-code-assist · 2025-12-02T21:42:47Z

autogen/safer-cluster/variables.tf.tmpl

+  type        = map(string)
+  description = "Map of strings containing cgroup node config by node-pool name"
+
+  # Default is being set in variables_defaults.tf


This comment is misleading as the default value is defined in the block immediately below. Please remove this line to avoid confusion.

gemini-code-assist · 2025-12-02T21:42:47Z

autogen/safer-cluster/variables.tf.tmpl

+  type        = map(string)
+  description = "Map of strings containing hugepage size 2m node config by node-pool name"
+
+  # Default is being set in variables_defaults.tf


This comment is misleading as the default value is defined in the block immediately below. Please remove this line to avoid confusion.

gemini-code-assist · 2025-12-02T21:42:47Z

metadata.display.yaml

+        monitoring_auto_monitoring_config_scope:
+          name: monitoring_auto_monitoring_config_scope
+          title: Monitoring Auto Monitoring Config Scope


The addition of monitoring_auto_monitoring_config_scope seems unrelated to the other changes in this pull request. To keep PRs focused and easier to review, it's best to submit unrelated changes in a separate PR.

gemini-code-assist · 2025-12-02T21:42:48Z

modules/safer-cluster-update-variant/variables.tf

+  type        = map(string)
+  description = "Map of strings containing cgroup node config by node-pool name"
+
+  # Default is being set in variables_defaults.tf


This comment is misleading as the default value is defined in the block below. Please remove this line.

gemini-code-assist · 2025-12-02T21:42:48Z

modules/safer-cluster-update-variant/variables.tf

+  type        = map(string)
+  description = "Map of strings containing hugepage size 2m node config by node-pool name"
+
+  # Default is being set in variables_defaults.tf


This comment is misleading as the default value is defined in the block below. Please remove this line.

gemini-code-assist · 2025-12-02T21:42:48Z

modules/safer-cluster/variables.tf

+  type        = map(string)
+  description = "Map of strings containing cgroup node config by node-pool name"
+
+  # Default is being set in variables_defaults.tf


This comment is misleading as the default value is defined in the block below. Please remove this line.

gemini-code-assist · 2025-12-02T21:42:48Z

modules/safer-cluster/variables.tf

+  type        = map(string)
+  description = "Map of strings containing hugepage size 2m node config by node-pool name"
+
+  # Default is being set in variables_defaults.tf


This comment is misleading as the default value is defined in the block below. Please remove this line.

apeabody · 2025-12-02T21:51:46Z

I removed node_pools_linux_node_configs_sysctls and node_pools_hugepage_size_1g and ran the make build - which then also added "display" yaml files (more than last run in August) where some still have the removed vars. As mentioned I have to migrate away from this module so feel free to merge this or not.

Thanks @bergemalm - Yes, if you need sysctls then rather than safe-cluster I would recommend using the beta-private-cluster module.

bergemalm requested review from a team, apeabody and ericyz as code owners August 29, 2025 07:02

apeabody requested changes Sep 2, 2025

View reviewed changes

bergemalm force-pushed the safer-clusters-missing-variables branch from 95b1031 to 49f136d Compare September 3, 2025 06:46

gemini-code-assist bot reviewed Oct 31, 2025

View reviewed changes

autogen/safer-cluster/variables.tf.tmpl Outdated Show resolved Hide resolved

modules/safer-cluster-update-variant/metadata.yaml Show resolved Hide resolved

modules/safer-cluster/metadata.yaml Show resolved Hide resolved

apeabody requested changes Dec 2, 2025

View reviewed changes

autogen/safer-cluster/main.tf.tmpl Outdated Show resolved Hide resolved

autogen/safer-cluster/main.tf.tmpl Outdated Show resolved Hide resolved

feat(safer-cluster): add some missing variables

a5ce97e

- node_pools_resource_manager_tags - node_pools_cgroup_mode - node_pools_hugepage_size_2m - enable_secret_manager_addon

bergemalm force-pushed the safer-clusters-missing-variables branch from 490beed to a5ce97e Compare December 2, 2025 10:04

gemini-code-assist bot reviewed Dec 2, 2025

View reviewed changes

feat(safer-cluster): add some missing variables #2429

Are you sure you want to change the base?

feat(safer-cluster): add some missing variables #2429

Uh oh!

Conversation

bergemalm commented Aug 29, 2025

Uh oh!

apeabody left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergemalm commented Sep 3, 2025

Uh oh!

apeabody commented Sep 3, 2025

Uh oh!

bergemalm commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apeabody commented Oct 31, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apeabody commented Oct 31, 2025

Uh oh!

bergemalm commented Dec 1, 2025

Uh oh!

apeabody left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bergemalm commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apeabody commented Dec 2, 2025

Uh oh!

apeabody commented Dec 2, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

apeabody commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

apeabody left a comment •

edited

Loading

bergemalm commented Oct 13, 2025 •

edited

Loading

bergemalm commented Dec 2, 2025 •

edited

Loading