cdk8s-team / cdk8s

Define Kubernetes native apps and abstractions using object-oriented programming
https://cdk8s.io
Apache License 2.0
4.29k stars 291 forks source link

[cdk8s-cli] Incorrect manifest when configuring a `StatefulSet` with `volumeClaimTemplates` #140

Closed iliapolo closed 3 years ago

iliapolo commented 4 years ago

Describe the bug

A StatefulSet that defines VolumeClaimTemplates is incorrectly synthesized.

To Reproduce

new k8s.StatefulSet(this, 'StatefulSet', {
  spec: {
    serviceName: 'service',
    selector: {
      matchLabels: { a: 'b' }
    },
    template: {
      spec: {
        containers: [{
          name: 'hello'
        }]
      }
    },
    volumeClaimTemplates: [new k8s.PersistentVolumeClaim(this, 'VolumeClaim', {
      spec: {
        accessModes: ['ReadWriteOnce']
      },
    })]
  }
})

This will produce the following manifest:

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: cdk8s-playground-volumeclaim-0db7a75b
spec:
  accessModes:
    - ReadWriteOnce
---
apiVersion: apps/v1
kind: StatefulSet
metadata:
  clusterName: cluster
  name: cdk8s-playground-statefulset-4c20ee82
spec:
  selector:
    matchLabels:
      a: b
  serviceName: service
  template:
    spec:
      containers:
        - name: hello
  volumeClaimTemplates:
    - apiVersion: v1
      chart:
        manifestFile: cdk8splayground.k8s.yaml
      kind: PersistentVolumeClaim
      name: cdk8s-playground-volumeclaim-0db7a75b
      options:
        apiVersion: v1
        kind: PersistentVolumeClaim
        spec:
          accessModes:
            - ReadWriteOnce

There are two problems here:

  1. PersistentVolumeClaim is present both as a top level resource, and a nested resource inside the spec of theStatefulSet.

  2. The nested resource spec contains keys that actually make it invalid. For example, chart, options...

Expected behavior

According to the kubernetes spec for a StatefulSet, I would expect the synthesized manifest to look like so:

apiVersion: apps/v1
kind: StatefulSet
metadata:
  clusterName: cluster
  name: cdk8s-playground-statefulset-4c20ee82
spec:
  selector:
    matchLabels:
      a: b
  serviceName: service
  template:
    spec:
      containers:
        - name: hello
  volumeClaimTemplates:
    - apiVersion: v1
      kind: PersistentVolumeClaim
      metadata:
        name: cdk8s-playground-volumeclaim-0db7a75b
      spec:
        accessModes:
          - ReadWriteOnce

Additional context

The problem stems from the fact that the volumeClaimTemplates property of the StatefulSet spec references a PersistentVolumeClaim object, which is, in a very non characteristic way, an ApiObject itself, rather than a simple struct.

The in turn causes the generated code to force users to create a top level PersistentVolumeClaim object:

volumeClaimTemplates: [new k8s.PersistentVolumeClaim(this, 'VolumeClaim', {
      spec: {
        accessModes: ['ReadWriteOnce']
      },
    })]

Which in turn causes all the problems mentioned here.

As a side thought, even if the synthesis was somehow correct, the API presented here is wrong and doesn't correctly represent the mental model of the use-case, as there is no reason to create a PersistentVolumeClaim construct (resource), if all thats needed is to pass its spec to a parent resource.

This implies that the generated code has to change has to change, and provide a way to simply specify the spec for the volumeClaimTemplates property, and not the actual construct.

dodtsair commented 4 years ago

Is it possible to do PersistentVolumeClaims with StatefulSets while this bug is unfixed, in short is there a work around?

iliapolo commented 4 years ago

@dodtsair Unfortunately there is no workaround at the moment. We hope to address this issue soon, so stay tuned.

mjwilkerson-strateos commented 4 years ago

I'm having the same issue, is there a workaround?

enkoder commented 4 years ago

Any update on timing of this bug? It's blocking me from adopting cdk8s.

iliapolo commented 4 years ago

@enkoder @mjwilkerson-strateos @dodtsair

We understand this issue may block many uses cases so we will be prioritizing this high on our list. I don't have a specific due date yet but a reasonable timeline would be a couple of weeks.

In the meantime, I did actually manage to find a kind of workaround for this issue. Basically, to define a StatefulSet with persistent volume claims, you'll need to drop down to the raw ApiObject construct. It means you won't have type-checks for it, but you can still configure it inside your cdk8s application and use k8s for your other resources that don't present this problem.

Here is how that would look like:

import { ApiObject } from 'cdk8s';

new ApiObject(this, 'StatefulSet', {
  apiVersion: 'apps/v1',
  kind: 'StatefulSet',
  spec: {
    serviceName: 'service',
    selector: {
      matchLabels: { a: 'b' }
    },
    template: {
      spec: {
        containers: [{
          name: 'hello'
        }]
      },
      metadata: {
        labels: {
          a: 'b'
        }
      }
    },
    volumeClaimTemplates: [{
      metadata: {
        name: 'read-volume',
      },
      spec: {
        accessModes: ['ReadWriteOnce']
      }
    }]
  }
})

The spec property here is essentially anything you want, since its not tied to a specific resource spec. Note also that metadata isn't required because cdk8s will still generate a unique name for the resource on your behalf.

Please let us know if the above workaround indeed unblocks you or is there anything else that needs addressing.

Thanks!