fix apiserver crash caused by nil pointer #58438

carlory · 2018-01-18T06:37:16Z

What this PR does / why we need it:

$ref should be not supported
redefine CRClientGetter interface

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #58427

Release note:

CustomResourceDefinitions: OpenAPI v3 validation schemas containing `$ref`references are no longer permitted (valid references could not be constructed previously because property ids were not permitted either). Before upgrading, ensure CRD definitions do not include those `$ref` fields.

liggitt · 2018-01-18T06:42:29Z

/ok-to-test

carlory · 2018-01-18T06:51:30Z

/retest

nikhita · 2018-01-18T08:21:42Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

@@ -318,6 +305,10 @@ func (v *specStandardValidatorV3) validate(schema *apiextensions.JSONSchemaProps
 		allErrs = append(allErrs, field.Forbidden(fldPath.Child("dependencies"), "dependencies is not supported"))
 	}

+	if schema.Ref != nil {
+		allErrs = append(allErrs, field.Forbidden(fldPath.Child("$ref"), "$ref is not supported"))


just ref here i.e. $ref -> ref

nikhita · 2018-01-18T08:28:13Z

We should cherry pick this PR to 1.8 and 1.9.

nikhita · 2018-01-18T08:30:01Z

/cc @sttts

hzxuzhonghu · 2018-01-18T08:35:43Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -314,12 +314,13 @@ func (r *crdHandler) removeDeadStorage() {

 // GetCustomResourceListerCollectionDeleter returns the ListerCollectionDeleter for
 // the given uid, or nil if one does not exist.
-func (r *crdHandler) GetCustomResourceListerCollectionDeleter(crd *apiextensions.CustomResourceDefinition) finalizer.ListerCollectionDeleter {
+func (r *crdHandler) GetCustomResourceListerCollectionDeleter(crd *apiextensions.CustomResourceDefinition) (finalizer.ListerCollectionDeleter, error) {
 	info, err := r.getOrCreateServingInfoFor(crd)
 	if err != nil {
 		utilruntime.HandleError(err)


If a error returned, there is no need here.

hzxuzhonghu · 2018-01-18T09:22:36Z

@carlory run update bazel

carlory · 2018-01-18T10:20:00Z

@hzxuzhonghu thank you

carlory · 2018-01-18T10:58:58Z

/test pull-kubernetes-node-e2e

carlory · 2018-01-18T10:59:20Z

/test pull-kubernetes-e2e-kops-aw

carlory · 2018-01-18T10:59:57Z

/test pull-kubernetes-verify

carlory · 2018-01-18T11:02:27Z

/test pull-kubernetes-e2e-kops-aw

carlory · 2018-01-18T11:16:00Z

/retest

sttts · 2018-01-22T08:49:39Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

@@ -318,6 +312,10 @@ func (v *specStandardValidatorV3) validate(schema *apiextensions.JSONSchemaProps
 		allErrs = append(allErrs, field.Forbidden(fldPath.Child("dependencies"), "dependencies is not supported"))
 	}

+	if schema.Ref != nil {


Does this mean we forbid schemata like described in https://spacetelescope.github.io/understanding-json-schema/structuring.html with local definitions? Then I do not agree with this PR.

To my understanding the id of a document is only used as a base for relative references. With #/definitions/foo you can reference absolute definitions. The later is completely fine and users might use them. If we forbid that suddenly, we break compatibility.

@sttts We don't support definitions now:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

Lines 313 to 315 in 23226c2

if len(schema.Definitions) != 0 {

allErrs = append(allErrs, field.Forbidden(fldPath.Child("definitions"), "definitions is not supported"))

}

because OpenAPI v3 does not support definitions (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#schemaObject)

Ref is used in OpenAPI v3 like this: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#reference-object.

Alright. Poor OpenAPI (again). Then, without definitions my concerns are gone.

/unhold

thanks for your review

sttts · 2018-01-22T08:56:03Z

/hold

until #58438 (review) is discussed.

sttts · 2018-01-22T09:10:25Z

Shortened and clarified release notes.

sttts · 2018-01-22T12:04:52Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

@@ -318,6 +312,10 @@ func (v *specStandardValidatorV3) validate(schema *apiextensions.JSONSchemaProps
 		allErrs = append(allErrs, field.Forbidden(fldPath.Child("dependencies"), "dependencies is not supported"))
 	}

+	if schema.Ref != nil {
+		allErrs = append(allErrs, field.Forbidden(fldPath.Child("ref"), "ref is not supported"))


Shouldn't this be $ref ?

@sttts

#58438 (review)

@nikhita

Is $ref better?

It should match the json field

@liggitt ok

sttts · 2018-01-22T12:06:06Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

-	if err := apiservervalidation.ConvertToOpenAPITypes(crd, openapiSchema); err != nil {
-		return nil, err
-	}
-	if err := openapispec.ExpandSchema(openapiSchema, nil, nil); err != nil {


are we sure this is only about expanding refs?

@sttts
yes, see this comment

sttts · 2018-01-22T12:09:54Z

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go

-		err := fmt.Errorf("unable to find a custom resource client for %s.%s", crd.Status.AcceptedNames.Plural, crd.Spec.Group)
+	crClient, err := c.crClientGetter.GetCustomResourceListerCollectionDeleter(crd)
+	if err != nil {
+		err = fmt.Errorf("unable to find a custom resource client for %s.%s due to %v", crd.Status.AcceptedNames.Plural, crd.Spec.Group, err)


s/ due to/:/, let's use the normal error style of Go

you are right.

sttts · 2018-01-22T12:11:29Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

+	// if validation passed otherwise, make sure we can actually construct a schema validator from this custom resource validation.
+	if len(allErrs) == 0 {
+		if _, err := apiservervalidation.NewSchemaValidator(customResourceValidation); err != nil {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("customResourceValidation"), "", fmt.Sprintf("error building validator: %v", err)))


wrong child string. I guess we do not need any child at all here.

carlory · 2018-01-23T01:34:46Z

/test pull-kubernetes-e2e-kops-aws

sttts · 2018-01-23T09:55:09Z

/lgtm
/approve

k8s-ci-robot · 2018-01-23T09:55:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, sttts

Associated issue: #58427

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

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

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [sttts]

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

k8s-github-robot · 2018-01-23T09:55:17Z

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@carlory @deads2k @enisoc @lavalamp @sttts @kubernetes/sig-api-machinery-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer.

Pull Request Labels

sig/api-machinery: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

sttts · 2018-01-23T09:55:23Z

Please create a cherry-pick for 1.9.

k8s-github-robot · 2018-01-23T10:34:06Z

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

k8s-github-robot · 2018-01-23T11:18:26Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

hzxuzhonghu · 2018-01-23T11:26:50Z

Please create a cherry-pick for 1.9.

I can do it.

…58438-#58914-origin-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #58438: fix apiserver crash caused by nil pointer and ensure CRD #58914: fix GetCustomResourceListerCollectionDeleter comments Cherry pick of #58438 #58914 on release-1.9. #58438: fix apiserver crash caused by nil pointer and ensure CRD #58914: fix GetCustomResourceListerCollectionDeleter comments ```release-note CustomResourceDefinitions: OpenAPI v3 validation schemas containing `$ref`references are no longer permitted. Before upgrading, ensure CRD definitions do not include those `$ref` fields. ```

k8s-cherrypick-bot · 2018-02-06T07:31:41Z

Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-github-robot assigned enisoc and deads2k Jan 18, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 18, 2018

k8s-ci-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 Jan 18, 2018

nikhita reviewed Jan 18, 2018

View reviewed changes

nikhita mentioned this pull request Jan 18, 2018

The apiserver crash when I delete crd #58427

Closed

k8s-ci-robot requested a review from sttts January 18, 2018 08:30

hzxuzhonghu reviewed Jan 18, 2018

View reviewed changes

timothysc added kind/bug Categorizes issue or PR as related to a bug. cherrypick-candidate labels Jan 18, 2018

timothysc added this to the v1.9 milestone Jan 18, 2018

k8s-github-robot added the milestone/incomplete-labels label Jan 18, 2018

timothysc added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed milestone/incomplete-labels labels Jan 18, 2018

sttts reviewed Jan 22, 2018

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2018

sttts reviewed Jan 22, 2018

View reviewed changes

ref -> $ref

5819a60

k8s-ci-robot assigned sttts Jan 23, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2018

k8s-github-robot merged commit 30d42dc into kubernetes:master Jan 23, 2018

hzxuzhonghu mentioned this pull request Jan 23, 2018

Automated cherry pick of #58438: fix apiserver crash caused by nil pointer and ensure CRD #58688

Closed

hzxuzhonghu mentioned this pull request Feb 6, 2018

Automated cherry pick of #58438: fix apiserver crash caused by nil pointer and ensure CRD #58914: fix GetCustomResourceListerCollectionDeleter comments #59385

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Feb 6, 2018

carlory mentioned this pull request Jan 5, 2021

REQUEST: New membership for carlory kubernetes/org#2402

Closed

6 tasks

	if len(schema.Definitions) != 0 {
	allErrs = append(allErrs, field.Forbidden(fldPath.Child("definitions"), "definitions is not supported"))
	}

fix apiserver crash caused by nil pointer #58438

fix apiserver crash caused by nil pointer #58438

Conversation

carlory commented Jan 18, 2018 • edited by sttts Loading

liggitt commented Jan 18, 2018

carlory commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita commented Jan 18, 2018 • edited Loading

nikhita commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Jan 18, 2018

carlory commented Jan 18, 2018

carlory commented Jan 18, 2018

carlory commented Jan 18, 2018

carlory commented Jan 18, 2018

carlory commented Jan 18, 2018

carlory commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jan 22, 2018

sttts commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlory Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

sttts Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlory commented Jan 23, 2018

sttts commented Jan 23, 2018

k8s-ci-robot commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

sttts commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

k8s-github-robot commented Jan 23, 2018

hzxuzhonghu commented Jan 23, 2018

k8s-cherrypick-bot commented Feb 6, 2018

carlory commented Jan 18, 2018 •

edited by sttts

Loading

nikhita commented Jan 18, 2018 •

edited

Loading

carlory Jan 22, 2018 •

edited

Loading

sttts Jan 22, 2018 •

edited

Loading