OCI support in Helm builtin #4614

mikebz · 2022-05-02T16:54:38Z

This is currently WIP.

Questions for owner: @natasha41575 @yuwenma @KnVerey
It seems like @monopole has added environment variables to put the data and cache for HELM into a temp directory. Do we know why that is? The reason why this is undesirable with OCI is that most of the OCI repos are protected with auth. Auth is possible if one does authentication with Helm ahead of time. Example:

helm registry login -u _json_key --password-stdin https://us-central1-docker.pkg.dev

After that is run I am able to do things like:

helmCharts:
- name: chart1
  version: 0.1.0
  repo: oci://us-central1-docker.pkg.dev/mikebz-ex1/charts
  valuesInline:
    nameOverride: foobar

and if the configuration is not in the temp location I can easily get the chart.

One of the ideas is to include some sort of an authentication in kustomization.yaml, but I think that would be undesirable.

Things that are left to figure out:

create a publicly available registry or something that can be used to ensure that the unit test works.
figure out if unsetting the HELM* variables will have a security or other undesirable impact.
is there an issue updating the default Helm download to 3.8+? OCI is enabled by default in those builds

k8s-ci-robot · 2022-05-02T16:54:39Z

@mikebz: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

k8s-ci-robot · 2022-05-02T16:54:47Z

Hi @mikebz. 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/test-infra repository.

mikebz · 2022-05-02T17:11:59Z

/label tide/merge-method-squash

natasha41575 · 2022-05-02T18:15:42Z

This PR is probably more appropriate to make in this repo: https://github.com/kubernetes-sigs/krm-functions-registry/tree/main/krm-functions/sig-cli/render-helm-chart

On the kustomize side, we have paused development on the helm plugin until we are able to migrate to the KRM function. The long term plan is here: #4401 and work is in progress on this front

mikebz · 2022-05-02T18:30:36Z

I am happy to add the work there as well, I would say there is a short term and longer term goals here. (1) - we should unblock customers who will use the builtin, (2) - we should migrate.

natasha41575 · 2022-05-13T17:07:24Z

/hold

Having the test infra will be great, but it's also been pointed out to me that this is a hotfix for a particular use case. I'm designing a more general solution that we will put into the KRM function, after which we can see if we want to put it in the builtin plugin. But, this is a more substantial feature than I'd realized when I first saw this PR so I believe a better course of action would be to prioritize migration (which I plan to do this quarter).

Again, the helm builtin is slated for deprecation and eventual removal, so our support on it is extremely limited.

natasha41575 · 2022-05-16T18:13:49Z

An update from the infra folks:

The k8s infra doesn't yet properly support Artifact Registry, and they are hesitant to provision it for us ad-hoc for a single-cluster use case that the kustomize maintainers are skeptical about. It is more realistic for them to support it for us for the KRM functions registry a few months from now.

mikebz · 2022-05-16T21:32:01Z

Thanks for the update. One of the things to consider is that the current tests are hitting 3rd party chart registries. Maybe we can create one for functions and still use it for built in and the function. There is no precedent right now that all of the test repositories are CNCF owned.

tis-rpage · 2022-05-29T21:44:46Z

plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go

+
+	// OCI pull combine the repo and the chart name into one URL
+	if strings.HasPrefix(p.Repo, "oci://") {
+		chartUrl := p.Repo + "/" + p.Name


Just to clarify, it's saying Url should be URL in the variable name, I missed the distinction on first read-through.

tis-rpage · 2022-05-29T21:48:59Z

/hold

Having the test infra will be great, but it's also been pointed out to me that this is a hotfix for a particular use case. I'm designing a more general solution that we will put into the KRM function, after which we can see if we want to put it in the builtin plugin. But, this is a more substantial feature than I'd realized when I first saw this PR so I believe a better course of action would be to prioritize migration (which I plan to do this quarter).

Again, the helm builtin is slated for deprecation and eventual removal, so our support on it is extremely limited.

Is it truly unacceptable to accept this patch as a stop-gap measure to unblock the user base of kustomize from using OCI registries?

I agree the KRM function will be better long-term, but as it stands, the user base has to run external commands to pull OCI charts and make them available to kustomize locally before its invocation. Seeing as the helm team broke semver compliance, it stands to reason a hotfix should be acceptable to address the upstream products bad behavior.

bgrant0607 · 2022-06-03T18:16:27Z

KRM functions aren't yet a viable replacement for built-in plugins. For instance, of the GitOps tools that support kustomize, which ones support KRM functions? Are there examples of executing KRM functions in CI?

There was a suggestion to fork kustomize here:
argoproj/argo-cd#5553 (comment)

Since there are uses of the helm built-in plugin, I recommend continuing to support it until KRM function execution obstacles are overcome.

bgrant0607 · 2022-06-03T21:19:14Z

Example user: https://twitter.com/todaywasawesome/status/1532078601448787968

k8s-triage-robot · 2022-09-01T22:07:08Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-10-01T22:26:04Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

igstbagusdharmaputra · 2022-10-14T16:46:43Z

Waiting for closed this issue 😁

mikebz · 2022-10-14T16:51:03Z

I am guessing this addition is unwelcome. We have already created a workaround in ConfigSync, so my interest in lobbying for this more has waned :)

