rhauch commented on a change in pull request #8620:
URL: https://github.com/apache/kafka/pull/8620#discussion_r420464820
##########
File path: checkstyle/checkstyle.xml
##########
@@ -132,7 +132,7 @@
</module>
<module name="NPathComplexity">
<!-- default is 200 -->
- <property name="max" value="500"/>
+ <property name="max" value="550"/>
Review comment:
Why change the setting instead of modifying `suppressions.xml` to
exclude certain classes from this rule?
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java
##########
@@ -244,6 +244,14 @@
+ "user requests to reset the set of active topics per connector.";
protected static final boolean TOPIC_TRACKING_ALLOW_RESET_DEFAULT = true;
+ /**
+ * @link
"https://www.eclipse.org/jetty/documentation/current/header-filter.html";
+ * @link
"https://www.eclipse.org/jetty/javadoc/9.4.28.v20200408/org/eclipse/jetty/servlets/HeaderFilter.html";
+ **/
+ public static final String RESPONSE_HTTP_HEADERS_CONFIG =
"response.http.headers.config";
+ public static final String RESPONSE_HTTP_HEADERS_DOC = "Set values for
Jetty HTTP response headers";
Review comment:
I don't think we should expose `Jetty` here. Yes, we're following the
Jetty grammar and format for these, but let's not unnecessarily expose the
internals.
```suggestion
public static final String RESPONSE_HTTP_HEADERS_DOC = "Rules for REST
API HTTP response headers";
```
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java
##########
@@ -461,4 +469,18 @@ public static String urlJoin(String base, String path) {
return base + path;
}
+ /**
+ * Register header filter to ServletContextHandler.
+ * @param context The serverlet context handler
+ */
+ protected void configureHttpResponsHeaderFilter(ServletContextHandler
context) {
+ String headerConfig =
config.getString(WorkerConfig.RESPONSE_HTTP_HEADERS_CONFIG);
+ log.debug("headerConfig : " + headerConfig);
Review comment:
Is this line really necessary? Isn't the `response.http.headers.config`
property already logged at INFO level when the worker starts up, via the
WorkerConfig (or rather DistributedConfig or StandaloneConfig) constructor?
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java
##########
@@ -400,6 +410,52 @@ public WorkerConfig(ConfigDef definition, Map<String,
String> props) {
logInternalConverterDeprecationWarnings(props);
}
+ public static void validateHttpResponseHeaderConfig(String config) {
Review comment:
Why not implement these as a `ConfigDef.Validator` implementation,
similar to the existing `AdminListenersValidator` below?
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java
##########
@@ -392,6 +393,106 @@ public void testDisableAdminEndpoint() throws IOException
{
Assert.assertEquals(404, response.getStatusLine().getStatusCode());
}
+ @Test
+ public void TestValidCustomizedHttpResponseHeaders() throws IOException {
+ String headerConfig =
+ "add X-XSS-Protection: 1; mode=block, \"add Cache-Control:
no-cache, no-store, must-revalidate\"";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ expectedHeaders.put("X-XSS-Protection", "1; mode=block");
+ expectedHeaders.put("Cache-Control", "no-cache, no-store,
must-revalidate");
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test
+ public void TestDefaultCustomizedHttpResponseHeaders() throws IOException
{
Review comment:
Nit:
```suggestion
public void testDefaultCustomizedHttpResponseHeaders() throws
IOException {
```
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java
##########
@@ -392,6 +393,106 @@ public void testDisableAdminEndpoint() throws IOException
{
Assert.assertEquals(404, response.getStatusLine().getStatusCode());
}
+ @Test
+ public void TestValidCustomizedHttpResponseHeaders() throws IOException {
Review comment:
Nit:
```suggestion
public void testValidCustomizedHttpResponseHeaders() throws IOException
{
```
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java
##########
@@ -392,6 +393,106 @@ public void testDisableAdminEndpoint() throws IOException
{
Assert.assertEquals(404, response.getStatusLine().getStatusCode());
}
+ @Test
+ public void TestValidCustomizedHttpResponseHeaders() throws IOException {
+ String headerConfig =
+ "add X-XSS-Protection: 1; mode=block, \"add Cache-Control:
no-cache, no-store, must-revalidate\"";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ expectedHeaders.put("X-XSS-Protection", "1; mode=block");
+ expectedHeaders.put("Cache-Control", "no-cache, no-store,
must-revalidate");
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test
+ public void TestDefaultCustomizedHttpResponseHeaders() throws IOException
{
+ String headerConfig = "";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test(expected = ConfigException.class)
+ public void testInvalidHeaderConfigFormat() {
+ String headerConfig = "add X-XSS-Protection";
+ Map<String, String> workerProps = baseWorkerProps();
+ workerProps.put("offset.storage.file.filename", "/tmp");
+ workerProps.put(WorkerConfig.RESPONSE_HTTP_HEADERS_CONFIG,
headerConfig);
+ WorkerConfig workerConfig = new StandaloneConfig(workerProps);
+
+ EasyMock.expect(herder.kafkaClusterId()).andReturn(KAFKA_CLUSTER_ID);
+ EasyMock.expect(herder.plugins()).andStubReturn(plugins);
+ EasyMock.expect(plugins.newPlugins(Collections.emptyList(),
+ workerConfig,
+
ConnectRestExtension.class)).andStubReturn(Collections.emptyList());
+
+ EasyMock.expect(herder.connectors()).andReturn(Arrays.asList("a",
"b"));
+
+ PowerMock.replayAll();
+
+ server = new RestServer(workerConfig);
+ server.initializeServer();
+ server.initializeResources(herder);
+ server.stop();
+ }
+
+ @Test(expected = ConfigException.class)
+ public void testInvalidHeaderConfigAction() {
+ String headerConfig = "badaction X-XSS-Protection: 1; mode=block";
+ Map<String, String> workerProps = baseWorkerProps();
+ workerProps.put("offset.storage.file.filename", "/tmp");
+ workerProps.put(WorkerConfig.RESPONSE_HTTP_HEADERS_CONFIG,
headerConfig);
+ WorkerConfig workerConfig = new StandaloneConfig(workerProps);
+
+ EasyMock.expect(herder.kafkaClusterId()).andReturn(KAFKA_CLUSTER_ID);
+ EasyMock.expect(herder.plugins()).andStubReturn(plugins);
+ EasyMock.expect(plugins.newPlugins(Collections.emptyList(),
+ workerConfig,
+
ConnectRestExtension.class)).andStubReturn(Collections.emptyList());
+
+ EasyMock.expect(herder.connectors()).andReturn(Arrays.asList("a",
"b"));
+
+ PowerMock.replayAll();
+
+ server = new RestServer(workerConfig);
+ server.initializeServer();
+ server.initializeResources(herder);
+ server.stop();
+ }
+
+ public void checkCustomizedHttpResponseHeaders(String headerConfig,
Map<String, String> expectedHeaders)
+ throws IOException {
+ Map<String, String> workerProps = baseWorkerProps();
+ workerProps.put("offset.storage.file.filename", "/tmp");
+ workerProps.put(WorkerConfig.RESPONSE_HTTP_HEADERS_CONFIG,
headerConfig);
+ WorkerConfig workerConfig = new StandaloneConfig(workerProps);
Review comment:
The advantage of using `ConfigDef.validator` on the
`response.http.headers.config` config key is that this constructor call would
throw an exception if any invalid value is used, and much sooner, too.
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java
##########
@@ -392,6 +393,106 @@ public void testDisableAdminEndpoint() throws IOException
{
Assert.assertEquals(404, response.getStatusLine().getStatusCode());
}
+ @Test
+ public void TestValidCustomizedHttpResponseHeaders() throws IOException {
+ String headerConfig =
+ "add X-XSS-Protection: 1; mode=block, \"add Cache-Control:
no-cache, no-store, must-revalidate\"";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ expectedHeaders.put("X-XSS-Protection", "1; mode=block");
+ expectedHeaders.put("Cache-Control", "no-cache, no-store,
must-revalidate");
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test
+ public void TestDefaultCustomizedHttpResponseHeaders() throws IOException
{
+ String headerConfig = "";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test(expected = ConfigException.class)
+ public void testInvalidHeaderConfigFormat() {
+ String headerConfig = "add X-XSS-Protection";
Review comment:
Might be nice to have quite a few of these tests that verify various
values are invalid and valid, to act as regression tests.
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java
##########
@@ -392,6 +393,106 @@ public void testDisableAdminEndpoint() throws IOException
{
Assert.assertEquals(404, response.getStatusLine().getStatusCode());
}
+ @Test
+ public void TestValidCustomizedHttpResponseHeaders() throws IOException {
+ String headerConfig =
+ "add X-XSS-Protection: 1; mode=block, \"add Cache-Control:
no-cache, no-store, must-revalidate\"";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ expectedHeaders.put("X-XSS-Protection", "1; mode=block");
+ expectedHeaders.put("Cache-Control", "no-cache, no-store,
must-revalidate");
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test
+ public void TestDefaultCustomizedHttpResponseHeaders() throws IOException
{
+ String headerConfig = "";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test(expected = ConfigException.class)
+ public void testInvalidHeaderConfigFormat() {
+ String headerConfig = "add X-XSS-Protection";
+ Map<String, String> workerProps = baseWorkerProps();
+ workerProps.put("offset.storage.file.filename", "/tmp");
+ workerProps.put(WorkerConfig.RESPONSE_HTTP_HEADERS_CONFIG,
headerConfig);
+ WorkerConfig workerConfig = new StandaloneConfig(workerProps);
Review comment:
Should we have tests for the `DistributedConfig` class? Again, much of
the logic should be the same, but the tests would each be simpler if using a
`ConfigDef.Validator`.
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/RestServer.java
##########
@@ -461,4 +469,18 @@ public static String urlJoin(String base, String path) {
return base + path;
}
+ /**
+ * Register header filter to ServletContextHandler.
+ * @param context The serverlet context handler
+ */
+ protected void configureHttpResponsHeaderFilter(ServletContextHandler
context) {
+ String headerConfig =
config.getString(WorkerConfig.RESPONSE_HTTP_HEADERS_CONFIG);
+ log.debug("headerConfig : " + headerConfig);
+ String[] configs = StringUtil.csvSplit(headerConfig);
+ Arrays.stream(configs)
+ .forEach(WorkerConfig::validateHttpResponseHeaderConfig);
Review comment:
Is there a reason we don't want to validate these properties up front
when all of the other configuration validation is being performed, via
`ConfigDef.Validator` on `response.http.headers.config`? If we do that, we
don't need this line.
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/RestServerTest.java
##########
@@ -392,6 +393,106 @@ public void testDisableAdminEndpoint() throws IOException
{
Assert.assertEquals(404, response.getStatusLine().getStatusCode());
}
+ @Test
+ public void TestValidCustomizedHttpResponseHeaders() throws IOException {
+ String headerConfig =
+ "add X-XSS-Protection: 1; mode=block, \"add Cache-Control:
no-cache, no-store, must-revalidate\"";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ expectedHeaders.put("X-XSS-Protection", "1; mode=block");
+ expectedHeaders.put("Cache-Control", "no-cache, no-store,
must-revalidate");
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test
+ public void TestDefaultCustomizedHttpResponseHeaders() throws IOException
{
+ String headerConfig = "";
+ Map<String, String> expectedHeaders = new HashMap<>();
+ checkCustomizedHttpResponseHeaders(headerConfig, expectedHeaders);
+ }
+
+ @Test(expected = ConfigException.class)
+ public void testInvalidHeaderConfigFormat() {
+ String headerConfig = "add X-XSS-Protection";
+ Map<String, String> workerProps = baseWorkerProps();
+ workerProps.put("offset.storage.file.filename", "/tmp");
+ workerProps.put(WorkerConfig.RESPONSE_HTTP_HEADERS_CONFIG,
headerConfig);
+ WorkerConfig workerConfig = new StandaloneConfig(workerProps);
+
+ EasyMock.expect(herder.kafkaClusterId()).andReturn(KAFKA_CLUSTER_ID);
+ EasyMock.expect(herder.plugins()).andStubReturn(plugins);
+ EasyMock.expect(plugins.newPlugins(Collections.emptyList(),
+ workerConfig,
+
ConnectRestExtension.class)).andStubReturn(Collections.emptyList());
+
+ EasyMock.expect(herder.connectors()).andReturn(Arrays.asList("a",
"b"));
+
+ PowerMock.replayAll();
+
+ server = new RestServer(workerConfig);
+ server.initializeServer();
+ server.initializeResources(herder);
+ server.stop();
+ }
+
+ @Test(expected = ConfigException.class)
+ public void testInvalidHeaderConfigAction() {
+ String headerConfig = "badaction X-XSS-Protection: 1; mode=block";
+ Map<String, String> workerProps = baseWorkerProps();
+ workerProps.put("offset.storage.file.filename", "/tmp");
+ workerProps.put(WorkerConfig.RESPONSE_HTTP_HEADERS_CONFIG,
headerConfig);
+ WorkerConfig workerConfig = new StandaloneConfig(workerProps);
+
+ EasyMock.expect(herder.kafkaClusterId()).andReturn(KAFKA_CLUSTER_ID);
+ EasyMock.expect(herder.plugins()).andStubReturn(plugins);
+ EasyMock.expect(plugins.newPlugins(Collections.emptyList(),
+ workerConfig,
+
ConnectRestExtension.class)).andStubReturn(Collections.emptyList());
+
+ EasyMock.expect(herder.connectors()).andReturn(Arrays.asList("a",
"b"));
+
+ PowerMock.replayAll();
+
+ server = new RestServer(workerConfig);
+ server.initializeServer();
+ server.initializeResources(herder);
+ server.stop();
Review comment:
If using `ConfigDef.Validator`, all of these lines would go away, and we
actually don't need mocks of any kind.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
