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

fix(eks): Add reconciliation for addon configurationValues #1844

Merged

Conversation

smcavallo
Copy link
Contributor

Description of your changes

eks addon configurationValues is supported already in the spec but is not included in isUpToDate and therefore can never be changed with the provider.
https://github.com/crossplane-contrib/provider-aws/blob/master/apis/eks/v1alpha1/zz_addon.go#L40-L43

This PR adds the custom isUpToDate in order to update the configurationValues
The configurationValues is a string value but both json and yaml are supported. (See - https://aws.amazon.com/blogs/containers/amazon-eks-add-ons-advanced-configuration/)
Note the implementation for the string comparison.
Whitespace and indendation is difficult to get correct when using a string value.
This PR does UnMarshal and Marshal the json or yaml to ensure that the comparison is more accurate. It lessens the risk that this provider will continually request an update of the external resource due to formatting, similar to the "normalization" done with iam policies. Unit test cases have been added for testing format differences.

The addOn example has been updated to reflect yaml definition but json is supported as well.

yaml:

    configurationValues: |
      controller:
        podAnnotations:
          fluentbit.io/exclude: "true"

json:

      configurationValues: '{"controller": {"podAnnotations": {"fluentbit.io/exclude":
      "true"}}}'

What this PR does NOT do:

  • Any validation for yaml/json formatting
  • Each addOn publishes a configuration schema. This provider does not validate the cr against the provided schema. This PR will send the update and aws will reject the change due to invalid schema

Fixes #

I have:

  • [X ] Read and followed Crossplane's contribution process.
  • [X ] Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • define configurationValues for an addOn using the above json

  • ensure provider-aws reconciles the resource and json configurationValues appear in the console

  • ensure that upon the next reconcile the External resource is up to date

  • set configurationValues to ""

  • ensure provider-aws reconciles the resource and the default value of - appears in the console

  • ensure that upon the next reconcile the External resource is up to date

  • do the same as the above but with yaml

  • ensure that no changes are made when configurationValues is not defined for an addOn - provider-aws reconciles the resource and the default value of - appears in the console

  • ensure that the isUpToDate for configurationValues handles exceptions and returns them so they can be written as errors in the logs

  • ensure that when using values which violate the schema the provider throws the exception to the logs:

2023-08-18T03:16:27.324Z        DEBUG   events  cannot update Addon in AWS: InvalidParameterException: ConfigurationValue provided in request is not supported: Yaml schema validation failed with error: [$.controller2: is not defined in the schema and the schema does not allow additional properties]
{
  RespMetadata: {
    StatusCode: 400,
    RequestID: ""
  },

@smcavallo smcavallo force-pushed the eks-addon-configurationValues branch from 64c6683 to c32f2b3 Compare August 18, 2023 13:06
pkg/controller/eks/addon/setup.go Outdated Show resolved Hide resolved
pkg/controller/eks/addon/setup.go Outdated Show resolved Hide resolved
@smcavallo smcavallo force-pushed the eks-addon-configurationValues branch from 55806e9 to 5f42495 Compare August 29, 2023 02:23
Copy link
Collaborator

@MisterMX MisterMX 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 very much for your contribution @smcavallo!

@MisterMX MisterMX merged commit c62342b into crossplane-contrib:master Aug 29, 2023
8 checks passed
@smcavallo smcavallo deleted the eks-addon-configurationValues branch September 1, 2023 15:14
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