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

controllers: store HelmChart Artifact with suffix #611

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Mar 11, 2022

This adds a Unix suffix to the HelmChart Artifact filename, to ensure
it is unique for sequential builds triggered due to e.g. a controller
restart.

The result of this is that consumers who think they are fetching an
Artifact with a certain checksum run into a 404 when attempting to
download a previously advertised but now unavailable file, instead of
running into a checksum validation error (due to non-repetitive Helm
builds).

Potentially fixes #610

@hiddeco hiddeco added enhancement New feature or request area/helm Helm related issues and pull requests labels Mar 11, 2022
@hiddeco
Copy link
Member Author

hiddeco commented Mar 11, 2022

@aryan9600 you probably want to have a look at this, and make suggestions if it does not combine well with your proposed changes in #605.

This adds a Unix suffix to the HelmChart Artifact filename, to ensure
it is unique for sequential builds triggered due to e.g. a controller
restart.

The result of this is that consumers who _think_ they are fetching an
Artifact with a certain checksum run into a 404 when attempting to
download a previously advertised but now unavailable file, instead of
running into a checksum validation error (due to non-repetitive Helm
builds).

For more information, see:
#610

Signed-off-by: Hidde Beydals <[email protected]>
@@ -626,11 +626,11 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *source
}()

// Create artifact from build data
artifact := r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), b.Version, fmt.Sprintf("%s-%s.tgz", b.Name, b.Version))
artifact := r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), b.Version, fmt.Sprintf("%s-%s-%d.tgz", b.Name, b.Version, time.Now().Unix()))
Copy link
Member

Choose a reason for hiding this comment

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

This will break signature verification for cached charts (ref: #605). The chart name should be exactly how it's mentioned in it's provenance file, i.e. name-version.tgz. We could probably get around this by adding a symlink linking name-version-ts.tgz to name-version.tgz.

Copy link
Member Author

Choose a reason for hiding this comment

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

Was afraid this would be the case. Ack, I'll give it another shot when #605 is merged to get a better view on the options available then.

Copy link
Member

@aryan9600 aryan9600 Apr 7, 2022

Choose a reason for hiding this comment

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

We should go ahead and merge this now, because of: #605 (comment)

Converting PR to draft, as this is being put on hold until we reach GA (ref: fluxcd/flux2#2592)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather like to come up with a solution first that works for #605, as otherwise we're shooting ourselves in the foot with premeditation.

Copy link
Member

@aryan9600 aryan9600 Apr 19, 2022

Choose a reason for hiding this comment

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

We could modify #605 such that we make a copy of the packaged chart file named <name>-<version>.tgz and use that copy for sig verification and delete it after the verification process is completed. I can't think of any drawbacks for this approach @hiddeco, what do you think?

@hiddeco hiddeco marked this pull request as draft March 13, 2022 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unique (HelmChart) Artifact URLs
2 participants