Bump Inja dependency #248
Bump Inja dependency #248
Conversation
| extractions, json_body, environ_, | ||
| cluster_metadata, rng_); | ||
|
|
||
| tls_->set([header_map=&header_map, request_headers, get_body, extractions=&extractions, json_body=&json_body, environ=&environ_, cluster_metadata, rng=&rng_](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { |
There was a problem hiding this comment.
you can't call set() from transform() as set will set the object on all worker threads.
call set() from the ctor (you only need to call it one time).
Here you can call getTyped and update the existing object
There was a problem hiding this comment.
I changed this to have a set() call construct an empty object during the InjaTransformer constructor, then request context is populated by getting a reference to the thread-local object with getTyped. Is this more in line with how this should be used?
source/extensions/filters/http/transformation/inja_transformer.h
Outdated
Show resolved
Hide resolved
|
Issues linked to changelog: |
source/extensions/filters/http/transformation/inja_transformer.cc
Outdated
Show resolved
Hide resolved
| @@ -115,7 +122,8 @@ class InjaTransformer : public Transformer { | |||
|
|
|||
| absl::optional<inja::Template> body_template_; | |||
| bool merged_extractors_to_body_{}; | |||
| Envoy::Random::RandomGenerator &rng_; | |||
| ThreadLocal::SlotPtr tls_; | |||
| std::unique_ptr<TransformerInstance> instance_; | |||
There was a problem hiding this comment.
micro-nit: i don't think this needs to be a pointer
| std::unique_ptr<TransformerInstance> instance_; | |
| TransformerInstance instance_; |
(the .cc will change a bit as well)
There was a problem hiding this comment.
I followed the changes for this all the way until it needs a member function (Environment::render) within the Inja library to be marked const. I think this is because we're using it within the const transform method?
There was a problem hiding this comment.
It's unclear why the build errors occur when this field is a concrete object, but not when it is a pointer. However, the performance overhead should be negligent. We confirmed that the render calls are thread safe because the Renderer constructor that ends up being called by Inja::Environment::render takes const parameters. The same cannot be said of the Parser constructor, but we should not be calling parse on the request path anyway.
bazel/repository_locations.bzl
Outdated
| @@ -5,7 +5,7 @@ REPOSITORY_LOCATIONS = dict( | |||
| remote = "https://github.com/envoyproxy/envoy", | |||
| ), | |||
| inja = dict( | |||
| commit = "4c0ee3a46c0bbb279b0849e5a659e52684a37a98", | |||
| commit = "5a18986825fc7e5d2916ff345c4369e4b6ea7122", # v3.4 + JSON Pointers | |||
| remote = "https://github.com/pantor/inja", | |||
There was a problem hiding this comment.
if your PR gets closed / deleted this might become invalid in the future.
Better to point to a commit in your fork, where you tag it so GH won't delete it
There was a problem hiding this comment.
Good call, I made a solo-io fork, created a tag, and pointed to the commit in that repo. I should probably also update the source of the PR to be the solo-io branch
Uses thread-local storage to pass request data into the
TransformerInstanceso that we can use the instance (and its registered callbacks) to parse templates at init time.