Skip to content

Commit

Permalink
fix(eks): changing the subnets or securityGroupIds order causes an er…
Browse files Browse the repository at this point in the history
…ror (#24163)

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](#24162)

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
  • Loading branch information
AviorSchreiber committed Feb 22, 2023
1 parent 1e1131c commit 09c2c19
Show file tree
Hide file tree
Showing 42 changed files with 449 additions and 482 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,8 @@ function analyzeUpdate(oldProps: Partial<aws.EKS.CreateClusterRequest>, newProps
return {
replaceName: newProps.name !== oldProps.name,
replaceVpc:
JSON.stringify(newVpcProps.subnetIds) !== JSON.stringify(oldVpcProps.subnetIds) ||
JSON.stringify(newVpcProps.securityGroupIds) !== JSON.stringify(oldVpcProps.securityGroupIds),
JSON.stringify(newVpcProps.subnetIds?.sort()) !== JSON.stringify(oldVpcProps.subnetIds?.sort()) ||
JSON.stringify(newVpcProps.securityGroupIds?.sort()) !== JSON.stringify(oldVpcProps.securityGroupIds?.sort()),
updateAccess:
newVpcProps.endpointPrivateAccess !== oldVpcProps.endpointPrivateAccess ||
newVpcProps.endpointPublicAccess !== oldVpcProps.endpointPublicAccess ||
Expand Down
19 changes: 19 additions & 0 deletions packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,25 @@ describe('cluster resource provider', () => {
});
});

test('change subnets or security groups order should not trigger an update', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
...mocks.MOCK_PROPS,
resourcesVpcConfig: {
subnetIds: ['subnet1', 'subnet2'],
securityGroupIds: ['sg1', 'sg2'],
},
}, {
...mocks.MOCK_PROPS,
resourcesVpcConfig: {
subnetIds: ['subnet2', 'subnet1'],
securityGroupIds: ['sg2', 'sg1'],
},
}));
const resp = await handler.onEvent();

expect(resp).toEqual(undefined);
});

test('"roleArn" requires a replacement', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
roleArn: 'new-arn',
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,16 @@ export class ClusterResourceHandler extends ResourceHandler {
return this.updateClusterVersion(this.newProps.version);
}

if (updates.updateLogging && updates.updateAccess) {
throw new Error('Cannot update logging and access at the same time');
}

if (updates.updateLogging || updates.updateAccess) {
const config: aws.EKS.UpdateClusterConfigRequest = {
name: this.clusterName,
logging: this.newProps.logging,
};
if (updates.updateLogging) {
config.logging = this.newProps.logging;
};
if (updates.updateAccess) {
// Updating the cluster with securityGroupIds and subnetIds (as specified in the warning here:
Expand Down Expand Up @@ -320,8 +326,8 @@ function analyzeUpdate(oldProps: Partial<aws.EKS.CreateClusterRequest>, newProps
return {
replaceName: newProps.name !== oldProps.name,
replaceVpc:
JSON.stringify(newVpcProps.subnetIds) !== JSON.stringify(oldVpcProps.subnetIds) ||
JSON.stringify(newVpcProps.securityGroupIds) !== JSON.stringify(oldVpcProps.securityGroupIds),
JSON.stringify(newVpcProps.subnetIds?.sort()) !== JSON.stringify(oldVpcProps.subnetIds?.sort()) ||
JSON.stringify(newVpcProps.securityGroupIds?.sort()) !== JSON.stringify(oldVpcProps.securityGroupIds?.sort()),
updateAccess:
newVpcProps.endpointPrivateAccess !== oldVpcProps.endpointPrivateAccess ||
newVpcProps.endpointPublicAccess !== oldVpcProps.endpointPublicAccess ||
Expand All @@ -334,5 +340,5 @@ function analyzeUpdate(oldProps: Partial<aws.EKS.CreateClusterRequest>, newProps
}

function setsEqual(first: Set<string>, second: Set<string>) {
return first.size === second.size || [...first].every((e: string) => second.has(e));
return first.size === second.size && [...first].every((e: string) => second.has(e));
}
Binary file not shown.

This file was deleted.

Loading

0 comments on commit 09c2c19

Please sign in to comment.