From f927006999d11b2415b7719d809b21074d325013 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 | 80 ++++++++++++++++++++-- internal/controller/model_nodeport_test.go | 24 +++---- 2 files changed, 85 insertions(+), 19 deletions(-) diff --git a/internal/controller/model_nodeport.go b/internal/controller/model_nodeport.go index 7151d6d..8469d71 100644 --- a/internal/controller/model_nodeport.go +++ b/internal/controller/model_nodeport.go @@ -15,6 +15,10 @@ 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" @@ -23,6 +27,14 @@ import ( "github.com/cloudandheat/ch-k8s-lbaas/internal/openstack" ) +var ( + ErrUnknownAddressFamily = errors.New("cannot determine the correct IP address family") +) + +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 +52,61 @@ func NewNodePortLoadBalancerModelGenerator( } } -func (g *NodePortLoadBalancerModelGenerator) getDestinationAddresses() (result []string, err error) { +func isValidIp(ipString string) (err error) { + ipParsed := net.ParseIP(ipString) + if ipParsed != nil { + return nil + } + return ErrInvalidIpAddress +} + +func isIPv4Address(ipString string) (isIPv4 bool, err error) { + return strings.Count(ipString, ":") < 2, isValidIp(ipString) +} + +func isIPv6Address(ipString string) (isIPv6 bool, err error) { + return strings.Count(ipString, ":") >= 2, isValidIp(ipString) +} + +func (g *NodePortLoadBalancerModelGenerator) getDestinationAddresses() (addresses_v4 []string, addresses_v6 []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) + + nodeAddressIsIPv4, err := isIPv4Address(addr.Address) + if err != nil { + return nil, nil, err + } + if nodeAddressIsIPv4 { + addresses_v4 = append(addresses_v4, addr.Address) + } + + nodeAddressIsIPv6, err := isIPv6Address(addr.Address) + if err != nil { + return nil, nil, err + } + if nodeAddressIsIPv6 { + addresses_v6 = append(addresses_v6, addr.Address) + } + + if !nodeAddressIsIPv4 && !nodeAddressIsIPv6 { + return nil, nil, ErrUnknownAddressFamily + } } } - return result, nil + return addresses_v4, addresses_v6, nil } func (g *NodePortLoadBalancerModelGenerator) GenerateModel(portAssignment map[string]string) (*model.LoadBalancer, error) { - addresses, err := g.getDestinationAddresses() + addresses_v4, addresses_v6, err := g.getDestinationAddresses() if err != nil { return nil, err } @@ -88,12 +134,32 @@ func (g *NodePortLoadBalancerModelGenerator) GenerateModel(portAssignment map[st } } + ingressIpIsIpv4, err := isIPv4Address(ingress.Address) + if err != nil { + return nil, err + } + ingressIpIsIpv6, err := isIPv6Address(ingress.Address) + if err != nil { + return nil, err + } + if !ingressIpIsIpv4 && !ingressIpIsIpv6 { + return nil, ErrUnknownAddressFamily + } + + var destAddresses []string + + if ingressIpIsIpv4 { + destAddresses = append(destAddresses, addresses_v4...) + } else if ingressIpIsIpv6 { + destAddresses = append(destAddresses, addresses_v6...) + } + 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) {