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

apiextensions: validation for customresources #47263

Merged
merged 6 commits into from Aug 30, 2017
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Validate CustomResource

* convert our types to openAPI types
* update strategy to include crd
* use strategy to validate customresource
* add helper funcs
* Fix conversion of empty ref field
* add validation for forbidden fields
* add defaulting for schema field
* Validate CRD Schema
  • Loading branch information...
nikhita committed Jun 22, 2017
commit fd09c3dbb67374e4f4b46b820b5cd3d9a3124cda
@@ -24,6 +24,7 @@ go_library(
],
deps = [
"//vendor/github.com/gogo/protobuf/proto:go_default_library",
"//vendor/github.com/gogo/protobuf/sortkeys:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/conversion:go_default_library",
@@ -10,10 +10,13 @@ go_library(
name = "go_default_library",
srcs = ["validation.go"],
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/features:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/validation:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
"//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
],
)

@@ -20,10 +20,15 @@ import (
"fmt"
"strings"

"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
"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"
apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features"
)

// ValidateCustomResourceDefinition statically validates
@@ -100,6 +105,12 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi

allErrs = append(allErrs, ValidateCustomResourceDefinitionNames(&spec.Names, fldPath.Child("names"))...)

if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceValidation) {

This comment has been minimized.

Copy link
@deads2k

deads2k Aug 18, 2017

Contributor

if the flag is off, you should disallow setting it: kubernetes/community#869

This comment has been minimized.

Copy link
@nikhita

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 18, 2017

Member

belt and suspenders... I would clear it (but not in validation... maybe in PrepareForCreate/PrepareForUpdate?) and ensure it is not set in validation

This comment has been minimized.

Copy link
@sttts

sttts Aug 18, 2017

Contributor

That's done as well.

allErrs = append(allErrs, ValidateCustomResourceDefinitionValidation(spec.Validation, fldPath.Child("validation"))...)
} else if spec.Validation != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("validation"), "disabled by feature-gate"))

This comment has been minimized.

Copy link
@sttts

sttts Aug 15, 2017

Contributor

what does the alpha field policy say? Should we just set this to zero on create and update or should we reject it?

This comment has been minimized.

Copy link
@nikhita

nikhita Aug 15, 2017

Author Member

From the policy: ensure the field is entirely absent from the API when empty (might require making the field a pointer).

}

return allErrs
}

@@ -158,3 +169,162 @@ func ValidateCustomResourceDefinitionNames(names *apiextensions.CustomResourceDe

return allErrs
}

// specStandardValidator applies validations for different OpenAPI specfication versions.
type specStandardValidator interface {
validate(spec *apiextensions.JSONSchemaProps, fldPath *field.Path) field.ErrorList
}

// ValidateCustomResourceDefinitionValidation statically validates
func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiextensions.CustomResourceValidation, fldPath *field.Path) field.ErrorList {

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 11, 2017

Member

The name has two Validate (in different forms) in it. Would it be better to name this ValidateCustomResourceDefinitionSpec?

This comment has been minimized.

Copy link
@nikhita

nikhita Aug 11, 2017

Author Member

Yeah, the name looks a bit funny. But ValidateCustomResourceDefinitionSpec exists already :/

(https://github.com/kubernetes/kubernetes/pull/47263/files#diff-54fdd67617b0f2ef1b5a09148f77303eR64-R107).

This comment has been minimized.

Copy link
@sttts

sttts Aug 11, 2017

Contributor

I would keep the name. It's logical. Better something logical and consistent than something shorter/different, but inconsistent.

allErrs := field.ErrorList{}

if customResourceValidation == nil {
return allErrs
}

if customResourceValidation.OpenAPIV3Schema != nil {
openAPIV3Schema := &specStandardValidatorV3{}
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(customResourceValidation.OpenAPIV3Schema, fldPath.Child("openAPIV3Schema"), openAPIV3Schema)...)

This comment has been minimized.

Copy link
@sttts

sttts Aug 14, 2017

Contributor

no .Child here. It's the same object, only additional checks.

This comment has been minimized.

Copy link
@nikhita

nikhita Aug 14, 2017

Author Member

This would make the JSON path as validation.openAPIV3Schema.type. Without the .Child, we get validation.type. Do we want the latter?

This comment has been minimized.

Copy link
@sttts

sttts Aug 14, 2017

Contributor

you are right. I thought we are inside the openAPIV3Schema here already.

}

return allErrs
}

// ValidateCustomResourceDefinitionOpenAPISchema statically validates
func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSchemaProps, fldPath *field.Path, ssv specStandardValidator) field.ErrorList {
allErrs := field.ErrorList{}

if schema == nil {
return allErrs
}

allErrs = append(allErrs, ssv.validate(schema, fldPath)...)

if schema.UniqueItems == true {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("uniqueItems"), "uniqueItems cannot be set to true since the runtime complexity becomes quadratic"))

This comment has been minimized.

Copy link
@sttts

sttts Aug 18, 2017

Contributor

