Maintain sticky flag on LB method validation failure. #1585
Conversation
timoreimann
added
area/sticky-session
kind/bug/fix
|
@containous/traefik FYI. /cc @ kleinsasserm. |
timoreimann
force-pushed
the
1-3-maintain-sticky-flag-on-lb-method-validation-failure
branch
from
b6c9484
to
3f02c2c
Compare
server/server.go
Outdated
| log.Debugf("Validation of load balancer method for backend %s failed: %s. Using default method wrr.", backendName, err) | ||
| backend.LoadBalancer = &types.LoadBalancer{ | ||
| Method: "wrr", | ||
| Sticky: backend.LoadBalancer.Sticky, |
There was a problem hiding this comment.
Effectively, this should be the only addition to the production code.
|
Integration tests keep failing. Not exactly sure why, will have to investigate. |
server/server.go
Outdated
| } | ||
| } | ||
|
|
||
| func configureBackends(backends map[string]*types.Backend) { |
There was a problem hiding this comment.
why not func (server *Server) configureBackends?
There was a problem hiding this comment.
Because the backend configuration requires nothing from server for now.
There was a problem hiding this comment.
I also find this a bit odd to be honest ^^
There was a problem hiding this comment.
I can turn it into a receiver method if you really want to, but it would carry the wrong message to the reader (and my IDE's "find usage" function) that server is somehow needed for this method to work properly, which it isn't. It also makes things like testing more complicated.
Effectively, it'd be similar to passing an unused variable to a function call.
There was a problem hiding this comment.
@timoreimann I think there is at least 2 reasons to add func (server *Server) to both functions:
- it's homogeneous (to the reader, it would be easier to understand if
configureFrontendsandconfigureBackendsadd the same signature) - we could only call
configureBackendsif we had a reference to aServerwhich is makes more sense
There was a problem hiding this comment.
Okay, converted configureBackends into a method.
PTAL again.
timoreimann
force-pushed
the
1-3-maintain-sticky-flag-on-lb-method-validation-failure
branch
from
6de81f4
to
1d526a6
Compare
| } | ||
| } | ||
|
|
||
| func (*Server) configureBackends(backends map[string]*types.Backend) { |
timoreimann
force-pushed
the
1-3-maintain-sticky-flag-on-lb-method-validation-failure
branch
from
38a2c39
to
1d47418
Compare
We previously did not copy the sticky flag if the load-balancer method validation failed, causing enabled stickiness to be dropped in case of a validation error (which, technically, for us is the same as a load-balancer configuration without an explicitly set method). This change fixes that. A few refactorings and improvements along the way: - Move the frontend and backend configuration steps into separate methods/functions for better testability. - Include the invalid method name in the error value and avoid log duplication. - Add tests for the backend configuration part.
timoreimann
force-pushed
the
1-3-maintain-sticky-flag-on-lb-method-validation-failure
branch
from
1d47418
to
f9839f7
Compare
timoreimann
deleted the
1-3-maintain-sticky-flag-on-lb-method-validation-failure
branch
We previously did not copy the sticky flag if the load-balancer method validation failed, causing enabled stickiness to be dropped in case of a validation error (which, technically, for us is the same as a load-balancer configuration without an explicitly set method). This change fixes that.
A few refactorings and improvements along the way:
Fixes #1568.