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

api: wrap the audit logger extension with an is_optional bool #26415

Merged
merged 3 commits into from
Apr 4, 2023
Merged

api: wrap the audit logger extension with an is_optional bool #26415

merged 3 commits into from
Apr 4, 2023

Conversation

rockspore
Copy link
Contributor

Commit Message: Make the audit logger config optional
Risk Level: low
Docs Changes: N/A
Release Notes: N/A

API Considerations:
We decided to make individual audit logger configurations optional such that if the data plane is not updated to support a new type of logger, we can configure it to not NACK the RBAC filter entirely.

It's technically a breaking change to #26001 but that one was merged less than 10 days ago so implementation exists for the API and nobody should be using it right now.

@rockspore
wrap the audit logger extension with an is_optional bool
65ec858 
Signed-off-by: Luwei Ge <lwge@google.com>
@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #26415 was opened by rockspore.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #26415 was opened by rockspore.

see: more, trace.

@rockspore rockspore marked this pull request as ready for review March 28, 2023 18:36
@markdroth
Copy link
Contributor

/lgtm api

@rockspore
logger configs should be optional
822c089 
Signed-off-by: Luwei Ge <lwge@google.com>
@rockspore
Copy link
Contributor Author

Per offline discussion, the logger config should be completely optional so that there is no operation when audit condition is present but no logger config is given.

cc @markdroth @ejona86 @mattklein123

@markdroth
Copy link
Contributor

/lgtm api

@wbpcode
Copy link
Member

wbpcode commented Mar 29, 2023

ci need to be fixed.

@wbpcode
Copy link
Member

wbpcode commented Mar 29, 2023 •
edited

cc @markdroth seems that there are lots of is_optional wrapper for the typed extension config in the /api. Will we consider to add the is_optional field to the TypedExtentionConfig directly to tell an extension is optional?

@wbpcode
Copy link
Member

wbpcode commented Mar 29, 2023

/wait

@rockspore
Copy link
Contributor Author

ci need to be fixed.

I am not sure why it was complaining Unable to find extension category: envoy.rbac.audit_loggers since this category was already introduced in my last PR. Do you know what went wrong? @wbpcode Thanks!

@wbpcode
Copy link
Member

wbpcode commented Mar 29, 2023

You can add a [#not-implemented-hide:] to the AuditLoggerConfig to around this problem first.

@rockspore
Fix CI
c1a9f22 
Signed-off-by: Luwei Ge <lwge@google.com>
@markdroth
Copy link
Contributor

cc @markdroth seems that there are lots of is_optional wrapper for the typed extension config in the /api. Will we consider to add the is_optional field to the TypedExtentionConfig directly to tell an extension is optional?

I thought about something like that, but I wasn't sure if I wanted to take on that invasive of a change. :) But I agree that it would be a more general-purpose solution.

I'm curious as to what other @envoyproxy/api-shepherds think of this idea.

@rockspore
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #26415 (comment) was created by @rockspore.

see: more, trace.

@rockspore rockspore mentioned this pull request Mar 31, 2023
Merged
@rockspore
Copy link
Contributor Author

If we add such field to the TypedExtensionConfig directly, is it true that quite a number of existing extensions will have to modify their implementation to support this? I assume it'd not be hard but perhaps a little tedious.

Please let me know if this PR should wait on that. Thanks!

@rockspore
Copy link
Contributor Author

@mattklein123 Since you were the reviewer of #26001, would you mind taking a look at this as well? Thank you!

@repokitteh-read-only repokitteh-read-only bot removed the api label Apr 4, 2023
@mattklein123 mattklein123 merged commit 38d6cfa into envoyproxy:main Apr 4, 2023
75 checks passed
@rockspore rockspore deleted the logger-optinality branch April 4, 2023 03:08
@rockspore rockspore mentioned this pull request Apr 5, 2023
Merged
rockspore added a commit to grpc/grpc that referenced this pull request Apr 6, 2023
@rockspore
[third_party] update envoy-api dependency (#32807)
15ef046 
update to main commit 68d4315167352ffac71f149a43b8088397d3f33d

This is to include the latest RBAC related change
(envoyproxy/envoy#26415) for audit logging.

I intentionally did not run steps in
https://github.com/grpc/grpc/tree/master/third_party#updating-third_partyenvoy-api
because I saw some earlier changes also didn't do that and this
envoy-api change isn't going to be needed in Python any time soon.
RiverPhillips pushed a commit to RiverPhillips/envoy that referenced this pull request Apr 7, 2023
@rockspore @RiverPhillips
api: wrap the audit logger extension with an is_optional bool (envoyp…
023ad7f 
…roxy#26415)

Signed-off-by: Luwei Ge <lwge@google.com>
Signed-off-by: River Phillips <riverphillips1@gmail.com>
@erm-g erm-g mentioned this pull request Apr 26, 2023
Closed
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
@rockspore @XuanWang-Amos
[third_party] update envoy-api dependency (grpc#32807)
08c8314 
update to main commit 68d4315167352ffac71f149a43b8088397d3f33d

This is to include the latest RBAC related change
(envoyproxy/envoy#26415) for audit logging.

I intentionally did not run steps in
https://github.com/grpc/grpc/tree/master/third_party#updating-third_partyenvoy-api
because I saw some earlier changes also didn't do that and this
envoy-api change isn't going to be needed in Python any time soon.
wanlin31 pushed a commit to grpc/grpc that referenced this pull request May 18, 2023
@rockspore @wanlin31
[third_party] update envoy-api dependency (#32807)
b0118fa 
update to main commit 68d4315167352ffac71f149a43b8088397d3f33d

This is to include the latest RBAC related change
(envoyproxy/envoy#26415) for audit logging.

I intentionally did not run steps in
https://github.com/grpc/grpc/tree/master/third_party#updating-third_partyenvoy-api
because I saw some earlier changes also didn't do that and this
envoy-api change isn't going to be needed in Python any time soon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

@markdroth markdroth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rockspore @markdroth @wbpcode @mattklein123