Set-Metadata HTTP filter by aguinet · Pull Request #16400 · envoyproxy/envoy

aguinet · 2021-05-09T14:33:44Z

Commit Message: HTTP filter that can set or update dynamic metadata.

Additional Description:

This extension simply allows to add user-defined dynamic metadata. This is intended to be used with matchers, so that the "result" of a matching process can imply specific metadata.

See issue #16266

Risk Level: Medium
Testing: Unit tests for the extension are added
Docs Changes: A small documentation for the extension is added in docs/root/configuration/http/http_filters
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

HTTP filter that can set or update dynamic metadata. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

repokitteh-read-only · 2021-05-09T14:33:51Z

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

🐱

Caused by: #16400 was opened by aguinet.

see: more, trace.

htuch

Looks good generally, just want to verify the details around merge.

htuch · 2021-05-09T16:13:30Z

api/BUILD

        "//envoy/extensions/filters/http/ratelimit/v3:pkg",
        "//envoy/extensions/filters/http/rbac/v3:pkg",
        "//envoy/extensions/filters/http/router/v3:pkg",
+        "//envoy/extensions/filters/http/set_metadata/v3:pkg",


Need to update https://github.com/envoyproxy/envoy/blob/main/CODEOWNERS

I tried to added myself, but it asked for a second maintainer. Who should I add? @snowp as he is sponsoring it :) ?

Yes, Snow and you.

Sorry missed this answer. Done in d139579 !

source/extensions/filters/http/set_metadata/set_metadata_filter.cc

htuch · 2021-05-09T16:22:10Z

api/envoy/extensions/filters/http/set_metadata/v3/set_metadata.proto

+  // The metadata namespace.
+  string metadata_namespace = 1 [(validate.rules).string = {min_len: 1}];
+
+  // The value to update the namespace with.


RST link to docs providing details of the merge semantics.

Should be fixed in 9d26ca2

api/envoy/extensions/filters/http/set_metadata/v3/set_metadata.proto

@htuch

@htuch review: coding style + cleanup + documentation improvements Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Implement a structure update algorithm that merges keys together, with a "last-one-win" strategy for scalar values, and appending for lists. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Use StructUtil::update, and update documentation about keys that have different types between the original and new structure. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Add documentation for StructUtil::update Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

source/common/protobuf/utility.cc

@htuch

review by @htuch Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-05-14T09:11:57Z

@htuch it looks like this conflicts with main. Should I rebase on main and squash into two commits? (one for StructUtil::update and one for the extension)

htuch · 2021-05-14T13:16:44Z

@aguinet in general, you should have a flow that you never rebase after creating a PR. Instead do local merges from origin/main.

htuch

Thanks. The merging now makes more sense. One question I have is how this extension relates to the header_to_metadata filter? https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/http/header_to_metadata/v3/header_to_metadata.proto

It seems that really we want both to behave similarly, and if we were designing from scratch they would be somewhat unified. We also have other filters, e.g. ext_authz, that set dynamic metadata base on return values from remote servers. Ideally these have aligned merge semantics.

We don't need to boil the ocean here, but having some explanation and game plan to get consistent behaviors would be helpful.

aguinet · 2021-05-14T17:06:13Z

Thanks. The merging now makes more sense. One question I have is how this extension relates to the header_to_metadata filter? https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/http/header_to_metadata/v3/header_to_metadata.proto

It seems that really we want both to behave similarly, and if we were designing from scratch they would be somewhat unified. We also have other filters, e.g. ext_authz, that set dynamic metadata base on return values from remote servers. Ideally these have aligned merge semantics.

We don't need to boil the ocean here, but having some explanation and game plan to get consistent behaviors would be helpful.

These are good questions I tried to resolve before starting this. Let me first explain my use case. What we'd like to do is leverage the matching engine to add "tags", and log this tag and/or take decisions according to these tags. This is configurable by the user in the end, and would be transformed to an envoy configuration file.

As an example, I could add the tag "tag0" if the cookie "mycookie" matches a given regular expression, and another tag "tag1" if the IP comes from AS3215. I would like these two tags to be logged, and let's say block the request if the two of them are there.

In order to reuse the matching engine, we would configure HTTP filters like this:

# Wrapped into a first matcher
- filter0: 
    @type: set_metadata
    namespace: mynamespace
    value:
      tags:
       tag0: 1
#  Wrapped into a second matcher
- filter1:
    @type: set_metadata 
    namespace: mynamespace
    value:
      tags:
        tag1: 1

