diff --git a/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go index c9aea6ff50c..24d45452c44 100644 --- a/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go +++ b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go @@ -739,7 +739,7 @@ func updateNodeAddressesFromNodeIP(node *v1.Node, nodeAddresses []v1.NodeAddress providedNodeIP, exists := node.ObjectMeta.Annotations[cloudproviderapi.AnnotationAlphaProvidedIPAddr] if exists { - nodeAddresses, err = cloudnodeutil.GetNodeAddressesFromNodeIP(providedNodeIP, nodeAddresses) + nodeAddresses, err = cloudnodeutil.GetNodeAddressesFromNodeIP(providedNodeIP, nodeAddresses, false) } return nodeAddresses, err diff --git a/staging/src/k8s.io/cloud-provider/node/helpers/address.go b/staging/src/k8s.io/cloud-provider/node/helpers/address.go index 627167799aa..41112cdaeb3 100644 --- a/staging/src/k8s.io/cloud-provider/node/helpers/address.go +++ b/staging/src/k8s.io/cloud-provider/node/helpers/address.go @@ -85,7 +85,7 @@ func GetNodeAddressesFromNodeIPLegacy(nodeIP net.IP, cloudNodeAddresses []v1.Nod } // Otherwise the result is the same as for GetNodeAddressesFromNodeIP - return GetNodeAddressesFromNodeIP(nodeIP.String(), cloudNodeAddresses) + return GetNodeAddressesFromNodeIP(nodeIP.String(), cloudNodeAddresses, false) } // GetNodeAddressesFromNodeIP filters the provided list of nodeAddresses to match the @@ -102,32 +102,40 @@ func GetNodeAddressesFromNodeIPLegacy(nodeIP net.IP, cloudNodeAddresses []v1.Nod // GetNodeAddressesFromNodeIPLegacy, because that case never occurs for external cloud // providers, because kubelet does not set the `provided-node-ip` annotation in that // case.) -func GetNodeAddressesFromNodeIP(providedNodeIP string, cloudNodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) { - nodeIP, err := nodeutil.ParseNodeIPAnnotation(providedNodeIP) +func GetNodeAddressesFromNodeIP(providedNodeIP string, cloudNodeAddresses []v1.NodeAddress, allowDualStack bool) ([]v1.NodeAddress, error) { + nodeIPs, err := nodeutil.ParseNodeIPAnnotation(providedNodeIP, allowDualStack) if err != nil { return nil, fmt.Errorf("failed to parse node IP %q: %v", providedNodeIP, err) } - // For every address supplied by the cloud provider that matches nodeIP, nodeIP is the enforced node address for - // that address Type (like InternalIP and ExternalIP), meaning other addresses of the same Type are discarded. - // See #61921 for more information: some cloud providers may supply secondary IPs, so nodeIP serves as a way to - // ensure that the correct IPs show up on a Node object. enforcedNodeAddresses := []v1.NodeAddress{} - nodeIPTypes := make(map[v1.NodeAddressType]bool) - for _, nodeAddress := range cloudNodeAddresses { - if netutils.ParseIPSloppy(nodeAddress.Address).Equal(nodeIP) { - enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address}) - nodeIPTypes[nodeAddress.Type] = true + + for _, nodeIP := range nodeIPs { + // For every address supplied by the cloud provider that matches nodeIP, + // nodeIP is the enforced node address for that address Type (like + // InternalIP and ExternalIP), meaning other addresses of the same Type + // are discarded. See #61921 for more information: some cloud providers + // may supply secondary IPs, so nodeIP serves as a way to ensure that the + // correct IPs show up on a Node object. + + matched := false + for _, nodeAddress := range cloudNodeAddresses { + if netutils.ParseIPSloppy(nodeAddress.Address).Equal(nodeIP) { + enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address}) + nodeIPTypes[nodeAddress.Type] = true + matched = true + } + } + + // nodeIP must be among the addresses supplied by the cloud provider + if !matched { + return nil, fmt.Errorf("failed to get node address from cloud provider that matches ip: %v", nodeIP) } } - // nodeIP must be among the addresses supplied by the cloud provider - if len(enforcedNodeAddresses) == 0 { - return nil, fmt.Errorf("failed to get node address from cloud provider that matches ip: %v", nodeIP) - } - - // nodeIP was found, now use all other addresses supplied by the cloud provider NOT of the same Type as nodeIP. + // Now use all other addresses supplied by the cloud provider NOT of the same Type + // as any nodeIP. for _, nodeAddress := range cloudNodeAddresses { if !nodeIPTypes[nodeAddress.Type] { enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address}) diff --git a/staging/src/k8s.io/cloud-provider/node/helpers/address_test.go b/staging/src/k8s.io/cloud-provider/node/helpers/address_test.go index 6007bedf206..cf208df1a28 100644 --- a/staging/src/k8s.io/cloud-provider/node/helpers/address_test.go +++ b/staging/src/k8s.io/cloud-provider/node/helpers/address_test.go @@ -428,11 +428,131 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) { }, shouldError: false, }, + { + name: "Single-stack cloud, dual-stack request", + nodeIP: "10.1.1.1,fc01:1234::5678", + nodeAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: true, + }, + { + name: "Dual-stack cloud, IPv4 first, IPv4-primary request", + nodeIP: "10.1.1.1,fc01:1234::5678", + nodeAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "Dual-stack cloud, IPv6 first, IPv4-primary request", + nodeIP: "10.1.1.1,fc01:1234::5678", + nodeAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "Dual-stack cloud, dual-stack request, multiple IPs", + nodeIP: "10.1.1.1,fc01:1234::5678", + nodeAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeInternalIP, Address: "10.1.1.2"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::1234"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + // additional IPs of the same type are removed, as in the + // single-stack case. + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "Dual-stack cloud, dual-stack request, extra ExternalIP", + nodeIP: "10.1.1.1,fc01:1234::5678", + nodeAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::1234"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + // The ExternalIP is preserved, since no ExternalIP was matched + // by --node-ip. + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "Dual-stack cloud, dual-stack request, multiple ExternalIPs", + nodeIP: "fc01:1234::5678,10.1.1.1", + nodeAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeExternalIP, Address: "2001:db1::1"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + // The ExternalIPs are preserved, since no ExternalIP was matched + // by --node-ip. + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, + {Type: v1.NodeExternalIP, Address: "2001:db1::1"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "Dual-stack cloud, dual-stack request, mixed InternalIP/ExternalIP match", + nodeIP: "55.55.55.55,fc01:1234::5678", + nodeAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, + {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeExternalIP, Address: "2001:db1::1"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + // Since the IPv4 --node-ip value matched an ExternalIP, that + // filters out the IPv6 ExternalIP. Since the IPv6 --node-ip value + // matched in InternalIP, that filters out the IPv4 InternalIP + // value. + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, + {Type: v1.NodeInternalIP, Address: "fc01:1234::5678"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, } for _, tt := range cases { t.Run(tt.name, func(t *testing.T) { - got, err := GetNodeAddressesFromNodeIP(tt.nodeIP, tt.nodeAddresses) + got, err := GetNodeAddressesFromNodeIP(tt.nodeIP, tt.nodeAddresses, true) if (err != nil) != tt.shouldError { t.Errorf("GetNodeAddressesFromNodeIP() error = %v, wantErr %v", err, tt.shouldError) return diff --git a/staging/src/k8s.io/component-helpers/node/util/ips.go b/staging/src/k8s.io/component-helpers/node/util/ips.go index 60d3097cc88..ff306a3dc3c 100644 --- a/staging/src/k8s.io/component-helpers/node/util/ips.go +++ b/staging/src/k8s.io/component-helpers/node/util/ips.go @@ -74,12 +74,9 @@ func ParseNodeIPArgument(nodeIP, cloudProvider string, allowCloudDualStack bool) } // ParseNodeIPAnnotation parses the `alpha.kubernetes.io/provided-node-ip` annotation, -// which should be a single IP address. Unlike with ParseNodeIPArgument, invalid values +// which can be either a single IP address or (if allowDualStack is true) a +// comma-separated pair of IP addresses. Unlike with ParseNodeIPArgument, invalid values // are considered an error. -func ParseNodeIPAnnotation(nodeIP string) (net.IP, error) { - ips, err := parseNodeIP(nodeIP, false, false) - if err != nil || len(ips) == 0 { - return nil, err - } - return ips[0], nil +func ParseNodeIPAnnotation(nodeIP string, allowDualStack bool) ([]net.IP, error) { + return parseNodeIP(nodeIP, allowDualStack, false) } diff --git a/staging/src/k8s.io/component-helpers/node/util/ips_test.go b/staging/src/k8s.io/component-helpers/node/util/ips_test.go index 8afb03f5cf2..0d3586ef582 100644 --- a/staging/src/k8s.io/component-helpers/node/util/ips_test.go +++ b/staging/src/k8s.io/component-helpers/node/util/ips_test.go @@ -214,10 +214,11 @@ func TestParseNodeIPArgument(t *testing.T) { func TestParseNodeIPAnnotation(t *testing.T) { testCases := []struct { - desc string - in string - out net.IP - err string + desc string + in string + out []net.IP + err string + ssErr string }{ { desc: "empty --node-ip", @@ -237,7 +238,9 @@ func TestParseNodeIPAnnotation(t *testing.T) { { desc: "single IPv4", in: "1.2.3.4", - out: netutils.ParseIPSloppy("1.2.3.4"), + out: []net.IP{ + netutils.ParseIPSloppy("1.2.3.4"), + }, }, { desc: "single IPv4 with whitespace", @@ -247,7 +250,9 @@ func TestParseNodeIPAnnotation(t *testing.T) { { desc: "single IPv4 non-canonical", in: "01.2.3.004", - out: netutils.ParseIPSloppy("1.2.3.4"), + out: []net.IP{ + netutils.ParseIPSloppy("1.2.3.4"), + }, }, { desc: "single IPv4 invalid", @@ -262,7 +267,9 @@ func TestParseNodeIPAnnotation(t *testing.T) { { desc: "single IPv4 unspecified", in: "0.0.0.0", - out: net.IPv4zero, + out: []net.IP{ + net.IPv4zero, + }, }, { desc: "single IPv4 plus garbage", @@ -272,17 +279,25 @@ func TestParseNodeIPAnnotation(t *testing.T) { { desc: "single IPv6", in: "abcd::ef01", - out: netutils.ParseIPSloppy("abcd::ef01"), + out: []net.IP{ + netutils.ParseIPSloppy("abcd::ef01"), + }, }, { desc: "single IPv6 non-canonical", in: "abcd:0abc:00ab:0000:0000::1", - out: netutils.ParseIPSloppy("abcd:abc:ab::1"), + out: []net.IP{ + netutils.ParseIPSloppy("abcd:abc:ab::1"), + }, }, { desc: "simple dual-stack", in: "1.2.3.4,abcd::ef01", - err: "not supported in this configuration", + out: []net.IP{ + netutils.ParseIPSloppy("1.2.3.4"), + netutils.ParseIPSloppy("abcd::ef01"), + }, + ssErr: "not supported in this configuration", }, { desc: "dual-stack with whitespace", @@ -300,14 +315,16 @@ func TestParseNodeIPAnnotation(t *testing.T) { err: "either a single IP or a dual-stack pair of IPs", }, { - desc: "dual-stack with unspecified", - in: "1.2.3.4,::", - err: "not supported in this configuration", + desc: "dual-stack with unspecified", + in: "1.2.3.4,::", + err: "cannot include '0.0.0.0' or '::'", + ssErr: "not supported in this configuration", }, { - desc: "dual-stack with unspecified", - in: "0.0.0.0,abcd::1", - err: "not supported in this configuration", + desc: "dual-stack with unspecified", + in: "0.0.0.0,abcd::1", + err: "cannot include '0.0.0.0' or '::'", + ssErr: "not supported in this configuration", }, { desc: "dual-stack plus garbage", @@ -322,21 +339,36 @@ func TestParseNodeIPAnnotation(t *testing.T) { } for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - parsed, err := ParseNodeIPAnnotation(tc.in) + for _, allowDualStack := range []bool{false, true} { + desc := fmt.Sprintf("%s, allowDualStack=%v", tc.desc, allowDualStack) + t.Run(desc, func(t *testing.T) { + parsed, err := ParseNodeIPAnnotation(tc.in, allowDualStack) - if !reflect.DeepEqual(parsed, tc.out) { - t.Errorf("expected %#v, got %#v", tc.out, parsed) - } - if err != nil { - if tc.err == "" { - t.Errorf("unexpected error %v", err) - } else if !strings.Contains(err.Error(), tc.err) { - t.Errorf("expected error with %q, got %v", tc.err, err) + expectedOut := tc.out + expectedErr := tc.err + + if !allowDualStack { + if len(tc.out) == 2 { + expectedOut = nil + } + if tc.ssErr != "" { + expectedErr = tc.ssErr + } } - } else if tc.err != "" { - t.Errorf("expected error with %q, got no error", tc.err) - } - }) + + if !reflect.DeepEqual(parsed, expectedOut) { + t.Errorf("expected %#v, got %#v", expectedOut, parsed) + } + if err != nil { + if expectedErr == "" { + t.Errorf("unexpected error %v", err) + } else if !strings.Contains(err.Error(), expectedErr) { + t.Errorf("expected error with %q, got %v", expectedErr, err) + } + } else if expectedErr != "" { + t.Errorf("expected error with %q, got no error", expectedErr) + } + }) + } } }