-
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?
Conversation
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
ec6de8a
to
afcd343
Compare
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.
Looks good, thanks! 👍
Left some minor suggestions for updating the documentation.
2926163
to
5e9a3f6
Compare
* | ||
* @default - No values are provided to the chart. | ||
*/ | ||
readonly values?: {[key: string]: any}; |
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.
readonly values?: {[key: string]: any}; | |
readonly helmChartValues?: {[key: string]: any}; |
Can you rename this to helmChartValues
? Just having this be values
in this context is not clear what these values are being used for. Ideally I'd have like the property within Helm Chart to be named something other than values
as well but unfortunately it's too late for that now.
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.
yes that's fine with me. I was using the HelmChartProps as an guide but I can see the value in being specific within the scope of a broader construct that's not limited to helm already.
* values: { | ||
* 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 |
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.
Do we know the full set of values that can be passed in to the helmChart.values
property? If so could we add some stronger typing checks to them? If not then could we add some validation for the values that cannot be overridden?
From your unit test case it seems like Helm Chart will just ignore these values but I would rather prevent these values from being set at all instead of allowing it to be passed in and have it silently fail.
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 think the set of values varies (or can potentially vary) depending on the helm chart version. I think defining a type (e.g. AlbControllerHelmChartValues) will create a loose link to the helm chart that has the potential for breaking. Validating the passed values against the keys we know we explicitly set seems like a better approach to me.
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.
Gotcha, thanks for clarifying. Then let's go with the second option to validate the passed values that cannot be overridden.
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.
@paulhcsun i've updated with the requested changes. Assuming the suite passes, would you (or someone else) be able to run/update the integration tests? I ran into some issues with the teardown last time.
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.
Hey @mtrspringer, thanks for making the changes! and ya for sure, I'll give that a run later today.
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.
Hey @mtrspringer, apologies some other items came up. I will run the tests now.
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.
It took a while to run on Friday and looks like it failed due to a credentials related error but it still seems to have updated the snapshots. I've pushed them so hopefully the codebuild will be happy with the new snapshots.
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.
@paulhcsun thanks very much! yes it takes ages, looks like the build is passing though
5e9a3f6
to
4289418
Compare
4625454
to
b55781a
Compare
b55781a
to
cd2f5a8
Compare
Hi @mtrspringer, apologies for another delay to getting around to reviewing this. I was oncall last week. Thank you for making the requested changes for validating the values that can be passed to Would you mind explaining what the changes within |
cd2f5a8
to
0d9c286
Compare
@paulhcsun hi sorry i've been on vacation the last 2 weeks. I dont remember making any changes to those files, maybe they got included when i updated my branch via the github button? im happy to leave them out |
bff33f3
to
285af63
Compare
@paulhcsun looks like they were .d.ts and .js files that somehow got included. i've just removed them so this PR should be ready for review now |
* | ||
* @default - No values are provided to the chart. | ||
*/ | ||
readonly helmChartValues?: {[key: string]: any}; |
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...
Hi, any updates or estimation on this? Can i help somehow? |
81733cc
to
89d6dbd
Compare
Pull request has been modified.
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
hi @Necrokefalos im still waiting on a response as to what changes, if any, need to be made. good to know that the values also need to be modified for cert-manager as we are looking to adopt that sometime soon. in the meantime i think the integration tests need to be run again before the checks pass. @paulhcsun would you be able to do that again at your earliest convenience? thanks. |
ab070af
to
925b12e
Compare
@Necrokefalos not sure if this will work for your use-case (and it's hacky) but in the interim while this is being deliberated I realized we can "disable" alb controller behavior via the serviceAccount policy, which we can access via the construct node.findChild api: // TODO: remove once cdk supports passing values to albController Helm Chart:
const {enableWafv2 = false} = albControllerValues;
if (!enableWafv2) {
const albControllerServiceAccount =
this.eksCluster.albController!.node.findChild(
'alb-sa'
) as eks.ServiceAccount;
albControllerServiceAccount.addToPrincipalPolicy(
new iam.PolicyStatement({
effect: iam.Effect.DENY,
actions: [
'waf-regional:GetWebACL',
'waf-regional:GetWebACLForResource',
'waf-regional:AssociateWebACL',
'waf-regional:DisassociateWebACL',
'wafv2:GetWebACL',
'wafv2:GetWebACLForResource',
'wafv2:AssociateWebACL',
'wafv2:DisassociateWebACL',
],
resources: ['*'],
})
);
} i still have to test how this affects the controller's operation (i.e. can it gracefully handle the permission errors) but it might be easier than replacing the cdk.eks.Cluster.albController with a custom HelmChart implementation. |
Hi @mtrspringer, apologies for the late response I was away recently and just got back to this. I've started running the integ tests so hopefully i'll get those pushed today or tmr. I think when I last discussed with Kendra there was one design change for how we allow the users to update the helm chart values. I will confirm with her and get back asap. Apologies for the churn. |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
I've pushed the snapshot changes for the
I'm not quite sure how this test is related or why it's failing now. Don't se anything related to |
@paulhcsun thanks yeah thats bizarre, i didnt make any changes to the nlb tests or source code. maybe something strange happened with the test assets during rebasing? should i revert all asset/generated changes so that its just my desired code and let you run the suite again? |
45a2cf9
to
6840f72
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Thank you @mtrspringer for submitting the PR, looking at the initial suggestion here I would also prefer a function based implementation that can give users the option to override only set of allowed values possible for alb-controller.
Issue # (if applicable)
Closes #29707
Reason for this change
Need to be able to disable aws-load-balancer-controller wafv2 behavior so it doesn't remove waf associations created by AWS FMS.
Description of changes
I added a
values
property to theAlbControllerOptions
interface for passing optional values to the underlying helm chart.Description of how you validated changes
I added two tests to validate my changes:
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license