api: add stdout audit logger for RBAC audit logging. #26453
Conversation
Signed-off-by: Luwei Ge <lwge@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Luwei Ge <lwge@google.com>
Signed-off-by: Luwei Ge <lwge@google.com>
| // Custom configuration for the RBAC audit logger that writes log entries | ||
| // directly to the operating system's standard output. | ||
| // The logger outputs in JSON format and is currently not configurable. | ||
| message StdoutAuditLog { |
There was a problem hiding this comment.
stdout is only one type of log sink, that may not be usable by everyone. Please add something similar to the request access log configuration https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/accesslog/v3/accesslog.proto#config-accesslog-v3-accesslog which allows different implementations of log sinks (i.e. gRPC).
You can exclude the filter field, since there are no defined filtering semantics for RBAC log yet (although I can see someone only logging denied requests).
There was a problem hiding this comment.
Thanks for the review! I realized I forgot to include enough context in the PR description, this logger type is specifically for the API created in #26001 (#26415 as an amendment). The AuditCondition there specifies when to log the request and the logger configuration itself is already a TypedExtensionConfig. We don't need an access log like configuration in this case, right?
| // Custom configuration for the RBAC audit logger that writes log entries | ||
| // directly to the operating system's standard output. | ||
| // The logger outputs in JSON format and is currently not configurable. | ||
| message StdoutAuditLog { |
There was a problem hiding this comment.
I think the directory/file names and the proto name should match. If we're calling this StdoutLogger, then let's rename the directory from stream to stdout and the filename from stream.proto to stdout.proto.
There was a problem hiding this comment.
Oh, wait, just realized that these names are what is used for the stdout access logger you mentioned above. In that case, this is fine as-is.
|
/lgtm api |
|
I believe nothing needs to be changed here till a new round of review but I am not able to remove the |
…t_logger Signed-off-by: Luwei Ge <lwge@google.com>
Commit Message: Add stdout audit logger for RBAC audit logging.
Additional Description: This is the built-in audit logger gRPC plans to support for now. We intentionally make it not configurable until we have use cases that need to modify the behavior such as log format. The layout resembles StdoutAccessLog.
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
cc @markdroth @ejona86