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

fix(eks): EKS tags do not update if edited after initial deploy #23793

Closed
wants to merge 13 commits into from
40 changes: 40 additions & 0 deletions packages/@aws-cdk/aws-eks/lib/cluster-resource-handler/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,44 @@ export class ClusterResourceHandler extends ResourceHandler {
return { EksUpdateId: updateResponse.update?.id };
}

if (updates.updateTags) {
console.log(`updating cluster tags to ${JSON.stringify(this.newProps.tags)}`);

const resp = await this.eks.describeCluster({
name: this.clusterName,
});

if (!resp.cluster?.arn) {
throw new Error(`Cannot obtain cluster ARN with cluster name ${this.clusterName}`);
}

const { oldTags, newTags } = {
oldTags: this.oldProps.tags ?? {},
newTags: this.newProps.tags ?? {},
};
const { oldKeys, newKeys } = {
oldKeys: Object.keys(oldTags),
newKeys: Object.keys(newTags),
};

const removeKeys = oldKeys.filter((v) => !newKeys.includes(v));
if (removeKeys.length) {
await this.eks.untagResource({
resourceArn: resp.cluster.arn,
tagKeys: removeKeys,
});
}

if (newKeys.length) {
await this.eks.tagResource({
resourceArn: resp.cluster.arn,
tags: newTags,
});
}

return;
}

// no updates
return;
}
Expand Down Expand Up @@ -309,6 +347,7 @@ interface UpdateMap {
updateLogging: boolean; // logging
updateEncryption: boolean; // encryption (cannot be updated)
updateAccess: boolean; // resourcesVpcConfig.endpointPrivateAccess and endpointPublicAccess
updateTags: boolean; // tags
}

