cdk8s-team / cdk8s

Define Kubernetes native apps and abstractions using object-oriented programming
https://cdk8s.io
Apache License 2.0
4.39k stars 293 forks source link

Escape hatches seem to not be working as expected #2163

Open srfrnk opened 8 months ago

srfrnk commented 8 months ago

Description of the bug:

The following code:

    var test = new KubeNamespace(this, "test", {
      metadata: {
        name: "test"
      }
    })

    test.addJsonPatch(JsonPatch.add("/spec", { 'test': 1 }))

Generates this:

apiVersion: v1
kind: Namespace
metadata:
  name: test
spec: {}

While AFAIK it should generate:

apiVersion: v1
kind: Namespace
metadata:
  name: test
spec:
  test: 1

Reproduction Steps:

Environment:


This is :bug: Bug Report

cm3brian commented 8 months ago

When doing this kind of thing I found that test.addJsonPatch(JsonPatch.add("/spec/test", 1)) works

For what it's worth...

srfrnk commented 8 months ago

Thanks @cm3brian but when I try running this:


    var test = new KubeNamespace(this, "test", {
      metadata: {
        name: "test"
      }
    })

    test.addJsonPatch(JsonPatch.add("/spec/test", 1))

I get an exception:

Synthesizing application
.../llm-playground/k8s-setup-crd/node_modules/cdk8s/src/api-object.ts:224
      throw new Error(`Failed serializing construct at path '${this.node.path}' with name '${this.name}': ${e}`);
            ^
Error: Failed serializing construct at path 'setup-crd/test' with name 'test': TypeError: Cannot set properties of undefined (setting 'test')
    at KubeNamespace.toJson (.../llm-playground/k8s-setup-crd/node_modules/cdk8s/src/api-object.ts:224:13)
    at KubeNamespace.toJson (.../llm-playground/k8s-setup-crd/imports/k8s.ts:2664:28)
    at .../llm-playground/k8s-setup-crd/node_modules/cdk8s/src/app.ts:116:46
    at Array.map (<anonymous>)
    at Function._synthChart (.../llm-playground/k8s-setup-crd/node_modules/cdk8s/src/app.ts:116:31)
    at MyChart.toJson (.../llm-playground/k8s-setup-crd/node_modules/cdk8s/src/chart.ts:148:16)
    at App.synth (.../llm-playground/k8s-setup-crd/node_modules/cdk8s/src/app.ts:216:47)
    at Object.<anonymous> (.../llm-playground/k8s-setup-crd/main.ts:25:5)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at Module.m._compile (.../llm-playground/k8s-setup-crd/node_modules/ts-node/src/index.ts:1618:23)
Error: command "npx ts-node main.ts " at .../llm-playground/k8s-setup-crd returned a non-zero exit code 1
    at ChildProcess.<anonymous> (.../llm-playground/k8s-setup-crd/node_modules/cdk8s-cli/lib/util.js:54:27)
    at Object.onceWrapper (node:events:632:26)
    at ChildProcess.emit (node:events:517:28)
    at ChildProcess._handle.onexit (node:internal/child_process:292:12)
.../llm-playground

Is there any working example you can send?

EDIT: I tried to extend your example like so:

    var test = new KubeNamespace(this, "test", {
      metadata: {
        name: "test"
      }
    })

    test.addJsonPatch(JsonPatch.add("/spec", {}))
    test.addJsonPatch(JsonPatch.add("/spec/test", 1))

This time I got no exception but output is the same:

apiVersion: v1
kind: Namespace
metadata:
  name: test
spec: {}
cm3brian commented 8 months ago

Ah, I did not expect you to be directly running the contrived code I provided, it was an example only, pseudo-code, if you will. As your example was quite contrived I didn't expect it to be what you were actually using, but a public-friendly sharable snippet.

I question why you are trying to use patching at all where you are using KubeNamespace directly as opposed to something like cdk8s-plus Namespace, as you have access to practically everything in spec directly. Anywho...

First things first, in order to patch something; its root must exist, so you should define it first (see below, where I have defined spec).

There are also other considerations, like the fact you need to work within the spec of the resource you are trying to create, for Namespace, it's here: https://kubernetes.io/docs/reference/kubernetes-api/cluster-resources/namespace-v1/ (you could also interrogate the TypeScript definitions for these also)

const test = new KubeNamespace(this, 'test', {
  metadata: {
    name: 'test'
  },
  spec: {}
})

//won't work, this isn't in the resource spec
test.addJsonPatch(JsonPatch.add('/spec/test', 1))

// works as its in the spec
test.addJsonPatch(JsonPatch.add('/spec/finalizers', ['test']))

outputs:

apiVersion: v1
kind: Namespace
metadata:
  name: test
spec:
  finalizers:
    - test
srfrnk commented 8 months ago

Thanks @cm3brian that explains some stuff. You are correct in that this was a contrived example. I was under the impressions I could use a JSON patch to add anything but that's my bad.

