Skip to content

Commit

Permalink
csi: volume deregistration should require exact ID (#11852)
Browse files Browse the repository at this point in the history
The command line client sends a specific volume ID, but this isn't
enforced at the API level and we were incorrectly using a prefix match
for volume deregistration, resulting in cases where a volume with a
shorter ID that's a prefix of another volume would be deregistered
instead of the intended volume.
  • Loading branch information
tgross authored and lgfa29 committed Jan 17, 2022
1 parent 2813ba1 commit 1a3a8ff
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 4 deletions.
3 changes: 3 additions & 0 deletions .changelog/11852.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
csi: Fixed a bug where deregistering volumes would attempt to deregister the wrong volume if the ID was a prefix of the intended volume
```
2 changes: 1 addition & 1 deletion nomad/state/state_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -2323,7 +2323,7 @@ func (s *StateStore) CSIVolumeDeregister(index uint64, namespace string, ids []s
defer txn.Abort()

for _, id := range ids {
existing, err := txn.First("csi_volumes", "id_prefix", namespace, id)
existing, err := txn.First("csi_volumes", "id", namespace, id)
if err != nil {
return fmt.Errorf("volume lookup failed: %s: %v", id, err)
}
Expand Down
11 changes: 8 additions & 3 deletions nomad/state/state_store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2990,16 +2990,21 @@ func TestStateStore_CSIVolume(t *testing.T) {
// registration is an error when the volume is in use
index++
err = state.CSIVolumeRegister(index, []*structs.CSIVolume{v0})
require.Error(t, err, fmt.Sprintf("volume exists: %s", vol0))
require.Error(t, err, "volume re-registered while in use")
// as is deregistration
index++
err = state.CSIVolumeDeregister(index, ns, []string{vol0}, false)
require.Error(t, err, fmt.Sprintf("volume in use: %s", vol0))
require.Error(t, err, "volume deregistered while in use")

// even if forced, because we have a non-terminal claim
index++
err = state.CSIVolumeDeregister(index, ns, []string{vol0}, true)
require.Error(t, err, fmt.Sprintf("volume in use: %s", vol0))
require.Error(t, err, "volume force deregistered while in use")

// we use the ID, not a prefix
index++
err = state.CSIVolumeDeregister(index, ns, []string{"fo"}, true)
require.Error(t, err, "volume deregistered by prefix")

// release claims to unblock deregister
index++
Expand Down

0 comments on commit 1a3a8ff

Please sign in to comment.