From 285af63268f2881386ce535b06eef16a5042d979 Mon Sep 17 00:00:00 2001 From: mtrspringer Date: Mon, 13 May 2024 17:43:52 +0100 Subject: [PATCH] changes from code review --- .../test/aws-eks/test/integ.alb-controller.ts | 6 ++-- packages/aws-cdk-lib/aws-eks/README.md | 4 +-- .../aws-cdk-lib/aws-eks/lib/alb-controller.ts | 27 +++++++++++++++-- .../aws-eks/test/alb-controller.test.ts | 30 ++++--------------- 4 files changed, 34 insertions(+), 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 index a6e30b105f329..d30ad7a727f79 100644 --- 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 index 99ca57292720c..a0cb9599e8689 100644 --- a/packages/aws-cdk-lib/aws-eks/README.md +++ b/packages/aws-cdk-lib/aws-eks/README.md @@ -688,7 +688,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. @@ -697,7 +697,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 index 42a6088d31b2b..ab53f16a4563e 100644 --- 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 { + 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 index b6006ca835a13..d6eef77cc2a17 100644 --- 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: .*/); });