NIFI-14458 - Added run command to nifi.cmd

[philci52]

Summary

NIFI-14458
Added run command to nifi.cmd to allow NiFi to run in the foreground and be used with Apache ProcRun

Tracking

Issue Tracking

• Apache NiFi Jira issue created

Pull Request Tracking

• Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
• Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

• Pull Request based on current revision of the main branch
• Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

• Build completed using mvn clean install -P contrib-check
  • JDK 21

Licensing

• New dependencies are compatible with the Apache License 2.0 according to the License Policy
• New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

• Documentation formatting appears as expected in rendered files

[exceptionfactory]

Thanks for proposing this change @philci52.

However, calling BootstrapProcess with the start command is different than the behavior of the nifi.sh script. From that perspective, the behavior should be the same across both platforms.

If you aren't in a position to implement those details, I may be able to take a look at that approach.

[philci52]

I guess I don't understand the difference. Is it in the Java code? Will I still be able to run "nifi stop" to stop the application if run that way?

[exceptionfactory]

Here's the section of nifi.sh that handles the run command:

https://github.com/apache/nifi/blob/main/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/bin/nifi.sh#L272

As shown, it calls the BootstrapProcess class with get-run-command, then executes the returned command.

Yes, nifi.cmd stop will work either way.

[philci52]

Looks like there will be more issues with paths that have spaces (probably an issue on linux too). https://github.com/apache/nifi/blob/main/nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/command/GetRunCommandBootstrapCommand.java#L78

[philci52]

@exceptionfactory Fixing the issue with the paths that have spaces in the GetRunCommandBootstrapCommand.java is a potentially breaking change. If I just surround all arguments with double quotes, while that works for me, it will break any users who have figured out that they need quotes in bootstrap.conf. I'm not sure how common this use case is.

For example, if I have java.arg.7="-Dtest=test test" in my bootstrap.conf to get around this existing bug, then simply wrapping the arguments with double quotes would result in the following in the returned command: ""-Dtest=test test"" rather than then previous "-Dtest=test test".

Additionally, another way to get around this on windows would have been to use the caret to escape the next character, for example, I could have used java.arg.7=-Dtest=test^ test to get around the existing bug. Surrounding this with double quotes would result in keeping the carat in the string. I don't think this solution is used a lot, but its a possibility.

So, this leaves a few courses of action that I see

1. Introduce the double quotes, potentially breaking things and let users update their bootstrap.conf when conflicts occur.
2. Check to see if an argument is already wrapped in double quotes before wrapping it, which would not find all cases (for example -Dtest=test" "test,) but would allow most of the existing cases to work as is. This does introduce an inconsistency in how things work though
3. Add a java or bootstrap property to determine if commands should be wrapped. I'm not a fan of this myself as it creates some potential issues, documentation, maintenance, etc
4. Other options similar to 2 ?

[exceptionfactory]

Thanks for the thoughtful evaluation of options @philci52.

I think there are two different elements to consider.

The first aspect is bootstrap.conf. Property values using java.arg contain user-controller values, and it seems acceptable to depend the user to introduce quotes when necessary, such as the following:

java.arg.100=-Dsystem.property.name=escaped\\ string

The second aspect is the standard arguments to the command, which include the following from StandardProcessBuilderProvider:

1. Class Path argument containing file paths
2. Log Directory system property containing log directory path
3. Application Properties system property containing file path
4. Management Server Address
5. Additional arguments from bootstrap.conf
6. Application class name to run

Arguments 1, 2, and 3 contain paths that may need to be quoted, so for those cases, quoting the value of the argument seems like the way to go. Arguments 4 and 6 do not need to be quoted. The additional arguments in 5 can be addressed as described above.

With that background, it seems like making adjustments to StandardProcessBuilderProvider could provide a workable solution that should apply to all platforms.

[philci52]

@exceptionfactory Thank you for the quick feedback. There is one other path that may need to be quoted and that is the (7) path to java itself.

With regards to using StandardProcessBuilderProvider, that class will have to know if its creating arguments for the shell/console or for using ProcessBuilder. When using ProcessBuilder, passing "argument", the child process will actually get "argument" as opposed to argument when passed from the shell/console. Is that acceptable?

[exceptionfactory]

Good question @philci52, the List<String> command property of ProcessBuilder may not need the escaping.

I will take a closer look at this and follow up soon.

One other potential path is to pass the run command to the BootstrapProcess class and invoke the RunBootstrapCommand, but that may have other consequences that need to be considered.

[philci52]

Here is a comment from some of our internal code regarding the use of process builder and windows/linux differences. This application uses an XML file for configuration, but I figured it would be relevant to this conversation

Java Application Operating system Differences:
MS Windows:
When calling a java process on MS Windows from OURAPP, java will automatically
expand glob characters and Java uses double quotes to suppress glob expansion.  Glob characters
are []{}*?.  This can lead to some unexpected behavior when calling java processes from OURAPP.
To suppress this expansion when calling a java application from within OURAPP, you must surround the
argument with double quotes.  For example <arg>&quot;test_*&quot;</arg> or
<arg><![CDATA["test_*"]]></arg>.  The application will then get test_* as the parameter.
If you need to place quotes in argument itself, you must surround the argument with double quotes
and the use two double quotes to represent one double quote.  For example, if you want the string 
P"J"D, then you would use <arg><![CDATA["P""J""D"]]></arg>
Linux/Unix:
When calling a java process from within from OURAPP on Linux, the java application will get
the exact value that you pass it, except for -cp argument, which java will expand.
If you want glob expansion, you must wrap the java application with a call to the shell.

So it appears that windows will remove the quote characters and linux will keep them.

[philci52]

@exceptionfactory Any updates on how you want to handle the issues here?

[exceptionfactory]

Thanks for checking @philci52, I evaluated some options last week using the existing run command, but didn't find a satisfactory solution.

At the moment, evaluating the impact of selective quoting in the StandardProcessBuilderProvider, and updating the Windows nifi.cmd to use get-run-command seems like the best way forward.

Are you in a position to implement and evaluate the approach for Windows, along with the impact on Linux platforms? If not, I may be able to take a closer look.

[philci52]

I can work on this, but it will take me a few weeks to get back to it.