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

kubernetes.helm.v3.Release always shows "chart" diff #2573

Closed
ffMathy opened this issue Sep 21, 2023 · 14 comments
Closed

kubernetes.helm.v3.Release always shows "chart" diff #2573

ffMathy opened this issue Sep 21, 2023 · 14 comments
Assignees
Labels
kind/enhancement Improvements or new features
Milestone

Comments

@ffMathy
Copy link

ffMathy commented Sep 21, 2023

What happened?

I always get a "chart" diff when I deploy my release via Pulumi. See attached for the chart I am using.

I am doing it from within a dev-container.

Example

Try it with this chart:

ftrack-4.12.2+chart-1.2.11.tar.gz

Output of pulumi about

CLI
Version 3.84.0
Go Version go1.21.1
Go Compiler gc

Plugins
NAME VERSION
nodejs unknown

Host
OS debian
Version 12.1
Arch aarch64

This project is written in nodejs: executable='/usr/local/bin/node' version='v18.18.0'

Current Stack: organization/iac/sandbox

The rest of the information has been redacted, as I consider it sensitive.

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@ffMathy ffMathy added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Sep 21, 2023
@ffMathy
Copy link
Author

ffMathy commented Sep 21, 2023

If I show details of the preview, it shows:

chart : "/__w/ftrack/helm/ftrack-4.10.2+chart-1.2.9.tar.gz" => "/workspaces/ftrack/helm/ftrack-4.10.2+chart-1.2.9.tar.gz"

So it is somehow doing something weird to my path. The real path is not __w.

@ffMathy
Copy link
Author

ffMathy commented Sep 21, 2023

For specifying the chart path, I am using:

path.join(__dirname, 'helm/ftrack-4.10.2+chart-1.2.9.tar.gz')

Perhaps the offender is somehow __dirname?

@mikhailshilkov
Copy link
Member

Hi @ffMathy. Do you have a repro code so that I could try it out locally?

@mikhailshilkov mikhailshilkov added awaiting-feedback Blocked on input from the author and removed needs-triage Needs attention from the triage team labels Sep 21, 2023
@ffMathy
Copy link
Author

ffMathy commented Sep 21, 2023

No, unfortunately I do not, as the code is proprietary. I am quite sure it's as simple as doing a __dirname and then running Pulumi up, then cloning your repo into another folder on your host and do the same. Since the path is a full path, I think that's what goes wrong.

Does it support relative paths?

@mikhailshilkov mikhailshilkov added needs-triage Needs attention from the triage team and removed awaiting-feedback Blocked on input from the author labels Sep 21, 2023
@rquitales
Copy link
Contributor

@ffMathy Unfortunately yes, I believe this issue is due to the path to a local chart being different. A new feature addition last week (#2568) was submitted to use the chart checksum to determine if an update is needed or not. This will be rolled into a release early this week.

@EronWright could you check to see if using dynamic local chart paths can work nicely with the new checksum feature? Thanks!

@rquitales rquitales removed the needs-triage Needs attention from the triage team label Sep 25, 2023
@mikhailshilkov mikhailshilkov added kind/question Questions about existing features awaiting-feedback Blocked on input from the author and removed kind/bug Some behavior is incorrect or out of spec labels Sep 25, 2023
@EronWright
Copy link
Contributor

A dynamic local chart path will work fine with the checksum feature, in the sense that content changes would still be detected. But the chart path is still one of the inputs that is considered (in addition to the checksum of the rendered content), so #2568 would not address this issue.

@ffMathy relative paths do work, and symlinks probably work, so I wonder if you could stabilize the path to avoid a spurious diff. Another option may be to use ignoreChanges: ["chart"].

@ffMathy
Copy link
Author

ffMathy commented Sep 25, 2023

Hmmm, why does Pulumi even take "chart" into account now that it uses hashing too? Shouldn't that be all that matters? Could we maybe make this part of the fix?

@rquitales
Copy link
Contributor

@EronWright I think we could improve the experience here as per @ffMathy's suggestion. Since we already calculate checksums, I think we should update our diff'ing to use this calculated value as the determiner instead of simple resource names/paths that could be in flux. Wdyt?

@rquitales rquitales added kind/bug Some behavior is incorrect or out of spec and removed awaiting-feedback Blocked on input from the author kind/question Questions about existing features labels Sep 26, 2023
@EronWright
Copy link
Contributor

While I agree that the checksum is a reasonable alternative, are relative paths not sufficient? How are file paths handled in other resources, has it been done elsewhere?

@ffMathy
Copy link
Author

ffMathy commented Oct 1, 2023

I just couldn't get relative paths working.

It's a monorepo. Sometimes we run Pulumi up directly when testing things locally. There, the working directory is different than it is from when running it via NX (where I think(?) the working directory is the repository root).

Making the path absolute ensures it runs the same across.

@EronWright EronWright added this to the 0.95 milestone Oct 2, 2023
@EronWright EronWright self-assigned this Oct 2, 2023
@madappa-sharath
Copy link

madappa-sharath commented Oct 5, 2023

I'm seeing the same issue even on charts that aren't local. Example:

const controllerHelmChart = new k8s.helm.v3.Release("aws-load-balancer-controller", {
            chart: "aws-load-balancer-controller",
            version: "1.5.3",
            repositoryOpts: {
                repo: "https://aws.github.io/eks-charts",
            },
            namespace: "kube-system",
            values: {
                clusterName: cluster.name!,
                serviceAccount: {
                    create: false,
                    name: 'aws-load-balancer-controller'
                },
                image: {
                    repository: <private-registry-url>,
                    tag: <private-registry-tag>
                }
            },
            timeout: 1800,
            skipAwait: false,
            atomic: true,
            cleanupOnFail: true,
        }, {
            dependsOn: [controllerServiceAccount],
            provider: this._getK8Provider()
        })

Nothing has changed but I get a diff on checksum every time I run pulumi up. I recently bumped version of pulumi kubernetes provider. I have 3 external charts installed via k8s.helm.v3.Release and two consistently produce the checksum diff.

~   ├─ kubernetes:helm.sh/v3:Release  cilium                                update     [diff: ~checksum]
~   └─ kubernetes:helm.sh/v3:Release  aws-load-balancer-controller          update     [diff: ~checksum]

One observation I have in my limited data set is that seems to be affecting charts in 'kube-system' namespace. The one that doesn't have the problem is in its own namespace.

@EronWright
Copy link
Contributor

EronWright commented Oct 10, 2023

@ffMathy I'm looking into how to leverage the checksum and disregard the local path in the diff calculation, stay tuned. #2251 is a possible duplicate.

@madappa-sharath I have a theory that the issue is related to the use of computed inputs. Is the diff output shown during preview or actual deployment? Do you get different results if you hardcode values for the inputs? If so, which input is the culprit? Thanks.

@EronWright
Copy link
Contributor

@madappa-sharath I think your issue is a dup of #2578.

@EronWright EronWright removed the kind/bug Some behavior is incorrect or out of spec label Oct 16, 2023
@EronWright EronWright added the kind/enhancement Improvements or new features label Oct 16, 2023
@EronWright
Copy link
Contributor

EronWright commented Oct 16, 2023

Thanks again @ffMathy for reporting this issue, I am closing this as a duplicate of #2251, and I posted a PR there.

@EronWright EronWright closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests

5 participants