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

Add ability to skip OpenAPI handler installation #100341

Merged
merged 1 commit into from Mar 19, 2021

Conversation

kevindelgado
Copy link
Contributor

@kevindelgado kevindelgado commented Mar 17, 2021

What type of PR is this?

/kind bug

What this PR does / why we need it:

This provides the ability to configure a GenericAPIServer to not install the OpenAPI handler so that the kube-aggregator (which has its own OpenAPI handler) does not double register the openapi/v2 handler.

Which issue(s) this PR fixes:

Addresses #99046 (comment)

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note kind/bug size/M cncf-cla: yes labels Mar 17, 2021
Copy link
Contributor Author

@kevindelgado kevindelgado left a comment

/assign @liggitt

cmd/kube-apiserver/app/aggregator.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery label Mar 17, 2021
@kevindelgado kevindelgado requested a review from liggitt Mar 17, 2021
@kevindelgado kevindelgado requested a review from liggitt Mar 17, 2021
@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Mar 17, 2021
@kevindelgado kevindelgado requested a review from liggitt Mar 17, 2021
@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 18, 2021

cc @apelisse

@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 18, 2021

Thinking about Anotine's feedback about not wanting to store a bool another idea I have is to require consumers to set the StaticOpenAPISpec on the generic config themselves at the same time they set the OpenAPIConfig. This would require adding the spec to the generic config struct.

Then, the handler installation is triggered by the spec being non-nil rather than the config. Instead of calling routes.OpenAPI#Install which creates a spec to then install the handler, we install the handler directly from the provided spec.

The end result is that GenericAPIServer's StaticOpenAPISpec is only used for openAPI handler installation while the openAPIConfig is only used for generating openAPI models (which is in turn used by the field manager).

This seems like a better approach in terms of decoupling the OpenAPIConfig and StaticOpenAPISpec as long as we're okay with requiring the consumer to be the one to generate the spec from the config (if they want the handler installed).

Let me know what you think @liggitt @apelisse

@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 18, 2021

Okay, I just spoke to @apelisse and we decided to continue with the boolean approach. My suggestion about requiring consumers to pass the spec could introduce a big foot-gun where the spec doesn't match the config and so we'd probably need to generate the spec anyways to check for that.

Other ideas like passing a function in the config to optionally convert to a spec seemed like overkill compared to the bool, so we've agreed on that for now. Thoughts? PTAL @liggitt

@liggitt
Copy link
Member

@liggitt liggitt commented Mar 18, 2021

yeah, this seems reasonable

/lgtm
/approve
/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test label Mar 18, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 18, 2021
@liggitt
Copy link
Member

@liggitt liggitt commented Mar 18, 2021

/milestone v1.21
/kind bug

avoids a spurious stack trace in the API server log on every start as a regression in 1.21

@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 18, 2021
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

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 added the approved label Mar 18, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 18, 2021
@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 19, 2021

Can you give it another lgtm @liggitt, I had to change the name of the config in server/start (s/genericConfig/serverConfig). All tests passing now.

@liggitt
Copy link
Member

@liggitt liggitt commented Mar 19, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 19, 2021
@kevindelgado
Copy link
Contributor Author

@kevindelgado kevindelgado commented Mar 19, 2021

/retest

@k8s-ci-robot k8s-ci-robot merged commit 019080f into kubernetes:master Mar 19, 2021
13 of 14 checks passed
@k8s-ci-robot k8s-ci-robot added release-note-none and removed release-note labels Mar 27, 2021
k8s-ci-robot added a commit that referenced this issue Apr 8, 2021
…-#100341-#98576-upstream-release-1.19

cherry pick  #100341 #98576 #95836 on 1.19 to enable SSA with APIService
k8s-ci-robot added a commit that referenced this issue Apr 8, 2021
…-#100341-#98576-upstream-release-1.20

cherry pick #100341 #98576 on 1.20 to enable SSA with APIService
k8s-ci-robot added a commit that referenced this issue Apr 9, 2021
…-#100341-#98576-#--dry-run-upstream-release-1.18-1617135987

cherry pick  #100341 #98576 #95836 on 1.18 to enable SSA with APIService
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apiserver cncf-cla: yes kind/bug lgtm ok-to-test release-note-none sig/api-machinery size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants