Added config for socket options for listeners #5352

tsaarni · 2023-05-10T15:14:59Z

This PR adds new optional config field to support DSCP marking for outbound IP packets, for both IPv4 (TOS field) and IPv6 (Traffic Class field).

Fixes #4605

Signed-off-by: Tero Saarni [email protected]

Detailed description

The new configuration is added to existing listener stanza:

Example config file:

listener:
  socket-options:
   tos: 64
   traffic-class: 64

Example ContourConfiguration CR

apiVersion: projectcontour.io/v1alpha1
kind: ContourConfiguration
metadata:
  name: contour
  namespace: projectcontour
spec:
  envoy:
   listener:
    socketOptions:
     tos: 64
     trafficClass: 64

The DSCP field is the 6 most significant bits of the exposed fields. However, the PR proposes exposing the socket options as they are exposed in Envoy and the socket API (complete TOS and TrafficClass bytes). For example, to set DSCP value 16 user needs to bit shift the value to 6 most significant bits of the byte and set the tos and/or trafficClass to value 64.

The PR proposes moving the existing TCP keepalive options into a generic socket option file. If more socket options are added in the future, the new file would be the place to add them - regardless of being TCP or IP level options.

Limitations

Since the two new socket options depend on IP version, the user needs to know in advance which one they want to set.

For example, if the worker nodes where Envoy runs have IPv6 support enabled in the kernel, and Envoy was configured to listen to IPv6 address, then traffic-class can be set. If not, setting traffic-class will cause failure to configure listeners to Envoy. Envoy will call setsockopt() to set IPv6 socket option on a socket that does not support IPv6, the syscall returns error and Envoy rejects the whole listener configuration. Contour cannot validate in advance if this syscall will succeed on the nodes where Envoy runs, so unfortunately it will be runtime failure.

The documentation for the config has a note about this limitation.

codecov · 2023-05-10T15:42:15Z

Codecov Report

Merging #5352 (f616af2) into main (aa18b98) will increase coverage by 0.03%.
The diff coverage is 94.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5352      +/-   ##
==========================================
+ Coverage   78.56%   78.59%   +0.03%     
==========================================
  Files         138      138              
  Lines       19101    19150      +49     
==========================================
+ Hits        15006    15051      +45     
- Misses       3809     3812       +3     
- Partials      286      287       +1

Files Changed	Coverage Δ
cmd/contour/serve.go	`20.26% <0.00%> (-0.03%)`	⬇️
pkg/config/parameters.go	`85.80% <72.72%> (-0.46%)`	⬇️
cmd/contour/servecontext.go	`83.55% <100.00%> (+0.14%)`	⬆️
internal/envoy/v3/listener.go	`98.48% <100.00%> (ø)`
internal/envoy/v3/socket_options.go	`100.00% <100.00%> (ø)`
internal/envoy/v3/stats.go	`100.00% <100.00%> (ø)`
internal/featuretests/v3/envoy.go	`99.06% <100.00%> (ø)`
internal/xdscache/v3/listener.go	`92.05% <100.00%> (+0.15%)`	⬆️

github-actions · 2023-05-25T00:19:05Z

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

github-actions · 2023-06-09T00:23:15Z

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

github-actions · 2023-06-25T00:27:14Z

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

tsaarni · 2023-06-30T08:15:41Z

I've now resolved merge conflicts that had appeared during recent changes in main. The PR is now again ready for review.

github-actions · 2023-07-15T00:24:06Z

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

github-actions · 2023-08-04T00:21:14Z

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

tsaarni · 2023-08-04T17:04:23Z

Catching up with some more merge conflicts. These changes seem to be on a path that is bit prone to merge conflicts over the time, so reviews would be appreciated! 😄 🙏

sunjayBhatia

overall looks correct from a quick pass over the code, seems straightforward enough to set some socket options on the Envoy Listeners

should maybe add a featuretest in lieu of an e2e test to make sure config gets plumbed all the way through correctly

this looks like it will intersect a bit with #5523 but I can sort that out if this merges first 👍🏽

sunjayBhatia · 2023-08-04T17:13:43Z

pkg/config/parameters.go

