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

Start deprecation process for StreamingProxyRedirects #88290

Merged
merged 1 commit into from
Feb 21, 2020
+6 −2
Merged

Start deprecation process for StreamingProxyRedirects #88290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/features/kube_features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS

// inherited features from generic apiserver, relisted here to get a conflict if it is changed
// unintentionally on either side:
genericfeatures.StreamingProxyRedirects: {Default: true, PreRelease: featuregate.Beta},
genericfeatures.StreamingProxyRedirects: {Default: true, PreRelease: featuregate.Deprecated},
genericfeatures.ValidateProxyRedirects: {Default: true, PreRelease: featuregate.Beta},
genericfeatures.AdvancedAuditing: {Default: true, PreRelease: featuregate.GA},
genericfeatures.DynamicAuditing: {Default: false, PreRelease: featuregate.Alpha},
Expand Down
1 change: 1 addition & 0 deletions pkg/kubelet/config/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ func (s *ContainerRuntimeOptions) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&s.ContainerRuntime, "container-runtime", s.ContainerRuntime, "The container runtime to use. Possible values: 'docker', 'remote'.")
fs.StringVar(&s.RuntimeCgroups, "runtime-cgroups", s.RuntimeCgroups, "Optional absolute name of cgroups to create and run the runtime in.")
fs.BoolVar(&s.RedirectContainerStreaming, "redirect-container-streaming", s.RedirectContainerStreaming, "Enables container streaming redirect. If false, kubelet will proxy container streaming data between apiserver and container runtime; if true, kubelet will return an http redirect to apiserver, and apiserver will access container runtime directly. The proxy approach is more secure, but introduces some overhead. The redirect approach is more performant, but less secure because the connection between apiserver and container runtime may not be authenticated.")
fs.MarkDeprecated("redirect-container-streaming", "Container streaming redirection will be removed from the kubelet in v1.20, and this flag will be removed in v1.22. For more details, see http://git.k8s.io/enhancements/keps/sig-node/20191205-container-streaming-requests.md")

// Docker-specific settings.
fs.BoolVar(&s.ExperimentalDockershim, "experimental-dockershim", s.ExperimentalDockershim, "Enable dockershim only mode. In this mode, kubelet will only start dockershim without any other functionalities. This flag only serves test purpose, please do not use it unless you are conscious of what you are doing. [default=false]")
Expand Down
5 changes: 4 additions & 1 deletion staging/src/k8s.io/apiserver/pkg/features/kube_features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,12 @@ const (
// owner: @tallclair
// alpha: v1.5
// beta: v1.6
// deprecated: v1.18
//
// StreamingProxyRedirects controls whether the apiserver should intercept (and follow)
// redirects from the backend (Kubelet) for streaming requests (exec/attach/port-forward).
//
// This feature is deprecated, and will be removed in v1.22.
StreamingProxyRedirects featuregate.Feature = "StreamingProxyRedirects"

// owner: @tallclair
Expand Down Expand Up @@ -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 :)

AdvancedAuditing: {Default: true, PreRelease: featuregate.GA},
DynamicAuditing: {Default: false, PreRelease: featuregate.Alpha},
Expand Down