[RFC] Default schema elements for CRDs

[gabe-l-hart]

Instructions:

Feature name	crd-default-schema-elements
Issue #	#285
Start Date	2020-08-04
RFC PR	(leave this empty)

Summary

This RFC proposes adding more sophisticated default behavior to import when determining the schema for a CRD (here). The proposed changes are:

1. Create a default schema similar to the following:

   export interface DefaultCrdSchema {
     metadata: { [k: string]: any };
     spec: { [k: string]: any };
   }

   NOTE: Not sure the specifics of the type specification (my typescript is a bit rusty), but this would be the equivalent of specifying additionalProperties: true in the swagger spec.

2. Override the default schema based on the content of the CRD's validation schema

Motivation

When using cdk8s with CRDs that you do not have direct control over, the current implementation for determining the schema has two major holes:

1. Some CRDs simply don't have a validation section and rely on convention in CRs to infer a schema
   • This is BAD but happens pretty frequently for operators which are not using the CRDs for codegen, for example etcd
2. Many CRDs which do contain a validation section only specify a schema for the spec portion of the CR and omit the standard kubernetes boilerplate (specifically metadata).
   • As a CRD developer, I would not expect to need to specify a schema for metadata since that is "owned" by kubernetes and not me, the developer of the resource type.

Basic Example

CRD without validation

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: widgets.com.foo.bar
spec:
  group: com.foo.bar
  names:
    kind: Widget
    listKind: Widgets
    plural: widgets
    singular: widget
  scope: Namespaced
  subresources:
    status: {}
  version: v1
  versions:
  - name: v1
    served: true
    storage: true

import cdk8s
from imports import k8s
from imports.com.foo.bar import widget

app = cdk8s.App()
chart = cdk8s.Chart(app, 'test')

widget.Widget(chart, 'widget-one',
    metadata=k8s.ObjectMetadata(name='some-override-name-widget-one'),
    spec={'foo': 'bar', 'baz': 1})

CRD with validation but no metadata schema

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: widgets.com.foo.bar
spec:
  group: com.foo.bar
  names:
    kind: Widget
    listKind: Widgets
    plural: widgets
    singular: widget
  scope: Namespaced
  subresources:
    status: {}
  version: v1
  versions:
  - name: v1
    served: true
    storage: true

  validation:
    openAPIV3Schema:
      properties:
        foo:
          type: string
        baz:
          type: integer

import cdk8s
from imports import k8s
from imports.com.foo.bar import widget

app = cdk8s.App()
chart = cdk8s.Chart(app, 'test')

widget.Widget(chart, 'widget-one',
    metadata=k8s.ObjectMetadata(name='some-override-name-widget-one'),
    spec=widget.WidgetSpec(foo='bar', 'baz'=1))

Design Summary

The design here is attempting to follow the "what would it do if I didn't read the documentation" principle. Without reading carefully, I would expect an imported CRD's construct to be flexible to all of the various forms that a CRD can take in kubernetes. I would also expect cdk8s to explicitly plan for the circumstance where a user is building an App that uses CRDs which that user does not have write-access to (i.e. third party CRDs).

Detailed Design

The details are pretty simple actually:

1. Create the appropriate default schema object in crd.ts
2. Near line 63, do a deep override merge with the schema pulled from spec.validation.openAPIV3Schema where schema elements in the validation section take precedence.
3. (maybe) create a generic construct for spec when not specified such that at runtime, the spec argument could be an arbitrary key/value store of plain-old-data.

Drawbacks

The major drawback to this proposal is that it weakens the cdk8s stance on well-formed data structures (which is generally a great stance to take!). With this in place, it allows users to write sloppy CRDs and rely on the default behavior to just put arbitrary keys into the constructs which could lead to a lack of maintainability in the user's code.

Rationale and Alternatives

The rationale for this proposal is to allow well-intentioned users to take advantage of good, but sloppy third-party work (i.e. CRDs without validation that do the job well but have no strict schema). There are several alternatives to this that could be considered:

• Solutions in cdk8s:

  • Allow a project to specify its own default schema that will be applied to all import constructs
    • This would be nicely flexible, but would result in almost every user that needs metadata repeating the same default template, and it would not allow a user to use k8s.ObjectMetadata there.
  • Allow a construct (including its python bindings) to be modified after construction with arbitrary properties that would propagate through to the final rendered yaml file
    • Something similar to this issue

• Solutions in user project:

  • Users can vendor a copy of troublesome CRDs into their own project (or repo fork) and add validation manually
    • This becomes brittle unless the user is vendoring the entire operator because the vendored CRD and the deployed operator could get out of sync
  • Users can script some middleware that injects into the cdk8s import process to adjust CRDs on-the-fly before the codegen looks at them
    • This is likely what we'll do for our project unless/until a more formal solution is available
    • This is gross for obvious reasons
  • Users can read and modify the output yaml files after rendering, but before applying them to the cluster
    • Again, gross for obvious reasons

Adoption Strategy

This would not be a breaking change, so no adoption strategy would be needed.

Unresolved questions

• Would it be at all possible to use the ObjectMetadata construct from the k8s package in conjunction with a construct generated from a CRD in a different package?

Future Possibilities

• Allowing projects to specify custom default schemas would be a nice future extension to this idea

[iliapolo]

Hi @gabe-l-hart - Thanks for kicking off this discussion!

Just to make sure I understand correctly, i'll try and recap what you've presented here.

As things stand now, when a CRD does not have the validations property, i.e it doesn't have a well defined schema, the result of importing such a CRD with cdk8s import would be this:

