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: ignore path conflict and resolve definition conflict when merging openapi spec #81436

Conversation

@roycaihw
Copy link
Member

commented Aug 15, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
partially fixes #78706 (comment)

Does this PR introduce a user-facing change?:

Fix a bug in CRD openapi controller that user-defined CRD can overwrite OpenAPI definition/path for the CRD API.
@jpbetz jpbetz referenced this pull request Aug 26, 2019
@roycaihw roycaihw force-pushed the roycaihw:crd-openapi/skip-duplicate-path-definition branch from 5d09b3f to 85a144c Aug 27, 2019
@roycaihw roycaihw changed the title [WIP] apiextensions-apiserver: skip duplicate path/definition when merging openapi spec apiextensions: ignore path conflict and resolve definition conflict when merging openapi spec Aug 27, 2019
@roycaihw roycaihw force-pushed the roycaihw:crd-openapi/skip-duplicate-path-definition branch from b170bc0 to 4c7fc42 Aug 27, 2019
@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

/retest

)

// MergeSpecs aggregates all OpenAPI specs, reusing the metadata of the first, static spec as the basis.
// Later paths and definitions override earlier ones. None of the input is mutated, but input
// and output share data structures.
func MergeSpecs(staticSpec *spec.Swagger, crdSpecs ...*spec.Swagger) *spec.Swagger {
func MergeSpecs(staticSpec *spec.Swagger, crdSpecs ...*spec.Swagger) (*spec.Swagger, error) {

This comment has been minimized.

Copy link
@sttts

sttts Aug 28, 2019

Contributor

we cannot assume that staticSpec has all kube-apiserver paths. Assume it only has apiextension, and maybe some discovery/status paths.

@roycaihw roycaihw force-pushed the roycaihw:crd-openapi/skip-duplicate-path-definition branch from 4c7fc42 to 6125a3b Aug 28, 2019
@k8s-ci-robot k8s-ci-robot removed the size/M label Aug 28, 2019
@sttts

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

I would like to see an etag follow-up that only retrieves the spec once and caches it (also for path and definition). For the freeze though this should be ok.

/lgtm
/approve

@sttts

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

/hold

for squashing.

}

func getOpenAPIPath(clientset *apiextensionsclientset.Clientset, path string) (spec.PathItem, bool, error) {
bs, err := clientset.RESTClient().Get().AbsPath("openapi", "v2").DoRaw()

This comment has been minimized.

Copy link
@sttts

sttts Aug 29, 2019

Contributor

follow-up: we shouldn't retrieve the spec twice, for path and definition (and the probe, so even three times).

@roycaihw roycaihw force-pushed the roycaihw:crd-openapi/skip-duplicate-path-definition branch from 271a290 to f30904c Aug 29, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 29, 2019
roycaihw added 3 commits Aug 28, 2019
from user-defined CRDs
@roycaihw roycaihw force-pushed the roycaihw:crd-openapi/skip-duplicate-path-definition branch from f30904c to 2a10b0d Aug 29, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@sttts

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

/lgtm

Follow-up with etag will come later.

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 29, 2019
@sttts

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

/retest

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 21cdcd5 into kubernetes:master Aug 30, 2019
24 checks passed
24 checks passed
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-conformance-kind-ipv6 Job succeeded.
Details
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-kubemark-e2e-gce-big 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
@roycaihw roycaihw referenced this pull request Sep 9, 2019
@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

/area custom-resources

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.