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

Tolerate 406 mime-type errors attempting to load new openapi schema #61949

Merged
merged 1 commit into from Apr 2, 2018
Merged

Tolerate 406 mime-type errors attempting to load new openapi schema #61949

merged 1 commit into from Apr 2, 2018

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Mar 30, 2018 •
edited

Fixes #61805
Fixes #61943

kubectl: improves compatibility with older servers when creating/updating API objects

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 30, 2018
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 30, 2018
@liggitt
Copy link
Member Author

liggitt commented Mar 30, 2018

cc @kubernetes/sig-cli-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Mar 30, 2018
@liggitt
Copy link
Member Author

liggitt commented Mar 30, 2018

/assign roycaihw

@liggitt liggitt added this to the v1.10 milestone Mar 30, 2018
@liggitt liggitt added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cherrypick-candidate labels Mar 30, 2018
CaoShuFeng
CaoShuFeng reviewed Mar 31, 2018
View reviewed changes
staging/src/k8s.io/client-go/discovery/discovery_client_test.go Outdated
@@ -326,12 +326,12 @@ var returnedOpenAPI = openapi_v2.Document{
},
}

func openapiSchemaDeprecatedFakeServer() (*httptest.Server, error) {
func openapiSchemaDeprecatedFakeServer(status int) (*httptest.Server, error) {
var sErr error
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
// old server returns 403 on new endpoint request
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, we need to update this:

// old server returns 403 on new endpoint request

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@liggitt
Copy link
Member Author

liggitt commented Mar 31, 2018

/status approved-for-milestone

@CaoShuFeng
Copy link
Contributor

looks good to me.

@liggitt
Copy link
Member Author

liggitt commented Mar 31, 2018

/retest

1 similar comment
@liggitt
Copy link
Member Author

liggitt commented Mar 31, 2018

/retest

@dixudx
Copy link
Member

dixudx commented Apr 2, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, 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-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@dixudx @liggitt @roycaihw

Pull Request Labels
  • sig/cli: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 6610069 into kubernetes:master Apr 2, 2018
@liggitt liggitt deleted the tolerate-openapi branch April 2, 2018 13:28
@liggitt liggitt mentioned this pull request Apr 2, 2018
Merged
@roycaihw
Copy link
Member

roycaihw commented Apr 2, 2018

Thanks. /lgtm

I didn't reproduce the 406 error against 1.8 & 1.9 masters (got 404), couldn't figure out what's the order our server used to honor the http errors in this case.

Also, are we seeing the 406 problem with aggregator server? This one is requesting application/json though:

kubernetes/staging/src/k8s.io/kube-aggregator/pkg/controllers/openapi/downloader.go

Lines 116 to 118 in 9789285

// single endpoint not found/registered in old server, try to fetch old endpoint
// TODO(roycaihw): remove this in 1.11
if writer.respCode == http.StatusForbidden || writer.respCode == http.StatusNotFound {

@liggitt liggitt mentioned this pull request Apr 3, 2018
Closed
k8s-github-robot pushed a commit that referenced this pull request Apr 4, 2018
Merge pull request #62024 from liggitt/automated-cherry-pick-of-#6194…
094504a 
…9-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #61949: Tolerate 406 mime-type errors attempting to load new openapi

Cherry pick of #61949 on release-1.10.

#61949: Tolerate 406 mime-type errors attempting to load new openapi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaoShuFeng CaoShuFeng CaoShuFeng left review comments

@j3ffml j3ffml Awaiting requested review from j3ffml

@tallclair tallclair Awaiting requested review from tallclair

Assignees

@dixudx dixudx

@roycaihw roycaihw

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. 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/cli Categorizes an issue or PR as relevant to SIG CLI. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@liggitt @CaoShuFeng @dixudx @k8s-ci-robot @k8s-github-robot @fejta-bot @roycaihw