gatewayapi: adds matching on GRPCRoute method and service #1050

sbko · 2023-02-16T15:10:55Z

Closes #963

codecov-commenter · 2023-02-16T15:15:21Z

Codecov Report

Merging #1050 (d5e3a46) into main (b6e0471) will decrease coverage by 0.61%.
The diff coverage is 73.68%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1050      +/-   ##
==========================================
- Coverage   64.82%   64.21%   -0.61%     
==========================================
  Files          67       70       +3     
  Lines        8730     9051     +321     
==========================================
+ Hits         5659     5812     +153     
- Misses       2697     2841     +144     
- Partials      374      398      +24

Impacted Files	Coverage Δ
internal/gatewayapi/helpers.go	`75.00% <0.00%> (-1.78%)`	⬇️
internal/gatewayapi/route.go	`86.31% <100.00%> (+1.18%)`	⬆️
internal/provider/kubernetes/helpers.go	`81.08% <0.00%> (-1.36%)`	⬇️
...rnal/infrastructure/kubernetes/proxy_deployment.go	`89.28% <0.00%> (-1.14%)`	⬇️
internal/provider/kubernetes/controller.go	`50.22% <0.00%> (-1.02%)`	⬇️
internal/gatewayapi/contexts.go	`76.22% <0.00%> (-0.57%)`	⬇️
internal/gatewayapi/translator.go	`97.10% <0.00%> (ø)`
internal/cmd/egctl/experimental.go	`0.00% <0.00%> (ø)`
internal/cmd/egctl/translate.go	`66.98% <0.00%> (ø)`
internal/cmd/egctl/version.go	`0.00% <0.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

internal/gatewayapi/route.go

arkodg · 2023-02-16T18:41:02Z

hey @sbko thanks for picking this up, can you incorporate one of these matches into the user doc https://github.com/envoyproxy/gateway/blob/main/docs/latest/user/grpc-routing.md, maybe add a service match for yages.Ping

internal/gatewayapi/route.go

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

sbko · 2023-02-17T14:38:22Z

hey @sbko thanks for picking this up, can you incorporate one of these matches into the user doc https://github.com/envoyproxy/gateway/blob/main/docs/latest/user/grpc-routing.md, maybe add a service match for yages.Ping

done

docs/latest/user/grpc-routing.md

arkodg · 2023-02-17T17:26:32Z

internal/gatewayapi/route.go

+				} else if match.Method.Method != nil {
+					// Use regex match if only the method name is specified to match any service
+					irRoute.PathMatch = &ir.StringMatch{
+						SafeRegex: StringPtr(fmt.Sprintf("^/(?i)\\.?[a-z_][a-z_0-9]*(\\.[a-z_][a-z_0-9]*)*/%s$", *match.Method.Method)),


lets make this into a constant, so it can be reused, so will need to spend some cycles to ensure this regex looks right

made it constant, refactored if else if so no need for nolint:gocritic anymore. please let me know if you prefer previous structure.

arkodg · 2023-02-17T17:27:54Z

internal/gatewayapi/route.go

+			case v1alpha2.GRPCMethodMatchRegularExpression:
+				if match.Method.Service != nil && match.Method.Method != nil {
+					irRoute.PathMatch = &ir.StringMatch{
+						SafeRegex: StringPtr(fmt.Sprintf("^/%s/%s", *match.Method.Service, *match.Method.Method)),


why is ^ needed ?

added for consistency between regexes. maybe slightly faster(?) :)

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

arkodg · 2023-02-17T22:47:12Z

internal/gatewayapi/route.go

@@ -26,6 +26,11 @@ type RoutesTranslator interface {
 	ProcessUDPRoutes(udpRoutes []*v1alpha2.UDPRoute, gateways []*GatewayContext, resources *Resources, xdsIR XdsIRMap) []*UDPRouteContext
 }

+const (
+	GRPCServiceRegexPattern = "(?i)\\.?[a-z_][a-z_0-9]*(\\.[a-z_][a-z_0-9]*)*"


why are there two different types here, isnt the only use case needed is when we want to match on method but service is empty ?

We need GRPMethodRegexPattern to support a use case when service is specified with type: RegularExpression and emtpy method. Envoy's prefix is used when type is empty or type: Exact

arkodg · 2023-02-17T22:48:31Z

internal/gatewayapi/route.go

@@ -26,6 +26,11 @@ type RoutesTranslator interface {
 	ProcessUDPRoutes(udpRoutes []*v1alpha2.UDPRoute, gateways []*GatewayContext, resources *Resources, xdsIR XdsIRMap) []*UDPRouteContext
 }

+const (


@lizan is there a smarter way to do path segment matching in envoy rather than relying on regex matching ?

From GEP-1016 we can use suffix match (with header matcher)?

lizan · 2023-02-18T19:18:03Z

internal/gatewayapi/route.go

+			case match.Method.Method == nil:
+				// Use prefix match if only the service name is specified
+				irRoute.PathMatch = &ir.StringMatch{
+					Prefix: StringPtr(fmt.Sprintf("/%s", service)),


use path_separated_prefix or add / at the end of prefix?

it uses path_separated_prefix

lizan · 2023-02-18T19:18:26Z

internal/gatewayapi/route.go

@@ -26,6 +26,11 @@ type RoutesTranslator interface {
 	ProcessUDPRoutes(udpRoutes []*v1alpha2.UDPRoute, gateways []*GatewayContext, resources *Resources, xdsIR XdsIRMap) []*UDPRouteContext
 }

+const (


From GEP-1016 we can use suffix match (with header matcher)?

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

docs/latest/user/grpc-routing.md

internal/gatewayapi/route.go

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

arkodg

LGTM thanks !

sbko requested a review from a team as a code owner February 16, 2023 15:10

gatewayapi: add matching on GRPCRoute method and service

2d4390a

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

sbko force-pushed the grpcroutematch branch from c9834a5 to 2d4390a Compare February 16, 2023 15:26

arkodg reviewed Feb 16, 2023

View reviewed changes

internal/gatewayapi/route.go Outdated Show resolved Hide resolved

arkodg reviewed Feb 16, 2023

View reviewed changes

internal/gatewayapi/route.go Outdated Show resolved Hide resolved

arkodg reviewed Feb 16, 2023

View reviewed changes

internal/gatewayapi/route.go Outdated Show resolved Hide resolved

arkodg added this to the 0.4.0-rc.1 milestone Feb 16, 2023

sbko added 2 commits February 17, 2023 09:23

gatewayapi: tweak regex for GRPCRoute match

400f1a4

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

gatewayapi: add GRPCRoute match example in docs

3cfc55b

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

sbko force-pushed the grpcroutematch branch from 25c576e to 3cfc55b Compare February 17, 2023 14:35

arkodg reviewed Feb 17, 2023

View reviewed changes

docs/latest/user/grpc-routing.md Show resolved Hide resolved

arkodg reviewed Feb 17, 2023

View reviewed changes

sbko added 2 commits February 17, 2023 14:01

docs: update GRPCRoute match example

8440478

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

refactor GRPCRoute method and service match

fbb0753

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

arkodg reviewed Feb 17, 2023

View reviewed changes

lizan reviewed Feb 18, 2023

View reviewed changes

sbko added 2 commits February 22, 2023 12:08

use suffix match for grpcroute when only method is specified

a5aa1fe

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

update GRPCRoute docs

d5e3a46

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

sbko requested a review from arkodg February 22, 2023 17:34

arkodg reviewed Feb 22, 2023

View reviewed changes

docs/latest/user/grpc-routing.md Show resolved Hide resolved

arkodg reviewed Feb 22, 2023

View reviewed changes

internal/gatewayapi/route.go Show resolved Hide resolved

arkodg reviewed Feb 22, 2023

View reviewed changes

internal/gatewayapi/route.go Show resolved Hide resolved

add PathMatch/Suffix comment

4611795

Signed-off-by: Stanislav Bondarenko <stanislav.bondarenko@gmail.com>

arkodg approved these changes Feb 22, 2023

View reviewed changes

arkodg merged commit bf7993c into envoyproxy:main Feb 23, 2023

sbko deleted the grpcroutematch branch February 23, 2023 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gatewayapi: adds matching on GRPCRoute method and service #1050

gatewayapi: adds matching on GRPCRoute method and service #1050

sbko commented Feb 16, 2023

codecov-commenter commented Feb 16, 2023 •

edited

arkodg commented Feb 16, 2023

sbko commented Feb 17, 2023

arkodg Feb 17, 2023

sbko Feb 17, 2023

arkodg Feb 17, 2023

sbko Feb 17, 2023

arkodg Feb 17, 2023

sbko Feb 18, 2023 •

edited

arkodg Feb 17, 2023 •

edited

lizan Feb 18, 2023

lizan Feb 18, 2023

sbko Feb 20, 2023

lizan Feb 18, 2023

arkodg left a comment

gatewayapi: adds matching on GRPCRoute method and service #1050

gatewayapi: adds matching on GRPCRoute method and service #1050

Conversation

sbko commented Feb 16, 2023

codecov-commenter commented Feb 16, 2023 • edited

Codecov Report

arkodg commented Feb 16, 2023

sbko commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbko Feb 18, 2023 • edited

Choose a reason for hiding this comment

arkodg Feb 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 16, 2023 •

edited

sbko Feb 18, 2023 •

edited

arkodg Feb 17, 2023 •

edited