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: Add secrets flag support for delete volume #11245

Merged
merged 2 commits into from
Apr 5, 2022

Conversation

ggriffiths
Copy link
Contributor

@ggriffiths ggriffiths commented Sep 30, 2021

Fixes #10997
Signed-off-by: Grant Griffiths ggriffiths@purestorage.com

@ggriffiths
Copy link
Contributor Author

ggriffiths commented Sep 30, 2021

Tested this on my cluster. The scenario is that a PX volume has been created with a token that expires in one minute. Deleting will fail after a minute because this token is expired,

[root@ip-70-0-139-170 nomadspecs]# nomad volume delete pxvol1 
Error deleting volume: Unexpected response code: 500 (controller delete volume: rpc error: controller delete volume: CSI.ControllerDeleteVolume: controller plugin returned an internal error, check the plugin allocation logs for more information: rpc error: code = Internal desc = Unable to delete volume with id 282299434038939129: rpc error: code = PermissionDenied desc = Token is expired)

Because the stored token has expired as seen above, we must generate a new token:

[root@ip-70-0-139-170 nomadspecs]# /opt/pwx/bin/pxctl auth token generate --auth-config admin.yaml --shared-secret mysecret --issuer openstorage.io --token-duration=1m
eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImdyYW50QG9wZW5zdG9yYWdlLmlvIiwiZXhwIjoxNjMyOTY2OTgzLCJncm91cHMiOlsiKiJdLCJpYXQiOjE2MzI5NjY5MjMsImlzcyI6Im9wZW5zdG9yYWdlLmlvIiwibmFtZSI6IkdyYW50Iiwicm9sZXMiOlsic3lzdGVtLmFkbWluIl0sInN1YiI6ImdyYW50QG9wZW5zdG9yYWdlLmlvIn0.g4qvXrq8kfDL1BhmKr5YYvInXwwZROLbW58XRm_uEtM

After that, pass it to the volume delete command:

[root@ip-70-0-139-170 nomadspecs]# nomad volume delete --secrets=auth-token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJlbWFpbCI6ImdyYW50QG9wZW5zdG9yYWdlLmlvIiwiZXhwIjoxNjMyOTY2OTgzLCJncm91cHMiOlsiKiJdLCJpYXQiOjE2MzI5NjY5MjMsImlzcyI6Im9wZW5zdG9yYWdlLmlvIiwibmFtZSI6IkdyYW50Iiwicm9sZXMiOlsic3lzdGVtLmFkbWluIl0sInN1YiI6ImdyYW50QG9wZW5zdG9yYWdlLmlvIn0.g4qvXrq8kfDL1BhmKr5YYvInXwwZROLbW58XRm_uEtM pxvol1
Successfully deleted volume "pxvol1"!

@ggriffiths
Copy link
Contributor Author

A few flakey tests seem to be failing, but I think that's unrelated. Ready for review

@lgfa29 lgfa29 self-requested a review October 2, 2021 02:32
@lgfa29
Copy link
Contributor

lgfa29 commented Oct 2, 2021

Thank you @ggriffiths! I review this soon 🙂

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

The PR looks good @ggriffiths.

Some minor comments, but the bigger blocker is on me to have the Nomad API accept and pass the secrets as HTTP headers instead of query params, as this is a safer option.

I will let you know once that PR is up and then you can adjust your work here 🙂

command/volume_delete.go Show resolved Hide resolved
api/csi.go Outdated Show resolved Hide resolved
command/volume_delete.go Outdated Show resolved Hide resolved
website/content/docs/commands/volume/delete.mdx Outdated Show resolved Hide resolved
@tgross
Copy link
Member

tgross commented Mar 1, 2022

@ggriffiths I've just merged #12144 which has that change to use HTTP headers. If you'd like to rebase this PR on main I'd be happy to help get it pushed thru review. Or if you'd like I can carry the PR from here and get it merged in.

@ggriffiths
Copy link
Contributor Author

Cool, I'll rebase this PR. Thanks @tgross !

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

Signed-off-by: Grant Griffiths <ggriffiths@purestorage.com>
@tgross
Copy link
Member

tgross commented Apr 4, 2022

Hi @ggriffiths! We're getting very close to releasing the Nomad 1.3.0 beta, so I've carried this PR to rebase it and bring it up to date with the other secrets-related PRs we've done recently. I'm going to make sure this is green in CI and then probably tap @lgfa29 for a review of the extra commits I've added before we get it merged in.

@ggriffiths
Copy link
Contributor Author

sounds good, thanks @tgross!

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Glad to see this moving forward, thank you for taking care of it @tgross and sorry for the delay in getting this merged @ggriffiths.

@tgross tgross merged commit a285905 into hashicorp:main Apr 5, 2022
@ggriffiths
Copy link
Contributor Author

Not a problem @lgfa29, thanks!

@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 Oct 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSI: Add secrets flag support for delete volume
4 participants