copilot-cli icon indicating copy to clipboard operation
copilot-cli copied to clipboard

Allow more than 5 Domains per Service

Open cmaerz opened this issue 2 years ago • 18 comments

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

cmaerz avatar Mar 07 '23 08:03 cmaerz

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 avatar Mar 07 '23 22:03 efekarakus

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?

efekarakus avatar Mar 07 '23 22:03 efekarakus

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

cmaerz avatar Mar 08 '23 07:03 cmaerz

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.

surrealchemist avatar May 24 '23 00:05 surrealchemist

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: "/"

KollaAdithya avatar Jun 08 '23 23:06 KollaAdithya

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

cmaerz avatar Jun 09 '23 07:06 cmaerz

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.

surrealchemist avatar Jun 09 '23 15:06 surrealchemist

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

KollaAdithya avatar Jun 09 '23 21:06 KollaAdithya

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 avatar Jun 09 '23 22:06 surrealchemist

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.

surrealchemist avatar Jun 12 '23 15:06 surrealchemist

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.

iamhopaul123 avatar Jun 13 '23 16:06 iamhopaul123

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

huanjani avatar Jul 19 '23 19:07 huanjani

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.

surrealchemist avatar Jul 20 '23 15:07 surrealchemist

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.

huanjani avatar Jul 20 '23 21:07 huanjani

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.

surrealchemist avatar Jul 24 '23 16:07 surrealchemist

@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!

huanjani avatar Jul 24 '23 20:07 huanjani

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'

ssyberg avatar Mar 05 '24 18:03 ssyberg

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

iamhopaul123 avatar Mar 05 '24 20:03 iamhopaul123