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

helm.v4.Chart: hook support #3284

Open
awoimbee opened this issue Oct 24, 2024 · 4 comments · May be fixed by #3285
Open

helm.v4.Chart: hook support #3284

awoimbee opened this issue Oct 24, 2024 · 4 comments · May be fixed by #3285
Labels
area/helm kind/enhancement Improvements or new features

Comments

@awoimbee
Copy link

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

helm.v3.Chart ignored the helm.sh/hook* annotations but still deployed the resources.
The helm.v4.Chart resource skips the hooks. This is mentioned in the blog but not in the docs BTW.

I think users should have a way to make v4.Chart deploy hooked resources like v3 did, because it's useful and becasue it makes migrating from v3 to v4 less painful.

@awoimbee awoimbee added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team labels Oct 24, 2024
@rquitales rquitales removed the needs-triage Needs attention from the triage team label Oct 24, 2024
@EronWright
Copy link
Contributor

I think the issue is that hook resources have special lifecycle/ordering requirements. Basically would they work as expected?

@awoimbee
Copy link
Author

awoimbee commented Oct 29, 2024

Yep, I have multiple charts that stopped working when moving from v3 to v4:

  • https://charts.min.io/ minio (the create bucket job did not run anymore, I'm using the old v4 chart)
  • https://kubernetes.github.io/ingress-nginx ingress-nginx (the patch job for the admission webhook)
  • https://kubernetes-sigs.github.io/aws-efs-csi-driver/ efs-csi-driver
  • https://charts.wiz.io/ wiz-kubernetes-integration
  • https://trow-registry.github.io/trow trow (the patch job just like ingress-nginx)

The issue I have with just removing the hooks is that I can't get the templated yaml out of pulumi.
If I want to remove a hook, I can do it myself, using something like (this v3 transform):

export const ignoreResourcesHelmTransformation = (...resources: string[]) => {
  const stack = pulumi.getStack();
  return (o: any, opts: pulumi.CustomResourceOptions) => {
    const resourceName = `${o.apiVersion}:${o.kind}:${o.metadata?.namespace || ""}:${o.metadata?.name || ""}`;
    if (resources.includes(resourceName)) {
      console.log(`Skip: ${resourceName}`);
      const dir = `../ignored-resources-${stack}`;
      if (!fsSync.existsSync(dir)) {
        fsSync.mkdirSync(dir);
      }
      fsSync.writeFileSync(
        `${dir}/${resourceName.replaceAll("/", "-")}.yaml`,
        yaml.dump(o)
      );
      for (const key of Object.keys(o)) {
        delete o[key];
      }
      o.apiVersion = "v1";
      o.kind = "List";
      o.items = [];
    }
  };
};

@EronWright
Copy link
Contributor

EronWright commented Oct 31, 2024

(here's an outline of the technical challenge)

The Helm documentation outlines the various hook behaviors that Pulumi would be expected to emulate:
https://helm.sh/docs/topics/charts_hooks/#the-available-hooks

In v3, Pulumi simply treated the hooks as normal resources, which can lead to incorrect behavior.

As we can see, hooks generally affect the order of operations, and also vary based on whether the chart is being installed, upgraded, or deleted. The Pulumi engine provides the dependsOn option to control order-of-operations between resources, which is likely sufficient to support the pre/post aspect. It is less clear how to support "upgrade" hooks, since the Chart/v4 resource is stateless, it cannot distinguish between install and upgrade. Also unclear how to support "delete" hooks, since the Chart/v4 resource is a component resource and there's no opportunity to "create" a hook resource during component deletion.

@EronWright EronWright changed the title helm.v4.Chart: make helm hooks optional (instead of force disabled) helm.v4.Chart: hook support Dec 4, 2024
@EronWright
Copy link
Contributor

EronWright commented Dec 4, 2024

A low-hanging improvement here would be to emit a clear warning message if the chart did contain a hook. The provider does have a warning like this but isn't effective in Chart/v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm kind/enhancement Improvements or new features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants