Unify initializer name validation #51283

caesarxuchao · 2017-08-24T17:29:52Z

Unify the validation rules on initializer names. Fix #51843.

Action required: validation rule on metadata.initializers.pending[x].name is tightened. The initializer name needs to contain at least three segments separated by dots. If you create objects with pending initializers, (i.e., not relying on apiserver adding pending initializers according to initializerconfiguration), you need to update the initializer name in existing objects and in configuration files to comply to the new validation rule.

caesarxuchao · 2017-08-24T17:30:04Z

/release-note-none
/sig api-machinery

caesarxuchao · 2017-08-25T00:50:41Z

/retest

deads2k · 2017-08-25T01:51:09Z

staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go

-		for _, msg := range validation.IsQualifiedName(initializer.Name) {
-			allErrs = append(allErrs, field.Invalid(fldPath.Child("pending").Index(i), initializer.Name, msg))
-		}
+		allErrs = append(allErrs, ValidateInitializerName(fldPath.Child("pending").Index(i).Child("name"), initializer.Name)...)


Was it possible for me to submit an object that contained a pending initializer before (not rely on admission to set it)? If so, an action-required release note is probably in order, since tightening validation may accidentally block deletion (finalizers), right?

It is an alpha feature, but yes, worth at least reminding people that alpha features can wedge your cluster.

xiang90 · 2017-08-25T06:43:27Z

staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go

