fix: preserve resolved scalar type for replacement-created fields - #6211
fix: preserve resolved scalar type for replacement-created fields#6211onematchfox wants to merge 2 commits into
Conversation
…n bug Demonstrates a bug where a strategic merge patch that only touches an unrelated part of a resource corrupts the type of sibling fields created by a `replacements` entry with `options.create: true`, turning e.g. `readOnly: true` into `readOnly: "true"`. Two cases are covered: - `TestReplacementsCreateBoolFieldSurvivesUnrelatedStrategicMergePatch`, where the replacement source is a ConfigMap (whose `data` values are always strings). - `TestReplacementsCreateBoolFieldFromLocalConfigSurvivesUnrelatedStrategicMergePatch`, where the source is a local-config resource with a natively typed boolean field, showing the bug is not specific to string-typed sources. Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Every field created by a `replacements` entry (`options.create: true`) was left with an empty YAML tag on its scalar value, since `setFieldValue` only ever copied `.Value` onto the newly created node and never resolved a type for it. This was invisible in isolation, because encoding an untagged, unstyled scalar implicitly resolves its type from its text (e.g. "true" -> bool). However, once any strategic merge patch was later applied to the same resource -- even to a completely unrelated part of the document -- `kyaml`'s `merge2` walk revisits every field via `FieldSetter.Filter`, which contains a YAML-1.1-compatibility safeguard that treats an untagged scalar as a plain string and force-quotes it if the value looks like a YAML 1.1 keyword. That safeguard can't distinguish "this is genuinely a string" from "this has no resolved type yet", so it silently turned e.g. a replacement-created `readOnly: true` into the string `readOnly: "true"`. Resolve and set the field's implicit YAML 1.2 tag right after creation, so its type is fixed before any later transformer can reinterpret it. Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
|
This PR has multiple commits, and the default merge method is: merge. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Welcome @onematchfox! |
kubernetes-prow
Bot
added
needs-ok-to-test
|
Hi @onematchfox. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
the
size/L
saitejabandaru-in
left a comment
saitejabandaru-in
left a comment
There was a problem hiding this comment.
LGTM! Using resolveImplicitTag to set the YAML 1.2 tag dynamically for newly created scalars is a clean way to ensure booleans and integers aren't silently coerced to strings later in the pipeline.
saitejabandaru-in
left a comment
saitejabandaru-in
left a comment
There was a problem hiding this comment.
LGTM! This is a great fix for type preservation in replacement scalars.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: onematchfox, saitejabandaru-in The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 participants
Every field created by a
replacementsentry (options.create: true) was left with an empty YAML tag on its scalar value, sincesetFieldValueonly ever copied.Valueonto the newly created node and never resolved a type for it. This was invisible in isolation, because encoding an untagged, unstyled scalar implicitly resolves its type from its text (e.g. "true" -> bool).However, once any strategic merge patch was later applied to the same resource -- even to a completely unrelated part of the document --
kyaml'smerge2walk revisits every field viaFieldSetter.Filter, which contains a YAML-1.1-compatibility safeguard that treats an untagged scalar as a plain string and force-quotes it if the value looks like a YAML 1.1 keyword. That safeguard can't distinguish "this is genuinely a string" from "this has no resolved type yet", so it silently turned e.g. a replacement-createdreadOnly: trueinto the stringreadOnly: "true".This PR was written in part with the assistance of generative AI.