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

Fallback to legacy discovery on a wider range of conditions #119870

Merged
merged 1 commit into from Sep 1, 2023

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Aug 9, 2023

/kind bug

What this PR does / why we need it:

Fallback to legacy discovery if Aggregated Discovery query was unsuccessful. This includes other response codes than 406, and missing/incorrect content-type headers.

Which issue(s) this PR fixes:

Fixes #119662

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed a regression in default 1.27 configurations in kube-apiserver: fixed the AggregatedDiscoveryEndpoint feature (beta in 1.27+) to successfully fetch discovery information from aggregated API servers that do not check `Accept` headers when serving the `/apis` endpoint

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2023
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.28 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.28.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Aug 9 16:32:55 UTC 2023.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 9, 2023
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 9, 2023
@Jefftree Jefftree force-pushed the agg-discovery-406 branch 2 times, most recently from 22582e1 to f8d6e7a Compare August 9, 2023 21:01
@Jefftree
Copy link
Member Author

Jefftree commented Aug 9, 2023

/assign @liggitt
/cc @apelisse @alexzielenski

@jiahuif
Copy link
Member

jiahuif commented Aug 10, 2023

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 10, 2023
Copy link
Contributor

@alexzielenski alexzielenski left a comment

Choose a reason for hiding this comment

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

mostly lgtm

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2023
@Jefftree Jefftree force-pushed the agg-discovery-406 branch 2 times, most recently from 169cb55 to 7fc9bab Compare August 17, 2023 18:29
staging/src/k8s.io/client-go/discovery/discovery_client.go Outdated Show resolved Hide resolved
staging/src/k8s.io/client-go/discovery/discovery_client.go Outdated Show resolved Hide resolved
staging/src/k8s.io/client-go/discovery/discovery_client.go Outdated Show resolved Hide resolved
staging/src/k8s.io/client-go/discovery/discovery_client.go Outdated Show resolved Hide resolved
@@ -217,8 +219,7 @@ func (dm *discoveryManager) fetchFreshDiscoveryForService(gv metav1.GroupVersion
writer := newInMemoryResponseWriter()
handler.ServeHTTP(writer, req)

switch writer.respCode {
Copy link
Member

Choose a reason for hiding this comment

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

I think a switch might still be more readable here

switch {
case writer.respCode == http.StatusNotModified:
  // ...cache case

case writer.respCode == http.StatusServiceUnavailable:
  // ...server unavailable

case writer.respCode == http.StatusOK && discovery.ContentTypeIsGVK(writer.Header().Get("Content-Type"), ...):
  // ...aggregated

default:
  // ...fallback to legacy
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Since discovery.ContentTypeIsGVK(...) returns two values, the condition would need to be checked outside the case statement. Eg:

isGVK, err := discovery.ContentTypeIsGVK(...)
switch {
case ...
case writer.respCode == http.StatusOK && isGVK && err == nil:
// ...aggregated
...
}

Wouldn't an if statement be slightly cleaner in this regard?

Copy link
Member

Choose a reason for hiding this comment

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

heh, true

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to a case statement with the GVK check outside of it, lmk if this is readable.

@liggitt
Copy link
Member

liggitt commented Sep 1, 2023

/priority important-soon

@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Sep 1, 2023
@liggitt
Copy link
Member

liggitt commented Sep 1, 2023

/kind regression

@k8s-ci-robot k8s-ci-robot added kind/regression Categorizes issue or PR as related to a regression from a prior release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 1, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jefftree, 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 Sep 1, 2023
@k8s-ci-robot k8s-ci-robot merged commit 42275da into kubernetes:master Sep 1, 2023
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Sep 1, 2023
k8s-ci-robot added a commit that referenced this pull request Sep 4, 2023
…9870-upstream-release-1.27

Automated cherry pick of #119870: Fallback to legacy discovery on a wider range of conditions
k8s-ci-robot added a commit that referenced this pull request Sep 4, 2023
…9870-upstream-release-1.28

Automated cherry pick of #119870: Fallback to legacy discovery on a wider range of conditions
brandond added a commit to johnatasr/k3s that referenced this pull request Sep 16, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to tashima42/k3s that referenced this pull request Sep 18, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to johnatasr/k3s that referenced this pull request Sep 18, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to tashima42/k3s that referenced this pull request Sep 19, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to johnatasr/k3s that referenced this pull request Sep 19, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to johnatasr/k3s that referenced this pull request Sep 19, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to johnatasr/k3s that referenced this pull request Sep 19, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to johnatasr/k3s that referenced this pull request Sep 19, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
johnatasr pushed a commit to k3s-io/k3s that referenced this pull request Sep 19, 2023
* Update to v1.28.2

Signed-off-by: Johnatas <johnatasr@hotmail.com>

* Bump containerd and stargz versions

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Print message on upgrade fail

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Send Bad Gateway instead of Service Unavailable when tunnel dial fails

Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Add 60 seconds to server upgrade wait to account for delays in apiserver readiness

Also change cleanup helper to ensure upgrade test doesn't pollute the
images for the rest of the tests.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

---------

Signed-off-by: Johnatas <johnatasr@hotmail.com>
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
Co-authored-by: Brad Davidson <brad.davidson@rancher.com>
tashima42 added a commit to k3s-io/k3s that referenced this pull request Sep 19, 2023
* Update to v1.27.6

Signed-off-by: Pedro Tashima <pedro.tashima@suse.com>

* Bump containerd and stargz versions

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Print message on upgrade fail

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Send Bad Gateway instead of Service Unavailable when tunnel dial fails

Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Add 60 seconds to server upgrade wait to account for delays in apiserver readiness

Also change cleanup helper to ensure upgrade test doesn't pollute the
images for the rest of the tests.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

---------

Signed-off-by: Pedro Tashima <pedro.tashima@suse.com>
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
Co-authored-by: Pedro Tashima <pedro.tashima@suse.com>
Co-authored-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to brandond/k3s that referenced this pull request Sep 19, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond added a commit to k3s-io/k3s that referenced this pull request Sep 19, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
johnatasr pushed a commit to k3s-io/k3s that referenced this pull request Sep 19, 2023
* Update to v1.26.9

Signed-off-by: Johnatas <johnatasr@hotmail.com>

* Bump containerd and stargz versions

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Print message on upgrade fail

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Send Bad Gateway instead of Service Unavailable when tunnel dial fails

Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

* Add 60 seconds to server upgrade wait to account for delays in apiserver readiness

Also change cleanup helper to ensure upgrade test doesn't pollute the
images for the rest of the tests.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

---------

Signed-off-by: Johnatas <johnatasr@hotmail.com>
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
Co-authored-by: Brad Davidson <brad.davidson@rancher.com>
xiaods added a commit to xiaods/k8e that referenced this pull request Sep 23, 2023
Works around new handling for Service Unavailable by apiserver aggregation added in kubernetes/kubernetes#119870

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
porting by
Signed-off-by: Deshi Xiao <xiaods@gmail.com>
@Jefftree Jefftree deleted the agg-discovery-406 branch January 25, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Namespace stuck in Terminating when deleted if ApiService doesn't implement Aggregated Discovery
5 participants