From 5566a16840cce92aa115928defc0bf3dbdeb78f5 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 23 Jan 2025 10:11:45 -0500 Subject: [PATCH] CSI: don't overwrite context with empty value from request 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: https://github.com/hashicorp/terraform-provider-nomad/pull/503 Fixes: https://github.com/democratic-csi/democratic-csi/issues/438 --- .changelog/24922.txt | 3 +++ nomad/structs/csi.go | 10 +++++++--- nomad/structs/csi_test.go | 15 +++++++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 .changelog/24922.txt diff --git a/.changelog/24922.txt b/.changelog/24922.txt new file mode 100644 index 00000000000..bbf1374bd3d --- /dev/null +++ b/.changelog/24922.txt @@ -0,0 +1,3 @@ +```release-note:bug +csi: Fixed a bug where volume context from the plugin would be erased on volume updates +``` diff --git a/nomad/structs/csi.go b/nomad/structs/csi.go index 8dd85db766e..4296b67c8c3 100644 --- a/nomad/structs/csi.go +++ b/nomad/structs/csi.go @@ -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() } diff --git a/nomad/structs/csi_test.go b/nomad/structs/csi_test.go index 9069d6e8b29..a2a087eddc9 100644 --- a/nomad/structs/csi_test.go +++ b/nomad/structs/csi_test.go @@ -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{ @@ -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")) + }, }, }