Skip to content

Commit c7e72fd

Browse files
kangastakangasta
authored
fix(storage): do not segfault on nil backup rule (#328)
1 parent 2e0ddbe commit c7e72fd

3 files changed

Lines changed: 82 additions & 115 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- In `storage modify`, avoid segfault if the target storage does not have backup rule in the storage details. This would have happened, for example, when renaming private templates.
13+
1014
## [3.11.0] - 2024-07-23
1115

1216
### Added

‎internal/commands/storage/modify.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ func setBackupFields(storageUUID string, p modifyParams, exec commands.Executor,
9696
}
9797
}
9898

99-
if details.BackupRule.Time == "" {
99+
if details.BackupRule == nil || details.BackupRule.Time == "" {
100100
if newBUR != nil {
101101
if newBUR.Time == "" {
102102
return fmt.Errorf("backup-time is required")

‎internal/commands/storage/modify_test.go‎

Lines changed: 77 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -47,31 +47,48 @@ func TestModifyCommandExistingBackupRule(t *testing.T) {
4747
Storage: Storage2,
4848
BackupRule: &upcloud.BackupRule{Time: "", Interval: "", Retention: 0},
4949
}
50+
templateUUID := "015ed643-c712-4d26-bc48-b5740772b9c1"
51+
template := upcloud.Storage{
52+
UUID: templateUUID,
53+
Title: "test-template",
54+
Access: "private",
55+
State: "online",
56+
Type: "template",
57+
Zone: "fi-hel1",
58+
Size: 1,
59+
Tier: "standard",
60+
}
61+
templateDetails := upcloud.StorageDetails{
62+
Storage: template,
63+
}
5064

51-
for _, test1 := range []struct {
52-
name string
53-
args []string
54-
storage upcloud.Storage
55-
methodCalls int
56-
expected request.ModifyStorageRequest
57-
error string
65+
for _, test := range []struct {
66+
name string
67+
args []string
68+
storages []upcloud.Storage
69+
storageDetails upcloud.StorageDetails
70+
methodCalls int
71+
expected request.ModifyStorageRequest
72+
error string
5873
}{
5974
{
60-
name: "without backup rule update of existing backup rule",
61-
args: []string{"--size", "50"},
62-
storage: Storage1,
63-
methodCalls: 1,
75+
name: "without backup rule update of existing backup rule",
76+
args: []string{"--size", "50"},
77+
storageDetails: StorageDetails1,
78+
storages: []upcloud.Storage{Storage1},
79+
methodCalls: 1,
6480
expected: request.ModifyStorageRequest{
6581
UUID: Storage1.UUID,
6682
Size: 50,
6783
BackupRule: StorageDetails1.BackupRule,
6884
},
6985
},
7086
{
71-
name: "modifying existing backup rule without time",
72-
args: []string{"--size", "50", "--backup-interval", "mon"},
73-
storage: Storage1,
74-
methodCalls: 1,
87+
name: "modifying existing backup rule without time",
88+
args: []string{"--size", "50", "--backup-interval", "mon"},
89+
storageDetails: StorageDetails1,
90+
storages: []upcloud.Storage{Storage1},
91+
methodCalls: 1,
7592
expected: request.ModifyStorageRequest{
7693
UUID: Storage1.UUID,
7794
Size: 50,
@@ -82,46 +99,23 @@ func TestModifyCommandExistingBackupRule(t *testing.T) {
8299
},
83100
},
84101
},
85-
} {
86-
t.Run(test1.name, func(t *testing.T) {
87-
CachedStorages = nil
88-
conf := config.New()
89-
testCmd := ModifyCommand()
90-
mService := new(smock.Service)
91-
92-
mService.On("GetStorages").Return(&upcloud.Storages{Storages: []upcloud.Storage{Storage1}}, nil)
93-
expected := test1.expected
94-
mService.On(targetMethod, &expected).Return(&StorageDetails1, nil)
95-
mService.On("GetStorageDetails", &request.GetStorageDetailsRequest{UUID: Storage1.UUID}).Return(&StorageDetails1, nil)
96-
97-
c := commands.BuildCommand(testCmd, nil, conf)
98-
err := c.Cobra().Flags().Parse(test1.args)
99-
assert.NoError(t, err)
100-
101-
_, err = c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), test1.storage.UUID)
102-
103-
if test1.error != "" {
104-
assert.EqualError(t, err, test1.error)
105-
} else {
106-
assert.Nil(t, err)
107-
mService.AssertNumberOfCalls(t, targetMethod, test1.methodCalls)
108-
}
109-
})
110-
}
111-
112-
for _, test2 := range []struct {
113-
name string
114-
args []string
115-
storage upcloud.Storage
116-
methodCalls int
117-
expected request.ModifyStorageRequest
118-
error string
119-
}{
120102
{
121-
name: "modifying existing backup rule without time",
122-
args: []string{"--size", "50", "--backup-interval", "mon"},
123-
storage: Storage1,
124-
methodCalls: 1,
103+
name: "resizing template should not segfault",
104+
args: []string{"--size", "2"},
105+
storageDetails: templateDetails,
106+
storages: []upcloud.Storage{template},
107+
methodCalls: 1,
108+
expected: request.ModifyStorageRequest{
109+
UUID: templateDetails.UUID,
110+
Size: 2,
111+
},
112+
},
113+
{
114+
name: "modifying existing backup rule without time",
115+
args: []string{"--size", "50", "--backup-interval", "mon"},
116+
storageDetails: StorageDetails1,
117+
storages: []upcloud.Storage{Storage1},
118+
methodCalls: 1,
125119
expected: request.ModifyStorageRequest{
126120
UUID: Storage1.UUID,
127121
Size: 50,
@@ -132,56 +126,23 @@ func TestModifyCommandExistingBackupRule(t *testing.T) {
132126
},
133127
},
134128
},
135-
} {
136-
t.Run(test2.name, func(t *testing.T) {
137-
CachedStorages = nil
138-
conf := config.New()
139-
testCmd := ModifyCommand()
140-
mService := new(smock.Service)
141-
storages := upcloud.Storages{Storages: []upcloud.Storage{Storage1}}
142-
mService.On("GetStorages").Return(&storages, nil)
143-
expected := test2.expected
144-
mService.On(targetMethod, &expected).Return(&StorageDetails1, nil)
145-
mService.On("GetStorageDetails", &request.GetStorageDetailsRequest{UUID: Storage1.UUID}).Return(&StorageDetails1, nil)
146-
147-
c := commands.BuildCommand(testCmd, nil, config.New())
148-
err := c.Cobra().Flags().Parse(test2.args)
149-
assert.NoError(t, err)
150-
151-
_, err = c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), test2.storage.UUID)
152-
153-
if test2.error != "" {
154-
assert.EqualError(t, err, test2.error)
155-
} else {
156-
assert.Nil(t, err)
157-
mService.AssertNumberOfCalls(t, targetMethod, test2.methodCalls)
158-
}
159-
})
160-
}
161-
162-
for _, test3 := range []struct {
163-
name string
164-
args []string
165-
storage upcloud.Storage
166-
methodCalls int
167-
expected request.ModifyStorageRequest
168-
error string
169-
}{
170129
{
171-
name: "without backup rule update of non-existing backup rule",
172-
args: []string{"--size", "50"},
173-
storage: Storage2,
174-
methodCalls: 1,
130+
name: "without backup rule update of non-existing backup rule",
131+
args: []string{"--size", "50"},
132+
storageDetails: StorageDetails2,
133+
storages: []upcloud.Storage{Storage2},
134+
methodCalls: 1,
175135
expected: request.ModifyStorageRequest{
176136
UUID: Storage2.UUID,
177137
Size: 50,
178138
},
179139
},
180140
{
181-
name: "adding backup rule",
182-
args: []string{"--size", "50", "--backup-time", "12:00"},
183-
storage: Storage2,
184-
methodCalls: 1,
141+
name: "adding backup rule",
142+
args: []string{"--size", "50", "--backup-time", "12:00"},
143+
storageDetails: StorageDetails2,
144+
storages: []upcloud.Storage{Storage2},
145+
methodCalls: 1,
185146
expected: request.ModifyStorageRequest{
186147
UUID: Storage2.UUID,
187148
Size: 50,
@@ -193,35 +154,37 @@ func TestModifyCommandExistingBackupRule(t *testing.T) {
193154
},
194155
},
195156
{
196-
name: "adding backup rule without backup time",
197-
args: []string{"--size", "50", "--backup-retention", "10"},
198-
storage: Storage2,
199-
methodCalls: 1,
200-
error: "backup-time is required",
157+
name: "adding backup rule without backup time",
158+
args: []string{"--size", "50", "--backup-retention", "10"},
159+
storageDetails: StorageDetails2,
160+
storages: []upcloud.Storage{Storage2},
161+
methodCalls: 1,
162+
error: "backup-time is required",
201163
},
202164
} {
203-
t.Run(test3.name, func(t *testing.T) {
165+
t.Run(test.name, func(t *testing.T) {
204166
CachedStorages = nil
205167
conf := config.New()
206168
testCmd := ModifyCommand()
207169
mService := new(smock.Service)
208-
storages := upcloud.Storages{Storages: []upcloud.Storage{Storage2}}
209-
mService.On("GetStorages").Return(&storages, nil)
210-
expected := test3.expected
211-
mService.On(targetMethod, &expected).Return(&StorageDetails2, nil)
212-
mService.On("GetStorageDetails", &request.GetStorageDetailsRequest{UUID: Storage2.UUID}).Return(&StorageDetails2, nil)
213170

214-
c := commands.BuildCommand(testCmd, nil, config.New())
215-
err := c.Cobra().Flags().Parse(test3.args)
171+
mService.On("GetStorages").Return(&upcloud.Storages{Storages: test.storages}, nil)
172+
expected := test.expected
173+
storageDetails := test.storageDetails
174+
mService.On(targetMethod, &expected).Return(&storageDetails, nil)
175+
mService.On("GetStorageDetails", &request.GetStorageDetailsRequest{UUID: test.storageDetails.UUID}).Return(&storageDetails, nil)
176+
177+
c := commands.BuildCommand(testCmd, nil, conf)
178+
err := c.Cobra().Flags().Parse(test.args)
216179
assert.NoError(t, err)
217180

218-
_, err = c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), test3.storage.UUID)
181+
_, err = c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), test.storageDetails.UUID)
219182

220-
if test3.error != "" {
221-
assert.EqualError(t, err, test3.error)
183+
if test.error != "" {
184+
assert.EqualError(t, err, test.error)
222185
} else {
223186
assert.Nil(t, err)
224-
mService.AssertNumberOfCalls(t, targetMethod, test3.methodCalls)
187+
mService.AssertNumberOfCalls(t, targetMethod, test.methodCalls)
225188
}
226189
})
227190
}

0 commit comments

Comments
 (0)