add fargate ha consul cluster backed by efs #139
Conversation
There was a problem hiding this comment.
I'd prefer to add only one additional module at the top of the modules/ directory for deploying a consul server cluster.
I'd suggest the following:
- Move both
ecs-serviceandefs-clustermodules to be submodules of the ha-cluster module- Move
modules/ecs-servicetomodules/ha-cluster/ecs-service - Move
modules/efs-clustertomodules/ha-cluster/efs-cluster
- Move
- Rename
modules/ha-clustertomodules/dev-server-ha(ormodules/dev-server-cluster) to better fit alongside the existingmodules/dev-servermodule
There was a problem hiding this comment.
Stated in another comment, but please rename this module to modules/dev-server-ha (or modules/dev-server-cluster or similar) to better fit alongside the existing modules/dev-server module, and indicate that the module is for dev/testing and not (yet) for production use.
| # --------------------------------------------------------------------------------------------------------------------- | ||
| # ENVIRONMENT VARIABLES | ||
| # Define these secrets as environment variables | ||
| # --------------------------------------------------------------------------------------------------------------------- | ||
|
|
||
| # AWS_ACCESS_KEY_ID | ||
| # AWS_SECRET_ACCESS_KEY | ||
| # AWS_DEFAULT_REGION | ||
|
|
||
| # --------------------------------------------------------------------------------------------------------------------- | ||
| # OPTIONAL PARAMETERS | ||
| # These parameters have reasonable defaults. | ||
| # --------------------------------------------------------------------------------------------------------------------- |
There was a problem hiding this comment.
I'd suggest removing these comments. Instead, we should update the README with any requirements for using this module.
There was a problem hiding this comment.
Please move this lambda-k6 module out of the modules directory. I'd suggest either examples/lambda-k6 or test/performance/lambda-k6 for the new location.
| } | ||
|
|
||
| variable "operating_system_family" { | ||
| default = "LINUX" |
| containerPort : 8502, | ||
| hostPort : 8502, | ||
| protocol : "tcp" | ||
| }, { |
There was a problem hiding this comment.
I notice you have ports.grpc_tls = 8503 above, so probably add 8503 to the list of container ports here for consistency. (Note that ports.grpc_tls is only supported in Consul 1.14+)
| consul_portmap = [{ | ||
| containerPort : 8300, | ||
| hostPort : 8300, | ||
| protocol : "tcp" |
There was a problem hiding this comment.
nit: Please remove the hostPort field from these port mappings. We only support the awsvpc network mode for our other modules, so we can assume the same here (for now). In awsvpc network mode, the host port cannot be different than the container port, so I'd prefer to leave the field out (similar to the dev-server module)
| default = "" | ||
| } | ||
|
|
||
| variable "sidecar_name" { |
There was a problem hiding this comment.
The sidecar_name variable appears to be unused? Can it be removed?
| variable "datadog_task_cpu" { | ||
| description = "Set the Consul Server task CPU limit. Default is 1792." | ||
| default = 256 | ||
| } | ||
|
|
||
| variable "datadog_task_memory" { | ||
| description = "Set the Consul Server container Memory limit. Default is 3584." | ||
| default = 512 | ||
| } |
There was a problem hiding this comment.
Please remove the datadog specific functionality.
Instead, we can support generic variables, like additional_container_definitions, additional_task_role_policies, etc, in order to inject an additional container into the task definition. That way we can support any kind of additional "sidecar" container.
|
|
||
| variable "consul_count" { | ||
| description = "Number of consul servers to deploy. Default 3" | ||
| default = 3 |
There was a problem hiding this comment.
minor / non-blocking: Validate the consul_count is odd and greater than 0. Something like the following:
| default = 3 | |
| default = 3 | |
| validation { | |
| error_message = "The consul_count must be odd and must be greater than 0." | |
| condition = var.consul_count % 2 == 1 && var.consul_count > 0 | |
| } |
Changes proposed in this PR:
How I've tested this PR:
After building consul with PR: 13782 to support ECS for Go Discover, create a variables file as described within the README.md to point your consul image variable at the image url for the image you have built (until consul is updated with the necessary go-discover version).
How I expect reviewers to test this PR:
Using the example
ha-cluster-fargate, create ca and certs, deploy resources to AWS and load test.Checklist: