-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
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
dcfbe7d
to
5566a16
Compare
There was a problem hiding this 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.
if len(other.Context) != 0 { | ||
v.Context = other.Context | ||
} |
There was a problem hiding this comment.
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 register
s 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.
There was a problem hiding this comment.
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
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
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
andvolume 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
After
Contributor Checklist
changelog entry using the
make cl
command.ensure regressions will be caught.
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
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.