search

openwallet-foundation / acapy

ACA-Py is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://aca-py.org
Apache License 2.0
421 stars 515 forks source link

Allow HEAD requests to default route #2068

Closed johnekent closed 1 year ago

johnekent commented 1 year ago

https://github.com/hyperledger/aries-cloudagent-python/blob/6f2ef55d2dbd22d7313f3c346180008f2ba93188/aries_cloudagent/admin/server.py#L436

The current default route does not allow_head. External systems can use the HEAD method to check the health of this port/route successfully because these external systems get a response, but the agent logs an HTTPMethodNotAllowed error and stack trace with each check. In some cases, the external system's check mechanism can't be changed.

Is there a reason why the default route does not have allow_head=True? If this change could be made and doesn't impose any risk or concern to ACA-py, then the same check functionality could be met while having cleaner server logs for easier monitoring and troubleshooting.

Steps to reproduce error:

  1. start cloud agent
  2. go to admin port in browser, and get a redirect to swagger
  3. run curl (-I, --head, etc.) with head method request to admin port
  4. see error in cloud agent log

Same steps with allow_head=True in line noted above. 1-3 the same as above.

  1. no errors in the agent log
swcurran commented 1 year ago

For those wondering, as I was, what HEAD is, here is a description: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD

Who knew?

@WadeBarnes -- any reason not to do this?

@johnekent -- would you do a PR? If so, make sure you use DCO: DCO - Developer Certificate of Origin - https://github.com/apps/dco (e.g. use commit -s ...).

ianco commented 1 year ago

Yes we should allow this, probably just on specific endpoints

johnekent commented 1 year ago

Thanks for the review of this issue and for the opportunity to modify. I will pursue a PR following the contributions guide, likely next week.

WadeBarnes commented 1 year ago

@johnekent, Just to confirm, did you know about the k8s compatible health check endpoints that are already available on the aca-py instances? They provide some additional internal checks to ensure the health and readiness of an aca-py instance that a HEAD query would not provide. They are /status/live and /status/ready.

johnekent commented 1 year ago

Thanks for ensuring this awareness @WadeBarnes . I am aware, and this was nicely described on discord. The issue I'm facing is a lack of control over the health checker, which is constrained by the issue noted. These more robust internal checks should be preferred when allowed by the checker.

johnekent commented 1 year ago

Hi @swcurran @ianco @WadeBarnes . PR is https://github.com/hyperledger/aries-cloudagent-python/pull/2077 Would you please review?

swcurran commented 1 year ago

Closing with #2077