rbac: add support for upstream ip policy. by conqerAtapple · Pull Request #17645 · envoyproxy/envoy

conqerAtapple · 2021-08-09T22:08:06Z

Based on the dicsussion in the issue #17410, this PR adds
implementation for ability to add rbac policy to filter upstream ip addresses for dynamically
resolved domains (by dynamic proxy filter).

risk: Medium
testing: Manual testing
api: added upstream_ip to RBAC::Policy::Permission

Signed-off-by: Jojy G Varghese jojy_varghese@apple.com

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2021-08-09T22:08:12Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17645 was opened by conqerAtapple.

see: more, trace.

conqerAtapple · 2021-08-09T22:15:13Z

PTAL @alyssawilk @htuch @yangminzhu.

yangminzhu

Also I think this should at least be covered in unit tests (preferably also in integration tests), could you add some tests?

yangminzhu · 2021-08-09T22:51:28Z

api/envoy/config/rbac/v3/rbac.proto

I wonder should we add a generic filter state matcher (similar to the dynamic metadata matcher) so that it can support other use cases in the future without the need to change the rbac API everytime?

cc @htuch for API review

yangminzhu · 2021-08-09T22:52:51Z

api/envoy/config/rbac/v3/rbac.proto

shouldn't this be 13?

conqerAtapple · 2021-08-10T00:38:21Z

Also I think this should at least be covered in unit tests (preferably also in integration tests), could you add some tests?

Thanks for looking :). Wanted to have a first pass to make sure that the changes are ok in general before I add tests.

htuch

Some API comments.

api/envoy/config/rbac/v3/rbac.proto

htuch · 2021-08-12T04:16:17Z

api/envoy/config/rbac/v3/rbac.proto

Can you explain in the documentation here what the resolution semantics are? What happens if there is a change in resolution during processing? What is the v4/v6 story? etc.

htuch · 2021-08-12T04:17:19Z

Tagging @alyssawilk (dynamic forward DNS) @lizan (RBAC) for implementation review.

conqerAtapple · 2021-08-12T17:40:11Z

@htuch thanks for reviewing the PR.

alyssawilk

Looks solid at a high level - let's add unit tests and an integration test to make sure it all hangs together!

alyssawilk · 2021-08-12T20:21:41Z

source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc

I'd actually like @mattklein123 's thoughts on this.
I'd mildly prefer to have rbac just look up the ip:host mapping to copying it all into stream info for every request, but that'd be a hard dep on the dynamic proxy code which may be worse in the long run.

If it were up to me I would probably leave this as is, but config guard tracking this in filter state to avoid the perf hit for people that don't care.

alyssawilk · 2021-08-12T20:23:16Z

source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc

is the right thing to do here to add the address, or replace the address?
I'd think if we did recreateStream (internal redirect) we'd want to remove old addresses rather than append to them, to make sure we only had the addresses we wanted to use

Do you think we should add a clear API so that the caller can then call clear and add? Or is it better to replace the set with a single address? The idea here is to have a set of resolved addresses for the upstream and if any of them match.

