feat(eks): alb controller include versions 2.4.2 - 2.5.1

[colifran]

Motivation:

We should provide users with all available ALB controller versions for use with aws-eks. This change does not prohibit users from using previous ALB controller versions. Instead, this adds support for versions 2.4.2 - 2.5.1. Previous ALB controller versions can be specified by using the static "of" method as part of the AlbControllerVersion class, e.g., AlbControllerVersion.of().

Testing:

Updated existing ALB controller integrity test to use ALB controller version 2.5.1.

Closes #25307

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[kaizencc]

I have some minor thoughts but other than that this looks good!

[kaizencc]

Lets give until Monday for @iliapolo to take a look, because I do find it weird to have a different policy per version and I didn't have time to investigate further.

In the meantime, I think the PR description could use an update (and note that users could specify these versions on their own via the AlbControllerVersion.of() method so we're not actually denying any uses these versions atm.

[kaizencc]

Looks good!

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[djnalluri]

These changes may not be enough to support version 2.5.0 and above. The release notes mention incompatibilities with deployment manifests from previous versions.

[djnalluri]

I have tried using .of('v2.5.1') with a custom policy. It is able to deploy, however, the pods cannot start up because they don't have the required permissions for the K8s Lease API that were added in version 1.5.0 of the helm chart. I would suggest adding an extra constructor parameter for the chart version. The appropriate chart can then be picked per ALB version instead of the hardcoded 1.4.1.

[colifran]

Thanks for catching this! I will work on addressing it.

[kaizencc]

Because of this, it's not good enough to default helm version to 1.4.1 everywhere. That would mean that if someone wanted to do this:

new AlbController(this, 'blah', {
  version: AlbControllerVersion.V2_5_1,
});

Somewhere along the line it would fail because it's not compatible with helm 1.4.1. So we'd force the user to have to do something like:

new AlbController(this, 'blah', {
  version: AlbControllerVersion.V2_5_1,
  helmChart: HelmChartVersion.V1_5_0,
});

Even though we know what helm chart version would be compatible. So we should be able to add the default in for the user.

In practice that means that we add a prop to the constructor of AlbControllerVersion called helmChartVersion.

class AlbControllerVersion {
  public static readonly V2_5_1 = new AlbControllerVersion('v2.5.1', HelmChartVersion.V1.5.0, false);
  private constructor(public readonly version: string, public readonly helmChart, public readonly custom: boolean) {}
}

// where we need it, we can do
props.albControllerVersion.helmChart;

None of this should be a breaking change since the constructor for AlbContrllerVersion is private

[colifran]

Thanks, Kaizen. This makes sense. I've what I think are the necessary updates for the AlbControllerVersion class to support the addition of helmChartVersion in the constructor

[kaizencc]

A couple more comments :)

[kaizencc]

adding this is a breaking change. It should be optional and default to 1.4.1 so that we dont break people

[colifran]

Thanks for catching this - I've removed this and added it to the AlbControllerVersion constructor to allow this to support current users.

[kaizencc]

having to add these here is the breaking change. once you make it optional, be sure to remove these

[colifran]

Removed.

[kaizencc]

Anyone using these old versions of helm? I think we can get by with an enum-like class here too:

class HelmChartVersion {
  public static V1_4_1 = new HelmChartVersion('1.4.1');
  public static V1_5_1 = new HelmChartVersion('1.5.1');

  public constructor(public version: string) {}
}

Let people just specify their other versions I think

[colifran]

Good point. I opted against using an enum-like class here though. I'm wondering if this would be too much abstraction for what would essentially just be a string? Instead, I decided to hard code the helm chart version for the static alb controller versions that create instances of the alb controller class similar to the already hard coded alb controller version that is passed to the constructor.

[kaizencc]

Because of this, it's not good enough to default helm version to 1.4.1 everywhere. That would mean that if someone wanted to do this:

new AlbController(this, 'blah', {
  version: AlbControllerVersion.V2_5_1,
});

Somewhere along the line it would fail because it's not compatible with helm 1.4.1. So we'd force the user to have to do something like:

new AlbController(this, 'blah', {
  version: AlbControllerVersion.V2_5_1,
  helmChart: HelmChartVersion.V1_5_0,
});

Even though we know what helm chart version would be compatible. So we should be able to add the default in for the user.

In practice that means that we add a prop to the constructor of AlbControllerVersion called helmChartVersion.

class AlbControllerVersion {
  public static readonly V2_5_1 = new AlbControllerVersion('v2.5.1', HelmChartVersion.V1.5.0, false);
  private constructor(public readonly version: string, public readonly helmChart, public readonly custom: boolean) {}
}

// where we need it, we can do
props.albControllerVersion.helmChart;

None of this should be a breaking change since the constructor for AlbContrllerVersion is private

[kaizencc]

@colifran one last thing. i think we need to add a unit test that tests that when you specify AlbControllerVersion.V2_5_1 you get the corresponding helm chart 1.5.2. That should be doable by looking through the properties of Custom::AWSCDK-EKS-HelmChart in the template.

You're doing this already in the integ test so its not like I think it won't succeed. I just think it's important to have a unit test for this actually new functionality you are adding.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 038d80b
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).