Add handshake validators #308
Conversation
service/handler.go
Outdated
| for _, fun := range a.GetSession().GetHandshakeValidators() { | ||
| if err := fun(handshakeData); err != nil { | ||
| a.SetStatus(constants.StatusClosed) | ||
| return fmt.Errorf("Handshake validation failed: %w. Id=%d", err, a.GetSession().ID()) |
There was a problem hiding this comment.
Is it possible to return specific error codes to clients if the validation fail? That'd be very useful to handle forced updates and server maintenance, for example.
There was a problem hiding this comment.
Isn't the err here already the one returned by the handshake validator? And if so it is something you have control over already, right?
There was a problem hiding this comment.
In this step, clients already received a message about the handshake.
Any error that may occur after sending the handshake response, just close the session.
There was a problem hiding this comment.
Hum, I see. I guess it'd be best to return the handshake response only after validation.
From the client perspective, getting a sudden connection close without reason might be very confusing during development, wouldn't it?
There was a problem hiding this comment.
Yes! I've changed the process to send more comprehensive messages to clients. This first version should not break anything on the client side, but it won't send any information about the error to clients, just a 400 code (Bad Request). A further (maybe breaking) change can be addressed to provide to client a clearer message about what happened in the server.
service/handler.go
Outdated
| @@ -227,6 +227,13 @@ func (h *HandlerService) processPacket(a agent.Agent, p *packet.Packet) error { | |||
| return fmt.Errorf("Invalid handshake data. Id=%d", a.GetSession().ID()) | |||
| } | |||
|
|
|||
| for _, fun := range a.GetSession().GetHandshakeValidators() { | |||
| if err := fun(handshakeData); err != nil { | |||
There was a problem hiding this comment.
The way it is, an error is returned when the first validation fails, ignoring following validations. Would it be interesting to run all validators and return an error if any one of them fails (possibly bundling validation messages)? This would provide less back and forth in case multiple validations are to fail.
There was a problem hiding this comment.
To be honest, I'm not sure if this use-case if that valuable, though.
There was a problem hiding this comment.
For simplicity sake's I say lets keep the way it is right now. The end result is the same whether multiple or only one validator failed.
service/handler.go
Outdated
| @@ -227,6 +227,13 @@ func (h *HandlerService) processPacket(a agent.Agent, p *packet.Packet) error { | |||
| return fmt.Errorf("Invalid handshake data. Id=%d", a.GetSession().ID()) | |||
| } | |||
|
|
|||
| for _, fun := range a.GetSession().GetHandshakeValidators() { | |||
| if err := fun(handshakeData); err != nil { | |||
There was a problem hiding this comment.
For simplicity sake's I say lets keep the way it is right now. The end result is the same whether multiple or only one validator failed.
service/handler.go
Outdated
| for _, fun := range a.GetSession().GetHandshakeValidators() { | ||
| if err := fun(handshakeData); err != nil { | ||
| a.SetStatus(constants.StatusClosed) | ||
| return fmt.Errorf("Handshake validation failed: %w. Id=%d", err, a.GetSession().ID()) |
There was a problem hiding this comment.
Isn't the err here already the one returned by the handshake validator? And if so it is something you have control over already, right?
Pull Request Test Coverage Report for Build 5068848464
💛 - Coveralls |
|
@reinaldooli please update the docs with HandshakeValidators information |
felipejfc
force-pushed
the
main
branch
3 times, most recently
from
1d5b0cf
to
98580b5
Compare
There was a problem hiding this comment.
Other the the comment I raised, the rest looks good. I also agree with the conclusion in @vitorbaraujo's thread.
However, I believe it'd be important to resolve my thread before merging.
Add post build hooks
Context
Sometimes, it is mandatory that the clients met some requirements before being allowed to connect.
In order to achieve this,
SessionPoolnow has a functionAddHandshakeValidatorand all the added validators will run as soon as the handshake is processed.With this, clients can send the requirements through handshake parameters, and it can be validated before establishing the connection.