Add validation for k8s label length restrictions #261
Conversation
aidy
force-pushed
the
label_length_error
branch
from
47a43eb
to
ba8ceca
Compare
aidy
force-pushed
the
label_length_error
branch
5 times, most recently
from
672b68e
to
ba52eca
Compare
aidy
requested review from
a team,
ettiee and
takanabe
and removed request for
a team
aidy
force-pushed
the
label_length_error
branch
2 times, most recently
from
5ca6b9a
to
4eb049a
Compare
|
📝 upgrading the terraform version requirement to >=0.14 is a breaking change for users using <0.14, and as this is a bug fix we'd want to patch this back to 3 previous minor versions of the module. Looks like you needed to use variable validation for the fix so you had to upgrade to >=0.13. We should consider if we want to introduce breaking changes in patch releases |
modules/asg_node_group/main.tf
Outdated
| var.gpu ? { "nvidia.com/gpu" = "true" } : {}, | ||
| var.labels, | ||
| ) | ||
| } | ||
|
|
||
| data "assert_test" "node_group_label" { | ||
| test = length(local.node_group_label) < 64 | ||
| throw = "node-group.k8s.cookpad.com/name label must be 63 characters or less" |
There was a problem hiding this comment.
This error message might not be intuitive to some users - maybe give some indication of what variable to change to not hit the error?
E.g. update name variable
maybe throw contents of node_group_label too so they can inspect
There was a problem hiding this comment.
Yeah, I had some conflict about what to do here - the general style of tf errors seems to be to just state the error, rather than suggested resolutions.
I think the tf philosophy is that it's not supposed to be a black box, and it's intended to be inspected, but you're probably right, and it's better to be more verbose, even if that's not in keeping with the style of other errors.
| @@ -17,6 +17,11 @@ variable "name" { | |||
| type = string | |||
| default = "" | |||
| description = "An optional identifier for this node group" | |||
|
|
|||
| validation { | |||
There was a problem hiding this comment.
Maybe covered by https://github.com/cookpad/terraform-aws-eks/pull/261/files#r750337723 already (and vice versa). This validation is more intuitive to a user
There was a problem hiding this comment.
The vice versa doesn't apply, as if this isn't present, the label is calculated.
Although this is redundant, it seemed more user friendly to duplicate it here. Also, I think the native tf validation fail is nicer.
Discussed in person - this isn't really a bug fix to patch back to previous releases. If users hit this they won't have a successfully deployed cluster, so nobody would be patching this into a deployed cluster. It's an improvement we'll include in 1.20.0 |
aidy
force-pushed
the
label_length_error
branch
2 times, most recently
from
0d86974
to
8990017
Compare
The version of terratest we're using has some incompatibilities with the output of later versions of terraform. Upgrade to the latest version in preparation for tf upgrade.
Pin version of cluster/environment so that we don't break other tests, as we persist that state file.
kubernetes labels have a 63 character limitation: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set As node-group label names may be calculated, they could exceed this length, which results in bottlerocket configuration errors.
As the node-group name variable is used as a label for kubernetes, add validation that it does not exceed the character limitation.
k8s labels should be 63 characters or less, or nodes may fail to launch due to misconfiguration.
aidy
force-pushed
the
label_length_error
branch
from
8990017
to
16c346b
Compare
Fix UPGRADING.md with breaking change from #261
connects #242