function analyzeUpdate(oldProps: Partial<aws.EKS.CreateClusterRequest>, newProps: aws.EKS.CreateClusterRequest): UpdateMap {
Expand Down Expand Up @@ -336,6 +375,7 @@ function analyzeUpdate(oldProps: Partial<aws.EKS.CreateClusterRequest>, newProps
updateVersion: newProps.version !== oldProps.version,
updateEncryption: JSON.stringify(newEnc) !== JSON.stringify(oldEnc),
updateLogging: JSON.stringify(newProps.logging) !== JSON.stringify(oldProps.logging),
updateTags: JSON.stringify(newProps.tags) !== JSON.stringify(oldProps.tags),
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,6 @@ export interface EksClient {
createFargateProfile(request: aws.EKS.CreateFargateProfileRequest): Promise<aws.EKS.CreateFargateProfileResponse>;
describeFargateProfile(request: aws.EKS.DescribeFargateProfileRequest): Promise<aws.EKS.DescribeFargateProfileResponse>;
deleteFargateProfile(request: aws.EKS.DeleteFargateProfileRequest): Promise<aws.EKS.DeleteFargateProfileResponse>;
untagResource(req: aws.EKS.UntagResourceRequest): Promise<aws.EKS.UntagResourceResponse>;
tagResource(req: aws.EKS.TagResourceRequest): Promise<aws.EKS.TagResourceResponse>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const defaultEksClient: EksClient = {
createFargateProfile: req => getEksClient().createFargateProfile(req).promise(),
deleteFargateProfile: req => getEksClient().deleteFargateProfile(req).promise(),
describeFargateProfile: req => getEksClient().describeFargateProfile(req).promise(),
untagResource: req => getEksClient().untagResource(req).promise(),
tagResource: req => getEksClient().tagResource(req).promise(),
configureAssumeRole: req => {
console.log(JSON.stringify({ assumeRole: req }, undefined, 2));
const creds = new aws.ChainableTemporaryCredentials({
Expand Down
28 changes: 21 additions & 7 deletions packages/@aws-cdk/aws-eks/test/cluster-resource-handler-mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ export let actualRequest: {
createFargateProfile?: sdk.EKS.CreateFargateProfileRequest;
describeFargateProfile?: sdk.EKS.DescribeFargateProfileRequest;
deleteFargateProfile?: sdk.EKS.DeleteFargateProfileRequest;
} = { };
untagResourceRequest?: sdk.EKS.UntagResourceRequest;
tagResourceRequest?: sdk.EKS.TagResourceRequest;
} = {};

/**
* Responses can be simulated by assigning values here.
Expand All @@ -27,11 +29,11 @@ export let simulateResponse: {
describeUpdateResponseMockErrors?: sdk.EKS.ErrorDetails;
deleteClusterErrorCode?: string;
describeClusterExceptionCode?: string;
} = { };
} = {};

export function reset() {
actualRequest = { };
simulateResponse = { };
actualRequest = {};
simulateResponse = {};
}

export const MOCK_UPDATE_STATUS_ID = 'MockEksUpdateStatusId';
Expand Down Expand Up @@ -124,18 +126,30 @@ export const client: EksClient = {

createFargateProfile: async req => {
actualRequest.createFargateProfile = req;
return { };
return {};
},

describeFargateProfile: async req => {
actualRequest.describeFargateProfile = req;
return { };
return {};
},

deleteFargateProfile: async req => {
actualRequest.deleteFargateProfile = req;
return { };
return {};
},

untagResource: async req => {
actualRequest.untagResourceRequest = req;
return {};
},

tagResource: async req => {
actualRequest.tagResourceRequest = req;
return {};
},


};

export const MOCK_PROPS = {
Expand Down
196 changes: 196 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 @@ -701,6 +701,202 @@ describe('cluster resource provider', () => {
expect(resp).toEqual({ EksUpdateId: 'MockEksUpdateStatusId' });
});
});

describe('tag change', () => {
test('add tags', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
tags: {
new: 'one',
another: 'two',
},
}, {
tags: undefined,
}));

const resp = await handler.onEvent();
expect(resp).toEqual(undefined);
expect(mocks.actualRequest.untagResourceRequest).toEqual(undefined);
expect(mocks.actualRequest.tagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tags: {
new: 'one',
another: 'two',
},
});
expect(mocks.actualRequest.createClusterRequest).toEqual(undefined);
});

test('remove all tags', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
tags: undefined,
}, {
tags: {
new: 'one',
another: 'two',
},
}));

const resp = await handler.onEvent();
expect(resp).toEqual(undefined);
expect(mocks.actualRequest.untagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tagKeys: ['new', 'another'],
});
expect(mocks.actualRequest.tagResourceRequest).toEqual(undefined);
expect(mocks.actualRequest.createClusterRequest).toEqual(undefined);
});

test('add tags to existing tags', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
tags: {
'new': 'one',
'another': 'two',
'the-other': 'three',
'latest': 'four',
},
}, {
tags: {
new: 'one',
another: 'two',
},
}));

const resp = await handler.onEvent();
expect(resp).toEqual(undefined);
expect(mocks.actualRequest.untagResourceRequest).toEqual(undefined);
expect(mocks.actualRequest.tagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tags: {
'new': 'one',
'another': 'two',
'the-other': 'three',
'latest': 'four',
},
});
expect(mocks.actualRequest.createClusterRequest).toEqual(undefined);
});

test('remove some tags', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
tags: {
'the-other': 'three',
'latest': 'four',
},
}, {
tags: {
'new': 'one',
'another': 'two',
'the-other': 'three',
'latest': 'four',
},
}));

const resp = await handler.onEvent();
expect(resp).toEqual(undefined);
expect(mocks.actualRequest.untagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tagKeys: ['new', 'another'],
});
expect(mocks.actualRequest.tagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tags: {
'the-other': 'three',
'latest': 'four',
},
});
expect(mocks.actualRequest.createClusterRequest).toEqual(undefined);
});

test('update tag value', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
tags: {
new: 'three',
another: 'two',
},
}, {
tags: {
new: 'one',
another: 'two',
},
}));

const resp = await handler.onEvent();
expect(resp).toEqual(undefined);
expect(mocks.actualRequest.untagResourceRequest).toEqual(undefined);
expect(mocks.actualRequest.tagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tags: {
new: 'three',
another: 'two',
},
});
expect(mocks.actualRequest.createClusterRequest).toEqual(undefined);
});

test('update tag key', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
tags: {
new: 'one',
another: 'two',
},
}, {
tags: {
old: 'one',
another: 'two',
},
}));

const resp = await handler.onEvent();
expect(resp).toEqual(undefined);
expect(mocks.actualRequest.untagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tagKeys: ['old'],
});
expect(mocks.actualRequest.tagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tags: {
new: 'one',
another: 'two',
},
});
expect(mocks.actualRequest.createClusterRequest).toEqual(undefined);
});

test('mixed', async () => {
const handler = new ClusterResourceHandler(mocks.client, mocks.newRequest('Update', {
tags: {
another: 'five',
the_other: 'three',
latest: 'four',
keep: 'same',
},
}, {
tags: {
'new': 'one',
'another': 'two',
'the-other': 'three',
'keep': 'same',
},
}));

const resp = await handler.onEvent();
expect(resp).toEqual(undefined);
expect(mocks.actualRequest.untagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tagKeys: ['new', 'the-other'],
});
expect(mocks.actualRequest.tagResourceRequest).toEqual({
resourceArn: 'arn:cluster-arn',
tags: {
another: 'five',
the_other: 'three',
latest: 'four',
keep: 'same',
},
});
expect(mocks.actualRequest.createClusterRequest).toEqual(undefined);
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ describe('fargate resource provider', () => {
});
});

function newRequestMock(props: any = { }): any {
function newRequestMock(props: any = {}): any {
return {
RequestType: 'Create',
ServiceToken: 'ServiceTokenMock',
Expand Down Expand Up @@ -282,5 +282,7 @@ function newEksClientMock() {
}),
deleteFargateProfile: sinon.fake(),
describeFargateProfile: sinon.fake.throws('not implemented'),
untagResource: sinon.fake.throws('not implemented'),
tagResource: sinon.fake.throws('not implemented'),
};
}
}