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.

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

ext_proc: allow override metadata without grpc_service #31544

Merged
merged 15 commits into from
Feb 27, 2024

Conversation

sitano
Copy link
Contributor

@sitano sitano commented Dec 28, 2023

Commit Message: ext_proc: allow override metadata without grpc_service

The patch introduces overrides.grpc_initial_metadata in ExtProcPerRoute.overrides that appends and overrides parent metadata:

  - match:
      ...
    route:
      ...
    typed_per_filter_config:
      envoy.filters.http.ext_proc:
        "@type": type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExtProcPerRoute
        overrides:
          grpc_initial_metadata:
            - key: "b"
              value: "c"
            - key: "c"
              value: "c"
http_filters:
- name: envoy.filters.http.ext_proc
typed_config:
  "@type": type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExternalProcessor
  grpc_service:
    envoy_grpc:
      cluster_name: ext-proc-proxy
    timeout: 60s
    initial_metadata:
      - key: "a"
        value: "a"
      - key: "b"
        value: "b"

that allows to have:

a: a
b: c
c: c

like the following:

[2024-01-09 16:40:23.241][1967519][debug][router] [source/common/router/router.cc:731] [Tags: "ConnectionId":"0","StreamId":"5894068517272876876"] router decoding headers:
':method', 'POST'
':path', '/envoy.service.ext_proc.v3.ExternalProcessor/Process'
':authority', 'ext-proc-proxy'
':scheme', 'http'
'te', 'trailers'
'content-type', 'application/grpc'
'a', 'a'
'b', 'c'
'c', 'c'
'x-envoy-internal', 'true'
'x-forwarded-for', '172...'

Additional Description: new ext_proc per route override of inherited grpc metadata without specifying grpc_service
Risk Level: low
Testing: Unit, Integration, Config
Docs Changes: will have to update https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_proc/v3/ext_proc.proto#extensions-filters-http-ext-proc-v3-extprocperroute regarding repeated config.core.v3.HeaderValue grpc_initial_metadata = 7;.

// Additional metadata to include into streams initiated to the ext_proc gRPC
// service. This can be used for scenarios in which additional ad hoc
// authorization headers (e.g. x-foo-bar: baz-key) are to be injected or
// when a route needs to partially override inherited metadata.

Release Notes: see changlelogs/current.yaml
Platform Specific Features: -

Testing

$ bazel test //test/extensions/filters/http/ext_proc:config_test --config=clang --verbose_failures --test_arg="-l trace"
$ bazel test //test/extensions/filters/http/ext_proc:filter_test --config=clang --verbose_failures --test_arg="-l trace"
$ bazel test //test/extensions/filters/http/ext_proc:ext --config=clang --verbose_failures --test_arg=--gtest_filter='*/ExtProcIntegrationTest.PerRouteGrpcMetadata/*' --test_arg="-l trace"

Test infrastructure https://github.com/sitano/envoy/tree/ivan_api_502/examples/grpc-bridge:

t1$ cd ./server && go run
t2$ cd ./ext_proc && go run
t3$ envoy/bazel-bin/source/exe/envoy-static --config-path ./envoy-proxy.yaml --log-level debug --base-id 1
// t4$ envoy/bazel-bin/source/exe/envoy-static --config-path ./envoy-proxy.yaml --log-level debug --base-id 2
t5$ envoy/bazel-bin/source/exe/envoy-static --config-path ./envoy-proxy-ext_proc-2.yaml --log-level trace --base-id 3
$ CLIENT_PROXY=http://127.0.0.1:9911 ./client.py set key1 1
$ CLIENT_PROXY=http://127.0.0.1:9911 ./client.py set key2 1
$ CLIENT_PROXY=http://127.0.0.1:9911 ./client.py set key3 1
$ ss -tnp

Example

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #31544 was opened by sitano.

see: more, trace.

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #31544 was opened by sitano.

see: more, trace.

@sitano
Copy link
Contributor Author

