fix(cdk8s): autogenerated names fail validation for some resource typ…

…es (#18)

Different resource types may have different constraints on names (`metadata.name`). The previous version of the name generator was compatible with DNS_SUBDOMAIN but not with DNS_LABEL.

For example, `Deployment` names must comply with DNS_SUBDOMAIN while `Service` names must comply with DNS_LABEL.

Since there is no formal specification for this, the default name generation scheme for kubernetes objects in cdk8s was changed to DNS_LABEL, since it’s the common denominator for all kubernetes resources (supposedly).

Fixes #16

README.md

@@ -366,7 +366,7 @@ export class WebService extends Construct {
           spec: {
             containers: [
               {
-                name: this.node.uniqueId,
+                name: 'web',
                 image: options.image,
                 ports: [ { containerPort } ]
               }
@@ -398,9 +398,7 @@ export class MyChart extends Chart {
 
 As you can see, we now add define `WebService` constructs inside our chart: one
 that runs the `paulbouwer/hello-kubernetes` image and one with an installation
-of [ghost](https://hub.docker.com/_/ghost/)
-
-
+of [ghost](https://hub.docker.com/_/ghost/).
 
 ## API Reference
 

examples/hello/__snapshots__/hello.test.js.snap

@@ -6,7 +6,7 @@ Array [
     "apiVersion": "v1",
     "kind": "Service",
     "metadata": Object {
-      "name": "hello.service.9878228b",
+      "name": "hello-service-9878228b",
     },
     "spec": Object {
       "ports": Array [
@@ -25,7 +25,7 @@ Array [
     "apiVersion": "apps/v1",
     "kind": "Deployment",
     "metadata": Object {
-      "name": "hello.deployment.c51e9e6b",
+      "name": "hello-deployment-c51e9e6b",
     },
     "spec": Object {
       "replicas": 1,

packages/cdk8s/lib/chart.ts

@@ -5,7 +5,7 @@ import { ApiObject } from './api-object';
 import * as YAML from 'yaml';
 import { resolve } from './_tokens';
 import { removeEmpty } from './_util';
-import { renderObjectName } from './names';
+import { Names } from './names';
 
 export class Chart extends Construct {
 
@@ -38,15 +38,27 @@ export class Chart extends Construct {
   }
 
   /**
-   * Generates a name for an object given it's construct node path.
+   * Generates a app-unique name for an object given it's construct node path.
+   *
+   * Different resource types may have different constraints on names
+   * (`metadata.name`). The previous version of the name generator was
+   * compatible with DNS_SUBDOMAIN but not with DNS_LABEL.
+   *
+   * For example, `Deployment` names must comply with DNS_SUBDOMAIN while
+   * `Service` names must comply with DNS_LABEL.
+   *
+   * Since there is no formal specification for this, the default name
+   * generation scheme for kubernetes objects in cdk8s was changed to DNS_LABEL,
+   * since it’s the common denominator for all kubernetes resources
+   * (supposedly).
    *
    * You can override this method if you wish to customize object names at the
    * chart level.
    *
    * @param apiObject The API object to generate a name for.
    */
   public generateObjectName(apiObject: ApiObject) {
-    return renderObjectName(apiObject.node.path);
+    return Names.toDnsLabel(apiObject.node.path);
   }
 
   protected synthesize(session: ISynthesisSession) {

packages/cdk8s/lib/names.ts

@@ -1,61 +1,78 @@
 import * as crypto from 'crypto';
 
-const MAX_LEN = 253;
-const VALIDATE = /^[0-9a-z\-.]+$/;
+const MAX_DNS_NAME_LEN = 63;
+const VALIDATE = /^[0-9a-z-]+$/;
 const HASH_LEN = 8;
 
 /**
- * Generates a name from a construct path.
- *
- * Kubernetes resources can have names up to 253 characters long. The characters
- * allowed in names are: digits (0-9), lower case letters (a-z), -, and .
- *
- * Names in cdk8s are always in the follow form:
- *
- *  <comp0>.<comp1>....<compN>.<short-hash>
- *
- * Where <comp> are construct node path components.
- *
- * Note that if the total length is longer than 253 characters, we will trim the
- * first components first since the last components usually are more inline with
- * the essence of the resource.
- *
- * @param path the construct path (components separated by "/")
- * @param maxLen maximum allowed length for name
- * @throws if any of the components do not adhere to naming constraints or length.
+ * Utilities for generating unique and stable names.
  */
-export function renderObjectName(path: string, maxLen = MAX_LEN) {
-  if (maxLen < HASH_LEN) {
-    throw new Error(`minimum max length for object names is ${HASH_LEN} (required for hash)`);
-  }
+export class Names {
+  /**
+   * Generates a unique and stable name compatible DNS_LABEL from RFC-1123 from
+   * a path.
+   *
+   * The generated name will:
+   *  - contain at most 63 characters
+   *  - contain only lowercase alphanumeric characters or ‘-’
+   *  - start with an alphanumeric character
+   *  - end with an alphanumeric character
+   *
+   * The generated name will have the form:
+   *  <comp0>-<comp1>-..-<compN>-<short-hash>
+   *
+   * Where <comp> are the path components (assuming they are is separated by
+   * "/").
+   *
+   * Note that if the total length is longer than 63 characters, we will trim
+   * the first components since the last components usually encode more meaning.
+   *
+   * @link https://tools.ietf.org/html/rfc1123
+   *
+   * @param path a path to a node (components separated by "/")
+   * @param maxLen maximum allowed length for name
+   * @throws if any of the components do not adhere to naming constraints or
+   * length.
+   */
+  public static toDnsLabel(path: string, maxLen = MAX_DNS_NAME_LEN) {
+    if (maxLen < HASH_LEN) {
+      throw new Error(`minimum max length for object names is ${HASH_LEN} (required for hash)`);
+    }
 
-  const components = path.split('/');
+    const components = path.split('/');
 
-  // verify components only use allowed chars.
-  for (const comp of components) {
-    if (!VALIDATE.test(comp)) {
-      throw new Error(`"${comp}" is not a valid object name. The characters allowed in names are: digits (0-9), lower case letters (a-z), -, and .`);
+    // verify components only use allowed chars.
+    for (const comp of components) {
+      if (!VALIDATE.test(comp)) {
+        throw new Error(`"${comp}" is not a valid object name. The characters allowed in names are: digits (0-9), lower case letters (a-z), -, and .`);
+      }
+
+      if (comp.length > maxLen) {
+        throw new Error(`Object name "${comp}" is too long. Maximum allowed length is ${maxLen}`);
+      }
     }
 
-    if (comp.length > maxLen) {
-      throw new Error(`Object name "${comp}" is too long. Maximum allowed length is ${maxLen}`);
+    // special case: if we only have one component in our path, we don't decorate it
+    if (components.length === 1) {
+      return components[0];
     }
-  }
 
-  // special case: if we only have one component in our path, we don't decorate it
-  if (components.length === 1) {
-    return components[0];
-  }
+    components.push(calcHash(path, HASH_LEN));
 
-  components.push(calcHash(path, HASH_LEN));
+    return components
+      .reverse()
+      .join('/')
+      .slice(0, maxLen)
+      .split('/')
+      .reverse()
+      .filter(x => x)
+      .join('-');
+  }
 
-  return components
-    .reverse()
-    .join('/')
-    .slice(0, maxLen)
-    .split('/')
-    .reverse()
-    .join('.');
+  /* istanbul ignore next */
+  private constructor() {
+    return;
+  }
 }
 
 function calcHash(path: string, maxLen: number) {

packages/cdk8s/lib/testing.ts

@@ -28,6 +28,7 @@ export class Testing {
     return YAML.parseAllDocuments(fs.readFileSync(filePath, 'utf-8')).map((doc: any) => doc.toJSON());
   }
 
+  /* istanbul ignore next */
   private constructor() {
     return;
   }

packages/cdk8s/test/__snapshots__/api-object.test.js.snap

@@ -9,7 +9,7 @@ Array [
     },
     "kind": "ResourceType",
     "metadata": Object {
-      "name": "test.resource.3953b2d0",
+      "name": "test-resource-3953b2d0",
     },
   },
 ]
@@ -21,7 +21,7 @@ Array [
     "apiVersion": "v1",
     "kind": "ResourceType",
     "metadata": Object {
-      "name": "test.resource.3953b2d0",
+      "name": "test-resource-3953b2d0",
     },
     "spec": Object {
       "prop1": "hello",
@@ -51,7 +51,7 @@ Array [
     "apiVersion": "v1",
     "kind": "MyResource",
     "metadata": Object {
-      "name": "test.my-resource.2998ce93",
+      "name": "test-my-resource-2998ce93",
     },
   },
 ]
@@ -63,14 +63,14 @@ Array [
     "apiVersion": "v1",
     "kind": "MyResource",
     "metadata": Object {
-      "name": "test.my-resource.2998ce93",
+      "name": "test-my-resource-2998ce93",
     },
   },
   Object {
     "apiVersion": "v1",
     "kind": "MyResource",
     "metadata": Object {
-      "name": "test.scope.my-resource.eddbdbb2",
+      "name": "test-scope-my-resource-eddbdbb2",
     },
   },
 ]

packages/cdk8s/test/__snapshots__/chart.test.js.snap

@@ -12,35 +12,35 @@ Array [
     "apiVersion": "v1",
     "kind": "Resource1",
     "metadata": Object {
-      "name": "test.resource1.2b59e3b8",
+      "name": "test-resource1-2b59e3b8",
     },
   },
   Object {
     "apiVersion": "v1",
     "kind": "Resource2",
     "metadata": Object {
-      "name": "test.resource2.22077398",
+      "name": "test-resource2-22077398",
     },
   },
   Object {
     "apiVersion": "v1",
     "kind": "Resource3",
     "metadata": Object {
-      "name": "test.resource3.365bba40",
+      "name": "test-resource3-365bba40",
     },
   },
   Object {
     "apiVersion": "v1",
     "kind": "Resource1",
     "metadata": Object {
-      "name": "test.scope.resource1.3d54e80a",
+      "name": "test-scope-resource1-3d54e80a",
     },
   },
   Object {
     "apiVersion": "v1",
     "kind": "Resource2",
     "metadata": Object {
-      "name": "test.scope.resource2.e1b938d2",
+      "name": "test-scope-resource2-e1b938d2",
     },
   },
 ]
@@ -52,7 +52,7 @@ Array [
     "apiVersion": "v1",
     "kind": "Resource1",
     "metadata": Object {
-      "name": "test.resource1.2b59e3b8",
+      "name": "test-resource1-2b59e3b8",
     },
     "spec": Object {
       "foo": 123,

packages/cdk8s/test/app.test.ts

@@ -1,5 +1,7 @@
-import { Testing, Chart } from '../lib';
+import { Testing, Chart, App } from '../lib';
 import * as fs from 'fs';
+import * as os from 'os';
+import * as path from 'path';
 
 test('empty app emits no files', () => {
   // GIVEN
@@ -26,4 +28,29 @@ test('app with two charts', () => {
     'chart1.k8s.yaml',
     'chart2.k8s.yaml'
   ]);
-});
+});
+
+test('default output directory is "dist"', () => {
+  // GIVEN
+  const prev = process.cwd();
+  const workdir = fs.mkdtempSync(path.join(os.tmpdir()));
+
+  try {
+    console.log(workdir);
+    process.chdir(workdir);
+
+    // WHEN
+    const app = new App();
+    new Chart(app, 'chart1');
+    app.synth();
+
+    // THEN
+    expect(app.outdir).toEqual('dist');
+    expect(fs.existsSync('./dist')).toBeTruthy();
+    expect(fs.statSync('./dist').isDirectory());
+    expect(fs.existsSync('./dist/chart1.k8s.yaml')).toBeTruthy();
+  } finally {
+    process.chdir(prev);
+    // fs.rmdirSync(workdir, { recursive: true });
+  }
+});