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

Automated cherry pick of #117768: QueryParamVerifierV3 resilient to minimal OpenAPI V3 #117796: QueryParamVerifier falls back on invalid v3 document #117918

Merged
merged 3 commits into from
May 11, 2023
Merged

Automated cherry pick of #117768: QueryParamVerifierV3 resilient to minimal OpenAPI V3 #117796: QueryParamVerifier falls back on invalid v3 document #117918

merged 3 commits into from
May 11, 2023

Conversation

seans3
Copy link
Contributor

@seans3 seans3 commented May 10, 2023 •
edited by liggitt

Cherry pick of #117768 #117796 on release-1.27.

#117768: QueryParamVerifierV3 resilient to minimal OpenAPI V3
#117796: QueryParamVerifier fallsback when error retrieving OpenAPI V3 document

Fixes #117762

For details on the cherry pick process, see the cherry pick requests page.

Fixes a 1.27 regression in client-go where an incomplete OpenAPI V3 document can cause a nil-pointer crash. Ensures fallback to OpenAPI V2 endpoint for errors retrieving OpenAPI V3 document.

@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone May 10, 2023
@k8s-ci-robot k8s-ci-robot added do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. size/L Denotes a PR that changes 100-499 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. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels May 10, 2023
@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/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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 May 10, 2023
@seans3
Copy link
Contributor Author

seans3 commented May 10, 2023

/kind bug
/kind regression
/sig cli
/priority critical-urgent
/triage accepted

@k8s-ci-robot k8s-ci-robot added 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. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 10, 2023
@seans3
Copy link
Contributor Author

seans3 commented May 10, 2023

/cc @apelisse

@apelisse
Copy link
Member

/lgtm
/approve

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

LGTM label has been added.

Git tree hash: 6c98ea2dd4bc58518158d88c2a12f42820d874d9

@seans3
Copy link
Contributor Author

seans3 commented May 10, 2023

/retest

unrelated error

@seans3
Copy link
Contributor Author

seans3 commented May 10, 2023

/assign @liggitt

@seans3
Copy link
Contributor Author

seans3 commented May 10, 2023

/cc kubernetes/release-managers

@k8s-ci-robot k8s-ci-robot requested a review from a team May 10, 2023 23:05
liggitt
liggitt reviewed May 11, 2023
View reviewed changes
staging/src/k8s.io/cli-runtime/pkg/resource/query_param_verifier_v3.go
}
return false
return NewParamUnsupportedError(gvk, queryParam)
Copy link
Member

Choose a reason for hiding this comment

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

This PR is an improvement over the current state of release-1.27, so merging it seems fine, but I'd like to see a follow-up in master: if we don't find the path we want in the openapi v3 doc (if we fall through to line 130 here), we should return an error about the path not being found, not at definite NewParamUnsupportedError answer, right?

Otherwise, a stub openapi v3 doc like paths:{}, or paths:{"/version":{"get":...}} wouldn't fall back to v2

liggitt
liggitt reviewed May 11, 2023
View reviewed changes
staging/src/k8s.io/cli-runtime/pkg/resource/query_param_verifier_v3_test.go
"empty document returns error": {
spec: &spec3.OpenAPI{},
},
"minimal document returns error": {
Copy link
Member

Choose a reason for hiding this comment

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

in the follow-up in master, include more minimal/stub documents we could encounter that should fall back to v2:

Paths: &Paths{}

Paths: &Paths{Paths: map[string]*Path{"/version":nil}}

Paths: &Paths{Paths: map[string]*Path{"/version":&Path{}}}

@liggitt
Copy link
Member

liggitt commented May 11, 2023

/lgtm
/approve
/cc @kubernetes/release-managers

xmudrii
xmudrii approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

As per #117770 this is only affecting 1.27
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, liggitt, seans3, xmudrii

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

@xmudrii xmudrii added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label May 11, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label May 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit 74a7d8a into kubernetes:release-1.27 May 11, 2023
14 checks passed
@liggitt liggitt added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@xmudrii xmudrii xmudrii approved these changes

@brianpursley brianpursley Awaiting requested review from brianpursley

@soltysh soltysh Awaiting requested review from soltysh

@apelisse apelisse Awaiting requested review from apelisse

Assignees

@liggitt liggitt

@apelisse apelisse

@xmudrii xmudrii

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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/feature Categorizes issue or PR as related to a new feature. 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/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. 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/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
[sig-release] Bug Triage
Archived in project
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@seans3 @apelisse @k8s-ci-robot @liggitt @xmudrii