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.

Sign up for GitHub

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

Jump to bottom

Kafka SSL with KRaft #951

Closed
wants to merge 2 commits into from
Closed

Kafka SSL with KRaft #951

wants to merge 2 commits into from

Conversation

nasark
Copy link
Member

@nasark nasark commented Mar 27, 2023 •
edited
Loading

@nasark nasark requested a review from bdunne as a code owner March 27, 2023 13:27
@miq-bot miq-bot added the wip label Mar 27, 2023
@nasark nasark force-pushed the kafka_ssl_with_kraft branch from e8c2bd2 to 8ab8296 Compare March 31, 2023 19:24
This was referenced Mar 31, 2023
Merged
Merged
Closed
Closed
nasark
nasark commented Mar 31, 2023
View reviewed changes
manageiq-operator/api/v1alpha1/helpers/miq-components/kafka.go Outdated
Comment on lines 274 to 277
corev1.EnvVar{
Name: "KAFKA_CFG_OFFSETS_TOPIC_NUM_PARTITIONS",
Value: "25",
},
Copy link
Member Author

Choose a reason for hiding this comment

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

See discussion in #936. If we move ahead with this partition limit then #936 can be closed.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should expose this from the CR? cc @agrare ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed this since the partition limit is no longer necessary as #936 is now merged

nasark
nasark commented Mar 31, 2023
View reviewed changes
manageiq-operator/api/v1alpha1/helpers/miq-components/kafka.go
Comment on lines +368 to +370
Spec: corev1.PodSpec{
Hostname: "kafka",
},
Copy link
Member Author

@nasark nasark Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This was needed because by default the hostname FQDN is the pod name however, for SSL it needs to match the CN used in the certs. See https://github.com/bitnami/containers/blob/main/bitnami/kafka/README.md#security

@nasark nasark changed the title [WIP] Kafka SSL with KRaft Kafka SSL with KRaft Mar 31, 2023
@nasark
Copy link
Member Author

nasark commented Mar 31, 2023

Dev complete, ready for review

@miq-bot assign @bdunne
@miq-bot add_reviewer @Fryguy
@miq-bot add_label enhancement

@nasark nasark mentioned this pull request Mar 31, 2023
Closed
1 task
@miq-bot miq-bot requested a review from Fryguy March 31, 2023 21:03
@miq-bot miq-bot added enhancement and removed wip labels Mar 31, 2023
@nasark nasark force-pushed the kafka_ssl_with_kraft branch from 8ab8296 to aee7643 Compare April 3, 2023 19:49
@nasark
Copy link
Member Author

nasark commented Apr 3, 2023

Added MESSAGING_SSL_CA and MESSAGING_KEYSTORE env vars as per comment ManageIQ/manageiq#22437 (comment)

Fryguy
Fryguy reviewed Apr 19, 2023
View reviewed changes
manageiq-operator/api/v1alpha1/helpers/miq-components/kafka.go
service.Spec.Ports[0].Name = "kafka"
service.Spec.Ports[0].Port = 9092

service.Spec.Ports = []corev1.ServicePort{
Copy link
Member

Choose a reason for hiding this comment

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

I thikn there are issues with recreating the ports inside the func. @bdunne?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is okay since nothing else should be editing the ports

Fryguy
Fryguy reviewed Apr 19, 2023
View reviewed changes
manageiq-operator/api/v1alpha1/helpers/miq-components/kafka.go Outdated
},
corev1.EnvVar{
Name: "KAFKA_CFG_BROKER_ID",
Value: "1",
Copy link
Member

Choose a reason for hiding this comment

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

Does this number have to be configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

This does not need to be configurable as the value itself doesn't matter and since we are only using one broker

The value should match the voter set in the quorum controller, in this case it is set to 1 to match the voter set here i.e. 1@kafka:9093

See also: https://github.com/bitnami/containers/blob/main/bitnami/kafka/3.4/debian-11/rootfs/opt/bitnami/scripts/libkafka.sh#L254-L264

bdunne
bdunne reviewed May 3, 2023
View reviewed changes
manageiq-operator/api/v1alpha1/helpers/miq-components/kafka.go Outdated
@@ -228,7 +240,51 @@ func KafkaDeployment(cr *miqv1alpha1.ManageIQ, scheme *runtime.Scheme) (*appsv1.
},
Env: []corev1.EnvVar{
corev1.EnvVar{
Name: "KAFKA_BROKER_USER",
Name: "KAFKA_ENABLE_KRAFT",
Copy link
Member

Choose a reason for hiding this comment

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

Please move all of the ENV vars into the reconcile function. You can use addOrUpdateEnvVar like here that way if any of these change or become configurable in the future, they will be updated in the deployment.

bdunne
bdunne reviewed May 3, 2023
View reviewed changes
manageiq-operator/api/v1alpha1/helpers/miq-components/kafka.go Outdated
},
}

if certSecret := InternalCertificatesSecret(cr, client); certSecret.Data["kafka_truststore"] != nil && certSecret.Data["kafka_keystore"] != nil && certSecret.Data["kafka_keystore_pass"] != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This conditional should also move into the reconcile function with the same env var comment above.

@nasark
Copy link
Member Author

nasark commented May 4, 2023

@bdunne I've moved the env vars to the reconcile function, please have another look when possible

@miq-bot
Copy link
Member

miq-bot commented May 4, 2023

Checked commits nasark/manageiq-pods@aee7643~...ba480b3 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@miq-bot miq-bot added the stale label Aug 7, 2023
@miq-bot
Copy link
Member

miq-bot commented Aug 7, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot
Copy link
Member

miq-bot commented Oct 31, 2023

This pull request is not mergeable. Please rebase and repush.

@miq-bot miq-bot closed this Nov 6, 2023
@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne left review comments

@Fryguy Fryguy Fryguy left review comments

Assignees

@bdunne bdunne

Labels
enhancement stale unmergeable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nasark @miq-bot @Fryguy @bdunne