add gRPC route controller #341

xWink · 2023-08-18T09:15:53Z

What type of PR is this?
feature

Which issue does this PR fix:
#25 (partial)

What does this PR do / Why do we need it:

Adds GRPCRoute CRD
Replaces HTTPRoute Controller and EventHandler with a consolidated Route controller and event handler
- This allows for all registered route types to be handled by the same controller and event handler, due to the abstraction of HTTP and GRPC routes
- Creation, Update, and Deletion of GRPCRoutes in Kubernetes now results in the same process as that of an HTTPRoute
Adds rule builder logic for GRPCRouteMatch which converts K8s route matches to Lattice rule matches
Fixes example grpc-route.yaml so that it deploys a GRPCRoute instead of HTTPRoute
Fixes example grpc-server.yaml so that it uses a working image and HTTPS port
Adds -v 6 flag when running in developer mode
Adds Equals() method to many route abstraction constructs to enable comparisons without relying on DeepEquals(), which may fail due to reliance of reflection
General cleanup (spacing, indentation, log migration from glog to zap, improved log contents)

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

kubectl apply -f examples/my-hotel-gateway.yaml
kubectl apply -f examples/grpc-server.yaml
kubectl apply -f examples/grpc-route.yaml

Should create VPC Lattice Service routing to GRPC server. If a service and method is specified in grpc-route.yaml, the rule using a PathMatchExact value of /<service>/<method> is expected. If a service is provided, but no method is provided, a PatchMatchPrefix value of /<service>/ is expected. If neither a service, nor a method is provided, a PathMatchPrefix value of / is expected. If no service is provided, but a method is provided, this should fail to reconcile since Lattice doesn't support Regex or Suffix matches.

Additionally, deletion of these resources should succeed, with both Lattice and K8s resources being deleted.

Testing done on this change:

Ran e2e tests using latest changes:

• [224.537 seconds]
------------------------------

Ran 11 of 11 Specs in 2631.797 seconds
SUCCESS! -- 11 Passed | 0 Failed | 0 Pending | 0 Skipped

And ran the manual steps mentioned above.

Automation added to e2e:

No, this is to be done separately.

Will this PR introduce any new dependencies?:

No.

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
The new CRD must be deployed to the cluster. The following commands will do so:

kubectl apply -f config/crds/bases/k8s-gateway-v0.6.1.yaml

Does this PR introduce any user-facing change?:

Yes, though the feature is not complete yet.
Remaining work in followup PR:

Reconcile GRPCRoutes on Gateway deletion
TargetGroupModelBuilder for Lattice GRPC Target Group
TargetGroupSynthesizer checks for GRPCRoute alongside existing check for HTTPRoute
Service EventHandler generification for all route types

action required
Added support for creating, updating, and deleting GRPCRoutes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

# Conflicts: # test/pkg/test/framework.go

…g struct value instead of reference in httproute_controller, Using type switch with default error instead of if statement without error, Joined two logs into one

# Conflicts: # pkg/deploy/lattice/rule_manager.go # pkg/gateway/model_build_rule.go # pkg/gateway/model_build_rule_test.go # pkg/model/lattice/rule.go

coveralls · 2023-08-18T09:25:00Z

Pull Request Test Coverage Report for Build 5930851420

38 of 250 (15.2%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.8%) to 39.924%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/gateway/model_build_lattice_service.go	0	5	0.0%
pkg/gateway/model_build_rule.go	38	44	86.36%
pkg/model/core/route.go	0	201	0.0%

Totals
Change from base Build 5930673692:	-0.8%
Covered Lines:	3455
Relevant Lines:	8654

💛 - Coveralls

mikhail-aws

I think it's worth to invest time on reusing existing http controller, there are similarities. Also amount of copy-pasted code good indicator for reuse.

cmd/aws-application-networking-k8s/main.go

controllers/eventhandlers/grpcroute.go

controllers/grpcroute_controller.go

pkg/gateway/model_build_lattice_service.go

pkg/k8s/events.go

controllers/grpcroute_controller.go

pkg/model/lattice/rule.go

controllers/grpcroute_controller.go

xWink · 2023-08-19T09:02:18Z

Addressed all comments. E2E tests still pass:

------------------------------

Ran 11 of 11 Specs in 2448.453 seconds
SUCCESS! -- 11 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (2449.31s)

And manual steps from description still function as expected.

mikhail-aws

Looks much better! I'm a bit worried that we have quite a bit of logic in controller and not covered by unit tests at all. We should have a follow up on this, after gRPC wrap up:

move everything into pkg, let controller be simple lunching pad
start adding unit tests

cmd/aws-application-networking-k8s/main.go

controllers/route_controller.go

zijun726911 · 2023-08-21T16:41:47Z

When I run the kubectl apply -f config/crds/bases/k8s-gateway-v1alpha2.yaml

it meet the:

Resource: "apiextensions.k8s.io/v1, Resource=customresourcedefinitions", GroupVersionKind: "apiextensions.k8s.io/v1, Kind=CustomResourceDefinition"
Name: "httproutes.gateway.networking.k8s.io", Namespace: ""
for: "config/crds/bases/k8s-gateway-v1alpha2.yaml": error when patching "config/crds/bases/k8s-gateway-v1alpha2.yaml": CustomResourceDefinition.apiextensions.k8s.io "httproutes.gateway.networking.k8s.io" is invalid: status.storedVersions[0]: Invalid value: "v1beta1": must appear in spec.versions

Is that normal?

zijun726911 · 2023-08-21T16:49:10Z

I think created TG ProtocolVersion should be GRPC or not? but you didn't change this part?:

aws-application-networking-k8s/pkg/gateway/model_build_targetgroup.go

Line 361 in 662a1fb

ProtocolVersion: vpclattice.TargetGroupProtocolVersionHttp1,

Or, you plan to change it in a separate PR?

zijun726911 · 2023-08-21T16:51:49Z

examples/grpc-route.yaml

-apiVersion: gateway.networking.k8s.io/v1beta1
-kind: HTTPRoute
+apiVersion: gateway.networking.k8s.io/v1alpha2
+kind: GRPCRoute
 metadata:
  name: grpc


Could you change this name to some thing like my-grpc-route?

xWink · 2023-08-21T19:22:54Z

I think created TG ProtocolVersion should be GRPC or not? but you didn't change this part?:

aws-application-networking-k8s/pkg/gateway/model_build_targetgroup.go

Line 361 in 662a1fb

ProtocolVersion: vpclattice.TargetGroupProtocolVersionHttp1,

Or, you plan to change it in a separate PR?

Yes, this is part of the actions required for a future PR mentioned in the description. This PR is purely meant to set up the controller and matches, the next PR will be to enable gRPC traffic. There are multiple places that need changes, with the target group model builder being one of them.

xWink · 2023-08-21T19:29:03Z

When I run the kubectl apply -f config/crds/bases/k8s-gateway-v1alpha2.yaml

it meet the:

Resource: "apiextensions.k8s.io/v1, Resource=customresourcedefinitions", GroupVersionKind: "apiextensions.k8s.io/v1, Kind=CustomResourceDefinition"
Name: "httproutes.gateway.networking.k8s.io", Namespace: ""
for: "config/crds/bases/k8s-gateway-v1alpha2.yaml": error when patching "config/crds/bases/k8s-gateway-v1alpha2.yaml": CustomResourceDefinition.apiextensions.k8s.io "httproutes.gateway.networking.k8s.io" is invalid: status.storedVersions[0]: Invalid value: "v1beta1": must appear in spec.versions

Is that normal?

Yes, this was an issue before my PR, and wasn't actually a necessary step. We only change the v0.6.1 CRD and only need to apply that one. I removed that command from the PR description. The issue with applying the alpha CRD is out of scope here

# Conflicts: # examples/grpc-route.yaml

zijun726911

LGTM, looking forward to your next few GRPCRoute PRs

Shawn Kaplan and others added 26 commits August 3, 2023 21:09

Added route abstraction

beada24

Merge branch 'main' into 25

bfdd085

# Conflicts: # test/pkg/test/framework.go

Fixed some broken test logic

51cbb5e

Fixed abstraction usage

9f673c8

Finalized initial abstraction

e5c5939

Added keys to unkeyed values

bf1b7fc

Switched to struct reference methods rather than value, Reverted usin…

e6c0aaf

…g struct value instead of reference in httproute_controller, Using type switch with default error instead of if statement without error, Joined two logs into one

