Add support for AppProtocol as well as service annotations #4603

evankanderson · 2022-06-29T08:17:36Z

name: Pull request
about: Support using AppProtocol to detect backend protocol (pass-through current "tls", "h2", "h2c" values) in addition to projectcontour.io/upstream-protocol.X annotations
labels: ["release-note/small"]

Note that this does not do any particular validation of the AppProtocol field, but it does add tests. 😁

Signed-off-by: Evan Anderson <evana@vmware.com>

codecov · 2022-06-29T19:20:22Z

Codecov Report

Merging #4603 (3dd4269) into main (26aa541) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4603   +/-   ##
=======================================
  Coverage   76.81%   76.81%           
=======================================
  Files         140      140           
  Lines       12993    12995    +2     
=======================================
+ Hits         9980     9982    +2     
  Misses       2751     2751           
  Partials      262      262

Impacted Files	Coverage Δ
internal/dag/accessors.go	`98.59% <100.00%> (+0.02%)`	⬆️
internal/status/gatewaystatus.go	`75.00% <0.00%> (ø)`
internal/status/routeconditions.go	`20.58% <0.00%> (ø)`
internal/status/gatewayclassconditions.go	`95.00% <0.00%> (ø)`

sunjayBhatia · 2022-06-29T19:56:29Z

I think we might want to add some validation here since the AppProtocol field could be a wide range of values and we assume internally that protocol really only is one of tls h2 or h2c

internal/dag/accessors_test.go

evankanderson · 2022-06-29T21:56:23Z

I think we might want to add some validation here since the AppProtocol field could be a wide range of values and we assume internally that protocol really only is one of tls h2 or h2c

This seems to only be used here:
https://github.com/projectcontour/contour/blob/main/internal/envoy/v3/cluster.go#L81

Which only uses certain specific values, and ignores the rest. I suppose we might be able to save a little memory by not copying these strings around.

evankanderson · 2022-06-29T21:56:49Z

(hit the wrong button again)

youngnick

The idea looks good, but I'm unclear how the logic in upstreamProtocol works. If it's just me, maybe a comment to explain it?

youngnick · 2022-07-07T04:16:49Z

internal/dag/accessors.go

