-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
rbac: expose filter state #13163
rbac: expose filter state #13163
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
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.
LGTM modulo nits.
/wait
} | ||
|
||
protected: | ||
ProtobufWkt::Arena* arena_; |
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.
How is this being used now?
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.
serializeString
returns a string by value, so a copy is made on this arena, because CelValue takes const string*
.
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.
Maybe I'm missing something, but this looks like just a pointer to the arena that gets assigned above, how is the copy done?
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.
arena_
is used in the implementation of the wrapper. The base class is shared with other wrappers, which is how it gets passed down. I am working on making this flow better upstream, and get rid of the arena use in this case, but it's not simple.
ProtobufWkt::Arena arena; | ||
wrapper.Produce(&arena); | ||
|
||
const auto key = "filter_state_key"; |
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.
Nit: prefer making string types explicit rather than too much auto.
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.
Sure
Signed-off-by: Kuat Yessenov <kuat@google.com>
/retest |
Retrying Azure Pipelines, to retry CircleCI checks, use |
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.
LGTM, thanks!
/retest |
Commit Message: expose filter state values to CEL context. This uses string serialization due to better efficiency.
Risk Level: low (opt-in)
Testing: unit
Docs Changes: yes
Release Notes: no