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

DO NOT publish openapi specs containing bad types #79587

Conversation

@yue9944882
Copy link
Member

commented Jul 1, 2019

for now, we can set arbitrary types for the json schema like:

  validation:
    openAPIV3Schema:
      properties:
        spec:
          type: bug // whatever type except for the root element
      type: object

this is violating https://tools.ietf.org/html/draft-wright-json-schema-00#section-4.2 and this pull adds a list of supported types and their validation according to the specification.

https://swagger.io/docs/specification/data-models/data-types/

NONE

/king bug
/sig api-machinery

/cc @sttts

@k8s-ci-robot k8s-ci-robot requested a review from sttts Jul 1, 2019

@yue9944882 yue9944882 force-pushed the yue9944882:bugfix/tighten-primitive-json-schema-type-validation branch from d175a7b to 96aa889 Jul 1, 2019

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

/retest

@yue9944882 yue9944882 force-pushed the yue9944882:bugfix/tighten-primitive-json-schema-type-validation branch 3 times, most recently from 3b09d32 to 43f1ea4 Jul 1, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 1, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

/test pull-kubernetes-bazel-build

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

/kind bug
/area custom-resources

@yliaog

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

@k8s-ci-robot k8s-ci-robot requested review from liggitt and roycaihw Jul 1, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

/hold

this is a breaking change, so it cannot go in unconditionally.

what is the behavior:

  1. of the custom resource validation?
  2. of the openapi publishing?

we should ensure:

  1. v1 CRDs do not permit introduction of invalid types
  2. a CRD previously persisted with an invalid type does not crash the server when validating custom resources

@liggitt liggitt added this to the v1.16 milestone Jul 1, 2019

@roycaihw

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

  1. of the custom resource validation?
  • CR gets created successfully if the field is optional and the CR blob omits that field
  • otherwise go-openapi infers schema type from input CR blob and fails validating the type
validation failure list:
spec in body must be of type bug // whatever type except for the root element: "object"
  1. of the openapi publishing?
    kubectl fails parsing the non-standard type
Unknown primitive type: "bug // whatever type except for the root element"
@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

yea.. we have to ensure backward-compatibility in this fix. from a slack discuss w/ @sttts, we can only forbid invalid-typed CRD under following cases, so that existing CRD will keep working:

  • creates new CRD w/ invalid-typed property
  • updates valid-typed property on an existing CRD to invalid
@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

of the openapi publishing?
kubectl fails parsing the non-standard type
Unknown primitive type: "bug // whatever type except for the root element"

confirmed this one, kubectl aborts after requesting /openapi/v2 leaving an schema error in this case:

error: SchemaError(example.v1alpha1.Cluster.spec): Unknown primitive type: "bug"

@liggitt

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

confirmed this one, kubectl aborts after requesting /openapi/v2 leaving an schema error in this case:

error: SchemaError(example.v1alpha1.Cluster.spec): Unknown primitive type: "bug"

we are free to refuse to publish openapi for a CRD if it has an invalid type

@k8s-ci-robot k8s-ci-robot added the size/L label Jul 4, 2019

@yue9944882 yue9944882 force-pushed the yue9944882:bugfix/tighten-primitive-json-schema-type-validation branch from f0e35cd to 306d21b Jul 4, 2019

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

/test pull-kubernetes-integration

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

/retest

@@ -59,17 +59,17 @@ var buildDefinitions sync.Once
var namer *openapi.DefinitionNamer

// BuildSwagger builds swagger for the given crd in the given version
func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string) (*spec.Swagger, error) {
func BuildSwagger(crd *apiextensions.CustomResourceDefinition, oldStructuralSchema *structuralschema.Structural, version string) (*spec.Swagger, *structuralschema.Structural, error) {

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 11, 2019

Member

needing to pass an old schema into this method doesn't make sense to me. if there are invalid types in the schema, we should not publish openapi for it

This comment has been minimized.

Copy link
@roycaihw

roycaihw Jul 12, 2019

Member

yes for openapi publishing we can stop publishing invalid schema and it's backwards compatible

anyChanged := false
for _, v := range crd.Spec.Versions {
if !v.Served {
continue
}
spec, err := BuildSwagger(crd, v.Name)
var oldStructuralSchema *structralschema.Structural
if oldSpecs[v.Name] != nil {

This comment has been minimized.

Copy link
@liggitt

liggitt Jul 11, 2019

Member

and maintaining oldStructuralSchema here doesn't make sense to me either

@yue9944882 yue9944882 force-pushed the yue9944882:bugfix/tighten-primitive-json-schema-type-validation branch from 306d21b to 691750a Jul 15, 2019

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

/retest

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

@liggitt @roycaihw i picked the commit and appended some minor changes

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

politely bumping

@yue9944882 yue9944882 force-pushed the yue9944882:bugfix/tighten-primitive-json-schema-type-validation branch 2 times, most recently from e1e8351 to b6e415a Aug 14, 2019

do not publish openapi for a schema containing bad types
minor rewords & refactor

run script: update misc

remove null from supported types

@yue9944882 yue9944882 force-pushed the yue9944882:bugfix/tighten-primitive-json-schema-type-validation branch from b6e415a to 1ec8746 Aug 14, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/hold cancel
/lgtm
/approve

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/priority important-soon

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, yue9944882

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit a9bc3c0 into kubernetes:master Aug 14, 2019

22 of 23 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation roycaihw authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@liggitt liggitt moved this from Required for GA, in progress to Complete in Custom Resource Definitions Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.