refactor: dedup the route context code using reflection by shawnh2 · Pull Request #1502 · envoyproxy/gateway

shawnh2 · 2023-06-07T13:18:49Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1252

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

codecov · 2023-06-07T13:26:58Z

Codecov Report

Merging #1502 (efd5395) into main (ca36e3f) will increase coverage by 0.00%.
The diff coverage is 84.16%.

@@           Coverage Diff            @@
##             main    #1502    +/-   ##
========================================
  Coverage   61.70%   61.71%            
========================================
  Files          81       81            
  Lines       12131    11967   -164     
========================================
- Hits         7485     7385   -100     
+ Misses       4179     4127    -52     
+ Partials      467      455    -12

Impacted Files	Coverage Δ
internal/gatewayapi/route.go	`88.36% <77.27%> (ø)`
internal/gatewayapi/contexts.go	`84.30% <83.13%> (+8.43%)`	⬆️
internal/cmd/egctl/translate.go	`82.14% <100.00%> (+0.41%)`	⬆️

... and 3 files with indirect coverage changes

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

…e-context

…k-route-context

arkodg · 2023-06-07T18:01:45Z

internal/gatewayapi/contexts.go

+// corresponding to the const defined in translator, like KindHTTPRoute,
+// KindGRPCRoute, KindTLSRoute, KindTCPRoute, KindUDPRoute
+func GetRouteType(route RouteContext) v1beta1.Kind {
+	rv := reflect.ValueOf(route)


do you think reflect is the right package to use here or should we use https://pkg.go.dev/k8s.io/apimachinery/pkg/util/validation/field which is used heavily in upstream gateway api, here an
example

thanks for pointing it out, but i think https://pkg.go.dev/k8s.io/apimachinery/pkg/util/validation/field is not suitable for our case.

despite the fact that this package provides several powerful methods, but its intention is for validations only. in our case, we need more than that, we need to write some values for some fields in one struct as well, but this package does not have that kind of ability.

i thought about whether we could replace current validations with the one provided in that package, and turns out we may not. for example, the strings.HasPrefix(*path.Value, "/") use the variable path directly, which has explicit type *gatewayv1b1.HTTPPathMatch. however, in our case, the only variable we can manipulate with is route, which is a interface RouteContext. so we need the ability provided in package reflect, to play with the real type behinds whatever it represents.

so basically, i think relfect is inevitable.

arkodg · 2023-06-07T18:11:50Z

thanks for picking this refactor up @shawnh2 !

arkodg · 2023-06-08T19:21:19Z

internal/gatewayapi/contexts.go

+	for i := 0; i < specParentRefs.Len(); i++ {
+		p := specParentRefs.Index(i).Interface().(v1beta1.ParentReference)
+		up := p
+		if !isHTTPRoute {


curious why this custom HTTPRoute logic is needed

becasue there are some differences between the implementation of HTTPRoute and other Routes in GetRouteParentContext method.

for example, HTTPRoute is like:

gateway/internal/gatewayapi/contexts.go

Lines 220 to 226 in 7f8a491

var parentRef *v1beta1.ParentReference

for i, p := range h.Spec.ParentRefs {

if reflect.DeepEqual(p, forParentRef) {

parentRef = &h.Spec.ParentRefs[i]

break

}

}

as for other Route:

gateway/internal/gatewayapi/contexts.go

Lines 471 to 479 in 7f8a491

var parentRef *v1beta1.ParentReference

for i, p := range u.Spec.ParentRefs {

p := UpgradeParentReference(p)

if reflect.DeepEqual(p, forParentRef) {

upgraded := UpgradeParentReference(u.Spec.ParentRefs[i])

parentRef = &upgraded

break

}

}

so add a if logic to deal with HTTPRoute

arkodg · 2023-06-14T16:57:41Z

internal/gatewayapi/contexts.go

+func GetRouteType(route RouteContext) v1beta1.Kind {
+	rv := reflect.ValueOf(route)
+	rt := rv.Type().String()
+	return v1beta1.Kind(strings.TrimSuffix(rt, "Context")[strings.LastIndex(rt, ".")+1:])


this looks a little intimidating to me, can we use FieldByName here ?

sure we can use FieldByName, in that case, we need to add one more field, like type string in struct HTTP/GRPC/.../Context, indicating the type of this struct, and all the value of field type comes from:

gateway/internal/gatewayapi/translator.go

Lines 17 to 22 in 4dd4f87

KindGRPCRoute = "GRPCRoute"

KindHTTPRoute = "HTTPRoute"

KindNamespace = "Namespace"

KindTLSRoute = "TLSRoute"

KindTCPRoute = "TCPRoute"

KindUDPRoute = "UDPRoute"

@arkodg see if this is ok?

or you could also use GetKind() ?
see it being used in other places in the codebase

gateway/internal/gatewayapi/filters.go

Line 769 in 4dd4f87

if res.GetKind() == string(extFilter.Kind) && res.GetName() == string(extFilter.Name) && res.GetNamespace() == filterNs {

thanks for the heads up!

now we can use FieldByName("Kind"), BTW, I add some TypeMeta for xRoute resources in translate func, nor it will keep getting "".

internal/gatewayapi/contexts.go

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

internal/gatewayapi/contexts.go

arkodg · 2023-06-16T16:30:35Z

thanks @shawnh2 for addressing the comments, adding a minor comment, else LGTM !

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

arkodg

LGTM thanks !

shawnh2 added 2 commits June 7, 2023 21:06

dedup the context via reflect

196d23c

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

clean up code

9db75cc

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

shawnh2 requested a review from a team as a code owner June 7, 2023 13:18

Merge branch 'main' into rework-route-context

64b345b

shawnh2 added 3 commits June 7, 2023 21:27

fix lint

00a1e80

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

Merge branch 'main' of github.com:envoyproxy/gateway into rework-rout…

9a1b7e1

…e-context

Merge remote-tracking branch 'origin/rework-route-context' into rewor…

0d098cc

…k-route-context

arkodg reviewed Jun 7, 2023

View reviewed changes

arkodg reviewed Jun 8, 2023

View reviewed changes

arkodg requested a review from kflynn June 13, 2023 22:20

arkodg reviewed Jun 14, 2023

View reviewed changes

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

arkodg reviewed Jun 14, 2023

View reviewed changes

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

shawnh2 added 2 commits June 17, 2023 00:05

get route type info from Kind by FieldByName

8b1aff9

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

merge from main and resolve conflicts

edfbbb4

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

arkodg reviewed Jun 16, 2023

View reviewed changes

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

arkodg reviewed Jun 16, 2023

View reviewed changes

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

arkodg reviewed Jun 16, 2023

View reviewed changes

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

replacing string contains with the constants that already defined

efd5395

Signed-off-by: Shawnh2 <shawnhxh@outlook.com>

arkodg approved these changes Jun 20, 2023

View reviewed changes

arkodg requested review from a team, Xunzhuo and chauhanshubham and removed request for a team June 21, 2023 18:28

zirain approved these changes Jun 22, 2023

View reviewed changes

zirain merged commit 8df2474 into envoyproxy:main Jun 22, 2023

shawnh2 deleted the rework-route-context branch June 23, 2023 06:42

	var parentRef *v1beta1.ParentReference
	for i, p := range h.Spec.ParentRefs {
	if reflect.DeepEqual(p, forParentRef) {
	parentRef = &h.Spec.ParentRefs[i]
	break
	}
	}

	var parentRef *v1beta1.ParentReference
	for i, p := range u.Spec.ParentRefs {
	p := UpgradeParentReference(p)
	if reflect.DeepEqual(p, forParentRef) {
	upgraded := UpgradeParentReference(u.Spec.ParentRefs[i])
	parentRef = &upgraded
	break
	}
	}

	KindGRPCRoute = "GRPCRoute"
	KindHTTPRoute = "HTTPRoute"
	KindNamespace = "Namespace"
	KindTLSRoute = "TLSRoute"
	KindTCPRoute = "TCPRoute"
	KindUDPRoute = "UDPRoute"

Conversation

shawnh2 commented Jun 7, 2023

Uh oh!

codecov bot commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arkodg Jun 7, 2023

Choose a reason for hiding this comment

Uh oh!

shawnh2 Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

arkodg commented Jun 7, 2023

Uh oh!

arkodg Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

shawnh2 Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

shawnh2 Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

arkodg Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

shawnh2 Jun 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arkodg commented Jun 16, 2023

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jun 7, 2023 •

edited

Loading

shawnh2 Jun 9, 2023 •

edited

Loading