-
Couldn't load subscription status.
- Fork 581
feat: support chash on multiple headers #7198
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
feat: support chash on multiple headers #7198
Conversation
| HeaderConsistentHashType ConsistentHashType = "Header" | ||
| // HeadersConsistentHashType hashes based on multiple request headers. | ||
| HeadersConsistentHashType ConsistentHashType = "Headers" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API and implementation between HeadersConsistentHashType and HeaderConsistentHashType looks nearly identical. Should we instead deprecate HeaderConsistentHashType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, that was actually the ask by Arko in the issue description, I was unaware how we would deprecate the HeaderConsistentHashType.
Do we replace HeaderConsistentHashType with the new HeadersConsistentHashType directly in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd mark the API field as deprecated similar to
gateway/api/v1alpha1/policy_helpers.go
Line 34 in 481a2ca
| // Deprecated: use targetRefs/targetSelectors instead |
HeadersConsistentHashType should be used instead as well as including it in the release notes file.
It would stick around in deprecated fashion for the upcoming release and actually removed in a future one (most likely the following release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks a lot for the help <3
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7198 +/- ##
==========================================
- Coverage 71.04% 71.01% -0.03%
==========================================
Files 229 229
Lines 41099 41113 +14
==========================================
Hits 29198 29198
- Misses 10181 10194 +13
- Partials 1720 1721 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/ir/xds.go
Outdated
| // Hash based on the Source IP Address | ||
| SourceIP *bool `json:"sourceIP,omitempty" yaml:"sourceIP,omitempty"` | ||
| Header *Header `json:"header,omitempty" yaml:"header,omitempty"` | ||
| Headers []*Header `json:"headers,omitempty" yaml:"headers,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this layer can you delete Header and copy over the Header API field into Headers ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing so would introduce a breaking change, won't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an internal layer, not user facing, and our libs are /internal too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, please review again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still see Header, suggestion was to delete it to improve maintenance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the confusion. I removed it finally.
| // Header configures the header hash policy when the consistent hash type is set to Header. | ||
| // | ||
| // +optional | ||
| // Deprecated: use Headers instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add the optional tag back
8e3632d to
c82805c
Compare
| name: seventh-route | ||
| route: | ||
| cluster: seventh-route-dest | ||
| hashPolicy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the input file be fixed with headers to fix this output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
This reverts commit 5c1e2f0. Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
Signed-off-by: Shreemaan Abhishek <[email protected]>
1ff0f42 to
c1173f1
Compare
* feat: support chash based on multiple headers Signed-off-by: Shreemaan Abhishek <[email protected]> Signed-off-by: Lin Moskovitch <[email protected]>
What type of PR is this?
Enhancement.
What this PR does / why we need it:
Chash on multiple headers is a legit use case. Example: when user identity is split across multiple headers, X-Tenant-ID + X-User-ID in multi-tenant apps.
Which issue(s) this PR fixes:
Fixes #7186
Release Notes: Yes/No