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

feat(anta)!: Use HTTP HEAD request instead of a TCP connection to check if device is online #851

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dlobato
Copy link
Contributor

@dlobato dlobato commented Oct 4, 2024

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:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have run pre-commit for code linting and typing (pre-commit run)
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (tox -e testenv)

@dlobato dlobato changed the title refactor(asynceapi): Use http head request to check_connection refactor(anta): Use http head request to check_connection Oct 4, 2024
Copy link
Collaborator

@mtache mtache left a comment

Choose a reason for hiding this comment

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

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 !

@mtache mtache added this to the v2.0.0 milestone Oct 10, 2024
@mtache mtache changed the title refactor(anta): Use http head request to check_connection feat(anta)!: Use HTTP HEAD request instead of opening a TCP connection Oct 10, 2024
@mtache mtache changed the title feat(anta)!: Use HTTP HEAD request instead of opening a TCP connection feat(anta)!: Use HTTP HEAD request instead of a TCP connection to check if device is online Oct 10, 2024
@dlobato
Copy link
Contributor Author

dlobato commented Oct 11, 2024

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

* HEAD request to /command-api url
* Handle HTTPStatus error in refresh, log error message
* Update unit tests
Copy link

sonarcloud bot commented Oct 11, 2024

Copy link

codspeed-hq bot commented Oct 11, 2024

CodSpeed Performance Report

Merging #851 will not alter performance

Comparing dlobato:check-connection-with-head (0235699) with main (92cbcd3)

Summary

✅ 8 untouched benchmarks

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants