fix(auth): propagate MDC across the thread boundary for pipeline execution #560
Conversation
…ution previous change (spinnaker#555) moved orca invocation to an executor thread without propagating MDC context.
| @@ -166,7 +167,20 @@ public void startPipeline(Pipeline pipeline, TriggerSource triggerSource) { | |||
|
|
|||
| private void triggerPipeline(Pipeline pipeline, TriggerSource triggerSource) | |||
| throws RejectedExecutionException { | |||
| executorService.submit(() -> triggerPipelineImpl(pipeline, triggerSource)); | |||
| final Map<String, String> mdc = MDC.getCopyOfContextMap(); | |||
There was a problem hiding this comment.
Curious on your thought process behind this vs propagate()?
There was a problem hiding this comment.
Is there any sort of test we could add around this ...
There was a problem hiding this comment.
I added a test, but then it actually didn't work. The problem is that the test is using a directExecutor and then it actually calls from the same thread... I will add a test but i will just need more time to figure out how to block on that thread in spock...
propagate() doesn't actually carry the MDC across thread boundary, annoyingly - maybe that was the intent but i don't see how it would actually do that short of what I am doing
There was a problem hiding this comment.
propagate() should be able to propagate across threads, it's not going to blindly propagate all MDC but that shouldn't matter?
But you'd need to call propagate() outside the submit() and pass the resulting Callable<> into it.
My go to way of testing something like this would to do an assert within the mock ... roughly something like:
1 * orca.blah() {
assert MDC.get("blah") == "what I expect"
}
There was a problem hiding this comment.
Thanks for tip - added a test and used propagate - much better this way!
| MDC.remove(AuthenticatedRequest.Header.ACCOUNTS.header) | ||
| MDC.remove(AuthenticatedRequest.Header.USER.header) | ||
|
|
||
| // Wait for orce to actually be called (happens on separate thread) |
| try { | ||
| triggerWithCapturedContext.call(); | ||
| } catch (Exception e) { | ||
| // This shouldn't happen since all exceptions are handled in triggerPipelineImpl... |
There was a problem hiding this comment.
uhh ... can we just have a test that asserts exceptions are caught and not sprinkle this all over the place?
There was a problem hiding this comment.
assuming the above ... then can we just pass triggerWithCapturedContext into executorService.submit()?
There was a problem hiding this comment.
yeah, that's much nicer, thanks!
…ary for pipeline execution
previous change (#555) moved orca invocation
to an executor thread without propagating MDC context.