In order to get the two tags, I need the current merging strategy.

Now about header_to_metadata. From my understanding, it's just setting strings (potentially base64 ones) or integers values, taken from headers. There are no issue of potentially merging two different recursive structures. But I might be missing something here.

I just checked ext_authz, the only place I found it setting metadata is here:

envoy/source/extensions/filters/http/ext_authz/ext_authz.cc

Lines 211 to 214 in 1f4ca4c

    
           if (!response->dynamic_metadata.fields().empty()) { 
        
             decoder_callbacks_->streamInfo().setDynamicMetadata(HttpFilterNames::get().ExtAuthorization, 
        
                                                                 response->dynamic_metadata); 
        
           }

. This looks like there are no merging strategy involved, but I don't know the use cases enough to know if that even makes sens!

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-05-14T17:25:03Z

@aguinet in general, you should have a flow that you never rebase after creating a PR. Instead do local merges from origin/main.

Okay. So I'll end up with a merge commit, right? Should I squash all of this before merging?

htuch · 2021-05-16T14:55:30Z

Thanks for the explanation. I agree this makes sense. @envoyproxy/api-shepherds I'd like us to think about adopting this as the standard merge strategy going forward, so we don't have inconsistent merging when filters are performing dynamic metadata update.

Okay. So I'll end up with a merge commit, right? Should I squash all of this before merging?

Yes, add a merge commit. You can't squash (since this is a rebase) once a PR has started. We will squash everything prior to the main merge once approved.

htuch

Thanks; a bunch of API and code comments to address, I think it's in the right shape.
/wait

htuch · 2021-05-16T15:02:14Z

docs/root/configuration/http/http_filters/set_metadata_filter.rst

+
+This filters adds or updates dynamic metadata with static data.
+
+Dynamic metadata values are updated with the following scheme. If a key


@envoyproxy/api-shepherds this differs from MergeFrom in that it supports merging structs together, e.g. if you have a disjoint namespace things compose as expected. I think this makes sense, but flagging this for consideration in case there is some alternate merging preferences.

source/common/protobuf/utility.cc

source/extensions/filters/http/set_metadata/set_metadata_filter.cc

test/extensions/filters/http/set_metadata/set_metadata_filter_test.cc

source/extensions/filters/http/set_metadata/config.cc

source/extensions/filters/http/set_metadata/set_metadata_filter.cc

test/extensions/filters/http/set_metadata/set_metadata_filter_test.cc

* add constness when relevant * s/T const&/const T&/ * explicit some types * add missing tests (factory + decodeData) Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch · 2021-05-18T02:31:59Z

test/common/protobuf/utility_test.cc


