storage: change Artifact checksum to SHA256

[hiddeco]

Addresses part of fluxcd/flux2#1707. Other usages of SHA1 (e.g. within the BucketReconciler) have been left untouched as they will be already addressed in e.g. #412.

⚠️ target branch of the PR is reconcilers-dev.

[johngmyers]

This is an incompatible change: anything depending on the existing field's containing a SHA-1 would be broken.

Adherence to apimachinery rules would require a new field and maintaining a SHA-1 in Checksum as long as the v1beta1 API is still supported.

[hiddeco]

This is an incompatible change: anything depending on the existing field's containing a SHA-1 would be broken.

The change is made to a development branch as clearly documented in the PR description.

Adherence to apimachinery rules would require a new field and maintaining a SHA-1 in Checksum as long as the v1beta1 API is still supported.

This branch will eventually replace the v1beta1 API, as there have been more changes in this branch related to the API.

[stefanprodan]

The checksum field is not used by any of the GitOps Toolkit controllers nor is it user-facing. I see no issues with changing the SHA algorithm in v1beta1.

[stefanprodan]

LGTM

Thanks @hiddeco

PS. We should add a helper function to fluxcd/pkg/runtime that downloads artifacts, checks the sha then extracts the files in tmp. This function should replace: https://github.com/fluxcd/kustomize-controller/blob/main/controllers/kustomization_controller.go#L429

[hiddeco]

PS. We should add a helper function to fluxcd/pkg/runtime that downloads artifacts, checks the sha then extracts the files in tmp. This function should replace: https://github.com/fluxcd/kustomize-controller/blob/main/controllers/kustomization_controller.go#L429

I think this would introduce a challenging dependency chain (runtime -> source API -> source-controller -> runtime). Instead, either the Artifact should be made generic and decoupled from the source API, or the helper utility should be introduced somewhere within this code base.

[hiddeco]

From short discussion in private: idea is to introduce a <Something>Download(url, checksum string) (string, error) later on. Which would prevent coupling to the API, and not assume having to untar.

[johngmyers]

I'm having trouble reconciling the statements here about changing/replacing the v1beta1 API against the statement in the August 2021 update:

The GOTK Custom Resource Definitions which are at v1beta1 and v2beta1 and their controllers are considered stable and production ready. Going forward, breaking changes to the beta CRDs will be accompanied by a conversion mechanism.

On a separate topic, I've had problems with source-controller OOMing because it downloads multi-GB files (such as our Helm repository index) into memory, causing it to exceed the pod's memory limit. I was starting to look into fixing it to stream directly to disk, but it appears there's a lot of change going in that area right now.

[hiddeco]

I'm having trouble reconciling the statements here about changing/replacing the v1beta1 [...]

It will be changed to v1beta2 (or something else) later on, with the current changes, the ones that break contract are all related to machine parts of the API, and not the declarative configuration.

On a separate topic, I've had problems with source-controller OOMing because it downloads multi-GB files (such as our Helm repository index) into memory, causing it to exceed the pod's memory limit.

Yes, this is a known issue, but challenging at the same time as you will run into this exact same issue when e.g. a chart revision is looked up in the index. So just preventing it from being loaded into memory when it is downloaded does not solve the whole issue.

Contributions to improve the situation are however always welcome, and you have two options:

1. You contribute to main, and I'll happily backport your fixes to the reconcilers-dev branch.
2. You contribute on top of the helmrepository-reconciler branch from Rewrite HelmRepositoryReconciler to new standards #413, which contains all refactored logic and is not expected to change much more for now (besides additional tests and a double check on emitted Events).

[johngmyers]

Okay, but strict adherence to apimachinery rules would require storing the SHA1 in an annotation of the v1beta2 resource so that a v1beta1 resource could round-trip. The SHA256 could either go into an annotation or new field in the v1beta1 resource.

If the project wants to declare parts of the API as outside of the stability guarantee, that's their decision to make. I don't have a use for the SHA1.

For me, verifying a secure hash is much higher priority than saving memory. That would seem to depend on the helmrepository-reconciler branch.