add helper for determining app protocol

[KauzClay]

Changes

• 🎁 new helper function for determining AppProtocol value

/kind enhancement

In support of knative/serving#14569

Release Note

N/A

Docs

N/A

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a34a5c5) 82.18% compared to head (f03650e) 82.23%.
Report is 5 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

[KauzClay]

Question for reviewers: I ended up creating the variable AppProtocolH2C as a var instead of a const, because I needed to use it in a string pointer. My understanding in go is that you can't take a pointer to a const.

But, given k8s uses string pointers for other stuff, is there maybe a nicer way to do this that I'm not aware of?

[skonto]

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?

[skonto]

Could we somehow make clear that this is gateway api related (via a comment maybe)?

[KauzClay]

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.

[KauzClay]

Could we somehow make clear that this is gateway api related?

Sure, like perhaps calling it GatewayAPIAppProtocolH2C or something?

[KauzClay]

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?

[dprotaso]

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

https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/3726-standard-application-protocols#new-standard-protocols

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)

eg.
https://github.com/knative/docs/blob/dab74765e0f0c60789fe97208aa0b0a1e23eb9cd/code-samples/serving/grpc-ping-go/service.yaml#L12C13-L12C22

[dprotaso]

If we need websocket support we would have to upgrade the runtime contract to define a constants users would use

https://github.com/knative/specs/blob/main/specs/serving/runtime-contract.md#protocols-and-ports

[KauzClay]

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?

[dprotaso]

My other PR (knative/serving#14757) is where that would happen, right?

Yeah

[dprotaso]

/lgtm
/approve

[knative-prow]

[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:

• OWNERS [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment