Skip to content

Commit

Permalink
fix(ecs): fromServiceArnWithCluster not accepting value from SSM Para…
Browse files Browse the repository at this point in the history
…meter string (aws#30902)

### Issue # (if applicable)

Closes aws#30798.

### Reason for this change

`fromServiceArnWithCluster()` function can't handle token value correctly. 

### Description of changes
Replace
```
const resourceName = arn.resourceName; 
```
With
```
Arn.extractResourceName()
```
because the function above has the ability to handle token values.

### Description of how you validated changes

- Updated the unit test 
- Manually verified the ECS cluster resource was used in the code pipeline when using the SSM parameter.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
xazhao authored and hemige committed Jul 25, 2024
1 parent d93a773 commit b27d968
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 10 deletions.
24 changes: 15 additions & 9 deletions packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
ArnFormat,
FeatureFlags,
Token,
Arn,
Fn,
} from '../../../core';
import * as cxapi from '../../../cx-api';
import { RegionInfo } from '../../../region-info';
Expand Down Expand Up @@ -516,16 +518,20 @@ export abstract class BaseService extends Resource
public static fromServiceArnWithCluster(scope: Construct, id: string, serviceArn: string): IBaseService {
const stack = Stack.of(scope);
const arn = stack.splitArn(serviceArn, ArnFormat.SLASH_RESOURCE_NAME);
const resourceName = arn.resourceName;
if (!resourceName) {
throw new Error(`Missing resource Name from service ARN: ${serviceArn}`);
}
const resourceNameParts = resourceName.split('/');
if (resourceNameParts.length !== 2) {
throw new Error(`resource name ${resourceName} from service ARN: ${serviceArn} is not using the ARN cluster format`);
const resourceName = Arn.extractResourceName(serviceArn, 'service');
let clusterName: string;
let serviceName: string;
if (Token.isUnresolved(resourceName)) {
clusterName = Fn.select(0, Fn.split('/', resourceName));
serviceName = Fn.select(1, Fn.split('/', resourceName));
} else {
const resourceNameParts = resourceName.split('/');
if (resourceNameParts.length !== 2) {
throw new Error(`resource name ${resourceName} from service ARN: ${serviceArn} is not using the ARN cluster format`);
}
clusterName = resourceNameParts[0];
serviceName = resourceNameParts[1];
}
const clusterName = resourceNameParts[0];
const serviceName = resourceNameParts[1];

const clusterArn = Stack.of(scope).formatArn({
partition: arn.partition,
Expand Down
7 changes: 6 additions & 1 deletion packages/aws-cdk-lib/aws-ecs/test/base-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('When import an ECS Service', () => {
test('throws an expection if no resourceName provided on fromServiceArnWithCluster', () => {
expect(() => {
ecs.BaseService.fromServiceArnWithCluster(stack, 'Service', 'arn:aws:ecs:service-region:service-account:service');
}).toThrowError(/Missing resource Name from service ARN/);
}).toThrowError(/Expected resource name in ARN, didn't find one: 'arn:aws:ecs:service-region:service-account:service'/);
});

test('throws an expection if not using cluster arn format on fromServiceArnWithCluster', () => {
Expand All @@ -47,6 +47,11 @@ describe('When import an ECS Service', () => {
}).toThrowError(/is not using the ARN cluster format/);
});

test('skip validation for tokenized values', () => {
expect(() => ecs.BaseService.fromServiceArnWithCluster(stack, 'Service',
cdk.Lazy.string({ produce: () => 'arn:aws:ecs:service-region:service-account:service' }))).not.toThrow();
});

test('should add a dependency on task role', () => {
// GIVEN
const vpc = new ec2.Vpc(stack, 'Vpc');
Expand Down

0 comments on commit b27d968

Please sign in to comment.