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

docs(manifest): add gotchas to template for multiple lb web services #1377

Closed
wants to merge 3 commits into from

Conversation

GavinRay97
Copy link

I'm not entirely sure about all of these, but I think these are generally the case. If mistaken about these can remove, but think it would be really helpful and prevent headaches to list common gotchas here when generating the template.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

I'm not entirely sure about all of these, but I think these are generally the case. If mistaken about these can remove, but think it would be really helpful and prevent headaches to list common gotchas here when generating the template.
Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you so much for these clarifications! What do you think about the suggestions below?

They wrap the comments behind an if-statements so that they only show up if users setup multiple load balanced web services.

templates/workloads/services/lb-web/manifest.yml Outdated Show resolved Hide resolved
Comment on lines +23 to 26
# WARNING: Passing healthchecks are required and are how ECS measures task health. Failure to pass healthcheck may result cyclical restarts.
# NOTE: The healthcheck URL should be appended to the "path" value.
# For example, if "path" is set to "api", and your API serves it's healthcheck on "/healthz", this value should be "/api/healthz"
# healthcheck: '{{.HealthCheckPath}}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# WARNING: Passing healthchecks are required and are how ECS measures task health. Failure to pass healthcheck may result cyclical restarts.
# NOTE: The healthcheck URL should be appended to the "path" value.
# For example, if "path" is set to "api", and your API serves it's healthcheck on "/healthz", this value should be "/api/healthz"
# healthcheck: '{{.HealthCheckPath}}'
# healthcheck: '{{.HealthCheckPath}}' {{if ne .Path "/"}} # Must be a path under '{{.Path}}'. For example: '{{.Path}}/_healthcheck' {{end}}

Copy link
Author

@GavinRay97 GavinRay97 Sep 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked on this one -- for one service, it threw errors that the prefixed path wasn't found.

Think this one has a weird interplay with the Load Balancer. For example, with Hasura:
https://github.com/hasura/graphql-engine

It seems that if you put this at a subpath, path: "hasura", you cannot load any endpoint after it being deployed with Copilot because the internal application router is broken.

Because the root path already had /hasura in it, when I tried to visit the route /console at /hasura/console, it throw nonexistent path error from the Hasura service, and then in the Fargate logs I could see it wasn't stripping the /hasura prefix from the route:

image

See http_info.url here:

{
  "type": "http-log",
  "timestamp": "2020-09-13T18:55:04.819+0000",
  "level": "error",
  "detail": {
    "operation": {
      "error": {
        "path": "$",
        "error": "resource does not exist",
        "code": "not-found"
      },
      "request_id": "105a938a-82b6-4b62-83bc-594ddaa2fc73",
      "response_size": 65,
      "raw_query": ""
    },
    "http_info": {
      "status": 404,
      "http_version": "HTTP/1.1",
      "url": "/hasura/console",
      "ip": "73.179.121.23",
      "method": "GET",
      "content_encoding": null
    }
  }
}

In Hasura's case, the Healthcheck endpoint lives at /healthz, but giving /hasura/healthz as a path to Copilot results in failure, and giving /healthz cannot work because it'll never hit the /hasura path and be routed 😬

image

This kind of effectively renders the web service useless since none of the paths will route properly.

(Unrelated: Would attaching a domain fix this?)

Might be better for someone from your team who understands this more to weigh in here, I think I may have been mistaken on this one, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya!

I double-checked this and the healthcheck path should still be the path in the service code (just /healthz and not /hasura/healthz). I believe this is because it's a property of the Target Group so you don't need to prefix it with the rule path ("hasura").

Out of curiosity: Does running the container locally work? My suspicion is that the container fails to come up so it never gets to the point where it can query the "/healthz" path.

Copy link
Author

@GavinRay97 GavinRay97 Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm 🤔

The hasura instance does run locally, and it was running on the ELB there (that error format with path: $, error: xxx is Hasura's web server response) but because of the LB having it at /hasura, when Hasura router handled the requests it was trying to send them to wrong place.

For example:

Load Balanced Route | Desired App Path | Routed App Path               
----------------------------------------------------------
/hasura/console     |     /console     | /hasura/console

So basically because the app webserver router is parsing /hasura/console and not stripping /hasura, it won't route properly.

healthcheck path should still be the path in the service code (just /healthz and not /hasura/healthz). I believe this is because it's a property of the Target Group so you don't need to prefix it with the rule path ("hasura").

This makes sense -- so it is not possible to check the healthcheck path from outside of the running container then right?

If you want to test this really quick, you can make a Dockerfile like below:

FROM hasura/graphql-engine:v1.3.1

And this manifest + try deploying it (it needs HASURA_GRAPHQL_DATABASE_URL env connected to Postgres):

name: hasura
type: Load Balanced Web Service
image:
  # Assuming Dockerfile in folder /hasura from project root
  build: hasura/Dockerfile
  port: 8080

http:
  path: "hasura"
  healthcheck: "/healthz"

cpu: 512
memory: 1024
count: 1

variables:
  HASURA_GRAPHQL_ENABLE_CONSOLE: "true"
  HASURA_GRAPHQL_DEV_MODE: "true"
  # Set Postgres RDS (or any other Postgres) credentials here
  HASURA_GRAPHQL_DATABASE_URL: postgres://<RDS DB Username>:<RDS DB Password>@xxxxxxxxxx.us-east-1.rds.amazonaws.com/<DB name>
  HASURA_GRAPHQL_ENABLED_LOG_TYPES: startup, http-log, webhook-log, websocket-log, query-log

templates/workloads/services/lb-web/manifest.yml Outdated Show resolved Hide resolved
@efekarakus efekarakus changed the title feat(manifest): Add notes about gotchas - template docs(manifest): add gotchas to template for multiple lb web services Sep 13, 2020
@efekarakus
Copy link
Contributor

Heya @GavinRay97 I'll close this PR as we talked on Gitter about the healthcheck path and a possible way forward for hasura as a backend service.

Please feel free to re-open it if you'd like to discuss further. Thanks!

@efekarakus efekarakus closed this Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants