Do not choke if the download_file task is invoked without a checksum

[spantaleev]

Previously (before 800c84d), download_file tasks were invoked with a sha256 (not a checksum parameter) and if not provided, downloads were done without verifying checksums.

That patch changed things (sha256 -> checksums), but it seems like unintentionally also made checksum required (no more omit fallbacks). At least some invocations of download_file do not provide a checksum, as reported here (for the ArgoCD addon): #12223

The failure behavior is also appaling, because the download task (which references checksum) has no_log enabled by default (via the unsafe_show_logs variable, defaulting to false), so users would see a failing download, but could not easily figure out that it's actually an "undefined variable" error.

While checksum support for ArgoCD is being added in #12266 it's probably better to restore the ability to invoke download_file without checksum, so that:

• any other current no-checksum callers (like ArgoCD) would be instantly made to work again
• any future calls without checksum would work, and not choke silently (due to no_log)

What type of PR is this?

/kind bug

Which issue(s) this PR fixes:
Fixes #12223

Special notes for your reviewer:
None

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: spantaleev
Once this PR has been reviewed and has the lgtm label, please assign ant31 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[k8s-ci-robot]

Hi @spantaleev. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[VannTen]

Sorry, this was not an unintentional change. Silently accepting lack of hash is not an option if we ever want to have an acceptable story on supply chain security.

An explicit opt-in to disable the hash check might be merged, but that'd need to be carefully done and probably needs some discussion.

[spantaleev]

I understand that it's better to have checksums for everything.

That said, neither the 800c84d commit, nor the #11890 pull request and comments comment on this being an explicit breaking change.

The fact that at least one service (ArgoCD) and possibly others were broken and that the download role was not made to check for checksum and fail with a nice error message (but rather fails with an error which is not shown due to no_log), tells that this whole endeavor of "improving supply chain guarantees" was very poorly handled and communicated.

[VannTen]

We certainly could have communicated this better, I'll give you that.

Unfortunately, there is not enough time and too much technical debt in Kubespray to foresee everything, particulary for kubernetes-apps (which mostly consists of things better deployed by something else).