rbac: add uri_template for path matching #31447

kozjan · 2023-12-19T09:17:15Z

Commit Message: rbac: add uri_template for path matching
Additional Description: Added uri_template with envoy.path.match extension category to allow matching with URI templates in RBAC.
Risk Level: low
Testing: unit, integration
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Fixes #30724

repokitteh-read-only · 2023-12-19T09:17:21Z

Hi @kozjan, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #31447 was opened by kozjan.

see: more, trace.

repokitteh-read-only · 2023-12-19T09:17:26Z

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

🐱

Caused by: #31447 was opened by kozjan.

see: more, trace.

htuch · 2023-12-20T06:15:27Z

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

@@ -358,6 +362,9 @@ message Principal {
    // Identifies the principal using a filter state object.
    type.matcher.v3.FilterStateMatcher filter_state = 12;

+    // Glob URL path matching.
+    envoy.extensions.path.match.uri_template.v3.UriTemplateMatchConfig glob_path = 13;


@envoyproxy/api-shepherds I'm wondering if type.matcher.v3.PathMatcher should include

envoy/api/envoy/config/route/v3/route_components.proto

Line 601 in b039d8a

// [#extension-category: envoy.path.match]

?

I.e. allow URI template matcher to be plugged in. In any case, we should not be referencing the extension config directly here and instead should have something like

envoy/api/envoy/config/route/v3/route_components.proto

Line 601 in b039d8a

// [#extension-category: envoy.path.match]

if we don't end up putting it inside PathMatcher.

hey, I'm going on holiday so I will rework it once I'm back, probably next year

adisuissa · 2024-01-02T15:27:09Z

/wait

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

sschepens · 2024-01-10T18:26:59Z

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

+
+    // Glob URL path matching.
+    // [#extension-category: envoy.rbac.matchers]
+    core.v3.TypedExtensionConfig glob_path = 13;


I don't think i like naming this glob_path given that it possible covers more features than just globbing, that's why it's actually called uri_template 🤔

I will replace glob everywhere with uri_template.

kozjan · 2024-01-11T16:55:57Z

I tried to push with prepush hooks, but shellcheck doesn't work on Mac i guess:

ERROR: /private/var/tmp/_bazel_jan.kozlowski/a20ce50f1167d001f45ad8fee633e1f8/external/com_github_aignas_rules_shellcheck/BUILD.bazel:25:6: configurable attribute "actual" in @com_github_aignas_rules_shellcheck//:shellcheck doesn't match this configuration: binaries for your platform could not be found

so i fixed everything that was before the shellcheck, but i'm not sure if that's all

kozjan · 2024-01-11T18:10:35Z

don't really see why some checks failed, envoy/prechecks build result look fine yet it failed, and in envoy/windows do not see any specific errors, so running a retest
/retest

repokitteh-read-only · 2024-01-22T13:57:32Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @htuch

🐱

Caused by: #31447 was synchronize by kozjan.

see: more, trace.

htuch · 2024-01-23T05:22:37Z

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

@@ -270,6 +270,10 @@ message Permission {
    // Extension for configuring custom matchers for RBAC.
    // [#extension-category: envoy.rbac.matchers]
    core.v3.TypedExtensionConfig matcher = 12;
+
+    // URI template path matching.
+    // [#extension-category: envoy.rbac.uri_template]


Suggested change

// [#extension-category: envoy.rbac.uri_template]

// [#extension-category: envoy.path.match]

htuch · 2024-01-23T05:23:30Z

bazel/repository_locations.bzl

@@ -1247,6 +1248,7 @@ REPOSITORY_LOCATIONS_SPEC = dict(
            "envoy.filters.network.wasm",
            "envoy.stat_sinks.wasm",
            "envoy.rbac.matchers.upstream_ip_port",
+            "envoy.rbac.uri_template.uri_template_matcher",


I don't get why this is needed?

I added it because of an error message in Envoy/Prechecks :

Dependency validation failed, please check metadata in bazel/repository_locations.bzl Extension envoy.rbac.matchers.uri_template depends on com_github_google_flatbuffers but com_github_google_flatbuffers does not list envoy.rbac.matchers.uri_template in its allowlist

However, it seems to me as if the checks are bugged and checkout older code, because when I run it locally I don't get this error know - I think envoy.rbac.matchers.uri_template value comes from source/extensions/extensions_build_config.bzl

htuch · 2024-01-23T05:24:13Z

source/extensions/extensions_build_config.bzl

+    # RBAC URI template matcher
+    #
+
+    "envoy.rbac.uri_template.uri_template_matcher":         "//source/extensions/filters/common/rbac/uri_template:uri_template_lib",


There's no need to declare a new matcher here, we can use the existing URI template path matcher extension?

If I understand correctly, this is necessary if I want to use a different factory for rbac, the one that was originally implemented with URI template path matcher returns absl::StatusOr<Router::PathMatcherSharedPtr>, while the type needed in RBAC is MatcherConstSharedPtr

would it be better to use the same factory, but then wrap the PathMatcherSharedPtr into MatcherConstSharedPtr?

htuch · 2024-01-23T22:07:49Z

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

@@ -194,7 +194,7 @@ message Policy {
 }

 // Permission defines an action (or actions) that a principal can take.
-// [#next-free-field: 13]


LGTM, can you add an entry to https://www.envoyproxy.io/docs/envoy/latest/version_history/v1.30/v1.30.0? Thanks.

done, also made some small changes to tests and unnecessary null check, since it works the same either way

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com> Signed-off-by: kozjan <jan.kozlowski@allegro.com> Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

This reverts commit 1db41e8. Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

kozjan · 2024-01-25T08:24:36Z

/retest

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

kozjan · 2024-01-25T09:03:41Z

don't know what to do with Envoy/Windows: https://github.com/envoyproxy/envoy/actions/runs/7651824181/job/20850377753#step:8:21416
looks like some Azure problem to me, hoped rerunning would fix it but nothing changed

phlax · 2024-01-25T09:11:43Z

looks like some Azure problem to me, hoped rerunning would fix it but nothing changed

its a backend (RBE) issue - its known but infrequent generally, and not obvious why its happening

ive kicked the CI again

phlax · 2024-01-25T09:53:11Z

@kozjan i think its failing because your branch is far behind

can you merge main please

phlax · 2024-01-25T09:59:24Z

hmm - the more i think about it the more im wondering why that would matter - and it looks like you have merged main relatively recently - and also it was passing previously

a bit of a mystery tbh - this is now failing every time, no other ci runs are mostly passing (EDITED: i saw another similar today - will follow up)

i think we may have some misbehaving backend auth server cc @adisuissa

htuch

LGTM for when CI passes.

kozjan · 2024-01-26T08:05:16Z

/retest
it never works :(

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

phlax · 2024-01-26T10:08:44Z

#32044

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

kozjan · 2024-01-30T11:24:54Z

hey, it passed today - do I have to contact someone for the second review?

Signed-off-by: kozjan <138656232+kozjan@users.noreply.github.com>

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

kozjan requested a review from alyssawilk as a code owner December 19, 2023 09:17

repokitteh-read-only bot added the api label Dec 19, 2023

repokitteh-read-only bot assigned htuch Dec 19, 2023

htuch reviewed Dec 20, 2023

View reviewed changes

kozjan requested a review from yanavlasov as a code owner December 21, 2023 07:45

repokitteh-read-only bot added waiting and removed waiting labels Jan 2, 2024

sschepens reviewed Jan 10, 2024

View reviewed changes

api/envoy/config/rbac/v3/rbac.proto Show resolved Hide resolved

sschepens reviewed Jan 10, 2024

View reviewed changes

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jan 22, 2024

htuch reviewed Jan 23, 2024

View reviewed changes

kozjan added 12 commits January 24, 2024 21:33

rbac: add path glob matching option

f342e17

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com> Signed-off-by: kozjan <jan.kozlowski@allegro.com> Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

use uri template matcher in rbac

29e33cb

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com> Signed-off-by: kozjan <jan.kozlowski@allegro.com> Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

add integration tests

3de77f6

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

rework glob matching to use TypedConfig

c4723f2

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

add validation visitor to principal for glob matching

1f9b667

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

formatting fixes

ce32c12

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

fix precheck format

58317bb

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

rename test

6d40744

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

add uri_template matcher instead another typed extension config

ae2da5f

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

Revert "add validation visitor to principal for glob matching"

72b9049

This reverts commit 1db41e8. Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

add rbac/matchers/uri_template to api/versioning

14aef0f

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

run proto_format fix

d72e909

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

kozjan force-pushed the rbac-glob-matching branch from 5fbfeca to cf19a2f Compare January 24, 2024 20:50

kozjan changed the title ~~rbac: add url glob matching~~ rbac: add uri_template for path matching Jan 24, 2024

fix code formatting

cdebad5

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

kozjan force-pushed the rbac-glob-matching branch from 881fadb to 5d71299 Compare January 24, 2024 21:59

kozjan added 2 commits January 24, 2024 23:07

Merge remote-tracking branch 'origin/main' into rbac-glob-matching

085c128

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

add feature to changelog

e673421

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

kozjan force-pushed the rbac-glob-matching branch from 5d71299 to e673421 Compare January 24, 2024 22:10

remove trailing spaces

0cfcb6f

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

Kick CI

f5e3279

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

htuch previously approved these changes Jan 25, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Jan 25, 2024

Kick CI

78f3595

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

kozjan added 2 commits January 30, 2024 08:27

Kick CI again

2194f12

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

Kick CI again

ef7dd77

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

Merge branch 'main' into rbac-glob-matching

d93d667

Signed-off-by: kozjan <138656232+kozjan@users.noreply.github.com>

kozjan dismissed htuch’s stale review via d93d667 January 31, 2024 06:41

repokitteh-read-only bot added the api label Jan 31, 2024

Kick CI after conflicts

01cfadb

Signed-off-by: jan.kozlowski <jan.kozlowski@allegro.com>

htuch approved these changes Feb 1, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Feb 1, 2024

htuch merged commit 20c7368 into envoyproxy:main Feb 1, 2024
54 checks passed

thiagogcm mentioned this pull request Feb 1, 2024

Support regex for ServiceRole spec.rules.paths istio/istio#16585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbac: add uri_template for path matching #31447

rbac: add uri_template for path matching #31447

kozjan commented Dec 19, 2023 •

edited

repokitteh-read-only bot commented Dec 19, 2023

repokitteh-read-only bot commented Dec 19, 2023

htuch Dec 20, 2023 •

edited

htuch Dec 20, 2023

kozjan Dec 22, 2023

adisuissa commented Jan 2, 2024

sschepens Jan 10, 2024

kozjan Jan 11, 2024

kozjan Jan 11, 2024

kozjan commented Jan 11, 2024

kozjan commented Jan 11, 2024

repokitteh-read-only bot commented Jan 22, 2024

htuch Jan 23, 2024

htuch Jan 23, 2024

kozjan Jan 23, 2024

htuch Jan 23, 2024

kozjan Jan 23, 2024

htuch Jan 23, 2024

kozjan Jan 24, 2024

kozjan commented Jan 25, 2024

kozjan commented Jan 25, 2024

phlax commented Jan 25, 2024

phlax commented Jan 25, 2024

phlax commented Jan 25, 2024 •

edited

htuch left a comment

kozjan commented Jan 26, 2024 •

edited

phlax commented Jan 26, 2024

kozjan commented Jan 30, 2024

	// [#extension-category: envoy.rbac.uri_template]
	// [#extension-category: envoy.path.match]

rbac: add uri_template for path matching #31447

rbac: add uri_template for path matching #31447

Conversation

kozjan commented Dec 19, 2023 • edited

repokitteh-read-only bot commented Dec 19, 2023

repokitteh-read-only bot commented Dec 19, 2023

htuch Dec 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozjan commented Jan 11, 2024

kozjan commented Jan 11, 2024

repokitteh-read-only bot commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozjan commented Jan 25, 2024

kozjan commented Jan 25, 2024

phlax commented Jan 25, 2024

phlax commented Jan 25, 2024

phlax commented Jan 25, 2024 • edited

htuch left a comment

Choose a reason for hiding this comment

kozjan commented Jan 26, 2024 • edited

phlax commented Jan 26, 2024

kozjan commented Jan 30, 2024

kozjan commented Dec 19, 2023 •

edited

htuch Dec 20, 2023 •

edited

phlax commented Jan 25, 2024 •

edited

kozjan commented Jan 26, 2024 •

edited