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

Validate the generated job names will be dns 1035 compliant #284

Merged
merged 6 commits into from
Sep 20, 2023
Merged

Validate the generated job names will be dns 1035 compliant #284

merged 6 commits into from
Sep 20, 2023

Conversation

danielvegamyhre
Copy link
Contributor

Fixes #283

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 31, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielvegamyhre

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 31, 2023
@danielvegamyhre danielvegamyhre force-pushed the validation branch 2 times, most recently from cbd9860 to e0e9a54 Compare August 31, 2023 20:39
@danielvegamyhre
Copy link
Contributor Author

/retest

@danielvegamyhre
Copy link
Contributor Author

cc @kannon92 am I correct this is considered a breaking change?

@kannon92
Copy link
Contributor

kannon92 commented Sep 1, 2023

cc @kannon92 am I correct this is considered a breaking change?

Yea I think making validation more strict would be considered a breaking change. OTOH I think having validation to catch errors is a much better UX than failing in reconcile and having someone edit the yaml.

So I don’t think this is being a breaking change should hold it. I was bringing up that in k/k this would be a difficult change to push through. Usually you would want to guarantee that this wouldn’t break any existing users who may have relied on this (prob unlikely).

I’d like to hear what @ahg-g thinks of changes like this.

kannon92
kannon92 reviewed Sep 1, 2023
View reviewed changes
api/jobset/v1alpha2/jobset_webhook.go Outdated
@@ -101,6 +101,12 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
if int64(parallelism)*int64(rjob.Replicas) > math.MaxInt32 {
allErrs = append(allErrs, fmt.Errorf("the product of replicas and parallelism must not exceed %d for replicatedJob '%s'", math.MaxInt32, rjob.Name))
}
// Check that the generated job names for this replicated job will be DNS 1035 compliant.
// Use job index 0 as the test example.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this comment concerning. I guess you are just validating a single job (as all others will fail since replica won't change the dns issue. But the comment test example and testJobName make it seem like this is leftover remnants of testing.

Maybe we can use rjob.Replicas - 1 or something like that. At least we pick the worst case to fail. I wonder if it would ever be possible that -0 case would pass but -(replicas - 1) would fail. I imagine that replicas-1 passing would mean that everything else should pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! The length of the job name with the largest job index will be >= the longest job name in the replicatedJob. I updated the test to use that instead of job index 0, since there are certain cases where job index 0 length is less than 63 chars but job index replicas-1 is not.

ahg-g
ahg-g reviewed Sep 7, 2023
View reviewed changes
api/jobset/v1alpha2/jobset_webhook.go Outdated
@@ -101,6 +101,12 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) {
if int64(parallelism)*int64(rjob.Replicas) > math.MaxInt32 {
allErrs = append(allErrs, fmt.Errorf("the product of replicas and parallelism must not exceed %d for replicatedJob '%s'", math.MaxInt32, rjob.Name))
}
// Check that the generated job names for this replicated job will be DNS 1035 compliant.
// Use job index 0 as the test example.
testJobName := fmt.Sprintf("%s-%s-%d", js.Name, rjob.Name, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a shared func between the webhook and the controller for name generation just so when changes are made, they get reflected in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok in order to avoid an import/dependency cycle, I had to create a shared util function. However, it didn't seem to fit into any of our existing util pacakges (util/test, util/cert, util/collections) so I made a new one: pkg/util/names. I tried to add GenSubdomain() into it as well, but it results in an import cycle as well, and unlike GenJobName there's no easy fix, so I left it out.

Let me know if you have any thoughts on this.

@kannon92
Copy link
Contributor

I think this looks good to me but I'll leave lgtm for @ahg-g

@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 Sep 13, 2023
kannon92
kannon92 reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@kannon92 kannon92 left a comment

Choose a reason for hiding this comment

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

One comment https://github.com/kubernetes-sigs/jobset/pull/284/files#r1313274666

/lgtm
/hold

if you want to ignore the comment

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2023
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 20, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 20, 2023
@kannon92
Copy link
Contributor

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 20, 2023
@k8s-ci-robot k8s-ci-robot merged commit ebe9613 into kubernetes-sigs:main Sep 20, 2023
9 checks passed
@danielvegamyhre danielvegamyhre mentioned this pull request Dec 12, 2023
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kannon92 kannon92 kannon92 left review comments

@ahg-g ahg-g ahg-g left review comments

Assignees

@kannon92 kannon92

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add validation to JobSet name to ensure child job names derived from it have no invalid chars
4 participants
@danielvegamyhre @k8s-ci-robot @kannon92 @ahg-g