feat(anta)!: Use HTTP HEAD request instead of a TCP connection to check if device is online #851
Conversation
049202b to
6e61446
Compare
There was a problem hiding this comment.
Hi David,
This changes the current behaviour, hence changing the PR name.
I understand that your use case is to actually check that eAPI is reachable behind a reverse proxy which is not possible today with the simple call to asyncio.open_connection().
I checked the behaviour of a HEAD request on the base_url and here is the result:
$ curl -k --head https://172.20.1.3
HTTP/1.1 301 Moved Permanently
Server: nginx
Date: Thu, 10 Oct 2024 15:34:22 GMT
Content-Type: text/html
Content-Length: 13
Connection: keep-alive
Location: https://172.20.1.3/eapi/
https://172.20.1.3/eapi/ is actually the eAPI Explorer.
So we can either do that, expect and 301 and test the redirect URL (https://www.python-httpx.org/quickstart/#redirection-and-history)
Or we can do a HEAD on https://{base_url}/command-api and expect a 401 if not authenticated:
$ curl -k --head https://172.20.1.3/command-api
HTTP/1.1 401 Unauthorized
Server: nginx
Date: Thu, 10 Oct 2024 15:38:59 GMT
Content-Type: text/plain
Content-Length: 30
Connection: keep-alive
I assume the code should also expect a 200 if the asynceapi.Device instance is authenticated.
Please update the code to handle that behaviour and write unit tests :)
Regarding login, I like the idea of checking the credentials before sending the "show version" eAPI request like we do today. I've opened #871 to track this.
Thanks !
|
Hi Matthieu, I also think that requesting headers from command-api would be better. Since we have the credentials we can then check the status code and conclude if we can actually connect to the eAPI server. I'll update the code and add the required tests. David |
6bf8440 to
0235699
Compare
|
CodSpeed Performance ReportMerging #851 will not alter performanceComparing Summary
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
b2b46e7 to
d9b4b61
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
There was a problem hiding this comment.
@gmuloc I am ok with merging this (after fixing my review comments) in a minor release but please confirm and consider the following for refresh:
1- Currently, is_online could be True even if subsequent commands failed due to HTTP errors (like 401 Unauthorized). Now, such errors will cause is_online to be False right away.
2- Errors like 401, 500 are detected before attempting show version, whereas before they were detected after the _collect(show_version) failed.
3- The warning message associated with these failures changes from potentially Cannot get hardware information... (if _collect failed) to the new Could not connect... eAPI endpoint: [Error details].
4- Because of this, self.established will become False earlier and for more reasons (HTTP errors during the initial check) than before.
To me it's arguably an improvement/fix but I will let you chime in.
* HEAD request to /command-api url * Handle HTTPStatus error in refresh, log error message * Update unit tests
d9b4b61 to
6c1e9aa
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
Description
Use HTTP HEAD request to check connection to a device instead of pure L3 check.
This allows to check connectivity on devices behind a reverse proxy, so rather than checking the reverse proxy is alive using HTTP the request will be forwarded to the right device.
The change is as close to the original as possible, but we could potentially check if the credentials are correct as well.
Checklist:
pre-commit run)tox -e testenv)