igstbagusdharmaputra · 2022-10-14T16:57:26Z

I am guessing this addition is unwelcome. We have already created a workaround in ConfigSync, so my interest in lobbying for this more has waned :)

i hope this issue can solving, because i need pull chart from oci repository

k8s-triage-robot · 2022-11-13T23:18:43Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

mikebz · 2022-11-23T16:54:03Z

/retest

k8s-ci-robot · 2022-11-23T16:54:19Z

@mikebz: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

k8s-triage-robot · 2022-12-23T17:45:18Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-12-23T17:45:22Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

antonydevanchi · 2023-01-02T11:01:33Z

@mikebz bump?

mikebz · 2023-01-02T16:56:14Z

/reopen

k8s-ci-robot · 2023-01-02T16:56:18Z

@mikebz: Reopened this PR.

In response to this:

/reopen

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.

k8s-ci-robot · 2023-01-14T11:13:22Z

@mikebz: PR needs rebase.

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.

k8s-ci-robot · 2023-02-07T19:39:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: diegoluisi, mikebz
Once this PR has been reviewed and has the lgtm label, please assign natasha41575 for approval by writing /assign @natasha41575 in a comment. For more information see the Kubernetes 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

antonydevanchi · 2023-02-16T11:58:51Z

@diegoluisi @mikebz thank you for yours job done earlier. What next steps? It's not clear from last activity in thread.

mikebz · 2023-02-16T21:25:45Z

I think we need to clear go ahead from the project maintainers @KnVerey
There is a concern here about backwards compatibility but for an undocumented feature.

antonydevanchi · 2023-02-21T02:32:38Z

@mikebz can I help something?

Also I did PR with fix lint alert: https://github.com/mikebz/kustomize/pull/1/files
If it's useles, so then just feel free just drop request :)

mikebz · 2023-02-21T17:05:08Z

Thank you @antonydevanchi ! There are a few things to do to get this PR back into shape. I think the major question is whether we introduce a potential breaking change or not.

natasha41575 · 2023-02-22T19:17:56Z

@KnVerey and I discussed this, and the primary concern we have is a lack of bandwidth. There are a few things we need to address here, one of which is auth.

It seems like @monopole has added environment variables to put the data and cache for HELM into a temp directory. Do we know why that is?

I believe this is because of kustomize's foundational principle that the kustomization file should not depend on its environment, which includes that it should not be able to get auth information from its environment. Breaking this encapsulation by allowing the user to run helm registry login -u _json_key --password-stdin https://us-central1-docker.pkg.dev/ prior to kustomize build may work but it would add the kustomize eschewed feature of build time side effects.

Because of that, the kustomize maintainers can't accept this PR as it is. OCI support in the Helm builtin then requires more thoughtful design, in particular how to properly do auth. As such, I think it would need a kustomize mini-KEP.

For transparency, there are only 3-4 people working on kustomize, most of whom are spending less than 10% of their time on it. We are trying to prioritize things that are core to kustomize functionality, and additionally, none of us are particularly familiar with helm. We already have a large number of open PRs and one open KEP that we are struggling to find time to review, and as a result, we cannot promise that we have the bandwidth to support this feature.

k8s-triage-robot · 2023-03-24T20:12:51Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2023-03-24T20:12:56Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

tis-rpage · 2023-04-04T13:24:34Z

I believe this is because of kustomize's foundational principle that the kustomization file should not depend on its environment, which includes that it should not be able to get auth information from its environment. Breaking this encapsulation by allowing the user to run helm registry login -u _json_key --password-stdin https://us-central1-docker.pkg.dev/ prior to kustomize build may work but it would add the kustomize eschewed feature of build time side effects.

If this is truly the case, kustomize needs to break the ability to support network references that are fetched at build time, making the tool far less valuable when external network resources for kustomizations and helm charts can no longer be referenced, which by necessity are build time side-effects of a network fetch at a point in time.

dharma-rise · 2023-04-04T13:29:24Z

I am guessing this addition is unwelcome. We have already created a workaround in ConfigSync, so my interest in lobbying for this more has waned :)

so sad

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 2, 2022

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 2, 2022

k8s-ci-robot requested review from KnVerey and natasha41575 May 2, 2022 16:54

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 2, 2022

eddiezane mentioned this pull request May 11, 2022

OCI Registry to test Kustomize Helm integration kubernetes/k8s.io#3725

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2022

tis-rpage reviewed May 29, 2022

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 1, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 1, 2022

mikebz added 2 commits November 23, 2022 08:15

another linter issue

a42e67e

another linter happy change

91d38e8

mikebz force-pushed the mb_oci branch from d654bb2 to 91d38e8 Compare November 23, 2022 16:16

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2022

resolving conflicts

6dac723

k8s-ci-robot closed this Dec 23, 2022

k8s-ci-robot reopened this Jan 2, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2023

diegoluisi approved these changes Feb 7, 2023

View reviewed changes

k8s-ci-robot closed this Mar 24, 2023

jkroepke mentioned this pull request May 10, 2023

kustomize: Add support for OCI based helm repos #5167

Merged

natasha41575 mentioned this pull request Oct 20, 2023

Consider using local credentials for helm to support private oci-based helm charts #5407

Closed

OCI support in Helm builtin #4614

OCI support in Helm builtin #4614

Conversation

mikebz commented May 2, 2022

k8s-ci-robot commented May 2, 2022

k8s-ci-robot commented May 2, 2022

mikebz commented May 2, 2022

natasha41575 commented May 2, 2022

mikebz commented May 2, 2022 • edited Loading

natasha41575 commented May 13, 2022 • edited Loading

natasha41575 commented May 16, 2022 • edited Loading

mikebz commented May 16, 2022

tis-rpage May 29, 2022

Choose a reason for hiding this comment

tis-rpage commented May 29, 2022

bgrant0607 commented Jun 3, 2022

bgrant0607 commented Jun 3, 2022

k8s-triage-robot commented Sep 1, 2022

k8s-triage-robot commented Oct 1, 2022

igstbagusdharmaputra commented Oct 14, 2022

mikebz commented Oct 14, 2022

igstbagusdharmaputra commented Oct 14, 2022

k8s-triage-robot commented Nov 13, 2022

mikebz commented Nov 23, 2022

k8s-ci-robot commented Nov 23, 2022

k8s-triage-robot commented Dec 23, 2022

k8s-ci-robot commented Dec 23, 2022

antonydevanchi commented Jan 2, 2023

mikebz commented Jan 2, 2023

k8s-ci-robot commented Jan 2, 2023

k8s-ci-robot commented Jan 14, 2023

k8s-ci-robot commented Feb 7, 2023

antonydevanchi commented Feb 16, 2023

mikebz commented Feb 16, 2023

antonydevanchi commented Feb 21, 2023

mikebz commented Feb 21, 2023

natasha41575 commented Feb 22, 2023 • edited Loading

k8s-triage-robot commented Mar 24, 2023

k8s-ci-robot commented Mar 24, 2023

tis-rpage commented Apr 4, 2023

dharma-rise commented Apr 4, 2023

mikebz commented May 2, 2022 •

edited

Loading

natasha41575 commented May 13, 2022 •

edited

Loading

natasha41575 commented May 16, 2022 •

edited

Loading

natasha41575 commented Feb 22, 2023 •

edited

Loading