/**
 * 
 *
 * @schema Widget
 */
export class Widget extends ApiObject {
  /**
   * Defines a "Widget" API object
   * @param scope the scope in which to define this object
   * @param name a scope-local name for the object
   * @param options configuration options
   */
  public constructor(scope: Construct, name: string, options: WidgetOptions = {}) {
    super(scope, name, {
      ...options,
      kind: 'Widget',
      apiVersion: 'com.foo.bar/v1',
    });
  }
}

/**
 * @schema Widget
 */
export interface WidgetOptions {
}

Using this Widget resource would basically allow passing any arbitrary keys, which will fail at apply time. For example:

new Widget(this, 'Widget, {
  not_a_spec: 'foo',
  not_a_metadata: 'bar'
})

What you are suggesting, is that for such CRD's, we will generate a default schema, like so:

/**
 * @schema Widget
 */
export interface WidgetOptions {
  readonly metadata: k8s.ObjectMeta;
  readonly spec: { [k: string]: any };
}

What about resources/crds that don't accept a spec? For example, ConfigMap:

export interface ConfigMapOptions {
  readonly binaryData?: { [key: string]: string };
  readonly data?: { [key: string]: string };
  readonly metadata?: ObjectMeta;
}

In addition, it looks like you are also suggesting to apply this default schema for CRD's with validation, but without a spec and metadata key.

So for example, a validation of this sort:

  validation:
    openAPIV3Schema:
      properties:
        foo:
          type: string
        baz:
          type: integer

Would result in the following schema:

/**
 * @schema Widget
 */
export interface WidgetSpec {
  readonly foo: string;
  readonly baz: int;
}

/**
 * @schema Widget
 */
export interface WidgetOptions {
  readonly metadata: k8s.ObjectMeta;
  readonly spec: WidgetSpec;
}

However, the validation doesn't mention spec anywhere, and it might be that the resource doesn't accept a spec, and the intent was to validate the top level foo and baz properties. Does it make sense to force a spec?

I hope I understood you correctly.

Waiting for more discussion :)

Thanks!

[gabe-l-hart]

Hi @iliapolo, thanks for digging in on this! I think your understanding of my initial proposal is accurate, but I definitely see the holes that you're pointing out, so I'm not sure my proposed solution is the right one anymore.

I think the one thing that is missing is how the options argument to the constructor of the current generated class interacts with the python wrapper. Currently (0.27.0) when a python wrapper class is generated for a CRD without validation, there is no way (that I can find) to pass the equivalent of the options key/value mapping through to the underlying node code. For example, this etcd CRD has no validation and the resulting generated python wrapper is:

import abc
import builtins
import datetime
import enum
import typing

import jsii
import publication
import typing_extensions

from ._jsii import *

import cdk8s
import constructs

class EtcdCluster(
    cdk8s.ApiObject,
    metaclass=jsii.JSIIMeta,
    jsii_type="etcddatabasecoreoscometcdcluster.EtcdCluster",
):
    """
    schema:
    :schema:: EtcdCluster
    """

    def __init__(self, scope: constructs.Construct, name: builtins.str) -> None:
        """Defines a "EtcdCluster" API object.

        :param scope: the scope in which to define this object.
        :param name: a scope-local name for the object.
        """
        options = EtcdClusterOptions()

        jsii.create(EtcdCluster, self, [scope, name, options])

@jsii.data_type(
    jsii_type="etcddatabasecoreoscometcdcluster.EtcdClusterOptions",
    jsii_struct_bases=[],
    name_mapping={},
)
class EtcdClusterOptions:
    def __init__(self) -> None:
        """
        schema:
        :schema:: EtcdCluster
        """
        self._values: typing.Dict[str, typing.Any] = {}

    def __eq__(self, rhs: typing.Any) -> builtins.bool:
        return isinstance(rhs, self.__class__) and rhs._values == self._values

    def __ne__(self, rhs: typing.Any) -> builtins.bool:
        return not (rhs == self)

    def __repr__(self) -> str:
        return "EtcdClusterOptions(%s)" % ", ".join(
            k + "=" + repr(v) for k, v in self._values.items()
        )

__all__ = [
    "EtcdCluster",
    "EtcdClusterOptions",
]

publication.publish()

The corresponding generated node code is:

class EtcdCluster extends cdk8s_1.ApiObject {
    /**
     * Defines a "EtcdCluster" API object
     * @param scope the scope in which to define this object
     * @param name a scope-local name for the object
     * @param options configuration options
     */
    constructor(scope, name, options = {}) {
        super(scope, name, {
            ...options,
            kind: 'EtcdCluster',
            apiVersion: 'etcd.database.coreos.com/v1beta3',
        });
    }
}
exports.EtcdCluster = EtcdCluster;

From what I can tell, there's no way to to populate the options in python and have them pass through jsii the node class, leaving the python version of the CRD pretty much useless. I think that fixing this ability to do arbitrary key/value passthroughts would likely solve the working problem.

It's still a bit awkward to not be able to use k8s.ObjectMeta, but that would be fairly awkward to solve since that comes from the generated k8s package and is not defined in the core of cdk8s anywhere. A nicer solution might be to allow ApiObjectMetadata through for the metadata argument of every CRD.

[iliapolo]

@gabe-l-hart I see what you mean about the python output.

So actually this just sounds like a bug.

I'm going to use your example here and file a bug in favor of this RFC. Sound good?

[gabe-l-hart]

@iliapolo Yes, that sounds good. Thanks for the discussion!

[iliapolo]

Closing in favor of https://github.com/awslabs/cdk8s/issues/316