Skip to content

Commit 76f0a7d

Browse files
k-wallk-wall
authored
refactoring - rename method requiresTls -> requiresServerNameIndication (kroxylicious#1854)
* refactoring - rename method requireTls -> requireServerNameIndication to better communicate the reliance on Server Name Indication TLS extension. why: renamed for clarity. Signed-off-by: Keith Wall <kwall@apache.org> * chase some coverage Signed-off-by: Keith Wall <kwall@apache.org> --------- Signed-off-by: Keith Wall <kwall@apache.org>
1 parent ca9f349 commit 76f0a7d

File tree

8 files changed

+104
-30
lines changed

8 files changed

+104
-30
lines changed

‎kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/clusternetworkaddressconfigprovider/SniRoutingClusterNetworkAddressConfigProvider.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ public Set<Integer> getSharedPorts() {
114114
}
115115

116116
@Override
117-
public boolean requiresTls() {
117+
public boolean requiresServerNameIndication() {
118118
return true;
119119
}
120120

‎kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/net/EndpointListener.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,12 @@ public interface EndpointListener {
3434
boolean isUseTls();
3535

3636
/**
37-
* true if this listener requires TLS (in other words, use SNI).
38-
* @return true if this listener requires TLS.
37+
* Indicates if the provider requires that connections utilise the Server Name Indication (SNI)
38+
* extension to TLS. If this is true, then the provider cannot support plain connections.
39+
*
40+
* @return true if this provider requires Server Name Indication (SNI).
3941
*/
40-
boolean requiresTls();
42+
boolean requiresServerNameIndication();
4143

4244
VirtualClusterModel virtualCluster();
4345

‎kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/internal/net/EndpointRegistry.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,7 @@ private CompletionStage<Endpoint> registerBinding(Endpoint key, String host, End
465465
var bindings = acceptorChannel.attr(CHANNEL_BINDINGS);
466466
bindings.setIfAbsent(new ConcurrentHashMap<>());
467467
var bindingMap = bindings.get();
468-
var bindingKey = virtualCluster.requiresTls() ? RoutingKey.createBindingKey(host) : RoutingKey.NULL_ROUTING_KEY;
468+
var bindingKey = virtualCluster.requiresServerNameIndication() ? RoutingKey.createBindingKey(host) : RoutingKey.NULL_ROUTING_KEY;
469469

470470
// we use a bindingMap attached to the channel to record the bindings to the channel. the #deregisterBinding path
471471
// knows to tear down the acceptorChannel when the map becomes empty.

‎kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/model/VirtualClusterModel.java‎

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,6 @@ private static String generateTlsSummary(Optional<Tls> tlsToSummarize) {
116116
}
117117

118118
public void addListener(String name, ClusterNetworkAddressConfigProvider provider, Optional<Tls> tls) {
119-
validateTLsSettings(provider, tls);
120-
validatePortUsage(provider);
121119
listeners.put(name, new VirtualClusterListenerModel(this, provider, tls, name));
122120
}
123121

@@ -250,21 +248,6 @@ private static SSLParameters getSupportedSSLParameters() {
250248
}
251249
}
252250

253-
private static void validatePortUsage(ClusterNetworkAddressConfigProvider clusterNetworkAddressConfigProvider) {
254-
var conflicts = clusterNetworkAddressConfigProvider.getExclusivePorts().stream().filter(p -> clusterNetworkAddressConfigProvider.getSharedPorts().contains(p))
255-
.collect(Collectors.toSet());
256-
if (!conflicts.isEmpty()) {
257-
throw new IllegalStateException(
258-
"The set of exclusive ports described by the cluster endpoint provider must be distinct from those described as shared. Intersection: " + conflicts);
259-
}
260-
}
261-
262-
private static void validateTLsSettings(ClusterNetworkAddressConfigProvider clusterNetworkAddressConfigProvider, Optional<Tls> tls) {
263-
if (clusterNetworkAddressConfigProvider.requiresTls() && (tls.isEmpty() || !tls.get().definesKey())) {
264-
throw new IllegalStateException("Cluster endpoint provider requires server TLS, but this virtual cluster does not define it.");
265-
}
266-
}
267-
268251
public @NonNull List<NamedFilterDefinition> getFilters() {
269252
return filters;
270253
}
@@ -286,8 +269,29 @@ public static class VirtualClusterListenerModel implements EndpointListener {
286269
this.virtualCluster = virtualCluster;
287270
this.provider = provider;
288271
this.tls = tls;
289-
this.downstreamSslContext = buildDownstreamSslContext();
290272
this.name = name;
273+
validatePortUsage(provider);
274+
validateTLsSettings(provider, tls);
275+
this.downstreamSslContext = buildDownstreamSslContext();
276+
}
277+
278+
private void validatePortUsage(ClusterNetworkAddressConfigProvider clusterNetworkAddressConfigProvider) {
279+
// This validation seems misplaced.
280+
var conflicts = clusterNetworkAddressConfigProvider.getExclusivePorts().stream().filter(p -> clusterNetworkAddressConfigProvider.getSharedPorts().contains(p))
281+
.collect(Collectors.toSet());
282+
if (!conflicts.isEmpty()) {
283+
throw new IllegalStateException(
284+
"The set of exclusive ports described by the cluster endpoint provider must be distinct from those described as shared. Intersection: "
285+
+ conflicts);
286+
}
287+
}
288+
289+
private void validateTLsSettings(ClusterNetworkAddressConfigProvider clusterNetworkAddressConfigProvider, Optional<Tls> tls) {
290+
if (clusterNetworkAddressConfigProvider.requiresServerNameIndication() && (tls.isEmpty() || !tls.get().definesKey())) {
291+
throw new IllegalStateException(
292+
"Cluster endpoint provider requires ServerNameIndication, but virtual cluster listener '%s' does not configure TLS and provide a certificate for the server"
293+
.formatted(name()));
294+
}
291295
}
292296

293297
@Override
@@ -320,8 +324,8 @@ public Optional<String> getBindAddress() {
320324
}
321325

322326
@Override
323-
public boolean requiresTls() {
324-
return getClusterNetworkAddressConfigProvider().requiresTls();
327+
public boolean requiresServerNameIndication() {
328+
return getClusterNetworkAddressConfigProvider().requiresServerNameIndication();
325329
}
326330

327331
@Override

‎kroxylicious-runtime/src/main/java/io/kroxylicious/proxy/service/ClusterNetworkAddressConfigProvider.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,11 +69,12 @@ default Optional<String> getBindAddress() {
6969
}
7070

7171
/**
72-
* Indicates if this provider requires the use of TLS.
72+
* Indicates if the provider requires that connections utilise the Server Name Indication (SNI)
73+
* extension to TLS. If this is true, then the provider cannot support plain connections.
7374
*
74-
* @return true if this provider requires the use of TLS.
75+
* @return true if this provider requires Server Name Indication (SNI).
7576
*/
76-
default boolean requiresTls() {
77+
default boolean requiresServerNameIndication() {
7778
return false;
7879
}
7980

‎kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/KafkaProxyTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,8 @@ public static Stream<Arguments> missingTls() {
120120
advertisedBrokerAddressPattern: broker-$(nodeId)
121121
targetCluster:
122122
bootstrapServers: kafka.example:1234
123-
""", "Cluster endpoint provider requires server TLS, but this virtual cluster does not define it"));
123+
""",
124+
"Cluster endpoint provider requires ServerNameIndication, but virtual cluster listener 'default' does not configure TLS and provide a certificate for the server"));
124125
}
125126

126127
@ParameterizedTest(name = "{0}")

‎kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/internal/net/EndpointRegistryTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,7 @@ private void configureVirtualClusterMock(EndpointListener cluster, HostPort down
733733
Map<Integer, HostPort> discoveryAddressMap) {
734734
when(cluster.getClusterBootstrapAddress()).thenReturn(downstreamBootstrap);
735735
when(cluster.isUseTls()).thenReturn(tls);
736-
when(cluster.requiresTls()).thenReturn(sni);
736+
when(cluster.requiresServerNameIndication()).thenReturn(sni);
737737
when(cluster.discoveryAddressMap()).thenReturn(discoveryAddressMap);
738738
when(cluster.targetCluster()).thenReturn(new TargetCluster(upstreamBootstrap.toString(), null));
739739
when(cluster.getBrokerIdFromBrokerAddress(any(HostPort.class))).thenReturn(null);

‎kroxylicious-runtime/src/test/java/io/kroxylicious/proxy/model/VirtualClusterListenerModelTest.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343

4444
import static io.kroxylicious.proxy.service.HostPort.parse;
4545
import static org.assertj.core.api.Assertions.assertThat;
46+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
4647
import static org.junit.jupiter.params.provider.Arguments.argumentSet;
4748
import static org.mockito.Mockito.mock;
4849
import static org.mockito.Mockito.when;
@@ -85,6 +86,33 @@ void delegatesToProviderForAdvertisedPort() {
8586
assertThat(listener.getAdvertisedBrokerAddress(0)).isEqualTo(advertisedHostPort);
8687
}
8788

89+
@Test
90+
void delegatesToProviderForBrokerAddress() {
91+
var mock = mock(ClusterNetworkAddressConfigProvider.class);
92+
var listener = new VirtualClusterListenerModel(mock(VirtualClusterModel.class), mock, Optional.empty(), "default");
93+
var brokerAddress = new HostPort("broker", 55);
94+
when(mock.getBrokerAddress(0)).thenReturn(brokerAddress);
95+
assertThat(listener.getBrokerAddress(0)).isEqualTo(brokerAddress);
96+
}
97+
98+
@Test
99+
void delegatesToProviderForBrokerIdFromBrokerAddress() {
100+
var mock = mock(ClusterNetworkAddressConfigProvider.class);
101+
var listener = new VirtualClusterListenerModel(mock(VirtualClusterModel.class), mock, Optional.empty(), "default");
102+
var brokerAddress = new HostPort("broker", 55);
103+
when(mock.getBrokerIdFromBrokerAddress(brokerAddress)).thenReturn(1);
104+
assertThat(listener.getBrokerIdFromBrokerAddress(brokerAddress)).isEqualTo(1);
105+
}
106+
107+
@Test
108+
void delegatesToProviderForServerNameIndication() {
109+
var mock = mock(ClusterNetworkAddressConfigProvider.class);
110+
var listener = new VirtualClusterListenerModel(mock(VirtualClusterModel.class), mock, Optional.empty(), "default");
111+
var brokerAddress = new HostPort("broker", 55);
112+
when(mock.requiresServerNameIndication()).thenReturn(true);
113+
assertThat(listener.requiresServerNameIndication()).isTrue();
114+
}
115+
88116
@Test
89117
void shouldBuildDownstreamSslContext() {
90118
// Given
@@ -214,6 +242,44 @@ void shouldApplyDownstreamCipherSuiteRestriction(AllowDeny<String> cipherSuiteAl
214242
}));
215243
}
216244

245+
static Stream<Arguments> downstreamListenerThatRequiresSniEnforcesTlsWithKey() {
246+
return Stream.of(
247+
argumentSet("no tls", Optional.empty()),
248+
argumentSet("no key", Optional.of(new Tls(null, null, null, null))));
249+
}
250+
251+
@ParameterizedTest
252+
@MethodSource
253+
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
254+
void downstreamListenerThatRequiresSniEnforcesTlsWithKey(Optional<Tls> downstreamTls) {
255+
// Given
256+
var model = mock(VirtualClusterModel.class);
257+
var provider = mock(ClusterNetworkAddressConfigProvider.class);
258+
when(provider.requiresServerNameIndication()).thenReturn(true);
259+
260+
// When/Then
261+
assertThatThrownBy(() -> new VirtualClusterListenerModel(model, provider, downstreamTls, "mylistener"))
262+
.isInstanceOf(IllegalStateException.class)
263+
.hasMessageContaining(
264+
"Cluster endpoint provider requires ServerNameIndication, but virtual cluster listener 'mylistener' does not configure TLS and provide a certificate for the server");
265+
}
266+
267+
@Test
268+
void detectsBadPortConfiguration() {
269+
// Given
270+
var tls = Optional.<Tls> empty();
271+
var model = mock(VirtualClusterModel.class);
272+
var provider = mock(ClusterNetworkAddressConfigProvider.class);
273+
when(provider.getSharedPorts()).thenReturn(Set.of(9080, 9081));
274+
when(provider.getExclusivePorts()).thenReturn(Set.of(9080));
275+
276+
// When/Then
277+
assertThatThrownBy(() -> new VirtualClusterListenerModel(model, provider, tls, "mylistener"))
278+
.isInstanceOf(IllegalStateException.class)
279+
.hasMessageContaining(
280+
"The set of exclusive ports described by the cluster endpoint provider must be distinct from those described as shared. Intersection: [9080]");
281+
}
282+
217283
@NonNull
218284
private ClusterNetworkAddressConfigProvider createTestClusterNetworkAddressConfigProvider() {
219285
final PortPerBrokerClusterNetworkAddressConfigProviderConfig clusterNetworkAddressConfigProviderConfig = new PortPerBrokerClusterNetworkAddressConfigProviderConfig(

0 commit comments

Comments
 (0)