Skip to content

rbac: expose filter state#13163

Merged
yanavlasov merged 2 commits intoenvoyproxy:masterfrom
kyessenov:filter_state_wrapper
Sep 23, 2020
Merged

rbac: expose filter state#13163
yanavlasov merged 2 commits intoenvoyproxy:masterfrom
kyessenov:filter_state_wrapper

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