ext_authz: expose fields latency, bytesSent and bytesReceived for CEL and logging #37074
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
hypnoce
force-pushed
the
ext_authz_fields
branch
6 times, most recently
from
8d1a776
to
d0b4962
Compare
| @@ -306,6 +306,8 @@ message ExtAuthz { | |||
| // | |||
| // If this is false the filter will not emit stats, but filter_metadata will still be respected if | |||
| // it has a value. | |||
| // | |||
| // Fields ``latency``, ``bytesSent`` and ``bytesReceived`` are exposed for CEL and logging | |||
There was a problem hiding this comment.
The comment on L304 indicates if not using Envoy gRPC, only latency is emitted. Does this comment override that one? If so, would be good to update that comment too.
There was a problem hiding this comment.
Updated. Let me know if the new formulation is clearer.
hypnoce
force-pushed
the
ext_authz_fields
branch
from
f3b7a3c
to
f61c35e
Compare
hypnoce
force-pushed
the
ext_authz_fields
branch
from
1115249
to
ee539a2
Compare
… and logging Signed-off-by: JACQUES Francois <[email protected]>
…ds in log and CEL Signed-off-by: JACQUES Francois <[email protected]>
Signed-off-by: JACQUES Francois <[email protected]>
hypnoce
force-pushed
the
ext_authz_fields
branch
from
ee539a2
to
b88b78e
Compare
| EXPECT_THAT(ext_authz_logging_info->getField("bytesReceived"), | ||
| testing::VariantWith<int64_t>(ext_authz_logging_info->bytesReceived().value())); | ||
| } else { | ||
| EXPECT_THAT(ext_authz_logging_info->getField("bytesReceived"), |
There was a problem hiding this comment.
I don't think this case is ever run; it should be failing with the current version that calls value() instead of has_value() in the check. Please make sure this case (field is requested, but not value available) is covered.
There was a problem hiding this comment.
This case is ran. It does not fail because the value is actually set to 0
, so the getField returns monostate (has_value is truthy while value() is falsy which is clearly misleading).
Yes I was a bit confused I thought you wanted to check the values are non 0. Will change that. |
|
/assign @antoniovleonti You may want to take a look? as you added the billing/logging feature |
Signed-off-by: JACQUES Francois <[email protected]>
Signed-off-by: JACQUES Francois <[email protected]>
Signed-off-by: JACQUES Francois <[email protected]>
Signed-off-by: JACQUES Francois <[email protected]>
Signed-off-by: JACQUES Francois <[email protected]>
I think you can add |
|
/retest |
|
@ggreenway @tyxia @antoniovleonti Thanks for your comments. Anything else to change ? |
Signed-off-by: François JACQUES <[email protected]>
Commit Message:
ext_authz: expose fields latency, bytesSent and bytesReceived for CEL and logging
Additional Description:
This PR adds field access support for ext_authz filter state object
ExtAuthzLoggingInfo. It exposes simple fieldsThis enables usage of ext_authz filter state in access logs, and any filter/logger using CEL after #35698.
Risk Level: Low
Testing:
Added field access test with valid and invalid field names
Docs Changes: added in proto api and docs
Release Notes: added filter state field latency, bytesSent and bytesReceived access for CEL and logging.
Platform Specific Features: none
PS: I'm not a native english speaker please let me know if I need to change any docs.