From 00f4f1f145cc3fdd3f99cb857cd65aa0dd3edbd9 Mon Sep 17 00:00:00 2001 From: Pahud Hsieh Date: Thu, 17 Nov 2022 16:03:44 +0000 Subject: [PATCH 1/5] fix: avoid updating both logging and access --- .../lib/cluster-resource-handler/cluster.ts | 51 +++++++++++++++---- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts index 0177a7e21b695..85f48e67bfd4e 100644 --- a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts +++ b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts @@ -136,26 +136,55 @@ export class ClusterResourceHandler extends ResourceHandler { return this.updateClusterVersion(this.newProps.version); } - if (updates.updateLogging || updates.updateAccess) { + if (updates.updateLogging && updates.updateAccess) { + throw new Error('Cannot update logging and access at the same time'); + } + + if (updates.updateLogging) { const config: aws.EKS.UpdateClusterConfigRequest = { name: this.clusterName, logging: this.newProps.logging, }; - if (updates.updateAccess) { - // Updating the cluster with securityGroupIds and subnetIds (as specified in the warning here: - // https://awscli.amazonaws.com/v2/documentation/api/latest/reference/eks/update-cluster-config.html) - // will fail, therefore we take only the access fields explicitly - config.resourcesVpcConfig = { - endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess, - endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess, - publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs, - }; - } const updateResponse = await this.eks.updateClusterConfig(config); + return { EksUpdateId: updateResponse.update?.id }; + } + if (updates.updateAccess) { + const config: aws.EKS.UpdateClusterConfigRequest = { + name: this.clusterName, + }; + // Updating the cluster with securityGroupIds and subnetIds (as specified in the warning here: + // https://awscli.amazonaws.com/v2/documentation/api/latest/reference/eks/update-cluster-config.html) + // will fail, therefore we take only the access fields explicitly + config.resourcesVpcConfig = { + endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess, + endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess, + publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs, + }; + const updateResponse = await this.eks.updateClusterConfig(config); return { EksUpdateId: updateResponse.update?.id }; } + // if (updates.updateLogging || updates.updateAccess) { + // const config: aws.EKS.UpdateClusterConfigRequest = { + // name: this.clusterName, + // logging: this.newProps.logging, + // }; + // if (updates.updateAccess) { + // // Updating the cluster with securityGroupIds and subnetIds (as specified in the warning here: + // // https://awscli.amazonaws.com/v2/documentation/api/latest/reference/eks/update-cluster-config.html) + // // will fail, therefore we take only the access fields explicitly + // config.resourcesVpcConfig = { + // endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess, + // endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess, + // publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs, + // }; + // } + // const updateResponse = await this.eks.updateClusterConfig(config); + + // return { EksUpdateId: updateResponse.update?.id }; + // } + // no updates return; } From 633cf2964f314ff8c3866ac43d4eb0b18d46fe66 Mon Sep 17 00:00:00 2001 From: Pahud Hsieh Date: Thu, 17 Nov 2022 16:12:09 +0000 Subject: [PATCH 2/5] remove comments --- .../lib/cluster-resource-handler/cluster.ts | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts index 85f48e67bfd4e..7cdde1a500be5 100644 --- a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts +++ b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts @@ -165,26 +165,6 @@ export class ClusterResourceHandler extends ResourceHandler { return { EksUpdateId: updateResponse.update?.id }; } - // if (updates.updateLogging || updates.updateAccess) { - // const config: aws.EKS.UpdateClusterConfigRequest = { - // name: this.clusterName, - // logging: this.newProps.logging, - // }; - // if (updates.updateAccess) { - // // Updating the cluster with securityGroupIds and subnetIds (as specified in the warning here: - // // https://awscli.amazonaws.com/v2/documentation/api/latest/reference/eks/update-cluster-config.html) - // // will fail, therefore we take only the access fields explicitly - // config.resourcesVpcConfig = { - // endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess, - // endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess, - // publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs, - // }; - // } - // const updateResponse = await this.eks.updateClusterConfig(config); - - // return { EksUpdateId: updateResponse.update?.id }; - // } - // no updates return; } From db021a83dfa75c68a743293dbfb2e7faa14cc26e Mon Sep 17 00:00:00 2001 From: Pahud Date: Fri, 9 Dec 2022 06:03:26 +0000 Subject: [PATCH 3/5] update test --- .../lib/cluster-resource-handler/cluster.ts | 33 +++++++++---------- .../test/cluster-resource-provider.test.ts | 27 +++++---------- 2 files changed, 23 insertions(+), 37 deletions(-) diff --git a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts index 7cdde1a500be5..aca9bbeca2110 100644 --- a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts +++ b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts @@ -140,28 +140,25 @@ export class ClusterResourceHandler extends ResourceHandler { throw new Error('Cannot update logging and access at the same time'); } - if (updates.updateLogging) { + if (updates.updateLogging || updates.updateAccess) { const config: aws.EKS.UpdateClusterConfigRequest = { name: this.clusterName, - logging: this.newProps.logging, }; - const updateResponse = await this.eks.updateClusterConfig(config); - return { EksUpdateId: updateResponse.update?.id }; - } - - if (updates.updateAccess) { - const config: aws.EKS.UpdateClusterConfigRequest = { - name: this.clusterName, - }; - // Updating the cluster with securityGroupIds and subnetIds (as specified in the warning here: - // https://awscli.amazonaws.com/v2/documentation/api/latest/reference/eks/update-cluster-config.html) - // will fail, therefore we take only the access fields explicitly - config.resourcesVpcConfig = { - endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess, - endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess, - publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs, + if (updates.updateLogging) { + config.logging = this.newProps.logging; }; + if (updates.updateAccess) { + // Updating the cluster with securityGroupIds and subnetIds (as specified in the warning here: + // https://awscli.amazonaws.com/v2/documentation/api/latest/reference/eks/update-cluster-config.html) + // will fail, therefore we take only the access fields explicitly + config.resourcesVpcConfig = { + endpointPrivateAccess: this.newProps.resourcesVpcConfig.endpointPrivateAccess, + endpointPublicAccess: this.newProps.resourcesVpcConfig.endpointPublicAccess, + publicAccessCidrs: this.newProps.resourcesVpcConfig.publicAccessCidrs, + }; + } const updateResponse = await this.eks.updateClusterConfig(config); + return { EksUpdateId: updateResponse.update?.id }; } @@ -344,4 +341,4 @@ function analyzeUpdate(oldProps: Partial, newProps function setsEqual(first: Set, second: Set) { return first.size === second.size || [...first].every((e: string) => second.has(e)); -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts b/packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts index 5bec15aaed2d8..d281362de9a39 100644 --- a/packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts +++ b/packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts @@ -589,25 +589,14 @@ describe('cluster resource provider', () => { resourcesVpcConfig: undefined, })); - const resp = await handler.onEvent(); - expect(resp).toEqual({ EksUpdateId: mocks.MOCK_UPDATE_STATUS_ID }); - expect(mocks.actualRequest.updateClusterConfigRequest!).toEqual({ - name: 'physical-resource-id', - logging: { - clusterLogging: [ - { - types: ['api', 'audit', 'authenticator', 'controllerManager', 'scheduler'], - enabled: true, - }, - ], - }, - resourcesVpcConfig: { - endpointPrivateAccess: true, - endpointPublicAccess: true, - publicAccessCidrs: ['0.0.0.0/0'], - }, - }); - expect(mocks.actualRequest.createClusterRequest).toEqual(undefined); + let error: any; + try { + await handler.onEvent(); + } catch (e) { + error = e; + } + + expect(error.message).toEqual('Cannot update logging and access at the same time'); }); }); }); From e3adc4bee2f53ece6a3146b0bf43f03ac973e33d Mon Sep 17 00:00:00 2001 From: Pahud Hsieh Date: Tue, 13 Dec 2022 23:02:15 +0000 Subject: [PATCH 4/5] add tests --- .../lib/cluster-resource-handler/cluster.ts | 2 +- .../test/cluster-resource-provider.test.ts | 106 +++++++++++++++++- 2 files changed, 105 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts index aca9bbeca2110..6ac87f2d4a76a 100644 --- a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts +++ b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts @@ -340,5 +340,5 @@ function analyzeUpdate(oldProps: Partial, newProps } function setsEqual(first: Set, second: Set) { - return first.size === second.size || [...first].every((e: string) => second.has(e)); + return [...first].every((e: string) => second.has(e)); } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts b/packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts index d281362de9a39..f1ad11b562838 100644 --- a/packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts +++ b/packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts @@ -588,16 +588,118 @@ describe('cluster resource provider', () => { logging: undefined, resourcesVpcConfig: undefined, })); - let error: any; try { await handler.onEvent(); } catch (e) { error = e; } - expect(error.message).toEqual('Cannot update logging and access at the same time'); }); + test('both logging and access defined and modify both of them', async () => { + const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', { + logging: { + clusterLogging: [ + { + types: ['api', 'audit', 'authenticator', 'controllerManager', 'scheduler'], + enabled: true, + }, + ], + }, + resourcesVpcConfig: { + endpointPrivateAccess: true, + endpointPublicAccess: false, + publicAccessCidrs: ['0.0.0.0/0'], + }, + }, { + logging: { + clusterLogging: [ + { + types: ['api', 'audit', 'authenticator', 'controllerManager'], + enabled: true, + }, + ], + }, + resourcesVpcConfig: { + endpointPrivateAccess: true, + endpointPublicAccess: true, + publicAccessCidrs: ['0.0.0.0/0'], + }, + })); + let error: any; + try { + await handler.onEvent(); + } catch (e) { + error = e; + } + expect(error.message).toEqual('Cannot update logging and access at the same time'); + }); + test('Given logging enabled and unchanged, updating the only publicAccessCidrs is allowed ', async () => { + const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', { + logging: { + clusterLogging: [ + { + types: ['api', 'audit', 'authenticator', 'controllerManager', 'scheduler'], + enabled: true, + }, + ], + }, + resourcesVpcConfig: { + endpointPrivateAccess: true, + endpointPublicAccess: true, + publicAccessCidrs: ['1.2.3.4/32'], + }, + }, { + logging: { + clusterLogging: [ + { + types: ['api', 'audit', 'authenticator', 'controllerManager', 'scheduler'], + enabled: true, + }, + ], + }, + resourcesVpcConfig: { + endpointPrivateAccess: true, + endpointPublicAccess: true, + publicAccessCidrs: ['0.0.0.0/0'], + }, + })); + const resp = await handler.onEvent(); + expect(resp).toEqual({ EksUpdateId: 'MockEksUpdateStatusId' }); + }); + test('Given logging enabled and unchanged, updating publicAccessCidrs from one to multiple entries is allowed ', async () => { + const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', { + logging: { + clusterLogging: [ + { + types: ['api', 'audit', 'authenticator', 'controllerManager', 'scheduler'], + enabled: true, + }, + ], + }, + resourcesVpcConfig: { + endpointPrivateAccess: true, + endpointPublicAccess: true, + publicAccessCidrs: ['2.4.6.0/24', '1.2.3.4/32', '3.3.3.3/32'], + }, + }, { + logging: { + clusterLogging: [ + { + types: ['api', 'audit', 'authenticator', 'controllerManager', 'scheduler'], + enabled: true, + }, + ], + }, + resourcesVpcConfig: { + endpointPrivateAccess: true, + endpointPublicAccess: true, + publicAccessCidrs: ['2.4.6.0/24'], + }, + })); + const resp = await handler.onEvent(); + expect(resp).toEqual({ EksUpdateId: 'MockEksUpdateStatusId' }); + }); }); }); }); From 6f18a71fc1b4980dcf22bc518df4c79ebd4994d9 Mon Sep 17 00:00:00 2001 From: Pahud Hsieh Date: Tue, 13 Dec 2022 18:34:36 -0500 Subject: [PATCH 5/5] minor update --- .../@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts index 6ac87f2d4a76a..e6930b7844d32 100644 --- a/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts +++ b/packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts @@ -340,5 +340,5 @@ function analyzeUpdate(oldProps: Partial, newProps } function setsEqual(first: Set, second: Set) { - return [...first].every((e: string) => second.has(e)); -} \ No newline at end of file + return first.size === second.size && [...first].every((e: string) => second.has(e)); +}