feat(operator): enforce RFC 1035 validation for TrainJob name by juniemariam · Pull Request #2767 · kubeflow/trainer

juniemariam · 2025-08-02T05:55:43Z

What this PR does / why we need it:

Adds validation to enforce RFC 1035 compliance for TrainJob.metadata.name
Checks for:
- DNS-1035 format (lowercase, alphanumeric, -, etc.)
- Max length of 63 characters

Why this is needed

This is a follow-up to #2734. Previously, invalid TrainJob names caused JobSet webhook rejections during reconciliation. This PR ensures invalid names are caught earlier at the TrainJob admission webhook level, improving clarity and user experience.

Related Issues

Fixes #2735
Related to #2621, #2734

Tests

Added unit tests for valid, uppercase, and too-long names
Verified via go test ./pkg/webhooks/... and make test
Unit tests, Integration tests passed
E2E test for PyTorch runtime passed
E2E test for OpenMPI runtime failed due to timeout

kannon92

Curious if you should just add this validation to the api?

you could easily do this via a CEL expression and avoid having to add it to the webhooks.

andreyvelich

/ok-to-test

andreyvelich · 2025-08-04T22:17:55Z

/retest

juniemariam · 2025-08-05T01:45:39Z

Thanks for the suggestion @kannon92 — that’s a great point! I followed the pattern from PRs like #2734, where similar validations used webhooks. I'm still getting familiar with the project, so if CEL would be a better fit, I’d really appreciate any guidance and happy to update the PR if needed.

kannon92 · 2025-08-06T03:01:02Z

Thanks for the suggestion @kannon92 — that’s a great point! I followed the pattern from PRs like #2734, where similar validations used webhooks. I'm still getting familiar with the project, so if CEL would be a better fit, I’d really appreciate any guidance and happy to update the PR if needed.

Hmm. I’ll leave that to the maintainers. I like to follow similar patterns as the other code.

cc @tenzen-y @andreyvelich

astefanutti · 2025-08-06T08:21:39Z

@juniemariam you can find some information about CEL validation rules in https://kubernetes.io/blog/2022/09/23/crd-validation-rules-beta/ and the documentation at
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules

The idea is to specify a CEL rule directly in the Golang API using the +kubebuilder:validation:XValidation:rule marker.

There is also the format library that conveniently exposes the dns1035Label function accessible from CEL rules: https://kubernetes.io/docs/reference/using-api/cel/#kubernetes-format-library

coveralls · 2025-08-08T19:19:16Z

Pull Request Test Coverage Report for Build 16923747863

Details

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 47.791%

Totals
Change from base Build 16882514285:	-0.2%
Covered Lines:	941
Relevant Lines:	1969

💛 - Coveralls

juniemariam · 2025-08-09T00:18:58Z

Update: Switched from webhook-based validation to CEL-based validation for TrainJob.metadata.name.

Added CEL validation rules to enforce RFC 1035 compliance
Removed webhook logic for name validation
Regenerated CRDs with make manifests and committed changes

Verified with:

kubectl apply (valid + invalid test cases)
make test (unit tests pass)
also ran integration and E2E test cases (passed)

Let me know if further changes are needed!
/cc @andreyvelich @astefanutti

kannon92 · 2025-08-10T17:36:09Z

Looking really good!

It would be nice to see some integration tests for this logic in this code

astefanutti · 2025-08-11T11:57:11Z

 // +kubebuilder:storageversion
 // +kubebuilder:printcolumn:name="State",type=string,JSONPath=`.status.conditions[-1:].type`
 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
+// +kubebuilder:validation:XValidation:rule="self.metadata.name.matches('^[a-z]([-a-z0-9]*[a-z0-9])?$')", message="metadata.name must match RFC 1035 DNS label format"


would using !format.dns1123Label().validate(self.metadata.name).hasValue() be slightly better?

Thanks for the suggestion @astefanutti ! I initially tried using the format library expression as recommended in the Kubernetes docs (e.g., !format.dns1035Label().validate(self.metadata.name).hasValue()), but it failed to pass the CEL cost estimation and could not be applied due to cost budget errors in the CRD. Hence I changed to regex check

Please let me know if you'd like me to try any other alternatives or if this existing approach works for now.

@juniemariam thanks for the explanation. That looks good to me as it is.

astefanutti · 2025-08-11T11:57:42Z

 			wantError:    nil,
 			wantWarnings: nil,
 		},
