Use workaround for windows x509.SystemCertPool() #2756
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2756 +/- ##
==========================================
+ Coverage 95.84% 95.88% +0.04%
==========================================
Files 217 218 +1
Lines 9625 9626 +1
==========================================
+ Hits 9225 9230 +5
+ Misses 330 327 -3
+ Partials 70 69 -1
Continue to review full report at Codecov.
|
|
Hi @yurishkuro Being very new to golang, I am confused about what changes we need here, would be very helpful if you can guide me. |
There was a problem hiding this comment.
So similar to how you split Windows and non-Windows logic in the new files, but you only implemented a narrow function there. I would start with L89
certPool, err := systemCertPool()
and basically implement this function differently in Win/Linux. Note that this is currently a pointer to a function, so the linux file will be simply
var systemCertPool = x509.SystemCertPool // to allow overriding in unit test
while in Windows file you can do whatever else is needed
Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: Ashmita Bohara <[email protected]>
Signed-off-by: Ashmita Bohara <[email protected]>
mergify
bot
dismissed
jpkrohling’s stale review
Pull request has been modified.
Signed-off-by: Ashmita Bohara <[email protected]>
|
Nice feedbacks Yuri. I can feel the redundant error checking I was doing before. Much cleaner now. |
|
I'm running version 1.22.0 but I still get failed to load TLS config: failed to load CA CertPool: failed to load SystemCertPool: crypto/x509: system root pool is not available on Window. Specifying --reporter.grpc.tls.ca with an appropriate .pem file fixes it |
|
Hi @roederja2 Thank you for reporting the issue. We will look into it. |
|
Thanks. For completeness these are my arguments to jaeger-agent: works: |
@DuckyC make sure you run agent with parameters: I had troubles when running agent on Windows with additional arguments |
|
Yeah, I ended up having to do that. But the CA is already trusted by the default certificate store in windows. |
Tests in vault_test.go had the following :
```go
// This call will throw an error on Windows systems because of the of
// the call x509.SystemCertPool() because system root pool is not
// available on Windows so ignore the error for when the tests are run
// on the Windows platform during CI
_ = target.Init(m)
```
As of Go 1.18 this is not the case for Windows anymore and
we can instead enforce error checking. References:
* golang/go#16736
* golang/go#18609
* rancher/system-agent#84
* jaegertracing/jaeger#2756
Given Dapr depends on Go 1.19, we can enforce tests on `Init` result
and remove this comment.
While enforcing error checking we notice that the code above was
actually hiding errors in the test setup. Component initialization was
ending prematurely due to those errors and the test code was wrongfully
testing for the behavior of a component that has not been successfully
initialized. This is also addressed in this PR.
Closes dapr#2330.
Signed-off-by: Tiago Alves Macambira <[email protected]>
Signed-off-by: Ashmita Bohara [email protected]
Which problem is this PR solving?
Fixes #2550
Short description of the changes
Doing exactly same as sensu/sensu-go#4018