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

feat: implement --push-repository flag #94

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

Conversation

machecazzon
Copy link
Contributor

@machecazzon machecazzon commented Oct 7, 2024

Description of the change

This is to support different repository for read/write operations

Usage:

dt unwrap ./kibana-11.2.23.wrap.tgz example.com/read --push-repository example.com/write
dt unwrap ./kibana-11.2.23.wrap.tgz example.com/read --push-repository example.com/write --push-chart-url example.com/write/bitnami

Applicable issues

…ifferent repository for read/write operations
@machecazzon machecazzon changed the title feat: implement --push-repository flag in order to support having a d… feat: implement --push-repository flag Oct 7, 2024
var successMessage = "Helm chart unwrapped successfully"
if fullChartURL != "" {
successMessage = fmt.Sprintf(`%s: You can use it now by running "helm install %s --generate-name"`, successMessage, fullChartURL)
//successMessage = fmt.Sprintf(`%s: You can use it now by running "helm install %s --generate-name"`, successMessage, fullChartURL)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow the index feature did not work in here as i was not able to pull chart without specifying the version

That's mainly the reason for this change. I am not sure why that

@@ -176,6 +176,34 @@ $ helm dt unwrap kibana-10.4.8.wrap.tgz demo.goharbor.io/helm-plugin/ --yes

If your wrap includes bundled artifacts (if you wrapped it using the `--fetch-artifacts` flag), they will be also pushed to the remote registry.

Unwrapping a Helm chart to a different target than the original using `--push-repository` flag.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use a new header before explaining this new flag?

Also, I find this sentence confusing. What's "the original" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I wanted to refer to helm dt unwrap oci://original_repo.
Perhaps unwrapping target ? Or what do you recommend?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like Unwrapping a Helm chart to a different registry than the one used on relocation or Using different registries for distribution and relocation?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would also be nice to add some context on why this is needed. Something like:

Sometimes you might need the content on the Helm chart pointing to a registry different than where the Helm chart needs to be pushed to. This is common for example on Airgap environments where a Helm chart might need to be pushed to an interim environment different from where it is meant to be consumed from, or for example in edge distribution channels where the Helm charts might need to be moved from an interim release registry into an edge registry for general public usage.

@@ -12,6 +12,7 @@ import (

// NewCmd builds a new annotate command
func NewCmd(cfg *config.Config) *cobra.Command {

Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid this kind of changes (this applies to other files) so the PR only shows the code you actually changed

@@ -508,6 +527,7 @@ func NewCmd(cfg *config.Config) *cobra.Command {

cmd.PersistentFlags().StringVar(&version, "version", version, "when unwrapping remote Helm charts from OCI, version to request")
cmd.PersistentFlags().StringVar(&pushChartURL, "push-chart-url", pushChartURL, "push the unwrapped Helm chart to the given URL")
cmd.PersistentFlags().StringVar(&pushRepository, "push-repository", pushRepository, "the repository to be used instead for write operations e.g (demo.goharbor.io/write/test_repo)")
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess a better parameter name would be push-container-registry, right? I mean, in a given chart there are N potential repositories published in a single registry/namespace, see:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. Unclear to me as the current flag for helm chart is push-chart-url ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@machecazzon machecazzon Oct 7, 2024

Choose a reason for hiding this comment

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

as oci repos are general purpose not specific to store binary images, charts, or just a file just registry might be a good option. Naming is the hardest in here. Let me know what you will like me to change and to what and I will update PR when that's defined. Thanks for having a look

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go for sth like this:

	cmd.PersistentFlags().StringVar(&pushContainerRegistry, "push-container-registry", pushContainerRegistry, "push the container images included in the unwrapped Helm chart to the given container registry (including namespace)")

What do you think @mpermar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I will attempt have this clean up tomorrow. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I take it back @machecazzon @juan131. The value can be both a registry and a repository so probably would need some other name like push-uri.

But, actually, while looking in detail to the naming, and this is totally my bad as I am lost touch with this project and it has evolved quite a while since I left it, I can see there is a push-chart-url which I believe it might do exactly what this PR is trying to do.

Copy link
Contributor Author

@machecazzon machecazzon Oct 10, 2024

Choose a reason for hiding this comment

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

Have been thinking about this a bit more and I believe that having --values-registry could be more appropriate as is actually what I am looking for.

"Have the values of the chart to be relocated not to the unwrap target but instead the value provided by e.g --values-registry."

That's more like what I did on my first attempt to cover the use case.

Right now I have a custom build with what I need to have the desired outputs that includes this and some other changes and is not a big deal but I would prefer that this tool could support this behaviour.

Back into the naming, kaniko has this switch and it's format somehow make me think of this Thread.

https://github.com/GoogleContainerTools/kaniko?tab=readme-ov-file#flag---registry-map

Very likely I will abandon this PR and come back later with a better solution and better PR hopefully, but if wanted can still try to address this PR comments.

In regards the push-chart-url I am currently using that switch to tune the chart destination path as well as needing this same "feature". This is related to some other "issue" I have with current implementation and patched for good on my end :)

RelocateImageURL method seems that it might change the original path and have something else on the unwrap target depending on the input provided and what the image url is. This turns that image path not be the exactly the same on the destination (apart the registry bits) as the original source what I need to ensure preservation.

My current scenario: GAR Reader / GAR Writer

  • GAR Reader (Virtual Repo) is a able to pull from Gitlab registry
  • dev teams build their images and push to Gitlab registry
  • dt is used to build a wrap on ci ensuring images exist, are pinned (lock/verify) but skip save images that source from Gitlab while still have those on the Images.lock. Chart's are annotated on CI and then wrapped.

dt charts annotate
dt wrap

  • the unwrap target gitlab what relocates chart values to be GAR reader repo instead of the target (in this case) and basically only pushes images that exist on disk what skips the ones that exist on lock but not on disk.
  • then based on some release process a "golden" wrap is built eventually sourcing from gitlab (the result of some unwrap like described above.. ), this wrap will be published/unwrapped to GAR Write repo (configured to have immutable tags). In this case as the source is different than the target is does the usual dt dance (although thinking that can just avoid saving any images on disk and just do a crane copy based on the Images.lock 🤔 ).
  • When this wrap is published it takes over and is what's will be consumed from now on ( if that version is requested ) cutting the link with Gitlab.
  • The chart values are again preserved to be the GAR Reader repo and not the GAR Writer that is the unwrap target
  • The versions available and published on the GAR Writer are the ones that are available and can be consumed for non dev envs

For all the previous to work transparently It's crucial to guarantee images path preservation from Gitlab to GAR and or have controlled rewrite if desired as there are some variants and eventual some restrictions depending on the unwrap target ghcr.io | docker.io | registry.gitlab.com | ....

Copy link
Contributor

Choose a reason for hiding this comment

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

That's great context. And wow, you have built a thing there.

I'm approaching your PR here from a point of view of trying to keep the tool as simplest as possible. Ideally I'd like to have very simple building blocks even when that might come at the expense of having extra complexity in the client scripts. It makes this project more sustainable in the long run.

From that perspective I see that helm dt unwrap target-images-registry --push-chart-url=target-chart-registry is giving us two basic primitive arguments: one to set where images are going to be pushed, and a second one to set where the chart is going to be pushed. It can certainly be improved (e.g. kaniko for multi-registry images), but it feels effective enough.

But at the same time I have to be honest here, I'm humbled by your use case and what you are doing. So, I'm not sure if I'm missing anything obvious. Perhaps the right thing to do is as you say to abandon this one and then try to approach it with some simpler baby-step kind of change. Not sure, tbh.

Copy link
Contributor Author

@machecazzon machecazzon Nov 4, 2024

Choose a reason for hiding this comment

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

Hello @mpermar

Sorry for delay getting back.

I ended with something like this

  • build a wrap (from local filesystem "helm chart") that does not pull images but just contain helm chart and Images.lock. Unwrap will basically just push the chart (source registry and target registry are the same) gitlab --> gitlab

  • with the previous wrap (no local images) be able to unwrap to destination registry but copy the referenced docker images as usual while source images instead of being local filesystem be a crane copy from registry source to target (source registry and target registry are not the same) .. in my case gitlab --> gar

  • still have the option to "--create-offline-bundle" where all images pulled/saved locally as usual and wrapped in the wrap

@@ -32,7 +33,6 @@ Use it cautiously. Very often the complete list of images cannot be guessed from
chartutils.WithAnnotationsKey(cfg.AnnotationsKey),
chartutils.WithLog(l),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

@@ -18,10 +19,8 @@ import (
"github.com/vmware-labs/distribution-tooling-for-helm/pkg/chartutils"
"github.com/vmware-labs/distribution-tooling-for-helm/pkg/imagelock"
"github.com/vmware-labs/distribution-tooling-for-helm/pkg/log"
"github.com/vmware-labs/distribution-tooling-for-helm/pkg/log/silent"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: undo reordering

@@ -176,6 +176,34 @@ $ helm dt unwrap kibana-10.4.8.wrap.tgz demo.goharbor.io/helm-plugin/ --yes

If your wrap includes bundled artifacts (if you wrapped it using the `--fetch-artifacts` flag), they will be also pushed to the remote registry.

Unwrapping a Helm chart to a different target than the original using `--push-repository` flag.
By unwrapping the Helm chart to a target OCI registry the `dt` tool will unwrap the wrapped file, proceed to push the container
images into the registry you have specified by `--push-repository`, relocate the references from the Helm chart and preserve the original target
Copy link
Contributor

Choose a reason for hiding this comment

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

Silly comment. For consistency it would be great if you can remove this line breaks on any single paragraph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unwrap/relocate option to allow values.yaml relocation to a different custom registry/repository
3 participants