Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape hatches seem to not be working as expected #2163

Open
srfrnk opened this issue Mar 29, 2024 · 10 comments
Open

Escape hatches seem to not be working as expected #2163

srfrnk opened this issue Mar 29, 2024 · 10 comments
Labels
bug Something isn't working @component/cdk8s-core Issue related to cdk8s-core duplicate This issue or pull request already exists

Comments

@srfrnk
Copy link

srfrnk commented Mar 29, 2024

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:

  • insert code section into a new Cdk8s TS project
  • run npm run synth

Environment:

  • Ubuntu 23
  • Node v18.20.0
  • Cdk8s 2.198.65

This is 🐛 Bug Report

@srfrnk srfrnk added bug Something isn't working needs-triage Priority and effort undetermined yet labels Mar 29, 2024
@cm3brian
Copy link

cm3brian commented Apr 3, 2024

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

For what it's worth...

@srfrnk
Copy link
Author

srfrnk commented Apr 3, 2024

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
Copy link

cm3brian commented Apr 3, 2024

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
Copy link
Author

srfrnk commented Apr 4, 2024

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
Copy link

cm3brian commented Apr 4, 2024

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
Copy link
Author

srfrnk commented Apr 5, 2024

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
Copy link

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), 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
Copy link
Member

iliapolo commented Jun 2, 2024

@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 iliapolo added priority/p1 Should be on near term plans duplicate This issue or pull request already exists and removed needs-triage Priority and effort undetermined yet priority/p1 Should be on near term plans labels Jun 2, 2024
@iliapolo
Copy link
Member

iliapolo commented Jun 2, 2024

Duplicate of #2172

@iliapolo iliapolo transferred this issue from cdk8s-team/cdk8s Jun 2, 2024
@a6patch
Copy link

a6patch commented Aug 26, 2024

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.

@iliapolo iliapolo added the @component/cdk8s-core Issue related to cdk8s-core label Sep 20, 2024
@iliapolo iliapolo transferred this issue from cdk8s-team/cdk8s-core Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working @component/cdk8s-core Issue related to cdk8s-core duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

5 participants