Skip to content

Commit

Permalink
CSI: don't overwrite context with empty value from request (#24922)
Browse files Browse the repository at this point in the history
When a volume is updated, we merge the new definition to the old. But the
volume's context comes from the plugin and is likely not present in the user's
volume specification. Which means that if the user re-submits the volume
specification to make an adjustment to the volume, we wipe out the context field
which might be required for subsequent operations by the CSI plugin. This was
discovered to be a problem with the Terraform provider and fixed there, but it's
also a problem for users of the `volume create` and `volume register` commands.

Update the merge so that we only overwrite the value of the context if it's been
explictly set by the user. We still need to support user-driven updates to
context for the `volume register` workflow.

Ref: hashicorp/terraform-provider-nomad#503
Fixes: democratic-csi/democratic-csi#438
  • Loading branch information
tgross authored Jan 23, 2025
1 parent 5befea6 commit c1dc9ed
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 3 deletions.
3 changes: 3 additions & 0 deletions .changelog/24922.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
csi: Fixed a bug where volume context from the plugin would be erased on volume updates
```
10 changes: 7 additions & 3 deletions nomad/structs/csi.go
Original file line number Diff line number Diff line change
Expand Up @@ -846,9 +846,13 @@ func (v *CSIVolume) Merge(other *CSIVolume) error {
"volume parameters cannot be updated"))
}

// Context is mutable and will be used during controller
// validation
v.Context = other.Context
// Context is mutable and will be used during controller validation, but we
// need to ensure we don't remove context that's been previously stored
// server-side if the user has submitted an update without adding it to the
// spec manually (which we should not require)
if len(other.Context) != 0 {
v.Context = other.Context
}
return errs.ErrorOrNil()
}

Expand Down
15 changes: 15 additions & 0 deletions nomad/structs/csi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,13 @@ func TestCSIVolume_Merge(t *testing.T) {
{Segments: map[string]string{"rack": "R2"}},
},
},
Context: map[string]string{
// a typical context for democratic-csi
"provisioner_driver": "nfs-client",
"node_attach_driver": "nfs",
"server": "192.168.1.170",
"share": "/srv/nfs_data/v/csi-volume-nfs",
},
},
update: &CSIVolume{
Topologies: []*CSITopology{
Expand Down Expand Up @@ -745,6 +752,14 @@ func TestCSIVolume_Merge(t *testing.T) {
test.Sprint("should add 2 requested capabilities"))
test.Eq(t, []string{"noatime", "another"}, v.MountOptions.MountFlags,
test.Sprint("should add mount flag"))
test.Eq(t, map[string]string{
"provisioner_driver": "nfs-client",
"node_attach_driver": "nfs",
"server": "192.168.1.170",
"share": "/srv/nfs_data/v/csi-volume-nfs",
}, v.Context,
test.Sprint("context should not be overwritten with empty update"))

},
},
}
Expand Down

0 comments on commit c1dc9ed

Please sign in to comment.