fix(eks): changing the subnets or securityGroupIds order causes an error

[AviorSchreiber]

When the subnet list is passed to the EKS Cluster construct in a different order, an update is triggered to the EKS cluster.
The update process fails as it falsely identifies a change for an unsupported update, although the list has the same items.

The solution is to change the analyzeUpdate function to return updateVpc: false if only the securityGroups/subnetsId order has been changed.

Fixes: #24162

---

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

[gitpod-io]

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[AviorSchreiber]

Clarification Request

How can I perform integration test for that change?
it only happens when updating the stack

[Naumel]

Clarification Request

How can I perform integration test for that change? it only happens when updating the stack

Hi, have you looked at the information present in https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md already?

[AviorSchreiber]

Clarification Request
How can I perform integration test for that change? it only happens when updating the stack

Hi, have you looked at the information present in https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md already?

@Naumel
I have read the guide and look over the current examples.
I don't see how can I perform an integration test that checks an event that only occurs on stack update.
The way I see it, the test must have a stack deployed and then updated, in order to evaluate whether the subnets have changed or not.
This is not something that can be determined by simply synthesizing the stack.
I think that it might be a case where we cannot write an integration test.

Any ideas?

[TheRealAmazonKendra]

Clarification Request
How can I perform integration test for that change? it only happens when updating the stack

Hi, have you looked at the information present in https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md already?

@Naumel I have read the guide and look over the current examples. I don't see how can I perform an integration test that checks an event that only occurs on stack update. The way I see it, the test must have a stack deployed and then updated, in order to evaluate whether the subnets have changed or not. This is not something that can be determined by simply synthesizing the stack. I think that it might be a case where we cannot write an integration test.

Any ideas?

Ah, actually this is a really good question because we don't have a function specifically for update operations. We can, however, edit one of these fields on an existing test because then it deploys the original template first, and then performs the update on it as long as you use the flag --update-on-failed. Can you do that one a test and commit the change? We'll call that good.

[TheRealAmazonKendra]

Removing the label for now but please feel free to add it back (using the same phrase in a comment) if you need more information or further clarification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

[AviorSchreiber]

Clarification Request
How can I perform integration test for that change? it only happens when updating the stack

Hi, have you looked at the information present in https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md already?

@Naumel I have read the guide and look over the current examples. I don't see how can I perform an integration test that checks an event that only occurs on stack update. The way I see it, the test must have a stack deployed and then updated, in order to evaluate whether the subnets have changed or not. This is not something that can be determined by simply synthesizing the stack. I think that it might be a case where we cannot write an integration test.
Any ideas?

Ah, actually this is a really good question because we don't have a function specifically for update operations. We can, however, edit one of these fields on an existing test because then it deploys the original template first, and then performs the update on it as long as you use the flag --update-on-failed. Can you do that one a test and commit the change? We'll call that good.

@TheRealAmazonKendra can you please review it now?

[TheRealAmazonKendra]

Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests)

[TheRealAmazonKendra]

Here is my concern about this change, and perhaps you can provide clarity on whether or not this will be a problem, by using sort, won't this cause the error this was meant to solve on existing an existing EKS cluster? I'm going to pull this PR down to test a couple scenarios, but this change may need to go under a feature flag. I'll provide a follow up later today.

[AviorSchreiber]

Here is my concern about this change, and perhaps you can provide clarity on whether or not this will be a problem, by using sort, won't this cause the error this was meant to solve on existing an existing EKS cluster? I'm going to pull this PR down to test a couple scenarios, but this change may need to go under a feature flag. I'll provide a follow up later today.

It won't cause an existing cluster to be failed and I'll explain why.

The change is analyzed by a lambda function that returns for each component if it changed or not.
The bug occurs due to the comparison mechanism, which takes into account the order of the list items and not just the list contents.

The expected behavior is not to consider the order of the list but the Subnets IDs only.

So if we compare the sorted subnets that passed to the function, new ones and old we will get the same object if the new subnets IDs are not changed.

[TheRealAmazonKendra]

Here is my concern about this change, and perhaps you can provide clarity on whether or not this will be a problem, by using sort, won't this cause the error this was meant to solve on existing an existing EKS cluster? I'm going to pull this PR down to test a couple scenarios, but this change may need to go under a feature flag. I'll provide a follow up later today.

It won't cause an existing cluster to be failed and I'll explain why.

The change is analyzed by a lambda function that returns for each component if it changed or not. The bug occurs due to the comparison mechanism, which takes into account the order of the list items and not just the list contents.

The expected behavior is not to consider the order of the list but the Subnets IDs only.

So if we compare the sorted subnets that passed to the function, new ones and old we will get the same object if the new subnets IDs are not changed.

Oh, wonderful! Thank you for clarifying!

[TheRealAmazonKendra]

@Mergifyio update

[mergify]

update

❌ Base branch update has failed

refusing to allow a GitHub App to create or update workflow .github/workflows/yarn-upgrade-v1main.yml without workflows permission
err-code: 5AF07

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[TheRealAmazonKendra]

Approving as long as the build succeeds.

[TheRealAmazonKendra]

Oh, it already did. Great!

[mergify]

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).