Implement GRPCRoute (GEP 1016) #1115

gnossen · 2022-04-14T22:19:37Z

What type of PR is this?
/kind feature
/kind api-change

What this PR does / why we need it:

Does this PR introduce a user-facing change?:

Introduce GRPCRoute resource.

k8s-ci-robot · 2022-04-14T22:19:45Z

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.

robscott · 2022-04-18T18:27:27Z

/ok-to-test

markmc

One other comment - it would be good to squash the commits (it's all one logical change, unless you wanted to have the autogen stuff in a separate commit) and at least add to the commit message the GEP number you're implementing

Just trying to smooth the way to merging. HTH

apis/v1alpha2/grpcroute_types.go

markmc · 2022-05-30T13:57:33Z

apis/v1alpha2/grpcroute_types.go

+	Service *string `json:"service,omitempty"`
+
+	// Value of the method to match against. If left empty or omitted, will
+	// match all services.


Should empty/omitted match all only if Type=Exact?

No. The behavior of our prototype implementation will match all regardless of the value of type.

However, I don't feel strongly about this, if you have a compelling reason to match all only when Type=Exact.

apis/v1alpha2/grpcroute_types.go

markmc · 2022-05-30T14:13:19Z

apis/v1alpha2/grpcroute_types.go

+	// implementation must raise an 'Accepted' condition with a status of
+	// 'False' in the corresponding RouteParentStatus.
+	//
+	// Support: Core


Just thinking about what it means for GRPC specific stuff to be marked as Core if GRPCRoute itself is Extended

Referring to: https://gateway-api.sigs.k8s.io/concepts/guidelines/#overlapping-support-levels

Since these fields are never embedded in a struct with Core support level, then these fields can never be Core?

So ... it's just a nod to "when GRPCRoute becomes Core, some fields will be Core and some will be Extended" ?

No, the idea here is that this feature is Core to the GRPCRoute feature. Independent of that, GRPCRoute is extended relative to the Gateway API. There was previously a discussion on that topic here.

apis/v1alpha2/grpcroute_types.go

hack/update-codegen.sh

gnossen · 2022-06-13T22:05:50Z

@markmc Thanks for the review. Sorry for the turnaround time on the response.

As for squashing, that's going to be a little more difficult now that I've merged master in. Is it not possible to enable auto-squashing on the repo if that is the preferred commit history style?

config/crd/kustomization.yaml

youngnick

Mostly looks LGTM, but I think we need to backport some of the changes we made to HTTPRoute for v0.5.0 about BackendRefs. I'm not one hundred percent sure the semantics are the same for GRPCRoute, but we need to review at least.

apis/v1alpha2/httproute_types.go

apis/v1alpha2/shared_types.go

apis/v1alpha2/grpcroute_types.go

robscott

Thanks @gnossen! This mostly LGTM.

apis/v1alpha2/httproute_types.go

apis/v1alpha2/grpcroute_types.go

gnossen · 2022-07-25T22:21:06Z

Is there no leeway for implementations that don't support upgrades over plain text but do support HTTP/2?

@hbagdi I'm not sure why I'm not able to respond to this comment directly, so I'm responding here.

I'm thinking this must be a typo? The spec requires that implementations supporting GRPCRoute with the HTTP protocol type support HTTP/2 without an upgrade. An implementation as in your comment is absolutely conformant to the current spec as written.

I'm guessing you meant

Is there no leeway for implementations that only support HTTP/2 over plain text with an upgrade from HTTP/1 but do support HTTP/2?

If so, the answer is "not really." AFAIK, this would not be useful to any of the existing gRPC clients and servers in the wild. It would more or less be a broken implementation.

robscott · 2022-07-28T18:58:48Z

Hey @gnossen I think this is good to merge, do you mind running make generate again with go 1.19rc2? Kubernetes updated it's test infra a couple days ago which means we needed to update our codegen: #1288

gnossen · 2022-07-29T00:14:58Z

Hey @gnossen I think this is good to merge, do you mind running make generate again with go 1.19rc2? Kubernetes updated it's test infra a couple days ago which means we needed to update our codegen: #1288

Done

robscott · 2022-08-01T19:20:00Z

@gnossen Actually one more step - it looks like this needs a rebase + make generate run to get presubmits to pass. (This is because of a conflict with #1276).

robscott · 2022-08-02T00:45:59Z

Thanks for all the work on this @gnossen! Discussed at community meeting today and there were no concerns about merging this in. Will plan to merge this at some point tomorrow unless else someone LGTMs before then (that's also welcome).

/approve

k8s-ci-robot · 2022-08-02T00:46:12Z

[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

shaneutt · 2022-08-02T13:07:00Z

/lgtm

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2022

k8s-ci-robot requested review from bowei and thockin April 14, 2022 22:19

gnossen changed the title ~~Implement grpc route~~ Implement GRPCRoute Apr 14, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 18, 2022

robscott added this to the v0.6.0 milestone May 23, 2022

markmc suggested changes May 30, 2022

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 8, 2022

gnossen changed the title ~~Implement GRPCRoute~~ Implement GRPCRoute (GEP 1016) Jun 13, 2022

youngnick mentioned this pull request Jul 15, 2022

Validate At Least One of GRPCRoute.Rules.Matches.{Method,Service} is Supplied #1098

Closed

gnossen force-pushed the implement_grpc_route branch from d4f4100 to 73158df Compare July 18, 2022 18:09

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2022

gnossen force-pushed the implement_grpc_route branch from 73158df to 2860f03 Compare July 18, 2022 20:28

robscott reviewed Jul 18, 2022

View reviewed changes

config/crd/kustomization.yaml Outdated Show resolved Hide resolved

gnossen force-pushed the implement_grpc_route branch 2 times, most recently from 63f956f to bf3d3e0 Compare July 18, 2022 22:15

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 18, 2022

youngnick reviewed Jul 19, 2022

View reviewed changes

robscott reviewed Jul 25, 2022

View reviewed changes

hbagdi reviewed Jul 25, 2022

View reviewed changes

apis/v1alpha2/grpcroute_types.go Outdated Show resolved Hide resolved

apis/v1alpha2/grpcroute_types.go Outdated Show resolved Hide resolved

gnossen force-pushed the implement_grpc_route branch from bf3d3e0 to eb297cf Compare July 25, 2022 22:09

gnossen force-pushed the implement_grpc_route branch 3 times, most recently from bf23c5d to c88e329 Compare July 26, 2022 00:25

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 26, 2022

gnossen force-pushed the implement_grpc_route branch from c88e329 to 1eba178 Compare July 29, 2022 00:14

Implement GRPCRoute

493e897

gnossen force-pushed the implement_grpc_route branch from 1eba178 to 493e897 Compare August 1, 2022 20:41

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2022

k8s-ci-robot assigned shaneutt Aug 2, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2022

k8s-ci-robot merged commit 9a08f65 into kubernetes-sigs:main Aug 2, 2022

shaneutt mentioned this pull request Aug 2, 2022

v1alpha2.GRPCRoute support Kong/kubernetes-ingress-controller#2761

Closed

2 tasks

gnossen deleted the implement_grpc_route branch August 2, 2022 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement GRPCRoute (GEP 1016) #1115

Implement GRPCRoute (GEP 1016) #1115

gnossen commented Apr 14, 2022

k8s-ci-robot commented Apr 14, 2022

robscott commented Apr 18, 2022

markmc left a comment

markmc May 30, 2022

gnossen Jun 13, 2022

markmc May 30, 2022

gnossen Jun 13, 2022

gnossen commented Jun 13, 2022

youngnick left a comment

robscott left a comment

gnossen commented Jul 25, 2022

robscott commented Jul 28, 2022

gnossen commented Jul 29, 2022

robscott commented Aug 1, 2022

robscott commented Aug 2, 2022

k8s-ci-robot commented Aug 2, 2022

shaneutt commented Aug 2, 2022

Implement GRPCRoute (GEP 1016) #1115

Implement GRPCRoute (GEP 1016) #1115

Conversation

gnossen commented Apr 14, 2022

k8s-ci-robot commented Apr 14, 2022

robscott commented Apr 18, 2022

markmc left a comment

Choose a reason for hiding this comment

markmc May 30, 2022

Choose a reason for hiding this comment

gnossen Jun 13, 2022

Choose a reason for hiding this comment

markmc May 30, 2022

Choose a reason for hiding this comment

gnossen Jun 13, 2022

Choose a reason for hiding this comment

gnossen commented Jun 13, 2022

youngnick left a comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

gnossen commented Jul 25, 2022

robscott commented Jul 28, 2022

gnossen commented Jul 29, 2022

robscott commented Aug 1, 2022

robscott commented Aug 2, 2022

k8s-ci-robot commented Aug 2, 2022

shaneutt commented Aug 2, 2022