Latest version of `k8s.ts` produced by the import command breaks usage of Lazy in resources about cdk8s-plus-17 HOT 8 CLOSED

@Chriscbr What if we just get rid of the resolve method on union types in json2jsii?

i.e get rid of https://github.com/cdklabs/json2jsii/blob/f891f67ee833bbde81d8f8e29bb43079c69b6773/src/type-generator.ts#L373

It seems like the toJson_xxx methods are effectively serving the same purpose since they return .value. If the resolve method won't exist, then super.toJson() will leave union types as is, allowing for toJson_xxx to resolve them.

from cdk8s-plus-17.

I spent a couple hours looking into potential fixes. I don't think we can just simply call resolve on properties before invoking toJson_xxx() methods because this would resolve the dynamic references immediately, instead of at synthesis time, defeating the point of tokens.

I also couldn't think of any ways to change how objects are resolved and synthesized in cdk8s-core to fix this.

The only fixes I could think of center around supporting Lazy values in the toJson_xxx() or manifest() methods. For example, if we could change how manifest() is generated to support Lazy's, that would be great since it means we don't have to make changes to json2jsii. Here is an example of something I came up with, though unfortunately it does not work:

// old
  public static manifest(props: KubeServiceAccountProps = {}): any {
    return {
      ...KubeServiceAccount.GVK,
      ...toJson_KubeServiceAccountProps(props),
    };
}

// new
  public static manifest(props: KubeServiceAccountProps = {}): any {
    return Lazy.any({
      produce: () => {
        props = resolve(props) as KubeServiceAccountProps;
        const result = {
          ...KubeServiceAccount.GVK,
          ...toJson_KubeServiceAccountProps(props),
        };
        return Object.entries(result).reduce((r, i) => (i[1] === undefined) ? r : ({ ...r, [i[0]]: i[1] }), {});
      },
    });
  }

