Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit 86841e2

Browse files
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)

AltStyle によって変換されたページ (->オリジナル) /