Configure provider-id for the machines/nodes (kubermatic#1723)

* Configure provider-id for the machines/nodes MC will inject provider-id for the machines that are created against cloud providers that don't have in-tree or external CCM support Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com> * Add exception in golangci-lint Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com> * Refactored code Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com> --------- Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com>
ahmedwaleedmalik · Mar 4, 2024 · 1ef6369 · 1ef6369
1 parent b3fe8a2
commit 1ef6369
Show file tree

Hide file tree

Showing 17 changed files with 111 additions and 2 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -59,3 +59,4 @@ issues:
     - 'cyclomatic complexity 34 of func `\(\*provider\)\.getConfig` is high'
     - 'cyclomatic complexity 31 of func `\(\*provider\)\.Validate` is high'
     - 'cyclomatic complexity 33 of func `\(\*provider\)\.Create` is high'
+    - 'cyclomatic complexity 32 of func `\(\*Reconciler\)\.ensureInstanceExistsForMachine` is high'
diff --git a/pkg/admission/machines.go b/pkg/admission/machines.go
@@ -64,6 +64,16 @@ func (ad *admissionData) mutateMachines(ctx context.Context, ar admissionv1.Admi
 		if oldMachine.Spec.Name != machine.Spec.Name && machine.Spec.Name == machine.Name {
 			oldMachine.Spec.Name = machine.Spec.Name
 		}
+
+		if oldMachine.Spec.ProviderID != nil && machine.Spec.ProviderID != nil && *oldMachine.Spec.ProviderID != *machine.Spec.ProviderID {
+			return nil, fmt.Errorf("providerID is immutable")
+		}
+
+		// Allow mutation of the ProviderID field, as it can only be computed after the machine is created.
+		if oldMachine.Spec.ProviderID == nil && machine.Spec.ProviderID != nil {
+			oldMachine.Spec.ProviderID = machine.Spec.ProviderID
+		}
+
 		// Allow mutation when:
 		// * machine has the `MigrationBypassSpecNoModificationRequirementAnnotation` annotation (used for type migration)
 		bypassValidationForMigration := machine.Annotations[BypassSpecNoModificationRequirementAnnotation] == "true"

diff --git a/pkg/cloudprovider/provider/anexia/instance.go b/pkg/cloudprovider/provider/anexia/instance.go
@@ -51,6 +51,9 @@ func (ai *anexiaInstance) ID() string {
 }
 
 func (ai *anexiaInstance) ProviderID() string {
+	if ai == nil || ai.ID() == "" {
+		return ""
+	}
 	return ai.ID()
 }
 

diff --git a/pkg/cloudprovider/provider/digitalocean/provider.go b/pkg/cloudprovider/provider/digitalocean/provider.go
@@ -506,6 +506,9 @@ func (d *doInstance) ID() string {
 }
 
 func (d *doInstance) ProviderID() string {
+	if d.droplet == nil || d.droplet.Name == "" {
+		return ""
+	}
 	return fmt.Sprintf("digitalocean://%d", d.droplet.ID)
 }
 

diff --git a/pkg/cloudprovider/provider/equinixmetal/provider.go b/pkg/cloudprovider/provider/equinixmetal/provider.go
@@ -400,6 +400,9 @@ func (s *metalDevice) ID() string {
 }
 
 func (s *metalDevice) ProviderID() string {
+	if s.device == nil || s.device.ID == "" {
+		return ""
+	}
 	return "equinixmetal://" + s.device.ID
 }
 

diff --git a/pkg/cloudprovider/provider/gce/instance.go b/pkg/cloudprovider/provider/gce/instance.go
@@ -61,6 +61,9 @@ func (gi *googleInstance) ID() string {
 }
 
 func (gi *googleInstance) ProviderID() string {
+	if gi.ci == nil || gi.ci.Name == "" {
+		return ""
+	}
 	return fmt.Sprintf("gce://%s/%s/%s", gi.projectID, gi.zone, gi.ci.Name)
 }
 

diff --git a/pkg/cloudprovider/provider/hetzner/provider.go b/pkg/cloudprovider/provider/hetzner/provider.go
@@ -553,6 +553,9 @@ func (s *hetznerServer) ID() string {
 }
 
 func (s *hetznerServer) ProviderID() string {
+	if s.server == nil || s.server.ID == 0 {
+		return ""
+	}
 	return fmt.Sprintf("hcloud://%d", s.server.ID)
 }
 

diff --git a/pkg/cloudprovider/provider/kubevirt/provider.go b/pkg/cloudprovider/provider/kubevirt/provider.go
@@ -160,6 +160,9 @@ func (k *kubeVirtServer) ID() string {
 }
 
 func (k *kubeVirtServer) ProviderID() string {
+	if k.vmi.Name == "" {
+		return ""
+	}
 	return "kubevirt://" + k.vmi.Name
 }
 

diff --git a/pkg/cloudprovider/provider/linode/provider.go b/pkg/cloudprovider/provider/linode/provider.go
@@ -405,6 +405,9 @@ func (d *linodeInstance) ID() string {
 }
 
 func (d *linodeInstance) ProviderID() string {
+	if d == nil || d.ID() == "" {
+		return ""
+	}
 	return fmt.Sprintf("linode://%s", d.ID())
 }
 

diff --git a/pkg/cloudprovider/provider/nutanix/provider.go b/pkg/cloudprovider/provider/nutanix/provider.go
@@ -89,6 +89,9 @@ func (nutanixServer Server) ID() string {
 }
 
 func (nutanixServer Server) ProviderID() string {
+	if nutanixServer.ID() == "" {
+		return ""
+	}
 	return fmt.Sprintf("nutanix://%s", nutanixServer.ID())
 }
 

diff --git a/pkg/cloudprovider/provider/opennebula/provider.go b/pkg/cloudprovider/provider/opennebula/provider.go
@@ -427,6 +427,9 @@ func (i *openNebulaInstance) ID() string {
 }
 
 func (i *openNebulaInstance) ProviderID() string {
+	if i.vm == nil || i.vm.ID == 0 {
+		return ""
+	}
 	return "opennebula://" + strconv.Itoa(i.vm.ID)
 }
 

diff --git a/pkg/cloudprovider/provider/openstack/provider.go b/pkg/cloudprovider/provider/openstack/provider.go
@@ -936,6 +936,9 @@ func (d *osInstance) ID() string {
 }
 
 func (d *osInstance) ProviderID() string {
+	if d.server == nil || d.server.ID == "" {
+		return ""
+	}
 	return "openstack:///" + d.server.ID
 }
 

diff --git a/pkg/cloudprovider/provider/vmwareclouddirector/provider.go b/pkg/cloudprovider/provider/vmwareclouddirector/provider.go
@@ -123,6 +123,9 @@ func (s Server) ID() string {
 }
 
 func (s Server) ProviderID() string {
+	if s.ID() == "" {
+		return ""
+	}
 	return fmt.Sprintf("vmware-cloud-director://%s", s.ID())
 }
 

diff --git a/pkg/cloudprovider/provider/vsphere/provider.go b/pkg/cloudprovider/provider/vsphere/provider.go
@@ -101,6 +101,9 @@ func (vsphereServer Server) ID() string {
 }
 
 func (vsphereServer Server) ProviderID() string {
+	if vsphereServer.uuid == "" {
+		return ""
+	}
 	return "vsphere://" + vsphereServer.uuid
 }
 

diff --git a/pkg/cloudprovider/provider/vultr/provider.go b/pkg/cloudprovider/provider/vultr/provider.go
@@ -338,6 +338,9 @@ func (v *vultrInstance) ID() string {
 }
 
 func (v *vultrInstance) ProviderID() string {
+	if v.instance == nil || v.instance.ID == "" {
+		return ""
+	}
 	return "vultr://" + v.instance.ID
 }
 

diff --git a/pkg/controller/machine/controller.go b/pkg/controller/machine/controller.go
@@ -100,6 +100,9 @@ const (
 	// AnnotationAutoscalerIdentifier is used by the cluster-autoscaler
 	// cluster-api provider to match Nodes to Machines.
 	AnnotationAutoscalerIdentifier = "cluster.k8s.io/machine"
+
+	// ProviderID pattern.
+	ProviderIDPattern = "kubermatic://%s/%s"
 )
 
 // Reconciler is the controller implementation for machine resources.
@@ -478,7 +481,24 @@ func (r *Reconciler) reconcile(ctx context.Context, log *zap.SugaredLogger, mach
 		return r.ensureInstanceExistsForMachine(ctx, log, prov, machine, userdataPlugin, providerConfig)
 	}
 
-	// case 3.3: if the node exists make sure if it has labels and taints attached to it.
+	// case 3.3: if the node exists and both external and internal CCM are not available. Then set the provider-id for the node.
+	inTree, err := providerconfigtypes.IntreeCloudProviderImplementationSupported(providerConfig.CloudProvider, machine.Spec.Versions.Kubelet)
+	if err != nil {
+		return nil, fmt.Errorf("failed to check if cloud provider %q has in-tree implementation: %w", providerConfig.CloudProvider, err)
+	}
+
+	if !inTree && !r.nodeSettings.ExternalCloudProvider && node.Spec.ProviderID == "" {
+		providerID := fmt.Sprintf(ProviderIDPattern, providerConfig.CloudProvider, machine.UID)
+		if err := r.updateNode(ctx, node, func(n *corev1.Node) {
+			n.Spec.ProviderID = providerID
+		}); err != nil {
+			return nil, fmt.Errorf("failed to update node %s with the ProviderID: %w", node.Name, err)
+		}
+
+		r.recorder.Event(machine, corev1.EventTypeNormal, "ProviderIDUpdated", "Successfully updated providerID on node")
+		nodeLog.Info("Added ProviderID to the node")
+	}
+	// case 3.4: if the node exists make sure if it has labels and taints attached to it.
 	return nil, r.ensureNodeLabelsAnnotationsAndTaints(ctx, nodeLog, node, machine)
 }
 
@@ -978,10 +998,26 @@ func (r *Reconciler) ensureInstanceExistsForMachine(
 		return a.Type < b.Type
 	})
 
