machine registration connectivity rework - part I #140
Conversation
fgiudici
force-pushed
the
websocket_reg01
branch
from
31bd79b to
4e35db2
Compare
github-actions
bot
added
area/operator
|
rebased on main |
Codecov Report
@@ Coverage Diff @@
## main #140 +/- ##
==========================================
- Coverage 33.77% 30.26% -3.52%
==========================================
Files 5 5
Lines 379 423 +44
==========================================
Hits 128 128
- Misses 246 290 +44
Partials 5 5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
fgiudici
force-pushed
the
websocket_reg01
branch
from
4e35db2 to
9e36eff
Compare
There was a problem hiding this comment.
Looks good to me, even I am not deeply familiar with the former issue of using https headers to pass data.
In any case it seams like a good move to me setting the websocket apart from the tpm authentication.
I am not sure the e2e tests are covering this (I don't think so), if not I am wondering that probably this is the right time to add some. It could be done in a follow up PR/card after the three parts are in though.
| // get the machine registration relevant to this request | ||
| registration, err := i.getMachineRegistration(req) | ||
| if err != nil { | ||
| http.Error(resp, err.Error(), http.StatusNotFound) | ||
| return | ||
| } | ||
|
|
||
| // attempt to authenticate the machine, if the machine is nil, authentication has failed | ||
| inventory, w, err := i.authMachine(resp, req, registration.Namespace) | ||
| if !websocket.IsWebSocketUpgrade(req) { |
There was a problem hiding this comment.
how does this works? we run the auth after this part? Not sure I understand it correctly
There was a problem hiding this comment.
Basically, in order to switch a plan HTTP connection to a websocket one, it is the client that must add an "Upgrade" header in the HTTP GET request first (the gorilla/websocket library Dial() function does this automatically for the client, so it is hidden in the code). The idea here is, as soon as we receive an HTTP GET, let's check if it contains the UPGRADE header to upgrade to a websocket connection. If so, it should be a register client, otherwise is just a request to download the registration yaml to feed the elemental-register cli (via curl or any plain HTTP client).
If the websocket upgrade header is missing, the client is likely a plain HTTP one, so let's return immediately the unauthenticated reply. Add a couple of debug logs to semplify debugging when needed. Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
we already log the same error in the calling function (writeMachineInventoryCloudConfig) Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
Till now the TPM package was in charge to upgrade the HTTP connection and manage the websocket one. Move the connection management to the server package to split connection management from TPM authentication. This will allow us later to pass the smbios data and the labels through the channel instead of abusing HTTP headers. Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
Error: cyclomatic complexity 16 of func `(*InventoryServer).ServeHTTP` is high (> 15) (gocyclo) Move code to update labels from headers to subfunction Signed-off-by: Francesco Giudici <francesco.giudici@suse.com>
fgiudici
force-pushed
the
websocket_reg01
branch
from
9e36eff to
71d2a3e
Compare
Till now the TPM package was in charge to upgrade the HTTP connection and manage the websocket one.
This PR moves the connection management (server side) to the server package to split the connection management from TPM authentication.
This will allow us later (part II) to make it easier to pass the smbios data and the labels through the websocket channel instead of abusing HTTP headers.
This PR doesn't introduce any functional change, so doesn't change (nor break) current websocket communication pattern.
Related to #5
Partially fixes #130