aws-ecs: Service resource named "Service" instead of "Resource"

[robwettach]

❓ General Issue

The Question

The aws-ecs BaseService construct names the L1 CfnService resource as "Service" instead of the standard "Resource" (BaseService, example Resource). This manifests in CloudFormation logical IDs (and thus CloudFormation-generated names) with an unexpected "Service" at the end. The logical ID path deduping logic helps, as a directly-constructed Service named "*Service" won't get a duplicate "Service" at the end, but any other name, or if you nest the FargateService construct within another construct and name it "Resource" you get this behavior. For example, the following code generates logical IDs: "FargateService7B4DE80D" and "ServiceService480762AE"

import cdk = require('@aws-cdk/core');
import ecs = require('@aws-cdk/aws-ecs');

class NestedService extends cdk.Construct {
  constructor(scope: cdk.Construct, id: string, props: ecs.FargateServiceProps) {
    super(scope, id);

    new ecs.FargateService(this, "Resource", props);
  }
}

export class EcsServiceStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const cluster = new ecs.Cluster(this, "Cluster");
    const taskDefinition = new ecs.FargateTaskDefinition(this, "TaskDefinition");
    taskDefinition.addContainer("Container", {
      image: ecs.ContainerImage.fromRegistry("Registry")
    });

    new ecs.FargateService(this, "Fargate", {
      cluster,
      taskDefinition
    });

    new NestedService(this, "Service", {
      cluster,
      taskDefinition
    });
  }
}

Environment

• CDK CLI Version: 1.4.0 (build 175471f)
• Module Version: 1.4.0
• OS: OSX High Sierra
• Language: TypeScript

[rix0rrr]

You're right, that is incorrect. Not sure whether we can safely change this. It will entail replacement of running Services. @SoManyHs, thoughts?

[piradeepk]

You're right, that is incorrect. Not sure whether we can safely change this. It will entail replacement of running Services. @SoManyHs, thoughts?

I'm not in favour of updating the default since replacement would impact every customer who's using it today (nested stack or not).

@rix0rrr There's already a property on the BaseServiceProps to pass in the service name. If someone wants to overwrite the current default, they could do it there, not forcing the replacement on users?

[rix0rrr]

This request is not about the service name though, but about the logical ID.

See https://docs.aws.amazon.com/cdk/latest/guide/identifiers.html and https://docs.aws.amazon.com/cdk/latest/guide/resources.html#resources_physical_names

[piradeepk]

This request is not about the service name though, but about the logical ID.

See https://docs.aws.amazon.com/cdk/latest/guide/identifiers.html and https://docs.aws.amazon.com/cdk/latest/guide/resources.html#resources_physical_names

If I'm not mistaken, the BaseService takes the ServiceName and uses it in place of the physicalName. Is this not the same thing?

https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/base/base-service.ts#L180

[rix0rrr]

Yes, but I was under the impression the issue was about this line:

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

Line 185 in 6ca23f9

