I know this is not the scope of this PR, but do we care about backend timeouts at some point? Should we state what timeouts are respected, are these the same established on .spec.timeout or something else?

We'll need to handle them in some other way - this is the first time we've had a reference to a backend that's an infrastructure one as opposed to a workload one, so we need to think carefully about how timeout config would work there.

Good thought though!

it is a bit confusing, at least for me this mix between ext_authz calls vs something else.

While I understand from the GEP that the idea of using gRPC here is to support Envoy ext_authz, this is not mentioned clearly on the API and on GEP it is just mentioned here: https://gateway-api.sigs.k8s.io/geps/gep-1494/#why-envoys-ext_authz

My first though here is that this is very envoy focused, and as there's an agreement with the community per the GEP description to follow the envoy approach, I think API wide the call for ext_authz should be avoided and instead added to the GRPC fields what API/protobuf the gRPC service should support.

Otherwise, as a user, I may end up a bit loss on what I should be implementing here, what the gRPC backend means, etc.

Probably also, as a good approach for backend implementors, I think we need to have a demo somewhere on what a gRPC based authentication/authorization service looks like.

I guess this is fair, I'll make sure that there are references to the fact that this is the Envoy ext_authz gRPC protocol, with a link to the protobufs. I think it's fair to mention that it's the Envoy thing though, since we are using the protobufs directly in this case.

To be honest, after talking to some users about this, I think the HTTP option may be more used, but that's the point of Experimental, to let us see what happens.

Added some explanation on this config structure based on the similar explanation available on the ExternalAuthProtocol field.

Thanks Nick! One additional though that I had is that given it uses Envoy's ext_authz gRPC protocol, can we state somewhere that while we plan on moving HTTPAuthConfig to standard at some point, GRPCAuthConfig will always be Implementation Specific?

I think this way we can have some conformance enhancement for any kind of backend on HTTPAuthConfig, but avoid forcing non-envoy proxies to be able to do gRPC auth. As an example, IIRC on upstream NGINX is not trivial to make it do external authentication, who can say about external gRPC authentication :)

When we talked with implementations about this, even non-Envoy implementations had some interest in implementing the Envoy gRPC protocol. That's why both options have their own Features, an implementation can choose to support one or the other, or both. In either case, to move that feature to Standard, it will need conformance tests.

So implementations don't have to implement this, in either HTTP or gRPC forms, but if they do, there will be tests to make sure they do it right.

At initial glance, I think nginx should be fine at supporting auth using the auth_request module. Though I haven't verified this, I'm assuming gRPC would work with this as well.

@rikatz, the thing is, we are not enforcing that an implementation has to support the gRPC protocol. If the implementations chooses to support gRPC, then they claim support for the HTTPRouteExtAuthGRPC feature. If they do not, then they don't claim that feature, and are not required to support gRPC to be conformant. That's what Extended means.

Implementations do not have to support Extended features to be conformant.

Both Extended and Implementation Specific are optional for conformant implementations. The difference is that, before graduating to Standard, Extended features must have conformance tests for implementations that claim support for that feature.

Implementations that do not choose to support HTTPRouteExtAuthGRPC can support HTTPRotueExtAuthHTTP instead, and the conformance suite will run the corresponding tests. Of course, implementations are also free to support both, in which case both sets of tests will be run.

ok I see now, thanks!

Circling back on this from the NGINX side, our auth_request module supports HTTP but not gRPC natively, so we would have to build some other solution in order to initiate gRPC auth requests.

the whole External Auth feature is considered extended, so I think it should be fine to say that you support the extended feature of using an HTTP filter, but not a GRPC.

If this is not clear on the docs, tho, I think we should make it clear that externalauth can be partially implemented (eg.: supporting just HTTP) and this be explicit on the conformance docs

Can we get some documentation/comment somewhere on what the regex here expects, and what it avoids (eg.: did it came from the path RFC?)

I am speaking here with my past trauma of all the ingress-nginx regexes and CVEs.

Also, maybe it is worth adding some test for this validation, even not being a "CEL" validation it would be good to double check that everything works here

cc @robscott you may remember on past some discussion we had about RFC vs path validation on ingress resource (and the lack of it), I will try to recover that discussion.

I understand that external auth paths may get not only the URI path but also get params on the request (eg.: oauth stuff) but I would love to make this a bit more strict, maybe even separate the Path from the params here

eg.: split something like /oauth2?clientid=xpto&something=bla into Path (as a validated strict path) + params as a map of validated parameters.

This is the same regex we use for Path in other places in the API, although I couldn't make it a CEL rule, because the size of the field combined with not having dependencies on other fields to cut the complexity meant it was over budget by 2.4x. So I swapped to a kubebuilder validation, but kept the regex the same.

There is an invalid config test in this PR, in hack/invalid-examples/httproute/invalid-filter-externalauth-bad-http-path.yaml. Happy for more suggestions on other invalid paths to put in there for tests, would definitely be good to learn from ingress-nginx on that front.

This field is talking about adding a prefix to paths sent to the external auth server, not matching params or anything, so we can probably trim down the regex if we want.

tbh, it may be worth logging a followup issue to check the other regex as well, since we can make changes to this Experimental path field, but making changes to the Stable one in HTTPRoute matches may be tricky. We could mitigate risks in either field by also adding conformance tests to validate that implementations reject problematic paths.

But in terms of this PR, we have until Monday to get it in before code freeze, so I would prefer to go ahead with what we have, unless you have specific tests you want to add? This field is experimental so we can change the validation later if need be.

I will add some words about how this input MUST be sanitized before use though.

Added reminder to sanitise inputs for Path.

do we care about response codes on HTTPAuthConfig?

I know "in theory" we should respect the response codes, but practically I've seen cases where a 200/OK was to be returned on success, but a 201 was returned instead, or that some authorization webservice would cause a temporary/permanent redirect inside its workflow.

Which btw leads me to the question if we should care about allowing redirects on internal authentication request (eg.: should envoy follow redirects when sending authentication requests? it is supported'ish at least with oauth2)

Again, this is based on what Envoy does in its ext_authz filter, so I've copied this for now. I think we should proceed with this and see what it looks like when folks start using it.

At initial glance, I think nginx should be fine at supporting auth using the auth_request module. Though I haven't verified this, I'm assuming gRPC would work with this as well.