From 692ebfe9fd610a0831aaca6864ede00d57da6c19 Mon Sep 17 00:00:00 2001 From: ComradeOgilvy Date: Thu, 22 Apr 2021 12:01:07 +0200 Subject: [PATCH] Filter dest addresses based on IngressIP The host nodes of a cluster can have IPv4 and/or IPv6 addresses. The controller does not distinguish the address families, which leads to the fact that all addresses will be used as Destination Addresses independent of the preferred IPFamily of the Service or the Ingress IP. This commit adjusts getDestinationAddress to return two lists, one for each supported address family. To do so, functions to check the correctness of a textual representation of an IP and to check the address family got implemented. golang does not natively support the determination of the address family, which is why the presence of ":" and "." is checked. The GenerateModel function has been adjusted to use only IP addresses of the same address family as the IngressIP as destination addresses. Fixes #25 --- internal/controller/model_nodeport.go | 65 +++++++++++++++++++--- internal/controller/model_nodeport_test.go | 24 ++++---- 2 files changed, 70 insertions(+), 19 deletions(-) diff --git a/internal/controller/model_nodeport.go b/internal/controller/model_nodeport.go index 7151d6d..312fda0 100644 --- a/internal/controller/model_nodeport.go +++ b/internal/controller/model_nodeport.go @@ -15,14 +15,23 @@ package controller import ( + "errors" + "net" + "strings" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" corelisters "k8s.io/client-go/listers/core/v1" + "k8s.io/klog" "github.com/cloudandheat/ch-k8s-lbaas/internal/model" "github.com/cloudandheat/ch-k8s-lbaas/internal/openstack" ) +var ( + ErrInvalidIpAddress = errors.New("the string is not a valid textual representation of an IP address") +) + type NodePortLoadBalancerModelGenerator struct { l3portmanager openstack.L3PortManager services corelisters.ServiceLister @@ -40,27 +49,51 @@ func NewNodePortLoadBalancerModelGenerator( } } -func (g *NodePortLoadBalancerModelGenerator) getDestinationAddresses() (result []string, err error) { +func validateIpAddress(ipString string) error { + ipParsed := net.ParseIP(ipString) + if ipParsed != nil { + return nil + } + return ErrInvalidIpAddress +} + +func isIPv4Address(ipString string) bool { + return strings.Count(ipString, ":") == 0 && strings.Count(ipString, ".") == 3 +} + +func isIPv6Address(ipString string) bool { + return strings.Count(ipString, ":") >= 2 +} + +func (g *NodePortLoadBalancerModelGenerator) getDestinationAddresses() (addressesV4 []string, addressesV6 []string, err error) { nodes, err := g.nodes.List(labels.Everything()) if err != nil { - return nil, err + return nil, nil, err } - result = []string{} for _, node := range nodes { for _, addr := range node.Status.Addresses { if addr.Type != corev1.NodeInternalIP { continue } - result = append(result, addr.Address) + if validateIpAddress(addr.Address) != nil { + continue + } + if isIPv4Address(addr.Address) { + addressesV4 = append(addressesV4, addr.Address) + } else if isIPv6Address(addr.Address) { + addressesV6 = append(addressesV6, addr.Address) + } else { + continue + } } } - return result, nil + return addressesV4, addressesV6, nil } func (g *NodePortLoadBalancerModelGenerator) GenerateModel(portAssignment map[string]string) (*model.LoadBalancer, error) { - addresses, err := g.getDestinationAddresses() + addressesV4, addressesV6, err := g.getDestinationAddresses() if err != nil { return nil, err } @@ -88,12 +121,30 @@ func (g *NodePortLoadBalancerModelGenerator) GenerateModel(portAssignment map[st } } + if validateIpAddress(ingress.Address) != nil { + continue + } + + var destAddresses []string + + if isIPv4Address(ingress.Address) { + destAddresses = append(destAddresses, addressesV4...) + } else if isIPv6Address(ingress.Address) { + destAddresses = append(destAddresses, addressesV6...) + } else { + klog.Warningf( + "could not determine address family of ingress IP %q for service %q", + ingress.Address, + svc.Name) + continue + } + for _, svcPort := range svc.Spec.Ports { ingress.Ports = append(ingress.Ports, model.PortForward{ Protocol: svcPort.Protocol, InboundPort: svcPort.Port, DestinationPort: svcPort.NodePort, - DestinationAddresses: addresses, + DestinationAddresses: destAddresses, }) } diff --git a/internal/controller/model_nodeport_test.go b/internal/controller/model_nodeport_test.go index a97c82b..47098d6 100644 --- a/internal/controller/model_nodeport_test.go +++ b/internal/controller/model_nodeport_test.go @@ -179,7 +179,7 @@ func TestNodePortSinglePortSingleServiceAssignment(t *testing.T) { model.FromService(svc).ToKey(): "port-id-1", } - f.l3portmanager.On("GetInternalAddress", "port-id-1").Return("ingress-ip-1", nil).Times(1) + f.l3portmanager.On("GetInternalAddress", "port-id-1").Return("10.0.0.2", nil).Times(1) f.runWith(func(g *NodePortLoadBalancerModelGenerator) { m, err := g.GenerateModel(a) @@ -187,7 +187,7 @@ func TestNodePortSinglePortSingleServiceAssignment(t *testing.T) { assert.NotNil(t, m) assert.Equal(t, 1, len(m.Ingress)) - anyIngressIP(t, m.Ingress, "ingress-ip-1", func(t *testing.T, i model.IngressIP) { + anyIngressIP(t, m.Ingress, "10.0.0.2", func(t *testing.T, i model.IngressIP) { assert.Equal(t, 1, len(i.Ports)) anyPort(t, i.Ports, 80, corev1.ProtocolTCP, func(t *testing.T, p model.PortForward) { @@ -220,7 +220,7 @@ func TestNodePortSinglePortMultiServiceAssignment(t *testing.T) { model.FromService(svc2).ToKey(): "port-id-1", } - f.l3portmanager.On("GetInternalAddress", "port-id-1").Return("ingress-ip-1", nil).Times(1) + f.l3portmanager.On("GetInternalAddress", "port-id-1").Return("10.0.0.2", nil).Times(1) f.runWith(func(g *NodePortLoadBalancerModelGenerator) { m, err := g.GenerateModel(a) @@ -228,7 +228,7 @@ func TestNodePortSinglePortMultiServiceAssignment(t *testing.T) { assert.NotNil(t, m) assert.Equal(t, 1, len(m.Ingress)) - anyIngressIP(t, m.Ingress, "ingress-ip-1", func(t *testing.T, i model.IngressIP) { + anyIngressIP(t, m.Ingress, "10.0.0.2", func(t *testing.T, i model.IngressIP) { assert.Equal(t, 3, len(i.Ports)) anyPort(t, i.Ports, 53, corev1.ProtocolUDP, func(t *testing.T, p model.PortForward) { @@ -273,8 +273,8 @@ func TestNodePortMultiPortSingleServiceAssignment(t *testing.T) { model.FromService(svc2).ToKey(): "port-id-2", } - f.l3portmanager.On("GetInternalAddress", "port-id-1").Return("ingress-ip-1", nil).Times(1) - f.l3portmanager.On("GetInternalAddress", "port-id-2").Return("ingress-ip-2", nil).Times(1) + f.l3portmanager.On("GetInternalAddress", "port-id-1").Return("10.0.0.2", nil).Times(1) + f.l3portmanager.On("GetInternalAddress", "port-id-2").Return("10.0.0.3", nil).Times(1) f.runWith(func(g *NodePortLoadBalancerModelGenerator) { m, err := g.GenerateModel(a) @@ -282,7 +282,7 @@ func TestNodePortMultiPortSingleServiceAssignment(t *testing.T) { assert.NotNil(t, m) assert.Equal(t, 2, len(m.Ingress)) - anyIngressIP(t, m.Ingress, "ingress-ip-1", func(t *testing.T, i model.IngressIP) { + anyIngressIP(t, m.Ingress, "10.0.0.2", func(t *testing.T, i model.IngressIP) { assert.Equal(t, 2, len(i.Ports)) anyPort(t, i.Ports, 80, corev1.ProtocolTCP, func(t *testing.T, p model.PortForward) { @@ -298,7 +298,7 @@ func TestNodePortMultiPortSingleServiceAssignment(t *testing.T) { }) }) - anyIngressIP(t, m.Ingress, "ingress-ip-2", func(t *testing.T, i model.IngressIP) { + anyIngressIP(t, m.Ingress, "10.0.0.3", func(t *testing.T, i model.IngressIP) { assert.Equal(t, 1, len(i.Ports)) anyPort(t, i.Ports, 53, corev1.ProtocolUDP, func(t *testing.T, p model.PortForward) { @@ -339,8 +339,8 @@ func TestNodePortMultiPortMultiServiceAssignment(t *testing.T) { model.FromService(svc3).ToKey(): "port-id-2", } - f.l3portmanager.On("GetInternalAddress", "port-id-1").Return("ingress-ip-1", nil).Times(1) - f.l3portmanager.On("GetInternalAddress", "port-id-2").Return("ingress-ip-2", nil).Times(1) + f.l3portmanager.On("GetInternalAddress", "port-id-1").Return("10.0.0.2", nil).Times(1) + f.l3portmanager.On("GetInternalAddress", "port-id-2").Return("10.0.0.3", nil).Times(1) f.runWith(func(g *NodePortLoadBalancerModelGenerator) { m, err := g.GenerateModel(a) @@ -348,7 +348,7 @@ func TestNodePortMultiPortMultiServiceAssignment(t *testing.T) { assert.NotNil(t, m) assert.Equal(t, 2, len(m.Ingress)) - anyIngressIP(t, m.Ingress, "ingress-ip-1", func(t *testing.T, i model.IngressIP) { + anyIngressIP(t, m.Ingress, "10.0.0.2", func(t *testing.T, i model.IngressIP) { assert.Equal(t, 2, len(i.Ports)) anyPort(t, i.Ports, 80, corev1.ProtocolTCP, func(t *testing.T, p model.PortForward) { @@ -364,7 +364,7 @@ func TestNodePortMultiPortMultiServiceAssignment(t *testing.T) { }) }) - anyIngressIP(t, m.Ingress, "ingress-ip-2", func(t *testing.T, i model.IngressIP) { + anyIngressIP(t, m.Ingress, "10.0.0.3", func(t *testing.T, i model.IngressIP) { assert.Equal(t, 3, len(i.Ports)) anyPort(t, i.Ports, 53, corev1.ProtocolUDP, func(t *testing.T, p model.PortForward) {