Skip to content

Commit

Permalink
Review comments PM
Browse files Browse the repository at this point in the history
Signed-off-by: Jakub Scholz <[email protected]>
  • Loading branch information
scholzj committed Sep 20, 2024
1 parent 6bc12ef commit 0411ee7
Show file tree
Hide file tree
Showing 9 changed files with 39 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public void setPublishNotReadyAddresses(Boolean publishNotReadyAddresses) {
this.publishNotReadyAddresses = publishNotReadyAddresses;
}

@Description("Configures the template for generating the host names of the individual brokers. " +
@Description("Configures the template for generating the hostnames of the individual brokers. " +
"Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`")
@JsonInclude(JsonInclude.Include.NON_NULL)
public String getHostTemplate() {
Expand All @@ -270,7 +270,7 @@ public void setHostTemplate(String hostTemplate) {
this.hostTemplate = hostTemplate;
}

@Description("Configures the template for generating the advertisedHost names of the individual brokers. " +
@Description("Configures the template for generating the advertised hostnames of the individual brokers. " +
"Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`")
@JsonInclude(JsonInclude.Include.NON_NULL)
public String getAdvertisedHostTemplate() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ private static void validateIngress(Set<String> errors, Set<NodeRef> brokerNodes

if (conf.getBootstrap() == null
|| conf.getBootstrap().getHost() == null) {
errors.add("listener " + listener.getName() + " is missing a bootstrap host name which is required for Ingress based listeners");
errors.add("listener " + listener.getName() + " is missing a bootstrap host property which is required for Ingress based listeners");
}

if (conf.getHostTemplate() != null) {
Expand All @@ -180,14 +180,14 @@ private static void validateIngress(Set<String> errors, Set<NodeRef> brokerNodes
GenericKafkaListenerConfigurationBroker broker = conf.getBrokers().stream().filter(b -> b.getBroker() == node.nodeId()).findFirst().orElse(null);

if (broker == null || broker.getHost() == null) {
errors.add("listener " + listener.getName() + " is missing a broker host name for broker with ID " + node.nodeId() + " which is required for Ingress based listeners");
errors.add("listener " + listener.getName() + " is missing a broker host property for broker with ID " + node.nodeId() + " which is required for Ingress based listeners");
}
}
} else {
errors.add("listener " + listener.getName() + " is missing a broker configuration with host names which is required for Ingress based listeners");
errors.add("listener " + listener.getName() + " is missing a broker configuration with host properties which are required for Ingress based listeners");
}
} else {
errors.add("listener " + listener.getName() + " is missing a configuration with host names which is required for Ingress based listeners");
errors.add("listener " + listener.getName() + " is missing a configuration with host properties which are required for Ingress based listeners");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -642,21 +642,21 @@ public void testIngressListenerHostNames() {
.withTls(true)
.build();

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a configuration with host names which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a configuration with host properties which are required for Ingress based listeners"));

listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
.withBrokers((List<GenericKafkaListenerConfigurationBroker>) null)
.build());

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host name which is required for Ingress based listeners",
"listener ingress is missing a broker configuration with host names which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host property which is required for Ingress based listeners",
"listener ingress is missing a broker configuration with host properties which are required for Ingress based listeners"));

listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
.withHostTemplate("my-host-{nodeIf}")
.withBrokers((List<GenericKafkaListenerConfigurationBroker>) null)
.build());

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host name which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host property which is required for Ingress based listeners"));

listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
.withBootstrap(new GenericKafkaListenerConfigurationBootstrapBuilder()
Expand All @@ -669,9 +669,9 @@ public void testIngressListenerHostNames() {
.build())
.build());

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host name which is required for Ingress based listeners",
"listener ingress is missing a broker host name for broker with ID 0 which is required for Ingress based listeners",
"listener ingress is missing a broker host name for broker with ID 1 which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host property which is required for Ingress based listeners",
"listener ingress is missing a broker host property for broker with ID 0 which is required for Ingress based listeners",
"listener ingress is missing a broker host property for broker with ID 1 which is required for Ingress based listeners"));

listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
.withBootstrap(new GenericKafkaListenerConfigurationBootstrapBuilder()
Expand All @@ -683,7 +683,7 @@ public void testIngressListenerHostNames() {
.build())
.build());

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a broker host name for broker with ID 0 which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, TWO_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a broker host property for broker with ID 0 which is required for Ingress based listeners"));

// Valid configurations
listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
Expand Down Expand Up @@ -735,14 +735,14 @@ public void testIngressListenerHostNamesInNodePools() {
.withTls(true)
.build();

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, NODE_POOL_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a configuration with host names which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, NODE_POOL_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a configuration with host properties which are required for Ingress based listeners"));

listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
.withBrokers((List<GenericKafkaListenerConfigurationBroker>) null)
.build());

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, NODE_POOL_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host name which is required for Ingress based listeners",
"listener ingress is missing a broker configuration with host names which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, NODE_POOL_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host property which is required for Ingress based listeners",
"listener ingress is missing a broker configuration with host properties which are required for Ingress based listeners"));

listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
.withBootstrap(new GenericKafkaListenerConfigurationBootstrapBuilder()
Expand All @@ -758,10 +758,10 @@ public void testIngressListenerHostNamesInNodePools() {
.build())
.build());

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, NODE_POOL_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host name which is required for Ingress based listeners",
"listener ingress is missing a broker host name for broker with ID 1000 which is required for Ingress based listeners",
"listener ingress is missing a broker host name for broker with ID 2000 which is required for Ingress based listeners",
"listener ingress is missing a broker host name for broker with ID 2001 which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, NODE_POOL_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a bootstrap host property which is required for Ingress based listeners",
"listener ingress is missing a broker host property for broker with ID 1000 which is required for Ingress based listeners",
"listener ingress is missing a broker host property for broker with ID 2000 which is required for Ingress based listeners",
"listener ingress is missing a broker host property for broker with ID 2001 which is required for Ingress based listeners"));

listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
.withBootstrap(new GenericKafkaListenerConfigurationBootstrapBuilder()
Expand All @@ -777,7 +777,7 @@ public void testIngressListenerHostNamesInNodePools() {
.build())
.build());

assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, NODE_POOL_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a broker host name for broker with ID 2000 which is required for Ingress based listeners"));
assertThat(ListenersValidator.validateAndGetErrorMessages(Reconciliation.DUMMY_RECONCILIATION, NODE_POOL_NODES, List.of(listener)), containsInAnyOrder("listener ingress is missing a broker host property for broker with ID 2000 which is required for Ingress based listeners"));

listener.setConfiguration(new GenericKafkaListenerConfigurationBuilder()
.withBootstrap(new GenericKafkaListenerConfigurationBootstrapBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ which is `9095` in the following example.
You must specify the hostname used by the bootstrap service using xref:type-GenericKafkaListenerConfigurationBootstrap-reference[`GenericKafkaListenerConfigurationBootstrap`] property.
And you must also specify the hostnames used by the per-broker services
using xref:type-GenericKafkaListenerConfigurationBroker-reference[`GenericKafkaListenerConfigurationBroker`]
and xref:type-GenericKafkaListenerConfiguration-reference[`hostTemplate`] properties.
or xref:type-GenericKafkaListenerConfiguration-reference[`hostTemplate`] properties.
With the `hostTemplate` property, you don't need to specify the configuration for every broker.
+
.Example `ingress` listener configuration
[source,yaml,subs="+attributes"]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,16 +182,16 @@ listeners:
# ...
----

Instead of specifying the `host` field for every broker, you can also use an `hostTemplate` to generate them automatically.
Instead of specifying the `host` property for every broker, you can also use an `hostTemplate` to generate them automatically.
The `hostTemplate` supports the following variables:

* The `{nodeId}` variable will be replaced with the ID of the Kafka node to which the template is applied.
* The `{nodePodName}` variable will be replaced with the Kubernetes pod name for the Kafka node where the template is applied.
* The `{nodeId}` variable is replaced with the ID of the Kafka node to which the template is applied.
* The `{nodePodName}` variable is replaced with the Kubernetes pod name for the Kafka node where the template is applied.

The `hostTemplate` field is applied only to the per-broker `host` fields.
The bootstrap `host` field has to be always specified.
The `hostTemplate` property applies only to per-broker values.
The bootstrap `host` property must always be specified.

.Example `ingress` listener with the `hostTemplate`
.Example `ingress` listener with `hostTemplate` configuration
[source,yaml,subs="+attributes"]
----
#...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,10 @@ listeners:
Instead of specifying the `advertisedHost` field for every broker, you can also use an `advertisedHostTemplate` to generate them automatically.
The `advertisedHostTemplate` supports the following variables:

* The `{nodeId}` variable will be replaced with the ID of the Kafka node to which the template is applied.
* The `{nodePodName}` variable will be replaced with the Kubernetes pod name for the Kafka node where the template is applied.
* The `{nodeId}` variable is replaced with the ID of the Kafka node to which the template is applied.
* The `{nodePodName}` variable is replaced with the Kubernetes pod name for the Kafka node where the template is applied.

.Example of an external `route` listener configured with overrides for advertised addresses using the `advertisedHostTemplate` field.
.Example `route` listener with `advertisedHostTemplate` configuration
[source,yaml,subs="attributes+"]
----
listeners:
Expand Down
4 changes: 2 additions & 2 deletions documentation/modules/appendix_crds.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -505,10 +505,10 @@ This property is used to select the preferred address type, which is checked fir
|Configures whether the service endpoints are considered "ready" even if the Pods themselves are not. Defaults to `false`. This field can not be used with `internal` listeners.
|hostTemplate
|string
|Configures the template for generating the host names of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`.
|Configures the template for generating the hostnames of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`.
|advertisedHostTemplate
|string
|Configures the template for generating the advertisedHost names of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`.
|Configures the template for generating the advertised hostnames of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`.
|====

[id='type-CertAndKeySecretSource-{context}']
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -482,10 +482,10 @@ spec:
description: Configures whether the service endpoints are considered "ready" even if the Pods themselves are not. Defaults to `false`. This field can not be used with `internal` listeners.
hostTemplate:
type: string
description: "Configures the template for generating the host names of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`."
description: "Configures the template for generating the hostnames of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`."
advertisedHostTemplate:
type: string
description: "Configures the template for generating the advertisedHost names of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`."
description: "Configures the template for generating the advertised hostnames of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`."
description: Additional listener configuration.
networkPolicyPeers:
type: array
Expand Down
4 changes: 2 additions & 2 deletions packaging/install/cluster-operator/040-Crd-kafka.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -481,10 +481,10 @@ spec:
description: Configures whether the service endpoints are considered "ready" even if the Pods themselves are not. Defaults to `false`. This field can not be used with `internal` listeners.
hostTemplate:
type: string
description: "Configures the template for generating the host names of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`."
description: "Configures the template for generating the hostnames of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`."
advertisedHostTemplate:
type: string
description: "Configures the template for generating the advertisedHost names of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`."
description: "Configures the template for generating the advertised hostnames of the individual brokers. Valid placeholders that you can use in the template are `{nodeId}` and `{nodePodName}`."
description: Additional listener configuration.
networkPolicyPeers:
type: array
Expand Down

0 comments on commit 0411ee7

Please sign in to comment.