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

design 10574: transformations #10689

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

nfuden
Copy link
Contributor

@nfuden nfuden commented Feb 24, 2025

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

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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?

@nfuden nfuden enabled auto-merge February 25, 2025 01:23
@nfuden nfuden disabled auto-merge February 25, 2025 01:23
Comment on lines +131 to +132
Given this the first addition here should contain the following functions:
"base64_encode", "header", "replace_with_random"
Copy link
Contributor

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?

Copy link
Contributor Author

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");

https://github.com/kgateway-dev/kgateway/pull/10677/files#diff-1992c8adc1fc21e3baa436d3ffd5a91d47311816cde7bad0d8785bbbc4865aeeR103

* Have full feature parity with gloo transformations (at first)
* Keep the same user facing api as gloo

## Implementation Details
Copy link
Contributor

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).

Copy link
Contributor Author

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.
Copy link
Contributor

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

Copy link
Contributor Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants