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.

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

CSI: don't overwrite context with empty value from request #24922

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

tgross
Copy link
Member

@tgross tgross commented Jan 23, 2025

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

Testing & Reproduction steps

Using the Democratic CSI plugin.

Before

$ nomad volume create ./volume.hcl
Created external volume csi-volume-nfs with ID csi-volume-nfs

$ nomad operator api "/v1/volume/csi/csi-volume-nfs?namespace=prod" | jq .Context
{
  "provisioner_driver": "nfs-client",
  "node_attach_driver": "nfs",
  "server": "192.168.1.170",
  "share": "/srv/nfs_data/v/csi-volume-nfs"
}

$ nomad volume create ./volume.hcl
Created external volume csi-volume-nfs with ID csi-volume-nfs

$ nomad operator api "/v1/volume/csi/csi-volume-nfs?namespace=prod" | jq .Context
{}

After

$ nomad volume create ./volume.hcl
Created external volume csi-volume-nfs with ID csi-volume-nfs

$ nomad operator api "/v1/volume/csi/csi-volume-nfs?namespace=prod" | jq .Context
{
  "server": "192.168.1.170",
  "share": "/srv/nfs_data/v/csi-volume-nfs",
  "provisioner_driver": "nfs-client",
  "node_attach_driver": "nfs"
}

$ nomad volume create ./volume.hcl
Created external volume csi-volume-nfs with ID csi-volume-nfs

$ nomad operator api "/v1/volume/csi/csi-volume-nfs?namespace=prod" | jq .Context
{
  "provisioner_driver": "nfs-client",
  "node_attach_driver": "nfs",
  "server": "192.168.1.170",
  "share": "/srv/nfs_data/v/csi-volume-nfs"
}

Contributor Checklist

  • Changelog Entry If this PR changes user-facing behavior, please generate and add a
    changelog entry using the make cl command.
  • Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
    ensure regressions will be caught.
  • Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
    and job configuration, please update the Nomad website documentation to reflect this. Refer to
    the website README for docs guidelines. Please also consider whether the
    change requires notes within the upgrade guide.

Reviewer Checklist

  • Backport Labels Please add the correct backport labels as described by the internal
    backporting document.
  • Commit Type Ensure the correct merge method is selected which should be "squash and merge"
    in the majority of situations. The main exceptions are long-lived feature branches or merges where
    history should be preserved.
  • Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
    within the public repository.

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
@tgross tgross force-pushed the b-csi-context-overwrite branch from dcfbe7d to 5566a16 Compare January 23, 2025 15:30
@tgross tgross added this to the 1.9.x milestone Jan 23, 2025
@tgross tgross added backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent backport/1.9.x backport to 1.9.x release line labels Jan 23, 2025
@tgross tgross marked this pull request as ready for review January 23, 2025 15:34
@tgross tgross requested review from a team as code owners January 23, 2025 15:34
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! I have one imaginary edge case to consider.

Comment on lines +853 to +855
if len(other.Context) != 0 {
v.Context = other.Context
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes good sense for create, where the controller plugin generates the context dynamically, but I wonder about this edge case:

what if a user registers a volume with some context, but later wants to remove the context entirely? is that even a plausible concern? I suppose one could deregister the volume then re-register it with no context, since the actual volume out in the world would be unaffected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only time that would happen is if they updated the plugin and the new version of the plugin requires no context. In that case, you're almost certainly better off deregistering and re-registering

@tgross tgross merged commit c1dc9ed into main Jan 23, 2025
45 checks passed
@tgross tgross deleted the b-csi-context-overwrite branch January 23, 2025 19:06
tgross added a commit that referenced this pull request Jan 23, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent backport/1.9.x backport to 1.9.x release line theme/storage type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DCSI with NFS on Nomad - unknown/unsupported node_attach_driver: undefined
2 participants