Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

authz: Stdout logger #6230

Merged
merged 30 commits into from May 17, 2023
Merged

authz: Stdout logger #6230

merged 30 commits into from May 17, 2023

Conversation

erm-g
Copy link
Contributor

@erm-g erm-g commented Apr 28, 2023

Part of RBAC audit logging effort for authz. The functionality in this PR includes a built in StdOut logger (prints the whole Event to stdout in json format)

RELEASE NOTES:

  • TBD

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, I think we definitely want to split this into a PR for the stdout logger and a PR for XDS registry and parsing, then title them accordingly

authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger_test.go Outdated Show resolved Hide resolved
xds/internal/httpfilter/rbac/audit/audit_logger.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rockspore rockspore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Left some comments. Also suggest simply some variable names following Go's style. Please follow existing code in the repo to do logging instead of fmt.Println

authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout_logger.go Outdated Show resolved Hide resolved
xds/internal/httpfilter/rbac/audit/audit_logger.go Outdated Show resolved Hide resolved
@rockspore rockspore changed the title Audit logger registry authz: Audit logger registry Apr 28, 2023
@erm-g erm-g marked this pull request as ready for review May 8, 2023 02:14
@erm-g erm-g changed the title authz: Audit logger registry authz: Stdout logger May 8, 2023
@erm-g erm-g requested review from rockspore and gtcooke94 May 8, 2023 16:18
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Have a few comments and some golang convention items to change

authz/audit/audit_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Have a few comments and some golang convention items to change

@easwars
Copy link
Contributor

easwars commented May 10, 2023

@erm-g Please don't mark comment threads as resolved. The policy we follow is that the reviewer marks them as resolved, once they feel that the comment has been appropriately addressed. It's a pain to unresolve every comment and see if they have been addressed appropriately. Thanks.

authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
erm-g added 2 commits May 14, 2023 03:56
…ger struct func. Add timestamp testing logic. Add registry presense test.
@erm-g erm-g requested a review from rockspore May 15, 2023 03:03
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Show resolved Hide resolved
@easwars
Copy link
Contributor

easwars commented May 15, 2023

@erm-g : Looks like there are more comments from the security team here. Please let me know once you have addressed all of them, and want me to take another pass. Thanks.

@erm-g erm-g requested a review from rockspore May 16, 2023 02:16
authz/audit/stdout/stdout_logger.go Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Outdated Show resolved Hide resolved
authz/audit/stdout/stdout_logger_test.go Show resolved Hide resolved
authz/audit/stdout/stdout_logger.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rockspore rockspore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit was not resolved yet. The rest LGTM! Thanks.

Comment on lines 70 to 71
l := log.New(&buf, "", 0)
builder := &loggerBuilder{goLogger: l}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: inline l into the next line.

builder := &loggerBuilder{goLogger: l}
auditLogger := builder.Build(nil)
auditLogger.Log(test.event)
var container map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Consider adding newlines to logically separate the steps of the test. At the very least, it would be good to separate out the following steps using newlines: setup, actual test logic, verification.

@easwars
Copy link
Contributor

easwars commented May 16, 2023

LGTM. Some very minor nits. Feel free to merge without another pass from me.

@erm-g erm-g merged commit 52fef6d into grpc:master May 17, 2023
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants