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

KEP-3726: Add 'kubernetes.io/raw' as a standard app protocol #4106

Closed
wants to merge 2 commits into from

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Jun 19, 2023

This PR is a continuation of #3996 - simplified diff is here 93d0269

  • One-line PR description: This defines kubernetes.io/raw as a standard app protocol

This is completing the follow up work outlined in the KEP-3726 update introduce here - #3912

Specifically

Some implementations are trying to guess the application protocol in absence of appProtocol. We should consider adding a new protocol like kubernetes.io/raw to the collection that would instructs implementations to process requests as raw. We need to look at combination of appProtocol: kubernetes.io/raw and the different supported port protocols (TCP, UDP, SCTP).

Motivation

Gateway API GEP - kubernetes-sigs/gateway-api#1911

Problem: How can users specify their traffic to a service/endpointslice port is exactly what's specified in protocol and automatic detection should be turned off.

I saw that the standard protocol KEP had a clause about raw - but it wasn't added.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 19, 2023
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 19, 2023
@shaneutt
Copy link
Member

For posterity, could we please get more of the context pulled in from the related bits into the PR description here?

@dprotaso
Copy link
Contributor Author

/assign @shaneutt @robscott @thockin

@dprotaso
Copy link
Contributor Author

For posterity, could we please get more of the context pulled in from the related bits into the PR description here?

Done

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Can we say what is the motivating case for this?

@@ -180,6 +181,7 @@ type ServicePort struct {
// * 'kubernetes.io/h2c' - HTTP/2 over cleartext as described in https://www.rfc-editor.org/rfc/rfc7540
// * 'kubernetes.io/ws' - WebSocket over cleartext as described in https://www.rfc-editor.org/rfc/rfc6455
// * 'kubernetes.io/wss' - WebSocket over TLS as described in https://www.rfc-editor.org/rfc/rfc6455
// * 'kubernetes.io/raw' - Traffic to this port is the protocol value defined by this port's `protocol` field. (ie. TCP/UDP/SCTP)
Copy link
Member

Choose a reason for hiding this comment

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

Traffic to any port is the protocol defined by protocol. That's tautological. How about something like:

"Opaque traffic which should not be handled inspected further"

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not quite right either though @thockin. We still want the node OS (or container runtime sandbox, if it's something like gVisor) to process the lower layer stuff. But there's no application protocol on top.

I don't want anyone to misconstrue this as an opt-out from their security appliance and packet / flow inspection.

How about

Suggested change
// * 'kubernetes.io/raw' - Traffic to this port is the protocol value defined by this port's `protocol` field. (ie. TCP/UDP/SCTP)
// * 'kubernetes.io/raw' - Opaque application traffic; implementations should not apply special processing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok updated. PTAL

@dprotaso
Copy link
Contributor Author

Can we say what is the motivating case for this?

Gateway API GEP - kubernetes-sigs/gateway-api#1911

How can users specify their traffic to a service/endpointslice port is exactly what's specified in protocol and automatic detection should be turned off.

Then I saw that the standard protocol KEP had a clause about raw

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dprotaso
Once this PR has been reviewed and has the lgtm label, please ask for approval from shaneutt. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@thockin
Copy link
Member

thockin commented Jul 12, 2023

I meant we should add text here explaining that sometimes implementations assume they can "sniff" the protocol and sometimes that causes problems and we don't want them to.

What I am struggling with is why this is generic API and not "hey <impl>, don't be stupid" ? Why would a user WANT to specify "raw" that doesn't boil down to "I know something about my impl that needs me to say work around it"?

@sftim
Copy link
Contributor

sftim commented Jul 12, 2023

Maybe kubernetes.io/opaque is a better name?

You might want to validate at admission time that an app protocol has been declared I guess. Not sure how a null value here is different from opaque though.

@dprotaso
Copy link
Contributor Author

dprotaso commented Jul 12, 2023

disclosure: not an ingress implementator

For the Gateway GEP I could suggest that if the appProtocol is not set then assume the target will accept traffic as defined by protocol and don't perform sniffing. But my guess is most users do not set appProtocol so there is a advantage to sniffing.

Thus having some constant to suggest that protocol should be interpreted as absolute is what I'm trying to do.

If we feel that this doesn't belong as a k8s standard protocol I can bring this to the Gateway folks for a discussion.

@howardjohn
Copy link

howardjohn commented Jul 12, 2023 via email

@thockin
Copy link
Member

thockin commented Jul 12, 2023

What I am asking is - why is sniffing ever bad? Is that implementation-centric or actually fundamental? It feels like "raw" is a way to express "I know I am using istio, and I know istio is broken in this specific way, so let's disable that".

@howardjohn
Copy link

What I am asking is - why is sniffing ever bad? Is that implementation-centric or actually fundamental? It feels like "raw" is a way to express "I know I am using istio, and I know istio is broken in this specific way, so let's disable that".

Ah, good question. The main issue is that for some protocols (mysql, most commonly), its entirely broken: https://istio.io/latest/docs/ops/deployment/requirements/#server-first-protocols.

There are also some more niche use cases where you already know its not HTTP so you why bother with the complexity, but I think that is mostly hypothetical

@thockin
Copy link
Member

thockin commented Jul 12, 2023

So, in this case it really is "I know something about my implementation". But why do we want "raw" instead of "mysql" ?

@howardjohn
Copy link

So, in this case it really is "I know something about my implementation". But why do we want "raw" instead of "mysql" ?

We could, but mysql is just one common case. I have seen maybe 10 different protocols that are "server first" in my experience, and I am sure there are more - so it can be challenging to keep up with.

@thockin
Copy link
Member

thockin commented Jul 12, 2023

I have seen maybe 10 different protocols that are "server first" in my experience, and I am sure there are more - so it can be challenging to keep up with.

So assume I am a user who invents a new, bespoke protocol which is server-first. Istio obviously doesn't support me. If I am using istio and sniffing breaks, should I specify kubernetes.io/raw or istio.io/dont-sniff ? Whats the value in a "common" name for this? I don't think we expect people to set it by default on every port, right?

@howardjohn
Copy link

So assume I am a user who invents a new, bespoke protocol which is server-first. Istio obviously doesn't support me. If I am using istio and sniffing breaks, should I specify kubernetes.io/raw or istio.io/dont-sniff ? Whats the value in a "common" name for this? I don't think we expect people to set it by default on every port, right?

You could and that would work fine, but its not really portable which seems to be the goal of all of the protocol definitions, right?

You could do istio.io/http, but there is value in telling everyone its "HTTP".

Similarly for raw, I may want to tell linkerd, kuma, etc as well. A single user may not really care (you probably only use one mesh), but a common helm chart may not want to use a vendor specific label for example.

Granted, I could make the same argument about almost any appProtocol value, and its a bit of a slippery slope...

@thockin
Copy link
Member

thockin commented Jul 12, 2023

Similarly for raw, I may want to tell linkerd, kuma, etc as well.

If we define "standard" raw, and then some implementations get better at sniffing, we've created an attractive nuisance. Premature pessimization.

"raw" is different from the rest - the others are affirmative statements, "raw" is not telling us what it IS, it's a bypass. That's why I am unsure

@dprotaso
Copy link
Contributor Author

dprotaso commented Jul 12, 2023

"raw" is not telling us what it IS,

My original definition was that "raw" meant look at the protocol field and don't assume anything else. That to me is affirmative and by-passing sniffing is a side-effect for some implementations.

I think an alternative would be to specify IANA names for TCP, UDP, etc in the appProtocol field - but are we ok with the duplicate values for protocol and appProtocol?. This question might be more for the Gateway GEP and not this KEP.

@aojea
Copy link
Member

aojea commented Jul 12, 2023

raw does not look something as a standard app protocol honestly, why not empty means raw?

@sftim
Copy link
Contributor

sftim commented Jul 12, 2023

raw does not look something as a standard app protocol honestly, why not empty means raw?

The challenge is to tell apart empty-by-intent from empty-by-default. Lots of clusters will have objects with no value specified for this field.

@thockin
Copy link
Member

thockin commented Jul 13, 2023

My original definition was that "raw" meant look at the protocol field and don't assume anything else

The absence of an AppProtocol really means "I have nothing further to express here". I should not need to explicitly say "this page intentionally left blank". It's implementations which are choosing to try to be clever (and sometimes failing). The desire to express "raw" derives from implementation(s) which fail to cover their tracks.

We're telling the end user "it's not your fault, but it is your problem -- you have to take action to clean up our mess". Yuck.

I'm not against "raw", I just think it shouldn't be "kubernetes.io/raw". If mesh implementations want to agree on a common name, that's outside my purvey.

@sftim
Copy link
Contributor

sftim commented Jul 13, 2023

I'm inferring that mesh vendors might agree on mesh-consortium.example/opaque and document the bodge to their users.

@aojea
Copy link
Member

aojea commented Jul 13, 2023

We're telling the end user "it's not your fault, but it is your problem -- you have to take action to clean up our mess". Yuck.

sorry, I have to make the joke 🥲 kubernetes.io/mess

@thockin
Copy link
Member

thockin commented Jul 14, 2023 via email

@thockin
Copy link
Member

thockin commented Jul 14, 2023

So after all this discussion, I think we say thanks, but no thanks to this one, right?

@thockin thockin closed this Jul 14, 2023
@dprotaso
Copy link
Contributor Author

Sounds good

@dprotaso dprotaso deleted the raw-protocol branch July 14, 2023 15:43
dprotaso added a commit to dprotaso/gateway-api that referenced this pull request Aug 2, 2023
Upstream K8s folks did not want to add this constant.

See: kubernetes/enhancements#4106
k8s-ci-robot pushed a commit to kubernetes-sigs/gateway-api that referenced this pull request Sep 27, 2023
* Initial draft of GEP-1911 allowing end-users to specify backend
protocols

* fix tab vs spaces

* indicate that per-protocol structs can be extended in the future

* Add small intro

* Address Candace's feedback

* add an option to consider re-using Route resources

* expand appProtocol based on findings

* how would we configure certain protocols via appProtocol

* rewrite doc to focus on supporting Kubernetes Standard Application Protocols

* fix link

* add back multiple protocols on the same port

* change multiple protocol example to TCP/UDP which is what K8s supports

* fix links

* move alternate API under a detail block so people don't confuse that as the proposed API

* add a compatability table

* fix links

* Drop raw support

Upstream K8s folks did not want to add this constant.

See: kubernetes/enhancements#4106

* tweak default protocol clause

* tweak tables so columns are the same

* limit multiplex protocols to just those supported by K8s (UDP/TCP for now)

* tweak new appProtocol section - include an example of using a gateway api special prefix

* label open questions explicitly

* three kubernetes.io appProtocols

* incompatible backend protocols make the backend invalid

* add godoc to HTTPBackendRef explaining and invalid backend

* drop alternative example because people keep commenting on it

* Drop alternative mechanic that's used in GAMMA

* address Nicks comments

* add references to Google and AWS's approach to the problem

* TLSConnectionPolicy => BackendTLSPolicy

* address Rob's feedback

* drop SCTP multiplexing question

* absence of appProtocol doesn't mean implementations disable features

* multiplexing UDP/TCP is optional

* TLSRoute paired to non-TLS listener doesn't make sense

* fix casing on some section titles

* specific protocol support is optional

* Update geps/gep-1911.md

Co-authored-by: Rob Scott <rob.scott87@gmail.com>

---------

Co-authored-by: Rob Scott <rob.scott87@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/network Categorizes an issue or PR as relevant to SIG Network. 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

8 participants