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

[jaeger] Cassandra TLS broken #15

Closed
naseemkullah opened this issue Dec 22, 2019 · 16 comments · Fixed by #239
Closed

[jaeger] Cassandra TLS broken #15

naseemkullah opened this issue Dec 22, 2019 · 16 comments · Fixed by #239
Labels
good first issue Good for newcomers

Comments

@naseemkullah
Copy link
Member

naseemkullah commented Dec 22, 2019

Please skip to #15 (comment)

Original issue has taken a different road.

@naseemkullah naseemkullah added the good first issue Good for newcomers label Dec 22, 2019
@arpitjindal97
Copy link
Contributor

arpitjindal97 commented Jan 9, 2020

While storing TLS credentials in Secret. They are stored in tls format.

If your certificates are self-signed, kubernetes will not accept it and you have to pass --insecure-skip-tls-verify flag while creating secret. In this mode, secret will only hold two files i.e. tls.crt and tls.key.

But, if you see at Jaeger's configuration. It requires CA certificate also. This file will not be there in secret if it self-signed. So, we can't go with the idea of storing TLS credentials in secret.

Correct me, If I'm wrong anywhere :)

@naseemkullah
Copy link
Member Author

You can create a secret with key,crt and ca. Or if you prefer one secret for server key and crt, and another for the ca cert.

Please see https://github.com/kubernetes/ingress-nginx/blob/master/docs/examples/auth/client-certs/README.md#creating-certificate-secrets for further instructions.

@naseemkullah
Copy link
Member Author

One thing is for sure you will want tls.key in a secret, as for the certs, if you want them in a separate configmap, that is your call.

@arpitjindal97
Copy link
Contributor

Okay, we can go with secrets. We can maintain 2 secrets. This will be good

@mmpetarpeshev
Copy link

I'm getting the following error , when enables tls , my secret is encoded with base64 and the contents is as from the example in the docs.(the secret is created with the correct name)

Error: release dealing-tiger failed: Deployment.apps "cassandra-collector" is invalid: [spec.template.spec.containers[0].volumeMounts[0].name: Not found: "cassandra-tls-secret", spec.template.spec.containers[0].volumeMounts[1].name: Not found: "cassandra-tls-secret", spec.template.spec.containers[0].volumeMounts[2].name: Not found: "cassandra-tls-secret"]

@arpitjindal97
Copy link
Contributor

Provide your values.yaml and relevant commands you used to deploy secret and helm chart

@mmpetarpeshev
Copy link

mmpetarpeshev commented Feb 24, 2020

I saw that I'm writing in the wrong issue , my case is with cassandra as storage , we can move the conversation or I can create new one.

values.txt

Here is my secret as the content is base64 encoded :

apiVersion: v1
kind: Secret
metadata:
name: cassandra-tls-secret
namespace: jaeger
data:
commonName: base64
ca-cert.pem: |
base64 blqblq
client-cert.pem: |
base64 blqblqb

client-key.pem: |
base64 bqlblq
cqlshrc: |
base64 ssl section and same as the example from the doc .

###############

The chart is installed in the same namespace as the secret "jaeger".

@arpitjindal97
Copy link
Contributor

okay, I figured out the problem. It's with the chart.

I will create PR to fix

After the fix, you will need to add the config at collector.extraSecretMounts in your values.yaml to mount the cassandra secret with pod. It will not get mounted automatically

example:

extraSecretMounts: 
  - name: cassandra-tls-secret
    mountPath: /cassandra-tls
    subPath: ""
    secretName: cassandra-tls-secret
    readOnly: true

same goes for query also, you will need to add config.

@mmpetarpeshev
Copy link

Great , thanks , let me know , when the fix is merged and ready to use.

@mmpetarpeshev
Copy link

I think the schema job also needs to be fixed.

@naseemkullah
Copy link
Member Author

Sorry you are experiencing this @mmpetarpeshev and thanks for looking into this @arpitjindal97.

Hi @Pehesi97 As the last contributor to work on the Cassandra TLS feature, could you please comment? Does this work for you?

@arpitjindal97
Copy link
Contributor

arpitjindal97 commented Feb 24, 2020

@naseemkullah

cassandra schema job doesn't have TLS support

https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/cassandra/schema/create.sh

How should we proceed ? I'm confused

@naseemkullah
Copy link
Member Author

@arpitjindal97 according to: https://www.jaegertracing.io/docs/1.17/deployment/#tls-support

cqlshrc file should contain:

[ssl]
certfile = ~/.cassandra/ca-cert
userkey = ~/.cassandra/client-key
usercert = ~/.cassandra/client-cert

And in the chart it's here:

- name: {{ .Values.storage.cassandra.tls.secretName }}
mountPath: "/root/.cassandra/cqlshrc"
subPath: "cqlshrc"
readOnly: true

@naseemkullah naseemkullah changed the title [jaeger] Update recommended TLS enabled ES approach to use secret instead of configmap [jaeger] Cassandra TLS broken Feb 26, 2020
@mmpetarpeshev
Copy link

mmpetarpeshev commented Feb 26, 2020

Any update on the PR approval or the issue ? I tried to run the chart with changes from the PR and if I'm not in mistake with configurations or values, there is still issue with the mounts and secrets.

@naseemkullah
Copy link
Member Author

naseemkullah commented Aug 24, 2020

Looks like #145 should fix the issue @mmpetarpeshev @arpitjindal97 PTAL

@sshah90
Copy link
Contributor

sshah90 commented Apr 26, 2021

I am getting the same error shown in comment.

is it a bug in Jaeger's helm chart? I don't see volumes code if tls enable.

Schema Job missing volumes block if tls enable

Same with Query, Ingester, and Collector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants