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

Error: validation: chart.metadata.version is invalid #9728

Closed
cforce opened this issue May 20, 2021 · 12 comments
Closed

Error: validation: chart.metadata.version is invalid #9728

cforce opened this issue May 20, 2021 · 12 comments

Comments

@cforce
Copy link

cforce commented May 20, 2021

In helm 3.5.3

Error: validation: chart.metadata.version "20210520.4-04427204" is invalid

Why is that not valid?
At least its valid for git :_)

@bacongobbler
Copy link
Member

bacongobbler commented May 20, 2021

Hi @cforce. We have documented in multiple locations that version strings must comply with the Semantic version 2.0 specification. This enables Helm to support version ranges and automatic upgrades to the latest release. 20210520.4-04427204 is not valid semver.

See https://semver.org/

Hope this helps.

@cforce
Copy link
Author

cforce commented May 21, 2021

The semver regex impl seems buggy,

<version core> ::= <major> "." <minor> "." <patch> "-" <pre-release>
<major> ::= <numeric identifier>   -> there is  no limit specified!!

So why is
2021.0520.404427204
not working ..but
202..0520.404427204
is
What kind of idea is behind limiting me to 999 major versions?

<dot-separated pre-release identifiers> ::= <pre-release identifier>
                                          | <pre-release identifier> "." <dot-separated pre-release identifiers>

Why is
202.10520.4-04427204
not working but
202.10520.4-0.4427204?
According to the spec the points are option in pre-release identifiers

Can you please remove that strict check, make it optional. I don't get why helm needs to enforce that

@bacongobbler
Copy link
Member

bacongobbler commented May 21, 2021

From https://semver.org/#spec-item-2:

A normal version number MUST take the form X.Y.Z where X, Y, and Z are non-negative integers, and MUST NOT contain leading zeroes.

Your example contains a leading zero in the minor version. It is not because it is limiting you to 999 major versions.

Your second example is also considered invalid semver because the minor version is not present, and you cannot have four dot separators in the version number unless the fourth is proceeded by a dash or a plus sign, which would indicate that it is part of the pre-release version or build metadata.

Why is
202.10520.4-04427204
not working but
202.10520.4-0.4427204?
According to the spec the points are option in pre-release identifiers

if you think you found a bug with the regex parser, please open a ticket at https://github.com/Masterminds/semver.

what are you using to test your version numbers for validity?

@cforce
Copy link
Author

cforce commented May 25, 2021

Actually i did not need to follow semver as helm started to force me in the middle of a minor version change.

Semver is a idea from behind the 90s - and in these days of CI/Cd and cloud it might not make sense for everyone.
I really dislike that helm tries to enforce this unnecessarily and even without staying downwards compatible which should be much more important to fulfill. No i am forced useless to change a blueprint we rely on all over the place because a toll wants to dictate the version format for my application cloud deployments.
We use <yyyy>.<mmdd>.<buildid>-<gitcommithash> created by CI/CD as the version for the application and also its helm chart if affected by the commits.

I could do that /by regexpr by my own if i need/want it. So this can be something you enforce when pushing to some remote helm chart repo if its required there , but not generally if i use helm locally.

@mattfarina
Copy link
Collaborator

I understand how people can dislike semver. It's an opinion.

On Helm we have identified roles. Sometimes they are the same people but often times they are different people at different organizations. I think it's useful for a package manager, like Helm, to understand the varying roles involved.

When dealing with upgrades to something, as a consumer of a package, it's useful to know the difference between a breaking API change, a feature addition, and something that's a bugfix. This is something semver communicates that <yyyy>.<mmdd>.<buildid>-<gitcommithash> does not. Especially to a consumer who is at a different organization (a primary situation for packages and packages managers).

@cforce
Copy link
Author

cforce commented Jun 8, 2021

It useful for some for other not. so let the user decide what version shema is needed. Don't dictate that each project has to follow "an around the world" role pattern.
And again its an downwards incompatible breaking change, which is even more worse than possible informational misunderstandings of upwards compatibility.

@mcg1969
Copy link

mcg1969 commented Jun 11, 2021

I don't have a problem with the semver requirement in principle. It took a little wrangling to fix our dev environment, but it's done, and all is well. Still, I hope the following feedback will be taken constructively.

Saying that it is "documented in multiple locations" is not a particularly strong defense, given that it wasn't actually true until 3.5.3 or so. That means there has either been a bug in Helm for years, or a bug in the documentation for that long. Looking through other related issues, it is clear that quite a few legacy helm charts are failing validation now. Even if you are firm in your belief that this has been a longstanding bug, it's now a bug people have been relying upon. See for instance: https://xkcd.com/1172/ :-) It is fair for critics to consider this a breaking change, not a bug fix. The less disruptive solution would have been to consider it a documentation bug, and modify the documentation so that SemVer adherence is a recommendation.

One practical consequence to the new enforcement is that not all valid SemVer values are valid Docker tags. We were using .Chart.Version as our default image tag; we can no longer do that, because the + character is invalid in Docker tags. I had to add a helper function to our Helm chart to convert the offending Chart.Version characters to dashes, and replace all of our image references with that helper.

