Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Validation for label keys and values according to Kubernetes specification #3284

Merged
merged 3 commits into from
Sep 3, 2024
Merged

Validation for label keys and values according to Kubernetes specification #3284

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28e6f1e
Validation for label keys and values according to Kubernetes specific…
snorwin Aug 20, 2024
cdc8e22
Validate prefix length of DNS subdomain in label and anootaion keys
snorwin Aug 21, 2024
3b35eef
add tests for label key and value validation
snorwin Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions apis/applyconfiguration/apis/v1/gatewayinfrastructure.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion apis/v1/gateway_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]$\"\"\"))"
Copy link
Contributor Author

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.

Copy link
Member

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 the LabelKey and LabelValue will gradually drift from these kinds of one-off CEL validations.

Copy link
Contributor Author

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 new LabelValue 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.

Copy link
Member

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.

// +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)"
Copy link
Member

Choose a reason for hiding this comment

The 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
https://github.com/kubernetes/kubernetes/blob/bd6f29fa2879ff1ef42eb0cc792e45d1e9c52a2f/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L64

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

func gatewayTweaks(channel string, props map[string]apiext.JSONSchemaProps) map[string]apiext.JSONSchemaProps {

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {0,61} in the regex pattern. However the length of of the DNS prefix should be not longer than 253.

Valid label keys have two segments: an optional prefix and name, separated by a slash (/). The name segment is required and must be 63 characters or less, beginning and ending with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_), dots (.), and alphanumerics between. The prefix is optional. If specified, the prefix must be a DNS subdomain: a series of DNS labels separated by dots (.), not longer than 253 characters in total, followed by a slash (/).

(https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a comment pointing to the relevant resources.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a comment pointing to the relevant resources.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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 {0,61} in the regex pattern. However the length of of the DNS prefix should be not longer than 253.

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.
//
Expand All @@ -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
Expand Down
41 changes: 40 additions & 1 deletion apis/v1/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,7 @@ type GatewayController string
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:Pattern=`^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]/?)*$`
// +kubebuilder:validation:Pattern=`^([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]$`
type AnnotationKey string

// AnnotationValue is the value of an annotation in Gateway API. This is used
Expand All @@ -678,6 +678,45 @@ type AnnotationKey string
// +kubebuilder:validation:MaxLength=4096
type AnnotationValue string

// LabelKey is the key of a label in the Gateway API. This is used for validation
// of maps such as Gateway infrastructure labels. This matches the Kubernetes
// "qualified name" validation that is used for labels.
//
// Valid values include:
//
// * example
// * example.com
// * example.com/path
// * example.com/path.html
//
// Invalid values include:
//
// * example~ - "~" is an invalid character
// * example.com. - can not start or end with "."
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:Pattern=`^([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]$`
type LabelKey string

// LabelValue is the value of a label in the Gateway API. This is used for validation
// of maps such as Gateway infrastructure labels. This matches the Kubernetes
// label validation rules:
// * must be 63 characters or less (can be empty),
// * unless empty, must begin and end with an alphanumeric character ([a-z0-9A-Z]),
// * could contain dashes (-), underscores (_), dots (.), and alphanumerics between.
//
// Valid values include:
//
// * MyValue
// * my.name
// * 123-my-value
//
// +kubebuilder:validation:MinLength=0
// +kubebuilder:validation:MaxLength=63
// +kubebuilder:validation:Pattern=`^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$`
type LabelValue string

// AddressType defines how a network address is represented as a text string.
// This may take two possible forms:
//
Expand Down
2 changes: 1 addition & 1 deletion apis/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

72 changes: 62 additions & 10 deletions config/crd/experimental/gateway.networking.k8s.io_gateways.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.