Hi @rgs1 ! Very nice! Let me know when you think it's ready to be tested. I see you're still adding commits. I'm happy to test it and confirm that it works for me too.

@bmetzdorf yep sorry, still ironing out a few things... Will let you know!

@bmetzdorf @ggreenway ok, I think it's going through now... Sorry for the noise. Let me know if this goes along with what you are thinking. Thanks!

cc: @zuercher who filed #2436.

@bmetzdorf sorry, had to rebase plus i added one more commit to avoid string copies.

Btw, I am not sold on calling this filter header-to-metadata... That's just what we called it internally. Something more descriptive would be better.

Hi @rgs1.

So, the simple request_rules use case that I'm interested in works fine for plain HTTP, but as you already mentioned in #2436 it is missing regex extraction support. In addition to that though, it seems as if currently (and I know there's some work in progress to overhaul this) this (or any) http filter does not get called for websocket connections, which unfortunately is what I'd require (sigh..).

So I'm wondering if we could provide some functionality of this in the route matcher (which does work for websocket connections) instead:

    - match:
        prefix: "/"
        headers:
        - name: x-something
          value: abc([0-9]+)
          regex: true
      route:
        use_websocket: true
        cluster: mycluster
        metadata_match:
          filter_metadata:
            envoy.lb:
              digits_from_x_something: \1

We could also have two different implementations, until the websocket stuff gets refactored.

What do you think?

@bmetzdorf we've got many many route matchers, so if we implement it there we'd need to be able to configure this at the RouteConfiguration [0] or Route [1] level — to avoid the copy/pasta. But yeah, I think it would work for us.

cc: @derekargueta

[0] https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/rds.proto#L39
[1] https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/route/route.proto#L116

@rgs1 If you have many route matchers, would the Route level still be ok for you?

Long term I'd think that the HTTP filter is the right approach. The filter should work with the upcoming websocket changes. Until then the limited functionality I require could be provided in the Route level, which would be referencing a regex capture group inside metadata_match. Regex matching is already supported.

@bmetzdorf yeah, Route level is good I think.

@zuercher thanks for the review! I've addressed most of your comments in 471e25f, the commit description has details and questions about the things I am not so sure about... LMK what you think. Thanks!

@zuercher i addressed your latest comments, lmk if i missed anything — thanks!

Hmm, we should probably emit some stats (e.g.: total count of metadata keys added, total per key, etc)? We haven't had a need for them internally (yet), but I actually think they could be useful for users to validate things are working as expected. Thoughts?

I think per https://github.com/envoyproxy/envoy/blob/master/GOVERNANCE.md#extension-addition-policy
If you add a CODEOWNERS with you and zuercher (assuming he volunteers to own) you're good to go with his approval. And fixed format :-)

Yup per @alyssawilk that's fine with me. I don't have time to review this right now. Please make sure that all the docs are in place though. Thank you!!

@alyssawilk ah nice, will do!

@zuercher are you ok with being added to CODEOWNERS for this? :-) (no pressure)

@zuercher done — i went with Header to Metadata though, since the IPTagging filter also interchanges IP Tagging and IPTagging.... though I'll switch back to Header-To-Metadata if you have strong feelings about it :-)

Output is here: https://65121-65214191-gh.circle-artifacts.com/0/source/generated/docs/api-v2/config/filter/http/header_to_metadata/v2/header_to_metadata.proto.html

@bmetzdorf so #3301 was fixed/addressed, which means this should now work with websockets... We'd probably need to extend it support regexes, which was another requirement you had....

Hi @rgs1 . Unfortunately my use case requires websockets with early body, which violates the websocket spec. While #3301 / #3376 might still allow this for now (I haven't tested it yet) it will be a problem in the future when strict websocket support will be enforced, in which case the http_filter approach for extracting metadata won't work for me. @alyssawilk @ggreenway Do you see any chance that we might allow early websocket bodies even in the long term?

We've got regression tests for early websocket bodies for both the old style and new style websockets so hopefully you'll be set.

When you say "websockets with early body" do you mean explicit content-length != 0, or websocket payload arriving with the upgrade request? I could see Envoy rejecting GET-with-body but I don't think anyone plans on killing off early data handling, especially if you comment on that regression test that there are users using it.

@alyssawilk I use both, upgrade request coming in with body payload as well as a upgrade response with content-length > 0. Is that something we could entertain to support long term (maybe hidden behind a use_relaxed_websockets if needed)?

More likely a more generic allow_disallowed_bodies or something but I suspect we could leave it config available. As I'd likely be the person implementing any adhere-to-the-spec feature and @mattklein123 or I would likely be the ones to review if someone else writes it, we'll keep your traffic in mind :-)

That sounds great! Thanks @alyssawilk !

@rgs1 In regards to regex, what do you think about something like this?

http_filters:
  - name: envoy.filters.http.header_to_metadata
    config:
      request_rules:
        - header: x-version
          match: "^[0-9]+\.([0-9]+)$"
          on_header_present:
            metadata_namespace: envoy.lb
            key: version
            type: STRING
          on_header_match:
            metadata_namespace: envoy.lb
            key: minor_version
            value: "$1"
            type: STRING
          on_header_missing:
            metadata_namespace: envoy.lb
            key: default
            value: 'true'
            type: STRING
          remove: false

@bmetzdorf generally lgtm, lets try to make it consistent with how exact match vs regex match is implemented in other places... Something like this perhaps:

https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/route/route.proto#L941

What do you think?

@rgs1 Sure!

Do you think we should support all these cases?

  "exact_match": "...",
  "regex_match": "...",
  "range_match": "{...}",
  "present_match": "...",
  "prefix_match": "...",
  "suffix_match": "...",
  "invert_match": "..."

Or just

  "regex_match": "...",
  "invert_match": "..."

for now?

@bmetzdorf lets start with the ones you'll need :-) So we have to keep exact_match (we can probably deprecated value), regex_match would be nice... What do you think? All the others would be depending on your needs.

@rgs1 I think regex_match and invert_match (to not have to use negative lookahead / lookbehind) would be sufficient for now. Should we model on_header_present with present_match instead and on_header_missing with present_match + invert_match and copy/re-use route.HeaderMatcher (see https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/route/route.proto.html#envoy-api-msg-route-headermatcher)?

Examples:

Now:

http_filters:
  - name: envoy.filters.http.header_to_metadata
    config:
      request_rules:
        - header: x-version
          on_header_present:
            metadata_namespace: envoy.lb
            key: version
            type: STRING
          on_header_missing:
            metadata_namespace: envoy.lb
            key: default
            value: 'true'
            type: STRING
          remove: false

Future:

http_filters:
  - name: envoy.filters.http.header_to_metadata
    config:
      request_rules:
        headers:
          - name: x-version
            present_match: true
            metadata_namespace: envoy.lb
            key: version
            type: STRING
          - name: x-version
            present_match: true
            invert_match: true
            metadata_namespace: envoy.lb
            key: default
            value: 'true'
            type: STRING
          - name: x-version
            regex_match: "^[0-9]+\.([0-9]+)$"
            metadata_namespace: envoy.lb
            key: minor_version
            value: "$1"
            type: STRING

with value being optional and defaulting to the actual complete header value (unless header is missing, then defaulting to empty string)?

@bmetzdorf I like the idea of reusing/emulating route.HeaderMatcher... However I am not sure what the policy is with breaking config for existing filters.... @zuercher ?

@rgs1 We could introduce a new key request_matche(r)s and keep request_rules ?

Yeah that would work.

@rgs1 Ok, so what do you think about this (request_headers instead of request_matche(r)s) ?

http_filters:
  - name: envoy.filters.http.header_to_metadata
    config:
      request_headers:
        - name: x-version
          present_match: true
          metadata_namespace: envoy.lb
          key: version
          type: STRING
        - name: x-version
          present_match: false
          metadata_namespace: envoy.lb
          key: default
          value: 'true'
          type: STRING
        - name: x-version
          regex_match: "^[0-9]+\.([0-9]+)$"
          metadata_namespace: envoy.lb
          key: minor_version
          value: "$1"
          type: STRING

I think we would have to copy route.HeaderMatcher and add the missing definitions though..

@bmetzdorf that looks good

@rgs1 While thinking about this more I'm realizing that it might be easier to create a new filter and retire the current one later. I think it could be a little bit messy trying to cram both types of configuration and logic into the existing one. What do you think?

@bmetzdorf sure that's fine too