Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flaky test pkg/config/tlscfg/TestReload (cert_watcher_test.go) #2644

Closed
yurishkuro opened this issue Nov 24, 2020 · 4 comments · Fixed by #2646 or #2655
Closed

Flaky test pkg/config/tlscfg/TestReload (cert_watcher_test.go) #2644

yurishkuro opened this issue Nov 24, 2020 · 4 comments · Fixed by #2646 or #2655
Labels
bug good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

https://travis-ci.com/github/jaegertracing/jaeger/jobs/445878517#L2150

=== RUN   TestReload
    cert_watcher_test.go:110: 
        	Error Trace:	cert_watcher_test.go:110
        	Error:      	Should be true
        	Test:       	TestReload
        	Messages:   	Failed to find wanted logs. All logs: [{{info 2020-11-23 14:13:36.375563204 +0000 UTC m=+0.010957869  Loading modified certificate undefined } [{certificate 15 0 /tmp/cert.crt414800473 <nil>} {event 15 0 WRITE <nil>}]} {{error 2020-11-23 14:13:36.377159282 +0000 UTC m=+0.012553950  Failed to load certificate undefined } [{certificate 15 0 /tmp/cert.crt414800473 <nil>} {event 15 0 WRITE <nil>} {error 26 0  tls: private key does not match public key}]} {{info 2020-11-23 14:13:36.381814355 +0000 UTC m=+0.017208997  Loading modified certificate undefined } [{certificate 15 0 /tmp/cert.crt414800473 <nil>} {event 15 0 WRITE <nil>}]} {{info 2020-11-23 14:13:36.383533045 +0000 UTC m=+0.018927712  Loaded modified certificate undefined } [{certificate 15 0 /tmp/cert.crt414800473 <nil>} {event 15 0 WRITE <nil>}]} {{info 2020-11-23 14:13:36.38356223 +0000 UTC m=+0.018956858  Loading modified certificate undefined } [{certificate 15 0 /tmp/key.crt548122084 <nil>} {event 15 0 WRITE <nil>}]}]
--- FAIL: TestReload (0.03s)
@yurishkuro yurishkuro added bug help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Nov 24, 2020
@albertteoh
Copy link
Contributor

This is on me.

@yurishkuro
Copy link
Member Author

@yurishkuro yurishkuro reopened this Nov 26, 2020
@albertteoh
Copy link
Contributor

albertteoh commented Nov 26, 2020

Thanks for the heads up @yurishkuro. It looks like the watcher isn't deterministically picking up every state transition of the cert file. In this case, it didn't detect the cert loading failure due to mismatching private & public keys, but only loaded it after both matching private and public keys were added to the cert, hence both updates were successful; which was not observed locally :(.

I'll put in a PR to remove the first assert since that signal is not reliable... and assert exactly on the condition we're waitUntil for. Please let me know if you can spot any holes in this proposed fix.

@albertteoh
Copy link
Contributor

I think we can close this now; at least I haven't seen this flaky test manifest for some time.

I had incorrectly referenced #2654 in the PR #2655, which should have closed this issue. This is now addressed as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
3 participants