Made abstracted constructs private

cc5c1b8

Fixed missing brackets

c8b117c

Added GRPC Route Match Support

7fc63ea

Switched to PathMatch for gRPC matches

ffc9945

Merge branch 'main' into 25

c4b9dbf

Merge branch 'aws:main' into 25

72565b4

Merge remote-tracking branch 'origin/25' into 25

59d4152

Merge branch 'main' into 25

bb4afc8

Merge branch 'main' into 25

dddbd66

Added GPRCRoute CRD

7fbd377

Added GRPCRoute model building

88a8c76

Added test for method match on all grpc methods

974b679

Added fix for target group deletion

7c84d19

Merge branch 'main' into 25

b56d552

Merge branch 'main' into 25

eac7c78

# Conflicts: # pkg/deploy/lattice/rule_manager.go # pkg/gateway/model_build_rule.go # pkg/gateway/model_build_rule_test.go # pkg/model/lattice/rule.go

Resolved merge conflicts from main

eab6b41

Added GRPCRoute controller and event handler

42bc17f

Merge branch 'main' into 25

5419903

Added vpclattice_mocks modification

8139ab1

xWink requested a review from mikhail-aws August 18, 2023 09:25

mikhail-aws changed the title 25 add gRPC route controller Aug 18, 2023

mikhail-aws reviewed Aug 18, 2023

View reviewed changes

zijun726911 reviewed Aug 18, 2023

View reviewed changes

controllers/grpcroute_controller.go Outdated Show resolved Hide resolved

Shawn Kaplan added 7 commits August 18, 2023 14:12

Add '-v 6' flag when running in dev mode

39195aa

Consolidated route event names

9ed57cc

Consolidated route controllers

24d6849

use gwlog instead of glog

cf36275

Consolidate route event handlers

6836ed8

Removed unused function

626907a

Removed unnecessary comments

7a90c5d

mikhail-aws reviewed Aug 20, 2023

View reviewed changes

Shawn Kaplan added 2 commits August 21, 2023 00:30

Merge branch 'main' into 25

608acfa

Code and log cleanup

ad6c346

zijun726911 reviewed Aug 21, 2023

View reviewed changes

Merge branch 'main' into 25

dacf189

# Conflicts: # examples/grpc-route.yaml

mikhail-aws approved these changes Aug 21, 2023

View reviewed changes

zijun726911 approved these changes Aug 21, 2023

View reviewed changes

xWink merged commit 1c36c1e into aws:main Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add gRPC route controller #341

add gRPC route controller #341

xWink commented Aug 18, 2023 •

edited

Loading

coveralls commented Aug 18, 2023 •

edited

Loading

mikhail-aws left a comment

xWink commented Aug 19, 2023 •

edited

Loading

mikhail-aws left a comment •

edited

Loading

zijun726911 commented Aug 21, 2023

zijun726911 commented Aug 21, 2023 •

edited

Loading

zijun726911 Aug 21, 2023

xWink Aug 21, 2023

xWink commented Aug 21, 2023

xWink commented Aug 21, 2023 •

edited

Loading

zijun726911 left a comment

add gRPC route controller #341

add gRPC route controller #341

Conversation

xWink commented Aug 18, 2023 • edited Loading

coveralls commented Aug 18, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5930851420

💛 - Coveralls

mikhail-aws left a comment

Choose a reason for hiding this comment

xWink commented Aug 19, 2023 • edited Loading

mikhail-aws left a comment • edited Loading

Choose a reason for hiding this comment

zijun726911 commented Aug 21, 2023

zijun726911 commented Aug 21, 2023 • edited Loading

zijun726911 Aug 21, 2023

Choose a reason for hiding this comment

xWink Aug 21, 2023

Choose a reason for hiding this comment

xWink commented Aug 21, 2023

xWink commented Aug 21, 2023 • edited Loading

zijun726911 left a comment

Choose a reason for hiding this comment

xWink commented Aug 18, 2023 •

edited

Loading

coveralls commented Aug 18, 2023 •

edited

Loading

xWink commented Aug 19, 2023 •

edited

Loading

mikhail-aws left a comment •

edited

Loading

zijun726911 commented Aug 21, 2023 •

edited

Loading

xWink commented Aug 21, 2023 •

edited

Loading