From a5d29973208500446394d67d05f94677c9943fda Mon Sep 17 00:00:00 2001 From: Christian Kruse Date: Fri, 19 Jun 2026 09:30:42 -0700 Subject: [PATCH] Fixes Controller Port Mapping Recovery Fixes flaw in port mapping recovery that prevented recovery from functioning. Also handles recent changes to tcpListener naming conventions for config entity name prefixing and multikeylisteners. Signed-off-by: Christian Kruse --- internal/kube/site/bindings_test.go | 97 +++++++++++++++++++++++++++++ internal/qdr/port_mapping.go | 21 +++++-- internal/qdr/port_mapping_test.go | 44 +++++++++++++ 3 files changed, 157 insertions(+), 5 deletions(-) create mode 100644 internal/qdr/port_mapping_test.go diff --git a/internal/kube/site/bindings_test.go b/internal/kube/site/bindings_test.go index ef75b2874..8818256e0 100644 --- a/internal/kube/site/bindings_test.go +++ b/internal/kube/site/bindings_test.go @@ -1,6 +1,8 @@ package site import ( + "log/slog" + "strconv" "testing" "github.com/skupperproject/skupper/internal/qdr" @@ -622,3 +624,98 @@ func TestBindingAdaptor_updateBridgeConfigForListener(t *testing.T) { }) } } + +func TestRecoveredPortMappingMatchesExtendedBindingKeys(t *testing.T) { + siteId := "00000000-0000-0000-0000-000000000001" + listener := &skupperv2alpha1.Listener{ + ObjectMeta: v1.ObjectMeta{ + Name: "backend", + Namespace: "test", + }, + Spec: skupperv2alpha1.ListenerSpec{ + Host: "backend", + Port: 8080, + RoutingKey: "backend", + }, + } + perTargetListener := &skupperv2alpha1.Listener{ + ObjectMeta: v1.ObjectMeta{ + Name: "backend-pods", + Namespace: "test", + }, + Spec: skupperv2alpha1.ListenerSpec{ + Port: 9090, + RoutingKey: "backend-pods", + ExposePodsByName: true, + }, + } + multiKeyListener := &skupperv2alpha1.MultiKeyListener{ + ObjectMeta: v1.ObjectMeta{ + Name: "multi", + Namespace: "test", + }, + Spec: skupperv2alpha1.MultiKeyListenerSpec{ + Host: "multi", + Port: 7070, + Strategy: skupperv2alpha1.MultiKeyListenerStrategy{ + Priority: &skupperv2alpha1.PriorityStrategySpec{ + RoutingKeys: []string{"alpha", "beta"}, + }, + }, + }, + } + network := []skupperv2alpha1.SiteRecord{ + { + Services: []skupperv2alpha1.ServiceRecord{ + { + RoutingKey: "backend-pods.pod-a", + Connectors: []string{"backend-pods"}, + }, + }, + }, + } + + generated := qdr.InitialConfig("router", "site", "test", false, 3) + allocator := &ExtendedBindings{ + context: NewMockBindingContext(nil), + mapping: qdr.RecoverPortMapping(nil), + exposed: ExposedPorts{}, + selectors: map[string]TargetSelection{}, + } + allocator.updateBridgeConfigForListener(siteId, listener, &generated.Bridges) + perTarget := newPerTargetListener(perTargetListener, slog.Default()) + _, err := perTarget.extractTargets(network, allocator.mapping, ExposedPorts{}, NewMockBindingContext(nil)) + assert.NilError(t, err) + perTarget.updateBridgeConfig(siteId, &generated.Bridges) + allocator.updateBridgeConfigForMultiKeyListener(siteId, multiKeyListener, &generated.Bridges) + + expectedListenerPort := tcpListenerPort(t, generated.Bridges, qdr.TcpListenerNamePrefix+listener.Name) + expectedPerTargetPort := tcpListenerPort(t, generated.Bridges, qdr.TcpListenerNamePrefix+perTargetListener.Name+"@pod-a") + expectedMultiKeyListenerPort := tcpListenerPort(t, generated.Bridges, "multiAddress/"+multiKeyListener.Name) + + recovered := &ExtendedBindings{ + context: NewMockBindingContext(nil), + mapping: qdr.RecoverPortMapping(&generated), + exposed: ExposedPorts{}, + selectors: map[string]TargetSelection{}, + } + recovered.ListenerUpdated(listener) + recovered.multiKeyListenerUpdated(multiKeyListener) + recoveredPerTarget := newPerTargetListener(perTargetListener, slog.Default()) + _, err = recoveredPerTarget.extractTargets(network, recovered.mapping, recovered.exposed, recovered.context) + assert.NilError(t, err) + + exposed := recovered.context.(*MockBindingContext).exposed + assert.Equal(t, exposed["backend"].Ports["backend"].TargetPort, expectedListenerPort) + assert.Equal(t, exposed["pod-a"].Ports["backend-pods"].TargetPort, expectedPerTargetPort) + assert.Equal(t, exposed["multi"].Ports["multiaddress-multi"].TargetPort, expectedMultiKeyListenerPort) +} + +func tcpListenerPort(t *testing.T, config qdr.BridgeConfig, name string) int { + t.Helper() + listener, ok := config.TcpListeners[name] + assert.Assert(t, ok, "expected tcpListener %q", name) + port, err := strconv.Atoi(listener.Port) + assert.NilError(t, err) + return port +} diff --git a/internal/qdr/port_mapping.go b/internal/qdr/port_mapping.go index 4ca85c551..3ac375679 100644 --- a/internal/qdr/port_mapping.go +++ b/internal/qdr/port_mapping.go @@ -3,6 +3,7 @@ package qdr import ( "log/slog" "strconv" + "strings" "github.com/skupperproject/skupper/internal/ports" ) @@ -41,10 +42,20 @@ func (p *PortMapping) recovered(key string, portstr string) { } p.pool.InUse(port) p.mappings[key] = port - if existing, ok := p.mappings[key]; ok { - p.pool.Release(existing) - delete(p.mappings, key) +} + +func portMappingKey(listener TcpEndpoint) string { + if strings.HasPrefix(listener.Name, TcpListenerNamePrefix) { + name := strings.TrimPrefix(listener.Name, TcpListenerNamePrefix) + if strings.Contains(name, "@") && listener.Address != "" { + return listener.Address + } + return name + } + if strings.HasPrefix(listener.Name, "multiAddress/") { + return "multiaddress-" + strings.TrimPrefix(listener.Name, "multiAddress/") } + return listener.Name } func RecoverPortMapping(config *RouterConfig) *PortMapping { @@ -58,8 +69,8 @@ func RecoverPortMapping(config *RouterConfig) *PortMapping { mapping.pool.InUse(int(listener.Port)) } - for key, listener := range config.Bridges.TcpListeners { - mapping.recovered(key, listener.Port) + for _, listener := range config.Bridges.TcpListeners { + mapping.recovered(portMappingKey(listener), listener.Port) } } return mapping diff --git a/internal/qdr/port_mapping_test.go b/internal/qdr/port_mapping_test.go new file mode 100644 index 000000000..e068f723d --- /dev/null +++ b/internal/qdr/port_mapping_test.go @@ -0,0 +1,44 @@ +package qdr + +import "testing" + +func TestRecoverPortMappingPreservesListenerPorts(t *testing.T) { + config := InitialConfig("router", "site", "test", false, 3) + config.Bridges.AddTcpListener(TcpEndpoint{ + Name: TcpListenerNamePrefix + "frontend", + Port: "12001", + Address: "frontend", + }) + config.Bridges.AddTcpListener(TcpEndpoint{ + Name: TcpListenerNamePrefix + "backend", + Port: "12002", + Address: "backend", + }) + config.Bridges.AddTcpListener(TcpEndpoint{ + Name: TcpListenerNamePrefix + "per-target@pod-a", + Port: "12003", + Address: "per-target.pod-a", + }) + config.Bridges.AddTcpListener(TcpEndpoint{ + Name: "multiAddress/multi", + Port: "12004", + }) + + mapping := RecoverPortMapping(&config) + + assertPortForKey(t, mapping, "frontend", 12001) + assertPortForKey(t, mapping, "backend", 12002) + assertPortForKey(t, mapping, "per-target.pod-a", 12003) + assertPortForKey(t, mapping, "multiaddress-multi", 12004) +} + +func assertPortForKey(t *testing.T, mapping *PortMapping, key string, expected int) { + t.Helper() + actual, err := mapping.GetPortForKey(key) + if err != nil { + t.Fatalf("GetPortForKey(%q) returned error: %v", key, err) + } + if actual != expected { + t.Fatalf("GetPortForKey(%q) = %d, want %d", key, actual, expected) + } +}