The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/6317
This e-mail was sent by the LXC bot, direct replies will not reach the author
unless they happen to be subscribed to this list.
=== Description (from pull-request) ===
This one through me for a while, although there seems to be snapshot renaming support in `storagePoolVolumeTypePost` it appears to be unreachable via the API because snapshot renaming is now covered by the `storagePoolVolumeSnapshotTypeCmd` handler and specifically `storagePoolVolumeSnapshotTypePost` function.
This removes the misleading code and clarifies what the function can do.
From 9b3a8998daa65d79d5aa261dfa3983856d48112b Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 17 Oct 2019 10:30:34 +0100
Subject: [PATCH 1/4] lxc/storage/volume: Fix panic when invalid snapshot
rename argument supplied
When running
lxc storage volume rename test myvol6/snap0 myvol18
I got this crash:
panic: runtime error: index out of range
goroutine 1 [running]:
github.com/lxc/lxd/shared.ExtractSnapshotName(0x7fff387181b1, 0x7, 0xb512d6,
0x1)
/home/user/go/src/github.com/lxc/lxd/shared/util.go:487 +0x84
main.(*cmdStorageVolumeRename).Run(0xc42022ad60, 0xc4202a4f00, 0xc420288ae0,
0x3, 0x3, 0x0, 0x0)
/home/user/go/src/github.com/lxc/lxd/lxc/storage_volume.go:1217 +0x285
main.(*cmdStorageVolumeRename).Run-fm(0xc4202a4f00, 0xc420288ae0, 0x3, 0x3,
0x0, 0x0)
/home/user/go/src/github.com/lxc/lxd/lxc/storage_volume.go:1177 +0x52
github.com/spf13/cobra.(*Command).execute(0xc4202a4f00, 0xc420288a20, 0x3, 0x3,
0xc4202a4f00, 0xc420288a20)
/home/user/go/src/github.com/spf13/cobra/command.go:829 +0x468
github.com/spf13/cobra.(*Command).ExecuteC(0xc420208280, 0x0, 0xc420296d80,
0x19)
/home/user/go/src/github.com/spf13/cobra/command.go:917 +0x306
github.com/spf13/cobra.(*Command).Execute(0xc420208280, 0xc4200aa010, 0x6)
/home/user/go/src/github.com/spf13/cobra/command.go:867 +0x2b
main.main()
/home/user/go/src/github.com/lxc/lxd/lxc/main.go:238 +0x1de8
This is because the rename command is expecting the 2nd volume name in the
format:
lxc storage volume rename test myvol6/snap0 myvol6/newname
This fixes this issue, as using ExtractSnapshotName is unsafe if you've not
checked whether the volume name is a snapshot or not.
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxc/storage_volume.go | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lxc/storage_volume.go b/lxc/storage_volume.go
index 111ae58df2..9bdc70de5e 100644
--- a/lxc/storage_volume.go
+++ b/lxc/storage_volume.go
@@ -1214,7 +1214,13 @@ func (c *cmdStorageVolumeRename) Run(cmd *cobra.Command,
args []string) error {
if isSnapshot {
// Create the storage volume entry
vol := api.StorageVolumeSnapshotPost{}
- vol.Name = shared.ExtractSnapshotName(args[2])
+ _, dstSnapName, dstIsSnap :=
shared.ContainerGetParentAndSnapshotName(args[2])
+
+ if !dstIsSnap {
+ return fmt.Errorf("Invalid new snapshot name")
+ }
+
+ vol.Name = dstSnapName
// If a target member was specified, get the volume with the
matching
// name on that member, if any.
From c5e766b0a35dbd34ce513c34b0f8c0a85374754b Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 17 Oct 2019 10:32:42 +0100
Subject: [PATCH 2/4] shared/util: Removes ExtractSnapshotName
ExtractSnapshotName was dangerous to use as it did not validate the lenth of
the slice returned when splitting the supplied volume name by the snapshot
delimiter (/).
When used with untrusted inputs it could cause a crash.
Additionally it's functionality overlapped with the safer and more feature rich
ContainerGetParentAndSnapshotName, so have removed in place of that.
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
shared/util.go | 4 ----
1 file changed, 4 deletions(-)
diff --git a/shared/util.go b/shared/util.go
index 4bcfc30bbc..e307f985fe 100644
--- a/shared/util.go
+++ b/shared/util.go
@@ -483,10 +483,6 @@ func IsSnapshot(name string) bool {
return strings.Contains(name, SnapshotDelimiter)
}
-func ExtractSnapshotName(name string) string {
- return strings.SplitN(name, SnapshotDelimiter, 2)[1]
-}
-
func MkdirAllOwner(path string, perm os.FileMode, uid int, gid int) error {
// This function is a slightly modified version of MkdirAll from the Go
standard library.
// https://golang.org/src/os/path.go?s=488:535#L9
From ff178f7a248900ca41da6e7752e11a439c1debaf Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 17 Oct 2019 10:36:18 +0100
Subject: [PATCH 3/4] lxd: Changes use of ExtractSnapshotName to
ContainerGetParentAndSnapshotName
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/migrate_container.go | 3 ++-
lxd/migrate_storage_volumes.go | 5 +++--
lxd/storage_volumes_utils.go | 3 ++-
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/lxd/migrate_container.go b/lxd/migrate_container.go
index 1a0c094556..69b5d362ff 100644
--- a/lxd/migrate_container.go
+++ b/lxd/migrate_container.go
@@ -386,7 +386,8 @@ func (s *migrationSourceWs) Do(migrateOp
*operations.Operation) error {
if err == nil {
for _, snap := range fullSnaps {
snapshots = append(snapshots,
snapshotToProtobuf(snap))
- snapshotNames = append(snapshotNames,
shared.ExtractSnapshotName(snap.Name()))
+ _, snapName, _ :=
shared.ContainerGetParentAndSnapshotName(snap.Name())
+ snapshotNames = append(snapshotNames, snapName)
}
}
}
diff --git a/lxd/migrate_storage_volumes.go b/lxd/migrate_storage_volumes.go
index 05ba8e7186..1c492dedb2 100644
--- a/lxd/migrate_storage_volumes.go
+++ b/lxd/migrate_storage_volumes.go
@@ -69,7 +69,8 @@ func (s *migrationSourceWs) DoStorage(migrateOp
*operations.Operation) error {
}
snapshots = append(snapshots,
volumeSnapshotToProtobuf(snapVolume))
- snapshotNames = append(snapshotNames,
shared.ExtractSnapshotName(name))
+ _, snapName, _ :=
shared.ContainerGetParentAndSnapshotName(name)
+ snapshotNames = append(snapshotNames,
snapName)
}
}
@@ -429,7 +430,7 @@ func volumeSnapshotToProtobuf(vol *api.StorageVolume)
*migration.Snapshot {
config = append(config, &migration.Config{Key: &kCopy, Value:
&vCopy})
}
- snapOnlyName := shared.ExtractSnapshotName(vol.Name)
+ _, snapOnlyName, _ := shared.ContainerGetParentAndSnapshotName(vol.Name)
return &migration.Snapshot{
Name: &snapOnlyName,
diff --git a/lxd/storage_volumes_utils.go b/lxd/storage_volumes_utils.go
index 46eeb2fac8..aa02763399 100644
--- a/lxd/storage_volumes_utils.go
+++ b/lxd/storage_volumes_utils.go
@@ -647,7 +647,8 @@ func storagePoolVolumeCreateInternal(state *state.State,
poolName string, vol *a
}
for _, snap := range snapshots {
- _, err :=
storagePoolVolumeSnapshotCopyInternal(state, poolName, vol,
shared.ExtractSnapshotName(snap))
+ _, snapName, _ :=
shared.ContainerGetParentAndSnapshotName(snap)
+ _, err :=
storagePoolVolumeSnapshotCopyInternal(state, poolName, vol, snapName)
if err != nil {
return err
}
From 54f79c936e95001f999b63decfc82ac3b02c7206 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 17 Oct 2019 14:00:54 +0100
Subject: [PATCH 4/4] lxd/storage/volumes: Removes unused snapshot logic from
storagePoolVolumeTypePost
There appears to be old unused snapshot logic in storagePoolVolumeTypePost that
is misleading as in fact the
"storage-pools/{pool}/volumes/{type}/{name}/snapshots/{snapshotName}" route is
handled by storagePoolVolumeSnapshotTypeCmd handler now.
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage_volumes.go | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/lxd/storage_volumes.go b/lxd/storage_volumes.go
index 6f3dd1d1be..e224bbafd1 100644
--- a/lxd/storage_volumes.go
+++ b/lxd/storage_volumes.go
@@ -440,21 +440,13 @@ func doVolumeMigration(d *Daemon, poolName string, req
*api.StorageVolumesPost)
// /1.0/storage-pools/{name}/volumes/{type}/{name}
// Rename a storage volume of a given volume type in a given storage pool.
+// Also supports moving a storage volume between pools.
func storagePoolVolumeTypePost(d *Daemon, r *http.Request, volumeTypeName
string) response.Response {
// Get the name of the storage volume.
- var volumeName string
- fields := strings.Split(mux.Vars(r)["name"], "/")
+ volumeName := mux.Vars(r)["name"]
- if len(fields) == 3 && fields[1] == "snapshots" {
- // Handle volume snapshots
- volumeName = fmt.Sprintf("%s%s%s", fields[0],
shared.SnapshotDelimiter, fields[2])
- } else if len(fields) > 1 {
- volumeName = fmt.Sprintf("%s%s%s", fields[0],
shared.SnapshotDelimiter, fields[1])
- } else if len(fields) > 0 {
- // Handle volume
- volumeName = fields[0]
- } else {
- return response.BadRequest(fmt.Errorf("Invalid storage volume
%s", mux.Vars(r)["name"]))
+ if shared.IsSnapshot(volumeName) {
+ return response.BadRequest(fmt.Errorf("Invalid storage volume
%s", volumeName))
}
// Get the name of the storage pool the volume is supposed to be
@@ -474,7 +466,8 @@ func storagePoolVolumeTypePost(d *Daemon, r *http.Request,
volumeTypeName string
return response.BadRequest(fmt.Errorf("No name provided"))
}
- if strings.Contains(req.Name, "/") {
+ // Check requested new volume name is not a snapshot volume.
+ if shared.IsSnapshot(volumeName) {
return response.BadRequest(fmt.Errorf("Storage volume names may
not contain slashes"))
}
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
