From c293db5c5317e7e4743fff46f7b5ad193b3e265c 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 66cd74f090462..b4fbbbaddbf3e 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 @@ -11,7 +11,7 @@ import * as eks from 'aws-cdk-lib/aws-eks'; const LATEST_VERSION: eks.AlbControllerVersion = eks.AlbControllerVersion.V2_8_2; interface EksClusterAlbControllerStackProps extends StackProps { - albControllerValues?: {[key:string]: any}; + albControllerHelmChartValues?: {[key:string]: any}; } class EksClusterAlbControllerStack extends Stack { @@ -27,7 +27,7 @@ class EksClusterAlbControllerStack extends Stack { ...getClusterVersionConfig(this, eks.KubernetesVersion.V1_30), albController: { version: LATEST_VERSION, - values: props.albControllerValues, + helmChartValues: props.albControllerHelmChartValues, }, }); @@ -77,7 +77,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 8761726ecfbf2..4a9b610f5c880 100644 --- a/packages/aws-cdk-lib/aws-eks/README.md +++ b/packages/aws-cdk-lib/aws-eks/README.md @@ -692,7 +692,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. @@ -701,7 +701,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 02496a3c4ce08..bde180cf3657b 100644 --- a/packages/aws-cdk-lib/aws-eks/lib/alb-controller.ts +++ b/packages/aws-cdk-lib/aws-eks/lib/alb-controller.ts @@ -275,7 +275,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 } * } @@ -291,7 +291,7 @@ export interface AlbControllerOptions { * * @default - No values are provided to the chart. */ - readonly values?: {[key: string]: any}; + readonly helmChartValues?: {[key: string]: any}; } /** @@ -352,6 +352,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', @@ -363,7 +365,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, @@ -402,4 +405,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 7829057432500..8d62237d6265a 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 @@ -164,7 +164,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, }, }); @@ -190,18 +190,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, @@ -214,27 +214,7 @@ 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: .*/); }); describe('AlbController AwsAuth creation', () => {