+	var providerID string
+	if machine.Spec.ProviderID == nil {
+		inTree, err := providerconfigtypes.IntreeCloudProviderImplementationSupported(providerConfig.CloudProvider, machine.Spec.Versions.Kubelet)
+		if err != nil {
+			return nil, fmt.Errorf("failed to check if cloud provider %q has in-tree implementation: %w", providerConfig.CloudProvider, err)
+		}
+
+		// If both external and internal CCM are not available. We set provider-id for the machine explicitly.
+		if !inTree && !r.nodeSettings.ExternalCloudProvider {
+			providerID = fmt.Sprintf(ProviderIDPattern, providerConfig.CloudProvider, machine.UID)
+		}
+	}
+
 	if err := r.updateMachine(machine, func(m *clusterv1alpha1.Machine) {
 		m.Status.Addresses = machineAddresses
+		if providerID != "" {
+			m.Spec.ProviderID = &providerID
+		}
 	}); err != nil {
-		return nil, fmt.Errorf("failed to update machine after setting .status.addresses: %w", err)
+		return nil, fmt.Errorf("failed to update machine after setting .status.addresses and providerID: %w", err)
 	}
 
 	return r.ensureNodeOwnerRef(ctx, log, providerInstance, machine, providerConfig)

diff --git a/pkg/providerconfig/types/types.go b/pkg/providerconfig/types/types.go
@@ -23,6 +23,8 @@ import (
 	"fmt"
 	"strconv"
 
+	"github.com/Masterminds/semver/v3"
+
 	clusterv1alpha1 "github.com/kubermatic/machine-controller/pkg/apis/cluster/v1alpha1"
 	"github.com/kubermatic/machine-controller/pkg/cloudprovider/util"
 	"github.com/kubermatic/machine-controller/pkg/jsonutil"
@@ -107,6 +109,27 @@ var (
 	}
 )
 
+func IntreeCloudProviderImplementationSupported(cloudProvider CloudProvider, version string) (inTree bool, err error) {
+	kubeletVer, err := semver.NewVersion(version)
+	if err != nil {
+		return false, fmt.Errorf("failed to parse kubelet version: %w", err)
+	}
+
+	switch cloudProvider {
+	case CloudProviderAzure, CloudProviderVsphere, CloudProviderGoogle:
+		return true, nil
+	case CloudProviderAWS:
+		// In-tree AWS support was removed in Kubernetes 1.27.
+		ltKube127Condition, _ := semver.NewConstraint("< 1.27")
+		if ltKube127Condition.Check(kubeletVer) {
+			return true, nil
+		}
+		return false, nil
+	default:
+		return false, nil
+	}
+}
+
 // DNSConfig contains a machine's DNS configuration.
 type DNSConfig struct {
 	Servers []string `json:"servers"`