Skip to content
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

Merged
merged 7 commits into from
Oct 21, 2024
Merged

Conversation

PeterKoegel
Copy link
Contributor

@PeterKoegel PeterKoegel commented Jul 15, 2024

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

  • enable-windows-prefix-delegation
  • warm-prefix-target
  • warm-ip-target
  • minimum-ip-target
  • branch-eni-cooldown

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.

@PeterKoegel PeterKoegel marked this pull request as ready for review July 15, 2024 15:06
Copy link
Collaborator

@shapirov103 shapirov103 left a 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.

@@ -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),
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@PeterKoegel
Copy link
Contributor Author

The schema that is returned by aws eks describe-addon-configuration --addon-name vpc-cni --addon-version v1.18.1-eksbuild.1 shows that there are some properties defines as format: integer, type: string while some of the properties that are introduced by this PR are defined as type: integer in the VPC-CNI addon's config. So it is not possible anymore to convert all integers into strings.
Instead JSON.stringify() is called at assignment of each parameter with format: boolean, type: string or format: integer, type: string.

@PeterKoegel PeterKoegel marked this pull request as ready for review August 9, 2024 07:09
Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shapirov103 shapirov103 merged commit a84e179 into aws-quickstart:main Oct 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants