From 9ba88450e9a2fb540576be86988e4d179483e39a Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Fri, 14 Jul 2023 18:31:03 +0530 Subject: [PATCH] fix-extra-port-mapping Signed-off-by: Daman Arora --- pkg/apis/config/defaults/networking.go | 6 ++ pkg/apis/config/v1alpha4/default.go | 27 +++++-- pkg/internal/apis/config/default.go | 25 +++++-- pkg/internal/apis/config/validate.go | 10 ++- pkg/internal/apis/config/validate_test.go | 91 +++++++++++++++-------- 5 files changed, 116 insertions(+), 43 deletions(-) diff --git a/pkg/apis/config/defaults/networking.go b/pkg/apis/config/defaults/networking.go index 1b7445d8bd..5726593eab 100644 --- a/pkg/apis/config/defaults/networking.go +++ b/pkg/apis/config/defaults/networking.go @@ -47,3 +47,9 @@ const ServiceSubnetIPv6 = "fd00:10:96::/112" // ServiceSubnetDualStack is the default DualStack subnet for the Networking.ServiceSubnet field const ServiceSubnetDualStack = "10.96.0.0/16,fd00:10:96::/112" + +// ExtraPortMappingListenAddressIPv4 is the default IPv4/DualStack listen address for ExtraPortMappings +const ExtraPortMappingListenAddressIPv4 = "0.0.0.0" + +// ExtraPortMappingListenAddressIPv6 is the default IPv6 listen address for ExtraPortMappings +const ExtraPortMappingListenAddressIPv6 = "::" diff --git a/pkg/apis/config/v1alpha4/default.go b/pkg/apis/config/v1alpha4/default.go index 976e710dc1..00cd224df3 100644 --- a/pkg/apis/config/v1alpha4/default.go +++ b/pkg/apis/config/v1alpha4/default.go @@ -31,14 +31,17 @@ func SetDefaultsCluster(obj *Cluster) { }, } } + + if obj.Networking.IPFamily == "" { + obj.Networking.IPFamily = IPv4Family + } + // default the nodes for i := range obj.Nodes { a := &obj.Nodes[i] - SetDefaultsNode(a) - } - if obj.Networking.IPFamily == "" { - obj.Networking.IPFamily = IPv4Family + SetDefaultsNode(a, obj.Networking.IPFamily) } + // default the API server address if obj.Networking.APIServerAddress == "" { obj.Networking.APIServerAddress = defaults.APIServerAddressIPv4 @@ -75,7 +78,7 @@ func SetDefaultsCluster(obj *Cluster) { } // SetDefaultsNode sets uninitialized fields to their default value. -func SetDefaultsNode(obj *Node) { +func SetDefaultsNode(obj *Node, ipFamily ClusterIPFamily) { if obj.Image == "" { obj.Image = defaults.Image } @@ -83,4 +86,18 @@ func SetDefaultsNode(obj *Node) { if obj.Role == "" { obj.Role = ControlPlaneRole } + + for i := 0; i < len(obj.ExtraPortMappings); i++ { + if obj.ExtraPortMappings[i].ListenAddress == "" { + if ipFamily == IPv6Family { + obj.ExtraPortMappings[i].ListenAddress = defaults.ExtraPortMappingListenAddressIPv6 + } else { + obj.ExtraPortMappings[i].ListenAddress = defaults.ExtraPortMappingListenAddressIPv4 + } + } + + if string(obj.ExtraPortMappings[i].Protocol) == "" { + obj.ExtraPortMappings[i].Protocol = PortMappingProtocolTCP + } + } } diff --git a/pkg/internal/apis/config/default.go b/pkg/internal/apis/config/default.go index 9a54072316..0dbaa81c0b 100644 --- a/pkg/internal/apis/config/default.go +++ b/pkg/internal/apis/config/default.go @@ -42,13 +42,14 @@ func SetDefaultsCluster(obj *Cluster) { } } + if obj.Networking.IPFamily == "" { + obj.Networking.IPFamily = IPv4Family + } + // default nodes for i := range obj.Nodes { a := &obj.Nodes[i] - SetDefaultsNode(a) - } - if obj.Networking.IPFamily == "" { - obj.Networking.IPFamily = IPv4Family + SetDefaultsNode(a, obj.Networking.IPFamily) } // default the API server address @@ -88,7 +89,7 @@ func SetDefaultsCluster(obj *Cluster) { } // SetDefaultsNode sets uninitialized fields to their default value. -func SetDefaultsNode(obj *Node) { +func SetDefaultsNode(obj *Node, ipFamily ClusterIPFamily) { if obj.Image == "" { obj.Image = defaults.Image } @@ -96,4 +97,18 @@ func SetDefaultsNode(obj *Node) { if obj.Role == "" { obj.Role = ControlPlaneRole } + + for i := 0; i < len(obj.ExtraPortMappings); i++ { + if obj.ExtraPortMappings[i].ListenAddress == "" { + if ipFamily == IPv6Family { + obj.ExtraPortMappings[i].ListenAddress = defaults.ExtraPortMappingListenAddressIPv6 + } else { + obj.ExtraPortMappings[i].ListenAddress = defaults.ExtraPortMappingListenAddressIPv4 + } + } + + if string(obj.ExtraPortMappings[i].Protocol) == "" { + obj.ExtraPortMappings[i].Protocol = PortMappingProtocolTCP + } + } } diff --git a/pkg/internal/apis/config/validate.go b/pkg/internal/apis/config/validate.go index 68185d1579..e082632f4e 100644 --- a/pkg/internal/apis/config/validate.go +++ b/pkg/internal/apis/config/validate.go @@ -20,6 +20,7 @@ import ( "fmt" "net" "regexp" + "strconv" "strings" "sigs.k8s.io/kind/pkg/errors" @@ -137,7 +138,7 @@ func (n *Node) Validate() error { } func validatePortMappings(portMappings []PortMapping) error { - errMsg := "port mapping with same listen address, port and protocol already configured" + errMsg := "port mapping with same listen address, host port and protocol already configured" wildcardAddrIPv4 := net.ParseIP("0.0.0.0") wildcardAddrIPv6 := net.ParseIP("::") @@ -152,11 +153,16 @@ func validatePortMappings(portMappings []PortMapping) error { } for _, portMapping := range portMappings { + // skipping validation if host port is not defined + if portMapping.HostPort == 0 { + continue + } + addr := net.ParseIP(portMapping.ListenAddress) addrString := addr.String() portProtocol := formatPortProtocol(portMapping.HostPort, portMapping.Protocol) - possibleErr := fmt.Errorf("%s: %s:%s", errMsg, addrString, portProtocol) + possibleErr := fmt.Errorf("%s: %s/%s", errMsg, net.JoinHostPort(addrString, strconv.Itoa(int(portMapping.HostPort))), portMapping.Protocol) // in golang 0.0.0.0 and [::] are equivalent, convert [::] -> 0.0.0.0 // https://github.com/golang/go/issues/48723 diff --git a/pkg/internal/apis/config/validate_test.go b/pkg/internal/apis/config/validate_test.go index 6d4ef54a0e..41dd2353e0 100644 --- a/pkg/internal/apis/config/validate_test.go +++ b/pkg/internal/apis/config/validate_test.go @@ -18,7 +18,6 @@ package config import ( "fmt" - "sigs.k8s.io/kind/pkg/internal/assert" "testing" "sigs.k8s.io/kind/pkg/errors" @@ -432,45 +431,66 @@ func TestPortValidate(t *testing.T) { } func TestValidatePortMappings(t *testing.T) { - newPortMapping := func(addr string, port int, protocol string) PortMapping { + newPortMapping := func(addr string, hostPort, containerPort int, protocol string) PortMapping { return PortMapping{ - HostPort: int32(port), + HostPort: int32(hostPort), + ContainerPort: int32(containerPort), ListenAddress: addr, Protocol: PortMappingProtocol(protocol), } } - errMsg := "port mapping with same listen address, port and protocol already configured" + errMsg := "port mapping with same listen address, host port and protocol already configured" cases := []struct { testName string portMappings []PortMapping expectErr string }{ + { + testName: "unique container ports", + portMappings: []PortMapping{ + newPortMapping("", 0, 1000, ""), + newPortMapping("", 0, 2000, ""), + newPortMapping("", 0, 3000, ""), + newPortMapping("", 0, 4000, ""), + }, + expectErr: "", + }, + { + testName: "duplicate container ports", + portMappings: []PortMapping{ + newPortMapping("", 0, 1000, ""), + newPortMapping("", 0, 2000, ""), + newPortMapping("", 0, 3000, ""), + newPortMapping("", 0, 3000, ""), + }, + expectErr: "", + }, { testName: "unique port mappings ipv4", portMappings: []PortMapping{ - newPortMapping("127.0.0.1", 80, "UDP"), - newPortMapping("127.0.0.1", 80, "TCP"), - newPortMapping("0.0.0.0", 3000, "UDP"), - newPortMapping("0.0.0.0", 5000, "TCP"), + newPortMapping("127.0.0.1", 80, 5000, "UDP"), + newPortMapping("127.0.0.1", 80, 5000, "TCP"), + newPortMapping("0.0.0.0", 3000, 8000, "UDP"), + newPortMapping("0.0.0.0", 5000, 8000, "TCP"), }, expectErr: "", }, { testName: "unique port mappings ipv6", portMappings: []PortMapping{ - newPortMapping("::1", 80, "UDP"), - newPortMapping("::1", 80, "TCP"), - newPortMapping("1e3d:6e85:424d:a011:a72e:9780:5f6f:a6fc", 3000, "UDP"), - newPortMapping("6516:944d:e070:a1d1:2e91:8437:a6b3:edf9", 5000, "TCP"), + newPortMapping("::1", 80, 5000, "UDP"), + newPortMapping("::1", 80, 5000, "TCP"), + newPortMapping("1e3d:6e85:424d:a011:a72e:9780:5f6f:a6fc", 3000, 8000, "UDP"), + newPortMapping("6516:944d:e070:a1d1:2e91:8437:a6b3:edf9", 5000, 8000, "TCP"), }, expectErr: "", }, { testName: "exact duplicate port mappings ipv4", portMappings: []PortMapping{ - newPortMapping("127.0.0.1", 80, "TCP"), - newPortMapping("127.0.0.1", 80, "UDP"), - newPortMapping("127.0.0.1", 80, "TCP"), + newPortMapping("127.0.0.1", 80, 3000, "TCP"), + newPortMapping("127.0.0.1", 80, 5000, "UDP"), + newPortMapping("127.0.0.1", 80, 8000, "TCP"), }, // error expected: exact duplicate expectErr: fmt.Sprintf("%s: 127.0.0.1:80/TCP", errMsg), @@ -479,9 +499,9 @@ func TestValidatePortMappings(t *testing.T) { { testName: "exact duplicate port mappings ipv6", portMappings: []PortMapping{ - newPortMapping("::1", 80, "TCP"), - newPortMapping("::1", 80, "UDP"), - newPortMapping("::1", 80, "TCP"), + newPortMapping("::1", 80, 3000, "TCP"), + newPortMapping("::1", 80, 5000, "UDP"), + newPortMapping("::1", 80, 8000, "TCP"), }, // error expected: exact duplicate expectErr: fmt.Sprintf("%s: [::1]:80/TCP", errMsg), @@ -489,10 +509,10 @@ func TestValidatePortMappings(t *testing.T) { { testName: "wildcard ipv4 & ipv6", portMappings: []PortMapping{ - newPortMapping("127.0.0.1", 80, "TCP"), - newPortMapping("0.0.0.0", 80, "UDP"), - newPortMapping("::1", 80, "TCP"), - newPortMapping("::", 80, "UDP"), + newPortMapping("127.0.0.1", 80, 3000, "TCP"), + newPortMapping("0.0.0.0", 80, 5000, "UDP"), + newPortMapping("::1", 80, 3000, "TCP"), + newPortMapping("::", 80, 5000, "UDP"), }, // error expected: 0.0.0.0 & [::] are same in golang // https://github.com/golang/go/issues/48723 @@ -501,8 +521,8 @@ func TestValidatePortMappings(t *testing.T) { { testName: "subset already configured ipv4", portMappings: []PortMapping{ - newPortMapping("127.0.0.1", 80, "TCP"), - newPortMapping("0.0.0.0", 80, "TCP"), + newPortMapping("127.0.0.1", 80, 3000, "TCP"), + newPortMapping("0.0.0.0", 80, 5000, "TCP"), }, // error expected: subset of 0.0.0.0 -> 127.0.0.1 is already defined for same port and protocol expectErr: fmt.Sprintf("%s: 0.0.0.0:80/TCP", errMsg), @@ -510,8 +530,8 @@ func TestValidatePortMappings(t *testing.T) { { testName: "subset already configured ipv6", portMappings: []PortMapping{ - newPortMapping("::1", 80, "TCP"), - newPortMapping("::", 80, "TCP"), + newPortMapping("::1", 80, 3000, "TCP"), + newPortMapping("::", 80, 5000, "TCP"), }, // error expected: subset of :: -> ::1 is already defined for same port and protocol expectErr: fmt.Sprintf("%s: [::]:80/TCP", errMsg), @@ -519,8 +539,8 @@ func TestValidatePortMappings(t *testing.T) { { testName: "port mapping already configured via wildcard ipv4", portMappings: []PortMapping{ - newPortMapping("0.0.0.0", 80, "TCP"), - newPortMapping("127.0.0.1", 80, "TCP"), + newPortMapping("0.0.0.0", 80, 5000, "TCP"), + newPortMapping("127.0.0.1", 80, 5000, "TCP"), }, // error expected: port mapping is already defined for wildcard interface - 0.0.0.0 expectErr: fmt.Sprintf("%s: 127.0.0.1:80/TCP", errMsg), @@ -528,8 +548,8 @@ func TestValidatePortMappings(t *testing.T) { { testName: "port mapping already configured via wildcard ipv6", portMappings: []PortMapping{ - newPortMapping("::", 80, "SCTP"), - newPortMapping("::1", 80, "SCTP"), + newPortMapping("::", 80, 5000, "SCTP"), + newPortMapping("::1", 80, 5000, "SCTP"), }, // error expected: port mapping is already defined for wildcard interface - :: expectErr: fmt.Sprintf("%s: [::1]:80/SCTP", errMsg), @@ -542,7 +562,16 @@ func TestValidatePortMappings(t *testing.T) { t.Parallel() err := validatePortMappings(tc.portMappings) - assert.ExpectError(t, len(tc.expectErr) > 0, err) + + // the error can be: + // - nil, in which case we should expect no errors or fail + if err == nil && len(tc.expectErr) > 0 { + t.Errorf("Test failed, unexpected error: %s", tc.expectErr) + } + + if err != nil && err.Error() != tc.expectErr { + t.Errorf("Test failed, error: %s expected error: %s", err, tc.expectErr) + } }) } }