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 chart v3.21.2 is broken for terraform #5162

Closed
viceice opened this issue Dec 7, 2021 · 10 comments
Closed

Helm chart v3.21.2 is broken for terraform #5162

viceice opened this issue Dec 7, 2021 · 10 comments

Comments

@viceice
Copy link

viceice commented Dec 7, 2021

I'm using the terraform helm_release to deploy the operator:

resource "helm_release" "tigera-operator" {
  name = "tigera-operator"

  repository = "https://docs.projectcalico.org/charts"
  chart      = "tigera-operator"
  version    = "v3.21.1"

  max_history = 10

  values = [
    file("${path.module}/tigera-operator/values.yaml")
  ]
}

values.yaml

installation:
  # Configures Calico networking.
  calicoNetwork:
    mtu: 1400
    nodeAddressAutodetectionV4:
      firstFound: false
      cidrs:
        - 172.xx.yy.0/24
        - 172.xx.zz.0/24
    # Note: The ipPools section cannot be modified post-install.
    ipPools:
    - blockSize: 26
      cidr: 10.xx.0.0/16
      encapsulation: IPIPCrossSubnet
      natOutgoing: Enabled
      nodeSelector: all()

if update the version to v3.21.2, it will break with the error i posted below 😕

Originally posted by @viceice in #5143 (comment)

image

@lwr20
Copy link
Member

lwr20 commented Dec 7, 2021

So the entry in the helm index at https://docs.projectcalico.org/charts/index.yaml for v3.21.1 is:

  - apiVersion: v2
    appVersion: v3.21.1
    created: '2021-12-06T19:36:19.79745053Z'
    description: Installs the Tigera operator for Calico
    digest: f963c1ae6dab7f0a63f51528296c82e294c977cd2cab1915fc0a8d771b3b4ab9
    name: tigera-operator
    urls:
    - https://github.com/projectcalico/calico/releases/download/v3.21.1/tigera-operator-v3.21.1-0.tgz
    version: v3.21.1

And for v3.21.2 is:

  - apiVersion: v2
    appVersion: v3.21.2
    created: '2021-12-06T19:36:19.798858941Z'
    description: Installs the Tigera operator for Calico
    digest: 7c2a106419dd84ca225790d34d0feab7dfaa5761cd2773fb61a432110eb24e61
    name: tigera-operator
    urls:
    - https://github.com/projectcalico/calico/releases/download/v3.21.2/tigera-operator-v3.21.2-0.tgz
    version: v3.21.2

I don't see any material difference between the two.

The only slightly odd thing I see is that v3.21.2 is at the bottom of the list, whereas v3.21.1 is at the top.

Is there any way you can confirm that that's the issue? If so, I can probably update the helm index updating tool to order the list before writing.

@mgleung
Copy link
Contributor

mgleung commented Dec 7, 2021

To fix some issues in our release process, we made some changes to the release script which would cause the chart version to more closely match the file names of the charts we uploaded. This means that tigera-operator-v3.21.2-0.tgz will have a chart version of v3.21.2-0 even though this is the helm chart for v3.21.2.

It seems that terraform expects the helm index version to match the version embedded in the chart that it pulls. This leaves us with 2 options:

  1. Revert the embedded version to match the helm index version
  2. Keep the embedded version and change the helm index version to match

Option 1 brings things back to how they were while option 2 makes our versioning a little strange (since semver denotes the version separated by a hyphen as pre-release versions), but it gives us the ability to release new helm charts separate from our normal patch releases. WDYT?

@viceice
Copy link
Author

viceice commented Dec 7, 2021

I would prefer option 1.

Maybe you should use a separate versioning for the chart? This is what most other helm charts do.

@mgleung
Copy link
Contributor

mgleung commented Dec 7, 2021

@viceice I'm not super familiar with how helm versioning usually works, but is there a separate field that other helm charts use to denote the helm chart versions separately? I saw that there is appVersion but that doesn't quite seem to match what we're looking for since that seems to more accurately describe the Calico version in this case. We could of course just denote what version the chart is in the .tgz name only, but that seems like a hacky solution that is prone to mistakes via renaming that I would like to avoid if possible.

@viceice
Copy link
Author

viceice commented Dec 7, 2021

Yes, the app version is usually used for the main docker image version. Eg the calico operator version.

The chart version should strictly follow semver.

We have a tool, renovate, which can update the app version and bump the chart version accordingly. 😉

@mgleung
Copy link
Contributor

mgleung commented Dec 7, 2021

I guess in the cases I am thinking about, if the app version is supposed to represent the docker image's version (in our case the operator version) and if we made a mistake in the helm chart, what field would we use to denote a new helm chart version? In this case, the change isn't in the docker image so the app version field wouldn't really be appropriate. Is there another field we could use to purely denote the chart version?

@viceice
Copy link
Author

viceice commented Dec 7, 2021

I this case you should bump the chart version, which don't need to follow the app version

@mgleung
Copy link
Contributor

mgleung commented Dec 7, 2021

Hmmm. I guess it's more that we haven't been using the chart version properly. It would be great though if it didn't have to divert from the Calico version for ease of use but it sounds like we don't have any other options.

@viceice
Copy link
Author

viceice commented Dec 8, 2021

Yes, we've the same issue. Currently our chart will only published on main docker image changes, see

@mgleung
Copy link
Contributor

mgleung commented Dec 15, 2021

@viceice I just went ahead and checked our fixes to the chart referenced in the helm index and it seems like it should work now so I'm going to close this issue. Please feel free to shout out if you're still hitting this issue.

@mgleung mgleung closed this as completed Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants