Skip to content

Commit

Permalink
changes from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
mtrspringer committed Nov 4, 2024
1 parent c47a993 commit c293db5
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -27,7 +27,7 @@ class EksClusterAlbControllerStack extends Stack {
...getClusterVersionConfig(this, eks.KubernetesVersion.V1_30),
albController: {
version: LATEST_VERSION,
values: props.albControllerValues,
helmChartValues: props.albControllerHelmChartValues,
},
});

Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/aws-eks/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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,
},
Expand Down
27 changes: 24 additions & 3 deletions packages/aws-cdk-lib/aws-eks/lib/alb-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
* }
Expand All @@ -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};
}

/**
Expand Down Expand Up @@ -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',
Expand All @@ -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,
Expand Down Expand Up @@ -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<string> {
return new Set([
'clusterName',
'serviceAccount',
'region',
'vpcId',
'image',
]);
}
}
30 changes: 5 additions & 25 deletions packages/aws-cdk-lib/aws-eks/test/alb-controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
});
Expand All @@ -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,
Expand All @@ -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', () => {
Expand Down

0 comments on commit c293db5

Please sign in to comment.