Enable "helm upgrade --install" equivalent behavior#1247
Enable "helm upgrade --install" equivalent behavior#1247BBBmau merged 4 commits intohashicorp:mainfrom odenio:upgrade-mode
Conversation
|
|
|
As I noted over in the #425, I've released a test build of this for people to kick the tires on: |
|
Bump: could someone re-approve the checks? I've updated the PR to address the failing ones. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Updated, all tests passing locally. |
|
@bd-spl gentle nudge? :) |
I welcome this change, albeit have no merging powers here. Please try reaching out the owners of this repository. |
|
@alexsomesan can I prevail on you to take a look here? I think this is ready to go. |
|
@alexsomesan nudge? :) |
|
@alexsomesan this is now rebased on top of the current main branch. Is there any chance this could be reviewed before EOY? I've got an internal project that's rather blocked by this and would like to avoid the temptation to use a forked version without a formal 👍 on the approach from your team. |
|
Happy New Year everyone! @alexsomesan my apologies for being a bother, but would it be possible to get the test workflows approved? The tests are passing locally and I believe absent any other feedback that this is ready for submission. |
n-oden
changed the title
|
Is there any news on this? |
|
@lodmfjord I'm honestly not sure what's going on here; we seem to have fallen into a bit of a hole. In the meantime if you'd like to kick the tires on this you can use https://registry.terraform.io/providers/n-oden/helm/0.0.2 which is a fork of the hashicorp helm provider with this PR cherry-picked on top. Obviously the caveat there is that there's no guarantee that they'll ever merge this back upstream, or hashicorp might ask for incompatible state schema changes before they do, so using it in production is very much an at your own risk proposition and I probably can't personally commit to keeping a fork up to date in perpetuity if this PR never gets accepted. |
|
@arybolovlev @SarahFrench @appilon @jrhouston my apologies for the spam, but I'm hoping to get someone to approve running the checks on this PR, which seems to have gotten a little lost. |
|
We're having a look at this, but I personally have to familiarise myself with the helm upgrade mechanism itself before I can evaluate this PR. I'm doing that, in the background of other tasks. |
|
@alexsomesan many thanks, and if I can answer any questions please don't hesitate to ask |
|
@alexsomesan just checking in: I've rebased on the current upstream main branch and dealt with the merge conflicts. Could you approve the testing workflows? |
|
@alexsomesan thanks! I'm a little baffled as to why the 0.12 test is failing, and the output gives no hint. Can you re-run that with debug logging enabled? |
|
FWIW I cobbled together a local test environment and was unable to reproduce the test failure using tf v0.12.31: https://gist.github.com/n-oden/99afa11625eee21c49efddf6ebc896db (It did eventually fail, down in |
|
After some more experimentationI'm gonna strongly assert that whatever was going on with the v0.12.31 test in the previous run was not a result of this PR. I set up a parallel PR in my own repo to make the tests run there and you can see here that it's passing: https://github.com/odenio/terraform-provider-helm/pull/1 @alexsomesan I've rebased this against the current main/HEAD -- can you re-approve the test runs? |
|
@BBBmau okay, updated to use a single boolean config value, updated the documentation to match, removed a now-redundant test. |
| upgradeClient.PostRenderer = pr | ||
| } | ||
|
|
||
| debug("%s Upgrading chart", logID) |
There was a problem hiding this comment.
From running it manually once more I noticed the following flow:
- Install
bitnami/redischart withversion = "19.4.0" - Remove tfstate + remove
version = "19.4.0" terraform init->terraform apply- It proceeds to perform an upgrade to the latest version.
19.6.1
This is something that we'll want to address since this goes against what was discussed prior:
This is good news since this PR should only support
helm upgrade --install, auto upgarding to the latest release is not something we want to support in the provider. This is noted here
I did test every other case and it works as expected, such as:
- installed
bitnami/rediswith no version set, marking the version as the latest (19.6.1)- explicitly add
version = 19.6.0, changing the version19.6.1->19.6.0- remove
versionto see whether the version is upgrade to19.6.1This results in no changes outputted by terraform
I'm requesting changes here since it seems like this would be where we'd want to check if the release exists and if it does use the version associated with the already existing release in order to prevent an accidental upgrade to the latest release. Let me know if you have any questions, we appreciate your effort in this! @n-oden
There was a problem hiding this comment.
I don't love the provider having explicitly different semantics than the default helm tooling, but c'est la guerre; I'll try to get this sorted today.
There was a problem hiding this comment.
Actually... I'm going to push back on this a little, because what you are asking for is directly in opposition to what your own documentation suggests is the expected behavior, which is that if the version field is absent, the latest version will be installed:
"version": {
Type: schema.TypeString,
Optional: true,
Computed: true,
Description: "Specify the exact chart version to install. If this is not specified, the latest version is installed.",
},...which is in alignment with how the helm cli works: if the --version flag is not passed in and the chart is being installed from a repo, the latest version will be picked, and this is true for both helm install and helm upgrade.
Changing this behavior in the case where upgrade_install=true seems to me like it breaks the principle of least surprise, and I think it would be better to explicitly warn in the documentation that the combination of upgrade_install=true and version=null will potentially change the installed release.
That all said commit 0470be1 does what you've requested and adds test coverage for it.
There was a problem hiding this comment.
The version description was a mistake on my part, this was actually brought up in the past where the documentation itself was updated. Unfortunately I did not update the description in the schema itself 😓 #924
In the past, users would try to create a helm_release resource where the release would already be present which where they would get this panic:
│ Error: cannot re-use a name that is still in use
│
│ with helm_release.upgrade,
│ on main.tf line 1, in resource "helm_release" "upgrade":
│ 1: resource "helm_release" "upgrade" {
│ the ideal way to resolve this would be to run terraform import in order to get the already installed release into the tfstate, this also prevents any unexpected changes to the release itself with this error.
The upgrade-install would also resolve this error but we still want to promote being explicit in regards to version bumping, something that's been mentioned in the past #102 (comment)
This is the expected behaviour - we want to encourage the practice of being explicit about the upgrade version when upgrading an installed chart. We'll take a note to improve the documentation referenced above.
This PR adds a (potentially) idempotent "upgrade mode" to the provider, mimicing the behavior of `helm upgrade --install` as defined in https://github.com/helm/helm/blob/main/cmd/helm/upgrade.go To wit: an `upgrade` block is added to the resource attributes, consisting of tool boolean values: `enable` and `install`. If `enable` is true, this causes the provider to create a `*action.Upgrade` client, and attempts to perform an upgrade on the named chart. If `install` is true, it will first create an `*action.History` client to determine if a release already exists; if it does not find one it creates an `*action.Install` client and attempts to install the release from scratch. If a release _is_ found, an upgrade is performed. This allows the resource to potentially co-exist with, e.g., CI/CD systems that could potentially install the release out-of-band from terraform's viewpoint.
|
The failure in acc_test (v0.15.5) does not look like it's in any way obviously relevant to this change; can you try re-running that job? |
Yeah often times the tests can fail due to a network issue. We'll want to add in retries to prevent us from needing to rerun the jobs all the time @JaylonmcShan03 |
|
@n-oden did the same manual test i did before and I see that it uses the latest version rather than the chart version already being used on the chart: when running 32 [DEBUG] [getInstalledReleaseVersion: mau-test-pr] Chart mau-test-pr is installed as release 19.6.1: timestamp=2024-08-09T15:58:32.743-0700
2024-08-09T15:58:33.849-0700 [INFO] provider.terraform-provider-helm_9.9.9_darwin_arm64: 2024/08/09 15:58:33 [DEBUG] [resourceDiff: mau-test-pr] Got chart redis version 19.6.1: timestamp=2024-08-09T15:58:33.849-0700
2024-08-09T15:58:33.850-0700 [INFO] provider.terraform-provider-helm_9.9.9_darwin_arm64: 2024/08/09 15:58:33 [DEBUG] [resourceDiff: mau-test-pr] Release validated: timestamp=2024-08-09T15:58:33.850-0700
2024-08-09T15:58:33.850-0700 [INFO] provider.terraform-provider-helm_9.9.9_darwin_arm64: 2024/08/09 15:58:33 [DEBUG] [resourceDiff: mau-test-pr] setting version to installed version 19.6.1: timestamp=2024-08-09T15:58:33.850-0700This appears with this tfconfig where tfstate is empty as well as the release being installe with version being
resource "helm_release" "test" {
name = "mau-test-pr"
chart = "bitnami/redis"
# version = "19.4.0"
upgrade_install = true
}from └─(16:00:30)──> helm list ──(Fri,Aug09)─┘
NAME NAMESPACE REVISION UPDATED STATUS CHART APP VERSION
mau-test-pr default 18 2024-08-09 15:58:07.823949 -0700 PDT deployed redis-19.4.0 7.2.5 |
|
@BBBmau okay that's weird -- I would think that the test case in TestAccResourceRelease_upgrade_with_install_warmstart_no_version would cover this exact scenario: we preinstall test-chart v1.2.3, apply the upgrade_install plan and then verify that v1.2.3 is still installed. I'll try to figure out what's going on, but can you please attach the full debug log of your tf plan/apply sessions as well as the helm cli commands your ran and their output in a gist? |
|
@BBBmau I don't seem to be able to reproduce this? Take a look at this gist: https://gist.github.com/n-oden/d54d278d3a2ffbc5e2fb81d6f00d3092 The upshot is:
Can you double-check this on your side? |
|
One thing that's probably worth explicitly noting is that this does create a new revision, which means that if there are deployments or statefulsets in your release, it will create a kubernetes rollout event, with the associated pod restarts. It's the moral equivalent of running: ...which if you do it locally you will observe does create rollout events for both the deploy and sts resources. I'm going to assert strongly that this is expected behavior and I'll add a note about it to the documentation. |
also use the test repo in the tests for upgrade_install scenarios
BBBmau
left a comment
BBBmau
left a comment
There was a problem hiding this comment.
When testing on my machine I was unable to get the behavior despite being able to run the tests and getting them to pass. @jrhouston was able to run manual tests on this PR and it all looks to be running as expected.
@n-oden We really appreciate your contribution! Just apply the documentation changes and we'll get this approved and merged 🎉
docs/index.md
Outdated
| * `command` - (Required) Command to execute. | ||
| * `args` - (Optional) List of arguments to pass when executing the plugin. | ||
| * `env` - (Optional) Map of environment variables to set when executing the plugin. | ||
| * `api_version` - (Required) API version to use when decoding the ExecCredentials resource, e.g. `client.authentication.k8s.io/v1beta1`. |
There was a problem hiding this comment.
These extra spaces should be removed.
There was a problem hiding this comment.
I agree! But tfplugindocs generate really wants them to be there, and fixing this breaks the documentation check build. I'll revert them out manually but someone should probably figure out why tfplugndocs keeps applying them.
There was a problem hiding this comment.
yeah for right now we've been ignoring the documentation workflow due to it generating documentation that's just flat out wrong. Thanks for at least attempting to use it though!
BBBmau
added
the
no-documentation
BBBmau
left a comment
BBBmau
left a comment
There was a problem hiding this comment.
This is a big contribution that the community has been asking for for quite sometime. You're more than welcome to pick up other issues that catch your eye if you're interested. Thank you again! 🎉
|
@BBBmau thank you for all of your help along this long and winding road! Now... ETA to the next release of the provider? 🤣 |
|
@n-oden Thank you for your contribution 😄 https://github.com/hashicorp/terraform-provider-helm/releases/tag/v2.15.0 |
6 participants
Description
This PR adds a (potentially) idempotent "upgrade mode" to the provider, mimicking the behavior of
helm upgrade --installas defined in https://github.com/helm/helm/blob/main/cmd/helm/upgrade.goTo wit: an
upgradeblock is added to the resource attributes, consisting of two boolean values:enableandinstall.If
enabledis true, this causes the provider to create a*action.Upgradeclient, and attempts to perform an upgrade on the named chart. Ifinstallis true, it will first create an*action.Historyclient to determine if a release already exists; if it does not find one it creates an*action.Installclient and attempts to install the release from scratch. If a release is found, an upgrade is performed. This allows the resource to potentially co-exist with, e.g., CI/CD systems that could potentially install the release out-of-band from terraform's viewpoint.Acceptance tests
Release Note
Release note for CHANGELOG:
References
Community Note