Investigate /dut/ip endpoint

balena-os / leviathan-worker

The worker layer for Leviathan including worker plugins QemuKit & AutoKit

Apache License 2.0

6 stars 4 forks source link

Investigate /dut/ip endpoint #114

Open vipulgupta2048 opened 3 weeks ago

vipulgupta2048 commented 3 weeks ago

The GET endpoint, as written on the leviathan-worker, doesn't seem to work unless a body is sent to it, as documented here by the Leviathan client sending it one. I want to understand why is this working and if the solution is to convert the method for this endpoint to POST instead and maybe it was an error in the code.

The investigation is into why it has been working over the years. cc: @rcooke-warwick if he knows. Also, @alexgg

Reported on https://balena.zendesk.com/agent/tickets/3524 https://balena.zulipchat.com/#narrow/stream/345889-balena-io.2Fos/topic/.5BLeviathan.5D.20OS.20development.20using.20autokit

rcooke-warwick commented 2 weeks ago

@vipulgupta2048

"why this is working" - it works because there is no violation happening here, GET requests are allowed to have a body. In this case a body is required, as the hostname/uuid of the DUT is required to find the matching IP. It doesn't work without a body intentionally, as the hsotname of the DUT isn't known
should this be a GET - probably not - while GET is allowed to have a body, it looks like it should be : https://www.rfc-editor.org/rfc/rfc2616#section-4.3 - this seems to say that bodies of GET requests should be ignored. So changing to a POST is probably for the best.

Overall I don't actually think there's an issue here, other than we've got a GET endpoint that is never used manually that parses the body when it probably shouldn't.

alexgg commented 2 weeks ago

I agree with @rcooke-warwick it might be better passing the hostname as an argument (dut/ip?<hostname=nnnnn>) or to serve more REST-like APIs GET dut/ip/<hostname> but I don't think it's something we need to address.

vipulgupta2048 commented 1 week ago

We've got a GET endpoint that is never used manually that parses the body when it probably shouldn't.

This GET endpoint is the easiest way to ascertain the IP address of the DUT and I use it manually a number of times. I am all ears, if there is an easier way to find the IP address of the DUT. We should probably have it show up in the logs along with the hostname and that would be an improvement.

The /dut/ip GET request without a body doesn't work. Our server parses the body, which should actually should be ignored. Creating confusion, and breaking semantics defined. I don't see how this is not an issue semantically and something we should fix.

The simplest solution would be for this request to POST on the client + server. Will create a PR for that.

alexgg commented 1 week ago

The simplest solution would be for this request to POST on the client + server. Will create a PR for that.

@vipulgupta2048 POST is not a solution - this is a request for an IP address, a GET conceptually and it just needs an argument which is totally OK for GETs.

If the problem you see is passing the argument in the body, which I think it's just inconvenient but curl can handle, the PR should be to pass the argument instead.

vipulgupta2048 commented 1 week ago

Yeah I would accept that as a compromise and with some comments which at the very least brings more clarity even though I still am not satisfied that we are not following the "no functional body" should be present in GET to be processed by the server.