Escape hatch output does not appear in generated YAML

[Chriscbr]

Currently, the JsonPatch class can be used to apply overrides / updates to resources that have already been generated. However, sometimes these modifications do not appear in the generated YAML because of type mismatches, especially when newer kubernetes versions expect IntOrString where they previously only required an int or string.

For example, applying this patch to a KubeDeployment will produce rollingUpdate: {}:

apiObject.addJsonPatch(
    JsonPatch.add("/spec/strategy", { rollingUpdate: { maxSurge: 1, maxUnavailable: 0 }})
);

...while applying this patch to a KubeDeployment will produce rollingUpdate: { maxSurge: 1, maxUnavailable: 0 } (in YAML):

apiObject.addJsonPatch(
    JsonPatch.add("/spec/strategy", {
		rollingUpdate: { maxSurge: IntOrString.fromNumber(1), maxUnavailable: IntOrString.fromNumber(0)}
	})
);

This type of no-op did not occur in older versions of cdk8s, but it does now since cdk8s-team/cdk8s-cli#55, as the toJson() method on the generated KubeDeployment class calls toJson_KubeDeployment(), which discards all values that were not expected (including in this case, values of the wrong type). (The patches get applied in the superclass, ApiObject, before toJson_KubeDeployment() is called).

I think this is problematic since JsonPatch should be able to add any extra content (including from newer / older kubernetes specs) to the ApiObject, not just those expected by the interface, so that users can patch their constructs for older or newer kubernetes versions. It also has a clunky experience (Why did the JSON Patch values not appear? There's no error message!)

---

Possible solutions:

• modify how json2jsii generates k8s.ts to try and "coerce" unexpected types somehow
• modify how json2jsii generates k8s.ts to do some kind of runtime checking to assert that values are the expected types, and print a warning when unexpected keys or values appear
• change when json patches get applied somehow, so the toJson() method in KubeDeployment instead calls toJsonUnpatched(), then toJson_KubeDeployment(), and then JsonPatch.apply(json, ...this.patches)

[github-actions]

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon.

[github-actions]

Closing this issue as it hasn't seen activity for a while. Please add a comment @mentioning a maintainer to reopen.

[Chriscbr]

I took some time to review this bug again. I've got one more solution idea which I think is promising - I've summarized the changes along with an example in this gist: https://gist.github.com/Chriscbr/8b8b15ffc22c30647cb861e0779e8b1d

The core of this fix is to remove the toJson method from our generated import files entirely. I think the way we currently override the toJson() method on ApiObject is somewhat problematic since it means patches can no longer get applied as the last step in ApiObject.synth. By shifting the logic to an intermediary hook which can be called by the superclass, it's now possible to apply transformations after any Lazy values have been evaluated, but before JSON patches have been applied, thus resolving the bug.

I don't think this is necessarily a breaking change, but it might require some care since the updated k8s imports should not be generated by cdk8s-cli unless a compatible version of cdk8s is being used. Nonetheless, it does further cement the idea that cdk8s does not do any validations on data that is escape-hatched "into" the template. I think is directionally correct for what we want the escape hatching to do - minimize restrictions for the user, even if it means they get less type safety.

@iliapolo curious to hear your thoughts.

[Chriscbr]

The one tricky part of this fix is whether it should be treated as a breaking change, and whether there's a way to make it backwards compatible.

The problem is that if someone uses the newly-generated k8s imports with an old version of cdk8s that doesn't have the updated ApiObject.synth method, then synthesizing these L1s will not work properly. The core library hasn't had any breaking changes, but the CLI definitely does.

One option I considered was making the cdk8s CLI "version aware" so that it error or warns you if you try running cdk8s import with an invalid version. But obtaining the necessary version information is error prone because it depends on which language you're writing cdk8s scripts in.

So I think it probably makes more sense to make this change alongside a major version bump of cdk8s-cli. I think there might be other breaking changes we want to add to the CLI (like updating the default templates to use v2 of the core library, updating the CLI to use ts-node by default, etc) so perhaps we could release it with a beta tag and stabilize it after some time.

But I'm also actively considering whether there's some way to make this backwards compatible 🤔

[jcunliffe1]

I agree, this problem makes escape hatches entirely unfriendly for modifications. Either fix it, or rename it to Escape mine field.

[jagu-sayan]

Any news ? I didn't find an easy solution. It's only possible to modify/remove existing key with JSON patch. I can't add or copy a key right now 🤔

It's blocking me, because I really need terminationGracePeriodSeconds for Deployment.

[iliapolo]

@jagu-sayan You should be able to patch terminationGracePeriodSeconds because its part of the spec. Can you share the code you are using so we can debug it? As well as which cdk8s-plus library are you using. (i.e which k8s version)

[iliapolo]

Just a note about this statement by @Chriscbr:

I think this is problematic since JsonPatch should be able to add any extra content (including from newer / older kubernetes specs) to the ApiObject, not just those expected by the interface

On the flip side - wouldn't it be nice if cdk8s could tell you when you escape hatch to a structure that doesn't fit the schema, and might therefore not be deployable? The current state of just ignoring this is definitely wrong, but I wonder if we should provide a choice between including those, or erroring out (maybe a config on the Chart level?)

[github-actions]

This issue has not received any attention in 1 year and will be closed soon. If you want to keep it open, please leave a comment below @mentioning a maintainer.

[iliapolo]

Reopening as this has surfaced again: #2163

[a6patch]

This escape hatch is a bit of a cruel joke. Like an emergency exit window that is just a painting of an emergency exit window~