kustomize fails to apply overlay if key in base yaml is numeric and missing

[Hammond95]

What happened?

kustomize failed to apply an overlay.

More details in the "How can we reproduce" section

What did you expect to happen?

I expected it to apply the overlay correctly, by overriding the key that is already defined in the base and adding the key that isn't present in the base.yaml.

How can we reproduce it (as minimally and precisely as possible)?

This bug has occurred in a real production scenario, here follows some instruction to reproduce and test the issue.

Instructions to reproduce

filesystem tree

.
├── base.yaml
├── kustomization.yaml
├── overlays
│   └── overlay.yaml
└── test-kustomize-versions.sh

files

base.yaml

apiVersion: v1
kind: ConfigMap
metadata:
  name: blabla
  namespace: blabla-ns
data:
  "6443": "foobar"

overlays/overlay.yaml

apiVersion: v1
kind: ConfigMap
metadata:
  name: blabla
  namespace: blabla-ns
data:
  "6443": "barfoo"
  "9110": "foo-foo"

kustomization.yaml

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization  
bases:
  - base.yaml
patchesStrategicMerge:
  - overlays/overlay.yaml

test-kustomize-versions.sh

#!/usr/bin/env bash

echo "---------------------------------------------------------"

for version in $(asdf list kustomize | tr -d ' '); do
	asdf local kustomize "$version"
	echo "Running kustomize $(kustomize version --short)"

	kustomize build && echo "SUCCESS" || echo "FAILED"
	echo "---------------------------------------------------------"
	sleep 1
done

Expected output

apiVersion: v1
data:
  "6443": barfoo
  "9110": "foo-foo"
kind: ConfigMap
metadata:
  name: blabla
  namespace: blabla-ns

Actual output

./test-kustomize-versions.sh

---------------------------------------------------------
Running kustomize {kustomize/v4.2.0  2021-06-30T22:49:26Z  }
Error: map[string]interface {}{"apiVersion":"v1", "data":map[interface {}]interface {}{9110:"foo-foo", "6443":"barfoo"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"name":"blabla", "namespace":"blabla-ns"}}: json: unsupported type: map[interface {}]interface {}
FAILED
---------------------------------------------------------
Running kustomize {kustomize/v4.3.0  2021-08-24T19:24:28Z  }
Error: map[string]interface {}{"apiVersion":"v1", "data":map[interface {}]interface {}{9110:"foo-foo", "6443":"barfoo"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"name":"blabla", "namespace":"blabla-ns"}}: json: unsupported type: map[interface {}]interface {}
FAILED
---------------------------------------------------------
Running kustomize {kustomize/v4.4.1  2021-11-11T23:36:27Z  }
Error: map[string]interface {}{"apiVersion":"v1", "data":map[interface {}]interface {}{9110:"foo-foo", "6443":"barfoo"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"name":"blabla", "namespace":"blabla-ns"}}: json: unsupported type: map[interface {}]interface {}
FAILED
---------------------------------------------------------
Running kustomize {kustomize/v4.5.0  2022-02-02T00:20:43Z  }
Error: map[string]interface {}{"apiVersion":"v1", "data":map[interface {}]interface {}{9110:"foo-foo", "6443":"barfoo"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"name":"blabla", "namespace":"blabla-ns"}}: json: unsupported type: map[interface {}]interface {}
FAILED
---------------------------------------------------------
Running kustomize {kustomize/v4.5.7  2022-08-02T16:35:54Z  }
Error: map[string]interface {}{"apiVersion":"v1", "data":map[interface {}]interface {}{9110:"foo-foo", "6443":"barfoo"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"name":"blabla", "namespace":"blabla-ns"}}: json: unsupported type: map[interface {}]interface {}
FAILED
---------------------------------------------------------

Kustomize version

4.5.7

Operating system

None

[yuwenma]

/triage accepted

This output error indicates a YAML anchor issue that we previously encountered
Some risk conditions could be left around patchesStrategicMerge.

I tried a previous version kustomize/v3.3.0 (no kyaml dependency ) and it gives the expected output.

[yuwenma]

/triage accepted

[Hammond95]

@yuwenma Could you please elaborate more on that? In the example I provided we don't have defined any YAML anchor 🤔

[yuwenma]

@Hammond95 Sorry for the confusion. The issue I linked is named "YAML Anchor support". kyaml library is where kustomize uses to parse YAML manifests. It looks like the double quote string for "patchesStrategicMerge" paths cannot be correctly parsed, while the others (e.g. if putting the overlays/overlay.yaml in another path like "bases") work well.

Putting the overlays/overlay.yaml in the "resources" path
e.g.

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization  
bases:
 - overlays/overlay.yaml

kustomize build can parse the ConfigMap correctly. (kustomize version is v4.5.4)

I verified that with a different version that does not have the kyaml dependency, and the same patchesStrategicMerge path actually works.

[yuwenma]

I need to take a deeper look at the patchesStrategicMerge code (sometime after the holiday season). But if @KnVerey has a good idea (who knows better than me about the kustomize YAML anchor issue), please feel free to take over the issue.

[ephesused]

I'm not sure if there are larger implications, but I believe the issue is simply that the creation of a new field currently does not apply an appropriate style when the key requires it. Assuming I have that correct, I believe #4948 will resolve the issue.

[ephesused]

It's worth noting that #4948 is coded to ensure the key is viable as a string, since there's an expectation that the content will get json-processed along the way.

Perhaps a better (and more involved) fix would be to have walkMap pass not only field names of the keys, but a slice of field name / style pairs. That way FieldSetter.Filter's field creation could honor whatever style came from the key's source node.

[Hammond95]

Hi @ephesused, thank you for your effort. I was trying to build the project to test if your change does solve our problem, but I couldn't get to build it.

It's worth noting that #4948 is coded to ensure the key is viable as a string, since there's an expectation that the content will get json-processed along the way.

What do you mean when you say that the key is viable as a string?

Would this:

apiVersion: v1
kind: ConfigMap
metadata:
  name: blabla
  namespace: blabla-ns
data:
  "6443": "foobar"

have a different behaviour il the key is not a string?

apiVersion: v1
kind: ConfigMap
metadata:
  name: blabla
  namespace: blabla-ns
data:
  6443: "foobar"

What if we have this use case here?

apiVersion: v1
kind: ConfigMap
metadata:
  name: blabla
  namespace: blabla-ns
data:
  "6443": "foobar"
  6443: "intfoobar"

I think that we may have internally developers using both string-numbers and int-numbers as keys, so If it does only expect a string as a key, I am afraid it won't cover the other use cases.

[ephesused]

It's worth highlighting that I'm no expert here - take my answers below as my perspective, not definitive answers.

Hi @ephesused, thank you for your effort. I was trying to build the project to test if your change does solve our problem, but I couldn't get to build it.

I do my builds under a golang docker image.

It's worth noting that #4948 is coded to ensure the key is viable as a string, since there's an expectation that the content will get json-processed along the way.

What do you mean when you say that the key is viable as a string?

The current handling does not retain the fact that the patch's key ("9110") was a quoted value. When the quotes are dropped, that leads to an internal processing state where one key ("6443") is a string but the other (9110) is an int. That means that the content cannot be handled cleanly and yields this error:

Error: map[string]interface {}{"apiVersion":"v1", "data":map[interface {}]interface {}{9110:"foo-foo", "6443":"barfoo"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"name":"blabla", "namespace":"blabla-ns"}}: json: unsupported type: map[interface {}]interface {}

This issue arises only for non-matching keys. Keys that match retain the quote style of the original key (like with "6443").

As you can see in the error message, kustomize is taking the content and processing it through a json engine. Since json requires string keys, the change I proposed enforces a string type on the new key when its content isn't recognizable as a string. In this case, the content is numeric.

I mentioned above that a likely better fix would be to retain the patch key's quoting style. In cases where keys aren't defined as strings, the change I've proposed would lead to failures. My thinking is that string keys are more common (such as in ConfigMaps), so the proposed change - while not perfect - would be an improvement.

apiVersion: v1
kind: ConfigMap
metadata:
  name: blabla
  namespace: blabla-ns
data:
  6443: "foobar"

Even if kustomize were to process this cleanly, I would think the result wouldn't work for kubernetes since a ConfigMap requires keys to be strings. I've never tried it, though, so if it's working for you then perhaps there's some special handling somewhere.

What if we have this use case here?

apiVersion: v1
kind: ConfigMap
metadata:
  name: blabla
  namespace: blabla-ns
data:
  "6443": "foobar"
  6443: "intfoobar"

I think that we may have internally developers using both string-numbers and int-numbers as keys, so If it does only expect a string as a key, I am afraid it won't cover the other use cases.

As above, I don't see how this would work. This one has a second problem in that the key types vary (one is a string and the other is an int). While I don't believe YAML itself requires matching types across keys, I would expect both kustomize and kubernetes to require consistency of key types.

Edited to correct link syntax for markdown.

[Hammond95]

The current handling does not retain the fact that the patch's key ("9110") was a quoted value. When the quotes are dropped, that leads to an internal processing state where one key ("6443") is a string but the other (9110) is an int. That means that the content cannot be handled cleanly and yields this error:

Error: map[string]interface {}{"apiVersion":"v1", "data":map[interface {}]interface {}{9110:"foo-foo", "6443":"barfoo"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"name":"blabla", "namespace":"blabla-ns"}}: json: unsupported type: map[interface {}]interface {}

This issue arises only for non-matching keys. Keys that match retain the quote style of the original key (like with "6443").

As you can see in the error message, kustomize is taking the content and processing it through a json engine. Since json requires string keys, the change I proposed enforces a string type on the new key when its content isn't recognizable as a string. In this case, the content is numeric.

Great thank you for the clarification, now I see your point.

I mentioned above that a likely better fix would be to retain the patch key's quoting style. In cases where keys aren't defined as strings, the change I've proposed would lead to failures. My thinking is that string keys are more common (such as in ConfigMaps), so the proposed change - while not perfect - would be an improvement.

Yeah, unfortunately the problem is that YAML does support more types as keys and also some fancy stuff like anchors and aliases, whilst JSON only supports strings.

I didn't know that kubernetes only supported (map[string]string) or (map[string][]byte), but it makes sense to me, that since kustomize is a tool specific to k8s world and not a generic YAML docs parser, it should only support/allow string keys and try to cast int keys automatically.

I think that we may have internally developers using both string-numbers and int-numbers as keys, so If it does only expect a string as a key, I am afraid it won't cover the other use cases.

As above, I don't see how this would work. This one has a second problem in that the key types vary (one is a string and the other is an int). While I don't believe YAML itself requires matching types across keys, I would expect both kustomize and kubernetes to require consistency of key types.

I know that this surely works into the YAML world, I have never checked if within k8s int keys are being automatically converted to strings, I will try to check this internally.

My concern was about the edge cases where we process a valid YAML with multiple keys (like "9110" and 9110), and then parse it into the JSON world, we would probably get a duplicate key error in the JSON world.
One may say that this should be a concern of the developers and that the tool shouldn't care about this, and personally I am agree.

[Hammond95]

Any news on this? @yuwenma

[Hammond95]

Could someone proceed with the review+merge process for this PR? #5005

@yuwenma anyway I could help to accelerate this process?