Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
shinebayar-g opened this issue May 23, 2024 · 3 comments · May be fixed by cdk8s-team/cdk8s-cli#2728
Open

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

shinebayar-g opened this issue May 23, 2024 · 3 comments · May be fixed by cdk8s-team/cdk8s-cli#2728
Labels
bug Something isn't working effort/medium 1 week tops priority/p2 Dependent on community feedback. PR's are welcome :)

Comments

@shinebayar-g
Copy link

shinebayar-g commented May 23, 2024

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:

  • Framework Version: "cdk8s-cli": "2.198.105", "cdk8s": "^2.68.69",
  • OS:

Other:

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


This is 🐛 Bug Report

@shinebayar-g shinebayar-g added bug Something isn't working needs-triage Priority and effort undetermined yet labels May 23, 2024
@iliapolo
Copy link
Member

@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!

@iliapolo iliapolo added effort/medium 1 week tops priority/p2 Dependent on community feedback. PR's are welcome :) and removed needs-triage Priority and effort undetermined yet labels May 27, 2024
@shinebayar-g
Copy link
Author

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? 🤔 ... 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
Copy link
Member

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)

shinebayar-g added a commit to shinebayar-g/cdk8s-cli that referenced this issue Sep 14, 2024
shinebayar-g added a commit to shinebayar-g/cdk8s-cli that referenced this issue Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working effort/medium 1 week tops priority/p2 Dependent on community feedback. PR's are welcome :)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants