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

Truncate SessionAffinity timeout values instead of wrapping around #3609

Conversation

antoninbas
Copy link
Contributor

SessionAffinity timeout is implemented using a hard_timeout in
OVS. hard_timeout is represented by a uint16 in the OpenFlow protocol,
hence we cannot support timeouts greater than 65535 seconds. However,
the K8s Service spec allows timeout values up to 86400 seconds for
ClientIP-based SessionAffinity
(https://godoc.org/k8s.io/api/core/v1#ClientIPConfig). For values
greater than 65535 seconds, we need to set the hard_timeout to 65535
rather than let the timeout value wrap around.

For #1578

Signed-off-by: Antonin Bas abas@vmware.com

@antoninbas antoninbas added area/ovs/openflow Issues or PRs related to Open vSwitch Open Flow. area/ovs Issues or PRs related to OVS area/proxy Issues or PRs related to proxy functions in Antrea action/release-note Indicates a PR that should be included in release notes. labels Apr 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2022

Codecov Report

Merging #3609 (1ebec42) into main (7fea8d3) will decrease coverage by 17.39%.
The diff coverage is 3.70%.

❗ Current head 1ebec42 differs from pull request most recent head c3229ed. Consider uploading reports for the commit c3229ed to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3609       +/-   ##
===========================================
- Coverage   64.24%   46.84%   -17.40%     
===========================================
  Files         278      245       -33     
  Lines       27929    35482     +7553     
===========================================
- Hits        17942    16623     -1319     
- Misses       8067    17221     +9154     
+ Partials     1920     1638      -282     
Flag Coverage Δ
e2e-tests 46.84% <3.70%> (?)
kind-e2e-tests ?
unit-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/proxy/proxier.go 47.64% <3.70%> (-13.56%) ⬇️
pkg/ipfix/ipfix_intermediate.go 0.00% <0.00%> (-89.48%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 4.58% <0.00%> (-86.85%) ⬇️
pkg/ipfix/ipfix_collector.go 0.00% <0.00%> (-81.82%) ⬇️
pkg/agent/util/iptables/lock.go 0.00% <0.00%> (-81.82%) ⬇️
pkg/flowaggregator/certificate.go 0.00% <0.00%> (-77.48%) ⬇️
pkg/controller/externalippool/validate.go 0.00% <0.00%> (-76.20%) ⬇️
...lowaggregator/clickhouseclient/clickhouseclient.go 0.00% <0.00%> (-75.84%) ⬇️
pkg/cni/client.go 0.00% <0.00%> (-75.52%) ⬇️
pkg/controller/networkpolicy/crd_utils.go 14.48% <0.00%> (-75.42%) ⬇️
... and 269 more

jianjuns
jianjuns previously approved these changes Apr 9, 2022
@antoninbas
Copy link
Contributor Author

/test-all

tnqn
tnqn previously approved these changes Apr 12, 2022
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, a typo

// let the timeout value wrap around.
affinityTimeout = maxSupportedAffinityTimeout
if !ok || (svcInfo.StickyMaxAgeSeconds() != pSvcInfo.StickyMaxAgeSeconds()) {
// We only log a warning when the Service hasn't been instealled
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// We only log a warning when the Service hasn't been instealled
// We only log a warning when the Service hasn't been installed

wenyingd
wenyingd previously approved these changes Apr 12, 2022
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM

SessionAffinity timeout is implemented using a hard_timeout in
OVS. hard_timeout is represented by a uint16 in the OpenFlow protocol,
hence we cannot support timeouts greater than 65535 seconds. However,
the K8s Service spec allows timeout values up to 86400 seconds for
ClientIP-based SessionAffinity
(https://godoc.org/k8s.io/api/core/v1#ClientIPConfig). For values
greater than 65535 seconds, we need to set the hard_timeout to 65535
rather than let the timeout value wrap around.

For antrea-io#1578

Signed-off-by: Antonin Bas <abas@vmware.com>
@antoninbas antoninbas dismissed stale reviews from wenyingd, tnqn, and jianjuns via f6c2a3d April 12, 2022 18:12
@antoninbas antoninbas force-pushed the truncate-service-session-affinity-timeout-instead-of-wrapping-it-around branch from c3229ed to f6c2a3d Compare April 12, 2022 18:12
@antoninbas antoninbas requested a review from tnqn April 12, 2022 18:12
@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas antoninbas merged commit 1d7a017 into antrea-io:main Apr 13, 2022
@antoninbas antoninbas deleted the truncate-service-session-affinity-timeout-instead-of-wrapping-it-around branch April 13, 2022 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/ovs/openflow Issues or PRs related to Open vSwitch Open Flow. area/ovs Issues or PRs related to OVS area/proxy Issues or PRs related to proxy functions in Antrea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants