Fix backend reuse #3312
Fix backend reuse #3312
Conversation
server/server.go
Outdated
| @@ -925,7 +925,7 @@ func (s *Server) loadConfig(configurations types.Configurations, globalConfigura | |||
| redirectHandlers[entryPointName] = handlerToUse | |||
| } | |||
| } | |||
| if backends[entryPointName+providerName+frontend.Backend] == nil { | |||
| if backends[entryPointName+providerName+frontend.Hash()] == nil { | |||
There was a problem hiding this comment.
Could this string be stored, and referenced, instead of being recalculated multiple times?
There was a problem hiding this comment.
Yes, that should be doable. I'll just need to make sure the frontend isn't changed between usages of a stored string.
There was a problem hiding this comment.
I was just thinking inside the loop. Instead of calculating .Hash() three times.
entryPointName+providerName+frontend.Hash() in your PR.
There was a problem hiding this comment.
I hadn't checked if shady things where done to the frontend in the loop (although that would also have been a bug if it did).
Implemented in b716cb2b3c892a678d9cac77954dea11ae9439bc.
types/types.go
Outdated
| @@ -188,6 +189,13 @@ type Frontend struct { | |||
| Redirect *Redirect `json:"redirect,omitempty"` | |||
| } | |||
|
|
|||
| // Hash returns the hash value of a Frontend struct. | |||
| func (f *Frontend) Hash() string { | |||
| hash, _ := hashstructure.Hash(f, nil) | |||
There was a problem hiding this comment.
Should handle the error here. If the hash goes sideways, you are going to want to log or potentially return an error.
There was a problem hiding this comment.
Agree. I think I took a shortcut because from te definition of the Hash() it is not able to produce an error with the current definition of the Frontend struct. But that's not very kind on the people doing changes in the future.
I'll create fixes for this (today or tomorrow probably).
There was a problem hiding this comment.
Implemented in d44fde676f86f2d039b2b8e4798f6b25a5d98e3a.
|
Thank you for the approve, @dtomcej. GitHub has detected a conflict in I have refactored my test method to use the new test helpers as well. Fun story. The refactored test helpers have a bug in I had to rebase on master to not make everything completely unreadable. I introduced a new commit (cc69be54446b021ed3bdbbf11d770497d7fa70ae) that fixes the bug in the test helper. Next commit (fb9ed685b943d918e6b1e78659061b0ca88282d2) is my original test case refactored to use the new test helpers. The rest of the commits are as you reviewed them. |
Reveal the bug described in #2323.
What does this PR do?
This fixes the way backends are currently cached leading t some frontend configuration being ignored when re-using same backend (reported in #2323).
Motivation
Not being able to reuse backends in different frontends is unexpected and the workaround results in defining additional, identical backends.
In a current project of mine I could potentially end up with 400 identical backends to work around this.
Fixes #2323
More
Additional Notes
Currently backends are cached under a key generated by
entryPointName+providerName+frontend.Backend. Thefrontend.Backenddoesn't respect the differences present in frontends.I add
Hash()method to theFrontendstruct and use that instead of just the backend name.The Hash method uses github.com/mitchellh/hashstructure which is also used in collector/collector.go (a nice feature of it is being able to ignore some fields when computing the hash).