Propose GRPCRoute Resource

[gnossen]

What type of PR is this?

/kind gep

What this PR does / why we need it:

Propose a GRPCRoute resource.

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Welcome @gnossen!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @gnossen. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[brianehlert]

Is the intention that this would also incorporate grpc-web in some way? Or the implementation would be strict to grpc.
I am asking since grpc-web is a project technically distinct from grpc with its own nuances.

[gnossen]

@brianehlert This particular GEP intentionally leaves gRPC Web support out of scope, but I think it would be an exciting future to add in a future proposal.

[hbagdi]

This has come before so I want to clarify.
Since gRPC filters are part of a bigger feature, gRPCRoute, they shouldn't have a conformance level that is higher than the parent.

Should this be extended?

[gnossen]

I think I need some guidance here.

Since gRPC filters are part of a bigger feature, gRPCRoute, they shouldn't have a conformance level that is higher than the parent.

I absolutely agree with this. In the places where I've written "Core" in this proposal, I've meant "Core to GRPCRoute." Likewise, "Custom" here means "Custom to GRPCRoute."

If I write the support levels in absolute terms, then I think that would flatten things out and everything would become "Support: extended." This loses much of the detail that I've written in about support levels between different GRPCRoute implementations.

Thoughts on how to resolve this?

[hbagdi]

I absolutely agree with this. In the places where I've written "Core" in this proposal, I've meant "Core to GRPCRoute." Likewise, "Custom" here means "Custom to GRPCRoute."

That is how I read this as well but we run into issues such as #655.

If I write the support levels in absolute terms, then I think that would flatten things out and everything would become "Support: extended." This loses much of the detail that I've written in about support levels between different GRPCRoute implementations.

Extended features must be portable if they are supported. Is that not enough for defining semantics here?

cc @robscott @youngnick

[youngnick]

I don't think we've explicitly discussed this before, but I've always conceived of the field-level support working like @gnossen described - this is because we clearly have to allow implementations not to support some objects, because Layer 7 ingress controllers may not be able to do Layer 4 resources (like TCPRoute and UDPRoute), and Layer 4 load balancers may not be able to do higher-layer resources (like HTTPRoute or TLSRoute).

So, the "Support: Core" markings on fields have to be on a per-object basis.

But like I said, this is a subtle point that I don't think we've explicitly made before.

[robscott]

I agree with the interpretation here. We do cover this in the docs, but it may not be prominent enough (https://gateway-api.sigs.k8s.io/concepts/guidelines/#overlapping-support-levels).

[hbagdi]

It may make sense to call this "HeaderName" instead to avoid unnecessary confusion. The "GRPC" prefix doesn't bring any additional value but can cause confusion here.

[hbagdi]

I find this pretty confusing and we should do a bit of optimization for the easy path.

Based on the defaults, the values are:

type: Exact
method: ""
service: ""

No matter what we document, this - 'exact match on empty strings' makes little sense to me and I feel it will make little sense to anyone.
If the type is Exact, then the values should contain something.
My experience is that using empty values to mean everything doesn't work well, especially in k8s.

Some options we can explore:

1. Can the code require at least one of method or service?
2. Can we ask the user to provide an explicit * as the value of the method or service to mean that they would like to match any method or service respectively? IMHO, that makes the configuration much more self-documenting.
3. If type=Exact, the code should be exacting. Splitting type into two types makes the configuration verbose but it also makes it a bit more machine-readable. Let's say the type=regex and then one of them is a regex while the other one is not, how is the code supposed to figure it out?
4. Exact with the method specified is core and no method specified is custom seems arbitrary and confusing. Can we classify both at the same level?

I should note that the above is done with the right technical reasons, I don't doubt. It doesn't feel it is intuitive enough.

Thoughts?

[gnossen]

Based on the defaults, the values are:

This is true. Those are the default values. But people generally don't see the default values, they see the default behavior. And the default behavior is to match all traffic.

My experience is that using empty values to mean everything doesn't work well, especially in k8s.

Can you provide some concrete examples?

Can the code require at least one of method or service?

Considering that supplying no match at all already matches all traffic, I'm totally open to this option. Going ahead and making this change. Anyone objecting, please jump in.

Can we ask the user to provide an explicit * as the value of the method or service to mean that they would like to match any method or service respectively? IMHO, that makes the configuration much more self-documenting.

I'm not the biggest fan of this option. As far as I know, no equivalent syntax exists for HTTPRoute. This could lead to confusion.

If type=Exact, the code should be exacting. Splitting type into two types makes the configuration verbose but it also makes it a bit more machine-readable.

I don't quite see how this solves the problem you introduced at the beginning of this comment? And as you point out, this would not be as nice a UX.

Let's say the type=regex and then one of them is a regex while the other one is not, how is the code supposed to figure it out?

Discussed already here.

Specifically, this:

What's nice here is that exact matches are a subset of regular expressions, so the fact that there's only one type for two values here doesn't actually get in the way of expressing the routing logic you want.

In practice, this shouldn't be a limitation.

Exact with the method specified is core and no method specified is custom seems arbitrary and confusing. Can we classify both at the same level?

This actually was mistaken. The combination that presents trouble for implementations is type: EXACT with service unspecified and method specified. I've corrected this in the proposal.

I expect that many implementations will add support by translating to HTTPRoutes. In this chart from earlier, you see that this combination is equivalent to either a suffix matcher or a regex matcher of the form /.+/${METHOD}. Suffix matchers don't exist for HTTPRoute and Regex matchers have "Custom" support. This case is called out to have the same support level as an HTTPRoute regex matcher to allow implementations that support HTTPRoute but not regex to also support GRPCRoute at some level.

If you have an alternative to address this problem, I'm open to suggestions.

[hbagdi]

This is true. Those are the default values. But people generally don't see the default values, they see the default behavior. And the default behavior is to match all traffic.

In that case, the default values should communicate the default behavior well.

Can you provide some concrete examples?

Empty label selector behavior comes to mind.

Considering that supplying no match at all already matches all traffic, I'm totally open to this option. Going ahead and making this change. Anyone objecting, please jump in.

Great. Could you create a tracking issue for this? This will require updates to validating webhook; the last time I looked, controller-runtime didn't allow for "at least one" JSON-schema validations.

Can we ask the user to provide an explicit * as the value of the method or service to mean that they would like to match any method or service respectively? IMHO, that makes the configuration much more self-documenting.

I'm not the biggest fan of this option. As far as I know, no equivalent syntax exists for HTTPRoute.

In HTTPRoute, the default value for path is / with the type PathPrefix.

[gnossen]

Great. Could you create a tracking issue for this? This will require updates to validating webhook; the last time I looked, controller-runtime didn't allow for "at least one" JSON-schema validations.

Created.

[youngnick]

/lgtm

but will leave this for @hbagdi to come back to for approval and his lgtm

[robscott]

/ok-to-test

[robscott]

Thanks for the work on this @gnossen! This is a well written and massive GEP. I'm deferring to @hbagdi for final LGTM.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnossen, robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hbagdi]

/lgtm

[robscott]

Looks like we're good to merge this in, thanks for all the work on this @gnossen!

/hold cancel