-
Notifications
You must be signed in to change notification settings - Fork 207
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
Additional Settings to VpcCniAddOn #1044
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.
@PeterKoegel thank you for the PR, please see my minor comment.
lib/addons/vpc-cni/index.ts
Outdated
@@ -450,7 +481,12 @@ function populateVpcCniConfigurationValues(props?: VpcCniAddOnProps): Values { | |||
WARM_PREFIX_TARGET: props?.warmPrefixTarget, | |||
}, | |||
enableNetworkPolicy: JSON.stringify(props?.enableNetworkPolicy), | |||
enableWindowsIpam: JSON.stringify(props?.enableWindowsIpam) | |||
enableWindowsIpam: JSON.stringify(props?.enableWindowsIpam), |
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.
minor: is there a reason to stringify this here? the code on line 495 should take care of that, if it does not, please let me know. Ideally, I want to have a consistent approach to extend this structure.
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 code in line 459 in deed does NOT take care for properties on the toplevel config object.
To avoid mixing up different things in one PR I created #1048 and added a more detailed description there.
The schema that is returned by |
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.
LGTM
This PR adds support to provide values for certain settings of the VpcCniAddOn that were introduced in amazon-vpc-cni-k8s v1.16.0 in PR additional settings to the helm chart
I verified that this code change does not introduce any functional changes for the already existing vpc-cni settings by comparing the generated cloud formation output for different sets of parameters generated with and without this change and assure they are equal. For the newly introduced vpc-cni settings I tested that they are deployed to an eks ckuster as desired.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.