chartutil.ReadValues unmarshals numbers into json.Number refs #1707

[osdrv]

This change is a second attempt to fix the aforementioned problem. The
first one (#6032) caused a major regression in numeric values #6708.
The new approach takes the experience of the failed attempt under
consideration and introduces some deeper-level changes to the rendering
engine. The proposed change overloads all defined template functions and
converts json.Number arguments in runtime before passing it over to the
rendering functions.

The rest of the description is preserved from the original ticket:

This change is an attempt to address the common problem of json number
unmarshalling where any number is converted into a float64 and represented in a
scientific notation on a marshall call. This behavior breaks things like: chart
versions and image tags if not converted to yaml strings explicitly.

An example of this behavior: k8s failure to fetch an image tagged with a big
number like:
$IMAGE:20190612073634
after a few steps of yaml re-rendering turns into:
$IMAGE:2.0190612073634e+13

Example issue: #1707

This commit forces yaml parser to use JSON modifiers and explicitly enables
interface{} unmarshalling instead of float64. The change introduced might be
breaking so should be processed with an extra care.

Due to the fact helm mostly deals with human-produced data (charts), we have a
decent level of confidence this change looses no functionality helm users rely
upon (the scientific notation).

Relevant doc: https://golang.org/pkg/encoding/json/#Decoder.UseNumber

Signed-off-by: Oleg Sidorov me@whitebox.io

[bacongobbler]

thanks @icanhazbroccoli. Because we're now in the final release candidate stages before we release Helm 3, we are only looking at major regressions or "[bugfix] now or forever hold your peace" fixes for Helm 3. Since this is a progressive bugfix that can be addressed in a feature release, this won't be in the 3.0.0 release. We will take a closer look at this proposal for 3.1.0 after Helm 3 has been released. Just letting you know that it'll take a while before we get around to reviewing this. Thanks!

[osdrv]

@bacongobbler totally makes sense to me, thanks for the heads up!

[mattfarina]

Moving this back to 3.1.0 milestone. I see that makes use of the "C" import and I think this is the first time this is being done in Helm. We need to discuss this in further detail. I've added it to the agenda for the Helm developer call.

[osdrv]

@mattfarina thanks a lot for keeping me posted. That totally makes sense. Please give me a heads up if my presence is wanted. Thank you!

[technosophos]

This was discussed in this morning's Helm Dev Call, and we'll follow up on it.

[thomasfulton]

@technosophos @icanhazbroccoli Just wondering if there is any ETA on when this fix will be released?

[technosophos]

I have not heard any more than what I posted last time. But this PR does need a quick rebase.

[osdrv]

@technosophos Will rebase it somewhere this week.

[osdrv]

@technosophos rebase looks fine, no explosions.

@thomasfulton would it be too much of a favor to ask you to build Helm from this dev branch and confirm it solves the problem you have in mind?

[kostyaplis]

@icanhazbroccoli
I went ahead and performed make build from icanhazbroccoli:icanhazbroccoli/chartutil-no-scientific-notation-v2 branch.
However I'm still getting scientific notation issue.

values.yaml:

image:
  tag: 1234567

$ ./bin/helm version
version.BuildInfo{Version:"v3.1+unreleased", GitCommit:"17f2d703484841ee8adf6db0469cc49db563803e", GitTreeState:"clean", GoVersion:"go1.13.6"}
$ ./bin/helm install test -f values.yaml repo/chart --dry-run --debug

Produced following output (redacted for brevity):

apiVersion: apps/v1
kind: Deployment
metadata:
  name: test
  labels:
    app: test
    version: "1.234567e+06" # we use version: {{ .Values.image.tag | quote }} 
spec:
  template:
    metadata:
      labels:
        app: test
        version: "1.234567e+06"
    spec:
      containers:
        - name: test
          image: "repo/test:1.234567e+06"
          imagePullPolicy: IfNotPresent

[osdrv]

@kostyaplis it's super nice of you, thanks a lot. I'll have a look at it some time soon, I expected a slightly different result. Thank you.

[osdrv]

@kostyaplis hey mate, once again: I appreciate you taking a moment to try this and for pointing me out to the issue. Indeed, one more component (loader) required an identical patch we applied before and without this one I managed to reproduce the problem. I've updated the branch and it renders as expected on my local machine. If it's not too much of a bother, please give it a try yourself. Thanks!

[kostyaplis]

Hi @icanhazbroccoli
Getting same results as before

./bin/helm version
version.BuildInfo{Version:"v3.1+unreleased", GitCommit:"6086ef398ba87ec833dc0a8081386c3bb8accc11", GitTreeState:"clean", GoVersion:"go1.13.6"}

./bin/helm install test -f values.yaml /path/to/chart --debug --dry-run

USER-SUPPLIED VALUES:
image:
  tag: 123456789

COMPUTED VALUES:
image:
  pullPolicy: IfNotPresent
  repository: test
  tag: 123456789

apiVersion: apps/v1
kind: Deployment
metadata:
  name: test
  labels:
    app: test
    version: "1.23456789e+08"
spec:
  template:
    metadata:
      labels:
        app: test
        version: "1.23456789e+08"
    spec:
        containers:
          - name: test
            image: "test/test:1.23456789e+08"

[osdrv]

@kostyaplis your example was super helpful: it seems i missed the CLI value parser :-( Got it patched as well. Thank you!

[kostyaplis]

Confirm that I'm getting correct results (--dry-run at least) with d2b9519 build

[technosophos]

Okay, at this point things are looking solid with this PR. Two questions of the core maintainers:

a) @mattfarina did you have any issues with the present version here?
b) Are there any compatibility breakers if we switch to this method of handling numerics? (e.g. is there anything that worked before, but will not if we apply this patch?)

[osdrv]

Please excuse some extra noise folks, the force-pushed change contains no new functional changes: mainly some naming corrections and extra comments to make the reflect code more human-readable.

[kostyaplis]

@icanhazbroccoli is there anything I can help to get this merged?
We are using ${SHA1:0:7} to tag our dev/stage images, it happens sometimes that first seven chars are all digits, so this still biting us occasionally.

[osdrv]

@kostyaplis thanks for the heads up! I'm afraid I have no data about it. The pr was de-tagged some time ago, which looks like the core team has some concerns (which is totally understandable in the light of the added C dependency). Tagging @technosophos.

[kostyaplis]

@icanhazbroccoli Actually, I don't observe an issue anymore with Helm 3.2.0
Tested just now:
2.16.7 - fails
3.2.0 - numeric values interpreted correctly and image deployed successfully.
There were some Go deps updates for 3.2.0, so maybe thats fixed it...

[osdrv]

@kostyaplis frankly, this surprises me as I replayed the test you provided (ensuring make clean && make build) and it gives me different results. It would be great to double-check the version that gives the right output is tagged 3.2.0 (no stale builds or something).

[bacongobbler]

Perhaps it may be worth going back to the base case to determine if the issue is still present in Helm 3.3.0 before considering moving forward with this PR. Do you agree @icanhazbroccoli?

[osdrv]

@bacongobbler please sorry for my late reply. Totally agree. Let me close the PR for now.