consul/connect: in-place update service definition when connect upstreams are modified #10776
Conversation
…eams are modified This PR fixes a bug where modifying the upstreams of a Connect sidecar proxy would not result Consul applying the changes, unless an additional change to the job would trigger a task replacement (thus replacing the service definition). The fix is to check if upstreams have been modified between Nomad's view of the sidecar service definition, and the service definition for the sidecar that is actually registered in Consul. Fixes #8754
| return true | ||
| case A.MeshGateway.Mode != B.MeshGateway.Mode: | ||
| return true | ||
| case !reflect.DeepEqual(A.Config, B.Config): |
There was a problem hiding this comment.
It looks like this big switch/case is a performance optimization to avoid doing reflect.DeepEqual on the api.Upstream if we can avoid it, but from a maintenance perspective this seems like it's brittle. If we upgrade the Consul library and they add new fields this check will silently break.
(Alternately, is there maybe a way we can write a unit test that automatically gives us non-default values for all fields so that we have a breaking test on upgrade? I don't know of one off the top of my head but that would be really cool.)
There was a problem hiding this comment.
I think you just gave me an excuse to play around with https://blog.golang.org/fuzz-beta 🙂
Using reflect.DeepEqual is similarly dangerous, for example it always returns false if a function is added to the struct fields:
https://play.golang.org/p/Yfu4d2RdTKd
Really thought the only correct thing to do here would be fore Consul to provide equality methods in their api package - they are the ones who define what the thing is and know how it compares.
consul/connect: in-place update service definition when connect upstreams are modified
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR fixes a bug where modifying the upstreams of a Connect sidecar proxy
would not result Consul applying the changes, unless an additional change to
the job would trigger a task replacement (thus replacing the service definition).
The fix is to check if upstreams have been modified between Nomad's view of the
sidecar service definition and the service definition for the sidecar that is
actually registered in Consul during service reconciliation.
Fixes #8754