changes from code review

aws · May 13, 2024 · 4289418 · 4289418
1 parent 5e65800
commit 4289418
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 33 deletions.
diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-eks/test/integ.alb-controller.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-eks/test/integ.alb-controller.ts
@@ -9,7 +9,7 @@ import { Pinger } from './pinger/pinger';
 import * as eks from 'aws-cdk-lib/aws-eks';
 
 interface EksClusterAlbControllerStackProps extends StackProps {
-  albControllerValues?: {[key:string]: any};
+  albControllerHelmChartValues?: {[key:string]: any};
 }
 
 class EksClusterAlbControllerStack extends Stack {
@@ -25,7 +25,7 @@ class EksClusterAlbControllerStack extends Stack {
       ...getClusterVersionConfig(this),
       albController: {
         version: eks.AlbControllerVersion.V2_6_2,
-        values: props.albControllerValues,
+        helmChartValues: props.albControllerHelmChartValues,
       },
     });
 
@@ -75,7 +75,7 @@ class EksClusterAlbControllerStack extends Stack {
 
 const app = new App();
 const stack = new EksClusterAlbControllerStack(app, 'aws-cdk-eks-cluster-alb-controller');
-const stackWithAlbControllerValues = new EksClusterAlbControllerStack(app, 'aws-cdk-eks-cluster-alb-controller-values', { albControllerValues: { enableWafv2: false } });
+const stackWithAlbControllerValues = new EksClusterAlbControllerStack(app, 'aws-cdk-eks-cluster-alb-controller-values', { albControllerHelmChartValues: { enableWafv2: false } });
 new integ.IntegTest(app, 'aws-cdk-cluster-alb-controller-integ', {
   testCases: [stack, stackWithAlbControllerValues],
   // Test includes assets that are updated weekly. If not disabled, the upgrade PR will fail.

diff --git a/packages/aws-cdk-lib/aws-eks/README.md b/packages/aws-cdk-lib/aws-eks/README.md
@@ -685,7 +685,7 @@ if (cluster.albController) {
 ```
 
 If you need to configure the underlying ALB Controller, you can pass optional values that will be forwarded to the underling HelmChart construct.
-For example, if your organization uses AWS Firewall Manager you will need to disable aws-load-balancer-controller's waf modification behavior or it will disassociate WAFs that are not specified in the Ingress' annotations.
+For example, if your organization uses AWS Firewall Manager you will need to disable aws-load-balancer-controller's waf modification behavior or it will periodically disassociate WAFs that are not specified in the Ingress' annotations.
 
 Note: supported value options may vary depending on the version of the aws-load-balancer-controller helm chart you are using. You should consult the [ArtifactHub documentation](https://artifacthub.io/packages/helm/aws/aws-load-balancer-controller) for your version to ensure you are configuring the chart correctly.
 
@@ -694,7 +694,7 @@ new eks.Cluster(this, 'HelloEKS', {
   version: eks.KubernetesVersion.V1_29,
   albController: {
     version: eks.AlbControllerVersion.V2_6_2,
-    values: {
+    helmChartValues: {
       enableWaf: false,
       enableWafv2: false,
     },

diff --git a/packages/aws-cdk-lib/aws-eks/lib/alb-controller.ts b/packages/aws-cdk-lib/aws-eks/lib/alb-controller.ts
@@ -245,7 +245,7 @@ export interface AlbControllerOptions {
   /**
    * Override values to be used by the chart.
    * For nested values use a nested dictionary. For example:
-   * values: {
+   * helmChartValues: {
    *  autoscaling: false,
    *  ingressClassParams: { create: true }
    * }
@@ -261,7 +261,7 @@ export interface AlbControllerOptions {
    *
    * @default - No values are provided to the chart.
    */
-  readonly values?: {[key: string]: any};
+  readonly helmChartValues?: {[key: string]: any};
 }
 
 /**
@@ -322,6 +322,8 @@ export class AlbController extends Construct {
     }
 
     // https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.2/deploy/installation/#add-controller-to-cluster
+    const { helmChartValues = {} } = props;
+    this.validateHelmChartValues(helmChartValues);
     const chart = new HelmChart(this, 'Resource', {
       cluster: props.cluster,
       chart: 'aws-load-balancer-controller',
@@ -333,7 +335,8 @@ export class AlbController extends Construct {
       wait: true,
       timeout: Duration.minutes(15),
       values: {
-        ...(props.values ?? {}),
+        ...helmChartValues,
+        // if you modify these values, you must also update this.restrictedHelmChartValueKeys
         clusterName: props.cluster.clusterName,
         serviceAccount: {
           create: false,
@@ -369,4 +372,22 @@ export class AlbController extends Construct {
     }
     return resources.map(rewriteResource);
   }
+
+  private validateHelmChartValues(values: {[key: string]: any}) {
+    const valuesKeySet = new Set(Object.keys(values));
+    const invalidKeys = new Set([...valuesKeySet].filter((key) => this.restrictedHelmChartValueKeys.has(key)));
+    if (invalidKeys.size > 0) {
+      throw new Error(`The following aws-load-balancer-controller HelmChart value keys are restricted and cannot be overridden: ${Array.from(this.restrictedHelmChartValueKeys).join(', ')}. (Invalid keys: ${Array.from(invalidKeys).join(', ')})`);
+    }
+  }
+
+  private get restrictedHelmChartValueKeys(): Set<string> {
+    return new Set([
+      'clusterName',
+      'serviceAccount',
+      'region',
+      'vpcId',
+      'image',
+    ]);
+  }
 }
diff --git a/packages/aws-cdk-lib/aws-eks/test/alb-controller.test.ts b/packages/aws-cdk-lib/aws-eks/test/alb-controller.test.ts
@@ -130,7 +130,7 @@ test('can pass values to the aws-load-balancer-controller helm chart', () => {
     cluster,
     version: AlbControllerVersion.V2_6_2,
     repository: 'custom',
-    values: {
+    helmChartValues: {
       enableWafv2: false,
     },
   });
@@ -156,18 +156,18 @@ test('can pass values to the aws-load-balancer-controller helm chart', () => {
   });
 });
 
-test('values passed to the aws-load-balancer-controller do not override values set by construct', () => {
+test('values passed to the aws-load-balancer-controller result in an error if they conflict with restricted values set by the construct', () => {
   const { stack } = testFixture();
 
   const cluster = new Cluster(stack, 'Cluster', {
     version: KubernetesVersion.V1_27,
   });
 
-  AlbController.create(stack, {
+  expect(() => AlbController.create(stack, {
     cluster,
     version: AlbControllerVersion.V2_6_2,
     repository: 'custom',
-    values: {
+    helmChartValues: {
       clusterName: 'test-cluster',
       serviceAccount: {
         create: true,
@@ -180,25 +180,5 @@ test('values passed to the aws-load-balancer-controller do not override values s
         tag: 'test-tag',
       },
     },
-  });
-
-  Template.fromStack(stack).hasResourceProperties(HelmChart.RESOURCE_TYPE, {
-    Version: '1.6.2', // The helm chart version associated with AlbControllerVersion.V2_6_2
-    Values: {
-      'Fn::Join': [
-        '',
-        [
-          '{"clusterName":"',
-          {
-            Ref: 'Cluster9EE0221C',
-          },
-          '","serviceAccount":{"create":false,"name":"aws-load-balancer-controller"},"region":"us-east-1","vpcId":"',
-          {
-            Ref: 'ClusterDefaultVpcFA9F2722',
-          },
-          '","image":{"repository":"custom","tag":"v2.6.2"}}',
-        ],
-      ],
-    },
-  });
+  })).toThrow(/The following aws-load-balancer-controller HelmChart value keys are restricted and cannot be overridden: .*/);
 });