@@ -99,6 +99,9 @@ func upstreamProtocol(svc *v1.Service, port v1.ServicePort) string {
 	if protocol == "" {
 		protocol = up[strconv.Itoa(int(port.Port))]
 	}
+	if protocol == "" && port.AppProtocol != nil {


Shouldn't this check be before the previous one? Since the previous one will change the value of protocol if it's empty, and this one expects protocol to be empty, so it will never get hit?

I kept the existing logic using the projectcontour.io/upstream-protocol.XXX annotations as primary overrides.

If the port's Name or Port as a string isn't in the annotation map, protocol will have the default string value (""), and so the first part of the if will be true.

I can add a comment if this default-ness of Go feels too subtle. (You can verify this works in the tests, though I should have added a case with both annotation and AppProtocol to make the priority more clear.)

I preferred the existing annotation to avoid breaking users who had made specific changes to their services for Contour, on the assumption that they knew what they were doing.

Signed-off-by: Evan Anderson <evana@vmware.com>

evankanderson

I added a changelog entry, and clarified that AppProtocol doesn't override the contour-specific Service annotation.

evankanderson · 2022-07-07T23:12:31Z

internal/dag/accessors.go

@@ -99,6 +99,9 @@ func upstreamProtocol(svc *v1.Service, port v1.ServicePort) string {
 	if protocol == "" {
 		protocol = up[strconv.Itoa(int(port.Port))]
 	}
+	if protocol == "" && port.AppProtocol != nil {


I kept the existing logic using the projectcontour.io/upstream-protocol.XXX annotations as primary overrides.

If the port's Name or Port as a string isn't in the annotation map, protocol will have the default string value (""), and so the first part of the if will be true.

I can add a comment if this default-ness of Go feels too subtle. (You can verify this works in the tests, though I should have added a case with both annotation and AppProtocol to make the priority more clear.)

I preferred the existing annotation to avoid breaking users who had made specific changes to their services for Contour, on the assumption that they knew what they were doing.

evankanderson · 2022-07-14T23:36:44Z

(Let me know if I should continue with this, or try a different avenue)

github-actions · 2022-07-29T02:19:01Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 30 days.

sunjayBhatia · 2022-08-17T20:41:05Z

We may want to reconsider whether we want to do this at all since this provision Gateway API GEP implies that appProtocol is a pretty broad field that Gateway API does not intend to use to configure backend app details: kubernetes-sigs/gateway-api#1333 (https://github.com/kubernetes-sigs/gateway-api/pull/1333/files#diff-5ea234dff68be219d19428c1c6cee202f7df6a867a11d294bb16b0dff0c081aeR26-R30)

github-actions · 2022-09-01T00:22:18Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 30 days.

github-actions · 2022-10-01T00:36:22Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

evankanderson requested a review from a team as a code owner June 29, 2022 08:17

evankanderson requested review from tsaarni and youngnick and removed request for a team June 29, 2022 08:17

Add support for AppProtocol as well as service annotations

ba66222

Signed-off-by: Evan Anderson <evana@vmware.com>

evankanderson force-pushed the add-appprotocol branch from 411859f to ba66222 Compare June 29, 2022 08:17

sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Jun 29, 2022

sunjayBhatia reviewed Jun 29, 2022

View reviewed changes

internal/dag/accessors_test.go Show resolved Hide resolved

evankanderson closed this Jun 29, 2022

evankanderson reopened this Jun 29, 2022

youngnick suggested changes Jul 7, 2022

View reviewed changes

Add release note

bdf5d57

Signed-off-by: Evan Anderson <evana@vmware.com>

evankanderson force-pushed the add-appprotocol branch from 8be2b80 to bdf5d57 Compare July 7, 2022 23:09

Add clarifying AppProtocol entry which is overridden by annotation

3dd4269

Signed-off-by: Evan Anderson <evana@vmware.com>

evankanderson commented Jul 7, 2022

View reviewed changes

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2022

carlisia mentioned this pull request Aug 10, 2022

[Upstream] Contour solution for h2c and websocket knative-extensions/net-gateway-api#366

Closed

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 18, 2022

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 1, 2022

skriss mentioned this pull request Sep 26, 2022

Add schedule for staling issues #4752

Merged

github-actions bot closed this Oct 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AppProtocol as well as service annotations #4603

Add support for AppProtocol as well as service annotations #4603

evankanderson commented Jun 29, 2022

codecov bot commented Jun 29, 2022 •

edited

Loading

sunjayBhatia commented Jun 29, 2022

evankanderson commented Jun 29, 2022

evankanderson commented Jun 29, 2022

youngnick left a comment

youngnick Jul 7, 2022

evankanderson Jul 7, 2022

evankanderson left a comment

evankanderson Jul 7, 2022

evankanderson commented Jul 14, 2022

github-actions bot commented Jul 29, 2022

sunjayBhatia commented Aug 17, 2022

github-actions bot commented Sep 1, 2022

github-actions bot commented Oct 1, 2022

Add support for AppProtocol as well as service annotations #4603

Add support for AppProtocol as well as service annotations #4603

Conversation

evankanderson commented Jun 29, 2022

name: Pull request about: Support using AppProtocol to detect backend protocol (pass-through current "tls", "h2", "h2c" values) in addition to projectcontour.io/upstream-protocol.X annotations labels: ["release-note/small"]

codecov bot commented Jun 29, 2022 • edited Loading

Codecov Report

sunjayBhatia commented Jun 29, 2022

evankanderson commented Jun 29, 2022

evankanderson commented Jun 29, 2022

youngnick left a comment

Choose a reason for hiding this comment

youngnick Jul 7, 2022

Choose a reason for hiding this comment

evankanderson Jul 7, 2022

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

evankanderson Jul 7, 2022

Choose a reason for hiding this comment

evankanderson commented Jul 14, 2022

github-actions bot commented Jul 29, 2022

sunjayBhatia commented Aug 17, 2022

github-actions bot commented Sep 1, 2022

github-actions bot commented Oct 1, 2022

name: Pull request
about: Support using AppProtocol to detect backend protocol (pass-through current "tls", "h2", "h2c" values) in addition to projectcontour.io/upstream-protocol.X annotations
labels: ["release-note/small"]

codecov bot commented Jun 29, 2022 •

edited

Loading