+func ValidateInitializerName(fldPath *field.Path, name string) field.ErrorList {
+	var allErrors field.ErrorList
+	if len(name) == 0 {
+		allErrors = append(allErrors, field.Required(fldPath, ""))


early return here?

smarterclayton · 2017-08-25T06:43:35Z

staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go

+	if errs := validationutil.IsDNS1123Subdomain(name); len(errs) > 0 {
+		allErrors = append(allErrors, field.Invalid(fldPath, name, strings.Join(errs, ",")))
+	}
+	if len(strings.Split(name, ".")) < 3 {


two dots is hard to translate - I think most people would expect to see three or more segments or parts

changed to "should be a domain with at least three segments separated by dots"

xiang90 · 2017-08-25T06:43:40Z

staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go

+		allErrors = append(allErrors, field.Required(fldPath, ""))
+	}
+	if errs := validationutil.IsDNS1123Subdomain(name); len(errs) > 0 {
+		allErrors = append(allErrors, field.Invalid(fldPath, name, strings.Join(errs, ",")))


early return?

caesarxuchao · 2017-08-25T07:25:43Z

Ack on all the comments. Will address them tmr.

caesarxuchao · 2017-08-25T18:49:50Z

Addressed the comments. Added release-note-action-required. PTAL.

caesarxuchao · 2017-08-25T20:40:09Z

staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go

@@ -67,6 +69,21 @@ func IsQualifiedName(value string) []string {
 	return errs
 }

+// IsFullyQualifiedName checks if the name is fully qualified.
+func IsFullyQualifiedName(fldPath *field.Path, name string) field.ErrorList {


I renamed the function as it's used by the admission webhook validation as well.

BenTheElder · 2017-08-28T22:56:31Z

/test pull-kubernetes-e2e-gce-gpu
(this is a CI issue, working on it!)

caesarxuchao · 2017-08-29T01:10:42Z

/retest

deads2k · 2017-08-30T16:59:32Z

/lgtm

smarterclayton · 2017-08-31T00:04:49Z

/approve

caesarxuchao · 2017-08-31T07:39:18Z

/retest

caesarxuchao · 2017-09-02T00:25:31Z

Created and linked to an issue. Manually added the "approved" label given by Clayton.

smarterclayton · 2017-09-02T00:39:18Z

/approve no-issue Just in case

k8s-github-robot · 2017-09-02T00:40:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, deads2k, smarterclayton

Associated issue: 51843

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/apis/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [caesarxuchao,deads2k,smarterclayton]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,smarterclayton]
~~test/OWNERS~~ [deads2k,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

caesarxuchao · 2017-09-02T06:06:42Z

/retest

fejta-bot · 2017-09-02T16:00:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-09-03T01:48:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-09-03T03:14:17Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-03T03:35:21Z

Automatic merge from submit-queue (batch tested with PRs 51583, 51283, 51374, 51690, 51716)

k8s-ci-robot · 2017-09-03T04:33:21Z

@caesarxuchao: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`85ee09e`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

caesarxuchao self-assigned this Aug 24, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 24, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 24, 2017

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 24, 2017

caesarxuchao mentioned this pull request Aug 24, 2017

Fix mergekey of initializers; Repair invalid update of initializers #51082

Merged

caesarxuchao force-pushed the fix-initializer-validate branch from f43635b to 3230d7f Compare August 24, 2017 19:52

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 24, 2017

caesarxuchao changed the title ~~[WIP] Unify initializer name validation~~ Unify initializer name validation Aug 24, 2017

caesarxuchao force-pushed the fix-initializer-validate branch from 3230d7f to 538e4fb Compare August 24, 2017 19:53

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 24, 2017

caesarxuchao force-pushed the fix-initializer-validate branch 2 times, most recently from 83c66c5 to 05a3241 Compare August 24, 2017 21:43

caesarxuchao assigned smarterclayton and deads2k and unassigned caesarxuchao Aug 24, 2017

deads2k reviewed Aug 25, 2017

View reviewed changes

xiang90 reviewed Aug 25, 2017

View reviewed changes

smarterclayton reviewed Aug 25, 2017

View reviewed changes

xiang90 reviewed Aug 25, 2017

View reviewed changes

caesarxuchao added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 25, 2017

caesarxuchao force-pushed the fix-initializer-validate branch from 05a3241 to cf0f87f Compare August 25, 2017 18:44

caesarxuchao force-pushed the fix-initializer-validate branch from cf0f87f to cd6c7c5 Compare August 25, 2017 20:39

caesarxuchao commented Aug 25, 2017

View reviewed changes

caesarxuchao force-pushed the fix-initializer-validate branch from cd6c7c5 to 7f55383 Compare August 25, 2017 20:48

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, 2017

Chao Xu added 2 commits August 28, 2017 16:17

unify the validation rules on initializer name

c33de9f

update initializer names to valid ones in tests

85ee09e

caesarxuchao force-pushed the fix-initializer-validate branch from e54ef61 to 85ee09e Compare August 28, 2017 23:18

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

caesarxuchao added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2017

caesarxuchao mentioned this pull request Sep 2, 2017

Initializerconfiguration and metadata.initializers have different rules for initializer name #51843

Closed

abgworrall modified the milestone: v1.8 Sep 2, 2017

k8s-github-robot merged commit 12f96e2 into kubernetes:master Sep 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify initializer name validation #51283

Unify initializer name validation #51283

caesarxuchao commented Aug 24, 2017 •

edited

Loading

caesarxuchao commented Aug 24, 2017

caesarxuchao commented Aug 25, 2017

deads2k Aug 25, 2017

smarterclayton Aug 25, 2017

xiang90 Aug 25, 2017

smarterclayton Aug 25, 2017

caesarxuchao Aug 25, 2017

xiang90 Aug 25, 2017

caesarxuchao commented Aug 25, 2017

caesarxuchao commented Aug 25, 2017

caesarxuchao Aug 25, 2017

BenTheElder commented Aug 28, 2017

caesarxuchao commented Aug 29, 2017

deads2k commented Aug 30, 2017

smarterclayton commented Aug 31, 2017

caesarxuchao commented Aug 31, 2017

caesarxuchao commented Sep 2, 2017

smarterclayton commented Sep 2, 2017 via email

k8s-github-robot commented Sep 2, 2017

caesarxuchao commented Sep 2, 2017

fejta-bot commented Sep 2, 2017

fejta-bot commented Sep 3, 2017

k8s-github-robot commented Sep 3, 2017

k8s-github-robot commented Sep 3, 2017

k8s-ci-robot commented Sep 3, 2017

Unify initializer name validation #51283

Unify initializer name validation #51283

Conversation

caesarxuchao commented Aug 24, 2017 • edited Loading

caesarxuchao commented Aug 24, 2017

caesarxuchao commented Aug 25, 2017

deads2k Aug 25, 2017

Choose a reason for hiding this comment

smarterclayton Aug 25, 2017

Choose a reason for hiding this comment

xiang90 Aug 25, 2017

Choose a reason for hiding this comment

smarterclayton Aug 25, 2017

Choose a reason for hiding this comment

caesarxuchao Aug 25, 2017

Choose a reason for hiding this comment

xiang90 Aug 25, 2017

Choose a reason for hiding this comment

caesarxuchao commented Aug 25, 2017

caesarxuchao commented Aug 25, 2017

caesarxuchao Aug 25, 2017

Choose a reason for hiding this comment

BenTheElder commented Aug 28, 2017

caesarxuchao commented Aug 29, 2017

deads2k commented Aug 30, 2017

smarterclayton commented Aug 31, 2017

caesarxuchao commented Aug 31, 2017

caesarxuchao commented Sep 2, 2017

smarterclayton commented Sep 2, 2017 via email

k8s-github-robot commented Sep 2, 2017

caesarxuchao commented Sep 2, 2017

fejta-bot commented Sep 2, 2017

fejta-bot commented Sep 3, 2017

k8s-github-robot commented Sep 3, 2017

k8s-github-robot commented Sep 3, 2017

k8s-ci-robot commented Sep 3, 2017

caesarxuchao commented Aug 24, 2017 •

edited

Loading