Adds labels validation to restrict namespaces #774
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 7a2da21 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/617b04ab89a58800076f0d3b |
| @@ -98,10 +98,30 @@ func (c *Constraints) validateLabels() (errs *apis.FieldError) { | |||
| if known, ok := WellKnownLabels[key]; ok && !functional.ContainsString(known, value) { | |||
| errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not in %s", value, known), fmt.Sprintf("labels[%s]", key))) | |||
| } | |||
| if _, ok := WellKnownLabels[key]; !ok && isRestrictedLabelPrefix(key) { | |||
There was a problem hiding this comment.
I know we've gone back and forth, but you could call this "IsRestrictedLabelDomain", since these are domains at the end of the day.
| return false | ||
| } | ||
|
|
||
| func getLabelPrefix(key string) string { |
There was a problem hiding this comment.
check out url.parse, url.Hostname()
There was a problem hiding this comment.
I'm not sure I follow how that helps. I still need to split the string by / regardless in this method, right?
The suffix check I need to do too because url.Hostname() gives me the whole path and not just the first subdomain which is what I'm looking for. Also, looks like url.Hostname() doesn't really work great when there's no scheme preceding the prefix like in our case.
| "kubernetes.io", | ||
| "k8s.io", | ||
| "karpenter.sh", | ||
| "k8s.aws", |
There was a problem hiding this comment.
Can you register this in the aws/apis/v1alpha1/register.go instead of putting in vendor neutral code?
There was a problem hiding this comment.
Good point! I'll move k8s.aws into aws/apis/v1alpha1/register.go but leave the others here since the rest should apply to the other cloud providers too.
| @@ -190,7 +190,7 @@ metadata: | |||
| name: default | |||
| spec: | |||
| requirements: | |||
| - key: node.k8s.io/capacity-type | |||
| - key: node.k8s.aws/capacity-type | |||
1. Issue, if available:
#728
2. Description of changes:
k8s.ioorkubernetes.ionamespace. See this for more detail on why this was changed upstream 1.16+.karpenter.shandk8s.awsnamespaces.The upstream checks are a little more nuanced. See this and this. The checks I'm enforcing here are stricter.
Testing -
k8s.ioandkubernetes.iowere being rejected. Also made sure that well known labels as per karpenter likenode.kubernetes.io/instance-typewere still accepted and honored.3. Does this change impact docs?
This should be called out in our docs as well.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.