feat: add dynamic metadata support in http ext authz #4164
Conversation
Signed-off-by: Taufik Mulyana <[email protected]>
| // Note: this metadata lives in envoy.filters.http.ext_authz and can only be consumed | ||
| // by the next filter if explicitly specified. | ||
| // +optional | ||
| HeadersToMetadata []string `json:"headersToMetadata,omitempty"` |
There was a problem hiding this comment.
looks like only support exact with this PR, what will happen if we want to support prefix/suffix/regex in the future?
BTW, please proposal the API first.
There was a problem hiding this comment.
yes, this will only support exact matching for now, similar to HeadersToBackend.
Apologies, I wasn't aware a proposal was needed. I have now created it on #4163.
There was a problem hiding this comment.
we will need to modify the API for HeadersToBackend as well if we want to support prefix/suffix/regex in the future, do you think this is necessary @zirain? if so I'll try to change the implementation for both
There was a problem hiding this comment.
let's focus on metadata first, WDYT?
There was a problem hiding this comment.
I believe it may be sufficient to just support the exact match for the HeadersToMetadata, considering that
- HTTP header names are typically fixed and don't change dynamically, and the number of HTTP headers is usually quite limited.
- A downstream filter that needs to extract the value from a Metada will also need to know the Metadata name in advance, so having a fixed name seems reasonable.
Supporting prefix/suffix/regex may unnecessarily complicate the API.
That said, I could be mistaken on this. @envoyproxy/gateway-maintainers, please feel free to chime in.
There was a problem hiding this comment.
yes, I think adding prefix, suffix, or regex might complicate the API. For now, could we consider only supporting exact matches @zirain?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4164 +/- ##
==========================================
+ Coverage 65.66% 65.69% +0.02%
==========================================
Files 197 197
Lines 23565 23579 +14
==========================================
+ Hits 15474 15490 +16
+ Misses 6982 6980 -2
Partials 1109 1109 ☔ View full report in Codecov by Sentry. |
|
@zhaohuabing @zirain @nothinux I'm a -1 on introducing this change. There's isnt any notion of reading and writing metadata yet for end users, its still an advanced case. |
|
@arkodg Do you mean that Metadata is an internal mechanism of Envoy, and it should not be directly exposed to end users? The real users of this API are the ext auth developers, who should already be familiar with Envoy and the concept of Metadata. Thinking it through, all custom extensions - Ext Auth, WASM, and Ext Proc, face similar problems: their APIs are designed for developers, not end users. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
What type of PR is this?
add
headersToMetadatafield to support emitting dynamic metadata from authorization response headerWhat this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #4163