fix: honor default gateway parent refs#1047
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of ParentReference objects within the HTTPRouteController and the datastore by introducing helper functions to identify Gateway resources. The review feedback identifies a logic error in the isGatewayParentRef function, which fails to correctly handle empty string groups as the default Gateway API group per the specification. This oversight also impacts the newly added test cases, which use empty strings to attempt to test non-matching groups. Additionally, the reviewer pointed out that the new helper functions and constants are duplicated across packages and should be moved to a shared location to improve maintainability.
| if parentRef.Group != nil && *parentRef.Group != gatewayv1.Group(gatewayAPIGroupName) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
According to the Gateway API specification for ParentReference, the Group field defaults to gateway.networking.k8s.io when it is either unspecified (nil) or an empty string. The current implementation treats an explicit empty string as a non-Gateway group, which is incorrect. This logic should be updated to handle the empty string case as a default.
| if parentRef.Group != nil && *parentRef.Group != gatewayv1.Group(gatewayAPIGroupName) { | |
| return false | |
| } | |
| if parentRef.Group != nil && *parentRef.Group != "" && *parentRef.Group != gatewayv1.Group(gatewayAPIGroupName) { | |
| return false | |
| } |
| const gatewayAPIGroupName = "gateway.networking.k8s.io" | ||
|
|
||
| func isGatewayParentRef(parentRef gatewayv1.ParentReference) bool { | ||
| if parentRef.Group != nil && *parentRef.Group != gatewayv1.Group(gatewayAPIGroupName) { | ||
| return false | ||
| } | ||
| return parentRef.Kind == nil || *parentRef.Kind == gatewayv1.Kind("Gateway") | ||
| } | ||
|
|
||
| func gatewayParentRefKey(defaultNamespace string, parentRef gatewayv1.ParentReference) string { | ||
| namespace := defaultNamespace | ||
| if parentRef.Namespace != nil { | ||
| namespace = string(*parentRef.Namespace) | ||
| } | ||
| return fmt.Sprintf("%s/%s", namespace, string(parentRef.Name)) | ||
| } |
There was a problem hiding this comment.
The constant gatewayAPIGroupName and the helper functions isGatewayParentRef and gatewayParentRefKey are duplicated between the controller and the datastore. To improve maintainability and ensure consistent behavior across the project, these should be defined in a single location (e.g., the datastore package or a common utils package) and shared.
| _, err = gatewayClient.GatewayV1().HTTPRoutes(ns).Create(ctx, httpRoute, metav1.CreateOptions{}) | ||
| assert.NoError(t, err) | ||
|
|
||
| serviceGroup := gatewayv1.Group("") |
There was a problem hiding this comment.
This test case uses Group("") to represent a non-Gateway parent reference. However, per the Gateway API specification, an empty string group in a ParentReference is inferred as gateway.networking.k8s.io. To correctly test a non-matching group, please use a different value (e.g., "core" or "example.com").
| serviceGroup := gatewayv1.Group("") | |
| serviceGroup := gatewayv1.Group("core") |
| func TestMatchModelServerIgnoresNonGatewayParentRef(t *testing.T) { | ||
| s := New() | ||
| weight := uint32(100) | ||
| serviceGroup := gatewayv1.Group("") |
There was a problem hiding this comment.
This test case uses Group("") to represent a non-Gateway parent reference. However, per the Gateway API specification, an empty string group in a ParentReference is inferred as gateway.networking.k8s.io. To correctly test a non-matching group, please use a different value (e.g., "core" or "example.com").
| serviceGroup := gatewayv1.Group("") | |
| serviceGroup := gatewayv1.Group("core") |
|
|
||
| gatewayAPIGroupName = "gateway.networking.k8s.io" |
There was a problem hiding this comment.
low priority, we already have these in yml
kthena/examples/kthena-router/HTTPRoute.yaml
Lines 6 to 10 in 4db2ddd
There was a problem hiding this comment.
Fair, the examples already set them explicitly, so this is not urgent.
The reason I opened it is that Gateway API allows group and kind to be omitted and defaulted to gateway.networking.k8s.io / Gateway. So this is more about accepting valid user manifests, not fixing the current examples.
Signed-off-by: pm-ju <pmdevops29@gmail.com>
pm-ju
force-pushed
the
fix-router-default-gateway-parentrefs
branch
from
8279242 to
f628670
Compare
Signed-off-by: pm-ju <pmdevops29@gmail.com>
| const gatewayAPIGroupName = "gateway.networking.k8s.io" | ||
|
|
||
| func isGatewayParentRef(parentRef gatewayv1.ParentReference) bool { | ||
| if parentRef.Group != nil && *parentRef.Group != "" && *parentRef.Group != gatewayv1.Group(gatewayAPIGroupName) { | ||
| return false | ||
| } | ||
| return parentRef.Kind == nil || *parentRef.Kind == "" || *parentRef.Kind == gatewayv1.Kind("Gateway") | ||
| } |
| func gatewayParentRefKey(defaultNamespace string, parentRef gatewayv1.ParentReference) string { | ||
| namespace := defaultNamespace | ||
| if parentRef.Namespace != nil { | ||
| namespace = string(*parentRef.Namespace) | ||
| } | ||
| return fmt.Sprintf("%s/%s", namespace, string(parentRef.Name)) | ||
| } |
| func gatewayParentRefNamespaceAndKey(defaultNamespace string, parentRef gatewayv1.ParentReference) (string, string) { | ||
| namespace := defaultNamespace | ||
| if parentRef.Namespace != nil { | ||
| namespace = string(*parentRef.Namespace) | ||
| } | ||
| return namespace, fmt.Sprintf("%s/%s", namespace, string(parentRef.Name)) | ||
| } |
/kind bug
What this PR does / why we need it:
Gateway API ParentReference defaults group to gateway.networking.k8s.io and kind to Gateway when those fields are omitted.
The router currently requires kind: Gateway in some HTTPRoute and ModelRoute paths, so valid routes using parentRefs with only a name can be skipped or not indexed under their Gateway.
This change:
Which issue(s) this PR fixes:
None
Special notes for reviewers:
Verification:
go test ./pkg/kthena-router/datastore ./pkg/kthena-router/controller