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

fix apiserver crash caused by nil pointer #58438

Merged
merged 2 commits into from
Jan 23, 2018
Merged

fix apiserver crash caused by nil pointer #58438

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b8d522
fix apiserver crash caused by nil pointer and ensure CRD schema
carlory Jan 18, 2018
5819a60
ref -> $ref
carlory Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ go_library(
srcs = ["validation.go"],
importpath = "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation",
deps = [
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/apiserver/validation:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/features:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/validation:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library",
Expand Down
24 changes: 11 additions & 13 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,13 @@ import (
"fmt"
"strings"

"github.com/go-openapi/spec"

genericvalidation "k8s.io/apimachinery/pkg/api/validation"
validationutil "k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
utilfeature "k8s.io/apiserver/pkg/util/feature"

"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
apiservervalidation "k8s.io/apiextensions-apiserver/pkg/apiserver/validation"
apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features"
)

Expand Down Expand Up @@ -188,6 +187,12 @@ func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiext
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(customResourceValidation.OpenAPIV3Schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema)...)
}

// 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)))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}
return allErrs
}

Expand All @@ -213,17 +218,6 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), ssv)...)
}

if schema.Ref != nil {
openapiRef, err := spec.NewRef(*schema.Ref)
if err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("ref"), *schema.Ref, err.Error()))
}

if !openapiRef.IsValidURI() {
allErrs = append(allErrs, field.Invalid(fldPath.Child("ref"), *schema.Ref, "ref does not point to a valid URI"))
}
}

if schema.AdditionalItems != nil {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.AdditionalItems.Schema, fldPath.Child("additionalItems"), ssv)...)
}
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

/unhold

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for your review

allErrs = append(allErrs, field.Forbidden(fldPath.Child("ref"), "ref is not supported"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be $ref ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sttts

#58438 (review)

@nikhita

Is $ref better?

Copy link
Member

Choose a reason for hiding this comment

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

It should match the json field

Copy link
Member Author

Choose a reason for hiding this comment

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

@liggitt ok

}

if schema.Type == "null" {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("type"), "type cannot be set to null"))
}
Expand Down
3 changes: 0 additions & 3 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ go_library(
],
importpath = "k8s.io/apiextensions-apiserver/pkg/apiserver",
deps = [
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/github.com/go-openapi/strfmt:go_default_library",
"//vendor/github.com/go-openapi/validate:go_default_library",
"//vendor/github.com/golang/glog:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/install:go_default_library",
Expand Down
20 changes: 6 additions & 14 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ import (
"sync/atomic"
"time"

openapispec "github.com/go-openapi/spec"
"github.com/go-openapi/strfmt"
"github.com/go-openapi/validate"
"github.com/golang/glog"

apiequality "k8s.io/apimachinery/pkg/api/equality"
Expand Down Expand Up @@ -313,13 +310,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 {
// the given uid, or nil if an error occurs.
func (r *crdHandler) GetCustomResourceListerCollectionDeleter(crd *apiextensions.CustomResourceDefinition) (finalizer.ListerCollectionDeleter, error) {
info, err := r.getOrCreateServingInfoFor(crd)
if err != nil {
utilruntime.HandleError(err)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to make the caller handle the error?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's what this is doing now

Copy link
Member Author

Choose a reason for hiding this comment

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

@langyenan

thanks for your review, but I think that an accurate description maybe better for k8s user to find the root cause.

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go#L160

i.e. could not list instances: unable to find a custom resource client for xxx.yyy due to zzz

return nil, err
}
return info.storage
return info.storage, nil
}

func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResourceDefinition) (*crdInfo, error) {
Expand Down Expand Up @@ -354,15 +351,10 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResource
}
creator := unstructuredCreator{}

// convert CRD schema to openapi schema
openapiSchema := &openapispec.Schema{}
if err := apiservervalidation.ConvertToOpenAPITypes(crd, openapiSchema); err != nil {
return nil, err
}
if err := openapispec.ExpandSchema(openapiSchema, nil, nil); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure this is only about expanding refs?

Copy link
Member Author

@carlory carlory Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@sttts
yes, see this comment

validator, err := apiservervalidation.NewSchemaValidator(crd.Spec.Validation)
if err != nil {
return nil, err
}
validator := validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default)

storage := customresource.NewREST(
schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Status.AcceptedNames.Plural},
Expand Down
1 change: 1 addition & 0 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ go_library(
importpath = "k8s.io/apiextensions-apiserver/pkg/apiserver/validation",
deps = [
"//vendor/github.com/go-openapi/spec:go_default_library",
"//vendor/github.com/go-openapi/strfmt:go_default_library",
"//vendor/github.com/go-openapi/validate:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
],
Expand Down
23 changes: 13 additions & 10 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,24 @@ package validation

import (
"github.com/go-openapi/spec"
"github.com/go-openapi/strfmt"
"github.com/go-openapi/validate"

"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
)

// NewSchemaValidator creates an openapi schema validator for the given CRD validation.
func NewSchemaValidator(customResourceValidation *apiextensions.CustomResourceValidation) (*validate.SchemaValidator, error) {
// Convert CRD schema to openapi schema
openapiSchema := &spec.Schema{}
if customResourceValidation != nil {
if err := convertJSONSchemaProps(customResourceValidation.OpenAPIV3Schema, openapiSchema); err != nil {
return nil, err
}
}
return validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default), nil
}

// ValidateCustomResource validates the Custom Resource against the schema in the CustomResourceDefinition.
// CustomResource is a JSON data structure.
func ValidateCustomResource(customResource interface{}, validator *validate.SchemaValidator) error {
Expand All @@ -33,16 +46,6 @@ func ValidateCustomResource(customResource interface{}, validator *validate.Sche
return nil
}

// ConvertToOpenAPITypes is used to convert internal types to go-openapi types.
func ConvertToOpenAPITypes(in *apiextensions.CustomResourceDefinition, out *spec.Schema) error {
if in.Spec.Validation != nil {
if err := convertJSONSchemaProps(in.Spec.Validation.OpenAPIV3Schema, out); err != nil {
return err
}
}
return nil
}

func convertJSONSchemaProps(in *apiextensions.JSONSchemaProps, out *spec.Schema) error {
if in == nil {
return nil
Expand Down
8 changes: 4 additions & 4 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ type ListerCollectionDeleter interface {
type CRClientGetter interface {
// GetCustomResourceListerCollectionDeleter gets the ListerCollectionDeleter for the given CRD
// UID.
GetCustomResourceListerCollectionDeleter(crd *apiextensions.CustomResourceDefinition) ListerCollectionDeleter
GetCustomResourceListerCollectionDeleter(crd *apiextensions.CustomResourceDefinition) (ListerCollectionDeleter, error)
}

// NewCRDFinalizer creates a new CRDFinalizer.
Expand Down Expand Up @@ -155,9 +155,9 @@ func (c *CRDFinalizer) deleteInstances(crd *apiextensions.CustomResourceDefiniti
// Now we can start deleting items. While it would be ideal to use a REST API client, doing so
// could incorrectly delete a ThirdPartyResource with the same URL as the CustomResource, so we go
// directly to the storage instead. Since we control the storage, we know that delete collection works.
crClient := c.crClientGetter.GetCustomResourceListerCollectionDeleter(crd)
if crClient == nil {
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)
Copy link
Contributor

@sttts sttts Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right.

return apiextensions.CustomResourceDefinitionCondition{
Type: apiextensions.Terminating,
Status: apiextensions.ConditionTrue,
Expand Down
13 changes: 12 additions & 1 deletion staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,16 +403,23 @@ func TestForbiddenFieldsInSchema(t *testing.T) {
t.Fatalf("unexpected non-error: uniqueItems cannot be set to true")
}

noxuDefinition.Spec.Validation.OpenAPIV3Schema.Ref = strPtr("#/definition/zeta")
noxuDefinition.Spec.Validation.OpenAPIV3Schema.Properties["zeta"] = apiextensionsv1beta1.JSONSchemaProps{
Type: "array",
UniqueItems: false,
}

_, err = testserver.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, clientPool)
if err == nil {
t.Fatal("unexpected non-error: ref cannot be non-empty string")
}

noxuDefinition.Spec.Validation.OpenAPIV3Schema.Ref = nil

_, err = testserver.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, clientPool)
if err != nil {
t.Fatal(err)
}

}

func float64Ptr(f float64) *float64 {
Expand All @@ -422,3 +429,7 @@ func float64Ptr(f float64) *float64 {
func int64Ptr(f int64) *int64 {
return &f
}

func strPtr(str string) *string {
return &str
}