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: add support for matching on route metadata #36957

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

agrawroh
Copy link
Contributor

@agrawroh agrawroh commented Nov 3, 2024

Background

This PR adds a new option called sourced_metadata to RBAC permissions and principals which allows specifying an optional source for the metadata to be matched. Currently it only supports Dynamic Metadata and Route Metadata. More options could be added later.

Fixes: #34913


Commit Message: rbac: add support for matching on route metadata
Additional Description: This PR adds a new option called sourced_metadata to RBAC permissions and principals which allows specifying an optional source for the metadata to be matched.
Risk Level: Low
Testing: Added Unit & Integration Tests
Docs Changes: Added
Release Notes: Added

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 @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #36957 was opened by agrawroh.

see: more, trace.

@agrawroh agrawroh force-pushed the route_meta branch 3 times, most recently from 16908e6 to 554622a Compare November 4, 2024 06:44
@agrawroh
Copy link
Contributor Author

agrawroh commented Nov 5, 2024

@yanavlasov @wbpcode Could you please help review this?

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. I am fine to this new feature overall. Only some minor comments are added.

api/envoy/config/rbac/v3/rbac.proto Show resolved Hide resolved
source/extensions/filters/common/rbac/matchers.cc Outdated Show resolved Hide resolved
@agrawroh
Copy link
Contributor Author

agrawroh commented Nov 7, 2024

Thanks for this contribution. I am fine to this new feature overall. Only some minor comments are added.

@wbpcode Thank you so much for taking a look. I addressed your feedback and am happy to do the changes as needed.

@agrawroh
Copy link
Contributor Author

agrawroh commented Nov 7, 2024

/retest

1 similar comment
@agrawroh
Copy link
Contributor Author

agrawroh commented Nov 7, 2024

/retest

@wbpcode
Copy link
Member

wbpcode commented Nov 8, 2024

Thanks for the update. Some new new minor comments are flushed.

cc @yanavlasov for a second pass for this rbac change. :)

Signed-off-by: Rohit Agrawal <[email protected]>
@agrawroh
Copy link
Contributor Author

agrawroh commented Nov 8, 2024

/retest

yanavlasov
yanavlasov previously approved these changes Nov 8, 2024
@yanavlasov
Copy link
Contributor

LGM, will wait for @wbpcode approval and then merge.

/wait-any

@agrawroh
Copy link
Contributor Author

agrawroh commented Nov 8, 2024

/retest

1 similar comment
@agrawroh
Copy link
Contributor Author

agrawroh commented Nov 8, 2024

/retest

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM overall. Only some nit comments to the annotation and tests. Then we are good to land it.

type.matcher.v3.MetadataMatcher metadata = 7 [
deprecated = true,
(envoy.annotations.deprecated_at_minor_version) = "3.0",
(envoy.annotations.disallowed_by_default) = true
Copy link
Member

Choose a reason for hiding this comment

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

Thanks so much. But I think we doesn't need to add this (envoy.annotations.disallowed_by_default) now.

type.matcher.v3.MetadataMatcher metadata = 7 [
deprecated = true,
(envoy.annotations.deprecated_at_minor_version) = "3.0",
(envoy.annotations.disallowed_by_default) = true
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

Comment on lines +702 to +719
TEST(PortRangeMatcher, EdgeCases) {
Envoy::Network::MockConnection conn;
Envoy::Http::TestRequestHeaderMapImpl headers;
NiceMock<StreamInfo::MockStreamInfo> info;

// Test boundary conditions
envoy::type::v3::Int32Range range;
range.set_start(1);
range.set_end(65535);

// Test port at start of range
Envoy::Network::Address::InstanceConstSharedPtr addr1 =
Envoy::Network::Utility::parseInternetAddressNoThrow("1.2.3.4", 1, false);
info.downstream_connection_info_provider_->setLocalAddress(addr1);
checkMatcher(PortRangeMatcher(range), true, conn, headers, info);

// Test port at end of range - 1
Envoy::Network::Address::InstanceConstSharedPtr addr2 =
Copy link
Member

Choose a reason for hiding this comment

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

Seems these are unreleated to this PR?

@wbpcode
Copy link
Member

wbpcode commented Nov 9, 2024

/wait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants