Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[incubator/kafka] SSL support - Disable env from secret when secret is mounted #7693

Merged
merged 1 commit into from
Sep 12, 2018

Conversation

kaarolch
Copy link
Contributor

@kaarolch kaarolch commented Sep 12, 2018

What this PR does / why we need it:
This PR allow to mount binary files like jkms during kafka deployment. This PR allow you to enable SSL inside kafka with jks. The previous version of charts by default try to read environment variables and also mount files from secret. When the secrets include some binary data container throw error during start:

writing syncT run type caused \"write parent: broken pipe\"

Because we didn't found any use case when someone needs to mount files from secret and in the same time load it as env. variable, the proposed changes do not add env. variable when mountPath was specified.
After this PR we can use secrets with jks or pem files and configurationOverrides

 configurationOverrides:
    ....
    "ssl.keystore.filename": server.jks
    "ssl.keystore.credentials": server.keystore.txt
    "ssl.truststore.filename": ca_test.truststore.jks
    "ssl.truststore.credentials": ca_test.truststore.txt

to enable SSL support.

**Which issue this PR fixes: #3951

Special notes for your reviewer:
If there is a special case why secrests need to be mounted and loaded as env variable we can propose refactoring of secrets section in helm:

## Useful if using any custom authorizer
## Pass in some secrets to use (if required)
# secrets:
# - name: myKafkaSecret
#   keys:
#     - name: username
#       type: file
#     - name: password
#       type: value
#   # mountPath: /opt/kafka/secret

When type is defined as file we can skip add env variable. Unfortunately this change broke compatibility with previous version.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 12, 2018
@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 12, 2018
Signed-off-by: Karol Chrapek <kchrapek@novomatic-tech.com>
@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 12, 2018
@kaarolch
Copy link
Contributor Author

/assign @benjigoldberg

@benjigoldberg
Copy link
Collaborator

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 12, 2018
@benjigoldberg
Copy link
Collaborator

@kaarolch this is a nice catch and fix. Thank you for taking the time to submit a patch for this. I agree with you, I can't think of any obvious reason why you would need both and they conflict if used together. LGTM.

@benjigoldberg
Copy link
Collaborator

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjigoldberg, kaarolch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2018
@k8s-ci-robot k8s-ci-robot merged commit 9597878 into helm:master Sep 12, 2018
@kaarolch kaarolch deleted the feature/kafka_ssl_support branch September 20, 2018 19:36
jicowan pushed a commit to jicowan/charts that referenced this pull request Oct 2, 2018
…7693)

Signed-off-by: Karol Chrapek <kchrapek@novomatic-tech.com>
Signed-off-by: jenkin-x <jicowan@hotmail.com>
Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
…7693)

Signed-off-by: Karol Chrapek <kchrapek@novomatic-tech.com>
Signed-off-by: Jakob Niggel <info@jakobniggel.de>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
…7693)

Signed-off-by: Karol Chrapek <kchrapek@novomatic-tech.com>
@encryptblockr
Copy link

encryptblockr commented Jan 22, 2020

@kaarolch
you mind sharing your values.yaml and possibly statefulset.yaml files you used to setup SSL for kafka?
even if it requires replacing sensitive info with dummy values for security/privacy

@amitsadafule
Copy link

In case anybody wants to setup SSL auth, I have documented the steps on below link
https://medium.com/inspiredbrilliance/helm-incubator-kafka-setup-with-ssl-auth-7cf6fbe333d4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[incubator/kafka] support SSL
6 participants