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

KEP-4006: RemoteCommand as Beta, and initial PortForward over WebSockets as Alpha #4416

Merged
merged 1 commit into from
Feb 8, 2024
Merged

KEP-4006: RemoteCommand as Beta, and initial PortForward over WebSockets as Alpha #4416

merged 1 commit into from
Feb 8, 2024

Conversation

seans3
Copy link
Contributor

@seans3 seans3 commented Jan 18, 2024 •
edited
Loading

KEP-4006: RemoteCommand as Beta and initial PortForward over WebSockets

  • Updates RemoteCommand over WebSockets to Beta in v1.30.
  • Adds design for PortForward over WebSockets, which will be Alpha in v1.30.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jan 18, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 18, 2024
@seans3
Copy link
Contributor Author

seans3 commented Jan 19, 2024

/cc @aojea
/cc @liggitt
/cc @soltysh
/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jan 19, 2024
@seans3 seans3 mentioned this pull request Jan 19, 2024
Open
8 tasks
@seans3 seans3 mentioned this pull request Jan 22, 2024
Closed
@aojea
Copy link
Member

aojea commented Jan 23, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 23, 2024
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 24, 2024
ardaguclu
ardaguclu reviewed Jan 25, 2024
View reviewed changes
Copy link
Member

@ardaguclu ardaguclu left a comment

Choose a reason for hiding this comment

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

Websockets + streaming functionality with headers. It closes to Http/2 in that manner with all the features of Websockets.

keps/sig-api-machinery/4006-transition-spdy-to-websockets/README.md Show resolved Hide resolved
keps/sig-api-machinery/4006-transition-spdy-to-websockets/README.md Show resolved Hide resolved
@ardaguclu
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2024
@seans3 seans3 force-pushed the websocket-kep branch 4 times, most recently from db01f7d to ac04b5f Compare February 6, 2024 08:28
soltysh
soltysh reviewed Feb 6, 2024
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

Minor wording changes suggested, all the rest looks great, thx Sean!

keps/sig-api-machinery/4006-transition-spdy-to-websockets/README.md Outdated
environment variable set to **ON**, so it will request the newer WebSockets streaming
Server. Additionally, the `kubectl` client must also have either the
KUBECTL_REMOTE_COMMAND_WEBSOCKETS or KUBECTL_PORT_FORWARD_WEBSOCKETS environment
variables set to **ON**, so this client will request the newer WebSockets streaming
Copy link
Contributor

Choose a reason for hiding this comment

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

The additions are great, I'd still try to change the wording in:

 Additionally, the `kubectl` client must also have either the
KUBECTL_REMOTE_COMMAND_WEBSOCKETS or KUBECTL_PORT_FORWARD_WEBSOCKETS environment
variables set to **ON**, so this client will request the newer WebSockets streaming

to something more explicit, iow:

Additionally, the `kubectl` client must also have the KUBECTL_REMOTE_COMMAND_WEBSOCKETS 
environment variable set to ON for exec, cp, and attach commands.  
KUBECTL_PORT_FORWARD_WEBSOCKETS environment
variables set to **ON** for port-forward command.

keps/sig-api-machinery/4006-transition-spdy-to-websockets/README.md Outdated
environment variable set to **ON**, so it will request the newer WebSockets streaming
Server. Additionally, the `kubectl` client must also have either the
KUBECTL_REMOTE_COMMAND_WEBSOCKETS or KUBECTL_PORT_FORWARD_WEBSOCKETS environment
variables set to **ON**, so this client will request the newer WebSockets streaming
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly in the kube-apiserver section:
Instead of :

For each of the two streaming subprotocols: `RemoteCommand` and `PortForward`,
enabling the feature gate on the API Server will allow the streaming mechanism
to be WebSockets instead of SPDY for communication between `kubectl` and the API

write

For each of the two streaming subprotocols: `RemoteCommand` (such as /exec and /attach APIs) 
and `PortForward` (for /portforward), enabling the respective feature gate on the API Server will 
allow the streaming mechanism to be WebSockets instead of SPDY for communication between 
`kubectl` and the API

BenTheElder
BenTheElder reviewed Feb 6, 2024
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

generally LGTM, one PRR question

keps/sig-api-machinery/4006-transition-spdy-to-websockets/README.md
- PortForward: `kubectl port-forward -v=7 <SERVICE|DEPLOYMENT|POD> <LOCAL_PORT:REMOTE_PORT>`
- PortForward: One of the output log lines will be
`...websocket-dialer.go:91] negotiated protocol: v2.portforward.k8s.io`
if websockets is enabled for port forwarding.

###### What are the reasonable SLOs (Service Level Objectives) for the enhancement?
Copy link
Member

Choose a reason for hiding this comment

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

Missing this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@seans3 seans3 force-pushed the websocket-kep branch 3 times, most recently from 31e986a to d2e0997 Compare February 6, 2024 23:46
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2024
@jpbetz
Copy link
Contributor

jpbetz commented Feb 7, 2024

PRR LGTM. Any concerns @BenTheElder?

soltysh
soltysh approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

lgtm
from sig-cli pov

@liggitt
Copy link
Member

liggitt commented Feb 7, 2024

/assign @jpbetz
for final PRR ack

@jpbetz jpbetz mentioned this pull request Feb 7, 2024
Merged
@BenTheElder
Copy link
Member

LGTM!

@jpbetz
Copy link
Contributor

jpbetz commented Feb 8, 2024

/approve PRR

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, seans3, soltysh

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 Feb 8, 2024
@jpbetz
Copy link
Contributor

jpbetz commented Feb 8, 2024

Any concerns for this one before we merge @deads2k?

@jpbetz
Copy link
Contributor

jpbetz commented Feb 8, 2024

/lgtm
I think this is ready.

Thanks @seans3

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2024
@k8s-ci-robot k8s-ci-robot merged commit a21b94b into kubernetes:master Feb 8, 2024
3 of 4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.30 milestone Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ardaguclu ardaguclu ardaguclu left review comments

@liggitt liggitt liggitt left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@jpbetz jpbetz jpbetz left review comments

@soltysh soltysh soltysh approved these changes

@apelisse apelisse Awaiting requested review from apelisse

@aojea aojea Awaiting requested review from aojea

Assignees

@jpbetz jpbetz

@soltysh soltysh

@aojea aojea

@ardaguclu ardaguclu

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/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
SIG CLI
Archived in project
Milestone
v1.30
Development

Successfully merging this pull request may close these issues.
8 participants
@seans3 @aojea @ardaguclu @jpbetz @liggitt @BenTheElder @k8s-ci-robot @soltysh