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

Strip trailing / from proxmox url #238

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Strip trailing / from proxmox url #238

merged 1 commit into from
Nov 7, 2023

Conversation

nick-holmquist
Copy link
Contributor

@nick-holmquist nick-holmquist commented Nov 7, 2023

Accidentally adding a trailing / to the proxmox_url causes issues with additional_iso_files. A 501 can be seen and the upload fails.
Unfortunately, that error is a red herring. While the url is logged in debug mode, it is simply a log print and not obvious as it doesn't actually point out the error

Can replicate by simply specifying a / at the end of the proxmox_url config
proxmox_url ="https://my-proxmox.my-domain:8006/api2/json/"

The upload request becomes; eg. POST /api2/json//nodes/pve/storage/local/upload
Note the json// with the extra slash.

Stripping the trailing slash should resolve this issue when someone accidentally copies/pastes the url to the config.

Edit: Another option would be to check the string format in general and give an error to resolve before continuing.

Accidentally adding a trailing / to the proxmox_url causes issues with additional_iso_files. A 501 can be seen and the upload fails.

proxmox_url ="https://my-proxmox.my-domain:8006/api2/json/"

Upload request becomes; eg. POST /api2/json//nodes/pve/storage/local/upload

Note the json// with the extra slash.
@nick-holmquist nick-holmquist requested a review from a team as a code owner November 7, 2023 06:00
@hashicorp-cla
Copy link

hashicorp-cla commented Nov 7, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Good catch @nick-holmquist!

The fix looks good to me, I'll wait for tests to go green, and merge this after.

I'll release a new version of the plugin today since this seems like a fix to an easy-to-hit problem.

Thanks for submitting this PR!

@lbajolet-hashicorp lbajolet-hashicorp merged commit 702dce8 into hashicorp:main Nov 7, 2023
12 checks passed
@nick-holmquist
Copy link
Contributor Author

Good catch @nick-holmquist!

The fix looks good to me, I'll wait for tests to go green, and merge this after.

I'll release a new version of the plugin today since this seems like a fix to an easy-to-hit problem.

Thanks for submitting this PR!

The other option would be to sanitize/verify the url on submission (regex match) and actually error out properly. I don't know GoLang to be able to do that proficiently though. I spent enough time trying to learn/debug Go on the fly just to find I had an extra slash in my url!

Thanks for the quick review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants