turcsanyip commented on a change in pull request #4603:
URL: https://github.com/apache/nifi/pull/4603#discussion_r513067655
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -74,7 +78,10 @@
@Tags({"ingest", "http", "https", "rest", "listen"})
@CapabilityDescription("Starts an HTTP Server and listens on a given base path
to transform incoming requests into FlowFiles. "
+ "The default URI of the Service will be
http://{hostname}:{port}/contentListener. Only HEAD and POST requests are "
- + "supported. GET, PUT, and DELETE will result in an error and the
HTTP response status code 405.")
+ + "supported. GET, PUT, and DELETE will result in an error and the
HTTP response status code 405. "
+ + "GET is supported on <service_URI>/healthcheck. If the service is
available, it returns \"200 OK\" with an empty response body. "
+ + "The health check functionality can be configured to be accessible
via a different port. "
Review comment:
The "empty response body" is not correct any more.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -104,6 +111,21 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.POSITIVE_INTEGER_VALIDATOR)
.build();
+ public static final PropertyDescriptor HEALTH_CHECK_PORT = new
PropertyDescriptor.Builder()
+ .name("health-check-port")
+ .displayName("Listening Port for health check requests")
Review comment:
Property names should be capitalized as titles: Listening Port for
Health Check Requests
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -290,29 +316,23 @@ synchronized private void
createHttpServerFromService(final ProcessContext conte
// get the configured port
final int port =
context.getProperty(PORT).evaluateAttributeExpressions().asInteger();
- final ServerConnector connector;
- final HttpConfiguration httpConfiguration = new HttpConfiguration();
- if (keystorePath == null) {
- // create the connector
- connector = new ServerConnector(server, new
HttpConnectionFactory(httpConfiguration));
- } else {
- // configure the ssl connector
- httpConfiguration.setSecureScheme("https");
- httpConfiguration.setSecurePort(port);
- httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ final ServerConnector connector = createServerConnector(server, port,
sslContextService, sslRequired, needClientAuth);
- // build the connector
+ List<Connector> connectors = new ArrayList<>(2);
+ connectors.add(connector);
- connector = new ServerConnector(server, new
SslConnectionFactory(contextFactory, "http/1.1"), new
HttpConnectionFactory(httpConfiguration));
+ // Add a separate connector for the health check port (if specified)
+ final String healthCheckPortString =
context.getProperty(HEALTH_CHECK_PORT).evaluateAttributeExpressions().getValue();
Review comment:
`asInteger()` could be used. In that way there is no need for the
additional string variable.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -174,12 +196,38 @@
public static final String CONTEXT_ATTRIBUTE_RETURN_CODE = "returnCode";
public static final String CONTEXT_ATTRIBUTE_MULTIPART_REQUEST_MAX_SIZE =
"multipartRequestMaxSize";
public static final String CONTEXT_ATTRIBUTE_MULTIPART_READ_BUFFER_SIZE =
"multipartReadBufferSize";
+ public static final String CONTEXT_ATTRIBUTE_PORT = "port";
private volatile Server server = null;
private final ConcurrentMap<String, FlowFileEntryTimeWrapper> flowFileMap
= new ConcurrentHashMap<>();
private final AtomicReference<ProcessSessionFactory>
sessionFactoryReference = new AtomicReference<>();
private final AtomicReference<StreamThrottler> throttlerRef = new
AtomicReference<>();
+ @Override
+ protected Collection<ValidationResult> customValidate(ValidationContext
context) {
+ List<ValidationResult> results = new ArrayList<>(1);
+
+ validatePortsAreNotEqual(context, results);
+
+ return results;
+ }
+
+ private void validatePortsAreNotEqual(ValidationContext context,
Collection<ValidationResult> validationResults) {
+ String healthCheckPortString =
context.getProperty(HEALTH_CHECK_PORT).evaluateAttributeExpressions().getValue();
Review comment:
`asInteger()` could be used. In that way there is no need for the
additional string variable.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -290,29 +316,23 @@ synchronized private void
createHttpServerFromService(final ProcessContext conte
// get the configured port
final int port =
context.getProperty(PORT).evaluateAttributeExpressions().asInteger();
- final ServerConnector connector;
- final HttpConfiguration httpConfiguration = new HttpConfiguration();
- if (keystorePath == null) {
- // create the connector
- connector = new ServerConnector(server, new
HttpConnectionFactory(httpConfiguration));
- } else {
- // configure the ssl connector
- httpConfiguration.setSecureScheme("https");
- httpConfiguration.setSecurePort(port);
- httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ final ServerConnector connector = createServerConnector(server, port,
sslContextService, sslRequired, needClientAuth);
- // build the connector
+ List<Connector> connectors = new ArrayList<>(2);
+ connectors.add(connector);
Review comment:
Server.addConnector() could simply be used instead.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -351,6 +372,59 @@ synchronized private void
createHttpServerFromService(final ProcessContext conte
initialized.set(true);
}
+ private ServerConnector createServerConnector(Server server, int port,
SSLContextService sslContextService, boolean sslRequired, boolean
needClientAuth) {
+ final SslContextFactory contextFactory =
createSslContextFactory(sslContextService, sslRequired, needClientAuth);
Review comment:
I don't think `createServerConnector()` should be overloaded / separated
into 2 methods.
This single line can be moved to the next method.
Actually, creating the `SslContextFactory` is only needed in the `if
(sslRequired) {}` block and should not be called otherwise.
##########
File path:
nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenHTTP.java
##########
@@ -351,6 +372,59 @@ synchronized private void
createHttpServerFromService(final ProcessContext conte
initialized.set(true);
}
+ private ServerConnector createServerConnector(Server server, int port,
SSLContextService sslContextService, boolean sslRequired, boolean
needClientAuth) {
+ final SslContextFactory contextFactory =
createSslContextFactory(sslContextService, sslRequired, needClientAuth);
+ return createServerConnector(server, port, contextFactory,
sslRequired);
+ }
+
+ private ServerConnector createServerConnector(Server server, int port,
SslContextFactory contextFactory, boolean sslRequired) {
+ final ServerConnector connector;
+ final HttpConfiguration httpConfiguration = new HttpConfiguration();
+ if (sslRequired) {
+ // configure the ssl connector
+ httpConfiguration.setSecureScheme("https");
+ httpConfiguration.setSecurePort(port);
+ httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+
+ // build the connector
+ connector = new ServerConnector(server, new
SslConnectionFactory(contextFactory, "http/1.1"), new
HttpConnectionFactory(httpConfiguration));
+ } else {
+ // create the connector
+ connector = new ServerConnector(server, new
HttpConnectionFactory(httpConfiguration));
+ }
+
+ // configure the port
+ connector.setPort(port);
+ return connector;
+ }
+
+ private SslContextFactory createSslContextFactory(SSLContextService
sslContextService, boolean sslRequired, boolean needClientAuth) {
Review comment:
I think `sslRequired` parameter can be omitted and this method should
only be called when SSL is required and `sslContextService` is not null (the
null check can be omitted too).
If this is not the case, then the whole method body should be guarded with
`if (sslRequired && sslContextService != null)`.
----------------------------------------------------------------
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
