Skip to content

Commit b5149b7

Browse files
AshleyDumaineAshleyDumaine
authored
[deprecation, improvement] deprecate LinodeMachine.spec.tags, only call updateInstance API if updating tags annotation, ensure unique instance tags (#775)
* deprecate LinodeMachine.spec.tags * save an update API call for tags, add integration test * use a set to deal with potential duplicate tag names * fix extra tagging
1 parent 767c255 commit b5149b7

8 files changed

+100
-74
lines changed

‎api/v1alpha2/linodemachine_types.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ type LinodeMachineSpec struct {
6565
BackupsEnabled bool `json:"backupsEnabled,omitempty"`
6666
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"
6767
PrivateIP *bool `json:"privateIP,omitempty"`
68+
// Deprecated: spec.tags is deprecated, use metadata.annotations.linode-vm-tags instead.
69+
// +kubebuilder:deprecatedversion:warning="spec.tags is deprecated, use metadata.annotations.linode-vm-tags instead"
6870
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"
6971
Tags []string `json:"tags,omitempty"`
7072
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"

‎config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachines.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,8 @@ spec:
363363
- message: Value is immutable
364364
rule: self == oldSelf
365365
tags:
366+
description: 'Deprecated: spec.tags is deprecated, use metadata.annotations.linode-vm-tags
367+
instead.'
366368
items:
367369
type: string
368370
type: array

‎config/crd/bases/infrastructure.cluster.x-k8s.io_linodemachinetemplates.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,8 @@ spec:
356356
- message: Value is immutable
357357
rule: self == oldSelf
358358
tags:
359+
description: 'Deprecated: spec.tags is deprecated, use metadata.annotations.linode-vm-tags
360+
instead.'
359361
items:
360362
type: string
361363
type: array

‎docs/src/reference/out.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ _Appears in:_
610610
| `interfaces` _[InstanceConfigInterfaceCreateOptions](#instanceconfiginterfacecreateoptions) array_ | | | |
611611
| `backupsEnabled` _boolean_ | | | |
612612
| `privateIP` _boolean_ | | | |
613-
| `tags` _string array_ | | | |
613+
| `tags` _string array_ | Deprecated: spec.tags is deprecated, use metadata.annotations.linode-vm-tags instead. | | |
614614
| `firewallID` _integer_ | | | |
615615
| `osDisk` _[InstanceDisk](#instancedisk)_ | OSDisk is configuration for the root disk that includes the OS,<br />if not specified this defaults to whatever space is not taken up by the DataDisks | | |
616616
| `dataDisks` _object (keys:string, values:[InstanceDisk](#instancedisk))_ | DataDisks is a map of any additional disks to add to an instance,<br />The sum of these disks + the OSDisk must not be more than allowed on a linodes plan | | |

‎internal/controller/linodemachine_controller.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"time"
2626

2727
"github.com/go-logr/logr"
28+
"github.com/google/go-cmp/cmp"
2829
"github.com/linode/linodego"
2930
corev1 "k8s.io/api/core/v1"
3031
apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -250,11 +251,6 @@ func (r *LinodeMachineReconciler) reconcile(ctx context.Context, logger logr.Log
250251
Reason: "BootstrapDataSecretReady", // We have to set the reason to not fail object patching
251252
})
252253

253-
// sync the tags on the machine with tags configured on machineTagsAnnotation.
254-
if err := reconcileTags(ctx, machineScope, logger); err != nil {
255-
return ctrl.Result{}, err
256-
}
257-
258254
// Update
259255
if machineScope.LinodeMachine.Status.InstanceState != nil {
260256
failureReason = util.UpdateError
@@ -751,10 +747,24 @@ func (r *LinodeMachineReconciler) reconcileUpdate(ctx context.Context, logger lo
751747
})
752748
}
753749

754-
// Clean up after instance creation.
750+
// update the tags if needed
751+
machineTags, err := getTags(machineScope)
752+
if err != nil {
753+
logger.Error(err, "Failed to get tags for Linode instance")
754+
return ctrl.Result{}, err
755+
}
756+
if cmp.Diff(machineTags, linodeInstance.Tags) != "" {
757+
_, err = machineScope.LinodeClient.UpdateInstance(ctx, instanceID, linodego.InstanceUpdateOptions{Tags: &machineTags})
758+
if err != nil {
759+
logger.Error(err, "Failed to update tags for Linode instance")
760+
return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerWaitForRunningDelay}, nil
761+
}
762+
}
763+
764+
// Clean up bootstrap data after instance creation.
755765
if linodeInstance.Status == linodego.InstanceRunning && machineScope.Machine.Status.Phase == "Running" {
756766
if err := deleteBootstrapData(ctx, machineScope); err != nil {
757-
logger.Error(err, "Fail to bootstrap data")
767+
logger.Error(err, "Fail to delete bootstrap data")
758768
}
759769
}
760770

‎internal/controller/linodemachine_controller_helpers.go‎

Lines changed: 35 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func retryIfTransient(err error, logger logr.Logger) (ctrl.Result, error) {
8080
return ctrl.Result{RequeueAfter: reconciler.DefaultMachineControllerRetryDelay}, nil
8181
}
8282

83-
func fillCreateConfig(createConfig *linodego.InstanceCreateOptions, machineScope *scope.MachineScope) error {
83+
func fillCreateConfig(createConfig *linodego.InstanceCreateOptions, machineScope *scope.MachineScope) {
8484
if machineScope.LinodeMachine.Spec.PrivateIP != nil {
8585
createConfig.PrivateIP = *machineScope.LinodeMachine.Spec.PrivateIP
8686
} else {
@@ -91,27 +91,6 @@ func fillCreateConfig(createConfig *linodego.InstanceCreateOptions, machineScope
9191
createConfig.Tags = []string{}
9292
}
9393

94-
if len(machineScope.LinodeMachine.Annotations) == 0 {
95-
machineScope.LinodeMachine.Annotations = make(map[string]string)
96-
}
97-
98-
// populate the tags into the machine-tags annotation.
99-
machineCreateTags, err := json.Marshal(createConfig.Tags)
100-
if err != nil {
101-
return fmt.Errorf("error in converting tags to string, %w", err)
102-
}
103-
machineScope.LinodeMachine.Annotations[machineTagsAnnotation] = string(machineCreateTags)
104-
105-
caplAutogenTags := util.GetAutoGenTags(*machineScope.LinodeCluster)
106-
createConfig.Tags = append(createConfig.Tags, caplAutogenTags...)
107-
108-
// populaate capl generated tags to another annotation.
109-
caplGenTags, err := json.Marshal(caplAutogenTags)
110-
if err != nil {
111-
return fmt.Errorf("error in converting name tag to string, %w", err)
112-
}
113-
machineScope.LinodeMachine.Annotations[machineCAPLTagsAnnotation] = string(caplGenTags)
114-
11594
if createConfig.Label == "" {
11695
createConfig.Label = machineScope.LinodeMachine.Name
11796
}
@@ -122,14 +101,17 @@ func fillCreateConfig(createConfig *linodego.InstanceCreateOptions, machineScope
122101
if createConfig.RootPass == "" {
123102
createConfig.RootPass = uuid.NewString()
124103
}
125-
126-
return nil
127104
}
128105

129106
func newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, gzipCompressionEnabled bool, logger logr.Logger) (*linodego.InstanceCreateOptions, error) {
130107
var err error
131108

132-
createConfig := linodeMachineSpecToInstanceCreateConfig(machineScope.LinodeMachine.Spec)
109+
machineTags, err := getTags(machineScope)
110+
if err != nil {
111+
logger.Error(err, "Failed to get machine tags")
112+
return nil, err
113+
}
114+
createConfig := linodeMachineSpecToInstanceCreateConfig(machineScope.LinodeMachine.Spec, machineTags)
133115
if createConfig == nil {
134116
err = errors.New("failed to convert machine spec to create instance config")
135117
logger.Error(err, "Panic! Struct of LinodeMachineSpec is different than InstanceCreateOptions")
@@ -141,9 +123,7 @@ func newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, gzip
141123
return nil, err
142124
}
143125

144-
if err := fillCreateConfig(createConfig, machineScope); err != nil {
145-
return nil, err
146-
}
126+
fillCreateConfig(createConfig, machineScope)
147127

148128
// Configure VPC interface if needed
149129
if err := configureVPCInterface(ctx, machineScope, createConfig, logger); err != nil {
@@ -578,7 +558,7 @@ func getVPCInterfaceConfigFromDirectID(ctx context.Context, machineScope *scope.
578558
}, nil
579559
}
580560

581-
func linodeMachineSpecToInstanceCreateConfig(machineSpec infrav1alpha2.LinodeMachineSpec) *linodego.InstanceCreateOptions {
561+
func linodeMachineSpecToInstanceCreateConfig(machineSpec infrav1alpha2.LinodeMachineSpec, machineTags []string) *linodego.InstanceCreateOptions {
582562
interfaces := make([]linodego.InstanceConfigInterfaceCreateOptions, len(machineSpec.Interfaces))
583563
for idx, iface := range machineSpec.Interfaces {
584564
interfaces[idx] = linodego.InstanceConfigInterfaceCreateOptions{
@@ -603,10 +583,9 @@ func linodeMachineSpecToInstanceCreateConfig(machineSpec infrav1alpha2.LinodeMac
603583
Image: machineSpec.Image,
604584
Interfaces: interfaces,
605585
PrivateIP: privateIP,
606-
Tags: machineSpec.Tags,
586+
Tags: machineTags,
607587
FirewallID: machineSpec.FirewallID,
608588
DiskEncryption: linodego.InstanceDiskEncryption(machineSpec.DiskEncryption),
609-
Group: machineSpec.Group,
610589
}
611590
}
612591

@@ -1020,42 +999,44 @@ func configureFirewall(ctx context.Context, machineScope *scope.MachineScope, cr
1020999
return nil
10211000
}
10221001

1023-
// updates tags on the linode whenever the tags annotation change.
1024-
func reconcileTags(ctx context.Context, machineScope *scope.MachineScope, logger logr.Logger) error {
1025-
if _, ok := machineScope.LinodeMachine.Annotations[machineTagsAnnotation]; !ok {
1026-
return nil
1027-
}
1028-
if machineScope.LinodeMachine.Spec.ProviderID == nil {
1029-
logger.Info("Skipping tags reconcile as ProviderID is not yet set for", "LinodeMachine", machineScope.LinodeMachine.Name)
1030-
return nil
1002+
// get tags on the linodemachine
1003+
func getTags(machineScope *scope.MachineScope) ([]string, error) {
1004+
if len(machineScope.LinodeMachine.Annotations) == 0 {
1005+
machineScope.LinodeMachine.Annotations = make(map[string]string)
10311006
}
10321007

1033-
var machineTags []string
1034-
if err := json.Unmarshal([]byte(machineScope.LinodeMachine.Annotations[machineTagsAnnotation]), &machineTags); err != nil {
1035-
return err
1008+
// add unique tags from the LinodeMachine linode-vm-tags annotation
1009+
machineTagSet := map[string]struct{}{}
1010+
if _, ok := machineScope.LinodeMachine.Annotations[machineTagsAnnotation]; ok {
1011+
var machineTags []string
1012+
if err := json.Unmarshal([]byte(machineScope.LinodeMachine.Annotations[machineTagsAnnotation]), &machineTags); err != nil {
1013+
return nil, err
1014+
}
1015+
for _, tag := range machineTags {
1016+
machineTagSet[tag] = struct{}{}
1017+
}
10361018
}
10371019

1020+
// add unique tags from the LinodeMachine linode-capl-vm-tags annotation
10381021
if _, ok := machineScope.LinodeMachine.Annotations[machineCAPLTagsAnnotation]; !ok {
10391022
caplGenTags, err := json.Marshal(util.GetAutoGenTags(*machineScope.LinodeCluster))
10401023
if err != nil {
1041-
return fmt.Errorf("error in converting name tag to string, %w", err)
1024+
return nil, fmt.Errorf("error in converting name tag to string, %w", err)
10421025
}
10431026
machineScope.LinodeMachine.Annotations[machineCAPLTagsAnnotation] = string(caplGenTags)
10441027
}
1045-
10461028
var caplAutogenTags []string
10471029
if err := json.Unmarshal([]byte(machineScope.LinodeMachine.Annotations[machineCAPLTagsAnnotation]), &caplAutogenTags); err != nil {
1048-
return err
1030+
return nil, err
10491031
}
1050-
1051-
machineTags = append(machineTags, caplAutogenTags...)
1052-
instanceID, err := util.GetInstanceID(machineScope.LinodeMachine.Spec.ProviderID)
1053-
if err != nil {
1054-
return err
1032+
for _, tag := range caplAutogenTags {
1033+
machineTagSet[tag] = struct{}{}
10551034
}
10561035

1057-
_, err = machineScope.LinodeClient.UpdateInstance(ctx, instanceID, linodego.InstanceUpdateOptions{
1058-
Tags: &machineTags,
1059-
})
1060-
return err
1036+
// use the set to create a slice of unique tags
1037+
machineTags := make([]string, 0, len(machineTagSet))
1038+
for tag := range machineTagSet {
1039+
machineTags = append(machineTags, tag)
1040+
}
1041+
return machineTags, nil
10611042
}

‎internal/controller/linodemachine_controller_helpers_test.go‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,9 @@ func TestLinodeMachineSpecToCreateInstanceConfig(t *testing.T) {
6161
},
6262
BackupsEnabled: true,
6363
PrivateIP: util.Pointer(true),
64-
Tags: []string{"tag"},
6564
}
6665

67-
createConfig := linodeMachineSpecToInstanceCreateConfig(machineSpec)
66+
createConfig := linodeMachineSpecToInstanceCreateConfig(machineSpec, []string{"tag"})
6867
assert.NotNil(t, createConfig, "Failed to convert LinodeMachineSpec to InstanceCreateOptions")
6968
}
7069

@@ -1230,15 +1229,11 @@ func TestReconcileTags(t *testing.T) {
12301229
},
12311230
},
12321231
},
1233-
mockSetup: func(mockLinodeClient *mock.MockLinodeClient) {
1234-
mockLinodeClient.EXPECT().UpdateInstance(gomock.Any(), gomock.Any(), linodego.InstanceUpdateOptions{Tags: &[]string{clusterName}}).Return(&linodego.Instance{}, nil)
1235-
},
1232+
mockSetup: func(mockLinodeClient *mock.MockLinodeClient) {},
12361233
},
12371234
{
1238-
name: "Success - Tag machines",
1239-
mockSetup: func(mockLinodeClient *mock.MockLinodeClient) {
1240-
mockLinodeClient.EXPECT().UpdateInstance(gomock.Any(), gomock.Any(), linodego.InstanceUpdateOptions{Tags: &[]string{"tag1", "tag2", "tag3", clusterName}}).Return(&linodego.Instance{}, nil)
1241-
},
1235+
name: "Success - Tag machines",
1236+
mockSetup: func(mockLinodeClient *mock.MockLinodeClient) {},
12421237
linodeMachine: &infrav1alpha2.LinodeMachine{
12431238
ObjectMeta: metav1.ObjectMeta{
12441239
Annotations: map[string]string{
@@ -1275,7 +1270,7 @@ func TestReconcileTags(t *testing.T) {
12751270
}
12761271

12771272
// Call the function being tested
1278-
err := reconcileTags(t.Context(), machineScope, testr.New(t))
1273+
_, err := getTags(machineScope)
12791274

12801275
// Check expectations
12811276
if tc.expectErrMsg != "" {

‎internal/controller/linodemachine_controller_test.go‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1391,6 +1391,7 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc
13911391
ID: 123,
13921392
IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))},
13931393
IPv6: "fd00::",
1394+
Tags: []string{"test-cluster-2"},
13941395
Status: linodego.InstanceOffline,
13951396
}, nil)
13961397
mck.LinodeClient.EXPECT().
@@ -1432,8 +1433,6 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc
14321433
ListInstanceConfigs(ctx, 123, gomock.Any()).
14331434
After(getAddrs).
14341435
Return(nil, &linodego.Error{Code: http.StatusTooManyRequests})
1435-
mck.LinodeClient.EXPECT().UpdateInstance(gomock.Any(), gomock.Any(), gomock.Any()).
1436-
Return(nil, nil)
14371436
res, err := reconciler.reconcile(ctx, mck.Logger(), mScope)
14381437
Expect(err).NotTo(HaveOccurred())
14391438
Expect(res.RequeueAfter).To(Equal(rutil.DefaultLinodeTooManyRequestsErrorRetryDelay))
@@ -1654,6 +1653,7 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"),
16541653
ID: 11111,
16551654
IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))},
16561655
IPv6: "fd00::",
1656+
Tags: []string{"test-cluster-2"},
16571657
Status: linodego.InstanceProvisioning,
16581658
Updated: util.Pointer(time.Now()),
16591659
}, nil)
@@ -1671,6 +1671,7 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"),
16711671
ID: 11111,
16721672
IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))},
16731673
IPv6: "fd00::",
1674+
Tags: []string{"test-cluster-2"},
16741675
Status: linodego.InstanceRunning,
16751676
Updated: util.Pointer(time.Now()),
16761677
}, nil)
@@ -1680,6 +1681,39 @@ var _ = Describe("machine-update", Ordered, Label("machine", "machine-update"),
16801681
Expect(rutil.ConditionTrue(linodeMachine, string(clusterv1.ReadyCondition))).To(BeTrue())
16811682
})),
16821683
),
1684+
Path(
1685+
Call("machine tag annotation is updated", func(ctx context.Context, mck Mock) {
1686+
mck.LinodeClient.EXPECT().GetInstance(ctx, 11111).Return(
1687+
&linodego.Instance{
1688+
ID: 11111,
1689+
IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))},
1690+
IPv6: "fd00::",
1691+
Tags: []string{"test-cluster-2"},
1692+
Status: linodego.InstanceRunning,
1693+
Updated: util.Pointer(time.Now()),
1694+
}, nil)
1695+
mck.LinodeClient.EXPECT().UpdateInstance(ctx, 11111, linodego.InstanceUpdateOptions{Tags: &[]string{"test-tag", "test-cluster-2"}}).Return(
1696+
&linodego.Instance{
1697+
ID: 11111,
1698+
IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))},
1699+
IPv6: "fd00::",
1700+
Tags: []string{"test-cluster-2", "test-tag"},
1701+
Status: linodego.InstanceRunning,
1702+
Updated: util.Pointer(time.Now()),
1703+
}, nil)
1704+
}),
1705+
Result("machine tag annotation is updated", func(ctx context.Context, mck Mock) {
1706+
linodeMachine.Spec.ProviderID = util.Pointer("linode://11111")
1707+
linodeMachine.Status.InstanceState = util.Pointer(linodego.InstanceRunning)
1708+
linodeMachine.Annotations = map[string]string{
1709+
machineTagsAnnotation: "[\"test-tag\"]",
1710+
}
1711+
_, err := reconciler.reconcile(ctx, logr.Logger{}, mScope)
1712+
Expect(err).NotTo(HaveOccurred())
1713+
Expect(linodeMachine.Annotations[machineTagsAnnotation]).To(Equal("[\"test-tag\"]"))
1714+
Expect(linodeMachine.Annotations[machineCAPLTagsAnnotation]).To(Equal("[\"test-cluster-2\"]"))
1715+
}),
1716+
),
16831717
)
16841718
})
16851719

0 commit comments

Comments
 (0)