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.

Sign up for GitHub

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

Jump to bottom

[Audit Logging] Xds Audit Logger Registry. #32828

Merged
merged 29 commits into from Apr 25, 2023
Merged

[Audit Logging] Xds Audit Logger Registry. #32828

merged 29 commits into from Apr 25, 2023

Conversation

rockspore
Copy link
Contributor

@rockspore rockspore commented Apr 6, 2023 •
edited

Third-party loggers will be added in subsequent PRs once the logger factory APIs are available to validate the configs here.

This registry is used in xds_http_rbac_filter.cc to generate service config json.

@rockspore rockspore added lang/core release notes: no Indicates if PR should not be in release notes labels Apr 6, 2023
@grpc-checks grpc-checks bot added bloat/low and removed bloat/none labels Apr 6, 2023
@rockspore rockspore marked this pull request as ready for review April 10, 2023 19:43
@rockspore rockspore marked this pull request as draft April 10, 2023 22:15
@rockspore rockspore marked this pull request as ready for review April 11, 2023 17:39
markdroth
markdroth reviewed Apr 24, 2023
View reviewed changes
src/core/ext/xds/xds_audit_logger_registry.cc Outdated Show resolved Hide resolved
test/core/xds/xds_audit_logger_registry_test.cc Outdated Show resolved Hide resolved
test/core/xds/xds_audit_logger_registry_test.cc Show resolved Hide resolved
test/core/xds/xds_audit_logger_registry_test.cc Show resolved Hide resolved
src/core/ext/xds/xds_listener.cc Outdated
@@ -60,6 +60,8 @@
#include "src/core/lib/gprpp/validation_errors.h"
#include "src/core/lib/iomgr/sockaddr.h"

// IWYU pragma: no_include "envoy/config/rbac/v3/rbac.upb.h"
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to understand why IWYU thinks that header is needed here. But ultimately, I don't think it does any harm to include it, so even if IWYU is wrong, I'd prefer to include it instead of having this pragma.

@rockspore
Copy link
Contributor Author

I can't comment on the IWYU so I'm responding here. I added the include of "envoy/config/rbac/v3/rbac.upb.h" for now. Meanwhile, I'm running tools/distrib/iwyu.sh locally. IIRC, it didn't say anything meaningful last time I ran it. I will report back when it finishes this time (can take a while).

@rockspore
Copy link
Contributor Author

Local iwyu.out didn't not include anything useful about why this header was needed.

markdroth
markdroth approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks great!

@rockspore rockspore enabled auto-merge (squash) April 25, 2023 04:17
@rockspore rockspore changed the title [Audit Logging] Xds Audit Logger Registry [Audit Logging] Xds Audit Logger Registry. Apr 25, 2023
@rockspore rockspore merged commit 2917804 into grpc:master Apr 25, 2023
62 of 64 checks passed
@rockspore rockspore deleted the xds-audit-registry branch April 25, 2023 15:00
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Apr 26, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
@rockspore @XuanWang-Amos
[Audit Logging] Xds Audit Logger Registry (grpc#32828)
9d5f34e 
Third-party loggers will be added in subsequent PRs once the logger
factory APIs are available to validate the configs here.

This registry is used in `xds_http_rbac_filter.cc` to generate service
config json.
markdroth
markdroth reviewed May 3, 2023
View reviewed changes
src/core/ext/xds/xds_http_rbac_filter.cc
@@ -412,6 +438,32 @@ Json ParseHttpRbacToJson(const envoy_extensions_filters_http_rbac_v3_RBAC* rbac,
}
inner_rbac_json.emplace("policies", std::move(policies_object));
}
// Flatten the nested messages defined in rbac.proto
if (envoy_config_rbac_v3_RBAC_has_audit_logging_options(rules)) {
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that we need env var protection for this code that looks at the new xDS fields. Can you please put together a PR to add that ASAP? I want to make sure that this PR doesn't make its way into a release branch before we have the env var protection, or else we'll have to deal with broken releases in the wild, which is a major pain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. Will work on it right now.

paulosjca pushed a commit to paulosjca/grpc that referenced this pull request May 4, 2023
@rockspore @paulosjca
[Audit Logging] Xds Audit Logger Registry (grpc#32828)
a5b93f2 
Third-party loggers will be added in subsequent PRs once the logger
factory APIs are available to validate the configs here.

This registry is used in `xds_http_rbac_filter.cc` to generate service
config json.
wanlin31 pushed a commit that referenced this pull request May 18, 2023
@rockspore @wanlin31
[Audit Logging] Xds Audit Logger Registry (#32828)
e81f78a 
Third-party loggers will be added in subsequent PRs once the logger
factory APIs are available to validate the configs here.

This registry is used in `xds_http_rbac_filter.cc` to generate service
config json.
@yijiem 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 erm-g removed the release notes: yes Indicates if PR needs to be in release notes label Jun 12, 2023
@yijiem yijiem added the release notes: no Indicates if PR should not be in release notes label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdroth markdroth markdroth approved these changes

@gtcooke94 gtcooke94 gtcooke94 approved these changes

Assignees
No one assigned
Labels
imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rockspore @gtcooke94 @markdroth @yijiem @grpc-kokoro @erm-g