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 unified matcher for RBAC filters #20877

Merged
merged 63 commits into from
Jun 17, 2022
Merged

Conversation

zhxie
Copy link
Contributor

@zhxie zhxie commented Apr 19, 2022

Signed-off-by: Xie Zhihao zhihao.xie@intel.com

Commit Message: rbac: add unified matcher for RBAC filters
Additional Description:

The patch add the matching API support for both RBAC network filter and HTTP filter. Users can configure rules and shadow rules in either policies or the matching API manner. There are some incompatibilities, TODOs and behavior changes compared to the policies way.

  1. RBAC matchers are not compatible with the matching API.
  2. URL path and CEL are not supported in the matching API. These matchers may come as custom matcher.
  3. Metadata is not supported in the matching API. These matchers may come as inputs.
  4. Connections and requests with no matcher matched will always be denied.

Risk Level: Medium
Testing: Unit and integration
Docs Changes: API and configuration
Release Notes: WIP
Platform Specific Features: N/A
Fixes #20623

zhxie added 22 commits April 19, 2022 09:56
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
refer to envoyproxy#20796

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
@repokitteh-read-only
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 @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20877 was opened by zhxie.

see: more, trace.

zhxie added 3 commits April 19, 2022 15:12
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
@zhxie
Copy link
Contributor Author

zhxie commented Apr 19, 2022

cc @htuch
Also cc @snowp @kyessenov for thoughts about unsupported matchers

@kyessenov
Copy link
Contributor

You can add this to resolve the ambiguity:

diff --git a/api/bazel/external_proto_deps.bzl b/api/bazel/external_proto_deps.bzl
index 6b11495d3c..b59df6b785 100644
--- a/api/bazel/external_proto_deps.bzl
+++ b/api/bazel/external_proto_deps.bzl
@@ -19,8 +19,8 @@ EXTERNAL_PROTO_IMPORT_BAZEL_DEP_MAP = {

 # This maps from the Bazel proto_library target to the Go language binding target for external dependencies.
 EXTERNAL_PROTO_GO_BAZEL_DEP_MAP = {
-    "@com_google_googleapis//google/api/expr/v1alpha1:checked_proto": "@com_google_googleapis//google/api/expr/v1alpha1:expr_go_proto",
-    "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto": "@com_google_googleapis//google/api/expr/v1alpha1:expr_go_proto",
+    "@com_google_googleapis//google/api/expr/v1alpha1:checked_proto": "@go_googleapis//google/api/expr/v1alpha1:expr_go_proto",
+    "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto": "@go_googleapis//google/api/expr/v1alpha1:expr_go_proto",
     "@opencensus_proto//opencensus/proto/trace/v1:trace_proto": "@opencensus_proto//opencensus/proto/trace/v1:trace_proto_go",
     "@opencensus_proto//opencensus/proto/trace/v1:trace_config_proto": "@opencensus_proto//opencensus/proto/trace/v1:trace_and_config_proto_go",
     "@opentelemetry_proto//:logs": "@opentelemetry_proto//:logs_go_proto",

@zhxie
Copy link
Contributor Author

zhxie commented May 26, 2022

Ok, let us have a try to align them with xDS's external_proto_deps.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 26, 2022
@repokitteh-read-only
Copy link

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 @phlax

🐱

Caused by: #20877 was synchronize by zhxie.

see: more, trace.

@zhxie
Copy link
Contributor Author

zhxie commented May 26, 2022

Thanks @kyessenov, you saved my day. It is really easy to miss the change and TODO since I have been looking around among repositories.bzl, repository_locations.bzl, BUILD and .proto and found nothing different.

@sergiitk
Copy link
Contributor

This is the source of this problem: bazel-contrib/rules_go#1986. cc @noahdietz.

As I understand, it has always been an issue in envoy, but it was sitting there dormant because go_build_test isn't testing any targets that rely on googleapis protos:

api_go_test(
name = "go_build_test",
size = "small",
srcs = ["go_build_test.go"],
importpath = "go_build_test",
deps = [
"//envoy/api/v2:pkg_go_proto",
"//envoy/api/v2/auth:pkg_go_proto",
"//envoy/service/accesslog/v2:pkg_go_proto",
"//envoy/service/discovery/v2:pkg_go_proto",
"//envoy/service/metrics/v2:pkg_go_proto",

However, cncf/xds does test these targets. To make it work, googleapis protos had to be mapped to from @ com_google_googleapis to @go_googleapis:

EXTERNAL_PROTO_GO_BAZEL_DEP_MAP = {
    # Note @com_google_googleapis are point to @go_googleapis.
    # This is done to address //test/build:go_build_test build error:
    #
    # link: package conflict error:
    #   google.golang.org/genproto/googleapis/api/annotations: multiple copies of package passed to linker:
    #
    # @go_googleapis//google/api:annotations_go_proto
    # @com_google_googleapis//google/api:annotations_go_proto
    #
    # TODO(https://github.com/bazelbuild/rules_go/issues/1986): update to
    #    @com_google_googleapis when the bug is resolved. Also see the note to
    #    go_googleapis in https://github.com/bazelbuild/rules_go/blob/master/go/dependencies.rst#overriding-dependencies
    "@com_google_googleapis//google/api/expr/v1alpha1:checked_proto": "@go_googleapis//google/api/expr/v1alpha1:expr_go_proto",
    "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto": "@go_googleapis//google/api/expr/v1alpha1:expr_go_proto",
}

@kyessenov
Copy link
Contributor

/retest

@repokitteh-read-only
Copy link

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

🐱

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

see: more, trace.

@kyessenov
Copy link
Contributor

@envoyproxy/api-shepherds Could we please re-stamp this? There's a build fix included.

kyessenov
kyessenov previously approved these changes Jun 1, 2022
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

This needs to be approved by API or senior maintainer. Thanks for completing this work!

@kyessenov
Copy link
Contributor

CC @envoyproxy/api-shepherds @envoyproxy/senior-maintainers

@kyessenov
Copy link
Contributor

@markdroth please take a look at the API change.
/assign @phlax
(for the dependency bit)

@markdroth
Copy link
Contributor

/lgtm api

zhxie added 3 commits June 14, 2022 09:20
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>
@kyessenov
Copy link
Contributor

@moderation Could you please approve dependency bazel fix?

@moderation
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jun 17, 2022
@kyessenov kyessenov merged commit 42cb844 into envoyproxy:main Jun 17, 2022
Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022
Signed-off-by: Xie Zhihao zhihao.xie@intel.com

Commit Message: rbac: add unified matcher for RBAC filters
Additional Description:

The patch add the matching API support for both RBAC network filter and HTTP filter. Users can configure rules and shadow rules in either policies or the matching API manner. There are some incompatibilities, TODOs and behavior changes compared to the policies way.

RBAC matchers are not compatible with the matching API.
URL path and CEL are not supported in the matching API. These matchers may come as custom matcher.
Metadata is not supported in the matching API. These matchers may come as inputs.
Connections and requests with no matcher matched will always be denied.
Risk Level: Medium
Testing: Unit and integration
Docs Changes: API and configuration
Release Notes: WIP
Platform Specific Features: N/A
Fixes envoyproxy#20623

Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
@zhxie zhxie deleted the rbac-matcher branch September 7, 2022 08:28
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.

rbac: add unified matcher for RBAC filters
10 participants