-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(eks): pass helm chart values to aws-load-balancer-controller #29723
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -271,6 +271,27 @@ export interface AlbControllerOptions { | |
* @default - Corresponds to the predefined version. | ||
*/ | ||
readonly policy?: any; | ||
|
||
/** | ||
* Override values to be used by the chart. | ||
* For nested values use a nested dictionary. For example: | ||
* helmChartValues: { | ||
* autoscaling: false, | ||
* ingressClassParams: { create: true } | ||
* } | ||
* | ||
* Note that the following values are set by the controller and cannot be overridden: | ||
* - clusterName | ||
* - serviceAccount.create | ||
* - serviceAccount.name | ||
* - region | ||
* - vpcId | ||
* - image.repository | ||
* - image.tag | ||
* | ||
* @default - No values are provided to the chart. | ||
*/ | ||
readonly helmChartValues?: {[key: string]: any}; | ||
} | ||
|
||
/** | ||
|
@@ -331,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', | ||
|
@@ -342,6 +365,8 @@ export class AlbController extends Construct { | |
wait: true, | ||
timeout: Duration.minutes(15), | ||
values: { | ||
...helmChartValues, | ||
// if you modify these values, you must also update this.restrictedHelmChartValueKeys | ||
clusterName: props.cluster.clusterName, | ||
serviceAccount: { | ||
create: false, | ||
|
@@ -380,4 +405,22 @@ export class AlbController extends Construct { | |
} | ||
return resources.map(rewriteResource); | ||
} | ||
|
||
private validateHelmChartValues(values: {[key: string]: any}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @paulhcsun i have already implemented the validation as a function that restricts the keys the user is allowed to modify. do you mean that overriding the values should be an instance method? this does not make sense to me and does not align with how other properties of the AlbController HelmChart wrapper are set. for example, there is no albController.updateVersion() method, the helm chart version is passed in the constructor. an instance method would also require updating the HelmChart construct to support modifying values after initialization (the AlbController constructor initializes the underlying HelmChart construct), which again makes the solution more complicated and expands beyond the scope of the AlbController - the HelmChart construct does not need to support value modification as it allows for passing arbitrary values already. let me know if any modifications to this implementation are required and i'll be happy to make them. |
||
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', | ||
]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this is quite the right direction here. From what I can tell, only one or two values need to be overridden for your use case and this is basically giving the user carte blanche to override far more than that. I chatted with @paulhcsun about this PR and I'm thinking that it would likely be better to have a function that can be called after instantiation to disable the load balancer wafv2 behavior instead of adding a prop.
In general, we don't want props to contradict one another or allow values that we know are not allowed (i.e. the list of values you added in the docstring).
What I'm really having trouble with here is understanding how those values are being set in the first place, in your use case. Can you explain what's going on in your specific case a bit more so that our suggestions for the fix don't send you in circles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also probably suggest that the function to update this behavior should be scoped to the ALB controller but, again, I'd like to understand better how these fields are being set before saying you should make this code change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @TheRealAmazonKendra thanks for your review.
you are correct that for my specific use case I only need to update one or two values, but my intention with this PR is to address the issue of aws-cdk-lib/aws-eks package not exposing the ability to set whatever values the user wants (apart from the ones it is already setting).
the AlbController is just a thin wrapper around the HelmChart construct, yet its constructor's props do not expose any ability to pass values to the underlying helm chart. IMO this was a poor design choice, as I don't think aws-cdk should be the arbiter of which values can be passed to the aws-load-balancer-controller helm chart.
the initial implementation of the Cluster construct did not include any props related to the albController. They were later added as a convenience due to the fact that it is commonly utilized to integrate with aws elbv2. my assumption is that the AlbController construct was defined at the same time, due to the fact that
AlbControllerOptions = Omit<AlbControllerProps, 'cluster'>
. However, the lack of an interface exposing configuration of the helm chart's values means that any user leveraging aws firewall manager either cannot use the "official" construct for the helm chart, or they have to accept that aws-load-balancer-controller will periodically disassociate an fms-applied web acl and their albs willbe unprotected until fms can detect & remediate (usually this is a couple minutes, but we have had occurrences of the remediation failing and albs being unprotected for hours).Due to the fact that a helm chart's values.yml is the way in which every helm chart's default behavior is overridden, i feel that adding a prop for passing values to the AlbController is a valid design choice. There may be other default behaviors of the helm chart beyond waf management that users may want to disable. Due to the fact that the supported values for aws-load-balancer-controller may vary from one version to the next, I didn't feel it was wise to define a type/interface for a subset of configurable values that will then need to be kept up-to-date. I think if users decide to pass values to the AlbController, they are at the point where they know they need to and we should trust that they are able to decide which values to set.
as far as your suggestion for a function to update values after instantiation, this would need to be exposed on both the AlbController construct and the HelmChart construct. i feel like this is not a great approach and it expands the scope of the solution beyond the AlbController construct.
in short, i feel that this is a fix for an overly-restrictive construct props definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I would also want to be able to edit values through this way, in my case for values
enableCertManager
,cluster.dnsDomain
and some more.I wonder why this functionality wasn't introduced from the beginning...