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

Add source ip hashing to RequestHash load balancing #4141

Merged

Conversation

sunjayBhatia
Copy link
Member

  • must specify exactly one of header hashing policy or source ip hashing
    in a list element (otherwise ignored and warning generated)
  • cant specify to hash source ip multiple times (otherwise ignored and
    warning generated)

Fixes: #3703

- must specify exactly one of header hashing policy or source ip hashing
in a list element (otherwise ignored and warning generated)
- cant specify to hash source ip multiple times (otherwise ignored and
warning generated)

Fixes: projectcontour#3703

Signed-off-by: Sunjay Bhatia <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia sunjayBhatia requested a review from a team as a code owner October 27, 2021 20:32
@sunjayBhatia sunjayBhatia requested review from tsaarni and skriss and removed request for a team October 27, 2021 20:32
@sunjayBhatia sunjayBhatia added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Oct 27, 2021
@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #4141 (858159f) into main (3ee57ac) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4141      +/-   ##
==========================================
+ Coverage   74.64%   74.71%   +0.06%     
==========================================
  Files         112      112              
  Lines        9722     9741      +19     
==========================================
+ Hits         7257     7278      +21     
+ Misses       2309     2307       -2     
  Partials      156      156              
Impacted Files Coverage Δ
internal/dag/dag.go 96.55% <ø> (ø)
internal/dag/policy.go 94.18% <100.00%> (+0.12%) ⬆️
internal/envoy/v3/route.go 65.56% <100.00%> (+0.48%) ⬆️
internal/featuretests/v3/envoy.go 100.00% <100.00%> (ø)
internal/sorter/sorter.go 98.78% <0.00%> (+1.21%) ⬆️

Signed-off-by: Sunjay Bhatia <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
// load balancing is desired. It must be the only hash option field set,
// otherwise this request hash policy object will be ignored.
// +optional
HashSourceIP bool `json:"hashSourceIP,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

another option would be to put this in a "connection properties" struct to mirror Envoy in case there are other options that could be set in parallel to this in the future, but they do not exist and my guess is Envoy won't really be able to make other options that can be set in parallel to this, rather they would be separate policies in practice

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @sunjayBhatia

Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

Nice work @sunjayBhatia !

@@ -0,0 +1,5 @@
### Source IP hash based load balancing
Copy link
Member

Choose a reason for hiding this comment

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

This changelog note is perfect, thanks @sunjayBhatia!

@sunjayBhatia sunjayBhatia merged commit cc6233e into projectcontour:main Nov 1, 2021
@sunjayBhatia sunjayBhatia deleted the source-ip-request-hash branch November 1, 2021 15:25
skriss added a commit to skriss/contour-operator that referenced this pull request Nov 1, 2021
stevesloka pushed a commit to projectcontour/contour-operator that referenced this pull request Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

session affinity: source ip
3 participants