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

Limit 52-character cronjob name validation to create #52967

Merged
merged 1 commit into from Sep 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/apis/batch/validation/validation.go
Expand Up @@ -176,6 +176,14 @@ func ValidateCronJob(scheduledJob *batch.CronJob) field.ErrorList {
return allErrs
}

func ValidateCronJobUpdate(job, oldJob *batch.CronJob) field.ErrorList {
allErrs := apivalidation.ValidateObjectMetaUpdate(&job.ObjectMeta, &oldJob.ObjectMeta, field.NewPath("metadata"))
allErrs = append(allErrs, ValidateCronJobSpec(&job.Spec, field.NewPath("spec"))...)
// skip the 52-character name validation limit on update validation
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to sunset this skip in a future release?

Copy link
Member Author

Choose a reason for hiding this comment

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

could, although the migrate strategy is pretty deployment-specific (ranging from "not at all" to "every release" to "tools available but not auto-run"), so it's unclear when we'd know they'd all been cleared out

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully the resolution to #52185 is "every release", but I agree with you re the status quo.

Copy link
Member Author

Choose a reason for hiding this comment

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

hope is a prereq for all my upgrades

Copy link
Contributor

Choose a reason for hiding this comment

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

@liggitt how long do we want to wait before "fixing" the update part entirely? Is 1.10 a good time frame or 1.11?

// to allow old cronjobs with names > 52 chars to be updated/deleted
return allErrs
}

func ValidateCronJobSpec(spec *batch.CronJobSpec, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

Expand Down
26 changes: 26 additions & 0 deletions pkg/apis/batch/validation/validation_test.go
Expand Up @@ -347,6 +347,14 @@ func TestValidateCronJob(t *testing.T) {
if errs := ValidateCronJob(&v); len(errs) != 0 {
t.Errorf("expected success for %s: %v", k, errs)
}

// Update validation should pass same success cases
// copy to avoid polluting the testcase object, set a resourceVersion to allow validating update, and test a no-op update
v = *v.DeepCopy()
v.ResourceVersion = "1"
if errs := ValidateCronJobUpdate(&v, &v); len(errs) != 0 {
t.Errorf("expected success for %s: %v", k, errs)
}
}

negative := int32(-1)
Expand Down Expand Up @@ -588,6 +596,24 @@ func TestValidateCronJob(t *testing.T) {
t.Errorf("unexpected error: %v, expected: %s", err, k)
}
}

// Update validation should fail all failure cases other than the 52 character name limit
// copy to avoid polluting the testcase object, set a resourceVersion to allow validating update, and test a no-op update
v = *v.DeepCopy()
v.ResourceVersion = "1"
errs = ValidateCronJobUpdate(&v, &v)
if len(errs) == 0 {
if k == "metadata.name: must be no more than 52 characters" {
continue
}
t.Errorf("expected failure for %s", k)
} else {
s := strings.Split(k, ":")
err := errs[0]
if err.Field != s[0] || !strings.Contains(err.Error(), s[1]) {
t.Errorf("unexpected error: %v, expected: %s", err, k)
}
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/batch/cronjob/strategy.go
Expand Up @@ -87,7 +87,7 @@ func (cronJobStrategy) AllowCreateOnUpdate() bool {

// ValidateUpdate is the default update validation for an end user.
func (cronJobStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList {
return validation.ValidateCronJob(obj.(*batch.CronJob))
return validation.ValidateCronJobUpdate(obj.(*batch.CronJob), old.(*batch.CronJob))
}

type cronJobStatusStrategy struct {
Expand Down