sitano commented Dec 28, 2023

Other considerations:

  1. probably FilterConfigPerRoute::merge code maybe simplified to something like replace_all or push_back, but I am not sure what minimal version of C++ we are supporting.
  2. I am not sure about context.messageValidationVisitor() in the config test.
  3. Speaking of integration test, I did not find in a couple of hours a way to catch the ext_proc processor stream metadata during sendDownstreamRequest(). Maybe someone would be kind to give suggestions if there is sense to extend it further.

Another PR working on ext_proc metadata, but unrelated: #30747

@sitano
Copy link
Contributor Author

sitano commented Dec 28, 2023

Also, I have full grpc exp_proc example in https://github.com/sitano/envoy/tree/ivan_api_502/examples/grpc-bridge which I would like to add too, but it will a bit more effort to polish and I think its worth putting it into a separate PR. Or we can add it here?

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks!
Left an API high-level comment.
Please note that this PR will be reviewed after marking it as non-draft (otherwise the reviewers may not be notified on its changes).

@sitano sitano force-pushed the feature/ext_proc/route-metadata branch from 46b7dbb to f45a200 Compare January 9, 2024 12:11
sitano added 2 commits January 9, 2024 16:03
allows `overrides.metadata` in `ExtProcPerRoute` that appends and
overrides parent metadata:

      - match:
          ...
        route:
          ...
        typed_per_filter_config:
          envoy.filters.http.ext_proc:
            "@type": type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExtProcPerRoute
            overrides:
              metadata:
                - key: "x-ext-proc-override"
                  value: "route3-override"
                - key: "x-router3-metadata-append"
                  value: "route3-metadata-append"
    http_filters:
    - name: envoy.filters.http.ext_proc
    typed_config:
      "@type": type.googleapis.com/envoy.extensions.filters.http.ext_proc.v3.ExternalProcessor
      grpc_service:
        envoy_grpc:
          cluster_name: ext-proc-proxy
        timeout: 60s
        initial_metadata:
          - key: "x-ext-proc-override"
            value: "root"
          - key: "x-ext-proc-original"
            value: "root"

that allows to have:

    x-ext-proc-override: route3-override
    x-router3-metadata-append: route3-metadata-append
    x-ext-proc-original: root

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
- TEST(OverrideTest, MetadataOverride)
- TEST_F(HttpFilterTest, GrpcServiceMetadataOverride)
- TEST_P(ExtProcIntegrationTest, PerRouteMetadata)

    $ bazel test //test/extensions/filters/http/ext_proc:config_test --config=clang --verbose_failures --test_arg="-l trace"
    $ bazel test //test/extensions/filters/http/ext_proc:filter_test --config=clang --verbose_failures --test_arg="-l trace"
    $ bazel test //test/extensions/filters/http/ext_proc:ext_proc_integration_test --config=clang --verbose_failures --test_arg=--gtest_filter='*/ExtProcIntegrationTest.PerRouteMetadata/*' --test_arg="-l trace"

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
@sitano sitano force-pushed the feature/ext_proc/route-metadata branch from f45a200 to 143ef4a Compare January 9, 2024 16:06
@sitano sitano marked this pull request as ready for review January 10, 2024 08:47
@sitano sitano requested a review from adisuissa January 10, 2024 08:48
@sitano
Copy link
Contributor Author

sitano commented Jan 10, 2024

