add helper for determining app protocol #904
Conversation
knative-prow
bot
added
kind/enhancement
size/S
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
==========================================
+ Coverage 82.18% 82.23% +0.05%
==========================================
Files 46 46
Lines 1746 1751 +5
==========================================
+ Hits 1435 1440 +5
Misses 268 268
Partials 43 43 ☔ View full report in Codecov by Sentry. |
* the ServicePort field for AppProtocol is a string pointer
knative-prow
bot
added
size/M
|
Question for reviewers: I ended up creating the variable But, given k8s uses string pointers for other stuff, is there maybe a nicer way to do this that I'm not aware of? |
| @@ -40,6 +40,11 @@ const ( | |||
| ServicePortNameHTTPS = "https" | |||
| ) | |||
|
|
|||
| var ( | |||
| // AppProtocolH2C is the name of the external port of the service for HTTP/2 | |||
| AppProtocolH2C = "kubernetes.io/h2c" | |||
There was a problem hiding this comment.
How about kubernetes.io/ws? I read that Contour has it on by default but that would be implicit so I suspect we still need to label services for consistency no?
There was a problem hiding this comment.
Could we somehow make clear that this is gateway api related (via a comment maybe)?
There was a problem hiding this comment.
How about kubernetes.io/ws? Is there a plan for this?
Ah, good point. I think I may have misinterpretted Dave's sentence on WS here: knative/serving#14569
He says:
For websocket - this in theory should just be on by default (contour toggled it on for Gateway API). There might be other implementations that have this off but let's revisit that when we find one that doesn't
Which when I first read it, I bet I read that last line and thought "oh, I can just skip it and leave it", but on second pass, that doesn't seem right.
There was a problem hiding this comment.
Could we somehow make clear that this is gateway api related?
Sure, like perhaps calling it GatewayAPIAppProtocolH2C or something?
There was a problem hiding this comment.
ah, you know, reading Dave's GEP, maybe we don't need to do anything for WS, since upgrades happen by default?
@dprotaso what were you imagining here?
also, how does one indicate that a Knative Service should be a websocket?
There was a problem hiding this comment.
Sure, like perhaps calling it GatewayAPIAppProtocolH2C or something?
These constants are technically Kubernetes constants - I don't think we should prefix them with the Gateway API
We can link the KEP so it's clear
Honestly, websocket is a weird one cause Contour should have probably not disabled it. For now we can skip it since I'd imagine most implementations support the upgrade flow via HTTP
So we really need to set the h2c protocol when the user specifies it in their port name (the way it's used for grpc services)
There was a problem hiding this comment.
If we need websocket support we would have to upgrade the runtime contract to define a constants users would use
There was a problem hiding this comment.
so, to summarize:
- keep the variable name, but link the KEP in the comment
- don't move forward with WS right now
is that correct?
As far as this comment:
So we really need to set the h2c protocol when the user specifies it in their port name (the way it's used for grpc services)
My other PR (knative/serving#14757) is where that would happen, right?
There was a problem hiding this comment.
My other PR (knative/serving#14757) is where that would happen, right?
Yeah
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, KauzClay 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 |
knative-prow
bot
added
the
approved
Changes
/kind enhancement
In support of knative/serving#14569
Release Note
Docs