Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

fix: mount cert and ca to tempo-query #1038

Merged
merged 2 commits into from
Sep 26, 2024
Merged

fix: mount cert and ca to tempo-query #1038

merged 2 commits into from
Sep 26, 2024

Conversation

frzifus
Copy link
Collaborator

@frzifus frzifus commented Sep 25, 2024 •
edited
Loading

Details: https://issues.redhat.com/browse/TRACING-4703

$ k get pods -n chainsaw-replicas       
NAME                                           READY   STATUS    RESTARTS   AGE
minio-679d9f746b-hngng                         1/1     Running   0          14m
tempo-cmpreps-compactor-bd74cd5c7-mlq2z        1/1     Running   0          6m2s
tempo-cmpreps-distributor-6bc6b5856f-dv92t     1/1     Running   0          6m2s
tempo-cmpreps-gateway-5897b57875-6wltl         2/2     Running   0          6m2s
tempo-cmpreps-ingester-0                       1/1     Running   0          6m2s
tempo-cmpreps-querier-7b74976fd4-d62hd         1/1     Running   0          6m2s
tempo-cmpreps-query-frontend-5558ffd89-5krzk   3/3     Running   0          6m2s

@frzifus frzifus force-pushed the fix/tls_tempo-query branch from a69324e to 7ef4fa9 Compare September 25, 2024 14:32
@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.10%. Comparing base (f221206) to head (1b96b3b).

Files with missing lines Patch % Lines
internal/manifests/queryfrontend/query_frontend.go 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
+ Coverage   73.07%   73.10%   +0.02%     
==========================================
  Files         106      106              
  Lines        6623     6630       +7     
==========================================
+ Hits         4840     4847       +7     
  Misses       1493     1493              
  Partials      290      290
Flag Coverage Δ
unittests 73.10% <91.66%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andreasgerstmayr
andreasgerstmayr reviewed Sep 25, 2024
View reviewed changes
internal/manifests/queryfrontend/query_frontend.go Outdated
@@ -48,11 +48,11 @@ func BuildQueryFrontend(params manifestutils.Params) ([]client.Object, error) {

if gates.HTTPEncryption || gates.GRPCEncryption {
caBundleName := naming.SigningCABundleName(tempo.Name)
if err := manifestutils.ConfigureServiceCA(&d.Spec.Template.Spec, caBundleName, 0, 1); err != nil {
if err := manifestutils.ConfigureServiceCA(&d.Spec.Template.Spec, caBundleName, 0, 1, 2); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the jaeger-query container use the certificates?

Not strictly related to this PR, but could we use the container name instead of the index of the container in the deployment?
Because of using the index, the TLS volume mounting broke in the first place :| (because the jaeger-query container was added before the tempo-query container in the deployment spec).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does the jaeger-query container use the certificates?

I assume no. But I will check.

Not strictly related to this PR, but could we use the container name instead of the index of the container in the deployment?
Sure :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andreasgerstmayr I have added a quick patch on top to determine the container index based on the container names.
In the long term, this functionality should probably be provided in manifestutils.

@frzifus
fix: mount cert and ca to tempo-query
73aad47 
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@frzifus frzifus force-pushed the fix/tls_tempo-query branch from 7ef4fa9 to 73aad47 Compare September 26, 2024 09:32
@frzifus
mtls: match container idx based on containername
1b96b3b 
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@frzifus frzifus marked this pull request as ready for review September 26, 2024 10:00
@frzifus frzifus merged commit 0c5313e into grafana:main Sep 26, 2024
11 checks passed
@frzifus frzifus deleted the fix/tls_tempo-query branch September 26, 2024 10:58
@IshwarKanse IshwarKanse mentioned this pull request Sep 27, 2024
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@andreasgerstmayr andreasgerstmayr andreasgerstmayr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frzifus @codecov-commenter @andreasgerstmayr