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

Start deprecation process for StreamingProxyRedirects #88290

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

tallclair
Copy link
Member

What type of PR is this?
/kind deprecation

What this PR does / why we need it:

Implements the first stage of the StreamingProxyRedirects deprecation, simply marking the feature & flag as deprecated. See https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/20191205-container-streaming-requests.md#rollout-plan

Which issue(s) this PR fixes:
For kubernetes/enhancements#1558

Does this PR introduce a user-facing change?:

The StreamingProxyRedirects feature and `--redirect-container-streaming` flag are deprecated, and will be removed in a future release. The default behavior (proxy streaming requests through the kubelet) will be the only supported option.
ACTION REQUIRED: If you are setting `--redirect-container-streaming=true`, then you must migrate off this configuration. The flag will no longer be able to be enabled starting in v1.20. If you are not setting the flag, no action is necessary.

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

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/20191205-container-streaming-requests.md

/sig node
/assign @Random-Liu @derekwaynecarr

@k8s-ci-robot k8s-ci-robot added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Feb 18, 2020
@k8s-ci-robot k8s-ci-robot added kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Feb 18, 2020
Copy link
Contributor

@mattjmcnaughton mattjmcnaughton left a comment

Choose a reason for hiding this comment

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

Thanks for the pr :)

I'm convinced this PR does the work outlined in your KEP. Thanks for such a clearly written KEP :)
LGTM modulo the test failures... feel free to ping me once the test failures are fixed and I will add the lgtm label again.

/lgtm

@@ -156,7 +159,7 @@ func init() {
// To add a new feature, define a key for it above and add it here. The features will be
// available throughout Kubernetes binaries.
var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
StreamingProxyRedirects: {Default: true, PreRelease: featuregate.Beta},
StreamingProxyRedirects: {Default: true, PreRelease: featuregate.Deprecated},
ValidateProxyRedirects: {Default: true, PreRelease: featuregate.Beta},
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm I'm understanding the KEP - do we ever intend to mark ValidateProxyRedirects as deprecated? Or as that unnecessary because its only utilized in conjunction with StreamingProxyRedirects, and we are marking StreamingProxyRedirects as deprecated (so when the time comes, we can just delete StreamingProxyRedirects)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, I was thinking about it a bit more when writing this change. The original KEP moved ValidateProxyRedirects to GA first, but folks thought that was weird since it would then be deleted. It might be more appropriate to mark it as deprecated, since it's going away, but I don't want people to see "oh this is deprecated, I should disable it", since it's dangerous to disable it before removing streaming proxy redirects.

I'm thinking of just leaving it as Beta, and marking it as deprecated once the behavior is removed (the feature gate can be removed a few releases after that). WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

That rationale makes sense to me! Mind capturing what yousaid above either in the KEP, commit message, or PR body? Or even a comment?

Thanks :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2020
@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 or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 19, 2020
@tallclair
Copy link
Member Author

Ah, I forgot to update the copy of the feature in the core package. Tests should pass now (hopefully).

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tallclair

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

Copy link
Contributor

@mattjmcnaughton mattjmcnaughton left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit 20e6883 into kubernetes:master Feb 21, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 21, 2020
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. area/apiserver area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants