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

Deploy trusted CA config map in OpenShift when agent injected into a … #1110

Merged
merged 3 commits into from
Jul 3, 2020
Merged

Deploy trusted CA config map in OpenShift when agent injected into a … #1110

merged 3 commits into from
Jul 3, 2020

Conversation

objectiser
Copy link
Contributor

…different namespace than the jaeger instance

Resolves #1092

Signed-off-by: Gary Brown [email protected]

…different namespace than the jaeger instance

Signed-off-by: Gary Brown <[email protected]>
@objectiser objectiser requested a review from jpkrohling July 1, 2020 09:44
@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #1110 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
+ Coverage   88.00%   88.02%   +0.02%     
==========================================
  Files          86       86              
  Lines        5254     5263       +9     
==========================================
+ Hits         4624     4633       +9     
  Misses        466      466              
  Partials      164      164              
Impacted Files Coverage Δ
pkg/autodetect/main.go 82.35% <100.00%> (ø)
pkg/config/ca/ca.go 100.00% <100.00%> (ø)
pkg/inject/sidecar.go 94.38% <100.00%> (+0.20%) ⬆️
pkg/strategy/all_in_one.go 91.11% <100.00%> (ø)
pkg/strategy/production.go 97.22% <100.00%> (ø)
pkg/strategy/streaming.go 97.01% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ad3f0...80e3001. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just need a confirmation that the reconciliation logic should really be prematurely finished when the bundle CA can't get created.

@@ -124,6 +125,25 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re

jaeger := inject.Select(dep, ns, jaegers)
if jaeger != nil && jaeger.GetDeletionTimestamp() == nil {
if jaeger.Namespace != request.Namespace {
log.WithFields(log.Fields{
"jaeger namespace": jaeger.Namespace,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For someone grepping the logs, it might be better to have jaeger-namespace or jaeger.namespace than jaeger namespace.

log.WithFields(log.Fields{
"jaeger namespace": jaeger.Namespace,
"app namespace": request.Namespace,
}).Info("different namespaces, so check whether trusted CA bundle configmap should be created")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Info/Debug/ ?

@objectiser
Copy link
Contributor Author

@jpkrohling Updated - don't think the otel-es test TestElasticSearchSuite/TestSparkDependenciesES is anything related to this PR.

@jpkrohling
Copy link
Contributor

No, it also failed on #1111 and #1105.

@jpkrohling jpkrohling merged commit f16e1df into jaegertracing:master Jul 3, 2020
jpkrohling pushed a commit to jpkrohling/jaeger-operator that referenced this pull request Dec 15, 2020
…gertracing#1110)

Deploy trusted CA config map in OpenShift when agent injected into a different namespace than the jaeger instance

Signed-off-by: Gary Brown <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConfigMap not found when injecting sidecar agent on OpenShift
3 participants