From d1d9b3661d321eb7025b0ec1099a8f7145df437d Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Mon, 3 Jun 2024 17:43:25 +0000 Subject: [PATCH 1/3] ccm integration test for node status addresses and provided-node-ip annotation --- test/integration/cloudprovider/ccm_test.go | 172 ++++++++++++++++++++- 1 file changed, 170 insertions(+), 2 deletions(-) diff --git a/test/integration/cloudprovider/ccm_test.go b/test/integration/cloudprovider/ccm_test.go index fc373094b50..dfc33263dee 100644 --- a/test/integration/cloudprovider/ccm_test.go +++ b/test/integration/cloudprovider/ccm_test.go @@ -18,8 +18,11 @@ package cloudprovider import ( "context" + "fmt" "io" "os" + "reflect" + "strings" "testing" "time" @@ -64,7 +67,7 @@ func Test_RemoveExternalCloudProviderTaint(t *testing.T) { defer os.Remove(kubeconfig) args := []string{ "--kubeconfig=" + kubeconfig, - "--cloud-provider=fakeCloud", + "--cloud-provider=fakeCloudTaints", "--cidr-allocator-type=" + string(ipam.RangeAllocatorType), "--configure-cloud-routes=false", } @@ -94,6 +97,10 @@ func Test_RemoveExternalCloudProviderTaint(t *testing.T) { Type: v1.NodeInternalIP, Address: "10.0.0.1", }, + { + Type: v1.NodeInternalIP, + Address: "192.168.0.1", + }, { Type: v1.NodeExternalIP, Address: "132.143.154.163", @@ -105,7 +112,7 @@ func Test_RemoveExternalCloudProviderTaint(t *testing.T) { // register fake GCE cloud provider cloudprovider.RegisterCloudProvider( - "fakeCloud", + "fakeCloudTaints", func(config io.Reader) (cloudprovider.Interface, error) { return fakeCloud, nil }) @@ -125,6 +132,9 @@ func Test_RemoveExternalCloudProviderTaint(t *testing.T) { if n.Spec.Taints[0].Key != v1.TaintNodeNotReady { return false, nil } + if len(n.Status.Addresses) != 4 { + return false, nil + } return true, nil }) if err != nil { @@ -133,6 +143,164 @@ func Test_RemoveExternalCloudProviderTaint(t *testing.T) { } } +// Test the behavior of the alpha.kubernetes.io/provided-node-ip annotation +// and the external cloud provider. +func Test_ExternalCloudProviderNodeAddresses(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Disable ServiceAccount admission plugin as we don't have serviceaccount controller running. + server := kubeapiservertesting.StartTestServerOrDie(t, nil, framework.DefaultTestServerFlags(), framework.SharedEtcd()) + defer server.TearDownFn() + + client := clientset.NewForConfigOrDie(server.ClientConfig) + + ns := framework.CreateNamespaceOrDie(client, "config-map", t) + defer framework.DeleteNamespaceOrDie(client, ns, t) + + // start cloud-controller-manager + kubeconfig := createKubeconfigFileForRestConfig(server.ClientConfig) + // nolint:errcheck // Ignore the error trying to delete the kubeconfig file used for the test + defer os.Remove(kubeconfig) + args := []string{ + "--kubeconfig=" + kubeconfig, + "--cloud-provider=fakeCloud", + "--cidr-allocator-type=" + string(ipam.RangeAllocatorType), + "--configure-cloud-routes=false", + } + originalAddresses := []v1.NodeAddress{ + { + Type: v1.NodeHostName, + Address: "node.cloud.internal", + }, + { + Type: v1.NodeInternalIP, + Address: "10.0.0.1", + }, + { + Type: v1.NodeInternalIP, + Address: "172.16.0.1", + }, + { + Type: v1.NodeInternalIP, + Address: "fd00:1:2:3:4::", + }, + { + Type: v1.NodeInternalIP, + Address: "192.168.0.1", + }, + { + Type: v1.NodeInternalIP, + Address: "2001:db2::1", + }, + { + Type: v1.NodeExternalIP, + Address: "132.143.154.163", + }, + } + + fakeCloud := &fakecloud.Cloud{ + Zone: cloudprovider.Zone{ + FailureDomain: "zone-0", + Region: "region-1", + }, + EnableInstancesV2: true, + ExistsByProviderID: true, + ProviderID: map[types.NodeName]string{ + types.NodeName("node-0"): "12345", + types.NodeName("node-1"): "12345", + types.NodeName("node-2"): "12345", + types.NodeName("node-3"): "12345", + types.NodeName("node-4"): "12345", + }, + Addresses: originalAddresses, + ErrByProviderID: nil, + Err: nil, + } + // register fake GCE cloud provider + cloudprovider.RegisterCloudProvider( + "fakeCloud", + func(config io.Reader) (cloudprovider.Interface, error) { + return fakeCloud, nil + }) + ccm := ccmservertesting.StartTestServerOrDie(ctx, args) + defer ccm.TearDownFn() + + testCases := []struct { + name string + nodeIPs string + }{ + { + name: "IPv4", + nodeIPs: "192.168.0.1", + }, + { + name: "IPv6", + nodeIPs: "2001:db2::1", + }, + { + name: "IPv6-IPv4", + nodeIPs: "2001:db2::1,172.16.0.1", + }, + { + name: "IPv4-IPv6", + nodeIPs: "192.168.0.1,fd00:1:2:3:4::", + }, + } + + for d, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + nodeName := fmt.Sprintf("node-%d", d) + + // Create fake node + node := makeNode(nodeName) + node.Annotations = map[string]string{cloudproviderapi.AnnotationAlphaProvidedIPAddr: tc.nodeIPs} + _, err := client.CoreV1().Nodes().Create(ctx, node, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create Node %v", err) + } + defer func() { + err := client.CoreV1().Nodes().Delete(ctx, node.Name, metav1.DeleteOptions{}) + if err != nil { + t.Fatalf("Failed to delete Node %v", err) + } + }() + // There should be only the taint TaintNodeNotReady, added by the admission plugin TaintNodesByCondition + err = wait.PollUntilContextTimeout(ctx, 1*time.Second, 50*time.Second, true, func(ctx context.Context) (done bool, err error) { + n, err := client.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) + if err != nil { + return false, err + } + if len(n.Spec.Taints) != 1 { + return false, nil + } + if n.Spec.Taints[0].Key != v1.TaintNodeNotReady { + return false, nil + } + + gotInternalIPs := []string{} + for _, address := range n.Status.Addresses { + if address.Type == v1.NodeInternalIP { + gotInternalIPs = append(gotInternalIPs, address.Address) + } + } + nodeIPs := strings.Split(tc.nodeIPs, ",") + // validate only the passed IP as annotation is present + if !reflect.DeepEqual(gotInternalIPs, nodeIPs) { + t.Logf("got node InternalIPs: %v expected node InternalIPs: %v", gotInternalIPs, nodeIPs) + return false, nil + } + + return true, nil + }) + if err != nil { + t.Logf("Fake Cloud Provider calls: %v", fakeCloud.Calls) + t.Fatalf("unexpected error: %v", err) + } + }) + } +} + // sigs.k8s.io/controller-runtime/pkg/envtest func createKubeconfigFileForRestConfig(restConfig *rest.Config) string { clusters := make(map[string]*clientcmdapi.Cluster) From 22fba6591dec555f40d0437606f2d7bd461823a9 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Wed, 5 Jun 2024 11:56:11 +0000 Subject: [PATCH 2/3] node-ip unspecified addresses initialize Node with cloud provider external The node.status.addresses logic grew organically and with weird semantics, this commit try to document existing semantics when the kubelet uses an external cloud provider and recover the same behavior existing pre-1.29. The node.status.addresses can be populated by the kubelet at startup or delegated to the external cloud provider. If the --node-ip flag is set to an IP in the node, the kubelet will add an annotation to the Node object that will be respected by the external cloud providers, no new IP addresses will be added for the same address type. If the IP set in the --node-ip flag is `0.0.0.0` or `::`, the kubelet will initialize the node with the default address of the corresponding IP family of the unspecified address, and the cloud-provider will override it later. --- cmd/kubelet/app/options/options.go | 2 +- pkg/kubelet/nodestatus/setters.go | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cmd/kubelet/app/options/options.go b/cmd/kubelet/app/options/options.go index b87cd735659..fea488e00ab 100644 --- a/cmd/kubelet/app/options/options.go +++ b/cmd/kubelet/app/options/options.go @@ -292,7 +292,7 @@ func (f *KubeletFlags) AddFlags(mainfs *pflag.FlagSet) { fs.StringVar(&f.HostnameOverride, "hostname-override", f.HostnameOverride, "If non-empty, will use this string as identification instead of the actual hostname. If --cloud-provider is set, the cloud provider determines the name of the node (consult cloud provider documentation to determine if and how the hostname is used).") - fs.StringVar(&f.NodeIP, "node-ip", f.NodeIP, "IP address (or comma-separated dual-stack IP addresses) of the node. If unset, kubelet will use the node's default IPv4 address, if any, or its default IPv6 address if it has no IPv4 addresses. You can pass '::' to make it prefer the default IPv6 address rather than the default IPv4 address.") + fs.StringVar(&f.NodeIP, "node-ip", f.NodeIP, "IP address (or comma-separated dual-stack IP addresses) of the node. If unset, kubelet will use the node's default IPv4 address, if any, or its default IPv6 address if it has no IPv4 addresses. You can pass '::' to make it prefer the default IPv6 address rather than the default IPv4 address. If cloud-provider is set to external, this flag will help to bootstrap the node with the corresponding IP.") fs.StringVar(&f.CertDirectory, "cert-dir", f.CertDirectory, "The directory where the TLS certs are located. "+ "If --tls-cert-file and --tls-private-key-file are provided, this flag will be ignored.") diff --git a/pkg/kubelet/nodestatus/setters.go b/pkg/kubelet/nodestatus/setters.go index 19bb6ce7295..6e97651413f 100644 --- a/pkg/kubelet/nodestatus/setters.go +++ b/pkg/kubelet/nodestatus/setters.go @@ -129,12 +129,15 @@ func NodeAddress(nodeIPs []net.IP, // typically Kubelet.nodeIPs if len(node.Status.Addresses) > 0 { return nil } - // If nodeIPs are not specified wait for the external cloud-provider to set the node addresses. + // If nodeIPs are not set wait for the external cloud-provider to set the node addresses. + // If the nodeIP is the unspecified address 0.0.0.0 or ::, then use the IP of the default gateway of + // the corresponding IP family to bootstrap the node until the out-of-tree provider overrides it later. + // xref: https://github.com/kubernetes/kubernetes/issues/125348 // Otherwise uses them on the assumption that the installer/administrator has the previous knowledge // required to ensure the external cloud provider will use the same addresses to avoid the issues explained // in https://github.com/kubernetes/kubernetes/issues/120720. // We are already hinting the external cloud provider via the annotation AnnotationAlphaProvidedIPAddr. - if !nodeIPSpecified { + if nodeIP == nil { node.Status.Addresses = []v1.NodeAddress{ {Type: v1.NodeHostName, Address: hostname}, } From 7917e8b3e03cb170dfe0e4a251d54b15ed3155f0 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Thu, 6 Jun 2024 14:15:47 +0000 Subject: [PATCH 3/3] add more testing for node.status.addresses --- pkg/kubelet/kubelet_node_status.go | 3 +- pkg/kubelet/nodestatus/setters.go | 4 +- pkg/kubelet/nodestatus/setters_test.go | 133 ++++++++++++++++++++++++- 3 files changed, 133 insertions(+), 7 deletions(-) diff --git a/pkg/kubelet/kubelet_node_status.go b/pkg/kubelet/kubelet_node_status.go index ca5e732ea38..78286082ae9 100644 --- a/pkg/kubelet/kubelet_node_status.go +++ b/pkg/kubelet/kubelet_node_status.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + utilnet "k8s.io/apimachinery/pkg/util/net" "k8s.io/apimachinery/pkg/util/sets" cloudprovider "k8s.io/cloud-provider" cloudproviderapi "k8s.io/cloud-provider/api" @@ -729,7 +730,7 @@ func (kl *Kubelet) defaultNodeStatusFuncs() []func(context.Context, *v1.Node) er } var setters []func(ctx context.Context, n *v1.Node) error setters = append(setters, - nodestatus.NodeAddress(kl.nodeIPs, kl.nodeIPValidator, kl.hostname, kl.hostnameOverridden, kl.externalCloudProvider, kl.cloud, nodeAddressesFunc), + nodestatus.NodeAddress(kl.nodeIPs, kl.nodeIPValidator, kl.hostname, kl.hostnameOverridden, kl.externalCloudProvider, kl.cloud, nodeAddressesFunc, utilnet.ResolveBindAddress), nodestatus.MachineInfo(string(kl.nodeName), kl.maxPods, kl.podsPerCore, kl.GetCachedMachineInfo, kl.containerManager.GetCapacity, kl.containerManager.GetDevicePluginResourceCapacity, kl.containerManager.GetNodeAllocatableReservation, kl.recordEvent, kl.supportLocalStorageCapacityIsolation()), nodestatus.VersionInfo(kl.cadvisor.VersionInfo, kl.containerRuntime.Type, kl.containerRuntime.Version), diff --git a/pkg/kubelet/nodestatus/setters.go b/pkg/kubelet/nodestatus/setters.go index 6e97651413f..1b1e8753d85 100644 --- a/pkg/kubelet/nodestatus/setters.go +++ b/pkg/kubelet/nodestatus/setters.go @@ -31,7 +31,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/errors" - utilnet "k8s.io/apimachinery/pkg/util/net" utilfeature "k8s.io/apiserver/pkg/util/feature" cloudprovider "k8s.io/cloud-provider" cloudproviderapi "k8s.io/cloud-provider/api" @@ -66,6 +65,7 @@ func NodeAddress(nodeIPs []net.IP, // typically Kubelet.nodeIPs externalCloudProvider bool, // typically Kubelet.externalCloudProvider cloud cloudprovider.Interface, // typically Kubelet.cloud nodeAddressesFunc func() ([]v1.NodeAddress, error), // typically Kubelet.cloudResourceSyncManager.NodeAddresses + resolveAddressFunc func(net.IP) (net.IP, error), // typically k8s.io/apimachinery/pkg/util/net.ResolveBindAddress ) Setter { var nodeIP, secondaryNodeIP net.IP if len(nodeIPs) > 0 { @@ -225,7 +225,7 @@ func NodeAddress(nodeIPs []net.IP, // typically Kubelet.nodeIPs } if ipAddr == nil { - ipAddr, err = utilnet.ResolveBindAddress(nodeIP) + ipAddr, err = resolveAddressFunc(nodeIP) } } diff --git a/pkg/kubelet/nodestatus/setters_test.go b/pkg/kubelet/nodestatus/setters_test.go index 1666683ef21..4fffe32e3c9 100644 --- a/pkg/kubelet/nodestatus/setters_test.go +++ b/pkg/kubelet/nodestatus/setters_test.go @@ -63,12 +63,14 @@ func TestNodeAddress(t *testing.T) { cloudProviderExternal cloudProviderNone ) + existingNodeAddress := v1.NodeAddress{Address: "10.1.1.2"} cases := []struct { name string hostnameOverride bool nodeIP net.IP secondaryNodeIP net.IP + resolvedIP net.IP cloudProviderType cloudProviderType nodeAddresses []v1.NodeAddress expectedAddresses []v1.NodeAddress @@ -234,8 +236,110 @@ func TestNodeAddress(t *testing.T) { shouldError: false, }, { - name: "cloud provider is external and nodeIP unspecified", + name: "no cloud provider and nodeIP IPv4 unspecified", + nodeIP: netutils.ParseIPSloppy("0.0.0.0"), + resolvedIP: netutils.ParseIPSloppy("10.0.0.2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderNone, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.0.0.2"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "no cloud provider and nodeIP IPv6 unspecified", nodeIP: netutils.ParseIPSloppy("::"), + resolvedIP: netutils.ParseIPSloppy("2001:db2::2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderNone, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "2001:db2::2"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "legacy cloud provider and nodeIP IPv4 unspecified", + nodeIP: netutils.ParseIPSloppy("0.0.0.0"), + resolvedIP: netutils.ParseIPSloppy("10.0.0.2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderLegacy, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: true, + }, + { + name: "legacy cloud provider and nodeIP IPv6 unspecified", + nodeIP: netutils.ParseIPSloppy("::"), + resolvedIP: netutils.ParseIPSloppy("2001:db2::2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderLegacy, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: true, + }, + { + name: "cloud provider is external and nodeIP IPv4 unspecified", + nodeIP: netutils.ParseIPSloppy("0.0.0.0"), + resolvedIP: netutils.ParseIPSloppy("10.0.0.2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderExternal, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.0.0.2"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "cloud provider is external and nodeIP IPv6 unspecified", + nodeIP: netutils.ParseIPSloppy("::"), + resolvedIP: netutils.ParseIPSloppy("2001:db2::2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderExternal, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "2001:db2::2"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "no cloud provider and no nodeIP resolve IPv4", + resolvedIP: netutils.ParseIPSloppy("10.0.0.2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderNone, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "10.0.0.2"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "no cloud provider and no nodeIP resolve IPv6", + resolvedIP: netutils.ParseIPSloppy("2001:db2::2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderNone, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeInternalIP, Address: "2001:db2::2"}, + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: false, + }, + { + name: "legacy cloud provider and no nodeIP", + resolvedIP: netutils.ParseIPSloppy("10.0.0.2"), + nodeAddresses: []v1.NodeAddress{}, + cloudProviderType: cloudProviderLegacy, + expectedAddresses: []v1.NodeAddress{ + {Type: v1.NodeHostName, Address: testKubeletHostname}, + }, + shouldError: true, + }, + { + name: "cloud provider is external and no nodeIP resolve IPv4", + resolvedIP: netutils.ParseIPSloppy("10.0.0.2"), nodeAddresses: []v1.NodeAddress{}, cloudProviderType: cloudProviderExternal, expectedAddresses: []v1.NodeAddress{ @@ -244,7 +348,8 @@ func TestNodeAddress(t *testing.T) { shouldError: false, }, { - name: "cloud provider is external and no nodeIP", + name: "cloud provider is external and no nodeIP resolve IPv6", + resolvedIP: netutils.ParseIPSloppy("2001:db2::2"), nodeAddresses: []v1.NodeAddress{}, cloudProviderType: cloudProviderExternal, expectedAddresses: []v1.NodeAddress{ @@ -638,6 +743,20 @@ func TestNodeAddress(t *testing.T) { return testCase.nodeAddresses, nil } + net.DefaultResolver = &net.Resolver{ + PreferGo: true, + Dial: func(ctx context.Context, network string, address string) (net.Conn, error) { + return nil, fmt.Errorf("error") + }, + } + defer func() { + net.DefaultResolver = &net.Resolver{} + }() + + resolveAddressFunc := func(net.IP) (net.IP, error) { + return testCase.resolvedIP, nil + } + // cloud provider is expected to be nil if external provider is set or there is no cloud provider var cloud cloudprovider.Interface if testCase.cloudProviderType == cloudProviderLegacy { @@ -659,7 +778,9 @@ func TestNodeAddress(t *testing.T) { testCase.hostnameOverride, testCase.cloudProviderType == cloudProviderExternal, cloud, - nodeAddressesFunc) + nodeAddressesFunc, + resolveAddressFunc, + ) // call setter on existing node err := setter(ctx, existingNode) @@ -739,6 +860,9 @@ func TestNodeAddress_NoCloudProvider(t *testing.T) { nodeAddressesFunc := func() ([]v1.NodeAddress, error) { return nil, fmt.Errorf("not reached") } + resolvedAddressesFunc := func(net.IP) (net.IP, error) { + return nil, fmt.Errorf("not reached") + } // construct setter setter := NodeAddress(testCase.nodeIPs, @@ -747,7 +871,8 @@ func TestNodeAddress_NoCloudProvider(t *testing.T) { false, // hostnameOverridden false, // externalCloudProvider nil, // cloud - nodeAddressesFunc) + nodeAddressesFunc, + resolvedAddressesFunc) // call setter on existing node err := setter(ctx, existingNode)