RFC 814: CDK Mixins

[mrgrain]

This is a request for comments about CDK Mixins: Composable Abstractions for AWS Resources. See #814 for additional details.

APIs are signed off by @rix0r.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[commiterate]

Not too sure if there was something specific in mind for this but I'll just interpret this as any breaking L2 changes.

#657 (comment) proposes such a change so I'll just use this thread (or the closest substitute we can get with GitHub PR comments) to discuss that proposal and how this RFC addresses it.

---

The linked proposal essentially wants L2 constructs to be split into 2 parts:

1. A "dumb/plain" dictionary/map of L1 constructs.
2. Wiring/currying/overlay functions which operate on L1 constructs.

L2 constructs today are fairly opaque since they complect both of these concerns without exposing much of the underlying details. The child L1 constructs aren't exactly that convenient to access (need to use IConstruct.node.children and do a bunch of if...else with instanceof to restore type information).

For 1, the general idea is possible but the implementation detail of using a TS interface (i.e. just a plain dictionary/map) isn't really possible since the CDK is heavily class based which is inherited from constructs. Passing immutable maps like in Clojure is out of the question.

For 2, it's essentially saying to extract all of the L2 wiring logic outside of the L2 construct (e.g. into functions, mixins, or whatever is decided upon).

---

If we assume something like the current class-based mixin design is accepted, that means we'd have something like this for AWS::Lambda::Function:

import { Construct, IConstruct } from "constructs";
import { IMixin, Resource } from "@aws-cdk/core";
import { aws_iam } from "@aws-cdk";

// L1. Auto-generated from Cfn resource definitions.

export class CfnFunctionProps {
  // ...
}

export class CfnFunction extends Construct {
  // Resource attributes. Same as today.
  public attr*: type;

  // Constructor. Same as today.
  constructor(scope: Construct, id: string, props: CfnFunctionProps) {
    // ...
  }
}

// L2. Manually written.

export interface FunctionExecutionRoleMixinProps {
  function: CfnFunction;
  role: aws_iam.CfnRole;
}

// I don't really know why this is a class. This can just be a function.
export class FunctionExecutionRoleMixin implements IMixin<FunctionExecutionRoleMixinProps> {
  static apply(props: FunctionExecutionRoleMixinProps): FunctionExecutionRoleMixinProps {
    // Add the Lambda-managed basic execution role policy.
    props.function.managedPolicyArns = (props.function.managedPolicyArns ?? []) + [aws_iam.ManagedPolicy.fromAwsManagedPolicyName("AWSLambdaBasicExecutionRole").managedPolicyArn];
    return props;
  }
}

export interface FunctionComponents {
  function: CfnFunction;
  role: aws_iam.CfnRole;
}

// Same as today.
export interface FunctionProps {
  // ...
}

export class Function extends Resource {
  // L1 components.
  public readonly components: FunctionComponents;

  // Existing properties for backwards compatibility.

  // Constructor.
  constructor(scope: Construct, id: String, props: FunctionProps) {
    this.components = {
      function: new CfnFunction(scope, "function", {
        // Wire in values from `props: FunctionProps`.
      }),
      role: props.role ?? new aws_iam.CfnRole(scope, "role", {
        // Default function execution role if absent.
      })
    };

    // Apply the function mixin. This is doing mutation in place, may not want this for the final design.
    FunctionExecutionRoleMixin.apply({
      function: this.components.function;
      role: this.components.role;
    });
  }

  // Existing methods for backwards compatibility.
  //
  // All of their logic (e.g. adding IAM policies for event sources) are
  // extracted into mixins. The methods just apply the mixin for users.
}

The L2 construct basically becomes a composition of L1s and default mixin applications. If users don't like the L2 construct, they're free to apply mixins directly to L1s they created or extract the L1s from the L2 and apply them.

[mrgrain]

The " Modular L2 Redesign" alternative is roughly what you have been proposing before. Not necessarily exactly that, but stands for any approach that changes the shape of L2s, how they are created and used. There's no detail to it, so it's hard to argue about it more.

The L2 construct basically becomes a composition of L1s and default mixin applications. If users don't like the L2 construct, they're free to apply mixins directly to L1s they created or extract the L1s from the L2 and apply them.

Agreed. That nicely captures the spirit and eventual goal of the Mixins proposal.

[commiterate]

In that case, would the above snippet that makes L2 constructs (e.g. Function) expose a property with a dictionary of L1 constructs (e.g. Function.components) nicely expose the L1 constructs with type information (no IConstruct.nodes.children with verbose instanceof matching to restore type information) without breaking backwards compatibility?

This could be enforced through an interface all higher level constructs (L2, L3) must implement (e.g. IResource). Said interface would require a components: Record<string, IInspectable> property (where L1 constructs must implement IInspectable).

[mrgrain]

I think that's an interesting idea that is worth thinking about. It fits nicely into other improvements we made recently like I_Ref interfaces. That makes it independent of this RFC though. Do you agree?

Like you said IConstruct.nodes.children already exists, it's just not strongly typed. An interface like FunctionComponents would help. The downside is that it limits refactoring to some extent (cannot remove implementation details anymore), that might be okay though.

I like the idea of IInspectable - what would it look like?

[commiterate]

IInspectable already exists. I just assumed that was some interface only implemented by L1's since those seem to be the only constructs that implement it.

The purpose of proposing that in the mixins RFC is because mixins are supposed to target L1 constructs and this lets users fish out the L1's to pass to a mixin function (e.g. functionIamExecutionRoleMixin(props: { function: aws_lambda.CfnFunction, role: aws_iam.CfnRole })).

That's assuming mixins are intended to be manually invoked at a per-construct level (e.g. functionIamExecutionRoleMixin({ function: myFunction.components.function, role: myFunction.components.role })). I haven't thought too hard about how such an interface would work if we applied a mixin to an entire construct tree like what's being proposed right now (closer to CSS selectors).

[mrgrain]

IInspectable already exists. I just assumed that was some interface only implemented by L1's since those seem to be the only constructs that implement it.

Oh haha, how embarrassing of me. I am aware of this interface, though clearly I didn't remember the name. I'm not sure how this interface helps with the problem though. Was that just to pick a type that represents all L1s?

The purpose of proposing that in the mixins RFC is because mixins are supposed to target L1 constructs and this lets users fish out the L1's to pass to a mixin function (e.g. functionIamExecutionRoleMixin(props: { function: aws_lambda.CfnFunction, role: aws_iam.CfnRole })).

Okay gotcha, thanks for the clarification. The intention of this proposal is that Mixins can be applied to either the L2 or the L1 or even a custom version of an L2. Implementation-wise Mixins should be at L1 level, but are applied to the tree and will determine if they are applicable ("support") a construct.

I haven't thought too hard about how such an interface would work if we applied a mixin to an entire construct tree like what's being proposed right now (closer to CSS selectors).

We have a provision for this in the advances Mixins.of(...) mode. Currently not implementing a CSS Selector like syntax, but with this proposal it could be easily done in userland.

[commiterate]

Yup I was just using IInspectable to represent L1s, apologies for the confusion.