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

[bitnami/kafka] change zookeeper keystore pem location #46901

Merged
merged 6 commits into from
Sep 7, 2023

Conversation

toondaey
Copy link
Contributor

@toondaey toondaey commented Sep 1, 2023

Description of the change

I think this necessary because from experience, I find that I keep having to update my local keystore which somehow became more of a burden

Benefits

Eliminate the need to update and change zookeeper.keystore.pem every single time.

Possible drawbacks

None that I can think of.

Applicable issues

Frustrating need to keep updating local zookeeper.keystore.pem evicted.

@github-actions github-actions bot added the kafka label Sep 1, 2023
@github-actions github-actions bot added the triage Triage is needed label Sep 1, 2023
Signed-off-by: Babatunde Aromire <babatunde.aromire@cognite.com>
Signed-off-by: Babatunde Aromire <babatunde.aromire@cognite.com>
@toondaey toondaey changed the title feat: change zookeeper ksystore pem location [bitnami/kafka] change zookeeper ksystore pem location Sep 1, 2023
@github-actions github-actions bot added in-progress and removed triage Triage is needed labels Sep 3, 2023
@bitnami-bot bitnami-bot removed the request for review from javsalgar September 3, 2023 08:56
@andresbono andresbono changed the title [bitnami/kafka] change zookeeper ksystore pem location [bitnami/kafka] change zookeeper keystore pem location Sep 4, 2023
Copy link
Contributor

@andresbono andresbono left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!! The current implementation would also be a problem if the file is mounted as a read-only secret.

I just added a minor suggestion about the style. Could you please check it?

toondaey and others added 2 commits September 4, 2023 18:59
…a.sh

Co-authored-by: Andrés Bono <andresbonojimenez@gmail.com>
Signed-off-by: B'Tunde Aromire <babatunde.aromire@cognite.com>
Signed-off-by: Babatunde Aromire <babatunde.aromire@cognite.com>
@toondaey
Copy link
Contributor Author

toondaey commented Sep 4, 2023

LGTM, thank you!! The current implementation would also be a problem if the file is mounted as a read-only secret.

I just added a minor suggestion about the style. Could you please check it?

Actually that might not because I've you can use cat with a readonly mount into container files. In my local docker setup, I mounted all the other certificates as ro save for those of zookeeper because of the modifications.

I've applied and made some changes to the modifications.

Copy link
Contributor

@andresbono andresbono left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!! The current implementation would also be a problem if the file is mounted as a read-only secret.
I just added a minor suggestion about the style. Could you please check it?

Actually that might not because I've you can use cat with a readonly mount into container files. In my local docker setup, I mounted all the other certificates as ro save for those of zookeeper because of the modifications.

By "current implementation" I meant the implementation before your PR. I was just highlighting that zookeeper.keystore.pem could not be mounted as ro because it needed to be writable. With your PR you are also fixing that. Is that correct?

Please, check my last in-line comment. Thank you!

Signed-off-by: Babatunde Aromire <babatunde.aromire@cognite.com>
Signed-off-by: Babatunde Aromire <babatunde.aromire@cognite.com>
@toondaey
Copy link
Contributor Author

toondaey commented Sep 5, 2023

I hope this is ok? I discovered that KAFKA_KRAFT_CLUSTER_ID had a typo here

@carrodher carrodher added the verify Execute verification workflow for these changes label Sep 5, 2023
Copy link
Contributor

@andresbono andresbono left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@andresbono andresbono merged commit 3bb6838 into bitnami:main Sep 7, 2023
15 checks passed
@github-actions github-actions bot added the solved label Sep 7, 2023
@andresbono
Copy link
Contributor

New container images have been released. Please, pull the latest versions to benefit from your improvement! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kafka solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants