Prevent x509_cert from hanging on UDP connection #9323
Conversation
|
@akrantz01 Does udp:// mean DTLS(rfc6347) after this PR ? (udp:// only works with DTLS servers and not with some homegrown tls on top of udp socket). Maybe |
|
That'd probably be good to mention. However, there shouldn't be any additional problems with homegrown implementations over UDP since previously, it fails to send a packet if you configure it to pull from a UDP address as the standard library isn't equipped to handle it. |
| m := &X509Cert{ | ||
| Sources: []string{"udp://" + listener.Addr().String()}, | ||
| } |
There was a problem hiding this comment.
You need to set the logging facility here or you will get segfaults (that are the failing CI tests you see):
| m := &X509Cert{ | |
| Sources: []string{"udp://" + listener.Addr().String()}, | |
| } | |
| m := &X509Cert{ | |
| Sources: []string{"udp://" + listener.Addr().String()}, | |
| Log: testutil.Logger{}, | |
| } |
There was a problem hiding this comment.
@akrantz01 thanks for looking into this. The code looks quite good and I added a comment to fix the failing CI tests. Let me know if I can be of any further help.
|
|
||
| go func() { | ||
| _, err := listener.Accept() | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
You shall not use any require methods that terminate (i.e. call require.Fail internally) in goroutines as this will only end the goroutine and not the test. See the go release notes for reference also describing a potential workaround.
In this specific instance I think just ignoring the error and quitting the goroutine might be sufficient as the test relying on the accept will probably fail anyway...
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
(cherry picked from commit 32d4234)
Required for all PRs:
resolves #9310
Fixes the bug where the
x509_certplugin would hang indefinitely when you try to gather certificate metrics from a UDP address. This was done by adding UDP support to the plugin using the pion/dtls library.Note: #9289 should be merged before this PR since all URLs are currently parsed as globs causing telegraf to fail to pull the certificate. Similarly, theRebased to master.TestGatherUDPCerttest will fail because of this.