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.

Sign up for GitHub

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

Jump to bottom

CSI: remove prefix matching from CSIVolumeByID and fix CLI prefix matching #10158

Merged
merged 3 commits into from
Mar 18, 2021
Merged

CSI: remove prefix matching from CSIVolumeByID and fix CLI prefix matching #10158

merged 3 commits into from
Mar 18, 2021

Conversation

tgross
Copy link
Member

@tgross tgross commented Mar 10, 2021 •
edited
Loading

Callers of CSIVolumeByID are generally assuming they should receive a single volume. Among other possible bugs, this potentially results in feasibility checking being performed against the wrong volume if a volume's ID is a prefix substring of other volume. Discovered this while working on #10136 because of "test" vs "test[0]"

But removing the incorrect prefix matching from CSIVolumeByID breaks prefix matching in the command line client. Add the required elements for prefix matching back to the commands and API.

@vercel vercel bot temporarily deployed to Preview – nomad March 10, 2021 19:38 Inactive
tgross
tgross commented Mar 10, 2021
View reviewed changes
api/csi.go
@@ -28,11 +28,6 @@ func (v *CSIVolumes) List(q *QueryOptions) ([]*CSIVolumeListStub, *QueryMeta, er
return resp, qm, nil
}

// PluginList returns all CSI volumes for the specified plugin id
func (v *CSIVolumes) PluginList(pluginID string) ([]*CSIVolumeListStub, *QueryMeta, error) {
return v.List(&QueryOptions{Prefix: pluginID})
Copy link
Member Author

@tgross tgross Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

CSIVolumes.List has always been how you get this info, and this code has never worked. It has no callers either, so I'm removing here... I can't imagine this will break anyone's code given that it didn't work. But tagging @cgbaker as a reviewer to see if he has a different opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me; we're not using it in the terraform provider, just using List

@tgross tgross added this to the 1.1.0 milestone Mar 10, 2021
@tgross tgross changed the title CSI: remove prefix matching from CSIVolumeByID and fix prefix matching CSI: remove prefix matching from CSIVolumeByID and fix CLI prefix matching Mar 10, 2021
@tgross tgross marked this pull request as ready for review March 10, 2021 19:54
@tgross
csi: CSIVolumeByID should not use prefix query
97bb912 
Callers of `CSIVolumeByID` are generally assuming they should receive a single
volume. This potentially results in feasibility checking being performed
against the wrong volume if a volume's ID is a prefix substring of other
volume (for example: "test" and "testing").
@tgross
csi: reimplement prefix matching
25782f3 
Removing the incorrect prefix matching from `CSIVolumeByID` breaks prefix
matching in the command line client. Add the required elements for prefix
matching to the commands and API.
@vercel vercel bot temporarily deployed to Preview – nomad March 10, 2021 20:21 Inactive
@tgross tgross mentioned this pull request Mar 10, 2021
Merged
tgross added a commit that referenced this pull request Mar 10, 2021
@tgross
remove prefix matching [from PR GH-10158]
d44d62e
@tgross tgross self-assigned this Mar 12, 2021
cgbaker
cgbaker approved these changes Mar 17, 2021
View reviewed changes
Copy link
Contributor

@cgbaker cgbaker left a comment

Choose a reason for hiding this comment

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

a couple of questions about blocking queries and CLI handling when prefix search returns no matches.

api/csi.go
@@ -28,11 +28,6 @@ func (v *CSIVolumes) List(q *QueryOptions) ([]*CSIVolumeListStub, *QueryMeta, er
return resp, qm, nil
}

// PluginList returns all CSI volumes for the specified plugin id
func (v *CSIVolumes) PluginList(pluginID string) ([]*CSIVolumeListStub, *QueryMeta, error) {
return v.List(&QueryOptions{Prefix: pluginID})
Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me; we're not using it in the terraform provider, just using List

nomad/csi_endpoint.go Outdated Show resolved Hide resolved
nomad/state/state_store.go Outdated Show resolved Hide resolved
nomad/state/state_store.go Outdated Show resolved Hide resolved
command/volume_deregister.go
@@ -91,6 +93,24 @@ func (c *VolumeDeregisterCommand) Run(args []string) int {
return 1
}

// Prefix search for the volume
Copy link
Contributor

Choose a reason for hiding this comment

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

🏆 these are so helpful to have...

command/volume_deregister.go Show resolved Hide resolved
command/volume_detach.go Show resolved Hide resolved
command/volume_status_csi.go Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – nomad March 18, 2021 12:16 Inactive
@tgross tgross merged commit a1eaad9 into main Mar 18, 2021
@tgross tgross deleted the b-csi-prefix-search branch March 18, 2021 18:32
@tgross tgross mentioned this pull request Mar 22, 2021
Closed
tgross added a commit that referenced this pull request Mar 30, 2021
@tgross
changelog: add section for backports from 1.1.0 to 1.0.5
32fc593 
* Added missing changlog entry for GH-10145
* Fixed misclassified entry for GH-10158
@tgross tgross mentioned this pull request Mar 30, 2021
Merged
tgross added a commit that referenced this pull request Mar 30, 2021
@tgross
changelog: add section for backports from 1.1.0 to 1.0.5
5cc5d80 
* Fixed order
* Added missing changlog entry for GH-10145
* Fixed misclassified entry for GH-10158
tgross added a commit that referenced this pull request Mar 30, 2021
@tgross
changelog: add section for backports from 1.1.0 to 1.0.5
f1645dd 
* Fixed order
* Added missing changlog entry for GH-10145
* Fixed misclassified entry for GH-10158
schmichael pushed a commit that referenced this pull request May 14, 2021
@tgross @schmichael
CSI: remove prefix matching from CSIVolumeByID and fix CLI prefix mat…
a904779 
…ching (#10158)

Callers of `CSIVolumeByID` are generally assuming they should receive a single
volume. This potentially results in feasibility checking being performed
against the wrong volume if a volume's ID is a prefix substring of other
volume (for example: "test" and "testing").

Removing the incorrect prefix matching from `CSIVolumeByID` breaks prefix
matching in the command line client. Add the required elements for prefix
matching to the commands and API.
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cgbaker cgbaker cgbaker approved these changes

@shoenig shoenig Awaiting requested review from shoenig

@notnoop notnoop Awaiting requested review from notnoop

Assignees

@tgross tgross

Labels
theme/storage type/bug
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tgross @cgbaker