Allow property security-inter-broker-protocol #85

cawright-rh · 2024-06-11T19:38:21Z

Description

Please provide a meaningful description of what this change will do, or is for. Bonus points for including links to
related issues, other PRs, or technical references.

This change is to allow for the kafka property security.inter.broker.protocol to be used by the broker pods. Currently, koperator will create the property inter.broker.listener.name, which is mutually exclusive with security.inter.broker.protocol.

see kafka documentation on the property: https://kafka.apache.org/documentation/#brokerconfigs_security.inter.broker.protocol

Note that by not including a description, you are asking reviewers to do extra work to understand the context of this
change, which may lead to your PR taking much longer to review, or result in it not being reviewed at all.

Type of Change

Checklist

I have read the contributing guidelines
Existing issues have been referenced (where applicable)
I have verified this change is not present in other open pull requests
Functionality is documented
All code style checks pass
New code contribution is covered by automated tests
All new and existing tests pass

pkg/resources/kafka/kafka.go

dobrerazvan · 2024-06-27T12:34:00Z

Looks good to me, just to rebase to have the latest changes.

…inistration and cc connection

dobrerazvan · 2024-08-08T12:19:37Z

api/v1beta1/kafkacluster_types.go

@@ -748,6 +748,7 @@ type CommonListenerSpec struct {
 	// At least one of the listeners should have this flag enabled
 	// +optional
 	UsedForInnerBrokerCommunication bool `json:"usedForInnerBrokerCommunication"`
+	UsedForKafkaAdminCommunication  bool `json:"usedForKafkaAdminCommunication,omitempty"`


Add description for when this flag is useful.

dobrerazvan · 2024-08-08T12:21:11Z

pkg/util/client/common.go

@@ -37,11 +37,18 @@ func UseSSL(cluster *v1beta1.KafkaCluster) bool {

 func getContainerPortForInnerCom(internalListeners []v1beta1.InternalListenerConfig, extListeners []v1beta1.ExternalListenerConfig) int32 {
 	for _, val := range internalListeners {
+		if val.UsedForKafkaAdminCommunication { // Cannot Currently update the CRD as I do not have permissions


CRDs are not updated in all envs, is this still needed?

I forgot to clean up this comment, the conditional statement is still needed though, I will add a comment explaining the use. The summary is, having a separate flag to indicate which port to use for the koperator and CruiseControl logic is valuable, as, it normally just uses UsedForInnerBrokerCommunication port, which may not be a port CC or Koperator has access to.

pkg/util/client/common.go

azun · 2024-08-08T12:37:20Z

pkg/resources/kafka/configmap.go

@@ -88,8 +88,10 @@ func (r *Reconciler) getConfigProperties(bConfig *v1beta1.BrokerConfig, id int32
 	}

 	// Add Cruise Control Metrics Reporter configuration
-	if err := config.Set(kafkautils.CruiseControlConfigMetricsReporters, "com.linkedin.kafka.cruisecontrol.metricsreporter.CruiseControlMetricsReporter"); err != nil {
-		log.Error(err, fmt.Sprintf("setting '%s' in broker configuration resulted an error", kafkautils.CruiseControlConfigMetricsReporters))
+	if !strings.Contains(r.KafkaCluster.Spec.ReadOnlyConfig, kafkautils.KafkaConfigSecurityInterBrokerProtocol+"=") {


I think you can convert the readonly config to properties using func getBrokerReadOnlyConfig, or some similar logic

Also, update the comment to reflect that the reported is disabled when inter broker communication is secured

Updated to use the getBrokerReadOnlyConfig function and added comment.

azun · 2024-08-09T09:43:27Z

pkg/resources/kafka/configmap.go

@@ -88,8 +88,10 @@ func (r *Reconciler) getConfigProperties(bConfig *v1beta1.BrokerConfig, id int32
 	}

 	// Add Cruise Control Metrics Reporter configuration
-	if err := config.Set(kafkautils.CruiseControlConfigMetricsReporters, "com.linkedin.kafka.cruisecontrol.metricsreporter.CruiseControlMetricsReporter"); err != nil {
-		log.Error(err, fmt.Sprintf("setting '%s' in broker configuration resulted an error", kafkautils.CruiseControlConfigMetricsReporters))
+	if !strings.Contains(r.KafkaCluster.Spec.ReadOnlyConfig, kafkautils.KafkaConfigSecurityInterBrokerProtocol+"=") {


Also, update the comment to reflect that the reported is disabled when inter broker communication is secured

azun · 2024-08-09T09:49:57Z

pkg/resources/kafka/configmap.go

@@ -292,9 +296,12 @@ func generateListenerSpecificConfig(l *v1beta1.ListenersConfig, serverPasses map
 	if err := config.Set(kafkautils.KafkaConfigListenerSecurityProtocolMap, securityProtocolMapConfig); err != nil {
 		log.Error(err, fmt.Sprintf("setting '%s' parameter in broker configuration resulted an error", kafkautils.KafkaConfigListenerSecurityProtocolMap))
 	}
-	if err := config.Set(kafkautils.KafkaConfigInterBrokerListenerName, interBrokerListenerName); err != nil {
-		log.Error(err, fmt.Sprintf("setting '%s' parameter in broker configuration resulted an error", kafkautils.KafkaConfigInterBrokerListenerName))
+	if !strings.Contains(r, kafkautils.KafkaConfigSecurityInterBrokerProtocol+"=") {


This means the inter broker listener needs to be explicitly specified through the readonly config right?

yes that is correct, it is a kafka property similar, but conflicting with, inter.broker.listener.name and inter.broker.listener.protocol

…t test - security_inter_broker_protocol_Set

* Fix flaky test by deleting nodeports explicitly (#67) * Upgrade Kafka to 3.6.0 (#69) * Upgrade dependencies * Fix wrong port on expectEnvoyWithConfigAz2Tls test (#70) * Upgrade Kafka to 3.6.1 (#71) Co-authored-by: Petruț™ <cpetrache@adobe.com> * Upgrade Kafka image to use Java v21 (#72) * Added arm64 to docker build platforms (#73) * Added arm64 to docker build platforms * Regenerated headers for 2024 * Upgrading Kafka to 3.7.0 (#77) * Update codeql-analysis.yml (#78) * [INTERNAL] Create uniq leader ID per operator deployment (#76) * [INTERNAL] Get watched namespaces from env variable (#75) (cherry picked from commit de6500b) * [CORE-106517] Fix outdated config in the sample (#83) * Cross-compile koperator for arm and intel. (#84) * Adding Contour Ingress support (#82) * Allow property security-inter-broker-protocol (#85) * adding the ability to use security-inter-broker-protocol in koperator * updating util.go to remove _ for generated names * adding replace all for external listener port name * fixing other places where externallistener name is used to not have _ * adding an alternative way to identify which port to use for kafka administration and cc connection * taking out comments for pr push * fixing kafka crd * setting omitempty so it will not be required * adding generated crds * adding comments with context for new flag UsedForKafkaAdminCommunication * Use getBrokerReadOnlyConfig function to get properties and update unit test - security_inter_broker_protocol_Set * Update crds to match generated manifest --------- Co-authored-by: Cameron Wright <red82277@adobe.com> Co-authored-by: Ha Van <red83362@adobe.com> --------- Co-authored-by: ctrlaltluc <96051211+ctrlaltluc@users.noreply.github.com> Co-authored-by: Adi Muraru <amuraru@adobe.com> Co-authored-by: Razvan Dobre <dobre@adobe.com> Co-authored-by: Cristian-Petrut Petrache <cristianpetrache@gmail.com> Co-authored-by: Petruț™ <cpetrache@adobe.com> Co-authored-by: Adrian Muraru <adi.muraru@gmail.com> Co-authored-by: Adrian <1664229+azun@users.noreply.github.com> Co-authored-by: aguzovatii <guzovatii.anatolii@gmail.com> Co-authored-by: cawright-rh <cawright@redhat.com> Co-authored-by: Cameron Wright <red82277@adobe.com> Co-authored-by: Ha Van <red83362@adobe.com> Co-authored-by: Daniel Vaseekaran <red10447@adobe.com>

* Fix flaky test by deleting nodeports explicitly (#67) * Upgrade Kafka to 3.6.0 (#69) * Upgrade dependencies * Fix wrong port on expectEnvoyWithConfigAz2Tls test (#70) * Upgrade Kafka to 3.6.1 (#71) Co-authored-by: Petruț™ <cpetrache@adobe.com> * Upgrade Kafka image to use Java v21 (#72) * Added arm64 to docker build platforms (#73) * Added arm64 to docker build platforms * Regenerated headers for 2024 * Upgrading Kafka to 3.7.0 (#77) * Update codeql-analysis.yml (#78) * [INTERNAL] Create uniq leader ID per operator deployment (#76) * [INTERNAL] Get watched namespaces from env variable (#75) (cherry picked from commit de6500b) * [CORE-106517] Fix outdated config in the sample (#83) * Cross-compile koperator for arm and intel. (#84) * Adding Contour Ingress support (#82) * Allow property security-inter-broker-protocol (#85) * adding the ability to use security-inter-broker-protocol in koperator * updating util.go to remove _ for generated names * adding replace all for external listener port name * fixing other places where externallistener name is used to not have _ * adding an alternative way to identify which port to use for kafka administration and cc connection * taking out comments for pr push * fixing kafka crd * setting omitempty so it will not be required * adding generated crds * adding comments with context for new flag UsedForKafkaAdminCommunication * Use getBrokerReadOnlyConfig function to get properties and update unit test - security_inter_broker_protocol_Set * Update crds to match generated manifest --------- Co-authored-by: Cameron Wright <red82277@adobe.com> Co-authored-by: Ha Van <red83362@adobe.com> * Revert "Allow concurrent broker restarts from same AZ (broker rack) (#62)" This reverts commit 514fa07. * Fixed build issues * Fix TestGenerateBrokerConfig * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added watch namesapces * Added tmate for debugging * Added tmate for debugging * Added tmate for debugging * Added enabled projectcontour helm install * Enabled cloud-provider-kind * Added ProjectContour cluster role * updated certificate name * updated certificate name * Run without SSL * Removing Project Contour * Adding cloud-provider-kind * Removing cloud-provider - manually adding during test * trigger test * Remove SnpshotClusterAndCompare * Increased log length for Snapshot and Compare * Re-Add Snapshot and compare * Increased log length for Snapshot and Compare * Increased log length even more * Add Uninstall Contour CRDs * Re-Add KafkaCluster_SSL Tests * Removing BanzaiCloud Helm Chart from list of repos * pushing up latest go.sum * Clean up Merge * Enabling Tmate to debug e2e Test * Revert Cert Changes * Revert "Revert Cert Changes" This reverts commit 5c5b19c. * Enable sslClientAuth * trigger test * WIP: Fix Listener Config * Clean up test case results - tc-1 * Clean up test case results - tc-2 * Updated Kraft Test Cases * Cleanup Linting Issues * Remove Tmate Debugger * Run Kraft CLuster E2E * Add kraft e2e test * Revert Test --------- Co-authored-by: ctrlaltluc <96051211+ctrlaltluc@users.noreply.github.com> Co-authored-by: Adi Muraru <amuraru@adobe.com> Co-authored-by: Razvan Dobre <dobre@adobe.com> Co-authored-by: Cristian-Petrut Petrache <cristianpetrache@gmail.com> Co-authored-by: Petruț™ <cpetrache@adobe.com> Co-authored-by: Adrian Muraru <adi.muraru@gmail.com> Co-authored-by: Adrian <1664229+azun@users.noreply.github.com> Co-authored-by: aguzovatii <guzovatii.anatolii@gmail.com> Co-authored-by: cawright-rh <cawright@redhat.com> Co-authored-by: Cameron Wright <red82277@adobe.com> Co-authored-by: Ha Van <red83362@adobe.com> Co-authored-by: Daniel Vaseekaran <red10447@adobe.com>

* Fix flaky test by deleting nodeports explicitly (#67) * Upgrade Kafka to 3.6.0 (#69) * Upgrade dependencies * Fix wrong port on expectEnvoyWithConfigAz2Tls test (#70) * Upgrade Kafka to 3.6.1 (#71) Co-authored-by: Petruț™ <cpetrache@adobe.com> * Upgrade Kafka image to use Java v21 (#72) * Added arm64 to docker build platforms (#73) * Added arm64 to docker build platforms * Regenerated headers for 2024 * Upgrading Kafka to 3.7.0 (#77) * Update codeql-analysis.yml (#78) * [INTERNAL] Create uniq leader ID per operator deployment (#76) * [INTERNAL] Get watched namespaces from env variable (#75) (cherry picked from commit de6500b) * [CORE-106517] Fix outdated config in the sample (#83) * Cross-compile koperator for arm and intel. (#84) * Adding Contour Ingress support (#82) * Allow property security-inter-broker-protocol (#85) * adding the ability to use security-inter-broker-protocol in koperator * updating util.go to remove _ for generated names * adding replace all for external listener port name * fixing other places where externallistener name is used to not have _ * adding an alternative way to identify which port to use for kafka administration and cc connection * taking out comments for pr push * fixing kafka crd * setting omitempty so it will not be required * adding generated crds * adding comments with context for new flag UsedForKafkaAdminCommunication * Use getBrokerReadOnlyConfig function to get properties and update unit test - security_inter_broker_protocol_Set * Update crds to match generated manifest --------- Co-authored-by: Cameron Wright <red82277@adobe.com> Co-authored-by: Ha Van <red83362@adobe.com> * Revert "Allow concurrent broker restarts from same AZ (broker rack) (#62)" This reverts commit 514fa07. * Fixed build issues * Fix TestGenerateBrokerConfig * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added LoadBalancer for Kind E2E test cluster * Added watch namesapces * Added tmate for debugging * Added tmate for debugging * Added tmate for debugging * Added enabled projectcontour helm install * Enabled cloud-provider-kind * Added ProjectContour cluster role * updated certificate name * updated certificate name * Run without SSL * Removing Project Contour * Adding cloud-provider-kind * Removing cloud-provider - manually adding during test * trigger test * Remove SnpshotClusterAndCompare * Increased log length for Snapshot and Compare * Re-Add Snapshot and compare * Increased log length for Snapshot and Compare * Increased log length even more * Add Uninstall Contour CRDs * Re-Add KafkaCluster_SSL Tests * Removing BanzaiCloud Helm Chart from list of repos * pushing up latest go.sum * Clean up Merge * Enabling Tmate to debug e2e Test * Revert Cert Changes * Revert "Revert Cert Changes" This reverts commit 5c5b19c. * Enable sslClientAuth * trigger test * WIP: Fix Listener Config * Clean up test case results - tc-1 * Clean up test case results - tc-2 * Updated Kraft Test Cases * Cleanup Linting Issues * Remove Tmate Debugger * Run Kraft CLuster E2E * Increate Timeout to allow pod termination * Trigger Test * Added Debugger * Fix App Labels for Controllers * Revert image upate * Revert "Fix App Labels for Controllers" This reverts commit a3cf8a5. * Include Broker/Controller Labels for Headless SVC Selector * Logic for controller listener * add controller service * Added Headless-Controller-SVC Labels * Fix controller addresses and labels for brokers * Empty commit to trigger e2e * Set up kafka-3 as controller only for troubleshooting * Empty commit to trigger e2e * Use controller address for JMXTemplate * Update uninstall timeout to 600s * fix lint * fix lint * fix lint * Enable TMate Debugger * Trigger E2E * Updated BrokerIdLabelkey * Updated BrokerIdLabelkey * Check for Kraft mode when setting the controller listener * Check for Kraft mode when setting the controller listener * Disable tmate from e2e test * adding //nolint:unparam to testProduceConsumeInternal func now that it is used twice * moving the //nolint:unparam to the none-ssl version * Fixed BrokerLabel Test * Add additional test cases for TestGetBrokerLabels * Add additional test cases for TestGetBrokerLabels * commenting out broker-1 in test to fix kraft test * adding conditional to check if kraft mode is enabled before selecting which expected results in test --------- Co-authored-by: ctrlaltluc <96051211+ctrlaltluc@users.noreply.github.com> Co-authored-by: Adi Muraru <amuraru@adobe.com> Co-authored-by: Razvan Dobre <dobre@adobe.com> Co-authored-by: Cristian-Petrut Petrache <cristianpetrache@gmail.com> Co-authored-by: Petruț™ <cpetrache@adobe.com> Co-authored-by: Adrian Muraru <adi.muraru@gmail.com> Co-authored-by: Adrian <1664229+azun@users.noreply.github.com> Co-authored-by: aguzovatii <guzovatii.anatolii@gmail.com> Co-authored-by: cawright-rh <cawright@redhat.com> Co-authored-by: Cameron Wright <red82277@adobe.com> Co-authored-by: Ha Van <red83362@adobe.com> Co-authored-by: Daniel Vaseekaran <red10447@adobe.com> Co-authored-by: Ha Van <168012087+musubi7726@users.noreply.github.com>

cawright-rh marked this pull request as draft June 11, 2024 19:46

dobrerazvan reviewed Jun 27, 2024

View reviewed changes

pkg/resources/kafka/kafka.go Show resolved Hide resolved

Cameron Wright added 6 commits June 27, 2024 10:13

adding the ability to use security-inter-broker-protocol in koperator

324a161

updating util.go to remove _ for generated names

6e7700e

adding replace all for external listener port name

5323ce9

fixing other places where externallistener name is used to not have _

24b9308

adding an alternative way to identify which port to use for kafka adm…

0888bba

…inistration and cc connection

taking out comments for pr push

869549e

cawright-rh force-pushed the allow-security-inter-broker-protocol branch from 51191e5 to 869549e Compare June 27, 2024 14:15

cawright-rh marked this pull request as ready for review June 27, 2024 14:16

Cameron Wright added 3 commits June 27, 2024 10:30

fixing kafka crd

11e6945

setting omitempty so it will not be required

f329251

adding generated crds

aa2c719

dobrerazvan reviewed Aug 8, 2024

View reviewed changes

pkg/util/client/common.go Show resolved Hide resolved

azun reviewed Aug 8, 2024

View reviewed changes

adding comments with context for new flag UsedForKafkaAdminCommunication

3eb4ee5

azun approved these changes Aug 9, 2024

View reviewed changes

dobrerazvan approved these changes Aug 14, 2024

View reviewed changes

Ha Van added 2 commits August 14, 2024 09:43

Use getBrokerReadOnlyConfig function to get properties and update uni…

2e3c514

…t test - security_inter_broker_protocol_Set

Update crds to match generated manifest

bed3f3b

dobrerazvan merged commit 1f0d47f into adobe:master Aug 14, 2024
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow property security-inter-broker-protocol #85

Allow property security-inter-broker-protocol #85

cawright-rh commented Jun 11, 2024 •

edited

Loading

dobrerazvan commented Jun 27, 2024

dobrerazvan Aug 8, 2024

dobrerazvan Aug 8, 2024

cawright-rh Aug 8, 2024 •

edited

Loading

azun Aug 8, 2024 •

edited

Loading

azun Aug 9, 2024

hvan Aug 14, 2024

azun Aug 9, 2024

azun Aug 9, 2024

cawright-rh Aug 9, 2024

Allow property security-inter-broker-protocol #85

Allow property security-inter-broker-protocol #85

Conversation

cawright-rh commented Jun 11, 2024 • edited Loading

Description

Type of Change

Checklist

dobrerazvan commented Jun 27, 2024

dobrerazvan Aug 8, 2024

Choose a reason for hiding this comment

dobrerazvan Aug 8, 2024

Choose a reason for hiding this comment

cawright-rh Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

azun Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

azun Aug 9, 2024

Choose a reason for hiding this comment

hvan Aug 14, 2024

Choose a reason for hiding this comment

azun Aug 9, 2024

Choose a reason for hiding this comment

azun Aug 9, 2024

Choose a reason for hiding this comment

cawright-rh Aug 9, 2024

Choose a reason for hiding this comment

cawright-rh commented Jun 11, 2024 •

edited

Loading

cawright-rh Aug 8, 2024 •

edited

Loading

azun Aug 8, 2024 •

edited

Loading