In principle it might do what we want: it returns an object that, when resolved, will have the correct property names and values. resolve has to be called within the Lazy so that toJson_xxx() can call methods like .map and so on. The problem is this implementation causes us to pass an entire Lazy value to ApiObject's props argument, which isn't valid (and I don't see a way to support this).

We could try the a similar approach with toJson_xxx where we wrap its entire method body in a Lazy, but then the ...toJson_xxx() spread operation in manifest() will not be valid (see the code above). (What does it mean to spread a Lazy value?)

An alternative that I think will work is to process the fields individually like this:

// old
export function toJson_KubeServiceAccountProps(obj: KubeServiceAccountProps | undefined): Record<string, any> | undefined {
  if (obj === undefined) { return undefined; }
  const result = {
    'automountServiceAccountToken': obj.automountServiceAccountToken,
    'imagePullSecrets': obj.imagePullSecrets?.map(y => toJson_LocalObjectReference(y)),
    'metadata': toJson_ObjectMeta(obj.metadata),
    'secrets': obj.secrets?.map(y => toJson_ObjectReference(y)),
  };
  // filter undefined values
  return Object.entries(result).reduce((r, i) => (i[1] === undefined) ? r : ({ ...r, [i[0]]: i[1] }), {});
}

// new
export function toJson_KubeServiceAccountProps(obj: KubeServiceAccountProps | undefined): Record<string, any> | undefined {
  if (obj === undefined) { return undefined; }
  const result = {
    'automountServiceAccountToken': Lazy.any({ produce: () => resolve(obj.automountServiceAccountToken) }),
    'imagePullSecrets': Lazy.any({ produce: () => resolve(obj.imagePullSecrets)?.map((y: any) => toJson_LocalObjectReference(y)) }),
    'metadata': Lazy.any({ produce: () => toJson_ObjectMeta(resolve(obj.metadata)) }),
    'secrets': Lazy.any({ produce: () => resolve(obj.secrets)?.map((y: any) => toJson_ObjectReference(y)) }),
  };
  // filter undefined values
  return Object.entries(result).reduce((r, i) => (i[1] === undefined) ? r : ({ ...r, [i[0]]: i[1] }), {});
}

If we wanted, we could even write the method in a more verbose way where the each values is only wrapped in an additional Lazy if the passed value was an instance of Lazy or not. (I think this should even work for edge cases where only nested values are Lazy, but I have to double check this).

So I believe this approach will fix the bug, but it still requires a full POC (I've only tried it on a few examples by hand). There are also some edge cases I noticed, like right now metadata does not support Lazy values (this unit test fails):

new ApiObject(chart, 'Pod', {
  apiVersion: 'v1',
  kind: 'Pod',
  metadata: Lazy.any({ produce: () => ({ name: 'my-pod' }) }),
});

If we went with this approach, I think we would want this behavior to be behind a flag in json2jsii so that others can disable the Lazy-injections if they want to.

@iliapolo WDYT? Open to ideas and suggestions.

Edit: One more potential solution. We could delay calling toJson_xxx until synthesis time by defining a standard method name like toJson() on all imported objects, and handling the property name translation in there. One possible implementation I've tried looks like this:

  public constructor(scope: Construct, id: string, props: KubeServiceProps = {}) {
    super(scope, id, {
      ...KubeService.GVK,
      ...props, // previously: ...toJson_KubeServiceProps(props)
    });
  }

  public toJson() {
    // note: this.props is currently private, needs to be changed to protected for this to work
    this.props = toJson_KubeServiceProps(resolve(this.props));
    return super.toJson();
  }

Including stateful effects in toJson() makes me less inclined towards this solution, but perhaps there a way around this - not sure.

from cdk8s-plus-17.

Update:

Eli and I brainstormed one more idea together offline. We liked the first solution, but the issue was there's no easy way to pass an entire lazy object into the ApiObject's props. So what if we just "made it so", by adding a field to ApiObject as an escape hatch to provide lazy values, and using it like this:

public static manifest(props: KubeServiceProps): any {
  return {
    ...KubeService.GVK,
    lazySpec: Lazy.any({ produce: () => toJson_KubeServiceProps(resolve(props)) }),
  };
}

This would avoid the need to modify json2jsii with all of the extra options needed to support Lazy fields.

I got this working and drafted a couple PRs with my implementation. (#59, cdk8s-team/cdk8s-core#44, cdk8s-team/cdk8s-cli#55).

@eladb Any thoughts on the issue or solutions in general?

from cdk8s-plus-17.

Downgraded priority to p1 since this isn't technically blocking us. We can currently choose to stay with the older version json2jsii. One thing to note is that does have the potential to break customers using Lazy in the resources regardless of cdk8s-plus.

from cdk8s-plus-17.

Discussed with @iliapolo and here's a proposal.

The generated code should look like this:

export class FooCronTab extends ApiObject {
  public static readonly GVK: GroupVersionKind = {
    apiVersion: 'stable.example.com/v1',
    kind: 'CronTab',
  }

  public static manifest(props: FooCronTabProps = {}): any {
    return {
      ...FooCronTab.GVK,
      ...toJson_FooCronTabProps(props),
    };
  }

  public constructor(scope: Construct, id: string, props: FooCronTabProps = {}) {

    // do not call `manifest()` here
    super(scope, id, {
      ...FooCronTab.GVK,
      ...props,
    });
  }

  // add this
  public toJson() {
    const resolved = super.toJson();

    return {
      ...FooCronTab.GVK,
      ...toJson_FooCronTabProps(resolved);
    };
  }
}

from cdk8s-plus-17.

Thanks for the proposal. This is the best solution so far, and definitely requires the fewest changes to existing APIs.

I've noticed just one issue preventing this from working out-of-the-box, which is that toJson() on any union-like fields (namely IntOrString) fails. To see why, consider when super.toJson() is called, any IntOrString value will be resolved to a string or number. Meanwhile the toJson_xxx code generated by json2jsii looks like this:

const result = {
  'name': obj.name,
  'nodePort': obj.nodePort,
  'port': obj.port,
  'protocol': obj.protocol,
  'targetPort': obj.targetPort?.value, // !!!
};

Strings and numbers don't have ".value" fields.

(I think this issue also comes up with some of the solutions I proposed.)

My planned workaround is to modify json2jsii and add toJson() fields to the union-like structs as well. This way the toJson_xxx method works both when the raw primitive is passed in and when the union struct are passed in.

Edit: wait, that's not how JS works... the only other workarounds I can think of is to define a custom valueOf method and specify 'targetPort': obj.targetPort.valueOf() (since this method is defined on all primitives), or to specify 'targetPort': obj.targetPort?.value ?? obj.targetPort.

from cdk8s-plus-17.

I think that's the right direction. The resolve method is a leak from another dimension.

from cdk8s-plus-17.

This has been resolved by cdk8s-team/cdk8s-cli#55.

The updated imports will be added in #59.

from cdk8s-plus-17.