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.

Sign up for GitHub

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

Jump to bottom

matching: add support for generic inputs and add environment variable input #15410

Merged
merged 16 commits into from
Apr 1, 2021
Merged

matching: add support for generic inputs and add environment variable input #15410

merged 16 commits into from
Apr 1, 2021

Conversation

snowp
Copy link
Contributor

@snowp snowp commented Mar 10, 2021

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

@snowp
matching: add support for generic input, add environment input
61d0fdd 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
add reference to generic input
835cce7 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
add codeowners
3c8aefb 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
fix lints
bc4d426 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
update proto
6f0b847 
Signed-off-by: Snow Pettersen <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15410 was opened by snowp.

see: more, trace.

@snowp
coverage
f7baa1f 
Signed-off-by: Snow Pettersen <[email protected]>
dmitri-d
dmitri-d reviewed Mar 11, 2021
View reviewed changes
api/envoy/extensions/matching/generic_inputs/environment/v3/environment.proto Outdated
// [#extension: envoy.matching.generic_inputs.environment]

// Reads an environment variable to provide an input for matching.
message Environment {
Copy link
Contributor

Choose a reason for hiding this comment

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

Environment sounds ambiguous, something more descriptive, perhaps EnvironmentVariable, would be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea agreed, will update

@dmitri-d
Copy link
Contributor

Could you add some docs/examples demonstrating how this DataInput is configured/used?

@snowp
use env helper to deal with platform compatibility
815a4fa 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
format
045988e 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
Copy link
Contributor Author

snowp commented Mar 11, 2021

@dmitri-d I think I would want this to be covered by docs just as an extension of the match API, so just I figured I'd just update the docs that will be added in #14864 (either in this PR or follow up, depending on merge order)

dmitri-d
dmitri-d reviewed Mar 12, 2021
View reviewed changes
source/extensions/matching/generic_inputs/environment/input.h Outdated
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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that, let me give that a go!

@dmitri-d
Copy link
Contributor

Looks good, other than a small nits re: naming.

lizan
lizan reviewed Mar 13, 2021
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

/lgtm api

@snowp
read env in factory creation
260fc8d 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
rename to EnvironmentVariable
b28f634 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
format
84a528b 
Signed-off-by: Snow Pettersen <[email protected]>
@dmitri-d
Copy link
Contributor

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15410 (comment) was created by @dmitri-d.

see: more, trace.

@dmitri-d
Copy link
Contributor

lgtm, not sure why precheck docs is failing; perhaps try merging in the latest from the main branch?

@lizan
Copy link
Member

lizan commented Mar 16, 2021

@dmitri-d the doc fails because:

2021-03-15T19:24:09.4627982Z looking for now-outdated files... none found
2021-03-15T19:24:09.4630549Z pickling environment... /source/generated/rst/api-v3/common_messages/common_messages.rst:4: WARNING: toctree contains reference to nonexisting document 'api-v3/extensions/matching/generic_inputs/environment/v3/environment.proto'
2021-03-15T19:24:09.5144597Z done

@snowp
rename to common protocol inputs
246653f 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
fix v4
fde8b7b 
Signed-off-by: Snow Pettersen <[email protected]>
lizan
lizan reviewed Mar 19, 2021
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

/lgtm api

can you check CI?

@snowp
fix tests
2a0ea2f 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp
Copy link
Contributor Author

snowp commented Mar 23, 2021

This should be ready for another pass

@snowp
Copy link
Contributor Author

snowp commented Mar 31, 2021

@htuch @mattklein123 either of you want to review this?

@mattklein123 mattklein123 self-assigned this Mar 31, 2021
@mattklein123
Copy link
Member

Sure I can take a look.

mattklein123
mattklein123 previously approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM!

@phlax
Copy link
Member

phlax commented Mar 31, 2021

this needs to merge main - it would fail linting on postsubmit if merged as is

/wait

@snowp
add newlines
8df489f 
Signed-off-by: Snow Pettersen <[email protected]>
@snowp snowp merged commit 758a9a9 into envoyproxy:main Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch left review comments

@lizan lizan lizan left review comments

@dmitri-d dmitri-d dmitri-d left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@lizan lizan

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add environment variable DataInput extension
6 participants
@snowp @dmitri-d @lizan @mattklein123 @phlax @htuch