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.

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

Merged
merged 2 commits into from
Sep 23, 2020
Merged

Conversation

kyessenov
Copy link
Contributor

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

Signed-off-by: Kuat Yessenov <kuat@google.com>
Copy link
Member

@htuch htuch left a 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_;
Copy link
Member

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?

Copy link
Contributor Author

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*.

Copy link
Member

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?

Copy link
Contributor Author

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";
Copy link
Member

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.

Copy link
Contributor Author

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>
@kyessenov
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13163 (comment) was created by @kyessenov.

see: more, trace.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch
Copy link
Member

htuch commented Sep 22, 2020

/retest

@yanavlasov yanavlasov merged commit c10e8ec into envoyproxy:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants