exceptionfactory commented on a change in pull request #4972:
URL: https://github.com/apache/nifi/pull/4972#discussion_r608067336
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/pom.xml
##########
@@ -256,5 +256,10 @@
<version>${nifi.groovy.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.eclipse.jetty</groupId>
+ <artifactId>jetty-servlets</artifactId>
Review comment:
Is this dependency addition necessary given that the most recent commit
does not include a new filter class?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -715,20 +717,40 @@ private static int
determineMaxWebRequestsPerSecond(NiFiProperties props) {
return configuredMaxRequestsPerSecond > 0 ?
configuredMaxRequestsPerSecond : defaultMaxRequestsPerSecond;
}
+ private static long determineRequestTimeoutInMilliseconds(NiFiProperties
props) {
+ long defaultRequestTimeout =
Math.round(FormatUtils.getPreciseTimeDuration(NiFiProperties.DEFAULT_WEB_REQUEST_TIMEOUT,
TimeUnit.MILLISECONDS));
+ long configuredRequestTimeout = 0L;
+ try {
+ configuredRequestTimeout =
Math.round(FormatUtils.getPreciseTimeDuration(props.getWebRequestTimeout(),
TimeUnit.MILLISECONDS));
+ } catch (final NumberFormatException e) {
+ logger.warn("Exception parsing property " +
NiFiProperties.WEB_REQUEST_TIMEOUT + "; using default value: " +
defaultRequestTimeout);
Review comment:
Recommend adjusting the log statement to use placeholders:
```suggestion
logger.warn("Exception parsing property [{}]: using default
value [{}] ", NiFiProperties.WEB_REQUEST_TIMEOUT, defaultRequestTimeout);
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -715,20 +717,40 @@ private static int
determineMaxWebRequestsPerSecond(NiFiProperties props) {
return configuredMaxRequestsPerSecond > 0 ?
configuredMaxRequestsPerSecond : defaultMaxRequestsPerSecond;
}
+ private static long determineRequestTimeoutInMilliseconds(NiFiProperties
props) {
Review comment:
Recommend adding `final` modifier:
```suggestion
private static long determineRequestTimeoutInMilliseconds(final
NiFiProperties props) {
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
##########
@@ -715,20 +717,40 @@ private static int
determineMaxWebRequestsPerSecond(NiFiProperties props) {
return configuredMaxRequestsPerSecond > 0 ?
configuredMaxRequestsPerSecond : defaultMaxRequestsPerSecond;
}
+ private static long determineRequestTimeoutInMilliseconds(NiFiProperties
props) {
+ long defaultRequestTimeout =
Math.round(FormatUtils.getPreciseTimeDuration(NiFiProperties.DEFAULT_WEB_REQUEST_TIMEOUT,
TimeUnit.MILLISECONDS));
+ long configuredRequestTimeout = 0L;
+ try {
+ configuredRequestTimeout =
Math.round(FormatUtils.getPreciseTimeDuration(props.getWebRequestTimeout(),
TimeUnit.MILLISECONDS));
+ } catch (final NumberFormatException e) {
+ logger.warn("Exception parsing property " +
NiFiProperties.WEB_REQUEST_TIMEOUT + "; using default value: " +
defaultRequestTimeout);
+ }
+
+ return configuredRequestTimeout > 0 ? configuredRequestTimeout :
defaultRequestTimeout;
+ }
+
/**
* Adds the {@link org.eclipse.jetty.servlets.DoSFilter} to the specified
context and path. Limits incoming web requests to {@code
maxWebRequestsPerSecond} per second.
*
* @param path the path to apply this filter
* @param webAppContext the context to apply this filter
* @param maxWebRequestsPerSecond the maximum number of allowed requests
per second
*/
- private static void addWebRequestRateLimitingFilter(String path,
WebAppContext webAppContext, int maxWebRequestsPerSecond) {
+ private static void addWebRequestRateLimitingFilter(String path,
WebAppContext webAppContext, int maxWebRequestsPerSecond, long
requestTimeoutInMilliseconds, final String ipWhitelist) {
FilterHolder holder = new FilterHolder(DoSFilter.class);
holder.setInitParameters(new HashMap<String, String>() {{
put("maxRequestsPerSec", String.valueOf(maxWebRequestsPerSecond));
+ put("maxRequestMs", String.valueOf(requestTimeoutInMilliseconds));
+ put("ipWhitelist", String.valueOf(ipWhitelist));
}});
holder.setName(DoSFilter.class.getSimpleName());
- logger.debug("Adding DoSFilter to context at path: " + path + " with
max req/sec: " + maxWebRequestsPerSecond);
+
+ String ipWhitelistLoggable = ipWhitelist;
+ if(ipWhitelist == null) {
+ ipWhitelistLoggable = "none";
+ }
+ logger.info("Adding DoSFilter to context at path: {} with max req/sec:
{}, request timeout: {} ms. Whitelisted IPs not subject to filter: {}",
+ path, maxWebRequestsPerSecond, requestTimeoutInMilliseconds,
ipWhitelistLoggable);
Review comment:
The loggable representation could be handled using
`StringUtils.defaultIfEmpty()`
```suggestion
logger.info("Adding DoSFilter Path [{}] Max Requests per Second [{}]
Timeout [{} ms] Excluded Addresses [{}]",
path, maxWebRequestsPerSecond, requestTimeoutInMilliseconds,
StringUtils.defaultIfEmpty(ipWhitelist, "none"));
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/test/groovy/org/apache/nifi/web/server/JettyServerGroovyTest.groovy
##########
@@ -540,6 +540,21 @@ class JettyServerGroovyTest extends GroovyTestCase {
assert filterNames.contains("DoSFilter")
assert !filterNames.contains("ContentLengthFilter")
}
+
+ @Test
+ void testDetermineRequestTimeoutInMilliseconds() {
+ // Arrange
+ Map badProps = [
+ (NiFiProperties.WEB_REQUEST_TIMEOUT): "50 sec",
+ ]
+
+ NiFiProperties mockProps = new StandardNiFiProperties(new
Properties(badProps))
+
+ // Act
+ long requestTimeout =
JettyServer.determineRequestTimeoutInMilliseconds(mockProps)
Review comment:
Recommend avoiding calls to private methods even though Groovy allows it
to work. Instead, is it possible to retrieve the configured DoSFilter and
evaluate the configuration? Otherwise, it seems acceptable to remove this test
method.
--
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
