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(ecs): new arn format not supported (under feature flag) #18140

Merged
merged 22 commits into from
Jul 28, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3223be3
fix(ecs): new ARN format not recognized
jumic Dec 22, 2021
f5a3560
refactor duplicate code, add URL
jumic Dec 27, 2021
b5c9bf5
refactor from*ServiceArn methods
jumic Dec 27, 2021
a534380
Merge branch 'master' into ecs-new-arn-format
jumic Jan 25, 2022
dc8bcff
Merge branch 'master' into ecs-new-arn-format
madeline-k Mar 25, 2022
30546e6
switch fromServiceAttributes to use Construct from core. All v1 libra…
madeline-k Mar 25, 2022
010502e
Merge branch 'master' into ecs-new-arn-format
madeline-k Mar 31, 2022
2a2a047
Merge branch 'master' into ecs-new-arn-format
jumic Apr 24, 2022
728a094
add readme
jumic Apr 24, 2022
8f67295
use Construct from core
jumic Apr 24, 2022
5b145a7
Revert "use Construct from core"
jumic Apr 24, 2022
6a05966
use Construct from constructs
jumic Apr 24, 2022
dd295d8
Merge branch 'main' into ecs-new-arn-format
TheRealAmazonKendra Jun 13, 2022
92136f2
fix typo
TheRealAmazonKendra Jun 13, 2022
4db80e2
typo fix
TheRealAmazonKendra Jun 13, 2022
66b087f
Merge branch 'main' into ecs-new-arn-format
jumic Jun 21, 2022
c3097a7
Merge branch 'main' into ecs-new-arn-format
jumic Jun 21, 2022
afe21d9
Merge branch 'ecs-new-arn-format' of github.com:jumic/aws-cdk into ec…
jumic Jun 21, 2022
9caa1a5
Merge branch 'main' into ecs-new-arn-format
jumic Jul 25, 2022
a22ef04
Merge branch 'ecs-new-arn-format' of github.com:jumic/aws-cdk into ec…
jumic Jul 27, 2022
764d034
feature flag issue workaround
jumic Jul 27, 2022
f88e7fb
Merge branch 'main' into ecs-new-arn-format
mergify[bot] Jul 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions packages/@aws-cdk/aws-ecs/lib/base/from-service-attributes.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { ArnFormat, Resource, Stack } from '@aws-cdk/core';
import { ArnFormat, FeatureFlags, Fn, Resource, Stack, Token } from '@aws-cdk/core';
import { ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME } from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { IBaseService } from '../base/base-service';
import { ICluster } from '../cluster';

// v2 - keep this import as a separate section to reduce merge conflict when forward merging with the v2 branch.
// eslint-disable-next-line
import { Construct as CoreConstruct } from '@aws-cdk/core';

/**
* The properties to import from the service.
*/
Expand Down Expand Up @@ -32,22 +37,38 @@ export function fromServiceAtrributes(scope: Construct, id: string, attrs: Servi
throw new Error('You can only specify either serviceArn or serviceName.');
}

const featureConstruct = new CoreConstruct(scope, randomString());
const newArnFormat = FeatureFlags.of(featureConstruct).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you create a new construct to check the feature flags on here? Shouldn't it be possible to just do this?

Suggested change
const newArnFormat = FeatureFlags.of(featureConstruct).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME);
const newArnFormat = FeatureFlags.of(scope).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME);

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 tried FeatureFlags.of(scope) first because it should be the preferred solution. Unfortunately, it causes this error:

Argument of type 'import("/Users/julian/dev/aws-cdk/node_modules/constructs/lib/construct").Construct' is not assignable to parameter of type 'import("/Users/julian/dev/aws-cdk/packages/@aws-cdk/core/lib/construct-compat").Construct'.
Type 'Construct' is missing the following properties from type 'Construct': node, validate, prepare, synthesizets(2345)

Copy link
Contributor

Choose a reason for hiding this comment

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

Since FeatureFlags expects a Construct from the core library, I think we can just switch to using that one in the fromServiceAttributes() function. I pushed this change to your branch, waiting for the build to see if this has other impacts.


const stack = Stack.of(scope);
let name: string;
let arn: string;
if (attrs.serviceName) {
name = attrs.serviceName as string;
const resourceName = newArnFormat ? `${attrs.cluster.clusterName}/${attrs.serviceName}` : attrs.serviceName as string;
arn = stack.formatArn({
partition: stack.partition,
service: 'ecs',
region: stack.region,
account: stack.account,
resource: 'service',
resourceName: name,
resourceName,
});
} else {
arn = attrs.serviceArn as string;
name = stack.splitArn(arn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
if (Token.isUnresolved(arn)) {
if (newArnFormat) {
const components = Fn.split(':', arn);
const lastComponents = Fn.split('/', Fn.select(5, components));
name = Fn.select(2, lastComponents);
} else {
name = stack.splitArn(arn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
}
} else {
const resourceName = stack.splitArn(arn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
const resourceNameSplit = resourceName.split('/');
name = resourceNameSplit.length === 1 ? resourceName : resourceNameSplit[1];
}
}
class Import extends Resource implements IBaseService {
public readonly serviceArn = arn;
Expand All @@ -58,3 +79,8 @@ export function fromServiceAtrributes(scope: Construct, id: string, attrs: Servi
environmentFromArn: arn,
});
}

function randomString() {
// Crazy
return Math.random().toString(36).replace(/[^a-z0-9]+/g, '');
}
23 changes: 20 additions & 3 deletions packages/@aws-cdk/aws-ecs/lib/ec2/ec2-service.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as ec2 from '@aws-cdk/aws-ec2';
import { ArnFormat, Lazy, Resource, Stack } from '@aws-cdk/core';
import { ArnFormat, FeatureFlags, Fn, Lazy, Resource, Stack, Token } from '@aws-cdk/core';
import { ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME } from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { BaseService, BaseServiceOptions, DeploymentControllerType, IBaseService, IService, LaunchType } from '../base/base-service';
import { fromServiceAtrributes } from '../base/from-service-attributes';
Expand Down Expand Up @@ -128,9 +129,25 @@ export class Ec2Service extends BaseService implements IEc2Service {
public static fromEc2ServiceArn(scope: Construct, id: string, ec2ServiceArn: string): IEc2Service {
class Import extends Resource implements IEc2Service {
public readonly serviceArn = ec2ServiceArn;
public readonly serviceName = Stack.of(scope).splitArn(ec2ServiceArn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
public readonly serviceName: string;
constructor() {
super(scope, id);
const resourceName = Stack.of(scope).splitArn(ec2ServiceArn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
if (Token.isUnresolved(ec2ServiceArn)) {
if (FeatureFlags.of(this).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME)) {
const components = Fn.split(':', ec2ServiceArn);
const lastComponents = Fn.split('/', Fn.select(5, components));
this.serviceName = Fn.select(2, lastComponents);
} else {
this.serviceName = resourceName;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is repeated (or at least extremely similar) here, in fargate-service.ts and in from-service-attributes.ts. We should factor this out to one place. It could probably be shared from from-service-attributes.ts.

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 moved the code to a new function extractServiceNameFromArn in from-service-attributes.ts.

} else {
const resourceNameSplit = resourceName.split('/');
this.serviceName = resourceNameSplit.length === 1 ? resourceName : resourceNameSplit[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be factored out to one place as well.

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 moved the code to a new function extractServiceNameFromArn in from-service-attributes.ts.

}
}
}
return new Import(scope, id);
return new Import();
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something here, but I don't understand this. Can you explain why is this changing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, passing the parameters was necessary because they were passed to the super class. The new implementation contains a constructor. In this case, the super() call has to be implemented in the constructor. Therefore, the parameters are not necessary anymore.

If you prefer the existing code (return new Import(scope, id);), I could implement it too. For example:

  constructor(importScope: Construct, importId: string) {
    super(importScope, importId);
    // ...
  }
}
return new Import(scope, id);

However, we have to use new attribute names. If I use scope instead of importScope, the following error is displayed:'scope' is already declared in the upper scope.eslint@typescript-eslint/no-shadow.

Which option do you prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No constructor is used anymore --> done.

}

/**
Expand Down
23 changes: 20 additions & 3 deletions packages/@aws-cdk/aws-ecs/lib/fargate/fargate-service.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as ec2 from '@aws-cdk/aws-ec2';
import * as cdk from '@aws-cdk/core';
import { ArnFormat } from '@aws-cdk/core';
import { ArnFormat, FeatureFlags, Fn, Token } from '@aws-cdk/core';
import { ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME } from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { BaseService, BaseServiceOptions, DeploymentControllerType, IBaseService, IService, LaunchType } from '../base/base-service';
import { fromServiceAtrributes } from '../base/from-service-attributes';
Expand Down Expand Up @@ -105,9 +106,25 @@ export class FargateService extends BaseService implements IFargateService {
public static fromFargateServiceArn(scope: Construct, id: string, fargateServiceArn: string): IFargateService {
class Import extends cdk.Resource implements IFargateService {
public readonly serviceArn = fargateServiceArn;
public readonly serviceName = cdk.Stack.of(scope).splitArn(fargateServiceArn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
public readonly serviceName: string;
constructor() {
super(scope, id);
const resourceName = cdk.Stack.of(scope).splitArn(fargateServiceArn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
if (Token.isUnresolved(fargateServiceArn)) {
if (FeatureFlags.of(this).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME)) {
const components = Fn.split(':', fargateServiceArn);
const lastComponents = Fn.split('/', Fn.select(5, components));
this.serviceName = Fn.select(2, lastComponents);
} else {
this.serviceName = resourceName;
}
} else {
const resourceNameSplit = resourceName.split('/');
this.serviceName = resourceNameSplit.length === 1 ? resourceName : resourceNameSplit[1];
}
}
}
return new Import(scope, id);
return new Import();
}

/**
Expand Down
Loading