-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(functionsv2): Add sample for Audit Logs #2304
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
Conversation
Sample function prints fields, and has been tested with GCS write audit logs.
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
I removed a log statement that the tests were looking for 🤦
grayside
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very good sample PR, thank you. I can tell a lot of care was taken to track existing practices & patterns.
Unfortunately the google-cloudevents-go library is not recommended for use, we need to try another way. You can see in https://github.com/GoogleCloudPlatform/golang-samples/blob/main/functions/functionsv2/hellopubsub/hello_pubsub_test.go we created an ad hoc struct. We hope to switch to that library in the future.
| "log" | ||
|
|
||
| "github.com/cloudevents/sdk-go/v2/event" | ||
| auditevents "github.com/googleapis/google-cloudevents-go/cloud/audit/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking: Unfortunately this library is not GA and we shouldn't use it yet. I'm curious how you found your way to this library, and what we might do to prevent others for deciding to use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this to my attention. I've removed use of this library in favor of some minimal structs.
As for finding the library:
There is a documentation page that appears to conflate the two things called "cloud event": https://cloud.google.com/eventarc/docs/cloudevents#open-source
there are additional references in the Common Events section that link to protobufs, and the google-cloudevents repo docs.
| payload auditevents.ProtoPayload | ||
| expectedLogs []string | ||
| }{ | ||
| {"sample-output", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: name declarations for longer struct definitions improve readability. I went looking for whether we have a standard on this, and I couldn't find one so it may just be opinion ;)
Since I'm curious if I'm missing something, @codyoss do you know?
I skimmed through https://golang.org/doc/effective_go and https://github.com/golang/go/wiki/CodeReviewComments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convention or not, i'm happy to add that. it seems like a reasonable practice, even if it is not standard. (will leave comment open for cody's input).
instead, use minimal structs to unmarshal protoPayload.
muncus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i've addressed your comments, and this is ready for another round of review.
(some comments left open for your input, feel free to resolve if you're satisfied).
changes to canonical sample were made in GoogleCloudPlatform/nodejs-docs-samples#2433
|
|
||
| // AuditLogProtoPayload represents AuditLog within the LogEntry.protoPayload | ||
| // See https://cloud.google.com/logging/docs/reference/audit/auditlog/rest/Shared.Types/AuditLog | ||
| type AuditLogProtoPayload struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we me making use of generated proto stubs? https://github.com/googleapis/go-genproto/blob/4c6863e31247658eee07a5b0075340c04399d1d4/googleapis/cloud/audit/audit_log.pb.go#L43
Or does this even lib not work with those types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Cody, we're following a medium-term pattern of inlining the data structure. We're doing this both as a bridge to the future library dependency we plan to take (from https://github.com/googleapis/google-cloudevents libraries) and to cover a gap in documentation that's still being resolved.
I'm not strongly opposed to go using that reference, if you feel it's a reasonable thing for developers to take direct dependency. It would be a difference for this sample vs. the other GCF v2 samples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline struct is fine by me, I just wanted to make sure this choice was intentional. Anyone that uses our client libraries would already have this dep in their tree.
Sample function prints fields to the logs, and has been manually tested with GCS write audit logs.
This is the Go version of this canonical sample: https://github.com/GoogleCloudPlatform/nodejs-docs-samples/blob/2991f86146742d00725c4dd73bf3719bd25e809b/functions/v2/index.js#L65