Skip to content

Commit

Permalink
Update GetNodeAddressesFromNodeIP and ParseNodeIPAnnotation for cloud…
Browse files Browse the repository at this point in the history 
… dual-stack

Update callers for the new APIs, but don't change their behavior yet.
  • Loading branch information
@danwinship
danwinship committed Mar 15, 2023
1 parent d6a11b7 commit e3aacb4
Show file tree
Hide file tree
Showing 5 changed files with 213 additions and 56 deletions.
2 changes: 1 addition & 1 deletion staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
42 changes: 25 additions & 17 deletions staging/src/k8s.io/cloud-provider/node/helpers/address.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 len(enforcedNodeAddresses) == 0 {
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 !matched {
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})
Expand Down
122 changes: 121 additions & 1 deletion staging/src/k8s.io/cloud-provider/node/helpers/address_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 4 additions & 7 deletions staging/src/k8s.io/component-helpers/node/util/ips.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
92 changes: 62 additions & 30 deletions staging/src/k8s.io/component-helpers/node/util/ips_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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)

expectedOut := tc.out
expectedErr := tc.err

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)
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)
}
})
}
}
}

0 comments on commit e3aacb4

Please sign in to comment.