this.resource = new CfnService(this, "Service", {

Where it says "Service" in all other L2 constructs that string is "Resource".

[piradeepk]

Ah I see, I don't think we can safely modify that without impacting every single user (from "Service" to "Resource"). As much as this is a bug, I believe this would be too much of a negative impact, especially for those users who don't mind the "Service" naming today. It would force replacement and that could potentially cause issues for any services that are already live today.

@rix0rrr would it make sense to create a new flag that could be something like isResource which, if set to true, would replace the name with "Resource", but would default to false which would have logic to leave it as "Service"? This would allow users to update it to "Resource" if they choose, but leave it as "Service" for those who may already have a live service?

Something like this:

const resourceName = props.isResource ? "Resource" : "Service";

this.resource = new CfnService(this, resourceName, {

[rix0rrr]

Could be. It's a little elaborate though.

What I'd like to know is: how bad is it to replace a running Service? (Keeping in mind that CloudFormation will bring up the new one before it will destroy the old one)

If it can be done without availability impact, I would be okay to take the hit.

As an example, I recall it's pretty easy to make changes to a TaskDefinition which will also cause the Service to be replaced. I half-remember even noticing it's on every TaskDefinition change. Now that kind of behavior requires an impactless update anyway, so if that's true, there's really no difference between this and a user-caused change.

But I'm looking towards your team to tell me how bad the replacement would be.

[skinny85]

Can't you override the node getter in Service to return a subclass of ConstructNode that overrides defaultChild to look for the "Service" logical ID instead of "Resource"?

[rix0rrr]

The issue is not about what defaultChild does. We have a way of overriding what defaultChild returns. The issue is about the logical IDs that get generated, which now end in "ServiceService".

[rix0rrr]

I'm actually inclined to do the rename. @pkandasamy91, objections?

[piradeepk]

I'm actually inclined to do the rename. @pkandasamy91, objections?

I think there might be more impacted than just replacing the resource, I'd like to hold off until we can test a couple different scenarios.

Couple questions that I had in mind:

1. If the logical id changes, will that impact the physical id/arn?
2. (Keeping in mind that CloudFormation will bring up the new one before it will destroy the old one) curious, how does CFN know that the two resources are the same (any idea what cfn keys off of)?
3. If there are autoscaling limitations how would that work, and if you have a separate stack dependent on a service in another stack, will this work or will it fail because it cannot delete the old resource? How likely is this scenario today?

[rix0rrr]

1. If the logical id changes, will that impact the physical id/arn?

Depends; if the service has a physical name, then doing the rename will actually fail. Otherwise, the name would be autogenerated (which it would be anyway), so it would have a semi-random suffix.

2. curious, how does CFN know that the two resources are the same (any idea what cfn keys off of)?

Yes. The logical id! Look at it this way, CloudFormation keeps a "logical ID -> physical ID" table in memory for every stack. So for example, the table could look like this:

┌──────────────┬───────────────────────────────────┐
│  logical id  │           physical name           │
├──────────────┼───────────────────────────────────┤
│MyTaskDef     │MyStack-MyTaskDef-BZCUVBKX         │
│              │                                   │
│MyService     │MyStack-MyService-UHGBUHER         │
│              │                                   │
│MyLoadBalancer│IGaveMyLoadBalancerAName           │
│              │                                   │
│              │                                   │
└──────────────┴───────────────────────────────────┘

Now, when you make a change to MyService that requires replacement, it will bring up a new service, maybe it's called MyStack-MyService-ZNVJBNDF, and once the new service has been brought up, CloudFormation will delete MyStack-MyService-UHGBUHER and replace that entry in the table with the new physical service name.

Now you'll also see why replacement of a hard-named service won't work. If the load balancer needed to be replaced, CloudFormation would first try to create a load balancer with name IGaveMyLoadBalancerAName, and that would fail because a load balancer with that name already exists.

3. I don't know how likely it is that something would depend upon an ECS Service by ARN. The autoscaling rules for the old service would be deleted, and new ones created for the new service.
   FWIW, CloudFormation will just call DeleteService.

For reference, from this page: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecs-service.html

Here are all the properties that any change to them cause the service to be replaced:

• Cluster (duh)
• EnableECSManagedTags
• LaunchType (duh)
• LoadBalancers (so adding a new LB will replace the service!)
• PlacementConstraints
• PlacementStrategies
• PlatformVersion
• PropagateTags
• Role
• SchedulingStrategy
• ServiceRegistries

[piradeepk]

@rix0rrr based on the above, I have no issue. Is this something we still want to do, and should we do it sooner rather than later?

[rix0rrr]

The safe solution is probably to do it using a feature flag: https://github.com/aws/aws-cdk-rfcs/pull/56/files

It's not a process that has gotten a lot of exercise, so talk to @eladb if you run into issues executing on the RFC.

[github-actions]

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.