This test Mobile/Compile time options is also finished but did not update the status (https://github.com/envoyproxy/envoy/actions/runs/7463682673).

It is ready for review. The PR touches the same areas as the aforementioned #30747 and it could be easier for the same reviewers to do the review as they already are familiar with the ext_proc code base.

@ravenblackx
Copy link
Contributor

Added reviewer and approver from #30747 as suggested.

Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

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

/wait

source/extensions/filters/http/ext_proc/ext_proc.cc Outdated Show resolved Hide resolved
Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
@yanavlasov
Copy link
Contributor

LGTM from me. I will wait for API LGTM.

/wait-any

Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

@sitano Just one high-level question, Can I understand as follow?
Before this PR, ext_proc overrides whole grpc_service
with this PR, you want to only overrides initial_metadata of the grpc_service, rather than whole gRPC_service

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
@sitano
Copy link
Contributor Author

sitano commented Jan 31, 2024

list of additional changes in this main update:

  • override.metadata became override.grpc_metadata - must be much less confusing about what metadata it is extending
  • initNamespaces, initUntypedForwardingNamespaces, initTypedForwardingNamespaces,initUntypedReceivingNamespaces made static into anonymous namespace
  • initNamespaces has removed unnecessary (IMHO) early exit path:
-  if (ns.empty()) {
-    return {};
-  }

@sitano sitano requested a review from yanavlasov January 31, 2024 22:56
@htuch
Copy link
Member

htuch commented Feb 1, 2024

I think this also has the issue I point to in #32125 (comment). I'm not sure if we want to have multiple different ways to do the same thing - when to use metadata vs. attributes?

They both sound like words for OOB properties, is it possible to formally state when you would use one vs the other?

CC @rshriram

@sitano
Copy link
Contributor Author

sitano commented Feb 1, 2024

I'm not sure if we want to have multiple different ways to do the same thing - when to use metadata vs. attributes?

@htuch attributes of the https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_proc/v3/ext_proc.proto (request_attributes) (see also https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes#arch-overview-request-attributes) are completely different from the gRPC service metadata.

If I understand the point in 32125 the question is that existing attributes are not flexible enough and serve the same goal as metadata. I also think it is better to only have 1 thing. However, I would expect we can not deprecate the metadata from the grpc_service - it is used in many other places. Then why extend attributes functionality? It looks like it makes sense but it is out of the scope of this PR.

is it possible to formally state when you would use one vs the other?

in this PR we are only providing a way to extend grpc service initial_metadata on per route basis.

@sitano
Copy link
Contributor Author

sitano commented Feb 1, 2024

EDIT of last msg:

If I understand the point in 32125 the question is that existing attributes are not flexible enough and serve the same goal as metadata. I also think it is better to only have 1 thing. However, I would expect we can not deprecate the metadata from the grpc_service - it is used in many other places. Then why extend attributes functionality? It looks like it makes sense but it is out of the scope of this PR.

@htuch
Copy link
Member

htuch commented Feb 1, 2024

Let's continue this discussion in #32125. I don't think there is a problem with making initial metadata per-route, if we go a certain direction #32125, then this is somewhat redundant and I think we would do many of the things we do with metadata via attributes. Let's discuss.

@alyssawilk
Copy link
Contributor

@htuch I'm not clear on the status of tihs PR. is it awaiting review or waiting on offline discussion?

@sitano
Copy link
Contributor Author

sitano commented Feb 7, 2024

As initial metadata stays as per #32125 (comment) I think we can proceed?

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
@sitano sitano requested review from htuch and jbohanon February 10, 2024 12:03
@kyessenov
Copy link
Contributor

I'm also confused about the primary approver for this PR. @yanavlasov @htuch @rshriram let's clarify the status, is this mostly ready and awaiting for nits, or major changes needed.

Copy link
Member

@htuch htuch 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

@htuch
Copy link
Member

htuch commented Feb 16, 2024

API LGTM, handing back to @yanavlasov for implementation review and merge.

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
@sitano sitano requested a review from jbohanon February 17, 2024 10:47
Copy link
Contributor

@jbohanon jbohanon left a comment

Choose a reason for hiding this comment

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

Awesome work!

@sitano
Copy link
Contributor Author

sitano commented Feb 26, 2024

@yanavlasov I am not sure if we would like to have it merged, but just pinging you because > yanavlasov requested changes, and there are no changes actually to do

@yanavlasov yanavlasov merged commit 372a262 into envoyproxy:main Feb 27, 2024
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.