fix(ecs): fix serviceArn token to have region/account in serviceArn #18382

tobytipton · 2022-01-12T15:19:07Z

This fixes #18246 by changing the serviceArn to token each part vs creating one token which allows for using the serviceArn to get the environment.

Fixes #18246

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-01-12T15:19:10Z

madeline-k

See my comment on #18140 (comment) as well

madeline-k · 2022-01-25T00:44:41Z

packages/@aws-cdk/aws-ecs/lib/base/from-service-attributes.ts

-    name = stack.splitArn(arn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
+    const resourceName = stack.splitArn(arn, ArnFormat.SLASH_RESOURCE_NAME).resourceName as string;
+    const resourceNameSplit = resourceName.split('/');
+    name = resourceNameSplit.length === 1 ? resourceName : resourceNameSplit[1];


This part is overlapping with the changes that @jumic is making in this PR https://github.com/aws/aws-cdk/pull/18140/files.

Let's keep this PR to only changing serviceArn public property for Ec2 and Fargate Service that are created with the BaseService constructor. And we can leave the necessary changes for the serviceArn and serviceName public props for fromXXX() services to @jumic 's PR.

Same comment for the changes in ec2-service.ts and fargate-service.ts

madeline-k · 2022-01-25T00:46:10Z

packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts

@@ -3280,24 +3307,61 @@ describe('ec2 service', () => {
  });

  describe('When import an EC2 Service', () => {
-    test('with serviceArn', () => {
+    test('with old serviceArn', () => {


Testing the new vs old service ARN format should be a part of #18140. It is out of scope for this one.

madeline-k · 2022-01-25T00:47:11Z

packages/@aws-cdk/aws-ecs/test/fargate/fargate-service.test.ts

@@ -2111,24 +2111,64 @@ describe('fargate service', () => {
  });

  describe('When import a Fargate Service', () => {
-    test('with serviceArn', () => {
+    test('with old serviceArn', () => {


These tests are also out of scope

madeline-k · 2022-01-25T00:49:05Z

packages/@aws-cdk/aws-ecs/test/ec2/ec2-service.test.ts

+      const importService = ecs.Ec2Service.fromEc2ServiceAttributes(stack, 'importService', {
+        cluster: cluster,
+        serviceArn: service.serviceArn,
+      });
+      expect(importService.serviceArn).toContain(`service/${cluster.clusterName}/${service.serviceName}`);
+      expect(importService.serviceName).toEqual(service.serviceName);


this should be it's own test once the fromXXX() issues are fixed in #18140

skinny85 · 2022-01-25T01:01:43Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

@@ -426,12 +426,12 @@ export abstract class BaseService extends Resource
      Annotations.of(this).addWarning('taskDefinition and launchType are blanked out when using external deployment controller.');
    }

-    this.serviceArn = this.getResourceArnAttribute(this.resource.ref, {
+    this.serviceName = this.getResourceNameAttribute(this.resource.attrName);
+    this.serviceArn = Stack.of(this).formatArn({


Note: this needs to keep using the getResourceArnAttribute() helper - otherwise, things will break.

Is there a better approach? The getResourceArnAttribute() generates a token which can't be parsed for region, account, clusterName or serviceName. Which means when creating the new service to get the ARN which can be leveraged to be imported you have to do this to generate the code.

The problem here is most likely the ${cluster.clusterName} part we are referencing here. We are investigating right now to confirm that 🙂.

@madeline-k did you ever conclude your investigation about that suspicious ${cluster.clusterName} expression causing problems?

Pull request has been modified.

TheRealAmazonKendra · 2022-07-29T00:02:50Z

@Mergifyio update

mergify · 2022-07-29T00:03:26Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2022-07-29T00:25:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 3d82774
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

TheRealAmazonKendra

@tobytipton We'll need the build to be passing before we can provide another review of this. It looks like your update broke the tests in another package so please take a look at those.

Additionally, please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests). In a fix, the title should state the problem that is being fixed, now what the fix itself is.

aws-cdk-automation · 2022-08-20T00:08:05Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2022-08-27T00:08:54Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

fix serviceArn token to have region/account in serviceArn

Verified

This commit was signed with the committer’s verified signature.

connor4312 Connor Peet

GPG key ID: CF8FD2EA0DBC61BD

Verified
Learn about vigilant mode

Loading
Loading status checks…

ee6f038

github-actions bot added the @aws-cdk/aws-ecs label Jan 12, 2022

github-actions bot assigned madeline-k Jan 12, 2022

updated expected response based on serviceArn change

7476b26

madeline-k mentioned this pull request Jan 25, 2022

fix(ecs): new arn format not supported (under feature flag) #18140

Merged

madeline-k previously requested changes Jan 25, 2022

View reviewed changes

skinny85 reviewed Jan 25, 2022

View reviewed changes

remove tests related to parsing new ARN format

7a4bad9

rix0rrr added bug p1 and removed @aws-cdk/aws-ecs labels Mar 4, 2022

TheRealAmazonKendra changed the base branch from v1-main to main July 6, 2022 22:26

github-actions bot added the effort/small label Jul 6, 2022

Merge branch 'main' into ecsServiceArnToken

129971e

Merge branch 'main' into ecsServiceArnToken

3d82774

TheRealAmazonKendra requested changes Aug 2, 2022

View reviewed changes

aws-cdk-automation added the closed-for-staleness label Aug 27, 2022

aws-cdk-automation closed this Aug 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecs): fix serviceArn token to have region/account in serviceArn #18382

fix(ecs): fix serviceArn token to have region/account in serviceArn #18382

tobytipton commented Jan 12, 2022

gitpod-io bot commented Jan 12, 2022

madeline-k left a comment

madeline-k Jan 25, 2022

madeline-k Jan 25, 2022

madeline-k Jan 25, 2022

madeline-k Jan 25, 2022

madeline-k Jan 25, 2022

skinny85 Jan 25, 2022

tobytipton Jan 25, 2022

skinny85 Jan 25, 2022

skinny85 Aug 20, 2022

TheRealAmazonKendra commented Jul 29, 2022

mergify bot commented Jul 29, 2022

aws-cdk-automation commented Jul 29, 2022

TheRealAmazonKendra left a comment

aws-cdk-automation commented Aug 20, 2022

aws-cdk-automation commented Aug 27, 2022

fix(ecs): fix serviceArn token to have region/account in serviceArn #18382

fix(ecs): fix serviceArn token to have region/account in serviceArn #18382

Conversation

tobytipton commented Jan 12, 2022

Fixes #18246

gitpod-io bot commented Jan 12, 2022

madeline-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Jul 29, 2022

mergify bot commented Jul 29, 2022

✅ Branch has been successfully updated

aws-cdk-automation commented Jul 29, 2022

AWS CodeBuild CI Report

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 20, 2022

aws-cdk-automation commented Aug 27, 2022