-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[safer-cluster] Replace "kubernetes_version" with "release_channel" #487
Changes from 5 commits
4d117c4
8e73494
503212d
cddc4d8
7723a6b
cabf08a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,7 @@ variable "subnetwork" { | |
variable "kubernetes_version" { | ||
type = string | ||
description = "The Kubernetes version of the masters. If set to 'latest' it will pull latest available version in the selected region. The module enforces certain minimum versions to ensure that specific features are available. " | ||
default = "latest" | ||
default = null | ||
} | ||
|
||
variable "node_version" { | ||
|
@@ -77,6 +77,12 @@ variable "node_version" { | |
default = "" | ||
} | ||
|
||
variable "release_channel" { | ||
type = string | ||
description = "(Beta) The release channel of this cluster. Accepted values are `UNSPECIFIED`, `RAPID`, `REGULAR` and `STABLE`. Defaults to `REGULAR`." | ||
default = "REGULAR" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, why pick REGULAR as default over STABLE? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #486 & #451 for additional context. tl;dr Google deems both STABLE & REGULAR to be "production-quality". REGULAR is usually just a few patch revisions behind the latest GKE version. @morgante (Cloud Foundation Toolkit maintainer who works for Google) would prefer safer-cluster use the latest version of GKE. I can see a number of reason for this preference. For instance if you wanted to apply additional CIS Benchmarks listed here: https://cloud.google.com/kubernetes-engine/docs/concepts/cis-benchmarks. That page currently only refers to GKE v1.15, which is also the current REGULAR channel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I looked through it but didn't see any particular reason for picking regular over stable (maybe I missed that section). While I don't have a strong opinion, based on the GKE documentation I think a default setting would be better suited to the more stable option. Users who want newer features etc can switch to regular etc if they would like by overriding. I will leave it up to your judgment, just doing a drive-by here. Thanks for the PR, we're hoping to use it as soon as it's released :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think regular is a reasonable default, because we'd potentially like to add enhanced security features in the future w/o waiting for them to appear in the stable channel. For clusters which require extra stability, they can override. |
||
} | ||
|
||
variable "master_authorized_networks" { | ||
type = list(object({ cidr_block = string, display_name = string })) | ||
description = "List of master authorized networks. If none are provided, disallow external access (except the cluster node IPs, which GKE automatically whitelists)." | ||
|
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.
@morgante Is node_version actually used anywhere in any of the modules? I was grepping through the code last night, but AFAICT, node_version is never consumed by any resource. Am I crazy, or can we just remove node_version, node_version_regional, and node_version_zonal from autogen/main & autogen/safer-cluster?
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.
You're right, it looks like we might have dropped it entirely. Feel free to remove the variable in that case.