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

GRPCRoute conformance #5705

Closed
1 of 4 tasks
mlavacca opened this issue Mar 14, 2024 · 9 comments · Fixed by #5918 or Kong/docs.konghq.com#7501
Closed
1 of 4 tasks

GRPCRoute conformance #5705

mlavacca opened this issue Mar 14, 2024 · 9 comments · Fixed by #5918 or Kong/docs.konghq.com#7501
Assignees
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API kind/conformance Conformance to upstream Kubernetes SIG Networking Gateway API priority/high release/highlight This part of the release is worth bragging about.
Milestone

Comments

@mlavacca
Copy link
Member

mlavacca commented Mar 14, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Problem Statement

Gateway API v1.1 will be released soon with a new set of GRPC conformance tests, and the graduation of the GRPCRoute API. Since we are implementing the GRPCRoute, we want to run the new GRPCRoute tests and claim conformance with it.

Acceptance Criteria

  • The GRPCRoute conformance tests are successfully run.
  • The default behavior changes only affect feature-gated alpha capability
  • Documentation explicitly covers both GRPC and GRPCS backend cases.
@mlavacca mlavacca added area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API kind/conformance Conformance to upstream Kubernetes SIG Networking Gateway API labels Mar 14, 2024
@mlavacca mlavacca added this to the KIC v3.2.x milestone Mar 14, 2024
@tao12345666333 tao12345666333 self-assigned this Mar 28, 2024
@tao12345666333
Copy link
Member

Let me pick up this one.

@tao12345666333
Copy link
Member

Update:

We encountered errors in some conformance test cases.

This happens because when the KIC translator processes the GRPCRoute resource, it sets the default protocol to grpcs instead of grpc.

// Create a service and attach the routes to it. Protocol for Service can be set via K8s object annotation
// "konghq.com/protocol", by default use "grpcs" to not break existing behavior when annotation is not specified.
service, err := generateKongServiceFromBackendRefWithRuleNumber(
t.logger, t.storer, result, grpcroute, ruleNumber, "grpcs", grpcBackendRefsToBackendRefs(rule.BackendRefs)...,

More background context here:

When we implemented this feature, the Kong Gateway version used was 3.5, which is based on OpenResty 1.21.4.2. Since version 3.6, it is based on 1.25.3.1 which added support for "Allow h2c and HTTP/1.1 support on the same listening socket"

We have set this default value grpcs in KIC#5283 because we don't want to break users existing configurations.

  • However, support for GRPCRoute is configured through GatewayAlpha and is not enabled by default.
  • As Gateway API's GRPCRoute is about to become generally available, I think we should consider modifying this behavior. We will implement the GRPCRoute v1 API, and it will be enabled like other GWAPI resources.

Why we need to change the default GRPCRoute's protocol

  • pass the GWAPI's conformance test
  • I have checked other GWAPI implementations. All of them's default implementation is grpc instead of grpcs.

@mflendrich
Copy link
Contributor

Decision reached with @tao12345666333 :

  • Since the GRPCRoute implementation is alpha and feature-gated, we will change the default behavior in a minor version of KIC
  • The changelog entry will explain how to migrate given the new default
  • The documentation will explicitly cover both the "GRPC non-TLS backend" and the "GRPCS" cases.

@rainest
Copy link
Contributor

rainest commented Apr 10, 2024

What's the actual conformance test that fails? Is it actually failing because the upstream traffic is sent over grpcs instead of grpc. or just because the gate is disabled?

If conformance is deploying an application that only supports plaintext, we should probably actually try to change that upstream, since it should be controlled by the Service, not any of the GWAPI resources, and shouldn't be a test requirement as such.

None of

https://gateway-api.sigs.k8s.io/guides/grpc-routing/
https://gateway-api.sigs.k8s.io/concepts/api-overview/#grpcroute
https://gateway-api.sigs.k8s.io/api-types/grpcroute/

appear to mention any default, and https://gateway-api.sigs.k8s.io/geps/gep-1016/#http2-cleartext mentions that this is more for testing, so it probably shouldn't be a default.

AFAICT the actual tests only care that we reach the correct backend, and don't see which protocol we use to do so. The Deployments created by https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/base/manifests.yaml do indeed not support TLS based on basic testing, but we shouldn't take that as a de facto part of the standard, it's just a shortcoming of the test environment.

Those Services arguably should indicate their protocol explicitly, though there's apparently no standard or de facto standard value for gRPC in appProtocol.

https://gateway-api.sigs.k8s.io/api-types/backendtlspolicy/ is another option, but I don't think we have any support for it yet (and the test manifests don't currently include it).

@shaneutt do you know anything about this? The tests apparently impose a hidden requirement that's arguably at odds with typical production gRPC expectations, where using encrypted HTTP/2 as the underlying transport is the norm.

@shaneutt
Copy link
Contributor

shaneutt commented Apr 10, 2024

I'm unavailable and wont be able to dig in for the next few days. I did however share this thread with other Gateway API folks and we're agreed to hold on releasing v1.1 until we've got things sorted out. @tao12345666333, @mlavacca: if you're working on this the next couple days and have any updates, let Rob and Richard know any updates (on Kubernetes slack) as they're aware of the situation.

@tao12345666333
Copy link
Member

The reason why the conformance test fails is because the test case only uses plaintext, but since KIC sets the protocol to grpcs, it will fail.

I have tested that it works correctly if set to grpc.

Thanks @rainest and @shaneutt. I will reach out to the GWAPI team.

@tao12345666333
Copy link
Member

tao12345666333 commented Apr 15, 2024

Interim conclusion:

  • GWAPI will bring test cases for grpcs, and the current test case is clearly defined as h2c protocol through app-protocol. Therefore, KIC also needs to support the h2c protocol.

I will complete this goal through the following steps to avoid generating a huge PR and facilitate review.

@randmonkey randmonkey added priority/high release/highlight This part of the release is worth bragging about. labels Apr 24, 2024
@tao12345666333 tao12345666333 linked a pull request Apr 25, 2024 that will close this issue
1 task
@tao12345666333
Copy link
Member

since #5776 hasn't been merged, I will reopen this issue.

@tao12345666333
Copy link
Member

the docs PR Kong/docs.konghq.com#7501 has been merged. I think we can close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API kind/conformance Conformance to upstream Kubernetes SIG Networking Gateway API priority/high release/highlight This part of the release is worth bragging about.
Projects
None yet
6 participants