kustomize 3.5.4 configMapGenerator deletes newlines

[kantica]

I have one problem with latest release version of kustomize (v 3.5.4). This version deletes all empty lines from ConfigMaps generated by configMapGenerator. Previous version(s) did not do this.

Example:

$ ./kustomize-3.5.3 version
{Version:kustomize/v3.5.3 GitCommit:5ba90fe5ef1dc4599e359edd41d1d0e6373b247d BuildDate:2019-12-17T21:57:37Z GoOs:linux GoArch:amd64}
$ ./kustomize-3.5.4 version
{Version:kustomize/v3.5.4 GitCommit:3af514fa9f85430f0c1557c4a0291e62112ab026 BuildDate:2020-01-11T03:12:59Z GoOs:linux GoArch:amd64}
$ ./kustomize-3.5.3 build . > build.3.5.3.yaml
$ ./kustomize-3.5.4 build . > build.3.5.4.yaml
$ diff -urN build.3.5.3.yaml build.3.5.4.yaml 
--- build.3.5.3.yaml    2020-01-17 12:41:30.026148995 +0100
+++ build.3.5.4.yaml    2020-01-17 12:41:26.058072165 +0100
@@ -3,33 +3,22 @@
   nginx.conf: |
     user  nginx;
     worker_processes  1;
-
     error_log  /var/log/nginx/error.log warn;
     pid        /var/run/nginx.pid;
-
-
     events {
         worker_connections  1024;
     }
-
-
     http {
         include       /etc/nginx/mime.types;
         default_type  application/octet-stream;
-
         log_format  main  '$remote_addr - $remote_user [$time_local] "$request" '
                           '$status $body_bytes_sent "$http_referer" '
                           '"$http_user_agent" "$http_x_forwarded_for"';
-
         access_log  /var/log/nginx/access.log  main;
-
         sendfile        on;
         #tcp_nopush     on;
-
         keepalive_timeout  65;
-
         #gzip  on;
-
         include /etc/nginx/conf.d/*.conf;
     }
 kind: ConfigMap

Files used for kustomize build can be found at https://gist.github.com/kantica/95b2b7f491cc2cb99d4307c80d947be9

[chrko]

I'm experiencing the same issues but with binary files (JKS keystores…) and also within the secret generator.

[chrko]

Bisecting yields the commit bda865e9e46727f04d21f73c6800b41f55ef4004

kustomization.yaml:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
configMapGenerator:
  - name: test
    files:
      - multiline.txt
generatorOptions:
  disableNameSuffixHash: true

multiline.txt

Foo
Bar

Foooo

Baaaar

git checkout kustomize/v3.5.3
(cd kustomize && go install)
cd /home/christian/work/contributing/debug-kustomize-v3.5.4 && ~/go/bin/kustomize build . > result && sha256sum result > check

git checkout kustomize/v3.5.4
git bisect start
git bisect bad
git bisect good kustomize/v3.5.3
(cd kustomize && go install . && cd /home/christian/work/contributing/debug-kustomize-v3.5.4 && ~/go/bin/kustomize build . > result && sha256sum -c check) && git bisect good || git bisect bad

[chrko]

And bisecting with further yields: fa8f504ff43551a1f0fe5d7c74b450553e652b15

[chrko]

So, this seems to be the intended behavior as of #2025. I would consider this especially for binary files as a huge error.
As written above my use case is to include a JKS keystore (you, java stuff…).

[kantica]

I don't have anything against trimming trailing spaces, but this also "trims" everything to one '\n', which is IMO wrong. It seems content of the file is available as one string, and regex trims "\s*\n" on every position in string (file representation).
https://github.com/kubernetes-sigs/kustomize/blob/89367be0085b45712b8807673c77d1e3327f15f9/api/kv/kv.go#L95

^- This should be rewritten to only trim whitespaces on end of each line.

[kantica]

Something like this should probably work better than current implementation.

func trimTrailingSpacesInLines(str string) string {
        var line string
        var s string
        var err error
        reader := bufio.NewReader(strings.NewReader(str))
        re := regexp.MustCompile(`\s*\n`)
        for {
                line, err = reader.ReadString('\n')
                s = s + re.ReplaceAllString(line, "\n")
                if err != nil {
                        break
                }
        }
        return s
}

[chrko]

Something like this should probably work better than current implementation.

func trimTrailingSpacesInLines(str string) string {
        var line string
        var s string
        var err error
        reader := bufio.NewReader(strings.NewReader(str))
        re := regexp.MustCompile(`\s*\n`)
        for {
                line, err = reader.ReadString('\n')
                s = s + re.ReplaceAllString(line, "\n")
                if err != nil {
                        break
                }
        }
        return s
}

No. This also modifies binary data.
I think the original issue which generate the corresponding pull request #2025, #1868 is misunderstood. The error lies no within the data itself but how it will be outputted.
I still consider that #2025 should be immediately reverted and the issue #1868 have to be fixed at the output generation stage.

[jeanfabrice]

+1
this is also causing trouble when configmap stores openldap ldif data, where each entry has to be separated from the others with a newline

[phanimarupaka]

Here is the PR to address the issue.

https://github.com/kubernetes-sigs/kustomize/pull/2129

[phanimarupaka]

The PR has been merged. You can pull the latest master and try it out if you still have any issues. You can see the change in next release.

[Liujingfang1]

Fixed by #2129. Thank you, @phanimarupaka

[chrko]

No. This also modifies binary data.
I think the original issue which generate the corresponding pull request #2025, #1868 is misunderstood. The error lies no within the data itself but how it will be outputted.
I still consider that #2025 should be immediately reverted and the issue #1868 have to be fixed at the output generation stage.

@Liujingfang1 @phanimarupaka As written above this currently modifies also binary data within configMaps and secrets. I'll still consider modifying (binary) data as an unexpected behaviour.

E.g. compare the result within k8s resulting of these two actions:

kubectl create cm example-cm --from-file multiline.txt

and the example given above https://github.com/kubernetes-sigs/kustomize/issues/2125#issuecomment-576277396.

My expected behaviour of the feature of kustomize's configMapGenerator and secretGenerator, that supplying a file yields the same representation in k8s as on disk. This assumption breaks by modifying the underlying data and not the output generation stage.

[xperimental]

@phanimarupaka Can you please reopen this issue as #2129 does not fix it?

The PR only modifies the behaviour, so that it does not remove repeating newlines, but just "trailing whitespace" before a newline character. This _should_ work fine when you are dealing with textfiles where trailing whitespace is usually a thing that you want to avoid, but correct behaviour can not be guaranteed with arbitrary inputs.

In my opinion Kustomize should not modify the input to configmaps or secrets at all, but just pass them on verbatim.

[freym]

@Liujingfang1
This also breaks the creation of secrets with an GPG Key. GPG needs a empty line between the armor header and the payload, which is removed by kustomize. Please add an Option to disable this behavior.

[Liujingfang1]

@phanimarupaka What about we add an option to disable deleting newlines, similar to the hash suffix?

[ambis]

Please make the trim an opt-in option. Or fix this proper, so no trimming needs to be done.

This is going to catch people off guard. And there is potential for really major f-ups when your backup scripts etc. are modified without you realizing.

This is what happened to me:

backup-software \
  --exclude foo \
  --exclude bar \
  dirs \

other-command

#Effectively:
# backup-software   --exclude foo   --exclude bar   dirs 
# other-command

But instead without the extra newline:

# backup-software   --exclude foo   --exclude bar   dirs   other-command

Not saying it's necessarily a great idea to end a command with \ and a newline in general, but that's what I got and it could have been much worse.

[brichins]

This bug is crippling and caused a production issue. We can’t use kustomize since this change since some of our secrets are binary. Please revert this change for backwards compatibility.

[gottwald]

Adding an opt-in to disable deleting newlines makes it at least possible to use kustomize again if you're currently affected, but it still doesn't protect you from falling into this trap while upgrading or creating new config maps.

Maybe I'm missing something, but to weigh the pros and cons of the two behaviors, I'd ask the question of what benefit the removal of newlines brings to the system?
The only thing I can come up with is that the resulting config map looks neater / cleaner but that comes at a high risk of breaking setups in certain constellations.

I don't think the pros outweigh the cons.
If anything I'd make the current line-removal behavior an optional flag.

[ambis]

@gottwald How I understood this from the initial PR, the problem was purely technical, where kustomize rendered configmaps with leading whitespaces on lines incorrectly to yaml.

This was an attempted fix gone very bad AFAIK. To my knowledge, no one wants to particularly touch the content other than make the yaml rendering work.

Please correct me if I'm wrong! :)

[kantica]

This issue is more related to kubectl than to kustomize. kustomize should not transform/modify input in any way.

To test this with kubectl.

Representation with trailing space:

$ kubectl -n default create cm trailing-space-cm --from-file=test=<(echo -en "first line w trailing space \nsecond line wo trailing space\nthird line w trailing space \nfourth line wo trailing space and newline")
configmap/trailing-space-cm created
$ kubectl -n default get cm trailing-space-cm -o yaml
apiVersion: v1
data:
  test: "first line w trailing space \nsecond line wo trailing space\nthird line w
    trailing space \nfourth line wo trailing space and newline"
kind: ConfigMap
metadata:
  creationTimestamp: "2020-05-06T09:42:35Z"
  name: trailing-space-cm
  namespace: default
  resourceVersion: "108963018"
  selfLink: /api/v1/namespaces/default/configmaps/trailing-space-cm
  uid: 4bcd0634-b379-4c4f-9d91-3a6e31f62ba1

Representation without trailing space:

$ kubectl -n default create cm wo-trailing-space-cm --from-file=test=<(echo -en "first line\nsecond line\nthird line\nfourth line")
configmap/wo-trailing-space-cm created
$ kubectl -n default get cm wo-trailing-space-cm -o yaml
apiVersion: v1
data:
  test: |-
    first line
    second line
    third line
    fourth line
kind: ConfigMap
metadata:
  creationTimestamp: "2020-05-06T09:43:53Z"
  name: wo-trailing-space-cm
  namespace: default
  resourceVersion: "108963277"
  selfLink: /api/v1/namespaces/default/configmaps/wo-trailing-space-cm
  uid: 18dae996-d4d5-4cac-8496-955c4a416b09

[phanimarupaka]

Agree, better to revert the change rather than introducing opt-in as it might have further implications for too few benefits.

https://github.com/kubernetes-sigs/kustomize/pull/2441

[via-jordan-sokolic]

Just got bit by this bug trying to generate a secret from a binary JKS keystore.
A single byte omission (in this case a form feed, 0x0c) corrupts the keystore and causes a Java EOFException.

Just built from source @abb9d5a5 (including #2441) which seems to fix it.

Please push a new release!

[phanimarupaka]

New release has the fix.

https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize%2Fv3.5.5

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[monopole]

Assuming this has been fixed.