+}
+
+// SocketOptions defines configurable socket options for Envoy listeners.
+type SocketOptions struct {


(w/o knowing the details of TOS vs. Traffic Class) is it valid to auto detect the Listener ip family to be able to collapse these two into one field?

Reliably auto-detecting could be challenging when listener address is set as ::. We use Ipv4Compat: true so Envoy accepts also IPv4 clients - unless IPv4 is disabled on the host where Envoy is running, I think.

ah fair point, if we wanted to be clever could we maybe do something like "only have one new API field and if listen address == :: then set the ipv4 and ipv6 option with that value, otherwise set the appropriate option for the address family"?

if in practice users may want to set different values between ipv4 and ipv6 seems more correct to keep these options/configurable values separate

tsaarni · 2023-08-04T18:11:46Z

Thank You for review @sunjayBhatia!

should maybe add a featuretest in lieu of an e2e test to make sure config gets plumbed all the way through correctly

So far, I've checked the values manually from IP header using Wireshark. It would be great to have e2e test, but I think it would require a test client that processes HTTP responses at IP packet level. Normally the values are processed only by the routers & host IP stacks, but the receiving application is unaware of the values.

Did you have some other approach in mind for e2e?

sunjayBhatia · 2023-08-04T19:42:31Z

Thank You for review @sunjayBhatia!

should maybe add a featuretest in lieu of an e2e test to make sure config gets plumbed all the way through correctly

So far, I've checked the values manually from IP header using Wireshark. It would be great to have e2e test, but I think it would require a test client that processes HTTP responses at IP packet level. Normally the values are processed only by the routers & host IP stacks, but the receiving application is unaware of the values.
Did you have some other approach in mind for e2e?

yeah totally agree, I think just an additional explicit test in internal/featuretest (unless I missed it is already there) for the new socket options added in this PR to make sure the config is generated properly when all the different internal components are hooked up should be sufficient on top of that validation you've done

tsaarni · 2023-08-07T13:49:10Z

Sorry @sunjayBhatia, somehow, I've accidentally managed to edit your reply instead of replying to you. Maybe I picked "edit" inadvertently, instead of "quote reply" 😵‍💫

Well, regardless of that confusion, I've now added test in internal/featuretests/ to validate the new socket options. I still left also internal/envoy/v3/socket_options_test.go, although it is bit redundant now.

sunjayBhatia

Looks like some conflicts to resolve and just to close out this thread #5352 (comment)

Otherwise 👍🏽

New config field was added to support DSCP marking for outbound traffic, for both IPv4 (TOS field) and IPv6 (Traffic Class field). Signed-off-by: Tero Saarni <[email protected]>

Signed-off-by: Tero Saarni <[email protected]>

tsaarni requested a review from a team as a code owner May 10, 2023 15:15

tsaarni requested review from stevesloka and skriss and removed request for a team May 10, 2023 15:15

tsaarni added the release-note/small A small change that needs one line of explanation in the release notes. label May 10, 2023

tsaarni force-pushed the dscp-marking branch from 5774ac6 to 03233ec Compare May 10, 2023 15:22

tsaarni added release-note/small A small change that needs one line of explanation in the release notes. and removed release-note/small A small change that needs one line of explanation in the release notes. labels May 10, 2023

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

skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 25, 2023

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

tsaarni removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 10, 2023

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

tsaarni force-pushed the dscp-marking branch from 03233ec to b81b1da Compare June 30, 2023 06:43

tsaarni removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 30, 2023

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

sunjayBhatia self-requested a review July 19, 2023 18:26

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

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

tsaarni force-pushed the dscp-marking branch from b81b1da to 7ec2aad Compare August 4, 2023 14:31

sunjayBhatia removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 4, 2023

sunjayBhatia requested a review from clayton-gonsalves August 4, 2023 17:03

sunjayBhatia reviewed Aug 4, 2023

View reviewed changes

sunjayBhatia reviewed Aug 14, 2023

View reviewed changes

Added config for socket options for listeners

f239572

New config field was added to support DSCP marking for outbound traffic, for both IPv4 (TOS field) and IPv6 (Traffic Class field). Signed-off-by: Tero Saarni <[email protected]>

tsaarni force-pushed the dscp-marking branch from 37aa160 to 7299332 Compare August 15, 2023 10:40

Added featuretest for non-default socket opts

f616af2

Signed-off-by: Tero Saarni <[email protected]>

tsaarni force-pushed the dscp-marking branch from 7299332 to f616af2 Compare August 15, 2023 17:38

sunjayBhatia approved these changes Aug 15, 2023

View reviewed changes

sunjayBhatia merged commit e508a4a into projectcontour:main Aug 15, 2023
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added config for socket options for listeners #5352

Added config for socket options for listeners #5352

tsaarni commented May 10, 2023 •

edited

Loading

codecov bot commented May 10, 2023 •

edited

Loading

github-actions bot commented May 25, 2023

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 25, 2023

tsaarni commented Jun 30, 2023

github-actions bot commented Jul 15, 2023

github-actions bot commented Aug 4, 2023

tsaarni commented Aug 4, 2023

sunjayBhatia left a comment

sunjayBhatia Aug 4, 2023

tsaarni Aug 4, 2023

sunjayBhatia Aug 7, 2023 •

edited

Loading

sunjayBhatia Aug 7, 2023

tsaarni commented Aug 4, 2023 •

edited

Loading

sunjayBhatia commented Aug 4, 2023 •

edited by tsaarni

Loading

tsaarni commented Aug 7, 2023

sunjayBhatia left a comment

Added config for socket options for listeners #5352

Added config for socket options for listeners #5352

Conversation

tsaarni commented May 10, 2023 • edited Loading

codecov bot commented May 10, 2023 • edited Loading

Codecov Report

github-actions bot commented May 25, 2023

github-actions bot commented Jun 9, 2023

github-actions bot commented Jun 25, 2023

tsaarni commented Jun 30, 2023

github-actions bot commented Jul 15, 2023

github-actions bot commented Aug 4, 2023

tsaarni commented Aug 4, 2023

sunjayBhatia left a comment

Choose a reason for hiding this comment

sunjayBhatia Aug 4, 2023

Choose a reason for hiding this comment

tsaarni Aug 4, 2023

Choose a reason for hiding this comment

sunjayBhatia Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

sunjayBhatia Aug 7, 2023

Choose a reason for hiding this comment

tsaarni commented Aug 4, 2023 • edited Loading

sunjayBhatia commented Aug 4, 2023 • edited by tsaarni Loading

tsaarni commented Aug 7, 2023

sunjayBhatia left a comment

Choose a reason for hiding this comment

tsaarni commented May 10, 2023 •

edited

Loading

codecov bot commented May 10, 2023 •

edited

Loading

sunjayBhatia Aug 7, 2023 •

edited

Loading

tsaarni commented Aug 4, 2023 •

edited

Loading

sunjayBhatia commented Aug 4, 2023 •

edited by tsaarni

Loading