-
Notifications
You must be signed in to change notification settings - Fork 493
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
Validation for label keys and values according to Kubernetes specification #3284
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -683,7 +683,9 @@ type GatewayInfrastructure struct { | |||
// | ||||
// +optional | ||||
// +kubebuilder:validation:MaxProperties=8 | ||||
Labels map[AnnotationKey]AnnotationValue `json:"labels,omitempty"` | ||||
// +kubebuilder:validation:XValidation:message="Label keys must be in the form of an optional DNS subdomain prefix followed by a required name segment of up to 63 characters.",rule="self.all(key, key.matches(r\"\"\"^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9]$\"\"\"))" | ||||
// +kubebuilder:validation:XValidation:message="If specified, the label key's prefix must be a DNS subdomain not longer than 253 characters in total.",rule="self.all(key, key.split(\"/\")[0].size() < 253)" | ||||
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 this is irrelevant as the max length of a label name appears to be 63: https://github.com/kubernetes/kubernetes/blob/bd6f29fa2879ff1ef42eb0cc792e45d1e9c52a2f/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go#L102 Would like to make it easier to keep track of this in the future. It would be great to have a comment close to wherever we define the regex that links to the corresponding upstream validation. With that said, we probably shouldn't litter our reference docs with this kind of information, so maybe adding another carve out here to add comments that don't actually make it to the CRD would be useful: gateway-api/pkg/generator/main.go Line 138 in f7d4a87
That last bit can easily be a follow up, so feel free to just create a follow up issue with links to the k8s validation that we're trying to replicate and our corresponding regex validation. 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. Maybe I'm wrong but only the actual name (with out the domain prefix) is restricted to 63 characters which is enforced using the repetition count
(https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) 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 will add a comment pointing to the relevant resources. 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.
Thanks! I created #3306, if you want to just leave a comment on that issue for now, we can go back through and add this to the spec when it's done. 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.
Good catch, I think I misread the upstream validation. |
||||
Labels map[LabelKey]LabelValue `json:"labels,omitempty"` | ||||
|
||||
// Annotations that SHOULD be applied to any resources created in response to this Gateway. | ||||
// | ||||
|
@@ -696,6 +698,8 @@ type GatewayInfrastructure struct { | |||
// | ||||
// +optional | ||||
// +kubebuilder:validation:MaxProperties=8 | ||||
// +kubebuilder:validation:XValidation:message="Annotation keys must be in the form of an optional DNS subdomain prefix followed by a required name segment of up to 63 characters.",rule="self.all(key, key.matches(r\"\"\"^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9]$\"\"\"))" | ||||
// +kubebuilder:validation:XValidation:message="If specified, the annotation key's prefix must be a DNS subdomain not longer than 253 characters in total.",rule="self.all(key, key.split(\"/\")[0].size() < 253)" | ||||
Annotations map[AnnotationKey]AnnotationValue `json:"annotations,omitempty"` | ||||
|
||||
// ParametersRef is a reference to a resource that contains the configuration | ||||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,135 @@ | ||
//go:build experimental | ||
// +build experimental | ||
|
||
/* | ||
Copyright 2023 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package main | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"strings" | ||
"testing" | ||
"time" | ||
|
||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" | ||
) | ||
|
||
func TestGatewayInfrastructureLabels(t *testing.T) { | ||
ctx := context.Background() | ||
baseGateway := gatewayv1.Gateway{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "foo", | ||
Namespace: metav1.NamespaceDefault, | ||
}, | ||
Spec: gatewayv1.GatewaySpec{ | ||
GatewayClassName: "foo", | ||
Listeners: []gatewayv1.Listener{ | ||
{ | ||
Name: gatewayv1.SectionName("http"), | ||
Protocol: gatewayv1.HTTPProtocolType, | ||
Port: gatewayv1.PortNumber(80), | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
testCases := []struct { | ||
name string | ||
wantErrors []string | ||
labels map[gatewayv1.LabelKey]gatewayv1.LabelValue | ||
}{ | ||
{ | ||
name: "valid label keys and values", | ||
labels: map[gatewayv1.LabelKey]gatewayv1.LabelValue{ | ||
"app": "gateway", | ||
"tier": "frontend", | ||
"example": "MyValue", | ||
"example.com": "my.name", | ||
"example.com/path": "123-my-value", | ||
"example.com/path.html": "", | ||
}, | ||
}, | ||
{ | ||
name: "invalid label key with invalid DNS prefix", | ||
labels: map[gatewayv1.LabelKey]gatewayv1.LabelValue{ | ||
"Example.com/key": "value", | ||
}, | ||
wantErrors: []string{"Label keys must be in the form of an optional DNS subdomain prefix followed by a required name segment of up to 63 characters"}, | ||
}, | ||
{ | ||
name: "invalid label key with invalid name", | ||
labels: map[gatewayv1.LabelKey]gatewayv1.LabelValue{ | ||
"key~@@@": "value", | ||
}, | ||
wantErrors: []string{"Label keys must be in the form of an optional DNS subdomain prefix followed by a required name segment of up to 63 characters"}, | ||
}, | ||
{ | ||
name: "invalid label key with DNS prefix too long", | ||
labels: map[gatewayv1.LabelKey]gatewayv1.LabelValue{ | ||
gatewayv1.LabelKey(strings.Repeat("a", 254) + "/key"): "value", | ||
}, | ||
wantErrors: []string{"If specified, the label key's prefix must be a DNS subdomain not longer than 253 characters in total."}, | ||
}, | ||
{ | ||
name: "invalid label key with name too long", | ||
labels: map[gatewayv1.LabelKey]gatewayv1.LabelValue{ | ||
gatewayv1.LabelKey(strings.Repeat("a", 64)): "value", | ||
}, | ||
wantErrors: []string{"Label keys must be in the form of an optional DNS subdomain prefix followed by a required name segment of up to 63 characters."}, | ||
}, | ||
{ | ||
name: "invalid label value with too many characters", | ||
labels: map[gatewayv1.LabelKey]gatewayv1.LabelValue{ | ||
"key": gatewayv1.LabelValue(strings.Repeat("a", 64)), | ||
}, | ||
wantErrors: []string{"Too long: may not be longer than 63"}, | ||
}, | ||
{ | ||
name: "invalid label value with invalid characters", | ||
labels: map[gatewayv1.LabelKey]gatewayv1.LabelValue{ | ||
"key": "v a l u e", | ||
}, | ||
wantErrors: []string{"spec.infrastructure.labels.key in body should match '^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$'"}, | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
gw := baseGateway.DeepCopy() | ||
gw.Name = fmt.Sprintf("foo-%v", time.Now().UnixNano()) | ||
|
||
gw.Spec.Infrastructure = &gatewayv1.GatewayInfrastructure{Labels: tc.labels} | ||
err := k8sClient.Create(ctx, gw) | ||
|
||
if (len(tc.wantErrors) != 0) != (err != nil) { | ||
t.Fatalf("Unexpected response while creating Gateway; got err=\n%v\n;want error=%v", err, tc.wantErrors != nil) | ||
} | ||
|
||
var missingErrorStrings []string | ||
for _, wantError := range tc.wantErrors { | ||
if !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(wantError)) { | ||
missingErrorStrings = append(missingErrorStrings, wantError) | ||
} | ||
} | ||
if len(missingErrorStrings) != 0 { | ||
t.Errorf("Unexpected response while creating Gateway; got err=\n%v\n;missing strings within error=%q", err, missingErrorStrings) | ||
} | ||
}) | ||
} | ||
} |
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.
The patterns, validation specified for the map's type of the key are ignored in the CRD generation therefore a CEL validation is required.
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.
It looks like there are new
pattern
fields in the generated CRD, are you sure this didn't work? Although it's nice to have useful messages like this, I'm worried that the primary regexes on theLabelKey
andLabelValue
will gradually drift from these kinds of one-off CEL validations.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.
I think the
pattern
is related to the newLabelValue
validation annotation. I didn't figure out a way to validate the key with a pattern without using CEL. Let me check the capabilities of the OpenAPI schema and kubebuilder again.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.
Thanks! The most important bit is that our validation works. Don't want to unnecessarily delay this. Feel free to file a follow up issue to look into what's possible here.