can you check that the error message include the field name usually? I thought the output would be something like fldPath + " " + err

This comment has been minimized.

Copy link
@sttts

This comment has been minimized.

Copy link
@nikhita

nikhita Aug 18, 2017

Author Member

the error message is: CustomResourceDefinition.apiextensions.k8s.io "noxus.mygroup.example.com" is invalid: spec.validation.openAPIV3Schema.additionalProperties: Forbidden: additionalProperties cannot be set to false which looks fine. So, leaving this as-is.

}

// additionalProperties contradicts Kubernetes API convention to ignore unknown fields
if schema.AdditionalProperties != nil {
if schema.AdditionalProperties.Allows == false {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalProperties"), "additionalProperties cannot be set to false"))

This comment has been minimized.

Copy link
@sttts

sttts Aug 18, 2017

Contributor

maybe a comment above: additionalProperties contradict Kubernetes API convention to ignore unknown fields

This comment has been minimized.

Copy link
@nikhita

nikhita Aug 18, 2017

Author Member

Done.

}
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)...)
}

allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.Not, fldPath.Child("not"), ssv)...)

if len(schema.AllOf) != 0 {
for _, jsonSchema := range schema.AllOf {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("allOf"), ssv)...)
}
}

if len(schema.OneOf) != 0 {
for _, jsonSchema := range schema.OneOf {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("oneOf"), ssv)...)
}
}

if len(schema.AnyOf) != 0 {
for _, jsonSchema := range schema.AnyOf {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("anyOf"), ssv)...)
}
}

if len(schema.Properties) != 0 {
for property, jsonSchema := range schema.Properties {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("properties").Key(property), ssv)...)
}
}

if len(schema.PatternProperties) != 0 {
for property, jsonSchema := range schema.PatternProperties {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("patternProperties").Key(property), ssv)...)
}
}

if len(schema.Definitions) != 0 {
for definition, jsonSchema := range schema.Definitions {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("definitions").Key(definition), ssv)...)
}
}

if schema.Items != nil {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(schema.Items.Schema, fldPath.Child("items"), ssv)...)
if len(schema.Items.JSONSchemas) != 0 {
for _, jsonSchema := range schema.Items.JSONSchemas {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(&jsonSchema, fldPath.Child("items"), ssv)...)
}
}
}

if schema.Dependencies != nil {
for dependency, jsonSchemaPropsOrStringArray := range schema.Dependencies {
allErrs = append(allErrs, ValidateCustomResourceDefinitionOpenAPISchema(jsonSchemaPropsOrStringArray.Schema, fldPath.Child("dependencies").Key(dependency), ssv)...)
}
}

return allErrs
}

type specStandardValidatorV3 struct{}

// validate validates against OpenAPI Schema v3.
func (v *specStandardValidatorV3) validate(schema *apiextensions.JSONSchemaProps, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if schema == nil {
return allErrs
}

if schema.Default != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("default"), "default is not supported"))
}

if schema.ID != "" {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("id"), "id is not supported"))
}

if schema.AdditionalItems != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalItems"), "additionalItems is not supported"))
}

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

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

if schema.Dependencies != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("dependencies"), "dependencies is not supported"))
}

if schema.Type == "null" {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("type"), "type cannot be set to null"))
}

if schema.Items != nil && len(schema.Items.JSONSchemas) != 0 {

This comment has been minimized.

Copy link
@mbohlool

mbohlool Aug 11, 2017

Member

Duplicate of the check in common validator?

allErrs = append(allErrs, field.Forbidden(fldPath.Child("items"), "items must be a schema object and not an array"))
}

return allErrs
}
@@ -14,10 +14,14 @@ go_library(
"customresource_handler.go",
],
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",
"//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/apiserver/validation:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/client/clientset/internalclientset:go_default_library",
"//vendor/k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions:go_default_library",
@@ -68,6 +72,9 @@ filegroup(

filegroup(
name = "all-srcs",
srcs = [":package-srcs"],
srcs = [
":package-srcs",
"//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation:all-srcs",
],
tags = ["automanaged"],
)
@@ -171,6 +171,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
groupDiscoveryHandler,
s.GenericAPIServer.RequestContextMapper(),
s.Informers.Apiextensions().InternalVersion().CustomResourceDefinitions().Lister(),
s.Informers.Apiextensions().InternalVersion().CustomResourceDefinitions(),
delegateHandler,
c.CRDRESTOptionsGetter,
c.GenericConfig.AdmissionControl,
@@ -26,6 +26,11 @@ import (
"sync/atomic"
"time"

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

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -44,8 +49,11 @@ import (
genericregistry "k8s.io/apiserver/pkg/registry/generic/registry"
"k8s.io/apiserver/pkg/storage/storagebackend"
"k8s.io/client-go/discovery"
cache "k8s.io/client-go/tools/cache"

"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
apiservervalidation "k8s.io/apiextensions-apiserver/pkg/apiserver/validation"
informers "k8s.io/apiextensions-apiserver/pkg/client/informers/internalversion/apiextensions/internalversion"
listers "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/internalversion"
"k8s.io/apiextensions-apiserver/pkg/controller/finalizer"
"k8s.io/apiextensions-apiserver/pkg/registry/customresource"
@@ -84,6 +92,7 @@ func NewCustomResourceDefinitionHandler(
groupDiscoveryHandler *groupDiscoveryHandler,
requestContextMapper apirequest.RequestContextMapper,
crdLister listers.CustomResourceDefinitionLister,
crdInformer informers.CustomResourceDefinitionInformer,
delegate http.Handler,
restOptionsGetter generic.RESTOptionsGetter,
admission admission.Interface) *crdHandler {
@@ -98,6 +107,10 @@ func NewCustomResourceDefinitionHandler(
admission: admission,
}

crdInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
UpdateFunc: ret.updateCustomResourceDefinition,

This comment has been minimized.

Copy link
@deads2k

deads2k Aug 18, 2017

Contributor

It's unusual to handle update and not create. Why the difference?

This comment has been minimized.

Copy link
@sttts

sttts Aug 18, 2017

Contributor

create is implicit. We create the strategy on demand (your code btw ;-)).

But on update we have to delete the cached strategy. That's what this handler does.

})

ret.customStorage.Store(crdStorageMap{})
return ret
}
@@ -155,7 +168,12 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {

terminating := apiextensions.IsCRDConditionTrue(crd, apiextensions.Terminating)

crdInfo := r.getServingInfoFor(crd)
crdInfo, err := r.getServingInfoFor(crd)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}

storage := crdInfo.storage
requestScope := crdInfo.requestScope
minRequestTimeout := 1 * time.Minute
@@ -250,23 +268,26 @@ 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 {
info := r.getServingInfoFor(crd)
info, err := r.getServingInfoFor(crd)
if err != nil {
utilruntime.HandleError(err)
}
return info.storage
}

func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefinition) *crdInfo {
func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefinition) (*crdInfo, error) {
storageMap := r.customStorage.Load().(crdStorageMap)
ret, ok := storageMap[crd.UID]
if ok {
return ret
return ret, nil
}

r.customStorageLock.Lock()
defer r.customStorageLock.Unlock()

ret, ok = storageMap[crd.UID]
if ok {
return ret
return ret, nil
}

// In addition to Unstructured objects (Custom Resources), we also may sometimes need to
@@ -287,6 +308,17 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti
unstructuredTyper: discovery.NewUnstructuredObjectTyper(nil),
}
creator := unstructuredCreator{}

// convert CRD schema to openapi schema
openapiSchema := &openapispec.Schema{}

This comment has been minimized.

Copy link
@deads2k

deads2k Aug 18, 2017

Contributor

Is this block likely to fail? If so, this reads like a status condition

This comment has been minimized.

Copy link
@sttts

sttts Aug 18, 2017

Contributor

No, the validation should make sure it does not fail. Everything else is a bug in validation.

This comment has been minimized.

Copy link
@sttts

sttts Aug 18, 2017

Contributor

Also note that the conversion is basically a 1:1 translation with a number of restrictions on the input, plus some string->openapi-type conversion which are done during validation already in order to guarantee they work in this step as well.

if err := apiservervalidation.ConvertToOpenAPITypes(crd, openapiSchema); 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.Spec.Names.Plural},
schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Spec.Names.ListKind},
@@ -295,6 +327,7 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti
typer,
crd.Spec.Scope == apiextensions.NamespaceScoped,
kind,
validator,
),
r.restOptionsGetter,
)
@@ -354,7 +387,27 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti

storageMap2[crd.UID] = ret
r.customStorage.Store(storageMap2)
return ret
return ret, nil
}

func (c *crdHandler) updateCustomResourceDefinition(oldObj, _ interface{}) {
oldCRD := oldObj.(*apiextensions.CustomResourceDefinition)
glog.V(4).Infof("Updating customresourcedefinition %s", oldCRD.Name)

c.customStorageLock.Lock()
defer c.customStorageLock.Unlock()

storageMap := c.customStorage.Load().(crdStorageMap)
storageMap2 := make(crdStorageMap, len(storageMap))

// Copy because we cannot write to storageMap without a race
// as it is used without locking elsewhere
for k, v := range storageMap {

This comment has been minimized.

Copy link
@sttts

sttts Aug 3, 2017

Contributor

can you put a comment above like "Copy because we cannot write to storageMap without a race as it is used without locking elsewhere

This comment has been minimized.

Copy link
@sttts

sttts Aug 18, 2017

Contributor

fixed

storageMap2[k] = v
}

delete(storageMap2, oldCRD.UID)
c.customStorage.Store(storageMap2)

This comment has been minimized.

Copy link
@sttts

sttts Aug 3, 2017

Contributor

We need the same for adding an element.

This comment has been minimized.

Copy link
@nikhita

nikhita Aug 3, 2017

Author Member

Created #50098.

}

type unstructuredNegotiatedSerializer struct {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.