Regardless of whether the validation check was perceived as correcting a past omission, adding it did break de facto back compatibility. I was disappointed to see that it was inserted in an otherwise minor update. Indeed, in a defense of the use of SemVer above, you said this: It's useful to know the difference between a breaking API change, a feature addition, and something that's a bugfix. I agree, and argue that thinking ought to apply here as well.

But what is done is done, and I do think we'll all move on. That said, I would recommend that the validation error message be made mode verbose, specifically

  1. Explicitly state, in the error message, that the chart version must obey semver format
  2. Calling out that enforcement of this validation began in 3.5.2.

@bacongobbler
Copy link
Member

Saying that it is "documented in multiple locations" is not a particularly strong defense, given that it wasn't actually true until 3.5.3 or so.

Semantic Versioning has been documented as a requirement v2.0.0, as noted in the archives. I placed emphasis in bold on what's allowed and disallowed by the system.

Every chart must have a version number. A version must follow the SemVer 2 standard. Unlike Helm Classic, Kubernetes Helm uses version numbers as release markers. Packages in repositories are identified by name plus version.

For example, an nginx chart whose version field is set to version: 1.2.3 will be named:

nginx-1.2.3.tgz

More complex SemVer 2 names are also supported, such as version: 1.2.3-alpha.1+ef365. But non-SemVer names are explicitly disallowed by the system.

I know this because I was the one advocating for semantic versioning back in the helm-classic days.

Semantic Versioning has been in use since helm-classic's users as well, though we started adding in checks and versioning constraints in version 2 to accommodate certain use cases as noted earlier. helm-classic had no notion of a chart repository back then, so things like automatic upgrades and version constraints were not possible.

So no, this isn't a "new thing" that was introduced in Helm 3.5.3. In fact, if you look at Helm 3.5.3's changelog, there is no mention of ANY changes to the strictness of versioning.

Looking through other related issues, it is clear that quite a few legacy helm charts are failing validation now.

Can you point those issues out, please?

Calling out that enforcement of this validation began in 3.5.2.

Please provide a test case that clearly demonstrates what worked in Helm 3.5.2, then failed in Helm 3.5.3. Once complete we can look into those findings. But right now your comment is neither constructive nor helpful.

@mcg1969
Copy link

mcg1969 commented Jun 11, 2021

Thanks for the response, Matthew! Let me tell you how I came to my conclusion. Until I stumbled into this problem, I had this entry in Chart.yaml:

version: $PLATFORM_VERSION

In our CI builds, we would use an environment variable substitution pass to replace $PLATFORM_VERSION with our version string. However:

  • our PLATFORM_VERSION was not actually SemVer compliant, as it turns out, because we replaced the + with a -, enabling us to use it as both our chart version and docker tag.
  • during local developmen_ we actually didn't do this environment variable substitution at all.

We have supported a variety of Helm 2 and Helm 3 environments with this chart.

Updating from Helm 3.3.4 to 3.6.0, I began to get these validation errors, and I bisected to one of the minor versions of 3.5 (sorry that I didn't remember precisely in my issue above). My investigation suggests that this commit is the one that added semver validation:
657ce55

Prior to this commit the only validation to chart.metadata.version was that it exists.

A little more investigation suggests it was actually 3.5.1 where the change was made, for security reasons:
GHSA-c38g-469g-cmgx

This change doc says this:

In the past, Helm would not permit charts with malformed versions.

I am actually not convinced this is true. (This is not an accusation of dishonestly, just incorrectness.) My experience working with a variety of Helm versions is that both of my particular malformed versions have been accepted for years. The evidence from the code is that version validation was not occurring before 3.5.1. As for v2, Some totally justified refactoring of the helm chart code is making it difficult for me to search for evidence of how the validation was performed in Helm v2 specifically, but if I find some, I'll share. To be fair, the security report indicates that they did not examine Helm 2 for this issue, and instructs users to assume it is vulnerable.

@bacongobbler
Copy link
Member

bacongobbler commented Jun 11, 2021

I am actually not convinced this is true. (This is not an accusation of dishonestly, just incorrectness). My experience working with a variety of Helm versions is that both of my particular malformed versions have been accepted for years.

Regardless, changes to fix security loopholes are allowed as per our contract:

Any chart that worked on a previous version of Helm 3 MUST work on a new version of Helm 3 (barring the cases where (a) Kubernetes itself changed, and (b) the chart worked because it exploited a bug)

In my view this would clearly fall under the second category. Even if previous versions silently allowed invalid versions through, it has been documented since 2.0.0 that this is explicitly disallowed. Your chart exploited a bug - whether intentional or not - and to fix the security issue we had to stop allowing invalid versions from being accepted.

From the notes:

While this fix does not constitute a breaking change, as all field formatting is now enforced as documented, it is possible that charts that were mistakenly allowed (but invalid) may no longer be available in search indexes. Specifically, malformed SemVer versions are no longer supported. This has always been the documented case, but it is true that malformed versions were allowed.

In other words this was an intentional decision to fix a security issue. And as demonstrated earlier, this has been documented since Helm 2.0.0. As such, I'm closing this as an intentional fix that we do not plan to "fix" due to the security issues linked above.

@mcg1969
Copy link

mcg1969 commented Jun 11, 2021

All good, Matthew, thank you for listening!

@mcg1969
Copy link

mcg1969 commented Jun 11, 2021

(love your username btw)

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