matching: add support for generic inputs and add environment variable input#15410
matching: add support for generic inputs and add environment variable input#15410snowp merged 16 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
| // [#extension: envoy.matching.generic_inputs.environment] | ||
|
|
||
| // Reads an environment variable to provide an input for matching. | ||
| message Environment { |
There was a problem hiding this comment.
Environment sounds ambiguous, something more descriptive, perhaps EnvironmentVariable, would be clearer.
There was a problem hiding this comment.
Yea agreed, will update
|
Could you add some docs/examples demonstrating how this |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
| explicit Input(absl::string_view name) { | ||
| // We read the env variable at construction time to avoid repeat lookups. | ||
| // This assumes that the environment remains stable during the process lifetime. | ||
| auto* value = getenv(name.data()); |
There was a problem hiding this comment.
Mostly curious what you think: if this side-effect was moved out of the constructor (into the factory perhaps?), this class would become a trivial wrapper around absl::optional<std::string>. This would make tests simpler, I think: GenericTestInput wouldn't be needed, and TestGenericDataInputFactory would only need to overload the method that retrieves env var with one that returns something expected in the test. Even though getenv() is pretty benign, side-effects in constructors always catch my attention...
There was a problem hiding this comment.
I like that, let me give that a go!
|
Looks good, other than a small nits re: naming. |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
lgtm, not sure why precheck docs is failing; perhaps try merging in the latest from the main branch? |
|
@dmitri-d the doc fails because: |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
|
This should be ready for another pass |
|
@htuch @mattklein123 either of you want to review this? |
|
Sure I can take a look. |
|
this needs to merge main - it would fail linting on postsubmit if merged as is /wait |
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Adds support for a "generic input" extension point that allows specifying inputs that are not dependent on protocol data.
Adds an environment variable generic input that allows matching on the value of an environment variable.
Risk Level: Medium
Testing: UTs for extension and generic input creation
Docs Changes: n/a
Release Notes:
Platform Specific Features:
Fixes #14781