Additional Fargate service #28
Conversation
There was a problem hiding this comment.
I did a read through pass. This LGTM – beyond some ignorable nits that I left throughout, my main concerns are:
- This pull seems to introduce some duplication around how services are defined and there might be an opportunity to DRY that up.
- I'm increasingly uncomfortable with the fact that templates like this and my reference architecture can't be updated safely due to the use of a seed image. I'm getting the strong urge to muscle a solution here - thoughts in the pull include using a custom resource to safely allow updates or moving partially back to a CloudFormation deploy action in the pipeline to at least create the infrastructure on first deploy.
The update issue seems like it can be handled separately and might require some more thought - but I'd spend some cycles DRY'ing up the usage of the service definitions before merging.
| @@ -291,7 +299,7 @@ Resources: | |||
| pre_build: | |||
| commands: | |||
| - printenv | |||
| - TAG="$ENVIRONMENT_NAME.$(date +%Y-%m-%d).$(echo $CODEBUILD_RESOLVED_SOURCE_VERSION | head -c 8)" | |||
| - TAG="$REPOSITORY_NAME.$REPOSITORY_BRANCH.$ENVIRONMENT_NAME.$(date +%Y-%m-%d.%H.%M.%S).$(echo $CODEBUILD_RESOLVED_SOURCE_VERSION | head -c 8)" | |||
There was a problem hiding this comment.
This is a lot of specificity for a Docker container image tag. I'm not sure what you're envisioning where this amount of detail is really required. An application will typically have a single source repository, that repository should have a single mainline branch, and aspects of the build such as the timestamp that the tag was generated seem superfluous.
There was a problem hiding this comment.
We can discuss tomorrow, but since I am not specifying a name for the ECR repository, this clears things up a bit in the console. Currently, I am not generating a new repository per application because this seems unnecessary. I am not religious about this, so we can chat about it on our call.
There was a problem hiding this comment.
I don't have a strong opinion here. Just stood out to me.
| @@ -291,7 +299,7 @@ Resources: | |||
| pre_build: | |||
| commands: | |||
| - printenv | |||
| - TAG="$ENVIRONMENT_NAME.$(date +%Y-%m-%d).$(echo $CODEBUILD_RESOLVED_SOURCE_VERSION | head -c 8)" | |||
| - TAG="$REPOSITORY_NAME.$REPOSITORY_BRANCH.$ENVIRONMENT_NAME.$(date +%Y-%m-%d.%H.%M.%S).$(echo $CODEBUILD_RESOLVED_SOURCE_VERSION | head -c 8)" | |||
There was a problem hiding this comment.
I'd also say, if you're going to concatenate like this, you may want to use variable expansion to make it easier for somebody to skim, and break out some of the variables into their own variables for easier readability
| # A CloudFormation template to deploy an additional service to Fargate. This requires an existing | ||
| # cluster deployed by fargate.cfn.yml. | ||
|
|
||
| Description: Fargate Service |
There was a problem hiding this comment.
IIRC, a Fargate service is also part of the Fargate template. Do you intend to DRY this up and use this template within a the "other" Fargate template? I notice several things in fargate.cfn.yml changed to mirror decisions you made in this file.
There was a problem hiding this comment.
Ha, I have been thinking about ways to DRY this up. Listener rules require a default target group, so I need to consider this. I can add a couple of conditions to a single template, but I am not sure if the result is going to be difficult for most to read. I created a ticket to investigate.
| MaxLength: 255 | ||
| AllowedPattern: "^[a-zA-Z][-a-zA-Z0-9]*$" | ||
|
|
||
| RegisterServiceWithAlb: |
There was a problem hiding this comment.
What about supporting NLB also by providing a dropdown for HTTP, HTTPS, or TCP in AppProtocol?
There was a problem hiding this comment.
Indeed added a ticket to support in the future.
templates/fargate-service.cfn.yml
Outdated
| - HTTPS | ||
| ConstraintDescription: Specify either HTTTP or HTTPS | ||
|
|
||
| ServiceURLPath: |
There was a problem hiding this comment.
Consistency nit: some initialisms appear as title case (e.g. Arn, Cpu) and others like this are upper case (e.g. URL, LB). CloudFormation appears to favor the former, while I favor the latter.
| - Fn::ImportValue: !Sub ${NetworkStackName}-PrivateSubnet1ID | ||
| - Fn::ImportValue: !Sub ${NetworkStackName}-PrivateSubnet2ID | ||
| DependsOn: | ||
| - TaskDefinition |
There was a problem hiding this comment.
The only place where you need explicit DependsOn declarations are where there's a dependency that isn't evident to CloudFormation from the resource definition. Here you're already referencing TaskDefinition as one of the service's parameters explicitly making this redundant.
There was a problem hiding this comment.
Well. That is kind of true. Having been burned by this in the past, I am still paranoid. I am hoping to gain more trust in dependency detection over time.
templates/fargate-service.cfn.yml
Outdated
| - CodeBuildProject | ||
| - CodePipelineServiceRole | ||
|
|
||
| # The namespace in Amazon CloudWatch Logs |
There was a problem hiding this comment.
It's not clear to me what this comment refers to.
templates/fargate-service.cfn.yml
Outdated
| LogGroup: | ||
| Type: AWS::Logs::LogGroup | ||
| Properties: | ||
| LogGroupName: !Sub /fargate/app/${EnvironmentName}/${GitHubSourceRepo}/${GitHubBranch} |
There was a problem hiding this comment.
This is an awful lot of specificity for what I assume will only be one permutation per environment name. I'd simplify.
| ExecutionRoleArn: !GetAtt TaskExecutionRole.Arn | ||
| ContainerDefinitions: | ||
| - Name: !Ref GitHubSourceRepo | ||
| Image: !Ref SeedDockerImage |
There was a problem hiding this comment.
💡: Could we replace this seed image with a custom resource that finds the current image deployed or returns a placeholder? Hadn't thought this through and it might result in a circular dependency, but it would potentially make stack updates safe. Either that, or perhaps we need to consider moving back to the CloudFormation deployment mechanism to create the service and task definitions.
| Value: !If [ IsDbStackSet, "Fn::ImportValue": !Sub "${DatabaseStackName}-DatabaseURL", "" ] | ||
| - Name: DATABASE_USER | ||
| Value: !If [ IsDbStackSet, "Fn::ImportValue": !Sub "${DatabaseStackName}-DatabaseUser", "" ] | ||
| - Name: LOAD_BALANCER_DNS |
There was a problem hiding this comment.
What is this, and why is it defined differently than https://github.com/aws-samples/startup-kit-templates/pull/28/files#diff-dc75529a746c95e046e723297f4b324eR607? I see the note in your pull request description but I still don't really understand what this environment variable is intended to do.
There was a problem hiding this comment.
This either displays the Route 53 record if the user specifies or the default DNS name for the ALB (exported value is conditional in the core/base fargate template) This is in place for peeps like me that like to route request to other microservices via the ALB. It is also in place for people who may need to specify links in various places. For example, I place an order and I send an email. The email contains an URL that links back to the product page. This env variable simplifies configuration management for the various environments (e.g., dev.foo.com, bar.foo.com).
There was a problem hiding this comment.
Got it So you're passing the canonical name of the application down so the application itself can use it for self-reference. I'm not sure LOAD_BALANCER_DNS is the most intention-revealing name. it's not wrong, of course, but it isn't precisely correct either, and its function was not obvious to me as a reader. Consider a name that more closely aligns to your intent such as: CANONICAL_NAME.
This PR adds the fargate-service.cfn.yml template which supports adding new service(s) to an existing Startup Kit Fargate cluster. Users can toggle registration with the ALB for their service, which they may want to disable if the service is background only.
Additionally, there are some minor modifications including:
This PR was tested extensively by manually launching stacks with unique parameters. It was also tested by adding numerous new services (via the new CFN template).
The README.md has not been updated to support the V3 links for this release.