yeah I think in the long run we're going to want to support multiple addresses, but also handle recreateStream, so either overwrite an existing set, or clear it and add new data.
(if that doesn't make sense please ping me on slack)

source/extensions/filters/common/rbac/matchers.cc

conqerAtapple · 2021-08-12T23:22:04Z

@alyssawilk thanks for reviewing. I have resolved most of the suggestions and comments. There is still some questions which I do not have clear answers for. Would appreciate your input.

htuch

/lgtm api

envoy/stream_info/filter_state.h

source/extensions/filters/common/rbac/matchers.cc

lizan · 2021-08-13T20:31:40Z

api/envoy/config/rbac/v3/rbac.proto

I'm a bit hesitate to add a field to support an extension while dfp might be an exception here. Though shouldn't this be an example that we should have an upstream HTTP filter for RBAC? while I know the current upstream filter doesn't support this yet.

Agree that this could be solved with a upstream filter. As discussed in the issue, this is a DFP specific targeted changed to solve a particular use case. We probably need to revisit this when upstream filters are more powerful.

Thanks @lizan for reviewing

Yeah sounds reasonable for now. Shall we name the field more explicitly, or do we expect other filter might set that filter state as well?

@lizan I think the construct could be used in a general context when we have upstream filters in their full form. When we generalize the RBAC policy to act on ANY of the upstream ip addresses, I think the address set could be the source of truth for it. WDYT?

Yeah I'm not crazy about this hard coding either which is effectively going to live forever. I have a vague idea that I'm going to throw out there that we might want to explore.

What if we had some type of generic matcher extension mechanism. Basically this becomes a typed_extension_config. One of the things the extension matcher gets access to when it matches is filter state. The config for the typed extension can be whatever it wants including a string name, hard coded, etc. (If you upstream the extension it needs to be more generic but if you just have an internal extension you can hard code it all.)

alyssawilk · 2021-08-16T13:29:25Z

mind checking out the coverage fail? Looks like it's legit and this is missing some unit testing.
/wait

conqerAtapple · 2021-08-16T21:46:15Z

mind checking out the coverage fail? Looks like it's legit and this is missing some unit testing.
/wait

Will add unit + integration tests.

conqerAtapple · 2021-08-18T20:33:24Z

/retest

repokitteh-read-only · 2021-08-18T20:33:28Z

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

🐱

Caused by: a #17645 (comment) was created by @conqerAtapple.

see: more, trace.

alyssawilk

Thanks for all the improved testing! Here's some follow up thoughts

alyssawilk · 2021-08-19T18:19:53Z

envoy/stream_info/address_set_accessor.h

javadoc comments for envoy/ functions if you leave them here

Alternately looking at another example filterSate object we just use the impl everywhere and don't have an abstract base class. I think that'd be fine here too, especially if we add a key() function so all the files using the class have to include the Impl anyway

alyssawilk · 2021-08-19T18:22:55Z

envoy/stream_info/filter_state.h

Looks like in at least one other use case, the Filterstate::Object had
static const std::string& key();

which is nice because then only components who care about that object pull that key. WDYT?

source/extensions/filters/common/rbac/matchers.cc

alyssawilk · 2021-08-19T18:28:29Z

source/extensions/filters/common/rbac/matchers.cc

would ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::rbac) work over the MISC logger macro?

alyssawilk · 2021-08-19T18:38:20Z

source/extensions/filters/http/dynamic_forward_proxy/proxy_filter.cc

yeah I think in the long run we're going to want to support multiple addresses, but also handle recreateStream, so either overwrite an existing set, or clear it and add new data.
(if that doesn't make sense please ping me on slack)

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

conqerAtapple · 2021-09-24T04:57:36Z

@htuch @mattklein123 I have added port as additional matching parameter in the matcher. Also, removed the upstream ip set.

htuch

API LGTM modulo nit. Thanks!

api/envoy/extensions/rbac/matchers/upstream_ip_port/v3/upstream_ip_port_matcher.proto

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

conqerAtapple · 2021-09-26T22:05:48Z

@mattklein123 @htuch @alyssawilk @ggreenway I think this PR should be ready now. I am not sure why the dependency CI checks are failing.

htuch

Thanks!

/lgtm api

ggreenway · 2021-09-27T15:29:59Z

/retest

repokitteh-read-only · 2021-09-27T15:30:04Z

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

🐱

Caused by: a #17645 (comment) was created by @ggreenway.

see: more, trace.

mattklein123

Thanks LGTM with small remaining comments.

/wait

api/envoy/extensions/rbac/matchers/upstream_ip_port/v3/upstream_ip_port_matcher.proto

mattklein123 · 2021-09-27T15:43:42Z

source/extensions/extensions_build_config.bzl


-    "envoy.rbac.matchers.upstream_ip":     "//source/extensions/filters/common/rbac/matchers:upstream_ip_lib",
+    "envoy.rbac.matchers.upstream_ip_port":     "//source/extensions/filters/common/rbac/matchers:upstream_ip_port_lib",
    "envoy.rbac.matchers.upstream_port":   "//source/extensions/filters/common/rbac/matchers:upstream_port_lib",


I think this needs to get removed? Or is this staying to match on ports only? Would it be better to just have a single matcher and allow address to be optional and if so any address will match? So basically either port, address, or both need to be set?

Thanks @mattklein123 .
I can see how having both could be confusing but I have kept the ip-port matcher to qualify IP with Port for matching IP & Port (if provided). Wanted to keep Port matcher separate so that a) be independent (no dependency on IP) b) Have IP as first class citizen of the IP-Port matcher a Port as an added qualifier.

