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

[GEP-1911] websocket backend protocol conformance #2495

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

dprotaso
Copy link
Contributor

/kind test
/area conformance

What this PR does / why we need it:

This PR adds a conformance test validating implementations support websocket over cleartext when the target Kubernetes Service Service Port has a kubernetes.io/ws appProtocol

This PR includes echo-basic changes so it will need to bumped after this PR

Which issue(s) this PR fixes:

Related #1911 #205

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 19, 2023
@dprotaso dprotaso changed the title [GEP-1911] websocket backend support [GEP-1911] websocket backend protocol conformance Oct 19, 2023
@dprotaso
Copy link
Contributor Author

/assign @arkodg @mlavacca @sunjayBhatia

@arkodg
Copy link
Contributor

arkodg commented Oct 20, 2023

this PR looks good, lets wait for #2456 to land first, since this PR will need a rebase

@mlavacca
Copy link
Member

/cc @mlavacca

@k8s-ci-robot k8s-ci-robot requested a review from mlavacca October 24, 2023 12:54
conformance/echo-basic/echo-basic.go Show resolved Hide resolved
conformance/tests/httproute-backend-protocol-ws.yaml Outdated Show resolved Hide resolved
protocol: TCP
appProtocol: kubernetes.io/ws
port: 8082
targetPort: 3000
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to use the same targetPort for both TCP and WS traffic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is your concern about using the same targetPort?

Copy link
Member

Choose a reason for hiding this comment

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

I have no real concern, I was just wondering if this choice was backed by a specific reason. If not, I think we should prefer not to use the same back-side port for multiple front-side ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no real concern, I was just wondering if this choice was backed by a specific reason. I think we should prefer not to use the same back-side port for multiple front-side ports.

Multiplexing HTTP/Websockets on the same port is really common. The only reason why I created a separate ServicePort with a unique port # is because of the following K8s limitation that's being tracked here - kubernetes/kubernetes#118407

Copy link
Member

Choose a reason for hiding this comment

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

ok, sounds good to me 👍

Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

Thanks @dprotaso, ltgm!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, mlavacca

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 Oct 24, 2023
@arkodg
Copy link
Contributor

arkodg commented Oct 24, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9c830e5 into kubernetes-sigs:main Oct 24, 2023
3 checks passed
@dprotaso dprotaso deleted the gep-1911-ws branch October 26, 2023 19:06
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/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants