[Audit Logging] Stdout logger implementation #33026

rockspore · 2023-05-05T18:19:50Z

The logger uses absl::FPrintF to write to stdout. After reading a number of sources online, I got the impression that std::fwrite which is used by absl::FPrintF is atomic so there is no locking required here.

Automated fix for refs/heads/stdout-logger

gtcooke94

Looks good, a few comments

gtcooke94 · 2023-05-05T19:00:22Z

src/core/lib/security/authorization/stdout_logger.cc

+constexpr absl::string_view kName = "stdout_logger";
+const char kLogFormat[] =
+    "{\"grpc_audit_log\":{\"timestamp\":%d,\"rpc_method\":\"%s\",\"principal\":"
+    "\"%s\",\"policy_name\":\"%s\",\"matched_rule\":\"%s\",\"authorized\":%s}}";


Will this have implicit newlines, and if so do we want a multi-line log?

Good catch! I meant to add a newline to flush the buffer but forgot. Added.

I actually meant the other way around, do we want any newlines here at all, or one big single line that is the audit log?

As you also saw in the Go PR, we want one log entry to be one-liner.

test/core/security/grpc_audit_logging_test.cc

gtcooke94 · 2023-05-05T19:06:24Z

test/core/security/grpc_audit_logging_test.cc

  EXPECT_TRUE(AuditLoggerRegistry::FactoryExists(kName));
  EXPECT_FALSE(AuditLoggerRegistry::FactoryExists("unknown_logger"));
 }

+//
+//  StdoutLoggerTest


Would it be worth having StdOutLoggerTest in it's own file?

The stdout logger is added to the same library target in the BUILD file. I think it's easier to have things in the same test target.

test/core/security/grpc_audit_logging_test.cc

gtcooke94

This LGTM other than the question about multi-line vs. single-line logs

Automated fix for refs/heads/stdout-logger

markdroth

This looks good overall! Comments are relatively minor.

Please let me know if you have any questions. Thanks!

src/core/lib/security/authorization/stdout_logger.cc

test/core/security/grpc_audit_logging_test.cc

src/core/lib/security/authorization/stdout_logger.cc

test/core/security/grpc_audit_logging_test.cc

rockspore

Thanks for the review. I addressed the comments as I can for now. Will ask for another look once the timestamp format is finalized.

src/core/lib/security/authorization/stdout_logger.cc

test/core/security/grpc_audit_logging_test.cc

Automated fix for refs/heads/stdout-logger

markdroth

Looks great! Just a couple of minor nits remaining. Feel free to merge after addressing.

test/core/security/grpc_audit_logging_test.cc

markdroth

Looks great!

rockspore · 2023-05-16T01:20:30Z

In one recent build, the invocation of Log happened so soon that time_before_log was actually equal to time_at_log even with nanosecond:

test/core/security/grpc_audit_logging_test.cc:146
Expected: (time_at_log) > (time_before_log), actual: 2023-05-16T00:45:04.663233+00:00 vs 2023-05-16T00:45:04.663233+00:00

I changed it back to EXPECT_GE and EXPECT_LE to avoid flakiness in the future.

The logger uses `absl::FPrintF` to write to stdout. After reading a number of sources online, I got the impression that `std::fwrite` which is used by `absl::FPrintF` is atomic so there is no locking required here. --------- Co-authored-by: rockspore <rockspore@users.noreply.github.com>

rockspore and others added 4 commits May 5, 2023 17:01

stdout audit logger impl

16bd834

generate projects

f8f5ee1

Automated change: Fix sanity tests

195f90f

Merge pull request #21 from rockspore/create-pull-request/patch-f8f5ee1

1c224f3

Automated fix for refs/heads/stdout-logger

github-actions bot added the lang/core label May 5, 2023

rockspore added the release notes: no Indicates if PR should not be in release notes label May 5, 2023

rockspore requested review from gtcooke94 and markdroth May 5, 2023 18:22

grpc-checks bot added bloat/low per-call-memory/neutral per-channel-memory/neutral labels May 5, 2023

gtcooke94 reviewed May 5, 2023

View reviewed changes

gtcooke94 removed the request for review from markdroth May 5, 2023 20:31

rockspore requested a review from gtcooke94 May 5, 2023 21:25

rockspore added 2 commits May 5, 2023 21:26

address PR comments

5cb67df

constexpr

fc13177

gtcooke94 reviewed May 8, 2023

View reviewed changes

gtcooke94 requested a review from markdroth May 8, 2023 14:29

rockspore mentioned this pull request May 12, 2023

authz: Stdout logger grpc/grpc-go#6230

Merged

rockspore and others added 5 commits May 12, 2023 14:46

Merge branch 'master' of github.com:grpc/grpc into stdout-logger

3cbd452

switch to public json hdr

1064479

sanity

db15d5c

Automated change: Fix sanity tests

4ecbc71

Merge pull request #23 from rockspore/create-pull-request/patch-db15d5c

5c0fc5b

Automated fix for refs/heads/stdout-logger

markdroth reviewed May 12, 2023

View reviewed changes

rockspore added 2 commits May 12, 2023 22:03

address PR comments

defee80

fix build

6092130

rockspore commented May 12, 2023

View reviewed changes

rockspore and others added 2 commits May 12, 2023 23:11

Automated change: Fix sanity tests

eedd236

log in seconds

2738e47

Merge pull request #24 from rockspore/create-pull-request/patch-6092130

f6427bb

Automated fix for refs/heads/stdout-logger

rockspore requested a review from markdroth May 15, 2023 16:37

markdroth approved these changes May 15, 2023

View reviewed changes

test/core/security/grpc_audit_logging_test.cc Outdated Show resolved Hide resolved

test/core/security/grpc_audit_logging_test.cc Outdated Show resolved Hide resolved

rockspore added 2 commits May 15, 2023 22:56

address PR comments

7c765ee

nanosecond

aca75f2

markdroth approved these changes May 16, 2023

View reviewed changes

allow equal time

82a2406

rockspore merged commit 6df358c into grpc:master May 16, 2023
63 of 65 checks passed

rockspore deleted the stdout-logger branch May 16, 2023 17:02

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 16, 2023

yijiem added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels May 31, 2023

erm-g removed the release notes: yes Indicates if PR needs to be in release notes label Jun 12, 2023

yijiem added the release notes: no Indicates if PR should not be in release notes label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Audit Logging] Stdout logger implementation #33026

[Audit Logging] Stdout logger implementation #33026

rockspore commented May 5, 2023

gtcooke94 left a comment

gtcooke94 May 5, 2023

rockspore May 5, 2023

gtcooke94 May 8, 2023

rockspore May 12, 2023

gtcooke94 May 5, 2023

rockspore May 5, 2023

gtcooke94 left a comment

markdroth left a comment

rockspore left a comment

markdroth left a comment

markdroth left a comment

rockspore commented May 16, 2023

[Audit Logging] Stdout logger implementation #33026

[Audit Logging] Stdout logger implementation #33026

Conversation

rockspore commented May 5, 2023

gtcooke94 left a comment

Choose a reason for hiding this comment

gtcooke94 May 5, 2023

Choose a reason for hiding this comment

rockspore May 5, 2023

Choose a reason for hiding this comment

gtcooke94 May 8, 2023

Choose a reason for hiding this comment

rockspore May 12, 2023

Choose a reason for hiding this comment

gtcooke94 May 5, 2023

Choose a reason for hiding this comment

rockspore May 5, 2023

Choose a reason for hiding this comment

gtcooke94 left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

rockspore left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

rockspore commented May 16, 2023