cdk8s-team / cdk8s

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

CRD versioning should be consistent. v1beta1, v1alpha1, v1 etc #1962

Open shinebayar-g opened 3 months ago

shinebayar-g commented 3 months ago

Description of the bug:

When using cdk8s import, if there are multiple versions of the CRD like v1alpha1 & v1beta1, cdk8s uses v1alpha1 for main class name. Because of this, a lot of users are unknowingly using the v1alpha1 resource besides there is a newer version available.

Reproduction Steps:

In this example,

cdk8s generates 2 different versions for ExternalSecret CRD. However v1alpha1 becomes the main class.

export class ExternalSecret extends ApiObject {
  /**
   * Returns the apiVersion and kind for "ExternalSecret"
   */
  public static readonly GVK: GroupVersionKind = {
    apiVersion: 'external-secrets.io/v1alpha1',
    kind: 'ExternalSecret',
  }
}

...

export class ExternalSecretV1Beta1 extends ApiObject {
  /**
   * Returns the apiVersion and kind for "ExternalSecretV1Beta1"
   */
  public static readonly GVK: GroupVersionKind = {
    apiVersion: 'external-secrets.io/v1beta1',
    kind: 'ExternalSecret',
  }
}

Or in this example, cdk8s generates

export class HttpRoute extends ApiObject {
  /**
   * Returns the apiVersion and kind for "HTTPRoute"
   */
  public static readonly GVK: GroupVersionKind = {
    apiVersion: 'gateway.networking.k8s.io/v1',
    kind: 'HTTPRoute',
  }
}

...

export class HttpRouteV1Beta1 extends ApiObject {
  /**
   * Returns the apiVersion and kind for "HTTPRouteV1Beta1"
   */
  public static readonly GVK: GroupVersionKind = {
    apiVersion: 'gateway.networking.k8s.io/v1beta1',
    kind: 'HTTPRoute',
  }
}

In this case v1 class is the main class, much better.

There are other cases where it gets inconsistent. For example, when there are v1 and v2 CRDs, v1 becomes the main class and v2 gets prefix like Somethingv2. However if there are v2 and v3 CRDs, v2 CRD becomes main class with no prefix.

export class Host extends ApiObject {
  /**
   * Returns the apiVersion and kind for "Host"
   */
  public static readonly GVK: GroupVersionKind = {
    apiVersion: 'getambassador.io/v2',
    kind: 'Host',
  }
}
...

export class HostV3Alpha1 extends ApiObject {
  /**
   * Returns the apiVersion and kind for "HostV3Alpha1"
   */
  public static readonly GVK: GroupVersionKind = {
    apiVersion: 'getambassador.io/v3alpha1',
    kind: 'Host',
  }
}

Error Log:

Environment:

Other:

Perhaps cdk8s can follow Terraform k8s provider's approach to version everything and remove unversioned class names.


This is :bug: Bug Report

iliapolo commented 3 months ago

@shinebayar-g thanks for reporting this.

I agree that we should be version suffixing everything, the problem is that would be a breaking change, which we decided to avoid for now. We thought it makes sense for the non suffixed class (i.e main class) to be the most stable version (not necessarily that latest) of the class. This is why we prefer v2 over v3alpha1 for Host, but I guess its a little ambiguous what to do when we have v1alpha1 and v1beta1...agree that intuitively v1beta1 seems like the more appropriate fit.

We'll look into it. Thanks!

shinebayar-g commented 3 months ago

Yeah I agree on the breaking change part. When there are only v2 and v3 CRDs, cdk8s generated non-suffixed version for the v2 CRD, none of us assumed that v2 CRD was the main class when we were using the non-suffexed version. What if we reserve non-suffixed version for only v1 of the classes? :thinking: ... Well it's still going to be a breaking change anyways, so we might as well just push for the v1 suffix at that point.

iliapolo commented 3 months ago

Yeah, we might consider a new major version if we have additional pressing issues that require a breaking change.

(please share if you know of any such issues)