So the real issue is that I'm failing to add a "status" definition which I can see in the link you sent. (https://kubernetes.io/docs/reference/kubernetes-api/cluster-resources/namespace-v1/#NamespaceStatus) So why should this not work then?

    var test = new KubeNamespace(this, "test", {
      metadata: {
        name: "test"
      }
    })

    test.addJsonPatch(JsonPatch.add("/status", {
      conditions: {
        "status": "ready"
      }
    }))
cm3brian commented 8 months ago

Trying to set the status field by user content is completely against the entire convention of Kubernetes, as a result, it's not even in the imported KubeNamespaceProps interface/type (which is why you cannot set the field in the main object in the first place.

I suggest brushing up on your Kubernetes conventions, the specific part relating to this is here: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status

This is a user error, not an issue with cdk8s.

srfrnk commented 8 months ago

To be honest I was not saying this is "an issue with cdk8s" - but rather that it seems to not be doing what the documentation is saying.

To begin with I was trying to wrap a valid manifest file for MetalLB (see here). Is it possible this is not a valid manifest to begin with? (kubectl apply did seem to accept it well when I tried...)

Also I'm seeing this pattern repeat itself many times (see here) so I'm not so convinced this really goes against the convention. Perhaps that is specific to CRDs and should be adopted into cdk8s?

Regardless of the specific issue - maybe the documentation would benefit from some of the information you've provided in this thread? It seems to suggest the opposite (IMHO):

Similarly, in CDKs, escape hatches are mechanisms that allow users to tweak the synthesized output when the abstraction they use does not “hold water”. You are using an imported API object (e.g. KubeDeployment) and there is an issue with the schema or a bug in “import” which results in an invalid manifest or missing fields (as an example see issue cdk8s-team/cdk8s#140).

It doesn't mention anything about the other considerations you have mentioned. I'm concerned many other user would not be able to deduce these from the existing documentation.

In short I thought it might be good to bring that to your attention, for my own use case I have already found a workaround that doesn't involve cdk8s.

If you think I'm wrong - happy to drop this thread.

johncuyle commented 6 months ago

I appear to be having a similar issue. I've imported a CRD (the AWS CNI Plugin CRD, here: https://[raw.githubusercontent.com/aws/amazon-vpc-cni-k8s/master/charts/aws-vpc-cni/crds/customresourcedefinition.yaml](https://raw.githubusercontent.com/aws/amazon-vpc-cni-k8s/master/charts/aws-vpc-cni/crds/customresourcedefinition.yaml)), but the CRD appears to be defective (it doesn't specify a spec for a resource). The yaml definition looks like this:

apiVersion: crd.k8s.amazonaws.com/v1alpha1
kind: ENIConfig
metadata: 
  name: "{{availabilityZone}}"
spec: 
  security_groups: 
    - "{{security_group_id}}"
  subnet: "{{subnetId}}"

So I'm attempting to create the resource using ApiObject, but ApiObject doesn't allow passing a spec:

class ApiObject(
    _constructs_77d1e7e8.Construct,
    metaclass=jsii.JSIIMeta,
    jsii_type="cdk8s.ApiObject",
):
    def __init__(
        self,
        scope: _constructs_77d1e7e8.Construct,
        id: builtins.str,
        *,
        api_version: builtins.str,
        kind: builtins.str,
        metadata: typing.Optional[typing.Union["ApiObjectMetadata", typing.Dict[builtins.str, typing.Any]]] = None,
    ) -> None:
        '''Defines an API object.

        :param scope: the construct scope.
        :param id: namespace.
        :param api_version: API version.
        :param kind: Resource kind.
        :param metadata: Object metadata. If ``name`` is not specified, an app-unique name will be allocated by the framework based on the path of the construct within thes construct tree.
        '''
        if __debug__:
            type_hints = typing.get_type_hints(_typecheckingstub__7c3471c86f94453ef4d9efec6bd8f9cf9dbac2f11db69184e091d0a4f6d502be)
            check_type(argname="argument scope", value=scope, expected_type=type_hints["scope"])
            check_type(argname="argument id", value=id, expected_type=type_hints["id"])
        props = ApiObjectProps(api_version=api_version, kind=kind, metadata=metadata)

        jsii.create(self.__class__, self, [scope, id, props])
    ...
iliapolo commented 6 months ago

@srfrnk Thanks for brining this to our attention. Your expectation of how it should work makes sense. @cm3brian the considerations you laid out here for escape hatches also make sense, but they are actually inconsistent with our documentation, and with how we envisioned escape patching should be used. The fact escape hatches cannot be used on properties that don't appear in the schema is a bug.

@johncuyle the problem (and a workaround for it) you are describing is explained here.

iliapolo commented 6 months ago

Duplicate of https://github.com/cdk8s-team/cdk8s/issues/2172

a6patch commented 3 months ago

Wow this is rather frustrating. The purpose of this escape hatch as stated in the docs is to break through the abstraction and override the output, but its actually validated by the abstraction and allows no actual override of the output. I just spent a ton of time trying to use this with absolutely zero errors or hint that it is just going to eat the calls. Maybe the docs should be changed to reflect the lack of this functionality.