-
Notifications
You must be signed in to change notification settings - Fork 471
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
design 10574: transformations #10689
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Jenny Shu <28537278+jenshu@users.noreply.github.com>
design/10574.md
Outdated
|
||
|
||
## Alternatives | ||
Port the existing Transformation from the old project and figure out how to migrate the custom envoy to github actions. |
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.
migrate the custom envoy to github actions
can you clarify what this means? i.e. getting CI for a custom envoy build working?
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.
Does the update better answer?
Given this the first addition here should contain the following functions: | ||
"base64_encode", "header", "replace_with_random" |
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.
Is the implication here that these functions need to built directly in the dynamic module on top of the functionality in the inja replacement?
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.
Yep. As seen in the poc pr where we added header and substring as examples.
The other nice thing is that the previous transformation setup needed explicit access to Thread local storage due to how the inja library we used was setup. Now can use the dynamic module's memory given configuration setup not polluting.
So for example https://github.com/kgateway-dev/kgateway/pull/10677/files#diff-1992c8adc1fc21e3baa436d3ffd5a91d47311816cde7bad0d8785bbbc4865aeeR152
let rendered = tmpl.render(context!(headers => headers)); is called to render via inja with header info smashed into the inja context and the accessor function "header" is added at filter creation via env.add_function("header", header); which knows to ask for the inja state's headers which we inject in that context.
let headers = state.lookup("headers");
* Have full feature parity with gloo transformations (at first) | ||
* Keep the same user facing api as gloo | ||
|
||
## Implementation Details |
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.
IMO it's worth elaborating on the library that is planned to be used (including links).
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.
Updated and added as the dependency sub tag. Felt that it was better to talk about after the user facing api is discussed.
design/10574.md
Outdated
* Keep the same user facing api as gloo | ||
|
||
## Implementation Details | ||
As part of this we should look at leveraging the quickly stabilizing [Dynamic Module support](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/dynamic_modules) to reduce overhead in the repository and to let us have a potential move to using raw envoy releases as the default standard for kgateway builds. An example of how we might start to do this can be found in [poc](https://github.com/kgateway-dev/kgateway/pull/10677). This would allow for side by side of old and new functionality to make sure that as users migrate to kgateway 2 we can in the near term flip an os flag to find any gaps between the new transformation approach and the older approach that is based on the envoy-gloo repo. |
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.
Might be worth breaking this line up into paragraphs or just newline separated sentences to allow for reviewing.
I think it would be great to expand on the planned semantics of the 'flag' to switch between modes and how/if that is relevant to the consumer of the new transformation API
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.
Added more details and an example migration plan for a piece that is out of scope for now (upstream filter)
Adds a proposal to allow for access log enrichment via headers and the ability to make comlex substitutions to headers on a request/response.
Current idea is to end up with a repo structure along the lines of this