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

(ecs): defer the Fargate/Ec2Service essential container exception to the validation phase #13239

Closed
1 of 2 tasks
misterjoshua opened this issue Feb 23, 2021 · 1 comment · Fixed by #13240
Closed
1 of 2 tasks
Assignees
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container feature-request A feature should be added or improved.

Comments

@misterjoshua
Copy link
Contributor

misterjoshua commented Feb 23, 2021

I'd like FargateService and Ec2Service to defer throwing their A TaskDefinition must have at least one essential container exceptions until the validate phase.

Use Case

When I'm creating an ECS service with a container that needs to discover its siblings in other tasks via service discovery, I may need to get the service's cloudmap service and add that to an environment variable on the default container.

There's a problem here because the container must exist before the service can exist or we get the exception, but the container needs to know the service discovery service after the service has been created.

Right now, I can use a Lazy.string to set the environment variable later, but the way to do this is cumbersome.

Proposed Solution

If the services defer this validation error to the validate phase, we can still catch the user's mistake, but while also adding the ability to create the default container after the service exists. I can appreciate that one project philosophy is to validate as soon as possible, but I think this exception is thrown too soon.

Here's what I'd like to be able to do:

const taskDefinition = new ecs.FargateTaskDefinition(stack, 'FargateTaskDef');
// Currently, this part throws immediately:
const service = new ecs.FargateService(stack, 'Service', {
  cluster,
  taskDefinition,
  cloudMapOptions: {},
});

const container = taskDefinition.addContainer('MainContainer', {
  image: ecs.ContainerImage.fromRegistry('hello'),
  environment: {
    SERVICE_DISCOVERY: cdk.Fn.join('.', [
      service.cloudMapService!.serviceName,
      service.cloudMapService!.namespace.namespaceName,
    ]),
  },
});
container.addPortMappings({ containerPort: 80 });

I'd be fine if I needed to set a feature flag in the parent scope to defer the error, but I'm not sure that is necessary as it seems the intent of the exception is to catch a user that has misconfigured their task definition, which still happens in the case of this deferral.

Other

Here's how I currently work around this limitation:

// [1] Set this later when we have a service available
let serviceDiscoveryLazy: string | undefined;

const taskDefinition = new ecs.FargateTaskDefinition(stack, 'TD');
const mainContainer = taskDefinition.addContainer('main', {
  image: ecs.ContainerImage.fromRegistry('something'),
  essential: true,
  environment: {
    // [2] We set serviceDiscoveryLazy after we create the service
    SERVICE_DISCOVERY: cdk.Lazy.string({ produce: () => serviceDiscoveryLazy }),
  },
});
mainContainer.addPortMappings({ containerPort: 80 });

const service = new ecs.FargateService(stack, 'Service', {
  cluster,
  taskDefinition,
  // Enables cloudmap service with all-defaults
  cloudMapOptions: {},
});

// [3] Set the lazy value used above so the lazy string can evaluate.
serviceDiscoveryLazy = cdk.Fn.join('.', [
  service.cloudMapService!.serviceName,
  service.cloudMapService!.namespace.namespaceName,
]);

Ideally, I'd like to add the default container after the service has been created without needing to use this kind of lazy evaluation.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@misterjoshua misterjoshua added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Feb 23, 2021
@misterjoshua misterjoshua changed the title (ecs): move Fargate/Ec2Service one essential container exception to onValidate() (ecs): move Fargate/Ec2Service essential container exception to onValidate() Feb 23, 2021
@misterjoshua misterjoshua changed the title (ecs): move Fargate/Ec2Service essential container exception to onValidate() (ecs): defer the Fargate/Ec2Service essential container exception to the validation phase Feb 23, 2021
@peterwoodworth peterwoodworth added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Feb 24, 2021
@mergify mergify bot closed this as completed in #13240 Feb 26, 2021
mergify bot pushed a commit that referenced this issue Feb 26, 2021
…3240)

This PR defers the `A TaskDefinition must have at least one essential container`
error to the validate phase.

Closes #13239

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@SoManyHs SoManyHs removed the needs-triage This issue or PR still needs to be triaged. label Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants