Skip to content

Commit 86841e2

Browse files
sarthypartysarthyparty
authored
Add support for Port in ParentReference (#3778)
Problem: ParentRefs didn't support Port Solution: I added support for it and enabled conformance
1 parent 918b9d4 commit 86841e2

File tree

4 files changed

+223
-49
lines changed

4 files changed

+223
-49
lines changed

‎internal/controller/state/graph/policies.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -457,6 +457,9 @@ func checkForRouteOverlap(route *L7Route, hostPortPaths map[string]string) *cond
457457
for _, parentRef := range route.ParentRefs {
458458
if parentRef.Attachment != nil {
459459
port := parentRef.Attachment.ListenerPort
460+
// FIXME(sarthyparty): https://github.com/nginx/nginx-gateway-fabric/issues/3811
461+
// Need to merge listener hostnames with route hostnames so wildcards are handled correctly
462+
// Also the AcceptedHostnames is a map of slices of strings, so we need to flatten it
460463
for _, hostname := range parentRef.Attachment.AcceptedHostnames {
461464
for _, rule := range route.Spec.Rules {
462465
for _, match := range rule.Matches {

‎internal/controller/state/graph/route_common.go

Lines changed: 59 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ type ParentRefAttachmentStatus struct {
4646
// still attach. The backendRef condition would be displayed here.
4747
FailedConditions []conditions.Condition
4848
// ListenerPort is the port on the Listener that the Route is attached to.
49+
// FIXME(sarthyparty): https://github.com/nginx/nginx-gateway-fabric/issues/3811
50+
// Needs to be a map of <gatewayNamespacedName/listenerName> to port number
4951
ListenerPort v1.PortNumber
5052
// Attached indicates if the ParentRef is attached to the Gateway.
5153
Attached bool
@@ -307,24 +309,37 @@ func buildSectionNameRefs(
307309
gwNsName: gwNsName,
308310
}
309311

310-
// If there is no section name, we create ParentRefs for each listener in the gateway
312+
// If there is no section name, handle based on whether port is specified
313+
// FIXME(sarthyparty): https://github.com/nginx/nginx-gateway-fabric/issues/3811
314+
// this logic seems to be duplicated in findAttachableListeners so we should refactor this,
315+
// either here or in findAttachableListeners
311316
if p.SectionName == nil {
312-
for _, l := range gw.Listeners {
313-
k.sectionName = string(l.Source.Name)
314-
315-
if err := checkUniqueSections(k); err != nil {
316-
return nil, err
317-
}
318-
317+
// If port is specified, preserve the port-only nature for proper validation
318+
if p.Port != nil {
319319
sectionNameRefs = append(sectionNameRefs, ParentRef{
320-
// if the ParentRefs we create are for each listener in the same gateway, we keep the
321-
// parentRefIndex the same so when we look at a route's parentRef's we can see
322-
// if the parentRef is a unique parentRef or one we created internally
323320
Idx: i,
324321
Gateway: CreateParentRefGateway(gw),
325-
SectionName: &l.Source.Name,
322+
SectionName: nil, // Keep as nil to preserve port-only semantics
326323
Port: p.Port,
327324
})
325+
} else {
326+
// If there is no port and section name, we create ParentRefs for each listener in the gateway
327+
for _, l := range gw.Listeners {
328+
k.sectionName = string(l.Source.Name)
329+
330+
if err := checkUniqueSections(k); err != nil {
331+
return nil, err
332+
}
333+
334+
sectionNameRefs = append(sectionNameRefs, ParentRef{
335+
// if the ParentRefs we create are for each listener in the same gateway, we keep the
336+
// parentRefIndex the same so when we look at a route's parentRef's we can see
337+
// if the parentRef is a unique parentRef or one we created internally
338+
Idx: i,
339+
Gateway: CreateParentRefGateway(gw),
340+
SectionName: &l.Source.Name,
341+
})
342+
}
328343
}
329344

330345
continue
@@ -532,10 +547,8 @@ func validateParentRef(
532547

533548
ref.Attachment = attachment
534549

535-
path := field.NewPath("spec").Child("parentRefs").Index(ref.Idx)
536-
537550
attachableListeners, listenerExists := findAttachableListeners(
538-
getSectionName(ref.SectionName),
551+
ref,
539552
gw.Listeners,
540553
)
541554

@@ -546,17 +559,7 @@ func validateParentRef(
546559
return attachment, nil
547560
}
548561

549-
// Case 2: Attachment is not possible due to unsupported configuration.
550-
551-
if ref.Port != nil {
552-
valErr := field.Forbidden(path.Child("port"), "cannot be set")
553-
attachment.FailedConditions = append(
554-
attachment.FailedConditions, conditions.NewRouteUnsupportedValue(valErr.Error()),
555-
)
556-
return attachment, attachableListeners
557-
}
558-
559-
// Case 3: Attachment is not possible because Gateway is invalid
562+
// Case 2: Attachment is not possible because Gateway is invalid
560563

561564
if !gw.Valid {
562565
attachment.FailedConditions = append(attachment.FailedConditions, conditions.NewRouteInvalidGateway())
@@ -873,29 +876,50 @@ func tryToAttachL7RouteToListeners(
873876

874877
// findAttachableListeners returns a list of attachable listeners and whether the listener exists for a non-empty
875878
// sectionName.
876-
func findAttachableListeners(sectionName string, listeners []*Listener) ([]*Listener, bool) {
879+
func findAttachableListeners(ref *ParentRef, listeners []*Listener) ([]*Listener, bool) {
880+
sectionName := getSectionName(ref.SectionName)
881+
882+
// Case 1: sectionName is specified - look for that specific listener
877883
if sectionName != "" {
878884
for _, l := range listeners {
879885
if l.Name == sectionName {
880-
if l.Attachable {
886+
if l.Attachable && (ref.Port == nil || l.Source.Port == *ref.Port) {
881887
return []*Listener{l}, true
882888
}
889+
// We return false because we didn't find a listener that matches the port
890+
if ref.Port != nil && l.Source.Port != *ref.Port {
891+
return nil, false
892+
}
893+
// Return true because we found a listener that matches the sectionName and port but is not attachable
883894
return nil, true
884895
}
885896
}
886897
return nil, false
887898
}
888899

889-
attachableListeners := make([]*Listener, 0, len(listeners))
890-
for _, l := range listeners {
891-
if !l.Attachable {
892-
continue
900+
// Case 2: Only port is specified - find all attachable listeners matching that port
901+
if ref.Port != nil {
902+
var attachableListeners []*Listener
903+
var foundListener bool
904+
for _, l := range listeners {
905+
if l.Source.Port == *ref.Port {
906+
foundListener = true
907+
if l.Attachable {
908+
attachableListeners = append(attachableListeners, l)
909+
}
910+
}
893911
}
894-
895-
attachableListeners = append(attachableListeners, l)
912+
return attachableListeners, foundListener
896913
}
897914

898-
return attachableListeners, true
915+
// Case 3: Neither sectionName nor port specified - return all attachable listeners
916+
var attachableListeners []*Listener
917+
for _, l := range listeners {
918+
if l.Attachable {
919+
attachableListeners = append(attachableListeners, l)
920+
}
921+
}
922+
return attachableListeners, len(listeners) > 0
899923
}
900924

901925
func findAcceptedHostnames(listenerHostname *v1.Hostname, routeHostnames []v1.Hostname) []string {

‎internal/controller/state/graph/route_common_test.go

Lines changed: 160 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,9 @@ func TestBindRouteToListeners(t *testing.T) {
720720
Source: gw,
721721
Valid: true,
722722
Listeners: []*Listener{
723-
createListener("listener-80-1"),
723+
createModifiedListener("listener-80-1", func(l *Listener) {
724+
l.Source.Port = 80
725+
}),
724726
},
725727
},
726728
expectedSectionNameRefs: []ParentRef{
@@ -729,19 +731,24 @@ func TestBindRouteToListeners(t *testing.T) {
729731
Gateway: &ParentRefGateway{NamespacedName: client.ObjectKeyFromObject(gw)},
730732
SectionName: hrWithPort.Spec.ParentRefs[0].SectionName,
731733
Attachment: &ParentRefAttachmentStatus{
732-
Attached: false,
733-
FailedConditions: []conditions.Condition{
734-
conditions.NewRouteUnsupportedValue(
735-
`spec.parentRefs[0].port: Forbidden: cannot be set`,
736-
),
734+
Attached: true,
735+
FailedConditions: nil,
736+
AcceptedHostnames: map[string][]string{
737+
"test/gateway/listener-80-1": {"foo.example.com"},
737738
},
738-
AcceptedHostnames: map[string][]string{},
739+
ListenerPort: 80,
739740
},
740741
Port: hrWithPort.Spec.ParentRefs[0].Port,
741742
},
742743
},
743744
expectedGatewayListeners: []*Listener{
744-
createListener("listener-80-1"),
745+
func() *Listener {
746+
l := createModifiedListener("listener-80-1", func(l *Listener) {
747+
l.Source.Port = 80
748+
})
749+
l.Routes[CreateRouteKey(hrWithPort)] = routeWithPort
750+
return l
751+
}(),
745752
},
746753
name: "port is configured",
747754
},
@@ -1904,17 +1911,17 @@ func TestBindL4RouteToListeners(t *testing.T) {
19041911
Name: "gateway",
19051912
},
19061913
Listeners: []*Listener{
1907-
createListener("listener-443"),
1914+
createModifiedListener("listener-443", func(l *Listener) {
1915+
l.Source.Port = 443
1916+
}),
19081917
},
19091918
},
19101919
expectedSectionNameRefs: []ParentRef{
19111920
{
19121921
Attachment: &ParentRefAttachmentStatus{
19131922
AcceptedHostnames: map[string][]string{},
19141923
FailedConditions: []conditions.Condition{
1915-
conditions.NewRouteUnsupportedValue(
1916-
`spec.parentRefs[0].port: Forbidden: cannot be set`,
1917-
),
1924+
conditions.NewRouteNoMatchingParent(),
19181925
},
19191926
Attached: false,
19201927
},
@@ -1925,7 +1932,9 @@ func TestBindL4RouteToListeners(t *testing.T) {
19251932
},
19261933
},
19271934
expectedGatewayListeners: []*Listener{
1928-
createListener("listener-443"),
1935+
createModifiedListener("listener-443", func(l *Listener) {
1936+
l.Source.Port = 443
1937+
}),
19291938
},
19301939
name: "port is not nil",
19311940
},
@@ -3720,3 +3729,141 @@ func TestBindRoutesToListeners(t *testing.T) {
37203729
bindRoutesToListeners(nil, nil, nil, nil)
37213730
}).ToNot(Panic())
37223731
}
3732+
3733+
func TestFindAttachableListenersWithPort(t *testing.T) {
3734+
t.Parallel()
3735+
3736+
port80 := gatewayv1.PortNumber(80)
3737+
port443 := gatewayv1.PortNumber(443)
3738+
port8080 := gatewayv1.PortNumber(8080)
3739+
3740+
httpListener := &Listener{
3741+
Name: "http-80",
3742+
Attachable: true,
3743+
Source: gatewayv1.Listener{
3744+
Name: "http-80",
3745+
Port: port80,
3746+
},
3747+
}
3748+
3749+
httpsListener := &Listener{
3750+
Name: "https-443",
3751+
Attachable: true,
3752+
Source: gatewayv1.Listener{
3753+
Name: "https-443",
3754+
Port: port443,
3755+
},
3756+
}
3757+
3758+
nonAttachableListener := &Listener{
3759+
Name: "http-8080",
3760+
Attachable: false, // not attachable
3761+
Source: gatewayv1.Listener{
3762+
Name: "http-8080",
3763+
Port: port8080,
3764+
},
3765+
}
3766+
3767+
tests := []struct {
3768+
name string
3769+
parentRef *ParentRef
3770+
expectedListeners []*Listener
3771+
expectedListenerExists bool
3772+
}{
3773+
{
3774+
name: "port 80 filter returns only port 80 listener",
3775+
parentRef: &ParentRef{
3776+
Port: &port80,
3777+
},
3778+
expectedListeners: []*Listener{httpListener},
3779+
expectedListenerExists: true,
3780+
},
3781+
{
3782+
name: "port 443 filter returns only port 443 listener",
3783+
parentRef: &ParentRef{
3784+
Port: &port443,
3785+
},
3786+
expectedListeners: []*Listener{httpsListener},
3787+
expectedListenerExists: true,
3788+
},
3789+
{
3790+
name: "port 8080 filter returns empty because listener is not attachable",
3791+
parentRef: &ParentRef{
3792+
Port: &port8080,
3793+
},
3794+
expectedListeners: []*Listener{},
3795+
expectedListenerExists: true,
3796+
},
3797+
{
3798+
name: "port 9999 filter returns empty because no listener has that port",
3799+
parentRef: &ParentRef{
3800+
Port: helpers.GetPointer(gatewayv1.PortNumber(9999)),
3801+
},
3802+
expectedListeners: []*Listener{},
3803+
expectedListenerExists: false,
3804+
},
3805+
{
3806+
name: "no port specified returns all attachable listeners",
3807+
parentRef: &ParentRef{
3808+
Port: nil,
3809+
},
3810+
expectedListeners: []*Listener{httpListener, httpsListener},
3811+
expectedListenerExists: true,
3812+
},
3813+
{
3814+
name: "sectionName with matching port returns that specific listener",
3815+
parentRef: &ParentRef{
3816+
SectionName: helpers.GetPointer(gatewayv1.SectionName("http-80")),
3817+
Port: &port80,
3818+
},
3819+
expectedListeners: []*Listener{httpListener},
3820+
expectedListenerExists: true,
3821+
},
3822+
{
3823+
name: "sectionName with non-matching port returns empty",
3824+
parentRef: &ParentRef{
3825+
SectionName: helpers.GetPointer(gatewayv1.SectionName("http-80")),
3826+
Port: &port443, // wrong port for http-80 listener
3827+
},
3828+
expectedListeners: []*Listener{},
3829+
expectedListenerExists: false,
3830+
},
3831+
{
3832+
name: "sectionName that doesn't exist returns empty with false",
3833+
parentRef: &ParentRef{
3834+
SectionName: helpers.GetPointer(gatewayv1.SectionName("nonexistent")),
3835+
Port: &port80,
3836+
},
3837+
expectedListeners: []*Listener{},
3838+
expectedListenerExists: false,
3839+
},
3840+
}
3841+
3842+
for _, tt := range tests {
3843+
t.Run(tt.name, func(t *testing.T) {
3844+
t.Parallel()
3845+
g := NewWithT(t)
3846+
3847+
attachableListeners, listenerExists := findAttachableListeners(
3848+
tt.parentRef,
3849+
[]*Listener{httpListener, httpsListener, nonAttachableListener},
3850+
)
3851+
3852+
g.Expect(listenerExists).To(Equal(tt.expectedListenerExists))
3853+
g.Expect(attachableListeners).To(HaveLen(len(tt.expectedListeners)))
3854+
3855+
// Compare listeners by name since they're the same instances
3856+
expectedNames := make([]string, len(tt.expectedListeners))
3857+
for i, l := range tt.expectedListeners {
3858+
expectedNames[i] = l.Name
3859+
}
3860+
3861+
actualNames := make([]string, len(attachableListeners))
3862+
for i, l := range attachableListeners {
3863+
actualNames[i] = l.Name
3864+
}
3865+
3866+
g.Expect(actualNames).To(ConsistOf(expectedNames))
3867+
})
3868+
}
3869+
}

‎tests/Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ GW_SERVICE_TYPE = NodePort## Service type to use for the gateway
1212
NGF_VERSION ?= edge## NGF version to be tested
1313
PULL_POLICY = Never## Pull policy for the images
1414
NGINX_CONF_DIR = internal/controller/nginx/conf
15-
SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,GatewayAddressEmpty,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteRequestPercentageMirror,HTTPRouteBackendProtocolWebSocket
15+
SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,GatewayAddressEmpty,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteRequestPercentageMirror,HTTPRouteBackendProtocolWebSocket,HTTPRouteParentRefPort,HTTPRouteDestinationPortMatching
1616
STANDARD_CONFORMANCE_PROFILES = GATEWAY-HTTP,GATEWAY-GRPC
1717
EXPERIMENTAL_CONFORMANCE_PROFILES = GATEWAY-TLS
1818
CONFORMANCE_PROFILES = $(STANDARD_CONFORMANCE_PROFILES) # by default we use the standard conformance profiles. If experimental is enabled we override this and add the experimental profiles.

0 commit comments

Comments
 (0)