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] Audit logging config translation by rbac service config parser #33145

Merged
merged 23 commits into from May 17, 2023
Merged

[Audit Logging] Audit logging config translation by rbac service config parser #33145

merged 23 commits into from May 17, 2023

Conversation

rockspore
Copy link
Contributor

This translates the service config from HTTP RBAC filter into the rbac policy, which is used to construct authorization engines.

rockspore and others added 16 commits May 5, 2023 02:50
@rockspore
audit logging in service config parser
7ac6a4d
@rockspore
remove unused using decl and hdrs
72487d0
@rockspore
Automated change: Fix sanity tests
d2f15ec
@rockspore
Merge pull request #20 from rockspore/create-pull-request/patch-72487d0
bbe16bf 
Automated fix for refs/heads/service-config-parser
@rockspore
stdout audit logger impl
16bd834
@rockspore
generate projects
f8f5ee1
@rockspore
Automated change: Fix sanity tests
195f90f
@rockspore
Merge branch 'stdout-logger' of github.com:rockspore/grpc into servic…
50d7ad7 
…e-config-parser
@rockspore
Merge pull request #21 from rockspore/create-pull-request/patch-f8f5ee1
1c224f3 
Automated fix for refs/heads/stdout-logger
@rockspore
Merge branch 'stdout-logger' of github.com:rockspore/grpc into servic…
296ac6c 
…e-config-parser
@rockspore
remove fixture by using stdout logger
0e598e9
@rockspore
merge from origin
361798a
@rockspore
Automated change: Fix sanity tests
67e5571
@rockspore
Merge pull request #22 from rockspore/create-pull-request/patch-361798a
769dbc5 
Automated fix for refs/heads/service-config-parser
@rockspore
Merge branch 'master' of github.com:grpc/grpc into service-config-parser
9864edf
@rockspore
public json hdr
6adbc3f
@rockspore rockspore added the release notes: no Indicates if PR should not be in release notes label May 16, 2023
@rockspore rockspore requested review from gtcooke94 and erm-g May 16, 2023 17:53
gtcooke94
gtcooke94 reviewed May 16, 2023
View reviewed changes
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.

One question, otherwise looks good.
Though I am certainly no JSON C++ expert, the code makes logical sense at least :)

src/core/ext/filters/rbac/rbac_service_config_parser.cc Outdated Show resolved Hide resolved
@rockspore rockspore requested a review from markdroth May 16, 2023 22:32
markdroth
markdroth reviewed May 17, 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 good overall! There are some details to address, though.

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

src/core/ext/filters/rbac/rbac_service_config_parser.cc Outdated Show resolved Hide resolved
src/core/ext/filters/rbac/rbac_service_config_parser.cc Outdated Show resolved Hide resolved
src/core/ext/filters/rbac/rbac_service_config_parser.cc Outdated Show resolved Hide resolved
src/core/ext/filters/rbac/rbac_service_config_parser.cc Outdated Show resolved Hide resolved
src/core/lib/security/authorization/grpc_authorization_engine.h Outdated Show resolved Hide resolved
test/core/ext/filters/rbac/rbac_service_config_parser_test.cc Outdated Show resolved Hide resolved
test/core/ext/filters/rbac/rbac_service_config_parser_test.cc Outdated Show resolved Hide resolved
@rockspore rockspore requested a review from markdroth May 17, 2023 16:52
@rockspore rockspore requested a review from markdroth May 17, 2023 19:09
markdroth
markdroth approved these changes May 17, 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.

Looks great! Feel free to merge after addressing the remaining comments.

src/core/ext/filters/rbac/rbac_service_config_parser.cc Outdated
@@ -200,7 +200,7 @@ struct RbacConfig {
int action;
std::map<std::string, Policy> policies;
// Defaults to 0 since its json field is optional.
Copy link
Member

Choose a reason for hiding this comment

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

s/0/kNone/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

test/core/ext/filters/rbac/rbac_service_config_parser_test.cc Outdated
EXPECT_EQ(logger_configs_.find("test_logger")->second, "{\"foo\":\"bar\"}");
const auto& loggers =
parsed_rbac_config->authorization_engine(0)->audit_loggers();
ASSERT_EQ(loggers.size(), 2);
Copy link
Member

Choose a reason for hiding this comment

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

Suggest writing this as:

EXPECT_THAT(
    loggers
    ::testing::ElementsAre(
        ::testing::Property(&AuditLogger::name, "stdout_logger"),
        ::testing::Property(&AuditLogger::name, kLoggerName)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I had to use

  EXPECT_THAT(parsed_rbac_config->authorization_engine(0)->audit_loggers(),
              ::testing::ElementsAre(::testing::Pointee(::testing::Property(
                                         &AuditLogger::name, "stdout_logger")),
                                     ::testing::Pointee(::testing::Property(
                                         &AuditLogger::name, kLoggerName))));

because the vector stores unique pointers.

@rockspore rockspore merged commit f60d0c7 into grpc:master May 17, 2023
63 of 65 checks passed
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 17, 2023
wanlin31 pushed a commit that referenced this pull request May 18, 2023
@rockspore @wanlin31
[Audit Logging] Audit logging config translation by rbac service conf…
18b42c5 
…ig parser (#33145)

This translates the service config from HTTP RBAC filter into the rbac
policy, which is used to construct authorization engines.
eugeneo added a commit that referenced this pull request May 18, 2023
@eugeneo
Revert "[Audit Logging] Audit logging config translation by rbac serv…
e7bbfd4 
…ice config parser (#33145)"

This reverts commit f60d0c7.
@eugeneo eugeneo mentioned this pull request May 18, 2023
Merged
veblush pushed a commit that referenced this pull request May 18, 2023
@eugeneo
Revert "[Audit Logging] Audit logging config translation by rbac serv…
922b18f 
…ice config parser" (#33178)

Reverts #33145

It causes internal breakages.
@rockspore rockspore mentioned this pull request May 18, 2023
Merged
rockspore added a commit that referenced this pull request May 18, 2023
@rockspore
[Audit Logging] Second attempt: rbac service config parsing with audi…
0d70c63 
…t logging (#33183)

This is basically the same as #33145 except that the ctor `Rules()`
cannot be default but have to explicitly set a default audit condition.
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request May 19, 2023
@rockspore @eugeneo
[Audit Logging] Second attempt: rbac service config parsing with audi…
a1ae4f1 
…t logging (grpc#33183)

This is basically the same as grpc#33145 except that the ctor `Rules()`
cannot be default but have to explicitly set a default audit condition.
@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

@gtcooke94 gtcooke94 gtcooke94 left review comments

@markdroth markdroth markdroth approved these changes

@erm-g erm-g Awaiting requested review from erm-g

Assignees
No one assigned
Labels
bloat/low 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
5 participants
@rockspore @gtcooke94 @markdroth @yijiem @erm-g