Allow TLS flags to be disabled #1440
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1440 +/- ##
=======================================
Coverage 87.70% 87.70%
=======================================
Files 93 93
Lines 5838 5839 +1
=======================================
+ Hits 5120 5121 +1
Misses 552 552
Partials 166 166
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Could you please confirm that you ran a manual test like the following?
- On OpenShift, create a simple CR. This should enable TLS by default
- Change the CR to disable TLS
- Check that the CR (and underlying Jaeger config) look sane and without extra options like paths to the certs and keys.
| ca.Update(a.jaeger, commonSpec) | ||
| ca.AddServiceCA(a.jaeger, commonSpec) | ||
| if len(util.FindItem("--collector.grpc.tls.enabled", options)) == 0 { | ||
| tls.Update(a.jaeger, commonSpec, &options) |
There was a problem hiding this comment.
This doesn't feel right: if the option is set, then tls.Update will be skipped (and ca, and ca.AddServiceCA). Why did we have this as part of all reconciliation loops before? What happens when on the first reconciliation we did not have this parameter, causing the operator to add --reporter.grpc.tls.enabled=true, and the user then manually sets this to false?
There was a problem hiding this comment.
Good point, I need to test this case,
I ran manually tests only for the first case, I'll check what happens in the case of an update to the CR. but it seems like you're right.
There was a problem hiding this comment.
Did you test this case?
There was a problem hiding this comment.
I tested it with the last changes I did, it seems to work fine.
|
I did some changes to only touch the flags in case of the TLS certificates, still creating CA on the reconciliation, regarding of the flags. |
| ca.Update(a.jaeger, commonSpec) | ||
| ca.AddServiceCA(a.jaeger, commonSpec) | ||
| if len(util.FindItem("--collector.grpc.tls.enabled", options)) == 0 { | ||
| tls.Update(a.jaeger, commonSpec, &options) |
There was a problem hiding this comment.
Did you test this case?
|
@rubenvp8510 could you please rebase this? |
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
The TLS will be disabled, but all the volumes will be still there. I'll add an E2E test, may be we can do it in another PR? |
jpkrohling
changed the title
Fixes #1438
Signed-off-by: Ruben Vargas [email protected]