support ext_authz v3 API, still serve v2 #217

srenatus · 2020-11-19T13:39:31Z

This will make opa-envoy-plugin expose both the v2 and the v3 APIs:

$ grpcurl -plaintext 127.0.0.1:9191 list
envoy.service.auth.v2.Authorization
envoy.service.auth.v3.Authorization
grpc.reflection.v1alpha.ServerReflection

As such, we should be able to use it with in either variant. New features will only be added to the v3 API, and v2 is to be considered deprecated, and will be removed in the future, timeline TBD.

There's still some work to do here:

figure out the impact of switching to protojson for converting the CheckRequest into OPA's input, and determine if we want to use v3 as a chance to do that.

This package produces a different output than the standard "encoding/json" package, which does not operate correctly on protocol buffer messages.
documentation:
- what do I need to do to get to v3, (TODO: add istio)
- what changes in v3,
- did the check request change?
testing: the unit tests use v3, the e2e tests use v2 still

Fixes open-policy-agent/opa#2798.

tsandall

Compatibility is definitely nice to have, so +1 there. I have two questions that I think we need to answer (which we've talked about offline):

What config changes are required on the Envoy (and also Istio) side? E.g., does the filter configuration change significantly? An example would be useful.
What policy changes are required when switching from v2 to v3? Does using protojson change anything? Examples would be useful.

internal/internal.go

srenatus · 2020-11-20T13:07:05Z

I've had a close look at the proto defs for v2 and v3, and played around some more, capturing my findings in https://github.com/open-policy-agent/opa-envoy-plugin/pull/217/files#diff-9119e7abecbea4bb21414d5dbc6de682261b4fc2840aec10485494feb01125dd

Summing up, I think it's preferable to go with protojson in v3; an alternative we could explore is adding a configurable to let people opt-in to the old behaviour, using encoding/json instead.

srenatus · 2020-11-20T13:42:15Z

Remaining TODOs:

figure out istio with v3, document it
add e2e tests for v3
determine if duplicating the existing unit tests (checking v3) for v2 is worth it

(💭 It's interesting how our current set of unit tests seems to live exactly in the overlap of v2/v3 and encoding/json / protojson. While I'd like to expand them some day, I think it's better to focus on one set of those, and I'd think that's v3+protojson.)

README.md

srenatus · 2020-11-23T12:26:35Z

✔️ ensured that make test-e2e still works with istio 1.5.0. It turns out upgrading golang (#201) pulled in a change in net/http making the hardcoded TLS certs insufficient. CN in Subject no longer does the trick, we need a proper SNI record.

README.md

srenatus · 2020-11-24T09:10:35Z

ℹ️ Rebasing now, I'll squash post-review.

srenatus · 2020-11-24T12:28:14Z

Please note that I haven't updated the top-level quick_start.yaml file. It's using v2, and it should still be OK to do so.

tsandall

Looks great. A couple questions but nothing blocking.

README.md

tsandall · 2020-11-24T17:14:11Z

internal/internal.go

+	respV2 := v2Response(respV3)
+	if code := stop(); code != nil {
+		respV2.Status = code
+	}
+	return respV2, nil


I assume the conversion is relatively quick and accounts for a small portion (e.g., 1%) of the check response time...is that right? Since we're not including it in the decision log metrics, I just want to make sure it's negligible and can be ignored.

It was my intention to include it in the decision log time, that's where the awkward stop handling came from. It's converted in L769 right before stopping the time in L770. I might be misled about how it works...? 😄

Whoops. I didn't look closely enough at what stop() was doing. The metric tracking looks fine then.

However, we'll want to call stop() regardless of whether an error occurred--we want a decision log emitted in the case of errors.

Yep, that's an oversight. We'll want stop to happen for the L756 case, too, indeed.

I've fixed it, and duplicated a few of the unit tests for v2, to cover this logic.

build/gen-tls-certs.sh

srenatus · 2020-11-24T19:22:13Z

⏸️ I'll address all comments and proceed with merging this tomorrow. 😃

srenatus · 2020-11-25T08:32:23Z

I'll squash and merge. Let's take it from here in follow-up PRs.

This change makes opa-envoy-plugin expose two gRPC services: envoy.service.auth.v2.Authorization and envoy.service.auth.v3.Authorization. Since switching to v3 requires user action already, we've taken this chance to migrate the representation of the v3 CheckRequest to the specified JSON mapping. See the README.md for details. That aside, if you're using the v2 API, no changes to your policies are required, and you can keep using the v2 API for some time. It's TBD when we are going to drop support for it. This change also brings another input value that lets you determine the request version (v2/v3) and its encoding (protojson for the official mapping, "encoding/json" for the one used with v2). With respect to the software we're integrating with here, note - Envoy 1.13 is the first version to support the v3 API. - Istio 1.7.0 is the first version that lets you configure the v3 filters required to use the v3 API with Envoy. Support for the 1.6 series of Istio has been ended in Nov 21 2020. - That first version of Envoy released in 2021, presumably 1.18, will drop support for the v2 API. The `examples/istio/quick_start.yaml` file has been adapted: 1. it's using the v3 API 2. its hardcoded TLS certs (generated with `build/gen-tls-certs.sh`) work with a service that's built with Go 1.15 (which refuses CN in favour of SNI records) Signed-off-by: Stephan Renatus <stephan@styra.com>

Signed-off-by: Stephan Renatus <stephan@styra.com>

Also updates kind to 0.9.0, required for Istio 1.7.0. (Oversight of open-policy-agent#217) Signed-off-by: Stephan Renatus <stephan@styra.com>

Also updates kind to 0.9.0, required for Istio 1.7.0. (Oversight of #217) Signed-off-by: Stephan Renatus <stephan@styra.com>

srenatus self-assigned this Nov 19, 2020

srenatus force-pushed the sr/ext_auth-v3 branch from 4f37f6d to 7a93157 Compare November 19, 2020 13:45

srenatus changed the title ~~support ext_authz v3 API~~ support ext_authz v3 API, still serve v2 Nov 19, 2020

tsandall reviewed Nov 19, 2020

View reviewed changes

internal/internal.go Outdated Show resolved Hide resolved

internal/internal.go Outdated Show resolved Hide resolved

srenatus force-pushed the sr/ext_auth-v3 branch 2 times, most recently from dbb18ea to b30fab5 Compare November 20, 2020 13:35

srenatus force-pushed the sr/ext_auth-v3 branch from 879ab03 to b288771 Compare November 20, 2020 14:43

srenatus commented Nov 20, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

srenatus mentioned this pull request Nov 23, 2020

support basic gRPC payload decoding #202

Merged

tsandall reviewed Nov 23, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

srenatus force-pushed the sr/ext_auth-v3 branch 5 times, most recently from 54bb17e to e41df82 Compare November 24, 2020 09:07

srenatus marked this pull request as ready for review November 24, 2020 09:10

srenatus force-pushed the sr/ext_auth-v3 branch 2 times, most recently from 6c00fe7 to 67477dc Compare November 24, 2020 09:38

tsandall approved these changes Nov 24, 2020

View reviewed changes

srenatus mentioned this pull request Nov 25, 2020

Unable to get basic sample app to work with OPA envoy plugin for Istio open-policy-agent/opa#2831

Closed

srenatus force-pushed the sr/ext_auth-v3 branch 2 times, most recently from 77be495 to 2875814 Compare November 25, 2020 08:47

vendor: Support Envoy's v3 xDS API, envoy.service.auth.v3.Authorization

fa2a2d2

Signed-off-by: Stephan Renatus <stephan@styra.com>

srenatus force-pushed the sr/ext_auth-v3 branch from 2875814 to fa2a2d2 Compare November 25, 2020 08:49

srenatus merged commit fe9de5b into open-policy-agent:master Nov 25, 2020

srenatus deleted the sr/ext_auth-v3 branch November 25, 2020 08:51

srenatus mentioned this pull request Nov 25, 2020

actions: fix e2e test, add it to pull-request workflow #222

Merged

srenatus mentioned this pull request Nov 30, 2020

workflow: use docker-env for building image (to ensure golang version) #223

Closed

ashutosh-narkar pushed a commit that referenced this pull request Nov 30, 2020

actions: fix e2e test, add it to pull-request workflow

6ff608e

Also updates kind to 0.9.0, required for Istio 1.7.0. (Oversight of #217) Signed-off-by: Stephan Renatus <stephan@styra.com>

tsandall mentioned this pull request Jan 26, 2021

Update Envoy external authorization tutorial to use V3 API open-policy-agent/opa#3101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support ext_authz v3 API, still serve v2 #217

support ext_authz v3 API, still serve v2 #217

srenatus commented Nov 19, 2020 •

edited

tsandall left a comment

srenatus commented Nov 20, 2020

srenatus commented Nov 20, 2020 •

edited

srenatus commented Nov 23, 2020 •

edited

srenatus commented Nov 24, 2020

srenatus commented Nov 24, 2020

tsandall left a comment

tsandall Nov 24, 2020

srenatus Nov 24, 2020 •

edited

tsandall Nov 24, 2020

tsandall Nov 24, 2020

srenatus Nov 25, 2020

srenatus Nov 25, 2020

srenatus commented Nov 24, 2020

srenatus commented Nov 25, 2020

support ext_authz v3 API, still serve v2 #217

support ext_authz v3 API, still serve v2 #217

Conversation

srenatus commented Nov 19, 2020 • edited

tsandall left a comment

Choose a reason for hiding this comment

srenatus commented Nov 20, 2020

srenatus commented Nov 20, 2020 • edited

srenatus commented Nov 23, 2020 • edited

srenatus commented Nov 24, 2020

srenatus commented Nov 24, 2020

tsandall left a comment

Choose a reason for hiding this comment

tsandall Nov 24, 2020

Choose a reason for hiding this comment

srenatus Nov 24, 2020 • edited

Choose a reason for hiding this comment

tsandall Nov 24, 2020

Choose a reason for hiding this comment

tsandall Nov 24, 2020

Choose a reason for hiding this comment

srenatus Nov 25, 2020

Choose a reason for hiding this comment

srenatus Nov 25, 2020

Choose a reason for hiding this comment

srenatus commented Nov 24, 2020

srenatus commented Nov 25, 2020

srenatus commented Nov 19, 2020 •

edited

srenatus commented Nov 20, 2020 •

edited

srenatus commented Nov 23, 2020 •

edited

srenatus Nov 24, 2020 •

edited