NIFI-8462 Refactored PutSyslog and ListenSyslog using Netty by exceptionfactory · Pull Request #5044 · apache/nifi

exceptionfactory · 2021-04-30T15:14:39Z

Description of PR

NIFI-8462 Refactors the PutSyslog and ListenSyslog Processors to use Netty for socket communication. This approach replaces the use of custom socket channel dispatcher classes in nifi-processor-utils. Leveraging the Netty SslHandler also removes the use of custom SSLSocket and SSLEngine handling in nifi-security-socket-ssl classes. This approach resolves latent issues with TLS 1.3 communication in these processors, described in NIFI-7468.

The new nifi-event-transport module abstracts communication handling for both UDP and TCP communication, providing interfaces and classes that can be leveraged in other components. Refactoring other components to leverage Netty should eventually remove the need for error-prone SSLEngine handling in custom classes.

Changes also include refactored unit tests for PutSyslog and ListenSyslog, eliminating the use of mocked senders and removing unnecessary integration tests.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

...main/java/org/apache/nifi/event/transport/netty/ByteArrayMessageNettyEventServerFactory.java

ottobackwards · 2021-05-01T15:43:34Z

If someone was going to use these new classes, it might be good to have a readme or something for this part of the code

...fi-event-transport/src/main/java/org/apache/nifi/event/transport/netty/NettyEventSender.java

exceptionfactory · 2021-05-01T20:09:03Z

If someone was going to use these new classes, it might be good to have a readme or something for this part of the code

Thanks for the feedback @ottobackwards! Providing some additional documentation would be helpful, although I think it would be better to wait until other components start leveraging this module. I attempted to provide some high-level abstractions in the EventSender and EventServer interfaces, but even these interfaces and implementations are geared sending and receiving byte arrays for Syslog messages. There should be some reuse of these classes for other processors, such as PutTCP and ListenTCP, among others. If there are particular components that could use some additional description, feel free to point those out. However, I expect some changes when it comes to leveraging similar approaches with other components.

Please let me know if you have any additional comments or questions!

gresockj

This will be a nice upgrade, @exceptionfactory! I tested it successfully with UDP and TCP with and without SSL Context Service, works as expected. Just a few comments to address.

...-event-transport/src/main/java/org/apache/nifi/event/transport/message/ByteArrayMessage.java

...ent-transport/src/main/java/org/apache/nifi/event/transport/netty/EventLoopGroupFactory.java

...le/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSyslog.java

...t-transport/src/main/java/org/apache/nifi/event/transport/netty/NettyEventSenderFactory.java

gresockj · 2021-05-14T11:12:16Z

...le/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/PutSyslog.java

                    "messages will be sent over a secure connection.")
            .required(false)
            .identifiesControllerService(SSLContextService.class)
+            .dependsOn(PROTOCOL, TCP_VALUE)


I noticed that in ListenSyslog, the SSL Context Service property is always available, but in PutSyslog it's conditional on TCP being set. Can you add the condition to both?

Also, if you have an SSL Context Service selected and then change it back to UDP, the processor is marked as invalid even though the SSL Context Service property is no longer available to be configured. Is it possible to unset this property if protocol is set to UDP, or is that just a limitation of the current "dependsOn" functionality?

Good catch, I had intended to update both processors to make SSL Context Service depend on TCP.

I noticed the issue with SSL Context Service and changing from TCP to UDP. It seems like that is something that might make sense to address at the framework level.

...nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/ListenSyslog.java

- Added nifi-event-transport module encapsulating Netty classes - Refactored unit tests for PutSyslog and ListenSyslog - Removed integration tests for PutSyslog and ListenSyslog

…dressed other issues

exceptionfactory · 2021-05-14T13:51:07Z

Thanks for the thorough review and helpful feedback @gresockj! I added a commit including the changes and rebased on main to incorporate recent updates.

thenatog · 2021-05-25T02:44:35Z

Tested this out using the template found here: https://community.cloudera.com/t5/Community-Articles/NiFi-Send-to-syslog/ta-p/248638 and found that PutSyslog and ListenSyslog are working. Reviewed the code and it looks good.

thenatog · 2021-05-25T14:52:21Z

+1 will merge.

- Added nifi-event-transport module encapsulating Netty classes - Refactored unit tests for PutSyslog and ListenSyslog - Removed integration tests for PutSyslog and ListenSyslog NIFI-8462 Added context.yield() in PutSyslog when no FlowFiles and addressed other issues NIFI-8462 Removed unused import of ExpressionLanguageScope Signed-off-by: Nathan Gough <thenatog@gmail.com> This closes apache#5044.

exceptionfactory force-pushed the NIFI-8462 branch from e9386ba to 3ac102f Compare April 30, 2021 22:00

ottobackwards reviewed May 1, 2021

View reviewed changes

...main/java/org/apache/nifi/event/transport/netty/ByteArrayMessageNettyEventServerFactory.java Show resolved Hide resolved

ottobackwards reviewed May 1, 2021

View reviewed changes

...fi-event-transport/src/main/java/org/apache/nifi/event/transport/netty/NettyEventSender.java Show resolved Hide resolved

gresockj requested changes May 14, 2021

View reviewed changes

exceptionfactory added 2 commits May 14, 2021 08:35

NIFI-8462 Refactored PutSyslog and ListenSyslog using Netty

be366d1

- Added nifi-event-transport module encapsulating Netty classes - Refactored unit tests for PutSyslog and ListenSyslog - Removed integration tests for PutSyslog and ListenSyslog

NIFI-8462 Added context.yield() in PutSyslog when no FlowFiles and ad…

e166cfc

…dressed other issues

exceptionfactory force-pushed the NIFI-8462 branch from 3ac102f to e166cfc Compare May 14, 2021 13:50

NIFI-8462 Removed unused import of ExpressionLanguageScope

cbc5748

gresockj approved these changes May 14, 2021

View reviewed changes

thenatog closed this in a3365c8 May 25, 2021

exceptionfactory mentioned this pull request Sep 30, 2021

NIFI-9223 Correct ListenSyslog with default address of 0.0.0.0 #5426

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-8462 Refactored PutSyslog and ListenSyslog using Netty#5044

NIFI-8462 Refactored PutSyslog and ListenSyslog using Netty#5044
exceptionfactory wants to merge 3 commits intoapache:mainfrom
exceptionfactory:NIFI-8462

exceptionfactory commented Apr 30, 2021

Uh oh!

Uh oh!

ottobackwards commented May 1, 2021

Uh oh!

Uh oh!

exceptionfactory commented May 1, 2021

Uh oh!

gresockj left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gresockj May 14, 2021

Uh oh!

exceptionfactory May 14, 2021

Uh oh!

Uh oh!

exceptionfactory commented May 14, 2021

Uh oh!

thenatog commented May 25, 2021

Uh oh!

thenatog commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

exceptionfactory commented Apr 30, 2021

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

Uh oh!

ottobackwards commented May 1, 2021

Uh oh!

Uh oh!

exceptionfactory commented May 1, 2021

Uh oh!

gresockj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gresockj May 14, 2021

Choose a reason for hiding this comment

Uh oh!

exceptionfactory May 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

exceptionfactory commented May 14, 2021

Uh oh!

thenatog commented May 25, 2021

Uh oh!

thenatog commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants