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 - Backend Protocol Selection #1979

Merged
merged 39 commits into from
Sep 27, 2023

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Apr 27, 2023

What type of PR is this?

/kind gep

What this PR does / why we need it:

This PR flushes out how app developers can specify that their application supports different protocols

Which issue(s) this PR fixes:

Fixes: #1911 (Backend Protocol GEP)
Fixes: #205 (Websocket Support)

Does this PR introduce a user-facing change?:

Allow end-users to specify backend protocols

@k8s-ci-robot k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 27, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 27, 2023
geps/gep-1911.md Outdated Show resolved Hide resolved
@arkodg
Copy link
Contributor

arkodg commented Apr 27, 2023

+1 for adding an API / API field to be explicit about backend/upstream protocol

since this PR suggests a HOW , will share my preference, which would be to keep it close to the
backendRef

protocols: []Protocol

e.g.

- backendRefs:
    - name: foo-v1
      port: 8080
      protocols:
      - HTTP
      - PROXYPROTOCOL
      weight: 90

relates to envoyproxy/gateway#1328

@dprotaso
Copy link
Contributor Author

since this PR suggests a HOW , will share my preference, which would be to keep it close to the
backendRef

The only thing with the list of protocol strings is that it doesn't enable the goal I set out - Extensible - additional protocol specific fields should be easy to add in the future

This is why rather than having an h2c constant I added a Cleartext field under HTTP2PortProtocol.

I'm not super opinionated about this and welcome discussion.

geps/gep-1911.md Outdated Show resolved Hide resolved
@arkodg
Copy link
Contributor

arkodg commented Apr 27, 2023

since this PR suggests a HOW , will share my preference, which would be to keep it close to the
backendRef

The only thing with the list of protocol strings is that it doesn't enable the goal I set out - Extensible - additional protocol specific fields should be easy to add in the future

This is why rather than having an h2c constant I added a Cleartext field under HTTP2PortProtocol.

I'm not super opinionated about this and welcome discussion.

good point, a struct is better if we are envisioning customizing the backend protocol functionality.
I was taking the concept of upstream protocol https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.ProtocolType, converting it into a list and reusing it here

geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
@robscott
Copy link
Member

robscott commented May 3, 2023

@dprotaso We discussed this at the community meeting yesterday. Here are some rough notes:

  1. I think we should really consider the list of AppProtocols proposed for upstream Kubernetes as a solution to this specific problem
  2. If/when Service includes a list of AppProtocols, that would likely be propagated to ServiceImport since that API is just mirroring Service.

@LiorLieberman has been working on the upstream AppProtocol changes and has also been commenting here. Would be helpful to understand what we'd still be missing if AppProtocol became a list on the Service API.

Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

Some thoughts on the table caveats, but aside from that this looks pretty good to me.

I've specifically not addressed the open questions at this time because I think we should all take a bit and think about them before doing anything. Let's get this merged at Provisional first.

geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
Copy link
Member

@robscott robscott 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 all the work on this @dprotaso!

geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
Copy link
Member

@robscott robscott 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, didn't go all the way through it, but I think I saw most of your updates.

geps/gep-1911.md Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
geps/gep-1911.md Outdated Show resolved Hide resolved
@dprotaso
Copy link
Contributor Author

@youngnick @robscott Can you take another look? 🙇

@youngnick
Copy link
Contributor

/lgtm

/hold

for @robscott's review though.

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

@robscott robscott 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! One non-blocking nit. Feel free to remove hold and catch this in a follow up.

/lgtm

geps/gep-1911.md Outdated Show resolved Hide resolved
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2023
@robscott robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 27, 2023
@robscott
Copy link
Member

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 27, 2023
@k8s-ci-robot k8s-ci-robot merged commit 789326b into kubernetes-sigs:main Sep 27, 2023
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEP: Backend Protocol Selection WebSocket protocol support