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.

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

Merged

Conversation

snorwin
Copy link
Contributor

@snorwin snorwin commented Aug 20, 2024

What type of PR is this?
/kind feature

What this PR does / why we need it:
(see the discussion in #3197)

Which issue(s) this PR fixes:
Fixes #3197

Does this PR introduce a user-facing change?:

Label keys and values are validated according to the Kubernetes specification

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 20, 2024
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 20, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @snorwin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@@ -683,7 +683,8 @@ 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.

@keithmattix
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 20, 2024
@snorwin snorwin force-pushed the infrastructure-label-validation branch from b1473ec to 2ec4cb8 Compare August 20, 2024 09:20
…ation

Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
@snorwin snorwin force-pushed the infrastructure-label-validation branch from 2ec4cb8 to 28e6f1e Compare August 20, 2024 09:21
@snorwin
Copy link
Contributor Author

snorwin commented Aug 20, 2024

/retest

@youngnick
Copy link
Contributor

Nice work, this LGTM.

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2024
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
@snorwin
Copy link
Contributor Author

snorwin commented Aug 21, 2024

I ran further tests and found that the length of the DNS subdomain prefix of the key is not validated too, so I added another CEL validation for it.

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @snorwin!

@@ -683,7 +683,8 @@ 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
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.

@@ -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)"
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:

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.

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Given the complexity with some of these regexes, we should add tests here validating these.

@keithmattix
Copy link
Contributor

@snorwin Let me know if you're able/wanting to tackle the tests; happy to help if needed!

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 28, 2024
@snorwin
Copy link
Contributor Author

snorwin commented Aug 28, 2024

@keithmattix I have added some tests, maybe you can take a look and let me know if it looks good, I would then create similar tests for the annotations.

@snorwin
Copy link
Contributor Author

snorwin commented Aug 28, 2024

It seems that the tests run against the standard CRDs, is there a way to write tests for experimental functions as well?

@keithmattix
Copy link
Contributor

#3272 moves these fields to standard so maybe pull in those changes?

@keithmattix
Copy link
Contributor

@snorwin you could also try this approach which is per-channel. You create test files instead of Go test cases

@robscott
Copy link
Member

@snorwin You'd need a separate file with an experimental build tag to write CEL tests that target experimental channel. Here's an example:

//go:build experimental
// +build experimental

@shaneutt shaneutt dismissed their stale review August 29, 2024 17:41

didn't mean to block

@snorwin snorwin force-pushed the infrastructure-label-validation branch from f0c3004 to 70f449f Compare September 2, 2024 07:25
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
@snorwin snorwin force-pushed the infrastructure-label-validation branch from 70f449f to 3b35eef Compare September 2, 2024 07:28
@snorwin snorwin requested a review from shaneutt September 2, 2024 07:29
@keithmattix
Copy link
Contributor

Thanks @snorwin! Tests look good to me
/approve

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/approve

but I will defer to @mlavacca, @robscott or @youngnick to LGTM

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: keithmattix, shaneutt, snorwin, youngnick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @snorwin!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 3, 2024
@k8s-ci-robot k8s-ci-robot merged commit 8af72eb into kubernetes-sigs:main Sep 3, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Labels specified in the Gatway Infrastructure (i.e., .spec.infrastructure.labels) might be invalid
6 participants