Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(storage): do not segfault on nil backup rule #328

Merged
merged 1 commit into from
Aug 12, 2024
Merged

fix(storage): do not segfault on nil backup rule #328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- 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.

## [3.11.0] - 2024-07-23

### Added
Expand Down
2 changes: 1 addition & 1 deletion internal/commands/storage/modify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func setBackupFields(storageUUID string, p modifyParams, exec commands.Executor,
}
}

if details.BackupRule.Time == "" {
if details.BackupRule == nil || details.BackupRule.Time == "" {
if newBUR != nil {
if newBUR.Time == "" {
return fmt.Errorf("backup-time is required")
Expand Down
191 changes: 77 additions & 114 deletions internal/commands/storage/modify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,31 +47,48 @@ func TestModifyCommandExistingBackupRule(t *testing.T) {
Storage: Storage2,
BackupRule: &upcloud.BackupRule{Time: "", Interval: "", Retention: 0},
}
templateUUID := "015ed643-c712-4d26-bc48-b5740772b9c1"
template := upcloud.Storage{
UUID: templateUUID,
Title: "test-template",
Access: "private",
State: "online",
Type: "template",
Zone: "fi-hel1",
Size: 1,
Tier: "standard",
}
templateDetails := upcloud.StorageDetails{
Storage: template,
}

for _, test1 := range []struct {
name string
args []string
storage upcloud.Storage
methodCalls int
expected request.ModifyStorageRequest
error string
for _, test := range []struct {
name string
args []string
storages []upcloud.Storage
storageDetails upcloud.StorageDetails
methodCalls int
expected request.ModifyStorageRequest
error string
}{
{
name: "without backup rule update of existing backup rule",
args: []string{"--size", "50"},
storage: Storage1,
methodCalls: 1,
name: "without backup rule update of existing backup rule",
args: []string{"--size", "50"},
storageDetails: StorageDetails1,
storages: []upcloud.Storage{Storage1},
methodCalls: 1,
expected: request.ModifyStorageRequest{
UUID: Storage1.UUID,
Size: 50,
BackupRule: StorageDetails1.BackupRule,
},
},
{
name: "modifying existing backup rule without time",
args: []string{"--size", "50", "--backup-interval", "mon"},
storage: Storage1,
methodCalls: 1,
name: "modifying existing backup rule without time",
args: []string{"--size", "50", "--backup-interval", "mon"},
storageDetails: StorageDetails1,
storages: []upcloud.Storage{Storage1},
methodCalls: 1,
expected: request.ModifyStorageRequest{
UUID: Storage1.UUID,
Size: 50,
Expand All @@ -82,46 +99,23 @@ func TestModifyCommandExistingBackupRule(t *testing.T) {
},
},
},
} {
t.Run(test1.name, func(t *testing.T) {
CachedStorages = nil
conf := config.New()
testCmd := ModifyCommand()
mService := new(smock.Service)

mService.On("GetStorages").Return(&upcloud.Storages{Storages: []upcloud.Storage{Storage1}}, nil)
expected := test1.expected
mService.On(targetMethod, &expected).Return(&StorageDetails1, nil)
mService.On("GetStorageDetails", &request.GetStorageDetailsRequest{UUID: Storage1.UUID}).Return(&StorageDetails1, nil)

c := commands.BuildCommand(testCmd, nil, conf)
err := c.Cobra().Flags().Parse(test1.args)
assert.NoError(t, err)

_, err = c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), test1.storage.UUID)

if test1.error != "" {
assert.EqualError(t, err, test1.error)
} else {
assert.Nil(t, err)
mService.AssertNumberOfCalls(t, targetMethod, test1.methodCalls)
}
})
}

for _, test2 := range []struct {
name string
args []string
storage upcloud.Storage
methodCalls int
expected request.ModifyStorageRequest
error string
}{
{
name: "modifying existing backup rule without time",
args: []string{"--size", "50", "--backup-interval", "mon"},
storage: Storage1,
methodCalls: 1,
name: "resizing template should not segfault",
args: []string{"--size", "2"},
storageDetails: templateDetails,
storages: []upcloud.Storage{template},
methodCalls: 1,
expected: request.ModifyStorageRequest{
UUID: templateDetails.UUID,
Size: 2,
},
},
{
name: "modifying existing backup rule without time",
args: []string{"--size", "50", "--backup-interval", "mon"},
storageDetails: StorageDetails1,
storages: []upcloud.Storage{Storage1},
methodCalls: 1,
expected: request.ModifyStorageRequest{
UUID: Storage1.UUID,
Size: 50,
Expand All @@ -132,56 +126,23 @@ func TestModifyCommandExistingBackupRule(t *testing.T) {
},
},
},
} {
t.Run(test2.name, func(t *testing.T) {
CachedStorages = nil
conf := config.New()
testCmd := ModifyCommand()
mService := new(smock.Service)
storages := upcloud.Storages{Storages: []upcloud.Storage{Storage1}}
mService.On("GetStorages").Return(&storages, nil)
expected := test2.expected
mService.On(targetMethod, &expected).Return(&StorageDetails1, nil)
mService.On("GetStorageDetails", &request.GetStorageDetailsRequest{UUID: Storage1.UUID}).Return(&StorageDetails1, nil)

c := commands.BuildCommand(testCmd, nil, config.New())
err := c.Cobra().Flags().Parse(test2.args)
assert.NoError(t, err)

_, err = c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), test2.storage.UUID)

if test2.error != "" {
assert.EqualError(t, err, test2.error)
} else {
assert.Nil(t, err)
mService.AssertNumberOfCalls(t, targetMethod, test2.methodCalls)
}
})
}

for _, test3 := range []struct {
name string
args []string
storage upcloud.Storage
methodCalls int
expected request.ModifyStorageRequest
error string
}{
{
name: "without backup rule update of non-existing backup rule",
args: []string{"--size", "50"},
storage: Storage2,
methodCalls: 1,
name: "without backup rule update of non-existing backup rule",
args: []string{"--size", "50"},
storageDetails: StorageDetails2,
storages: []upcloud.Storage{Storage2},
methodCalls: 1,
expected: request.ModifyStorageRequest{
UUID: Storage2.UUID,
Size: 50,
},
},
{
name: "adding backup rule",
args: []string{"--size", "50", "--backup-time", "12:00"},
storage: Storage2,
methodCalls: 1,
name: "adding backup rule",
args: []string{"--size", "50", "--backup-time", "12:00"},
storageDetails: StorageDetails2,
storages: []upcloud.Storage{Storage2},
methodCalls: 1,
expected: request.ModifyStorageRequest{
UUID: Storage2.UUID,
Size: 50,
Expand All @@ -193,35 +154,37 @@ func TestModifyCommandExistingBackupRule(t *testing.T) {
},
},
{
name: "adding backup rule without backup time",
args: []string{"--size", "50", "--backup-retention", "10"},
storage: Storage2,
methodCalls: 1,
error: "backup-time is required",
name: "adding backup rule without backup time",
args: []string{"--size", "50", "--backup-retention", "10"},
storageDetails: StorageDetails2,
storages: []upcloud.Storage{Storage2},
methodCalls: 1,
error: "backup-time is required",
},
} {
t.Run(test3.name, func(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
CachedStorages = nil
conf := config.New()
testCmd := ModifyCommand()
mService := new(smock.Service)
storages := upcloud.Storages{Storages: []upcloud.Storage{Storage2}}
mService.On("GetStorages").Return(&storages, nil)
expected := test3.expected
mService.On(targetMethod, &expected).Return(&StorageDetails2, nil)
mService.On("GetStorageDetails", &request.GetStorageDetailsRequest{UUID: Storage2.UUID}).Return(&StorageDetails2, nil)

c := commands.BuildCommand(testCmd, nil, config.New())
err := c.Cobra().Flags().Parse(test3.args)
mService.On("GetStorages").Return(&upcloud.Storages{Storages: test.storages}, nil)
expected := test.expected
storageDetails := test.storageDetails
mService.On(targetMethod, &expected).Return(&storageDetails, nil)
mService.On("GetStorageDetails", &request.GetStorageDetailsRequest{UUID: test.storageDetails.UUID}).Return(&storageDetails, nil)

c := commands.BuildCommand(testCmd, nil, conf)
err := c.Cobra().Flags().Parse(test.args)
assert.NoError(t, err)

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

if test3.error != "" {
assert.EqualError(t, err, test3.error)
if test.error != "" {
assert.EqualError(t, err, test.error)
} else {
assert.Nil(t, err)
mService.AssertNumberOfCalls(t, targetMethod, test3.methodCalls)
mService.AssertNumberOfCalls(t, targetMethod, test.methodCalls)
}
})
}
Expand Down
Loading