cdk8s-team / cdk8s-core

Define Kubernetes native apps and abstractions using object-oriented programming
Apache License 2.0
67 stars 26 forks source link

"can't render non-simple object" of implemented generated interfaces #1873

Closed Hazzard17h closed 2 days ago

Hazzard17h commented 10 months ago

Description of the bug:

Implementing a CLI generated interface (from CRD) with a Class, and using an object from that class to construct the ApiObject, will result in the error Error: can't render non-simple object of type 'Class'.

Reproduction Steps:

import { ApiObject, App, Chart, GroupVersionKind } from 'cdk8s';
import { Construct } from 'constructs';

interface IObj {
  readonly first: string;
  readonly second: number;
}

class Obj implements IObj {
  constructor(public readonly first: string, public readonly second: number) {}
}

interface L1Props {
  readonly obj: IObj;
}

// simulates an L1 as generated via the CLI
class L1 extends ApiObject {
  static readonly GVK: GroupVersionKind = {
    apiVersion: 'v1',
    kind: 'Kind'
  };

  constructor(scope: Construct, id: string, props: L1Props) {
    super(scope, id, {
      ...L1.GVK,
      spec: {
        obj: props.obj
      }
    });
  }

  public toJson(): any {
    const resolved = super.toJson();

    return {
      ...L1.GVK,
      metadata: this.metadata.toJson(),
      spec: {
        obj: {
          first: resolved.spec.obj.first,
          second: resolved.spec.obj.second
        }
      }
    };
  }
}

const app = new App();
const chart = new Chart(app, 'Chart');

new L1(chart, 'L1', { obj: { first: '1', second: 2 } }); // Works
new L1(chart, 'L1', { obj: new Obj('1', 2) }); // NOT works

Error Log:

Error: Failed serializing construct at path 'Chart/L1' with name 'chart-l1-c87f58d1': Error: can't render non-simple object of type 'Obj'

Environment:

Other:

I guess this has been happening since #1573 was implemented.


This is :bug: Bug Report

ProcopVladimirAlexandru commented 9 months ago

I am also experiencing this along with issue 1962 stemming from the same PR #1573 being merged. Here is a simpler code sample to reproduce:

import { ApiObject, Chart } from 'cdk8s';

class Quantity {
    public static fromString(value: string): Quantity {
        return new Quantity(value);
    }
    public static fromNumber(value: number): Quantity {
        return new Quantity(value);
    }
    private constructor(public readonly value: string | number) {}
}

class CustomChart1 extends Chart {
    public resolvers = [];
    public pod;
    public job;
    public constructor(
        public uid: string,
        public namespace: string
    ) {
        super(null, uid, { namespace: namespace, labels:  {}});
        this.pod = this.getPod();
        this.job = new ApiObject(this, this.uid, {
            apiVersion: 'batch/v1',
            kind: 'Job', 
            metadata: {
                labels: {},
                name: 'test-job-name-45631685',
                annotations: {},
            },
            spec: {
                template: {
                    metadata: {
                        labels: {},
                        annotations: {},
                    },
                    spec: {
                        ...this.pod,
                    },
                },
            },
        }).toJson();
    }

    protected getPod() {
        return {
            containers: [
            {
                    name: "testing-cdk8s-123456789",
                    image: "nginx",
                    resources: {
                        requests: {
                            cpu: Quantity.fromString("500m"),
                            memory: Quantity.fromString("64Mi")
                        }
                    }
                }
        ]
        };
    }
}

new CustomChart1("test-cdk8s-chart-123456789", "test-namespace");

The exception raised is:


/home/me/Documents/proj/api/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 'test-cdk8s-chart-123456789/test-cdk8s-chart-123456789' with name 'test-job-name-45631685': Error: can't render non-simple object of type 'Quantity'
    at ApiObject.toJson (/home/me/Documents/proj/api/node_modules/cdk8s/src/api-object.ts:224:13)
    at new CustomChart1 (/home/me/Documents/proj/api/cdk8s_bug_example.ts:42:12
iliapolo commented 3 days ago

@Hazzard17h The use case you lay out is not supported. I'll explain.

interface IObj {
  readonly first: string;
  readonly second: number;
}

class Obj implements IObj {
  constructor(public readonly first: string, public readonly second: number) {}
}

The CLI would never generate classes like Obj. It only generates union like classes that have a public property called value. This property is then explicitly extracted by the toJson_XXX functions generated by the CLI. For example:

export function toJson_ResourceRequirements(obj: ResourceRequirements | undefined): Record<string, any> | undefined {
  if (obj === undefined) { return undefined; }
  const result = {
    'claims': obj.claims?.map(y => toJson_ResourceClaim(y)),
    'limits': ((obj.limits) === undefined) ? undefined : (Object.entries(obj.limits).reduce((r, i) => (i[1] === undefined) ? r : ({ ...r, [i[0]]: i[1]?.value }), {})),
    'requests': ((obj.requests) === undefined) ? undefined : (Object.entries(obj.requests).reduce((r, i) => (i[1] === undefined) ? r : ({ ...r, [i[0]]: i[1]?.value }), {})),
  };
  // filter undefined values
  return Object.entries(result).reduce((r, i) => (i[1] === undefined) ? r : ({ ...r, [i[0]]: i[1] }), {});
}

It might be that prior to https://github.com/cdk8s-team/cdk8s-core/pull/1770, it inadvertently worked because we treated classes as dictionaries. But this was not the correct behavior.

In order to properly implement classes like Obj, you need to add a resolve function, which cdk8s will invoke during resolving of this class. Otherwise, cdk8s doesn't actually know which properties should it take.

class Obj implements IObj {
  constructor(public readonly first: string, public readonly second: number) {}

  public resolve() {
    return {
      first: this.first,
      second: this.second
    };
  }
}

Apologies for the inconvenience this may have caused. Please reach out if you need any assistance sorting this out.

Thanks

Hazzard17h commented 2 days ago

@iliapolo thanks for the clarification.

Previously I worked it around using:

class Obj implements IObj {
  constructor(public readonly first: string, public readonly second: number) {}

  toSimpleObject = () => ({
    first: this.first,
    second: this.second
  });
}

new L1(chart, 'L1', { obj: new Obj('1', 2).toSimpleObject() });

Now it's clear that can be resolved just renaming toSimpleObject to resolve, without the needing to call it when instantiating the object.

Maybe could be useful to add this somewhere in the import documentation.