Skip to content

Commit eb36a37

Browse files
k-wallk-wall
authored
Name filter factories consistently (kroxylicious#1770)
* Name filter factories consistently We dropped the FilterFactory suffix from filters sometime ago. However, we didn't apply the naming convention to MultiTenant or the Sample. This change applies the convention whilst maintaining support for the existing name (deprecated). Signed-off-by: Keith Wall <kwall@apache.org>
1 parent 01afb06 commit eb36a37

File tree

36 files changed

+472
-326
lines changed

36 files changed

+472
-326
lines changed

‎CHANGELOG.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ Format `<github issue/pr number>: <short description>`.
77

88
## SNAPSHOT
99

10+
* [#1770](https://github.com/kroxylicious/kroxylicious/pull/1770) Name filter factories consistently
1011
* [#1743](https://github.com/kroxylicious/kroxylicious/pull/1743) Apply TLS protocol and cipher suite restrictions to HTTP Clients used by KMS impls too
1112
* [#1761](https://github.com/kroxylicious/kroxylicious/pull/1761) SNI exposition: user can control advertised broker port
1213
* [#1766](https://github.com/kroxylicious/kroxylicious/issues/1766) Bump apicurio-registry.version from 2.6.6.Final to 2.6.7.Final
@@ -23,6 +24,14 @@ Format `<github issue/pr number>: <short description>`.
2324

2425
### Changes, deprecations and removals
2526

27+
* The factory for the Multitenancy filter is renamed from `MultiTenantTransformationFilterFactory` to `MultiTenant`. The
28+
old factory name is deprecated.
29+
* The factories for the Kroxylicious Sample filters are renamed from `SampleProduceRequestFilterFactory` to
30+
`SampleProduceRequest` and `SampleFetchResponseFilterFactory` to `SampleFetchResponse` respectively. The old factory
31+
names are now deprecated.
32+
* The factories for the Kroxylicious Transform filters (used by the performance tests) are renamed from
33+
`ProduceRequestTransformationFilterFactory` to `ProduceRequestTransformation` and `FetchResponseTransformationFilterFactory`
34+
* to `FetchResponseTransformation` respectively. The old factory names are now deprecated.
2635
* The top level `filters` configuration property is deprecated. Configurations should use `filterDefinitions` and `defaultFilters` instead.
2736
* The `bootstrap_servers` property of a virtual cluster's `targetCluster` is deprecated. It is replaced by a property called `bootstrapServers`.
2837
* As per deprecation notice made at 0.7.0, `ProduceValidationFilterFactory` filter is removed. Use `RecordValidation` instead.

‎docs/modules/con-custom-filters.adoc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -464,14 +464,14 @@ file containing the classnames of each filter factory implementation into the JA
464464

465465
For example in the kroxylicious-samples we have the {github}/blob/main/kroxylicious-sample/src/main/java/io/kroxylicious/sample/config/SampleFilterConfig.java[SampleFilterConfig] class.
466466
This is used in the {github}/blob/main/kroxylicious-sample/src/main/java/io/kroxylicious/sample/SampleFetchResponseFilter.java[SampleFetchResponseFilter]). The configuration is routed to the Filter instance via the
467-
{github}/blob/main/kroxylicious-sample/src/main/java/io/kroxylicious/sample/SampleFetchResponseFilterFactory.java[SampleFetchResponseFilterFactory].
467+
{github}/blob/main/kroxylicious-sample/src/main/java/io/kroxylicious/sample/SampleFetchResponse.java[SampleFetchResponse].
468468

469469
Then, when we configure a filter in Kroxylicious configuration like:
470470

471471
[source,yaml]
472472
----
473473
filters:
474-
- type: SampleFetchResponseFilterFactory
474+
- type: SampleFetchResponse
475475
config:
476476
findValue: a
477477
replacementValue: b

‎docs/modules/multi-tenancy/proc-multi-tenancy.adoc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,12 @@ You need at least two virtual clusters to apply multi-tenancy.
2323

2424
.Procedure
2525

26-
. Configure a `MultiTenantTransformationFilterFactory` type filter.
26+
. Configure a `MultiTenant` type filter.
2727
+
2828
[source, yaml]
2929
----
3030
filters:
31-
- type: MultiTenantTransformationFilterFactory
31+
- type: MultiTenant
3232
config:
3333
prefixResourceNameSeparator: "." # <1>
3434
----

‎kroxylicious-app/example-proxy-config.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ virtualClusters:
2020
logFrames: false
2121
filterDefinitions:
2222
#- name: toUpper
23-
# type: ProduceRequestTransformationFilterFactory
23+
# type: ProduceRequestTransformation
2424
# config:
2525
# transformation: UpperCasing
2626
# transformationConfig:

‎kroxylicious-filters/kroxylicious-multitenant/src/main/java/io/kroxylicious/proxy/filter/multitenant/MultiTenant.java‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/*
2+
* Copyright Kroxylicious Authors.
3+
*
4+
* Licensed under the Apache Software License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
5+
*/
6+
7+
package io.kroxylicious.proxy.filter.multitenant;
8+
9+
import java.util.Objects;
10+
11+
import io.kroxylicious.proxy.filter.FilterFactory;
12+
import io.kroxylicious.proxy.filter.FilterFactoryContext;
13+
import io.kroxylicious.proxy.filter.multitenant.config.MultiTenantConfig;
14+
import io.kroxylicious.proxy.plugin.Plugin;
15+
16+
/**
17+
* A {@link FilterFactory} for {@link MultiTenantFilter}.
18+
*/
19+
@Plugin(configType = MultiTenantConfig.class)
20+
public class MultiTenant implements FilterFactory<MultiTenantConfig, MultiTenantConfig> {
21+
22+
private static final MultiTenantConfig DEFAULT_TENANT_CONFIG = new MultiTenantConfig(null);
23+
24+
@Override
25+
public MultiTenantConfig initialize(FilterFactoryContext context, MultiTenantConfig config) {
26+
return config;
27+
}
28+
29+
@Override
30+
public MultiTenantFilter createFilter(FilterFactoryContext context, MultiTenantConfig configuration) {
31+
return new MultiTenantFilter(Objects.requireNonNullElse(configuration, DEFAULT_TENANT_CONFIG));
32+
}
33+
}

kroxylicious-filters/kroxylicious-multitenant/src/main/java/io/kroxylicious/proxy/filter/multitenant/MultiTenantTransformationFilter.java renamed to kroxylicious-filters/kroxylicious-multitenant/src/main/java/io/kroxylicious/proxy/filter/multitenant/MultiTenantFilter.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@
112112
* <br/>
113113
* TODO disallow the use of topic uids belonging to one tenant by another.
114114
*/
115-
public class MultiTenantTransformationFilter
115+
class MultiTenantFilter
116116
implements ApiVersionsResponseFilter,
117117
CreateTopicsRequestFilter, CreateTopicsResponseFilter,
118118
DeleteTopicsRequestFilter, DeleteTopicsResponseFilter,
@@ -395,9 +395,8 @@ public CompletionStage<RequestFilterResult> onAddPartitionsToTxnRequest(short ap
395395

396396
request.transactions().forEach(addPartitionsToTxnTransaction -> {
397397
applyTenantPrefix(context, addPartitionsToTxnTransaction::transactionalId, addPartitionsToTxnTransaction::setTransactionalId, true);
398-
addPartitionsToTxnTransaction.topics().forEach(addPartitionsToTxnTopic -> {
399-
applyTenantPrefix(context, addPartitionsToTxnTopic::name, addPartitionsToTxnTopic::setName, true);
400-
});
398+
addPartitionsToTxnTransaction.topics()
399+
.forEach(addPartitionsToTxnTopic -> applyTenantPrefix(context, addPartitionsToTxnTopic::name, addPartitionsToTxnTopic::setName, true));
401400
});
402401
return context.forwardRequest(header, request);
403402
}
@@ -522,7 +521,7 @@ private String createKafkaResourcePrefixIfNecessary(FilterContext context) {
522521
return kafkaResourcePrefix;
523522
}
524523

525-
public MultiTenantTransformationFilter(@NonNull MultiTenantConfig configuration) {
524+
MultiTenantFilter(@NonNull MultiTenantConfig configuration) {
526525
Objects.requireNonNull(configuration);
527526
this.prefixResourceNameSeparator = configuration.prefixResourceNameSeparator();
528527
}

‎kroxylicious-filters/kroxylicious-multitenant/src/main/java/io/kroxylicious/proxy/filter/multitenant/MultiTenantTransformationFilterFactory.java‎

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,25 +6,16 @@
66

77
package io.kroxylicious.proxy.filter.multitenant;
88

9-
import java.util.Objects;
10-
119
import io.kroxylicious.proxy.filter.FilterFactory;
12-
import io.kroxylicious.proxy.filter.FilterFactoryContext;
1310
import io.kroxylicious.proxy.filter.multitenant.config.MultiTenantConfig;
1411
import io.kroxylicious.proxy.plugin.Plugin;
1512

13+
/**
14+
* A {@link FilterFactory} for {@link MultiTenantFilter}.
15+
*
16+
* @deprecated use {@link MultiTenant} instead.
17+
*/
18+
@Deprecated(since = "0.10.0", forRemoval = true)
1619
@Plugin(configType = MultiTenantConfig.class)
17-
public class MultiTenantTransformationFilterFactory implements FilterFactory<MultiTenantConfig, MultiTenantConfig> {
18-
19-
private static final MultiTenantConfig DEFAULT_TENANT_CONFIG = new MultiTenantConfig(null);
20-
21-
@Override
22-
public MultiTenantConfig initialize(FilterFactoryContext context, MultiTenantConfig config) {
23-
return config;
24-
}
25-
26-
@Override
27-
public MultiTenantTransformationFilter createFilter(FilterFactoryContext context, MultiTenantConfig configuration) {
28-
return new MultiTenantTransformationFilter(Objects.requireNonNullElse(configuration, DEFAULT_TENANT_CONFIG));
29-
}
20+
public class MultiTenantTransformationFilterFactory extends MultiTenant {
3021
}

‎kroxylicious-filters/kroxylicious-multitenant/src/main/resources/META-INF/services/io.kroxylicious.proxy.filter.FilterFactory‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
io.kroxylicious.proxy.filter.multitenant.MultiTenantTransformationFilterFactory
2+
io.kroxylicious.proxy.filter.multitenant.MultiTenant

kroxylicious-filters/kroxylicious-multitenant/src/test/java/io/kroxylicious/proxy/filter/multitenant/MultiTenantTransformationFilterFactoryFilterTest.java renamed to kroxylicious-filters/kroxylicious-multitenant/src/test/java/io/kroxylicious/proxy/filter/multitenant/MultiTenantFilterTest.java

Lines changed: 127 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,14 @@
88

99
import java.io.IOException;
1010
import java.util.List;
11+
import java.util.Objects;
1112
import java.util.concurrent.CompletableFuture;
1213
import java.util.regex.Pattern;
1314
import java.util.stream.Stream;
1415

1516
import org.apache.kafka.common.message.ApiMessageType;
17+
import org.apache.kafka.common.message.FetchResponseData;
18+
import org.apache.kafka.common.message.ProduceRequestData;
1619
import org.apache.kafka.common.message.RequestHeaderData;
1720
import org.apache.kafka.common.message.ResponseHeaderData;
1821
import org.apache.kafka.common.protocol.ApiKeys;
@@ -23,10 +26,11 @@
2326
import org.junit.jupiter.params.ParameterizedTest;
2427
import org.junit.jupiter.params.provider.Arguments;
2528
import org.junit.jupiter.params.provider.MethodSource;
29+
import org.junit.jupiter.params.provider.NullSource;
30+
import org.junit.jupiter.params.provider.ValueSource;
2631
import org.mockito.ArgumentCaptor;
2732
import org.mockito.Captor;
2833
import org.mockito.Mock;
29-
import org.mockito.Mockito;
3034
import org.mockito.junit.jupiter.MockitoExtension;
3135

3236
import com.flipkart.zjsonpatch.JsonDiff;
@@ -35,31 +39,42 @@
3539

3640
import io.kroxylicious.proxy.filter.FilterAndInvoker;
3741
import io.kroxylicious.proxy.filter.FilterContext;
38-
import io.kroxylicious.proxy.filter.FilterFactoryContext;
3942
import io.kroxylicious.proxy.filter.FilterInvoker;
4043
import io.kroxylicious.proxy.filter.RequestFilterResult;
4144
import io.kroxylicious.proxy.filter.ResponseFilterResult;
4245
import io.kroxylicious.proxy.filter.multitenant.config.MultiTenantConfig;
46+
import io.kroxylicious.test.condition.kafka.FetchResponseDataCondition;
4347
import io.kroxylicious.test.requestresponsetestdef.ApiMessageTestDef;
4448
import io.kroxylicious.test.requestresponsetestdef.RequestResponseTestDef;
4549

4650
import static com.google.common.base.Preconditions.checkState;
4751
import static com.google.common.collect.Iterables.getOnlyElement;
52+
import static io.kroxylicious.test.condition.kafka.ApiMessageCondition.forApiKey;
53+
import static io.kroxylicious.test.condition.kafka.ProduceRequestDataCondition.produceRequestMatching;
4854
import static io.kroxylicious.test.requestresponsetestdef.KafkaApiMessageConverter.requestConverterFor;
4955
import static io.kroxylicious.test.requestresponsetestdef.KafkaApiMessageConverter.responseConverterFor;
5056
import static org.assertj.core.api.Assertions.assertThat;
57+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
5158
import static org.junit.jupiter.api.Assertions.assertEquals;
59+
import static org.mockito.ArgumentMatchers.any;
60+
import static org.mockito.ArgumentMatchers.assertArg;
5261
import static org.mockito.Mock.Strictness.LENIENT;
62+
import static org.mockito.Mockito.verify;
5363
import static org.mockito.Mockito.when;
5464

5565
@ExtendWith(MockitoExtension.class)
56-
class MultiTenantTransformationFilterFactoryFilterTest {
66+
class MultiTenantFilterTest {
5767
private static final Pattern TEST_RESOURCE_FILTER = Pattern.compile(
58-
String.format("%s/.*\\.yaml", MultiTenantTransformationFilterFactoryFilterTest.class.getPackageName().replace(".", "/")));
68+
String.format("%s/.*\\.yaml", MultiTenantFilterTest.class.getPackageName().replace(".", "/")));
5969
private static final String TENANT_1 = "tenant1";
6070

71+
private static final String VIRTUAL_CLUSTER_NAME = "vc1";
72+
private static final String TEST_TOPIC = "testTopic";
73+
@Mock
74+
private FilterContext filterContext;
75+
6176
private static List<ResourceInfo> getTestResources() throws IOException {
62-
var resources = ClassPath.from(MultiTenantTransformationFilterFactoryFilterTest.class.getClassLoader()).getResources().stream()
77+
var resources = ClassPath.from(MultiTenantFilterTest.class.getClassLoader()).getResources().stream()
6378
.filter(ri -> TEST_RESOURCE_FILTER.matcher(ri.getResourceName()).matches()).toList();
6479

6580
// https://youtrack.jetbrains.com/issue/IDEA-315462: we've seen issues in IDEA in IntelliJ Workspace Model API mode where test resources
@@ -70,7 +85,7 @@ private static List<ResourceInfo> getTestResources() throws IOException {
7085
return resources;
7186
}
7287

73-
private final MultiTenantTransformationFilter filter = new MultiTenantTransformationFilter(new MultiTenantConfig(null));
88+
private final MultiTenantFilter filter = new MultiTenantFilter(new MultiTenantConfig(null));
7489

7590
private final FilterInvoker invoker = getOnlyElement(FilterAndInvoker.build(filter)).invoker();
7691

@@ -152,9 +167,111 @@ void responseTransformed(@SuppressWarnings("unused") String testName, ApiMessage
152167
}
153168

154169
@Test
155-
void testContributor() {
156-
MultiTenantTransformationFilterFactory factory = new MultiTenantTransformationFilterFactory();
157-
assertThat(factory.createFilter(Mockito.mock(FilterFactoryContext.class), Mockito.mock(MultiTenantConfig.class)))
158-
.isInstanceOf(MultiTenantTransformationFilter.class);
170+
void shouldReWriteTopic() {
171+
var multiTenantTransformationFilter = new MultiTenantFilter(new MultiTenantConfig(null));
172+
// Given
173+
when(filterContext.getVirtualClusterName()).thenReturn(VIRTUAL_CLUSTER_NAME);
174+
var request = createProduceRequest(TEST_TOPIC);
175+
// When
176+
multiTenantTransformationFilter.onProduceRequest(
177+
ProduceRequestData.HIGHEST_SUPPORTED_VERSION, new RequestHeaderData(), request, filterContext);
178+
179+
// Then
180+
verify(filterContext).forwardRequest(any(RequestHeaderData.class), assertArg(
181+
apiMessage -> assertThat(apiMessage)
182+
.is(forApiKey(ApiKeys.PRODUCE))
183+
.is(produceRequestMatching(produceRequestData -> produceRequestData.topicData()
184+
.stream()
185+
.allMatch(topicProduceData -> topicProduceData.name().equals("vc1-testTopic"))))));
186+
187+
}
188+
189+
@ParameterizedTest
190+
@ValueSource(strings = { ".", "_", "", "-.-" })
191+
@NullSource
192+
void produceWithCustomSeparator(String separator) {
193+
var expectedServerTopicName = "%s%s%s".formatted(VIRTUAL_CLUSTER_NAME, Objects.requireNonNullElse(separator, MultiTenantConfig.DEFAULT_SEPARATOR), TEST_TOPIC);
194+
var multiTenantTransformationFilter = new MultiTenantFilter(new MultiTenantConfig(separator));
195+
// Given
196+
when(filterContext.getVirtualClusterName()).thenReturn(VIRTUAL_CLUSTER_NAME);
197+
var request = createProduceRequest(TEST_TOPIC);
198+
// When
199+
multiTenantTransformationFilter.onProduceRequest(
200+
ProduceRequestData.HIGHEST_SUPPORTED_VERSION, new RequestHeaderData(), request, filterContext);
201+
202+
// Then
203+
verify(filterContext).forwardRequest(any(RequestHeaderData.class), assertArg(
204+
apiMessage -> assertThat(apiMessage)
205+
.is(forApiKey(ApiKeys.PRODUCE))
206+
.is(produceRequestMatching(produceRequestData -> produceRequestData.topicData()
207+
.stream()
208+
.allMatch(topicProduceData -> topicProduceData.name().equals(expectedServerTopicName))))));
209+
}
210+
211+
@ParameterizedTest
212+
@ValueSource(strings = { ".", "_", "", "-.-" })
213+
@NullSource
214+
void fetchWithCustomSeparator(String separator) {
215+
var serverTopic = "%s%s%s".formatted(VIRTUAL_CLUSTER_NAME, Objects.requireNonNullElse(separator, MultiTenantConfig.DEFAULT_SEPARATOR), TEST_TOPIC);
216+
var multiTenantTransformationFilter = new MultiTenantFilter(new MultiTenantConfig(separator));
217+
// Given
218+
when(filterContext.getVirtualClusterName()).thenReturn(VIRTUAL_CLUSTER_NAME);
219+
var response = createFetchResponseData(serverTopic);
220+
// When
221+
multiTenantTransformationFilter.onFetchResponse(
222+
FetchResponseData.HIGHEST_SUPPORTED_VERSION, new ResponseHeaderData(), response, filterContext);
223+
224+
// Then
225+
verify(filterContext).forwardResponse(any(ResponseHeaderData.class), assertArg(
226+
apiMessage -> assertThat(apiMessage)
227+
.is(forApiKey(ApiKeys.FETCH))
228+
.is(FetchResponseDataCondition.fetchResponseMatching(fetchResponseData -> fetchResponseData.responses()
229+
.stream()
230+
.allMatch(topicFetchData -> topicFetchData.topic().equals(TEST_TOPIC))))));
231+
}
232+
233+
@Test
234+
void illegalKafkaResourceCharsInVirtualClusterNameDetected() {
235+
var multiTenantTransformationFilter = new MultiTenantFilter(new MultiTenantConfig(MultiTenantConfig.DEFAULT_SEPARATOR));
236+
// Given
237+
when(filterContext.getVirtualClusterName()).thenReturn("$badvcn$");
238+
var request = createProduceRequest(TEST_TOPIC);
239+
var header = new RequestHeaderData();
240+
// When
241+
assertThatThrownBy(() -> {
242+
multiTenantTransformationFilter.onProduceRequest(
243+
ProduceRequestData.HIGHEST_SUPPORTED_VERSION, header, request, filterContext);
244+
}).isInstanceOf(IllegalStateException.class);
245+
}
246+
247+
@Test
248+
void illegalKafkaResourceCharsInSeparatorDetected() {
249+
var multiTenantTransformationFilter = new MultiTenantFilter(new MultiTenantConfig("$bad$"));
250+
// Given
251+
when(filterContext.getVirtualClusterName()).thenReturn(VIRTUAL_CLUSTER_NAME);
252+
var request = createProduceRequest(TEST_TOPIC);
253+
var header = new RequestHeaderData();
254+
// When
255+
assertThatThrownBy(() -> {
256+
multiTenantTransformationFilter.onProduceRequest(
257+
ProduceRequestData.HIGHEST_SUPPORTED_VERSION, header, request, filterContext);
258+
}).isInstanceOf(IllegalStateException.class);
159259
}
260+
261+
private ProduceRequestData createProduceRequest(String topic) {
262+
final ProduceRequestData request = new ProduceRequestData();
263+
final ProduceRequestData.TopicProduceData topicData = new ProduceRequestData.TopicProduceData();
264+
topicData.setName(topic);
265+
request.topicData().add(topicData);
266+
return request;
267+
}
268+
269+
private FetchResponseData createFetchResponseData(String topic) {
270+
var response = new FetchResponseData();
271+
var topicResponse = new FetchResponseData.FetchableTopicResponse();
272+
topicResponse.setTopic(topic);
273+
response.responses().add(topicResponse);
274+
return response;
275+
}
276+
160277
}

kroxylicious-filters/kroxylicious-multitenant/src/test/java/io/kroxylicious/proxy/filter/multitenant/MultiTenantFilterFactoryTest.java renamed to kroxylicious-filters/kroxylicious-multitenant/src/test/java/io/kroxylicious/proxy/filter/multitenant/MultiTenantTest.java

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,20 @@
1515

1616
import static org.assertj.core.api.Assertions.assertThat;
1717

18-
class MultiTenantFilterFactoryTest {
18+
class MultiTenantTest {
1919

2020
@Test
21-
void testGetInstance() {
22-
MultiTenantTransformationFilterFactory factory = new MultiTenantTransformationFilterFactory();
21+
void createFilter() {
22+
var factory = new MultiTenant();
2323
Filter filter = factory.createFilter(Mockito.mock(FilterFactoryContext.class), Mockito.mock(MultiTenantConfig.class));
24-
assertThat(filter).isNotNull().isInstanceOf(MultiTenantTransformationFilter.class);
24+
assertThat(filter).isNotNull().isInstanceOf(MultiTenantFilter.class);
25+
}
26+
27+
@Test
28+
void createFilterDeprecatedFactory() {
29+
var factory = new MultiTenantTransformationFilterFactory();
30+
Filter filter = factory.createFilter(Mockito.mock(FilterFactoryContext.class), Mockito.mock(MultiTenantConfig.class));
31+
assertThat(filter).isNotNull().isInstanceOf(MultiTenantFilter.class);
2532
}
2633

2734
}

0 commit comments

Comments
 (0)