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

handle stringification the same way for all vpc-cni configuration pro… #1048

Conversation

PeterKoegel
Copy link
Contributor

@PeterKoegel PeterKoegel commented Jul 26, 2024

A finding of #1044 was that the way the configuration properties for the vpc-cni plugin are converted to strings is not handled in the same way for all properties.

Configuration for the vpc-cni core addon is defined as following structure:

  const result: Values = {
    init: {
      env: {
        DISABLE_TCP_EARLY_DEMUX: props?.disableTcpEarlyDemux,
        ENABLE_V6_EGRESS: props?.enableV6Egress,
      }
    },
    env: {
      AWS_EC2_ENDPOINT: props?.awsEc2Endpoint,
      ADDITIONAL_ENI_TAGS: props?.additionalEniTags,
      .. some more properties
    },
    enableNetworkPolicy: props?.enableNetworkPolicy,
    enableWindowsIpam: props?.enableWindowsIpam
  };

Before that configuration is passed to the core addon undefined properties are removed and non-string properties are converted into strings by calling JSON.stringify().

For the "env" and "init.env" subobjects this is done in line 456 and 461 in a loop that iterates over all properties of this child objects, for the properties of the main object however JSON.stringify() is called when assigning the values in line 469 and 470.

This pull request changes that in a way that there is a recursive function "ConvertPropertiesToString" that handles the functionality described above by iterating over the whole configuration object including the embeded subobjects.

I verified that this code change does not introduce any functional changes by comparing the generated cloud formation output for different sets of parameters generated with and without this change and assure they are equal.

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 26, 2024 14:36
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.

let me validate the e2e test and will merge after the minor code quality issues are addressed

ConvertPropertiesToString(helmValues[key]);
}
else if (typeof helmValues[key] !== 'string'){
helmValues[key] = JSON.stringify(helmValues[key])
Copy link
Collaborator

Choose a reason for hiding this comment

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

github action issue missing semicolon here and line 410

@shapirov103
Copy link
Collaborator

/do-e2e-tests

Copy link

@aws-ia-ci aws-ia-ci left a comment

Choose a reason for hiding this comment

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

end to end tests failed. A maintainer can provide more details.

@shapirov103
Copy link
Collaborator

e2e test failure unrelated to the changes. Will be addressed in a separate PR.

@shapirov103
Copy link
Collaborator

/do-e2e-tests

Copy link

@aws-ia-ci aws-ia-ci left a comment

Choose a reason for hiding this comment

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

end to end tests passed

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, thank you!

@shapirov103 shapirov103 merged commit 4a5e65e into aws-quickstart:main Aug 8, 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.

3 participants