Please let me know if that makes sense.

I don't really follow. You have port optional in one of them. Why can't address be optional? Logically then shouldn't you have an address only matcher? I would remove the dedicated port matcher and just make a single matcher that requires either to be set or both.

I was hoping that we can avoid configurations where neither IP or Port are provided.

Will change to combine the matchers 👍

Just reject that case in code and I think that will turn out great, thank you.

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

alyssawilk · 2021-09-28T12:35:32Z

/retest

repokitteh-read-only · 2021-09-28T12:35:36Z

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

🐱

Caused by: a #17645 (comment) was created by @alyssawilk.

see: more, trace.

mattklein123

Almost there, thanks!

/wait

source/extensions/filters/common/rbac/matchers/upstream_ip_port.cc

- Added configuration check. Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

mattklein123

/wait

mattklein123 · 2021-09-28T18:51:35Z

source/extensions/filters/common/rbac/matchers/upstream_ip_port.cc

 UpstreamIpPortMatcher::UpstreamIpPortMatcher(
    const envoy::extensions::rbac::matchers::upstream_ip_port::v3::UpstreamIpPortMatcher& proto) {
+  if (!proto.has_upstream_ip() && !proto.has_upstream_port_range()) {
+    throw EnvoyException("Invalid UpstreamIpPortMatcher configuration - missing `upstream_ip` and "


Suggested change

throw EnvoyException("Invalid UpstreamIpPortMatcher configuration - missing `upstream_ip` and "

throw EnvoyException("Invalid UpstreamIpPortMatcher configuration - missing `upstream_ip` and/or "

Also, update the docs to make it more clear that one or both are required (I already mentioned this), thanks.

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

mattklein123

/wait

mattklein123 · 2021-09-28T20:27:41Z

api/envoy/extensions/rbac/matchers/upstream_ip_port/v3/upstream_ip_port_matcher.proto

+// Note that although both fields are optional, an exception will be thrown during configuration
+// load time if none of the fields are provided.


(Users don't care about exceptions)

Suggested change

// Note that although both fields are optional, an exception will be thrown during configuration

// load time if none of the fields are provided.

// Note that although both fields are optional, at least one of IP or port must be supplied. If only one is supplied the other is a wildcard match.

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

mattklein123

Awesome, thanks!

alyssawilk

Thanks for your patience slogging through this - looks great.

conqerAtapple requested review from alyssawilk and mattklein123 as code owners August 9, 2021 22:08

repokitteh-read-only bot added the api label Aug 9, 2021

repokitteh-read-only bot assigned htuch Aug 9, 2021

conqerAtapple force-pushed the rbac-upstream-dynamic-proxy branch from 7243aea to 72631b1 Compare August 9, 2021 22:11

yangminzhu reviewed Aug 9, 2021

View reviewed changes

conqerAtapple force-pushed the rbac-upstream-dynamic-proxy branch from 4ef7357 to 357904d Compare August 12, 2021 00:57

htuch reviewed Aug 12, 2021

View reviewed changes

htuch assigned lizan and alyssawilk Aug 12, 2021

alyssawilk added the waiting label Aug 12, 2021

repokitteh-read-only bot removed the waiting label Aug 12, 2021

alyssawilk reviewed Aug 12, 2021

View reviewed changes

htuch reviewed Aug 13, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 13, 2021

lizan reviewed Aug 13, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 16, 2021

repokitteh-read-only bot removed the waiting label Aug 17, 2021

mattklein123 self-assigned this Aug 19, 2021

alyssawilk reviewed Aug 19, 2021

View reviewed changes

Merge branch 'main' into rbac-upstream-dynamic-proxy

c5d5f2a

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

htuch previously requested changes Sep 26, 2021

View reviewed changes

api/envoy/extensions/rbac/matchers/upstream_ip_port/v3/upstream_ip_port_matcher.proto Show resolved Hide resolved

conqerAtapple added 2 commits September 26, 2021 11:38

Added comment about optional field.

d1e227b

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

Merge branch 'main' into rbac-upstream-dynamic-proxy

6486f62

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

htuch reviewed Sep 26, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Sep 26, 2021

mattklein123 requested changes Sep 27, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 27, 2021

conqerAtapple added 3 commits September 27, 2021 12:56

Combined IP and Port matcher.

0fb4bf8

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

Merge branch 'main' into rbac-upstream-dynamic-proxy

0812a31

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

format.

26dae37

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

repokitteh-read-only bot removed the waiting label Sep 27, 2021

conqerAtapple added 2 commits September 27, 2021 16:23

Fix clang tidy.

594f3d8

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

Merge branch 'main' into rbac-upstream-dynamic-proxy

88ae18f

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

mattklein123 requested changes Sep 28, 2021

View reviewed changes

source/extensions/filters/common/rbac/matchers/upstream_ip_port.cc Show resolved Hide resolved

source/extensions/filters/common/rbac/matchers/upstream_ip_port.cc Outdated Show resolved Hide resolved

conqerAtapple added 2 commits September 28, 2021 09:21

Review comments.

4104388

- Added configuration check. Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

Merge branch 'main' into rbac-upstream-dynamic-proxy

c027553

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

mattklein123 requested changes Sep 28, 2021

View reviewed changes

Better error message/docs.

3c166dd

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

mattklein123 requested changes Sep 28, 2021

View reviewed changes

Updated doc.

20cd653

Signed-off-by: Jojy George Varghese <jojy_varghese@apple.com>

mattklein123 approved these changes Sep 28, 2021

View reviewed changes

alyssawilk approved these changes Sep 29, 2021

View reviewed changes

alyssawilk mentioned this pull request Jan 18, 2022

DNS: officially (previously only unofficially) allowing null resolutions #19588

Merged

	throw EnvoyException("Invalid UpstreamIpPortMatcher configuration - missing `upstream_ip` and "
	throw EnvoyException("Invalid UpstreamIpPortMatcher configuration - missing `upstream_ip` and/or "

		// Note that although both fields are optional, an exception will be thrown during configuration
		// load time if none of the fields are provided.

Conversation

conqerAtapple commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 9, 2021

Uh oh!

conqerAtapple commented Aug 9, 2021

Uh oh!

yangminzhu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

conqerAtapple commented Aug 10, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 12, 2021

Uh oh!

conqerAtapple commented Aug 12, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

conqerAtapple Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

conqerAtapple commented Aug 12, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Aug 16, 2021

Uh oh!

conqerAtapple commented Aug 16, 2021

Uh oh!

conqerAtapple commented Aug 18, 2021

Uh oh!

repokitteh-read-only bot commented Aug 18, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

conqerAtapple commented Aug 9, 2021 •

edited

Loading

conqerAtapple Aug 12, 2021 •

edited

Loading

conqerAtapple Sep 27, 2021 •

edited

Loading