+class StructUtilTest : public ProtobufUtilityTest {
+protected:
+  ProtobufWkt::Struct updateSimpleStruct(ProtobufWkt::Value const& v0,


More const issues. Sorry if this seems nit-picky, but this style of const is different to the Envoy code base and consistency is important. There are some rare exceptions and we should fix them.

Can you just grep for const& in your difference from main and fix them all? Thanks.

I'm sorry I thought I had checked them all... I completely understand the style consistency. Will indeed recheck this properly!

Fixed in 45ca933 .

$ git diff main...feature/ext_add_metadata |grep 'const&' |wc -l 0

Hopefully I should be fine!

htuch · 2021-05-18T02:34:56Z

LGTM modulo nits and a main merge for conflict.

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch

LGTM, thanks!

Needs main merge.

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-05-20T07:09:01Z

@htuch main merged into the branch! Is there something else I need to do?

Thanks for the time you took for all the reviews and remarks!

htuch · 2021-05-21T03:28:50Z

@phlax do you know what is going on with this glint failure?

phlax · 2021-05-21T07:14:34Z

https://dev.azure.com/cncf/envoy/_build/results?buildId=76293&view=logs&j=c5dd2866-6ab3-5f3c-3a44-4cef0ec909b5&t=a9eb66d6-8944-5769-b3f7-476949dadcb8&l=60

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-05-21T07:48:41Z

https://dev.azure.com/cncf/envoy/_build/results?buildId=76293&view=logs&j=c5dd2866-6ab3-5f3c-3a44-4cef0ec909b5&t=a9eb66d6-8944-5769-b3f7-476949dadcb8&l=60

Should be fixed with 0469f86 . Added a configuration to my vim to highlight these...

phlax · 2021-05-21T07:50:21Z

Added a configuration to my vim to highlight these...

i have my editor (emacs) automatically remove/fix on save

htuch

LGTM, thanks!

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch · 2021-05-23T13:32:08Z

@aguinet the clang-tidy failures seem legit, see https://dev.azure.com/cncf/envoy/_build/results?buildId=76440&view=logs&j=430ab721-bbba-5b3a-3c7b-ff111cc657fe&t=b6238472-0f30-5195-100c-623a3015b9ef

aguinet · 2021-05-23T13:33:18Z

@htuch yep, I've just pushed d82b20a to fix it!

aguinet · 2021-05-25T08:47:05Z

\o/ Thanks a lot for the merge @htuch !

This extension simply allows to add user-defined dynamic metadata. This is intended to be used with matchers, so that the "result" of a matching process can imply specific metadata. See issue envoyproxy#16266 Risk Level: Medium Testing: Unit tests for the extension are added Docs Changes: A small documentation for the extension is added in docs/root/configuration/http/http_filters Signed-off-by: Adrien Guinet <adrien@reblaze.com>

SetMetadata http filter

46024aa

HTTP filter that can set or update dynamic metadata. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

repokitteh-read-only bot added the api label May 9, 2021

repokitteh-read-only bot assigned htuch May 9, 2021

htuch reviewed May 9, 2021

View reviewed changes

aguinet added 5 commits May 10, 2021 19:52

fixup! SetMetadata http filter

9d26ca2

@htuch review: coding style + cleanup + documentation improvements Signed-off-by: Adrien Guinet <adrien@reblaze.com>

protobuf: StructUtil::update algorithm

acb5e25

Implement a structure update algorithm that merges keys together, with a "last-one-win" strategy for scalar values, and appending for lists. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

fixup! SetMetadata http filter

7540c2a

Use StructUtil::update, and update documentation about keys that have different types between the original and new structure. Signed-off-by: Adrien Guinet <adrien@reblaze.com>

fixup! protobuf: StructUtil::update algorithm

4e846f0

Add documentation for StructUtil::update Signed-off-by: Adrien Guinet <adrien@reblaze.com>

fixup! SetMetadata http filter

d13f111

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch reviewed May 14, 2021

View reviewed changes

source/common/protobuf/utility.cc Outdated Show resolved Hide resolved

fixup! protobuf: StructUtil::update algorithm

56ea00f

review by @htuch Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch reviewed May 14, 2021

View reviewed changes

Merge branch 'main' into feature/ext_add_metadata

d001646

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch suggested changes May 16, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label May 16, 2021

review by @htuch

cfad084

* add constness when relevant * s/T const&/const T&/ * explicit some types * add missing tests (factory + decodeData) Signed-off-by: Adrien Guinet <adrien@reblaze.com>

repokitteh-read-only bot removed the waiting label May 17, 2021

Add @snowp and myself as contributors

d139579

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch reviewed May 18, 2021

View reviewed changes

s/T const&/const T&/

45ca933

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch previously approved these changes May 20, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 20, 2021

Merge branch 'main' into feature/ext_add_metadata

c113bb2

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet dismissed htuch’s stale review via c113bb2 May 20, 2021 07:13

repokitteh-read-only bot added the api label May 20, 2021

remove extra whitespace

0469f86

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

htuch previously approved these changes May 23, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 23, 2021

Fix issues found by clang-tidy

d82b20a

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet dismissed htuch’s stale review via d82b20a May 23, 2021 13:32

repokitteh-read-only bot added the api label May 23, 2021

htuch approved these changes May 25, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 25, 2021

htuch merged commit cbe08bb into envoyproxy:main May 25, 2021

aguinet deleted the feature/ext_add_metadata branch May 25, 2021 08:45


		This filters adds or updates dynamic metadata with static data.

		Dynamic metadata values are updated with the following scheme. If a key

Conversation

aguinet commented May 9, 2021

Uh oh!

repokitteh-read-only bot commented May 9, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aguinet commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented May 14, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

aguinet commented May 14, 2021

Uh oh!

aguinet commented May 14, 2021

Uh oh!

htuch commented May 16, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented May 18, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

aguinet commented May 20, 2021

Uh oh!

htuch commented May 21, 2021

Uh oh!

phlax commented May 21, 2021

Uh oh!

aguinet commented May 21, 2021

Uh oh!

phlax commented May 21, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented May 23, 2021

Uh oh!

aguinet commented May 23, 2021

Uh oh!

aguinet commented May 14, 2021 •

edited

Loading