feat(translator): support ext-auth dynamic metadata options (Envoy to auth server)#6453
feat(translator): support ext-auth dynamic metadata options (Envoy to auth server)#6453nareddyt wants to merge 8 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
…-authz-dynamic-metadata Signed-off-by: Teju Nareddy <tnareddy@confluent.io> # Conflicts: # release-notes/current.yaml
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6453 +/- ##
==========================================
+ Coverage 70.75% 70.76% +0.01%
==========================================
Files 220 220
Lines 37757 37767 +10
==========================================
+ Hits 26715 26727 +12
+ Misses 9482 9479 -3
- Partials 1560 1561 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
|
@guydc PTAL, addressed all review comments. Would like your input on our idea to expose both typed and untyped namespaces, as Confluent has a use case for typed namespaces |
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
|
/retest |
guydc
left a comment
There was a problem hiding this comment.
LGTM.
Would be nice to extend the e2es to cover this feature, e.g. have grcp ext-auth consume metadata and add it to the request headers through the Check Response.
Signed-off-by: Teju Nareddy <tnareddy@confluent.io>
Good idea, done. I tested static untyped route metadata in the e2e test. IMO there is no additional value is testing the other 3 fields (typed route metadata, typed + untyped listener metadata). |
guydc
left a comment
There was a problem hiding this comment.
LGTM. Thanks for adding the e2e test.
|
/retest |
|
hey @nareddyt can we limit this feature to route metadata ? |
|
@arkodg we do have a few use cases at Confluent where we need to propagate filter metadata from other OSS filters. We had a similar discussion in #6453 (comment) Are there any specific implications you are worried about? I can limit it to route metadata if really required, but then we can no longer use this feature and will still need an extension server modification. |
|
would A CEL validtion be tricky be add for this case ?
|
Just trying to understand what that would solve? IIRC we have precedent for filter metadata in EnvoyExtensionPolicy via ExtProc. https://gateway.envoyproxy.io/docs/api/extension_types/#extprocmetadata . This supports untyped filter metadata. And ExtProc can target both a HTTPRoute or a Gateway. I didn't see any sort of similar validations in ExtProc CRD, so just trying to understand if they are necessary. |
|
Just want to clear up some confusion: Envoy allows attaching xDS metadata to multiple resources, such as routes, clusters, listeners. Envoy filters also can modify per-request dynamic metadata. The ext_authz callout should allow sending all these types of metadata, regardless of whether the |
if a Envoy Gateway does not gives direct access to xds filters, except in the There's |
|
Thanks @arkodg , I understand the concern now. Unfortunately this will not work for Confluent's use case. Let me close this PR and rethink it. Perhaps I will go with a custom extension server mutation on our end for the time being. |
What this PR does / why we need it:
Added support for specifying dynamic metadata namespaces that External Authorization services can access in SecurityPolicy API
This follows the structure and CRD naming conventions established in #5023.
NOTE: This PR only introduces fields for "sending metadata to ext-authz server". It does NOT handle the reverse operation of "parsing ext-authz HTTP headers and turning them into dynamic metadata on Envoy" (which #4163 was initially opened for). However, this PR structures the CRD changes in a way that allows for the reverse operation to be easily added.
Which issue(s) this PR fixes:
Not aware of any pre-existing issue describing this specific use case. #4163 is related but for the reverse operation.
Release Notes: Yes