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.

Sign up for GitHub

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

Jump to bottom

Add ability to skip OpenAPI handler installation #100341

Merged
merged 1 commit into from Mar 19, 2021
Merged

Add ability to skip OpenAPI handler installation #100341

merged 1 commit into from Mar 19, 2021

Conversation

kevindelgado
Copy link
Contributor

@kevindelgado kevindelgado commented Mar 17, 2021 •
edited by liggitt

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 Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 17, 2021
kevindelgado
kevindelgado commented Mar 17, 2021
View reviewed changes
Copy link
Contributor Author

@kevindelgado kevindelgado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/assign @liggitt

cmd/kube-apiserver/app/aggregator.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 17, 2021
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 17, 2021
@kevindelgado
Copy link
Contributor Author

cc @apelisse

@kevindelgado
Copy link
Contributor Author

kevindelgado commented Mar 18, 2021 •
edited

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

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 commented Mar 18, 2021

yeah, this seems reasonable

/lgtm
/approve
/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 18, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 18, 2021
@liggitt
Copy link
Member

liggitt commented Mar 18, 2021 •
edited

/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

[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 Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 18, 2021
@kevindelgado
Copy link
Contributor Author

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 commented Mar 19, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2021
@kevindelgado
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 019080f into kubernetes:master Mar 19, 2021
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 27, 2021
This was referenced Mar 31, 2021
Merged
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Apr 8, 2021
@k8s-ci-robot
Merge pull request #100713 from kevindelgado/automated-cherry-pick-of…
64c9b22 
…-#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 pull request Apr 8, 2021
@k8s-ci-robot
Merge pull request #100714 from kevindelgado/automated-cherry-pick-of…
6edb1ec 
…-#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 pull request Apr 9, 2021
@k8s-ci-robot
Merge pull request #100715 from kevindelgado/automated-cherry-pick-of…
c552288 
…-#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
Reviewers

@hzxuzhonghu hzxuzhonghu Awaiting requested review from hzxuzhonghu

@lavalamp lavalamp Awaiting requested review from lavalamp

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kevindelgado @liggitt @k8s-ci-robot