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

chore(eks): update ALB controller versions #29470

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

nmussy
Copy link
Contributor

@nmussy nmussy commented Mar 13, 2024

Issue # (if applicable)

None as far as I could tell

Reason for this change

Update the CDK listed ALB controller versions to match the current availability

Description of changes

  • Added missing controller versions
  • Updated the Helm version of existing controller versions

Description of how you validated changes

I listed the list of available versions by using the ecr:ListImages command on the amazon/aws-load-balancer-controller repository. I'm also filtering out tags that do not match a v1.2.3 pattern, e.g. v2.0.0-rc5, v2.0.0-test-linux_amd64

For the Helm chart version, I initially manually went through the blame history of https://github.com/aws/eks-charts/blob/master/stable/aws-load-balancer-controller/Chart.yaml. @guessi then recommended I use the Helm CLI to obtain the corresponding versions, which worked a ton better and was easily integrated to my tool:

helm repo add eks https://aws.github.io/eks-charts
helm repo update
helm search repo aws-load-balancer-controller --versions --output json

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK p2 labels Mar 13, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team March 13, 2024 13:18
@nmussy
Copy link
Contributor Author

nmussy commented Mar 13, 2024

Updating the existing Helm chart versions changed the integ tests, not sure if I made a mistake by changing them. I can revert it or update the integs, let me know

EDIT: No longer necessary with the helm CLI output, the Git tags were incorrect

@nmussy
Copy link
Contributor Author

nmussy commented Mar 23, 2024

AlbControllerVersion should be up to date now, ~~I'll update the integs in a bit 👍 ~~

EDIT: No longer necessary with the helm CLI output, the Git tags were incorrect

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 23, 2024
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

Thanks for updating these @nmussy! And good callout on using the Helm CLI @guessi!

@paulhcsun
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented Apr 23, 2024

update

✅ Branch has been successfully updated

Copy link
Contributor

mergify bot commented Apr 23, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 23, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 45e193f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit f00f918 into aws:main Apr 23, 2024
12 checks passed
Copy link
Contributor

mergify bot commented Apr 23, 2024

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

paulhcsun added a commit that referenced this pull request Apr 24, 2024
mergify bot pushed a commit that referenced this pull request Apr 24, 2024
Reverts #29470

Reverting due to potential unwanted updating of resources with helm chart version changes.
@paulhcsun
Copy link
Contributor

Hey @nmussy, after discussing with the team we've decided to revert this PR to err on the side of caution in case there is any unintended consequences with resources being replaced. Before I'd like to accept the change again I just have some questions to clarify:

  1. Were there any integ test changes?
  2. Could you explain what you meant by the tests no longer being necessary with the helm CLI output, and that the Git tags were incorrect?
  3. Are you able to add any tests to verify that this would not be a breaking change that would not be a breaking change?
  4. Is there any other reason for this change besides matching the versions to the current availability? It seems like it was working fine before as there were no reported issues.

@nmussy
Copy link
Contributor Author

nmussy commented Apr 24, 2024

Hey @paulhcsun, sorry for the confusion.

  1. No, there were integ tests change initially, but using the helm CLI resolved this issue
  2. Before @guessi mentioned the helm CLI, I used the git file history of https://github.com/aws/eks-charts/blob/master/stable/aws-load-balancer-controller/Chart.yaml, as indicated in the comments of the class:
    /**
    * Controller version.
    *
    * Corresponds to the image tag of 'amazon/aws-load-balancer-controller' image.
    */
    export class AlbControllerVersion {

    This turned out to be erroneous, and I'd updated more versions than I should have, see ec94f1a. This caused integ tests changes. After switching to the CLI, only the controller versions with the 1.4.1 version required a change. These controller versions were introduced by feat(eks): alb controller include versions 2.4.2 - 2.5.1 #25330, by @colifran
  3. I'm not really sure how I would test that. It would definitely cause a change, but I don't know if it would be a breaking one
  4. No reason other than matching the Helm version

If we want to err on the side of caution I can reopen this PR and only add the new versions, let me know 👍

@paulhcsun
Copy link
Contributor

No worries at all! I jumped the gun on this one. I think let's just stick with adding the new versions for now until we have a better way of testing it helm chart version changes for the existing versions. The fact that no snapshots are changing could be due to a testing gap but I will confirm that with @colifran and get back to you.

For newly added versions it looks like we can use them in the existing unit and integ tests per #27910. It looks like we'd also need to add in the policy.json files for each of the new versions we're adding. I'm also checking with a team member to confirm if there are any other ways to test these changes.

@nmussy nmussy deleted the chore-eks-alb-controller-version branch April 25, 2024 08:00
@nmussy
Copy link
Contributor Author

nmussy commented Apr 25, 2024

Hey @paulhcsun, I just opened #29959, we can move our conversation there. Thanks for the help 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distinguished-contributor [Pilot] contributed 50+ PRs to the CDK p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants