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

✨ Add an explicit way to auto-detect ChecksumType #1480

Merged
merged 1 commit into from Dec 18, 2023
Merged

✨ Add an explicit way to auto-detect ChecksumType #1480

merged 1 commit into from Dec 18, 2023

Conversation

dtantsur
Copy link
Member

@dtantsur dtantsur commented Dec 5, 2023 •
edited

In preparation for the eventual removal of MD5 checksums, add a new
ChecksumType "auto" that instructs IPA to detect the type

Requires IPA from the 2023.2 release series. Users with older IPA
should provide the algorithm explicitly.

Signed-off-by: Dmitry Tantsur dtantsur@protonmail.com

@dtantsur
Copy link
Member Author

dtantsur commented Dec 5, 2023

/cc @hardys
in case you're affected

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 5, 2023
@dtantsur
Copy link
Member Author

dtantsur commented Dec 5, 2023

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@Rozzii
Copy link
Member

Rozzii commented Dec 6, 2023

/cc @kashifest @lentzi90 @elfosardo @zaneb

Rozzii
Rozzii previously approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot
Copy link
Contributor

@Rozzii: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@elfosardo
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023
@dtantsur
Copy link
Member Author

dtantsur commented Dec 6, 2023

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

This was just a rebase

lentzi90
lentzi90 reviewed Dec 7, 2023
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2023
@zaneb
Copy link
Member

zaneb commented Dec 7, 2023

I think I disagree with this change.

This is not about removing MD5, since it's still possible to specify MD5 explicitly.

The is about changing the type field from required to optional for SHA sums. That means that the same v1alpha1 CR may fail to work on older versions of Metal³ - or, as you acknowledge in the description, on the same version of Metal³ with different versions of IPA. Essentially this is starting to leak Ironic API changes through into the BMH.

If we want to remove the type field in v1beta1 (which I'm fine with) we can still translate back to v1alpha1 when required in the webhook by using the same logic that Ironic does (checking the length of the sum) to determine the algorithm. As far as I can see, this change is not required to do that.

@dtantsur
Copy link
Member Author

dtantsur commented Dec 8, 2023

we can still translate back to v1alpha1 when required in the webhook by using the same logic that Ironic does (checking the length of the sum)

We cannot, this logic is not trivial, and I don't want to replicate it there (it's not just length, there are many formats of downloadable checksums).

@zaneb would you be more okay with an explicit auto type as I initially planned? Otherwise, we'll have nothing to translate v1beta1 into.

@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2023
@dtantsur dtantsur changed the title ⚠️ Stop defaulting to MD5 for checksumType ✨ Add an explicit way to auto-detect ChecksumType Dec 8, 2023
@dtantsur
Copy link
Member Author

dtantsur commented Dec 8, 2023

Okay, this should no longer break the backward compatibility with older IPA.

@dtantsur
Copy link
Member Author

dtantsur commented Dec 8, 2023

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

zaneb
zaneb reviewed Dec 10, 2023
View reviewed changes
Copy link
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

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

Happy with this 👍
/approve

docs/api.md Show resolved Hide resolved
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rozzii, zaneb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2023
@dtantsur
Add an explicit way to auto-detect ChecksumType
0ed5eb1 
In preparation for the eventual removal of MD5 checksums, add a new
ChecksumType "auto" that instructs IPA to detect the type

Requires IPA from the 2023.2 release series. Users with older IPA
should provide the algorithm explicitly.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
@dtantsur
Copy link
Member Author

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@dtantsur
Copy link
Member Author

/test-ubuntu-integration-main

@hardys
Copy link
Member

hardys commented Dec 14, 2023

/cc @hardys
in case you're affected

Thanks for the heads-up 👍

/lgtm

@metal3-io-bot
Copy link
Contributor

@hardys: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/cc @hardys
in case you're affected

Thanks for the heads-up 👍

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lentzi90
lentzi90 reviewed Dec 18, 2023
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2023
@dtantsur
Copy link
Member Author

/test-ubuntu-integration-main

@metal3-io-bot metal3-io-bot merged commit bc07723 into metal3-io:main Dec 18, 2023
16 checks passed
@dtantsur dtantsur deleted the no-md5-default branch December 19, 2023 09:01
@Rozzii Rozzii mentioned this pull request Dec 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zaneb zaneb zaneb left review comments

@lentzi90 lentzi90 lentzi90 left review comments

@Rozzii Rozzii Rozzii left review comments

@hardys hardys Awaiting requested review from hardys

@elfosardo elfosardo Awaiting requested review from elfosardo

@kashifest kashifest Awaiting requested review from kashifest

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dtantsur @Rozzii @metal3-io-bot @elfosardo @zaneb @hardys @lentzi90