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 support for header rewrite as hash key #3180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ramaraochavali
Copy link
Contributor

@ramaraochavali ramaraochavali commented May 8, 2024

We have a use case to pick the rewritten header using regex when header based consistent hash is used. Envoy supports this. It is very difficult to craft an Envoy filter for this (need to replace the entire virtual host)

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali requested a review from a team as a code owner May 8, 2024 12:00
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 8, 2024
@istio-testing
Copy link
Collaborator

@ramaraochavali: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
release-notes_api 0413bfc link false /test release-notes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link
Contributor

@costinm costinm left a comment

Choose a reason for hiding this comment

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

Link to envoy feature ? Is it marked 'stable' in envoy ?

Since this is a change to a 'v1' API - not sure what is the process to add new fields, are they automatically at the same level of support as the rest ?

And for all 'regex' it is important to include more info ( maybe from envoy ) - what flavor ? Maybe some description of the use case - many http headers are structured and not friendly to regex.

Since it's unlikely to be a very common use case - perhaps improving EnvoyFilter to make it easier to patch would be a better approach, or using WASM ? I wouldn't mind it too much if it didn't involve regex.

@costinm
Copy link
Contributor

costinm commented May 8, 2024

@whitneygriffith

@ramaraochavali
Copy link
Contributor Author

@whitneygriffith
Copy link
Contributor

whitneygriffith commented May 8, 2024

TLDR to add a new field to a v1 API:

  • Add // +cue-gen:DestinationRule:releaseChannel:extended tag to the proto definition similar to what was done here to all API versions.
  • Update the stable validation policy to exclude the new field in the following manifests: base chart and istio-discovery chart

CC: @keithmattix

@howardjohn
Copy link
Member

Does it not work in envoy to do a normal header rewrite and use that as a key?

@ramaraochavali
Copy link
Contributor Author

Does it not work in envoy to do a normal header rewrite and use that as a key?

What do you mean by normal header rewrite?

@howardjohn
Copy link
Member

howardjohn commented May 9, 2024 via email

@ramaraochavali
Copy link
Contributor Author

No. We need just rewrite the header using regex capture groups (specifically path header) and use it as hash key but not rewrite the value of the header. Also there is no generic regex rewrite for headers in VS?

@howardjohn
Copy link
Member

I see. I worry this usage is extraordinarily niche. Consistent hash AND rewrite AND regex capture... this really feels like something best outside of core of Istio

@ramaraochavali
Copy link
Contributor Author

I also tried to solve this with Envoy filter but the filter means we need to replace the entire vhost/route which is not ideal.

@howardjohn
Copy link
Member

Maybe you can insert some custom filter that does some manipulation before the hashing runs

@costinm
Copy link
Contributor

costinm commented May 9, 2024

My suggestion is to see if we can improve EnvoyFilter to make it easy to support this case.

Did anyone check the EnvoyGateway equivalent of EnvoyPatchPolicy - basically json patch ? Would something like that work for this case ( so we don't reinvent the wheel ) ?

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 20, 2024
@istio-testing
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR needs to be rebased before being merged size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants