From 8b8d5222280c9ac28ade69d027bd06f96e20a8b8 Mon Sep 17 00:00:00 2001 From: carlory Date: Thu, 18 Jan 2018 14:30:53 +0800 Subject: [PATCH] fix apiserver crash caused by nil pointer and ensure CRD schema validator can be constructed during validation. --- .../pkg/apis/apiextensions/validation/BUILD | 2 +- .../apiextensions/validation/validation.go | 24 +++++++++---------- .../pkg/apiserver/BUILD | 3 --- .../pkg/apiserver/customresource_handler.go | 20 +++++----------- .../pkg/apiserver/validation/BUILD | 1 + .../pkg/apiserver/validation/validation.go | 23 ++++++++++-------- .../pkg/controller/finalizer/crd_finalizer.go | 8 +++---- .../test/integration/validation_test.go | 13 +++++++++- 8 files changed, 48 insertions(+), 46 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD index 15884eb628e..23bab9a868c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/BUILD @@ -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", diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index f064b99333c..4408d84274e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -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" ) @@ -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))) + } + } return allErrs } @@ -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)...) } @@ -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")) + } + if schema.Type == "null" { allErrs = append(allErrs, field.Forbidden(fldPath.Child("type"), "type cannot be set to null")) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD index dbed4134690..5547d38e8c0 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD @@ -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", diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index f87718d7621..d91cacdaac8 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -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" @@ -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) + return nil, err } - return info.storage + return info.storage, nil } func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResourceDefinition) (*crdInfo, error) { @@ -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 { + validator, err := apiservervalidation.NewSchemaValidator(crd.Spec.Validation) + if err != nil { return nil, err } - if err := openapispec.ExpandSchema(openapiSchema, nil, nil); 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}, diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/BUILD index 0fff89c68a6..a5e89d49930 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/BUILD @@ -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", ], diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go index c1cff141774..e39ac3e6d68 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go @@ -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 { @@ -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 diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go index b60900ab1ba..702cd4f00ba 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go @@ -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. @@ -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) return apiextensions.CustomResourceDefinitionCondition{ Type: apiextensions.Terminating, Status: apiextensions.ConditionTrue, diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index 2538149e266..257efc2140e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -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 { @@ -422,3 +429,7 @@ func float64Ptr(f float64) *float64 { func int64Ptr(f int64) *int64 { return &f } + +func strPtr(str string) *string { + return &str +}