Post-build substitution can mess up output

[wolfmah]

I was in the process of creating a CronJob, in our Flux 2 workflow, and got hit by a weird behavior. At first, I had to put the annotation kustomize.toolkit.fluxcd.io/substitute: disabled, otherwise, what was found inside the command action was not executable. I dug deeper to find the cause and here's what I found.

Here is a full simple CronJob with the faulty behavior (I will not repeat the complete manifest for subsequent examples). A thing to note is the usage of multi-line as well as the extra space at the end of the command.

apiVersion: batch/v1beta1
kind: CronJob
metadata:
  name: test-multiline-space
  namespace: default
spec:
  suspend: false
  schedule: 0 */6 * * *
  failedJobsHistoryLimit: 1
  successfulJobsHistoryLimit: 1
  jobTemplate:
    spec:
      template:
        spec:
          restartPolicy: Never
          containers:
            - image: debian:latest
              name: test
              imagePullPolicy: IfNotPresent
              command:
                - /bin/bash
                - -ce
                - |-
                  a \
                    -b c 

When parsed through kustomize build, this result in a command that look like thi:

> kustomize build test | yq eval '. | select(.metadata.name == "test-multiline-*space") | .spec.jobTemplate.spec.template.spec.containers[0]' -

command:
  - /bin/bash
  - -ce
  - "a \\\n  -b c "

When adding substitution into the mix (via drone/envsubst), the output changes to this:

> kustomize build test | $GOPATH/bin/envsubst | yq eval '. | select(.metadata.name == "test-multiline-*space") | .spec.jobTemplate.spec.template.spec.containers[0]' -

command:
  - /bin/bash
  - -ce
  - "a \\n  -b c "

If you didn't saw the difference, there's a backslash missing.

I really don't know why this behavior occurs when activating envsubst. Disabling substitution via the annotation allows the YAML to be recognize correctly by Kubernetes (with the triple backslash). On the other hand, removing the trailing whitespace changes the Kustomize output considerably, which doesn't get affected by substitution:

# manifest
...
              command:
                - /bin/bash
                - -ce
                - |-
                  a \
                    -b c

# kustomize output
command:
  - /bin/bash
  - -ce
  - |-
    a \
      -b c

# kustomize + envsubst output
command:
  - /bin/bash
  - -ce
  - |-
    a \
      -b c

Expected behavior

When there an extra space at the end of a multi-line string, Flux's substitution shouldn't remove any char from the string, but simply replace vars found within ${}.

Of course, the easy fix is to not add the extra space. And there's a workaround with the annotation that disable the substitution. But if both are not met, Flux's substitution will silently mangle up the manifest.

[kingdonb]

I'm sorry this issue has affected you! Thanks for the great description of how to reproduce this issue. I see some points in your report that I want to highlight, as it took me a few read-throughs to get all the way to the full understanding:

• You are not using envsubst. This is not a problem with misusing envsubst, it just happens that having the envsubst feature enabled messes with and indeed even spoils your manifest. This is clearly not desirable.

I wrote a response to this issue having only partially understood, missing this main point above. I'm just going to post it anyway because it still contains relevant information and I am short on time, having taken time to write this all out already... please know that it was written with the person in mind who is actively misusing the envsubst feature to handle some multi-line update to a YAML file, which requires that space you didn't mean to include, (who I anticipate we will hear from sooner or later.)

In our Flux dev meeting yesterday, it was discussed that a doc update is needed around the envsubst / PostBuild Substitution feature to explain what use cases it should and should not be expected to serve.

One of the use cases that was mentioned was for multi-line strings and YAML blocks. It is known and understood that the envsubst feature is a stringly-typed substitution, in other words it is not parsing YAML and updating the data structure that resulted from the YAML parse, it is parsing templates and updating the content within the bounds of the template. One might infer from this that you should be able to do surprising tricks like multi-line YAML strings, but this is meant to be discouraged.

The plan right now is to update the documentation so that we make it clear these surprising things are not supported, the envsubst feature is meant to be used for substituting a simple (string) value into a single YAML key's entry. The other issue that happens is https://github.com/fluxcd/flux2/issues/1839 where users expect non-string values to be treated as strings. I believe we want to rectify the issue behind fluxcd/flux2#1839 so that users do not have to know some weird double-escaped quote inside of quote trick to get their values accepted by Kustomize.

I am not sure how well this problem can be solved given that some substitutions downstream may expect a non-string value type, like for example replicas, but I don't think anything bad can happen if you pass in a string that parses as an integer in this case, it should be silently coerced into an integer, I think.

But what we should warn more strongly about is that you are manipulating the YAML document string itself, and you are not meant to do what I believe you have done, which is to make a change that requires some extra spaces in just the right place, so that the indentation formatting of the original YAML document is not messed up.

You can try the same suggestion I gave for that issue, more concisely described here: https://github.com/fluxcd/flux2/discussions/1795

...but the result for your multi-line string will not be supported and if it works, it should not be depended on to work this way forever. The existence of this issue and need for this workaround is in my opinion strictly a bug, though it is still not obvious how it can be fixed.

... We are going to document the bug for now, and explain through documentation how to work around the bug, then eventually hopefully fix the bug, (which I will further speculate: may require some users to update their configuration if they had cause in their configs to use this workaround.)

I'm glad you found a workaround, which was not to add the incidental space at EOL of the multi-line string, so if I understood correctly, you were able to work around this issue one of either two ways:

1. do not put that space at the EOL which caused the line to be double-quoted, (that definitely had something to do with the missing backslash that I guess was consumed during envsubst)
2. disable envsubst globally with an annotation at the head of the file

[wolfmah]

Yeah, the report may not be the best. I'm having a hard time wrapping my head around this in a cohesive way.

You are not using envsubst. This is not a problem with misusing envsubst, it just happens that having the envsubst feature enabled messes with and indeed even spoils your manifest. This is clearly not desirable.

Exactly! The parent Kustomization object, that include the faulty CronJob as resource, have variables substitutions enabled. Though, this particular CronJob is using none of them.

so if I understood correctly, you were able to work around this issue one of either two ways:

Yes!

The best solution is to simply not have the extra space at the end of a multiline string. That way, Kustomize doesn't output the string as a one-liner, thus by-passing all the backslash shenanigans. Though, this rely on the fact that Kustomize should not change it's behavior in the future on how to present multiline strings in it's output.

Solution 2 is more future proof, though only if the manifest doesn't requires the use of substitution.