From 98b4e795feb4b90b5243588b82414eed32919a68 Mon Sep 17 00:00:00 2001 From: Ashley Dumaine <5779804+AshleyDumaine@users.noreply.github.com> Date: Tue, 20 Aug 2024 09:25:35 -0400 Subject: [PATCH] deprecate ProviderID (#469) --- api/v1alpha1/linodemachine_conversion_test.go | 4 - .../linodemachinetemplate_conversion_test.go | 4 - api/v1alpha2/linodemachine_types.go | 3 +- cloud/services/domains_test.go | 34 ++++---- cloud/services/loadbalancers.go | 18 +++- cloud/services/loadbalancers_test.go | 18 ++-- ...cture.cluster.x-k8s.io_linodemachines.yaml | 4 +- controller/linodemachine_controller.go | 35 +++++--- controller/linodemachine_controller_test.go | 82 +++++++++++++------ util/helpers.go | 15 ++++ util/helpers_test.go | 38 +++++++++ 11 files changed, 176 insertions(+), 79 deletions(-) diff --git a/api/v1alpha1/linodemachine_conversion_test.go b/api/v1alpha1/linodemachine_conversion_test.go index 8cd49f34e..91181de6d 100644 --- a/api/v1alpha1/linodemachine_conversion_test.go +++ b/api/v1alpha1/linodemachine_conversion_test.go @@ -40,7 +40,6 @@ func TestLinodeMachineConvertTo(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-machine"}, Spec: LinodeMachineSpec{ ProviderID: ptr.To("linode://1234"), - InstanceID: ptr.To(1234), Region: "us-mia", Type: "g6-standard-2", Group: "", @@ -82,7 +81,6 @@ func TestLinodeMachineConvertTo(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-machine"}, Spec: infrav1alpha2.LinodeMachineSpec{ ProviderID: ptr.To("linode://1234"), - InstanceID: ptr.To(1234), Region: "us-mia", Type: "g6-standard-2", Group: "", @@ -168,7 +166,6 @@ func TestLinodeMachineConvertFrom(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-machine"}, Spec: infrav1alpha2.LinodeMachineSpec{ ProviderID: ptr.To("linode://1234"), - InstanceID: ptr.To(1234), Region: "us-mia", Type: "g6-standard-2", Group: "", @@ -215,7 +212,6 @@ func TestLinodeMachineConvertFrom(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "test-machine"}, Spec: LinodeMachineSpec{ ProviderID: ptr.To("linode://1234"), - InstanceID: ptr.To(1234), Region: "us-mia", Type: "g6-standard-2", Group: "", diff --git a/api/v1alpha1/linodemachinetemplate_conversion_test.go b/api/v1alpha1/linodemachinetemplate_conversion_test.go index f08b1b6d6..c64ec1ea2 100644 --- a/api/v1alpha1/linodemachinetemplate_conversion_test.go +++ b/api/v1alpha1/linodemachinetemplate_conversion_test.go @@ -42,7 +42,6 @@ func TestLinodeMachineTemplateConvertTo(t *testing.T) { Template: LinodeMachineTemplateResource{ Spec: LinodeMachineSpec{ ProviderID: ptr.To("linode://1234"), - InstanceID: ptr.To(1234), Region: "us-mia", Type: "g6-standard-2", Group: "", @@ -87,7 +86,6 @@ func TestLinodeMachineTemplateConvertTo(t *testing.T) { Template: infrav1alpha2.LinodeMachineTemplateResource{ Spec: infrav1alpha2.LinodeMachineSpec{ ProviderID: ptr.To("linode://1234"), - InstanceID: ptr.To(1234), Region: "us-mia", Type: "g6-standard-2", Group: "", @@ -176,7 +174,6 @@ func TestLinodeMachineTemplateConvertFrom(t *testing.T) { Template: infrav1alpha2.LinodeMachineTemplateResource{ Spec: infrav1alpha2.LinodeMachineSpec{ ProviderID: ptr.To("linode://1234"), - InstanceID: ptr.To(1234), Region: "us-mia", Type: "g6-standard-2", Group: "", @@ -221,7 +218,6 @@ func TestLinodeMachineTemplateConvertFrom(t *testing.T) { Template: LinodeMachineTemplateResource{ Spec: LinodeMachineSpec{ ProviderID: ptr.To("linode://1234"), - InstanceID: ptr.To(1234), Region: "us-mia", Type: "g6-standard-2", Group: "", diff --git a/api/v1alpha2/linodemachine_types.go b/api/v1alpha2/linodemachine_types.go index fc3f0d856..7e27f82a8 100644 --- a/api/v1alpha2/linodemachine_types.go +++ b/api/v1alpha2/linodemachine_types.go @@ -38,6 +38,7 @@ type LinodeMachineSpec struct { ProviderID *string `json:"providerID,omitempty"` // InstanceID is the Linode instance ID for this machine. // +optional + // +kubebuilder:deprecatedversion:warning="ProviderID deprecates InstanceID" InstanceID *int `json:"instanceID,omitempty"` // +kubebuilder:validation:Required @@ -215,7 +216,7 @@ type LinodeMachineStatus struct { // +kubebuilder:printcolumn:name="Cluster",type="string",JSONPath=".metadata.labels.cluster\\.x-k8s\\.io/cluster-name",description="Cluster to which this LinodeMachine belongs" // +kubebuilder:printcolumn:name="State",type="string",JSONPath=".status.instanceState",description="Linode instance state" // +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.ready",description="Machine ready status" -// +kubebuilder:printcolumn:name="InstanceID",type="string",JSONPath=".spec.providerID",description="Linode instance ID" +// +kubebuilder:printcolumn:name="ProviderID",type="string",JSONPath=".spec.providerID",description="Provider ID" // +kubebuilder:printcolumn:name="Machine",type="string",JSONPath=".metadata.ownerReferences[?(@.kind==\"Machine\")].name",description="Machine object which owns with this LinodeMachine" // LinodeMachine is the Schema for the linodemachines API diff --git a/cloud/services/domains_test.go b/cloud/services/domains_test.go index 199789a80..5edebd9a0 100644 --- a/cloud/services/domains_test.go +++ b/cloud/services/domains_test.go @@ -66,7 +66,7 @@ func TestAddIPToEdgeDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -129,7 +129,7 @@ func TestAddIPToEdgeDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -228,7 +228,7 @@ func TestRemoveIPFromEdgeDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -298,7 +298,7 @@ func TestRemoveIPFromEdgeDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -398,7 +398,7 @@ func TestAddIPToDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -472,7 +472,7 @@ func TestAddIPToDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -545,7 +545,7 @@ func TestAddIPToDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -613,7 +613,7 @@ func TestAddIPToDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -687,7 +687,7 @@ func TestAddIPToDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -754,7 +754,7 @@ func TestAddIPToDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: nil, @@ -811,7 +811,7 @@ func TestAddIPToDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -916,7 +916,7 @@ func TestDeleteIPFromDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -991,7 +991,7 @@ func TestDeleteIPFromDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -1066,7 +1066,7 @@ func TestDeleteIPFromDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, }, @@ -1113,7 +1113,7 @@ func TestDeleteIPFromDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -1174,7 +1174,7 @@ func TestDeleteIPFromDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ @@ -1240,7 +1240,7 @@ func TestDeleteIPFromDNS(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, Status: infrav1alpha2.LinodeMachineStatus{ Addresses: []clusterv1.MachineAddress{ diff --git a/cloud/services/loadbalancers.go b/cloud/services/loadbalancers.go index ee2cf81f9..a545a3974 100644 --- a/cloud/services/loadbalancers.go +++ b/cloud/services/loadbalancers.go @@ -122,8 +122,13 @@ func AddNodeToNB( return nil } + instanceID, err := util.GetInstanceID(machineScope.LinodeMachine.Spec.ProviderID) + if err != nil { + logger.Error(err, "Failed to parse instance ID from provider ID") + return err + } // Get the private IP that was assigned - addresses, err := machineScope.LinodeClient.GetInstanceIPAddresses(ctx, *machineScope.LinodeMachine.Spec.InstanceID) + addresses, err := machineScope.LinodeClient.GetInstanceIPAddresses(ctx, instanceID) if err != nil { logger.Error(err, "Failed get instance IP addresses") @@ -200,11 +205,16 @@ func DeleteNodeFromNB( return nil } - err := machineScope.LinodeClient.DeleteNodeBalancerNode( + instanceID, err := util.GetInstanceID(machineScope.LinodeMachine.Spec.ProviderID) + if err != nil { + logger.Error(err, "Failed to parse instance ID from provider ID") + return err + } + err = machineScope.LinodeClient.DeleteNodeBalancerNode( ctx, *machineScope.LinodeCluster.Spec.Network.NodeBalancerID, *machineScope.LinodeCluster.Spec.Network.ApiserverNodeBalancerConfigID, - *machineScope.LinodeMachine.Spec.InstanceID, + instanceID, ) if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { logger.Error(err, "Failed to update Node Balancer") @@ -217,7 +227,7 @@ func DeleteNodeFromNB( ctx, *machineScope.LinodeCluster.Spec.Network.NodeBalancerID, *portConfig.NodeBalancerConfigID, - *machineScope.LinodeMachine.Spec.InstanceID, + instanceID, ) if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { logger.Error(err, "Failed to update Node Balancer") diff --git a/cloud/services/loadbalancers_test.go b/cloud/services/loadbalancers_test.go index cedd17277..28ca3f1b0 100644 --- a/cloud/services/loadbalancers_test.go +++ b/cloud/services/loadbalancers_test.go @@ -450,7 +450,7 @@ func TestAddNodeToNBConditions(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, }, @@ -488,7 +488,7 @@ func TestAddNodeToNBConditions(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, }, @@ -522,7 +522,7 @@ func TestAddNodeToNBConditions(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, }, @@ -631,7 +631,7 @@ func TestAddNodeToNBFullWorkflow(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, }, @@ -687,7 +687,7 @@ func TestAddNodeToNBFullWorkflow(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, }, @@ -782,7 +782,7 @@ func TestDeleteNodeFromNB(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, LinodeCluster: &infrav1alpha2.LinodeCluster{ @@ -818,7 +818,7 @@ func TestDeleteNodeFromNB(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, LinodeCluster: &infrav1alpha2.LinodeCluster{ @@ -867,7 +867,7 @@ func TestDeleteNodeFromNB(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, LinodeCluster: &infrav1alpha2.LinodeCluster{ @@ -910,7 +910,7 @@ func TestDeleteNodeFromNB(t *testing.T) { UID: "test-uid", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(123), + ProviderID: ptr.To("linode://123"), }, }, LinodeCluster: &infrav1alpha2.LinodeCluster{ diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml index 258158e2e..fc38c501d 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml @@ -399,9 +399,9 @@ spec: jsonPath: .status.ready name: Ready type: string - - description: Linode instance ID + - description: Provider ID jsonPath: .spec.providerID - name: InstanceID + name: ProviderID type: string - description: Machine object which owns with this LinodeMachine jsonPath: .metadata.ownerReferences[?(@.kind=="Machine")].name diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go index eecee4dc3..939858a94 100644 --- a/controller/linodemachine_controller.go +++ b/controller/linodemachine_controller.go @@ -286,9 +286,18 @@ func (r *LinodeMachineReconciler) reconcileCreate( } tags := []string{machineScope.LinodeCluster.Name} + var instanceID *int + if machineScope.LinodeMachine.Spec.ProviderID != nil { + instID, err := util.GetInstanceID(machineScope.LinodeMachine.Spec.ProviderID) + if err != nil { + logger.Error(err, "Failed to parse instance ID from provider ID") + return ctrl.Result{}, err + } + instanceID = util.Pointer(instID) + } listFilter := util.Filter{ - ID: machineScope.LinodeMachine.Spec.InstanceID, + ID: instanceID, Label: machineScope.LinodeMachine.Name, Tags: tags, } @@ -342,7 +351,7 @@ func (r *LinodeMachineReconciler) reconcileCreate( } conditions.MarkTrue(machineScope.LinodeMachine, ConditionPreflightCreated) - machineScope.LinodeMachine.Spec.InstanceID = &linodeInstance.ID + machineScope.LinodeMachine.Spec.ProviderID = util.Pointer(fmt.Sprintf("linode://%d", linodeInstance.ID)) return r.reconcileInstanceCreate(ctx, logger, machineScope, linodeInstance) } @@ -445,8 +454,6 @@ func (r *LinodeMachineReconciler) reconcileInstanceCreate( conditions.MarkTrue(machineScope.LinodeMachine, ConditionPreflightLoadBalancing) } - machineScope.LinodeMachine.Spec.ProviderID = util.Pointer(fmt.Sprintf("linode://%d", linodeInstance.ID)) - // Set the instance state to signal preflight process is done machineScope.LinodeMachine.Status.InstanceState = util.Pointer(linodego.InstanceOffline) @@ -667,11 +674,13 @@ func (r *LinodeMachineReconciler) reconcileUpdate( res = ctrl.Result{} - if machineScope.LinodeMachine.Spec.InstanceID == nil { - return res, nil, errors.New("missing instance ID") + instanceID, err := util.GetInstanceID(machineScope.LinodeMachine.Spec.ProviderID) + if err != nil { + logger.Error(err, "Failed to parse instance ID from provider ID") + return ctrl.Result{}, nil, err } - if linodeInstance, err = machineScope.LinodeClient.GetInstance(ctx, *machineScope.LinodeMachine.Spec.InstanceID); err != nil { + if linodeInstance, err = machineScope.LinodeClient.GetInstance(ctx, instanceID); err != nil { if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { logger.Error(err, "Failed to get Linode machine instance") @@ -681,7 +690,6 @@ func (r *LinodeMachineReconciler) reconcileUpdate( // Create new machine machineScope.LinodeMachine.Spec.ProviderID = nil - machineScope.LinodeMachine.Spec.InstanceID = nil machineScope.LinodeMachine.Status.InstanceState = nil machineScope.LinodeMachine.Status.Conditions = nil @@ -726,7 +734,7 @@ func (r *LinodeMachineReconciler) reconcileDelete( ) (ctrl.Result, error) { logger.Info("deleting machine") - if machineScope.LinodeMachine.Spec.InstanceID == nil { + if machineScope.LinodeMachine.Spec.ProviderID == nil { logger.Info("Machine ID is missing, nothing to do") if err := machineScope.RemoveCredentialsRefFinalizer(ctx); err != nil { @@ -747,7 +755,13 @@ func (r *LinodeMachineReconciler) reconcileDelete( return ctrl.Result{}, fmt.Errorf("Failed to remove linodecluster finalizer %w", err) } - if err := machineScope.LinodeClient.DeleteInstance(ctx, *machineScope.LinodeMachine.Spec.InstanceID); err != nil { + instanceID, err := util.GetInstanceID(machineScope.LinodeMachine.Spec.ProviderID) + if err != nil { + logger.Error(err, "Failed to parse instance ID from provider ID") + return ctrl.Result{}, err + } + + if err := machineScope.LinodeClient.DeleteInstance(ctx, instanceID); err != nil { if util.IgnoreLinodeAPIError(err, http.StatusNotFound) != nil { logger.Error(err, "Failed to delete Linode instance") @@ -766,7 +780,6 @@ func (r *LinodeMachineReconciler) reconcileDelete( r.Recorder.Event(machineScope.LinodeMachine, corev1.EventTypeNormal, clusterv1.DeletedReason, "instance has cleaned up") machineScope.LinodeMachine.Spec.ProviderID = nil - machineScope.LinodeMachine.Spec.InstanceID = nil machineScope.LinodeMachine.Status.InstanceState = nil if err := machineScope.RemoveCredentialsRefFinalizer(ctx); err != nil { diff --git a/controller/linodemachine_controller_test.go b/controller/linodemachine_controller_test.go index 579c9c89c..9a7041e44 100644 --- a/controller/linodemachine_controller_test.go +++ b/controller/linodemachine_controller_test.go @@ -41,6 +41,7 @@ import ( infrav1alpha2 "github.com/linode/cluster-api-provider-linode/api/v1alpha2" "github.com/linode/cluster-api-provider-linode/cloud/scope" "github.com/linode/cluster-api-provider-linode/mock" + "github.com/linode/cluster-api-provider-linode/util" rutil "github.com/linode/cluster-api-provider-linode/util/reconciler" . "github.com/linode/cluster-api-provider-linode/mock/mocktest" @@ -112,7 +113,7 @@ var _ = Describe("create", Label("machine", "create"), func() { UID: "12345", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(0), + ProviderID: ptr.To("linode://123"), Type: "g6-nanode-1", Image: rutil.DefaultMachineControllerLinodeImage, DiskEncryption: string(linodego.InstanceDiskEncryptionEnabled), @@ -213,7 +214,6 @@ var _ = Describe("create", Label("machine", "create"), func() { Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightReady)).To(BeTrue()) Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) - Expect(*linodeMachine.Spec.InstanceID).To(Equal(123)) Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ {Type: clusterv1.MachineExternalIP, Address: "172.0.0.2"}, @@ -464,7 +464,6 @@ var _ = Describe("create", Label("machine", "create"), func() { Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightBootTriggered)).To(BeTrue()) Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightReady)).To(BeTrue()) - Expect(*linodeMachine.Spec.InstanceID).To(Equal(123)) Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ {Type: clusterv1.MachineExternalIP, Address: "172.0.0.2"}, @@ -652,7 +651,6 @@ var _ = Describe("create", Label("machine", "create"), func() { Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightReady)).To(BeTrue()) Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) - Expect(*linodeMachine.Spec.InstanceID).To(Equal(123)) Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ {Type: clusterv1.MachineExternalIP, Address: "172.0.0.2"}, @@ -727,7 +725,7 @@ var _ = Describe("createDNS", Label("machine", "createDNS"), func() { UID: "12345", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(0), + ProviderID: ptr.To("linode://0"), Type: "g6-nanode-1", Image: rutil.DefaultMachineControllerLinodeImage, }, @@ -828,7 +826,6 @@ var _ = Describe("createDNS", Label("machine", "createDNS"), func() { Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightReady)).To(BeTrue()) Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) - Expect(*linodeMachine.Spec.InstanceID).To(Equal(123)) Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ {Type: clusterv1.MachineExternalIP, Address: "172.0.0.2"}, @@ -859,7 +856,7 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc linodeMachine := &infrav1alpha2.LinodeMachine{ ObjectMeta: metadata, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(0), + ProviderID: ptr.To("linode://0"), Type: "g6-nanode-1", Image: rutil.DefaultMachineControllerLinodeImage, Configuration: &infrav1alpha2.InstanceConfiguration{Kernel: "test"}, @@ -950,23 +947,30 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc OneOf( Path( Call("machine is not created because there was an error creating instance", func(ctx context.Context, mck Mock) { - listInst := mck.LinodeClient.EXPECT(). - ListInstances(ctx, gomock.Any()). - Return([]linodego.Instance{}, nil) - getRegion := mck.LinodeClient.EXPECT(). - GetRegion(ctx, gomock.Any()). - After(listInst). - Return(&linodego.Region{Capabilities: []string{"Metadata"}}, nil) - getImage := mck.LinodeClient.EXPECT(). - GetImage(ctx, gomock.Any()). - After(getRegion). - Return(&linodego.Image{Capabilities: []string{"cloud-init"}}, nil) - mck.LinodeClient.EXPECT().CreateInstance(gomock.Any(), gomock.Any()). - After(getImage). - Return(nil, errors.New("failed to ensure instance")) }), OneOf( + Path(Result("create error", func(ctx context.Context, mck Mock) { + linodeMachine.Spec.ProviderID = util.Pointer("linode://foo") + _, err := reconciler.reconcile(ctx, mck.Logger(), mScope) + Expect(err).To(HaveOccurred()) + Expect(mck.Logs()).To(ContainSubstring("Failed to parse instance ID from provider ID")) + })), Path(Result("create requeues", func(ctx context.Context, mck Mock) { + linodeMachine.Spec.ProviderID = util.Pointer("linode://123") + listInst := mck.LinodeClient.EXPECT(). + ListInstances(ctx, gomock.Any()). + Return([]linodego.Instance{}, nil) + getRegion := mck.LinodeClient.EXPECT(). + GetRegion(ctx, gomock.Any()). + After(listInst). + Return(&linodego.Region{Capabilities: []string{"Metadata"}}, nil) + getImage := mck.LinodeClient.EXPECT(). + GetImage(ctx, gomock.Any()). + After(getRegion). + Return(&linodego.Image{Capabilities: []string{"cloud-init"}}, nil) + mck.LinodeClient.EXPECT().CreateInstance(gomock.Any(), gomock.Any()). + After(getImage). + Return(nil, errors.New("failed to ensure instance")) res, err := reconciler.reconcile(ctx, mck.Logger(), mScope) Expect(err).NotTo(HaveOccurred()) Expect(res.RequeueAfter).To(Equal(rutil.DefaultMachineControllerWaitForRunningDelay)) @@ -975,6 +979,20 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc Path(Result("create machine error - timeout error", func(ctx context.Context, mck Mock) { tempTimeout := reconciler.ReconcileTimeout reconciler.ReconcileTimeout = time.Nanosecond + listInst := mck.LinodeClient.EXPECT(). + ListInstances(ctx, gomock.Any()). + Return([]linodego.Instance{}, nil) + getRegion := mck.LinodeClient.EXPECT(). + GetRegion(ctx, gomock.Any()). + After(listInst). + Return(&linodego.Region{Capabilities: []string{"Metadata"}}, nil) + getImage := mck.LinodeClient.EXPECT(). + GetImage(ctx, gomock.Any()). + After(getRegion). + Return(&linodego.Image{Capabilities: []string{"cloud-init"}}, nil) + mck.LinodeClient.EXPECT().CreateInstance(gomock.Any(), gomock.Any()). + After(getImage). + Return(nil, errors.New("failed to ensure instance")) _, err := reconciler.reconcile(ctx, mck.Logger(), mScope) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("failed to ensure instance")) @@ -1147,7 +1165,6 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightReady)).To(BeTrue()) Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) - Expect(*linodeMachine.Spec.InstanceID).To(Equal(123)) Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ {Type: clusterv1.MachineExternalIP, Address: "172.0.0.2"}, @@ -1185,11 +1202,11 @@ var _ = Describe("machine-delete", Ordered, Label("machine", "machine-delete"), Network: infrav1alpha2.NetworkSpec{}, }, } - instanceID := 12345 + providerID := "linode://12345" linodeMachine := &infrav1alpha2.LinodeMachine{ ObjectMeta: metadata, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: &instanceID, + ProviderID: &providerID, }, } machine := &clusterv1.Machine{ @@ -1235,11 +1252,20 @@ var _ = Describe("machine-delete", Ordered, Label("machine", "machine-delete"), OneOf( Path( Call("machine is not deleted because there was an error deleting instance", func(ctx context.Context, mck Mock) { - mck.LinodeClient.EXPECT().DeleteInstance(gomock.Any(), gomock.Any()). - Return(errors.New("failed to delete instance")) }), OneOf( + Path(Result("delete error", func(ctx context.Context, mck Mock) { + tmpProviderID := linodeMachine.Spec.ProviderID + linodeMachine.Spec.ProviderID = util.Pointer("linode://foo") + _, err := reconciler.reconcileDelete(ctx, mck.Logger(), mScope) + Expect(err).To(HaveOccurred()) + Expect(mck.Logs()).To(ContainSubstring("Failed to parse instance ID from provider ID")) + linodeMachine.Spec.ProviderID = tmpProviderID + + })), Path(Result("delete requeues", func(ctx context.Context, mck Mock) { + mck.LinodeClient.EXPECT().DeleteInstance(gomock.Any(), gomock.Any()). + Return(errors.New("failed to delete instance")) res, err := reconciler.reconcileDelete(ctx, mck.Logger(), mScope) Expect(err).NotTo(HaveOccurred()) Expect(res.RequeueAfter).To(Equal(rutil.DefaultMachineControllerRetryDelay)) @@ -1248,6 +1274,8 @@ var _ = Describe("machine-delete", Ordered, Label("machine", "machine-delete"), Path(Result("create machine error - timeout error", func(ctx context.Context, mck Mock) { tempTimeout := reconciler.ReconcileTimeout reconciler.ReconcileTimeout = time.Nanosecond + mck.LinodeClient.EXPECT().DeleteInstance(gomock.Any(), gomock.Any()). + Return(errors.New("failed to delete instance")) _, err := reconciler.reconcileDelete(ctx, mck.Logger(), mScope) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("failed to delete instance")) @@ -1367,7 +1395,7 @@ var _ = Describe("machine in PlacementGroup", Label("machine", "placementGroup") UID: "12345", }, Spec: infrav1alpha2.LinodeMachineSpec{ - InstanceID: ptr.To(0), + ProviderID: ptr.To("linode://0"), Type: "g6-nanode-1", Image: rutil.DefaultMachineControllerLinodeImage, PlacementGroupRef: &corev1.ObjectReference{ diff --git a/util/helpers.go b/util/helpers.go index 76cf480f6..cc81cb0cf 100644 --- a/util/helpers.go +++ b/util/helpers.go @@ -5,6 +5,8 @@ import ( "io" "net/http" "os" + "strconv" + "strings" "github.com/linode/linodego" ) @@ -46,3 +48,16 @@ func IsRetryableError(err error) bool { http.StatusServiceUnavailable, linodego.ErrorFromError) || errors.Is(err, http.ErrHandlerTimeout) || errors.Is(err, os.ErrDeadlineExceeded) || errors.Is(err, io.ErrUnexpectedEOF) } + +// GetInstanceID determines the instance ID from the ProviderID +func GetInstanceID(providerID *string) (int, error) { + if providerID == nil { + err := errors.New("nil ProviderID") + return -1, err + } + instanceID, err := strconv.Atoi(strings.TrimPrefix(*providerID, "linode://")) + if err != nil { + return -1, err + } + return instanceID, nil +} diff --git a/util/helpers_test.go b/util/helpers_test.go index de084ffc8..7cc58fa85 100644 --- a/util/helpers_test.go +++ b/util/helpers_test.go @@ -89,3 +89,41 @@ func TestIsRetryableError(t *testing.T) { }) } } + +func TestGetInstanceID(t *testing.T) { + t.Parallel() + tests := []struct { + name string + providerID *string + wantErr bool + wantID int + }{{ + name: "nil", + providerID: nil, + wantErr: true, + wantID: -1, + }, { + name: "invalid provider ID", + providerID: Pointer("linode://foobar"), + wantErr: true, + wantID: -1, + }, { + name: "valid", + providerID: Pointer("linode://12345"), + wantErr: false, + wantID: 12345, + }} + for _, tt := range tests { + testcase := tt + t.Run(testcase.name, func(t *testing.T) { + t.Parallel() + gotID, err := GetInstanceID(testcase.providerID) + if testcase.wantErr && err == nil { + t.Errorf("wanted %v, got %v", testcase.wantErr, err) + } + if gotID != testcase.wantID { + t.Errorf("wanted %v, got %v", testcase.wantID, gotID) + } + }) + } +}