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

proxmox_template: Add optional checksum validation #9601

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Andrewb12505
Copy link
Contributor

@Andrewb12505 Andrewb12505 commented Jan 22, 2025

SUMMARY

Adds options to enable and compare a downloaded file with a checksum to proxmox_template module.
While implementing this feature I required a function to check if an api request returned a non OK exit status.
This felt best placed next to the api_task_ok function in proxmox.py.

The implementation is an alteration of fetch_template that only gets called if the user requests the usage of checksum validation.

Adds three new playbook options for proxmox_template:
verify_checksum as a boolean value
checksum as a string
checksum_algorithm as a choice between 5 options: md5, sha224, sha256, sha348, sha512

Fixes #9553

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

proxmox_template
plugins/module_utils/proxmox

ADDITIONAL INFORMATION

I was having issues with either vscode or git refactoring the documentation lines as I was making commits.
Most of the changes made to those sections unrelated to new code are unintentional. I tried to repair those changes as best I could.

@ansibullbot

This comment was marked as outdated.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added WIP Work in progress ci_verified Push fixes to PR branch to re-run CI feature This issue/PR relates to a feature request module module plugins plugin (any type) labels Jan 22, 2025
@Andrewb12505 Andrewb12505 changed the title [wip] proxmox_template: Add optional checksum validation proxmox_template: Add optional checksum validation Jan 22, 2025
@Andrewb12505 Andrewb12505 marked this pull request as ready for review January 22, 2025 13:59
@ansibullbot ansibullbot removed WIP Work in progress ci_verified Push fixes to PR branch to re-run CI labels Jan 22, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Jan 22, 2025
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I've added some first comments.

plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
@Andrewb12505
Copy link
Contributor Author

Thank you for the feedback! Sorry about the headache of all the re-formats, I'll be careful not to let those end up commited again.
They were not intentional, I'll implement you suggestions shortly.

@ansibullbot ansibullbot added the module_utils module_utils label Jan 22, 2025
@Andrewb12505
Copy link
Contributor Author

Alright, I think i fixed all the accidental refactorings to modules/proxmox.py, and properly added the function i needed to the module_utils/proxmox.py.

Super sorry about the pull request that tried to change a completely unrelated file, I didn't even realize I had opened the wrong file.
Thank you for the feedback and patience with this new guy.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Couple of comments

plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Hi @Andrewb12505

Thanks for the adjusments. I got another round of comments there.

plugins/modules/proxmox.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_template.py Outdated Show resolved Hide resolved
@Andrewb12505
Copy link
Contributor Author

Andrewb12505 commented Jan 23, 2025

All are very good points, I agree that having verify_checksum is a very redundant way of checking. It absolutely slipped my mind that i could just check for checksum having a value or not. The required together makes complete sense to me.

The fetch_and_verify function is nearly identical to the fetch_template function the module currently uses. The only difference is i had to assign the arguments to a dictionary before using urlencode to make the POST request understandable to the proxmox API. The fetch_template function does this on its own, but the difference is you can't assign parameters that would be illegal python variables like checksum-algorithm without assigning them to a dictionary manually.

The short answer on what the fetch_and_verify function does is it makes a POST request to the download-url endpoint of the proxmox API. Encoding any parameters in the dictionary. Finally it downloads the file specified in the url option of the playbook, and then proceeds to check the files integrity against the provided checksum.

Edit

The behavior of my changes are identical to the modules current behavior on a successful task. I added an explicit check for a task that exits and would normally appear successful to ansible, but on proxmox' end had failed with an error. This was to supplement the checksum verification function as it would return successfully even if it had failed the validation prior to the api_task_failed check.

@russoz
Copy link
Collaborator

russoz commented Jan 23, 2025

Fair enough, thanks for clarifying :-)

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM

@Andrewb12505
Copy link
Contributor Author

Thank you, completely forgot to change the documentation to specify its dependence on each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. feature This issue/PR relates to a feature request module_utils module_utils module module plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server-side artifact fetching feature in proxmox_template module does not include checksum option
4 participants