Repository: kroxylicious/kroxylicious (java)

Issue:
Remove deprecated configuration property bootstrap_servers
The `bootstrap_servers` configuration property was deprecated at 0.10.0.  According to our deprecation rules, this can be removed at 0.13.0

Pull request:
Fix #1871: Remove deprecated configuration property bootstrap_servers

### Type of change

_Select the type of your PR_

- Refactoring


### Description

Remove deprecated configuration property `bootstrap_servers`.  The configuration property was deprecated at `0.10.0` meaning that it is now eligible for removal.

_Please describe your pull request_

### Additional Context

_Why are you making this pull request?_

### Checklist

_Please go through this checklist and make sure all applicable tasks have been done_

- [x] PR raised from a fork of this repository and made from a branch rather than main. 
- [x] Write tests
- [ ] Update documentation
- [ ] Make sure all unit/integration tests pass
- [ ] Make sure all Sonarcloud warnings are addressed or are justifiably ignored.
- [ ] If applicable to the change, [trigger the system test suite](../blob/main/DEV_GUIDE.md#jenkins-pipeline-for-system-tests).  Make sure tests pass.
- [ ] If applicable to the change, [trigger the performance test suite](../blob/main/PERFORMANCE.md#jenkins-pipeline-for-performance). Ensure that any degradations to performance numbers are understood and justified.
- [ ] Ensure the PR references relevant issue(s) so they are closed on merging.
- [x] For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

> **_NOTE:_**  You must be a member of `@kroxylicious/developers` to trigger the system test and performance test suites.  If you are not part of this group, comment on the PR requesting a trigger, tagging `@kroxylicious/developers`.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6227682c..d685c92b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,15 @@ For changes that effect a public API, the [deprecation policy](./DEV_GUIDE.md#de
 Format `<github issue/pr number>: <short description>`.
 
 ## SNAPSHOT
+
+* [#1871](https://github.com/kroxylicious/kroxylicious/issues/1871) Remove deprecated configuration property bootstrap_servers
+
+### Changes, deprecations and removals
+
+* Remove the deprecated configuration property `bootstrap_servers` from the `targetCluster` object. Use `bootstrapServers`
+  instead.
+
+
 ## 0.12.0
 
 * [#2135](https://github.com/kroxylicious/kroxylicious/pull/2135) Require client certificates by default if user supplies downstream trust
diff --git a/kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/config/TargetCluster.java b/kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/config/TargetCluster.java
index 88bc54fa..df4b1943 100644
--- a/kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/config/TargetCluster.java
+++ b/kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/config/TargetCluster.java
@@ -9,10 +9,6 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
 
 import io.kroxylicious.proxy.config.tls.Tls;
@@ -20,31 +16,17 @@ import io.kroxylicious.proxy.service.HostPort;
 
 /**
  * Represents the target (upstream) kafka cluster.
+ *
+ * @param bootstrapServers A list of host/port pairs to use for establishing the initial connection to the target (upstream) Kafka cluster.
+ * @param tls tls configuration if a secure connection is to be used.
  */
-public record TargetCluster(
-                            @JsonProperty("bootstrapServers") String bootstrapServers,
+public record TargetCluster(@JsonProperty(value = "bootstrapServers", required = true) String bootstrapServers,
                             @JsonProperty(value = "tls") Optional<Tls> tls) {
 
-    private static final Logger LOGGER = LoggerFactory.getLogger(TargetCluster.class);
-
-    @JsonCreator
-    public static TargetCluster foo(
-                                    @JsonProperty("bootstrapServers") String bootstrapServers,
-                                    @Deprecated(since = "0.10.0", forRemoval = true) @JsonProperty("bootstrap_servers") String deprecatedBootstrapServers,
-                                    @JsonProperty(value = "tls") Optional<Tls> tls) {
-        if (bootstrapServers == null && deprecatedBootstrapServers == null) {
+    public TargetCluster {
+        if (bootstrapServers == null) {
             throw new IllegalArgumentException("'bootstrapServers' is required in a target cluster.");
         }
-        if (bootstrapServers != null && deprecatedBootstrapServers != null) {
-            throw new IllegalArgumentException("'bootstrapServers' and 'bootstrap_servers' cannot both be specified in a target cluster.");
-        }
-        if (deprecatedBootstrapServers != null) {
-            LOGGER.warn("'bootstrap_servers' in a target cluster is deprecated and will be removed in a future release: It should be changed to 'bootstrapServers'.");
-            return new TargetCluster(deprecatedBootstrapServers, tls);
-        }
-        else {
-            return new TargetCluster(bootstrapServers, tls);
-        }
     }
 
     /**
@@ -66,7 +48,7 @@ public record TargetCluster(
     public String toString() {
         final StringBuilder sb = new StringBuilder("TargetCluster[");
         sb.append("bootstrapServers='").append(bootstrapServers).append('\'');
-        sb.append(", clientTls=").append(tls.map(Tls::toString).orElse(null));
+        sb.append(", tls=").append(tls.map(Tls::toString).orElse(null));
         sb.append(']');
         return sb.toString();
     }


Test patch:
diff --git a/kroxylicious-integration-test-support/src/test/java/io/kroxylicious/proxy/config/ConfigurationTest.java b/kroxylicious-integration-test-support/src/test/java/io/kroxylicious/proxy/config/ConfigurationTest.java
index 680b287d..e4897016 100644
--- a/kroxylicious-integration-test-support/src/test/java/io/kroxylicious/proxy/config/ConfigurationTest.java
+++ b/kroxylicious-integration-test-support/src/test/java/io/kroxylicious/proxy/config/ConfigurationTest.java
@@ -55,63 +55,6 @@ class ConfigurationTest {
             .build();
     private final ConfigParser configParser = new ConfigParser();
 
-    @Test
-    void shouldAcceptOldBootstrapServers() throws IOException {
-        var vc = MAPPER.readValue(
-                """
-                              name: cluster
-                              targetCluster:
-                                bootstrap_servers: kafka.example:1234
-                              gateways:
-                              - name: default
-                                sniHostIdentifiesNode:
-                                  bootstrapAddress: cluster1:9192
-                                  advertisedBrokerAddressPattern: broker-$(nodeId)
-                                tls:
-                                  key:
-                                    certificateFile: /tmp/cert
-                                    privateKeyFile: /tmp/key
-                        """, VirtualCluster.class);
-
-        TargetCluster targetCluster = vc.targetCluster();
-        assertThat(targetCluster).isNotNull();
-        assertThat(targetCluster.bootstrapServers()).isEqualTo("kafka.example:1234");
-    }
-
-    @Test
-    void shouldRejectOldAndNewBootstrapServers() throws IOException {
-        assertThatThrownBy(() -> {
-            MAPPER.readValue(
-                    """
-                                  targetCluster:
-                                    bootstrap_servers: kafka.example:1234
-                                    bootstrapServers: kafka.example:1234
-                                  gateways:
-                                  - name: default
-                                    portIdentifiesNode:
-                                      bootstrapAddress: cluster1:9192
-                            """, VirtualCluster.class);
-        }).isInstanceOf(ValueInstantiationException.class)
-                .hasCauseInstanceOf(IllegalArgumentException.class)
-                .hasMessageContaining("'bootstrapServers' and 'bootstrap_servers' cannot both be specified in a target cluster.");
-    }
-
-    @Test
-    void shouldRequireOldOrNewBootstrapServers() throws IOException {
-        assertThatThrownBy(() -> {
-            MAPPER.readValue(
-                    """
-                                targetCluster: {}
-                                gateways:
-                                - name: default
-                                  portIdentifiesNode:
-                                    bootstrapAddress: cluster1:9192
-                            """, VirtualCluster.class);
-        }).isInstanceOf(ValueInstantiationException.class)
-                .hasCauseInstanceOf(IllegalArgumentException.class)
-                .hasMessageContaining("'bootstrapServers' is required in a target cluster.");
-    }
-
     @Test
     void shouldRejectVirtualClusterWithNoGateways() {
         assertThatThrownBy(() -> {
@@ -119,7 +62,7 @@ class ConfigurationTest {
                     """
                               name: cluster
                               targetCluster:
-                                bootstrap_servers: kafka.example:1234
+                                bootstrapServers: kafka.example:1234
                             """, VirtualCluster.class);
         }).isInstanceOf(ValueInstantiationException.class)
                 .hasCauseInstanceOf(IllegalConfigurationException.class)
@@ -133,7 +76,7 @@ class ConfigurationTest {
                     """
                               name: cluster
                               targetCluster:
-                                bootstrap_servers: kafka.example:1234
+                                bootstrapServers: kafka.example:1234
                               gateways: null
                             """, VirtualCluster.class);
         }).isInstanceOf(ValueInstantiationException.class)
@@ -148,7 +91,7 @@ class ConfigurationTest {
                     """
                               name: cluster
                               targetCluster:
-                                bootstrap_servers: kafka.example:1234
+                                bootstrapServers: kafka.example:1234
                               gateways: [null]
                             """, VirtualCluster.class);
         }).isInstanceOf(ValueInstantiationException.class)
@@ -163,7 +106,7 @@ class ConfigurationTest {
                     """
                               name: cluster
                               targetCluster:
-                                bootstrap_servers: kafka.example:1234
+                                bootstrapServers: kafka.example:1234
                               tls:
                                  key:
                                    certificateFile: /tmp/cert
@@ -183,7 +126,7 @@ class ConfigurationTest {
                     """
                               name: cluster
                               targetCluster:
-                                bootstrap_servers: kafka.example:1234
+                                bootstrapServers: kafka.example:1234
                               tls:
                                  key:
                                    certificateFile: /tmp/cert
@@ -204,7 +147,7 @@ class ConfigurationTest {
                     """
                               name: cluster
                               targetCluster:
-                                bootstrap_servers: kafka.example:1234
+                                bootstrapServers: kafka.example:1234
                               gateways:
                               - name: default
                                 sniHostIdentifiesNode:
@@ -232,7 +175,7 @@ class ConfigurationTest {
                 """
                                   name: cluster
                                   targetCluster:
-                                    bootstrap_servers: kafka.example:1234
+                                    bootstrapServers: kafka.example:1234
                                   clusterNetworkAddressConfigProvider:
                                     type: SniRoutingClusterNetworkAddressConfigProvider
                                     config:
@@ -251,7 +194,7 @@ class ConfigurationTest {
                 """
                                   name: cluster
                                   targetCluster:
-                                    bootstrap_servers: kafka.example:1234
+                                    bootstrapServers: kafka.example:1234
                                   clusterNetworkAddressConfigProvider:
                                     type: SniRoutingClusterNetworkAddressConfigProvider
                                     config: {}
diff --git a/kroxylicious-integration-tests/src/test/java/io/kroxylicious/proxy/ExpositionIT.java b/kroxylicious-integration-tests/src/test/java/io/kroxylicious/proxy/ExpositionIT.java
index 297871eb..1baf3eaf 100644
--- a/kroxylicious-integration-tests/src/test/java/io/kroxylicious/proxy/ExpositionIT.java
+++ b/kroxylicious-integration-tests/src/test/java/io/kroxylicious/proxy/ExpositionIT.java
@@ -107,7 +107,9 @@ class ExpositionIT extends BaseIT {
                                              @BrokerCluster(numBrokers = 2) KafkaCluster cluster) {
         virtualClusterBuilder
                 .withName("demo")
-                .editOrNewTargetCluster().withBootstrapServers(cluster.getBootstrapServers()).endTargetCluster();
+                .withNewTargetCluster()
+                .withBootstrapServers(cluster.getBootstrapServers())
+                .endTargetCluster();
         var builder = new ConfigurationBuilder()
                 .addToVirtualClusters(virtualClusterBuilder.build());
 
@@ -434,8 +436,6 @@ class ExpositionIT extends BaseIT {
         return Stream.of(
                 argumentSet("PortIdentifiesNode",
                         new VirtualClusterBuilder()
-                                .withNewTargetCluster()
-                                .endTargetCluster()
                                 .addToGateways(defaultPortIdentifiesNodeGatewayBuilder(PROXY_ADDRESS)
                                         .withNewTls()
                                         .withNewKeyStoreKey()
@@ -449,8 +449,6 @@ class ExpositionIT extends BaseIT {
                                 SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG, portPerBrokerKeystoreTrustStorePair.password())),
                 argumentSet("SniHostIdentifiesNode",
                         new VirtualClusterBuilder()
-                                .withNewTargetCluster()
-                                .endTargetCluster()
                                 .addToGateways(defaultSniHostIdentifiesNodeGatewayBuilder(SNI_BOOTSTRAP.toString(), SNI_BROKER_ADDRESS_PATTERN)
                                         .withNewTls()
                                         .withNewKeyStoreKey()
@@ -519,7 +517,9 @@ class ExpositionIT extends BaseIT {
                                                                                    KafkaCluster cluster) {
         virtualClusterBuilder
                 .withName("demo")
-                .editOrNewTargetCluster().withBootstrapServers(cluster.getBootstrapServers()).endTargetCluster();
+                .withNewTargetCluster()
+                .withBootstrapServers(cluster.getBootstrapServers())
+                .endTargetCluster();
 
         var builder = new ConfigurationBuilder()
                 .addToVirtualClusters(virtualClusterBuilder.build());
@@ -560,7 +560,9 @@ class ExpositionIT extends BaseIT {
                                    @BrokerCluster KafkaCluster cluster) {
         virtualClusterBuilder
                 .withName("demo")
-                .editOrNewTargetCluster().withBootstrapServers(cluster.getBootstrapServers()).endTargetCluster();
+                .withNewTargetCluster()
+                .withBootstrapServers(cluster.getBootstrapServers())
+                .endTargetCluster();
         var builder = new ConfigurationBuilder()
                 .addToVirtualClusters(virtualClusterBuilder.build());
 
diff --git a/kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/config/ConfigParserTest.java b/kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/config/ConfigParserTest.java
index f76691a0..92909569 100644
--- a/kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/config/ConfigParserTest.java
+++ b/kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/config/ConfigParserTest.java
@@ -527,6 +527,26 @@ class ConfigParserTest {
                 .hasMessageContaining("Missing required creator property 'targetCluster'");
     }
 
+    @Test
+    void shouldDetectMissingTargetClusterBootstrapServers() {
+        // Given
+        assertThatThrownBy(() ->
+        // When
+        configParser.parseConfiguration("""
+                virtualClusters:
+                  - name: demo
+                    targetCluster: {}
+                    gateways:
+                    - name: default
+                      portIdentifiesNode:
+                        bootstrapAddress: cluster1:9192
+                """))
+                // Then
+                .isInstanceOf(IllegalArgumentException.class)
+                .cause()
+                .hasMessageContaining("Missing required creator property 'bootstrapServers'");
+    }
+
     @ParameterizedTest
     @ValueSource(strings = {
             """
diff --git a/kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/config/TargetClusterTest.java b/kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/config/TargetClusterTest.java
new file mode 100644
index 00000000..e78956d7
--- /dev/null
+++ b/kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/config/TargetClusterTest.java
@@ -0,0 +1,25 @@
+/*
+ * Copyright Kroxylicious Authors.
+ *
+ * Licensed under the Apache Software License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
+ */
+
+package io.kroxylicious.proxy.config;
+
+import java.util.Optional;
+
+import org.junit.jupiter.api.Test;
+
+import io.kroxylicious.proxy.config.tls.Tls;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+class TargetClusterTest {
+
+    @Test
+    void shouldRejectEmptyBootstrapServers() {
+        Optional<Tls> empty = Optional.empty();
+        assertThatThrownBy(() -> new TargetCluster(null, empty))
+                .isInstanceOf(IllegalArgumentException.class);
+    }
+}
\ No newline at end of file