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.

Sign up for GitHub

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

Jump to bottom

fix(aws-eks): fail to update both logging and access at the same time #22957

Merged
merged 8 commits into from Dec 20, 2022
Merged

fix(aws-eks): fail to update both logging and access at the same time #22957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
00f4f1f
fix: avoid updating both logging and access
pahud Nov 17, 2022
633cf29
remove comments
pahud Nov 17, 2022
6cdbf52
Merge branch 'main' into pahud/aws-eks-cannot-update-cluster-21439
pahud Dec 8, 2022
db021a8
update test
pahud Dec 9, 2022
e3adc4b
add tests
pahud Dec 13, 2022
c1b5ea9
Merge branch 'main' into pahud/aws-eks-cannot-update-cluster-21439
pahud Dec 13, 2022
6f18a71
minor update
pahud Dec 13, 2022
452d01b
Merge branch 'main' into pahud/aws-eks-cannot-update-cluster-21439
mergify[bot] Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts
View file
Open in desktop
Expand Up @@ -136,10 +136,16 @@ export class ClusterResourceHandler extends ResourceHandler {
return this.updateClusterVersion(this.newProps.version);
}

if (updates.updateLogging && updates.updateAccess) {
Copy link

Choose a reason for hiding this comment

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

the EKS API does not allow both to be updated at the same time, but if we make 2 separate API calls, do we need to set this restriction on the custom resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if this is a good idea to have a custom resource making multiple API calls. Would love to hear comments from @iliapolo or any maintainers before we move this PR further.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have precedents for this already in other custom resources. Some examples involving multiple write calls:

  • In S3, for automatic deletion of objects, we call deleteObjects multiple times.
  • In IAM, for the OIDC provider, we call many IAM methods, such as updateOpenIDConnectProviderThumbprint, addClientIDToOpenIDConnectProvider and removeClientIDFromOpenIDConnectProvider.
  • In Logs, for log retention, we call createLogGroup, deleteRetentionPolicy and putRetentionPolicy.

And, if we also consider read calls, pretty much every custom resource makes a combination of read and write calls.

Copy link
Contributor Author

@pahud pahud Dec 8, 2022 •
edited

Choose a reason for hiding this comment

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

Given that, I agree we should make multiple API calls in this custom resource but I probably don't have capacity for that in the next few weeks. I will make this PR as is for immediate fix and leave the multiple API calls implementation for another PR if anyone is interested to move it forward.

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 @@ -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));
}
105 changes: 98 additions & 7 deletions packages/@aws-cdk/aws-eks/test/cluster-resource-provider.test.ts
View file
Open in desktop
Expand Up @@ -588,11 +588,16 @@ describe('cluster resource provider', () => {
logging: undefined,
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',
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: [
{
Expand All @@ -601,13 +606,99 @@ describe('cluster resource provider', () => {
},
],
},
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'],
},
});
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');
});
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' });
});
});
});
Expand Down