From 5b465d0bf36e9e70033866d873fd5c8c55ac2143 Mon Sep 17 00:00:00 2001 From: Dmitri Fedotov Date: Tue, 17 Dec 2024 12:41:28 +0200 Subject: [PATCH 1/5] Make `NodeAddresses` Configuration optional --- pkg/cloudprovider/metal/cloud.go | 7 ++++++- pkg/cloudprovider/metal/config.go | 7 ++++++- pkg/cloudprovider/metal/instances_v2.go | 25 ++++++++++++++----------- pkg/cloudprovider/metal/suite_test.go | 3 +++ 4 files changed, 29 insertions(+), 13 deletions(-) diff --git a/pkg/cloudprovider/metal/cloud.go b/pkg/cloudprovider/metal/cloud.go index 5fd77b5..d326591 100644 --- a/pkg/cloudprovider/metal/cloud.go +++ b/pkg/cloudprovider/metal/cloud.go @@ -80,7 +80,12 @@ func (o *cloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, log.Fatalf("Failed to create new cluster: %v", err) } - o.instancesV2 = newMetalInstancesV2(o.targetCluster.GetClient(), o.metalCluster.GetClient(), o.metalNamespace, o.cloudConfig.ClusterName) + o.instancesV2 = newMetalInstancesV2( + o.targetCluster.GetClient(), + o.metalCluster.GetClient(), + o.metalNamespace, + o.cloudConfig, + ) if err := o.metalCluster.GetFieldIndexer().IndexField(ctx, &metalv1alpha1.ServerClaim{}, serverClaimMetadataUIDField, func(object client.Object) []string { serverClaim := object.(*metalv1alpha1.ServerClaim) diff --git a/pkg/cloudprovider/metal/config.go b/pkg/cloudprovider/metal/config.go index 48cbbfd..6c1d2f0 100644 --- a/pkg/cloudprovider/metal/config.go +++ b/pkg/cloudprovider/metal/config.go @@ -22,8 +22,13 @@ type CloudProviderConfig struct { cloudConfig CloudConfig } +type NetworkingOpts struct { + ConfigureNodeAddresses bool `json:"configureNodeAddresses"` +} + type CloudConfig struct { - ClusterName string `json:"clusterName"` + ClusterName string `json:"clusterName"` + Networking NetworkingOpts `json:"networkingOpts"` } var ( diff --git a/pkg/cloudprovider/metal/instances_v2.go b/pkg/cloudprovider/metal/instances_v2.go index 9cfaa5c..2fbe14f 100644 --- a/pkg/cloudprovider/metal/instances_v2.go +++ b/pkg/cloudprovider/metal/instances_v2.go @@ -20,15 +20,15 @@ type metalInstancesV2 struct { targetClient client.Client metalClient client.Client metalNamespace string - clusterName string + cloudConfig CloudConfig } -func newMetalInstancesV2(targetClient client.Client, metalClient client.Client, namespace, clusterName string) cloudprovider.InstancesV2 { +func newMetalInstancesV2(targetClient client.Client, metalClient client.Client, namespace string, cloudConfig CloudConfig) cloudprovider.InstancesV2 { return &metalInstancesV2{ targetClient: targetClient, metalClient: metalClient, metalNamespace: namespace, - clusterName: clusterName, + cloudConfig: cloudConfig, } } @@ -98,7 +98,7 @@ func (o *metalInstancesV2) InstanceMetadata(ctx context.Context, node *corev1.No if serverClaim.Labels == nil { serverClaim.Labels = make(map[string]string) } - serverClaim.Labels[LabelKeyClusterName] = o.clusterName + serverClaim.Labels[LabelKeyClusterName] = o.cloudConfig.ClusterName klog.V(2).InfoS("Adding cluster name label to server claim object", "ServerClaim", client.ObjectKeyFromObject(serverClaim), "Node", node.Name) if err := o.metalClient.Patch(ctx, serverClaim, client.MergeFrom(serverClaimBase)); err != nil { return nil, fmt.Errorf("failed to patch server claim for Node %s: %w", node.Name, err) @@ -139,13 +139,16 @@ func (o *metalInstancesV2) InstanceMetadata(ctx context.Context, node *corev1.No klog.V(2).InfoS("No region label found for node instance", "Node", node.Name) } - return &cloudprovider.InstanceMetadata{ - ProviderID: providerID, - InstanceType: instanceType, - NodeAddresses: addresses, - Zone: zone, - Region: region, - }, nil + metaData := &cloudprovider.InstanceMetadata{ + ProviderID: providerID, + InstanceType: instanceType, + Zone: zone, + Region: region, + } + if o.cloudConfig.Networking.ConfigureNodeAddresses { + metaData.NodeAddresses = addresses + } + return metaData, nil } func (o *metalInstancesV2) getServerClaimForNode(ctx context.Context, node *corev1.Node) (*metalv1alpha1.ServerClaim, error) { diff --git a/pkg/cloudprovider/metal/suite_test.go b/pkg/cloudprovider/metal/suite_test.go index 21d41ba..69f8848 100644 --- a/pkg/cloudprovider/metal/suite_test.go +++ b/pkg/cloudprovider/metal/suite_test.go @@ -147,6 +147,9 @@ func SetupTest() (*corev1.Namespace, *cloudprovider.Interface, string) { }() cloudConfig := CloudConfig{ ClusterName: clusterName, + Networking: NetworkingOpts{ + ConfigureNodeAddresses: true, + }, } cloudConfigData, err := yaml.Marshal(&cloudConfig) Expect(err).NotTo(HaveOccurred()) From 13175b8bf9a11d11d6a2ab9efbf1618913725005 Mon Sep 17 00:00:00 2001 From: Dmitri Fedotov Date: Tue, 17 Dec 2024 14:47:47 +0200 Subject: [PATCH 2/5] rename field to `Networking` --- pkg/cloudprovider/metal/config.go | 6 +++--- pkg/cloudprovider/metal/suite_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cloudprovider/metal/config.go b/pkg/cloudprovider/metal/config.go index 6c1d2f0..81a5186 100644 --- a/pkg/cloudprovider/metal/config.go +++ b/pkg/cloudprovider/metal/config.go @@ -22,13 +22,13 @@ type CloudProviderConfig struct { cloudConfig CloudConfig } -type NetworkingOpts struct { +type Networking struct { ConfigureNodeAddresses bool `json:"configureNodeAddresses"` } type CloudConfig struct { - ClusterName string `json:"clusterName"` - Networking NetworkingOpts `json:"networkingOpts"` + ClusterName string `json:"clusterName"` + Networking Networking `json:"networking"` } var ( diff --git a/pkg/cloudprovider/metal/suite_test.go b/pkg/cloudprovider/metal/suite_test.go index 69f8848..75f8997 100644 --- a/pkg/cloudprovider/metal/suite_test.go +++ b/pkg/cloudprovider/metal/suite_test.go @@ -147,7 +147,7 @@ func SetupTest() (*corev1.Namespace, *cloudprovider.Interface, string) { }() cloudConfig := CloudConfig{ ClusterName: clusterName, - Networking: NetworkingOpts{ + Networking: Networking{ ConfigureNodeAddresses: true, }, } From ae907bf11273f4babeccdd69c028dc9d766de3d1 Mon Sep 17 00:00:00 2001 From: Dmitri Fedotov Date: Tue, 17 Dec 2024 17:39:21 +0200 Subject: [PATCH 3/5] add test to expect empty address field --- pkg/cloudprovider/metal/instances_v2_test.go | 148 +++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/pkg/cloudprovider/metal/instances_v2_test.go b/pkg/cloudprovider/metal/instances_v2_test.go index 7a350a7..a93904c 100644 --- a/pkg/cloudprovider/metal/instances_v2_test.go +++ b/pkg/cloudprovider/metal/instances_v2_test.go @@ -4,15 +4,23 @@ package metal import ( + "context" "fmt" + "os" + + "k8s.io/client-go/tools/clientcmd" + "sigs.k8s.io/controller-runtime/pkg/envtest" metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" cloudprovider "k8s.io/cloud-provider" + "k8s.io/controller-manager/pkg/clientbuilder" . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" + "sigs.k8s.io/yaml" ) var _ = Describe("InstancesV2", func() { @@ -250,6 +258,146 @@ var _ = Describe("InstancesV2", func() { Expect(err).To(Equal(cloudprovider.InstanceNotFound)) Expect(ok).To(BeFalse()) }) + + It("Should not configure node addresses if ConfigureNodeAddresses is false", func(ctx SpecContext) { + By("Starting up a cloud provider with cloud config to disable node address configuration") + user, err := testEnv.AddUser(envtest.User{ + Name: "dummy", + Groups: []string{"system:authenticated", "system:masters"}, + }, nil) + Expect(err).NotTo(HaveOccurred()) + + kubeconfigData, err := user.KubeConfig() + Expect(err).NotTo(HaveOccurred()) + + clientConfig, err := clientcmd.Load(kubeconfigData) + Expect(err).NotTo(HaveOccurred()) + clientConfig.Contexts[clientConfig.CurrentContext].Namespace = ns.Name + + namespacedKubeconfigData, err := clientcmd.Write(*clientConfig) + Expect(err).NotTo(HaveOccurred()) + + kubeconfigFile, err := os.CreateTemp(GinkgoT().TempDir(), "kubeconfig") + Expect(err).NotTo(HaveOccurred()) + defer func() { + _ = kubeconfigFile.Close() + }() + Expect(os.WriteFile(kubeconfigFile.Name(), namespacedKubeconfigData, 0666)).To(Succeed()) + + curr := MetalKubeconfigPath + defer func() { + MetalKubeconfigPath = curr + }() + MetalKubeconfigPath = kubeconfigFile.Name() + + cloudConfigFile, err := os.CreateTemp(GinkgoT().TempDir(), "cloud.yaml") + Expect(err).NotTo(HaveOccurred()) + defer func() { + _ = cloudConfigFile.Close() + }() + cloudConfig := CloudConfig{ + ClusterName: clusterName, + Networking: Networking{ + ConfigureNodeAddresses: false, + }, + } + cloudConfigData, err := yaml.Marshal(&cloudConfig) + Expect(err).NotTo(HaveOccurred()) + Expect(os.WriteFile(cloudConfigFile.Name(), cloudConfigData, 0666)).To(Succeed()) + + cloudProviderCtx, cancel := context.WithCancel(context.Background()) + DeferCleanup(cancel) + + k8sClientSet, err := kubernetes.NewForConfig(cfg) + Expect(err).NotTo(HaveOccurred()) + + clientBuilder := clientbuilder.NewDynamicClientBuilder(testEnv.Config, k8sClientSet.CoreV1(), ns.Name) + cp, err := cloudprovider.InitCloudProvider(ProviderName, cloudConfigFile.Name()) + Expect(err).NotTo(HaveOccurred()) + cp.Initialize(clientBuilder, cloudProviderCtx.Done()) + + By("Instantiating the instances v2 provider") + instancesProvider, ok := cp.InstancesV2() + Expect(ok).To(BeTrue()) + + By("Creating a Server") + server := &metalv1alpha1.Server{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + Labels: map[string]string{ + "instance-type": "foo", + "zone": "a", + "region": "bar", + }, + }, + Spec: metalv1alpha1.ServerSpec{ + UUID: "12345", + Power: "On", + }, + } + Expect(k8sClient.Create(ctx, server)).To(Succeed()) + DeferCleanup(k8sClient.Delete, server) + + By("Patching the Server object to have a valid network interface status") + Eventually(UpdateStatus(server, func() { + server.Status.PowerState = metalv1alpha1.ServerOnPowerState + server.Status.NetworkInterfaces = []metalv1alpha1.NetworkInterface{{ + Name: "my-nic", + IP: metalv1alpha1.MustParseIP("10.0.0.1"), + }} + })).Should(Succeed()) + + By("Creating a ServerClaim for a Node") + serverClaim := &metalv1alpha1.ServerClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "test-", + }, + Spec: metalv1alpha1.ServerClaimSpec{ + Power: "On", + ServerRef: &corev1.LocalObjectReference{Name: server.Name}, + }, + } + Expect(k8sClient.Create(ctx, serverClaim)).To(Succeed()) + DeferCleanup(k8sClient.Delete, serverClaim) + + By("Creating a Node object with a provider ID referencing the machine") + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-", + }, + Spec: corev1.NodeSpec{ + ProviderID: getProviderID(serverClaim.Namespace, serverClaim.Name), + }, + } + Expect(k8sClient.Create(ctx, node)).To(Succeed()) + DeferCleanup(k8sClient.Delete, node) + + By("Ensuring that an instance for a Node exists") + ok, err = instancesProvider.InstanceExists(ctx, node) + Expect(err).NotTo(HaveOccurred()) + Expect(ok).To(BeTrue()) + + By("Ensuring that the instance is not shut down") + ok, err = instancesProvider.InstanceShutdown(ctx, node) + Expect(err).NotTo(HaveOccurred()) + Expect(ok).To(BeFalse()) + + By("Ensuring that the instance meta data has empty addresses") + instanceMetadata, err := instancesProvider.InstanceMetadata(ctx, node) + Expect(err).NotTo(HaveOccurred()) + Eventually(instanceMetadata).Should(SatisfyAll( + HaveField("ProviderID", getProviderID(serverClaim.Namespace, serverClaim.Name)), + HaveField("InstanceType", "foo"), + HaveField("NodeAddresses", BeEmpty()), + HaveField("Zone", "a"), + HaveField("Region", "bar"))) + + By("Ensuring cluster name label is added to ServerClaim object") + Eventually(Object(serverClaim)).Should(SatisfyAll( + HaveField("Labels", map[string]string{LabelKeyClusterName: clusterName}), + )) + }) }) func getProviderID(namespace, serverClaimName string) string { From e3ecdcd80579ac0a7e416642076aba8ac2cc0bf2 Mon Sep 17 00:00:00 2001 From: Dmitri Fedotov Date: Wed, 18 Dec 2024 12:23:56 +0200 Subject: [PATCH 4/5] provide `cloudConfig` as argument to `SetupTest()` --- pkg/cloudprovider/metal/cloud_test.go | 8 +- pkg/cloudprovider/metal/instances_v2_test.go | 97 ++++++-------------- pkg/cloudprovider/metal/suite_test.go | 15 +-- 3 files changed, 38 insertions(+), 82 deletions(-) diff --git a/pkg/cloudprovider/metal/cloud_test.go b/pkg/cloudprovider/metal/cloud_test.go index 23da41f..c3ac466 100644 --- a/pkg/cloudprovider/metal/cloud_test.go +++ b/pkg/cloudprovider/metal/cloud_test.go @@ -9,7 +9,13 @@ import ( ) var _ = Describe("Cloud", func() { - _, cp, _ := SetupTest() + cloudConfig := CloudConfig{ + ClusterName: clusterName, + Networking: Networking{ + ConfigureNodeAddresses: true, + }, + } + _, cp, _ := SetupTest(cloudConfig) It("should ensure the correct cloud provider setup", func() { Expect((*cp).HasClusterID()).To(BeTrue()) diff --git a/pkg/cloudprovider/metal/instances_v2_test.go b/pkg/cloudprovider/metal/instances_v2_test.go index a93904c..5e6845b 100644 --- a/pkg/cloudprovider/metal/instances_v2_test.go +++ b/pkg/cloudprovider/metal/instances_v2_test.go @@ -4,30 +4,30 @@ package metal import ( - "context" "fmt" - "os" - - "k8s.io/client-go/tools/clientcmd" - "sigs.k8s.io/controller-runtime/pkg/envtest" metalv1alpha1 "github.com/ironcore-dev/metal-operator/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" cloudprovider "k8s.io/cloud-provider" - "k8s.io/controller-manager/pkg/clientbuilder" . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" - "sigs.k8s.io/yaml" +) + +var ( + instancesProvider cloudprovider.InstancesV2 + clusterName = "test" ) var _ = Describe("InstancesV2", func() { - var ( - instancesProvider cloudprovider.InstancesV2 - ) - ns, cp, clusterName := SetupTest() + cloudConfig := CloudConfig{ + ClusterName: clusterName, + Networking: Networking{ + ConfigureNodeAddresses: true, + }, + } + ns, cp, clusterName := SetupTest(cloudConfig) BeforeEach(func() { By("Instantiating the instances v2 provider") @@ -258,68 +258,25 @@ var _ = Describe("InstancesV2", func() { Expect(err).To(Equal(cloudprovider.InstanceNotFound)) Expect(ok).To(BeFalse()) }) +}) - It("Should not configure node addresses if ConfigureNodeAddresses is false", func(ctx SpecContext) { - By("Starting up a cloud provider with cloud config to disable node address configuration") - user, err := testEnv.AddUser(envtest.User{ - Name: "dummy", - Groups: []string{"system:authenticated", "system:masters"}, - }, nil) - Expect(err).NotTo(HaveOccurred()) - - kubeconfigData, err := user.KubeConfig() - Expect(err).NotTo(HaveOccurred()) - - clientConfig, err := clientcmd.Load(kubeconfigData) - Expect(err).NotTo(HaveOccurred()) - clientConfig.Contexts[clientConfig.CurrentContext].Namespace = ns.Name - - namespacedKubeconfigData, err := clientcmd.Write(*clientConfig) - Expect(err).NotTo(HaveOccurred()) - - kubeconfigFile, err := os.CreateTemp(GinkgoT().TempDir(), "kubeconfig") - Expect(err).NotTo(HaveOccurred()) - defer func() { - _ = kubeconfigFile.Close() - }() - Expect(os.WriteFile(kubeconfigFile.Name(), namespacedKubeconfigData, 0666)).To(Succeed()) - - curr := MetalKubeconfigPath - defer func() { - MetalKubeconfigPath = curr - }() - MetalKubeconfigPath = kubeconfigFile.Name() - - cloudConfigFile, err := os.CreateTemp(GinkgoT().TempDir(), "cloud.yaml") - Expect(err).NotTo(HaveOccurred()) - defer func() { - _ = cloudConfigFile.Close() - }() - cloudConfig := CloudConfig{ - ClusterName: clusterName, - Networking: Networking{ - ConfigureNodeAddresses: false, - }, - } - cloudConfigData, err := yaml.Marshal(&cloudConfig) - Expect(err).NotTo(HaveOccurred()) - Expect(os.WriteFile(cloudConfigFile.Name(), cloudConfigData, 0666)).To(Succeed()) - - cloudProviderCtx, cancel := context.WithCancel(context.Background()) - DeferCleanup(cancel) - - k8sClientSet, err := kubernetes.NewForConfig(cfg) - Expect(err).NotTo(HaveOccurred()) - - clientBuilder := clientbuilder.NewDynamicClientBuilder(testEnv.Config, k8sClientSet.CoreV1(), ns.Name) - cp, err := cloudprovider.InitCloudProvider(ProviderName, cloudConfigFile.Name()) - Expect(err).NotTo(HaveOccurred()) - cp.Initialize(clientBuilder, cloudProviderCtx.Done()) +var _ = Describe("InstancesV2", func() { + cloudConfig := CloudConfig{ + ClusterName: clusterName, + Networking: Networking{ + ConfigureNodeAddresses: false, + }, + } + ns, cp, clusterName := SetupTest(cloudConfig) + BeforeEach(func() { By("Instantiating the instances v2 provider") - instancesProvider, ok := cp.InstancesV2() + var ok bool + instancesProvider, ok = (*cp).InstancesV2() Expect(ok).To(BeTrue()) + }) + It("Should not configure node addresses if ConfigureNodeAddresses is false", func(ctx SpecContext) { By("Creating a Server") server := &metalv1alpha1.Server{ ObjectMeta: metav1.ObjectMeta{ @@ -374,7 +331,7 @@ var _ = Describe("InstancesV2", func() { DeferCleanup(k8sClient.Delete, node) By("Ensuring that an instance for a Node exists") - ok, err = instancesProvider.InstanceExists(ctx, node) + ok, err := instancesProvider.InstanceExists(ctx, node) Expect(err).NotTo(HaveOccurred()) Expect(ok).To(BeTrue()) diff --git a/pkg/cloudprovider/metal/suite_test.go b/pkg/cloudprovider/metal/suite_test.go index 75f8997..f0a4a2d 100644 --- a/pkg/cloudprovider/metal/suite_test.go +++ b/pkg/cloudprovider/metal/suite_test.go @@ -97,11 +97,10 @@ var _ = BeforeSuite(func() { SetClient(k8sClient) }) -func SetupTest() (*corev1.Namespace, *cloudprovider.Interface, string) { +func SetupTest(cloudConfig CloudConfig) (*corev1.Namespace, *cloudprovider.Interface, string) { var ( - ns = &corev1.Namespace{} - cp cloudprovider.Interface - clusterName = "test" + ns = &corev1.Namespace{} + cp cloudprovider.Interface ) BeforeEach(func(ctx SpecContext) { @@ -145,12 +144,6 @@ func SetupTest() (*corev1.Namespace, *cloudprovider.Interface, string) { defer func() { _ = cloudConfigFile.Close() }() - cloudConfig := CloudConfig{ - ClusterName: clusterName, - Networking: Networking{ - ConfigureNodeAddresses: true, - }, - } cloudConfigData, err := yaml.Marshal(&cloudConfig) Expect(err).NotTo(HaveOccurred()) Expect(os.WriteFile(cloudConfigFile.Name(), cloudConfigData, 0666)).To(Succeed()) @@ -167,5 +160,5 @@ func SetupTest() (*corev1.Namespace, *cloudprovider.Interface, string) { cp.Initialize(clientBuilder, cloudProviderCtx.Done()) }) - return ns, &cp, clusterName + return ns, &cp, cloudConfig.ClusterName } From 3123920588899b81df9de14c6a9a4acb2d6c5642 Mon Sep 17 00:00:00 2001 From: Dmitri Fedotov Date: Thu, 19 Dec 2024 11:18:29 +0200 Subject: [PATCH 5/5] edit description that we modify controler config --- pkg/cloudprovider/metal/instances_v2_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cloudprovider/metal/instances_v2_test.go b/pkg/cloudprovider/metal/instances_v2_test.go index 5e6845b..8156b9b 100644 --- a/pkg/cloudprovider/metal/instances_v2_test.go +++ b/pkg/cloudprovider/metal/instances_v2_test.go @@ -260,7 +260,7 @@ var _ = Describe("InstancesV2", func() { }) }) -var _ = Describe("InstancesV2", func() { +var _ = Describe("InstancesV2 with modified CloudConfig", func() { cloudConfig := CloudConfig{ ClusterName: clusterName, Networking: Networking{