-		"invalid trainjob name with uppercase letters": {


I had assumed this test would/could be kept?

@astefanutti Just to clarify—now that the validation is enforced in the CRD using CEL, do we still need to keep the Go/webhook logic and its related unit tests?

I moved the tests out of the webhook layer because the validation logic now lives in the CRD, so the webhook no longer performs this check.

Could you please advise whether it’s better to keep both the Go logic and tests (alongside the CEL rule and integration test), or just rely on CEL + integration tests?

@juniemariam that makes sense. Yes an integration test could be useful.

Signed-off-by: Junie <juniemariam@gmail.com>

…Job name Signed-off-by: Junie <juniemariam@gmail.com>

Signed-off-by: Junie <juniemariam@gmail.com>

astefanutti · 2025-08-13T07:52:20Z

/ok-to-test

astefanutti · 2025-08-13T13:41:32Z

/lgtm

Thanks @juniemariam!

tenzen-y

Thank you!
/lgtm
/approve

google-oss-prow · 2025-08-13T13:43:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [tenzen-y]

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

…ow#2767) * feat(webhook): enforce RFC 1035 validation for TrainJob name Signed-off-by: Junie <juniemariam@gmail.com> * feat(operator): enforce RFC 1035 validation using CEL rules for TrainJob name Signed-off-by: Junie <juniemariam@gmail.com> * test(integration): added integration test for TrainJob name validation Signed-off-by: Junie <juniemariam@gmail.com> --------- Signed-off-by: Junie <juniemariam@gmail.com>

google-oss-prow Bot requested review from astefanutti and jinchihe August 2, 2025 05:55

google-oss-prow Bot added the size/M label Aug 2, 2025

juniemariam changed the title ~~feat(webhook): enforce RFC 1035 validation for TrainJob name~~ feat(operator): enforce RFC 1035 validation for TrainJob name Aug 2, 2025

juniemariam force-pushed the feat/webhook-validate-trainjob-name branch from 6d4897e to 45be2cc Compare August 2, 2025 06:16

kannon92 reviewed Aug 3, 2025

View reviewed changes

andreyvelich reviewed Aug 4, 2025

View reviewed changes

google-oss-prow Bot added the ok-to-test label Aug 4, 2025

google-oss-prow Bot requested a review from andreyvelich August 9, 2025 00:19

astefanutti reviewed Aug 11, 2025

View reviewed changes

juniemariam added 3 commits August 12, 2025 16:21

feat(webhook): enforce RFC 1035 validation for TrainJob name

d913ea6

Signed-off-by: Junie <juniemariam@gmail.com>

feat(operator): enforce RFC 1035 validation using CEL rules for Train…

4f44aa2

…Job name Signed-off-by: Junie <juniemariam@gmail.com>

test(integration): added integration test for TrainJob name validation

8a0b2ca

Signed-off-by: Junie <juniemariam@gmail.com>

juniemariam force-pushed the feat/webhook-validate-trainjob-name branch from e4487b4 to 8a0b2ca Compare August 12, 2025 23:57

google-oss-prow Bot assigned astefanutti Aug 13, 2025

google-oss-prow Bot added the lgtm label Aug 13, 2025

tenzen-y reviewed Aug 13, 2025

View reviewed changes

google-oss-prow Bot assigned tenzen-y Aug 13, 2025

google-oss-prow Bot added the approved label Aug 13, 2025

google-oss-prow Bot merged commit 952f943 into kubeflow:master Aug 13, 2025
20 checks passed

google-oss-prow Bot added this to the v2.1 milestone Aug 13, 2025

Uh oh!

Conversation

juniemariam commented Aug 2, 2025

Why this is needed

Related Issues

Tests

Uh oh!

kannon92 left a comment

Choose a reason for hiding this comment

Uh oh!

andreyvelich left a comment

Choose a reason for hiding this comment

Uh oh!

andreyvelich commented Aug 4, 2025

Uh oh!

juniemariam commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kannon92 commented Aug 6, 2025

Uh oh!

astefanutti commented Aug 6, 2025

Uh oh!

coveralls commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16923747863

Details

💛 - Coveralls

Uh oh!

juniemariam commented Aug 9, 2025

Uh oh!

kannon92 commented Aug 10, 2025

Uh oh!

astefanutti Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

juniemariam Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

astefanutti Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

astefanutti Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

juniemariam Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

astefanutti Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

astefanutti commented Aug 13, 2025

Uh oh!

astefanutti commented Aug 13, 2025

Uh oh!

tenzen-y left a comment

Choose a reason for hiding this comment

Uh oh!

google-oss-prow Bot commented Aug 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

juniemariam commented Aug 5, 2025 •

edited

Loading

coveralls commented Aug 8, 2025 •

edited

Loading