aws / copilot-cli

The AWS Copilot CLI is a tool for developers to build, release and operate production ready containerized applications on AWS App Runner or Amazon ECS on AWS Fargate.
https://aws.github.io/copilot-cli/
Apache License 2.0
3.5k stars 408 forks source link

Allow more than 5 Domains per Service #4601

Open cmaerz opened 1 year ago

cmaerz commented 1 year ago

Context

We have a multi tenant app with > 20 Domains. And cannot add more than 5 Domains to the ALB, cause the Matching Rules are not split.

ToDo

If there are more than 5 Domains added to the ALB they should be split in multiple Matching Rules cause the maximum condition Count is 5.

More info here: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-limits.html

Gitter discussion: https://matrix.to/#/!QdxoBcgpJveoAoIPCc:gitter.im/$mjaMtBb694mAzgGB2p92KApi0CDxe-ad4j39yfWpLcA?via=gitter.im&via=matrix.org&via=matrix.martinides.de

efekarakus commented 1 year ago

Hi @cmaerz ! The feature request totally makes sense to me and thank you for suggesting a path for implementation too.

Splitting the listener rules to have 5 hostheaders each make sense to me 👍

efekarakus commented 1 year ago

In the mean time, is there a way of writing a wildcard alias such *.example.com to match the > 20+ domains or is that not possible today?

cmaerz commented 1 year ago

I'm pretty sure that worked when i tried it, if the SSL Certificate covers it.

surrealchemist commented 1 year ago

Yes wildcard for subdomains works fine. I have a use case where I have a longer list of domains that can't be covered by wildcard that it won't work out of the box. We will have to look at override or addon to address the issue with custom rules. My previous setup before co-pilot sent everything to the same listener, even if the cert wasn't there so I can see how its made to verify. If we had a longer and longer list in copilot manifest its not the cleanest thing either. I don't know if it would keep making the process take longer as it verifies the certs and then you have the output on the CLI listing every single domain, but this use case might not come up that often anyway to warrant addressing that.

KollaAdithya commented 1 year ago

Hello all!

As a workaround if you have more than 5 domains, we can the split the domains into http.additional_rules with the same container port and path as below.

image:
  build: ./Dockerfile
  # Port exposed through your container to route traffic to it.
  port: 80
http:
  path: "/"
  alias: ["example.com", "v1.example.com"]
  allowed_source_ips: ["192.0.2.0/24", "198.51.100.10/32"]
  additional_rules: 
    - alias: ["v2.example.com","v3.example.com"]
      allowed_source_ips: ["192.0.2.0/24", "198.51.100.10/32"]
      path: "/"
    - alias: ["v4.example.com","v5.example.com"]
      allowed_source_ips: ["192.0.2.0/24", "198.51.100.10/32"]
      path: "/"
cmaerz commented 1 year ago

Nice! Should be documented somewhere. I guess thats a problem for many multi tenant projects.

surrealchemist commented 1 year ago

I'm trying to use this method, but I have a custom healthcheck path which I had set at http.healthcheck and it seems like it adds the rules fine but then ignores the healthcheck. Trying to add it under additional_rules but failing validation for different reasons. If i set additional_rules.healthcheck to my custom path I get validate "http": validate "additional_rules[0]": "path" must be specified if I add healthcheck and then path under it I get certificate validation errors. Not really sure if/how I can get it to work.

KollaAdithya commented 1 year ago

Hey @surrealchemist ! Can you provide your manifest file to check the configuration

I am able to deploy the service with the below configuration ⬇️

image:
  build: ./Dockerfile
  # Port exposed through your container to route traffic to it.
  port: 80
http:
  path: "/"
  alias: ["example.com", "v1.example.com"]
  healthcheck:
    path: '/'
    port: 80
    success_codes: '200'
    healthy_threshold: 3
    unhealthy_threshold: 2
    interval: 15s
    timeout: 10s
    grace_period: 60s
  allowed_source_ips: ["192.0.2.0/24", "198.51.100.10/32"]
  additional_rules: 
    - alias: ["v2.example.com","v3.example.com"]
      allowed_source_ips: ["192.0.2.0/24", "198.51.100.10/32"]
      path: "/"
      healthcheck:
        path: '/'
        port: 80
        success_codes: '200'
        healthy_threshold: 3
        unhealthy_threshold: 2
        interval: 15s
        timeout: 10s
    - alias: ["v4.example.com","v5.example.com"]
      allowed_source_ips: ["192.0.2.0/24", "198.51.100.10/32"]
      path: "/"
      healthcheck:
        path: '/'
        port: 80
        success_codes: '200'
        healthy_threshold: 3
        unhealthy_threshold: 2
        interval: 15s
        timeout: 10s
surrealchemist commented 1 year ago

I gave it another go, added healthcheck with the path I need for each rule and that seems to work. It doesn't seem obvious that is how it would work, as the health check is at the target group level, so it wasn't clear how it related to the rules. I guess each rule points at the target group, but I would think if I set it up above with http it would just inherit it for everything. Anyway, I think this is enough for us to use for our use case now but maybe it can be explained better or have some more examples in the documentation that combine the features.

surrealchemist commented 1 year ago

I see now what was happening with this. When you add an additional rule it creates a new target group for each rule. So if you don't specify the healthcheck it defaults back to /. It also means I am getting multiple health checks to the same container every time it fires, where before I just got a single one. It seems a bit excessive, especially if people end up having more rules it compounds the requests. It may still work for my use case, but it does create another thing to be aware of.

iamhopaul123 commented 1 year ago

It doesn't seem obvious that is how it would work, as the health check is at the target group level, so it wasn't clear how it related to the rules.

Yeah I would agree it is not that obvious. I think we are currently making it better. At least make the error msg better when users have this "too many aliases" issue, with good recommendation how they can get it around by splitting into multiple rules.

It also means I am getting multiple health checks to the same container every time it fires, where before I just got a single one. It seems a bit excessive, especially if people end up having more rules it compounds the requests. It may still work for my use case, but it does create another thing to be aware of.

Very good point. It actually doesn't make any sense to specify multiple healthcheck for the same container. We are currently thinking to keep just one healthcheck which is under http, so that we won't confuse customers. Eventually we need to restructure the manifest for this. Sorry again for the confusion and inconvenience.

huanjani commented 1 year ago

This improvement has been released in v1.29.0: https://github.com/aws/copilot-cli/releases/tag/v1.29.0!

surrealchemist commented 1 year ago

Is there any open issue to track the use of all the extra target groups, or the extra health checks? I'd like to follow that if its in the works. It might be a more involved thing than this fix.

huanjani commented 1 year ago

We can reopen this issue (and you can add more context if you'd like), or you can open a new separate issue; I don't think there's an existing one that's specific to your ask.

surrealchemist commented 1 year ago

Basically adding extra rules creates a new target group for each rule and adds a health check for each one. They all hit the same app and have to be changed if you want to use a custom health check. This is going to build up the amount of extra requests all going to the same container doing redundant health checks. This is going to build up over time as we add more domains so I even though the error is more helpful, I don't think the solution is good for long term use.

huanjani commented 1 year ago

@surrealchemist-- I just learned that we do have a more involved solution in the works, so you're right-- this issue shouldn't have been closed!

ssyberg commented 7 months ago

I'm getting this error with only 4 domains:

environments:
    production:
        http:
            deregistration_delay: 20s
            alias: ['<redacted>.org', '<redacted>.de']
            path: 'cms'
            healthcheck:
                path: '/cms/api/users/me'
                grace_period: 60s

            additional_rules:
                - path: 'static'
                  alias: ['<redacted>.org', '<redacted>.de', '<redacted>.de', '<redacted>.org']
                  healthcheck:
                      path: '/cms/api/users/me'
iamhopaul123 commented 7 months ago

Hello @ssyberg. Sorry for the confusion. I think alias: ['<redacted>.org', '<redacted>.de'] counts as well.