KAFKA-3692: Add quotes to variables in kafka-run-class.sh

[Ishiihara]

No description provided.

[ijuma]

It would be good to explain the purpose of doing this.

[Ishiihara]

@ijuma @ewencp @granders @hachikuji Please review this. We did a check whether CLASSPATH is defined or empty to determine whether we should include that. When doing that check, we should make sure that spaces in the external CLASSPATH do not mess the check. This causes issues in some murkrake tests.

[ijuma]

Thanks @Ishiihara. Are they needed everywhere or just in conditions?

[Ishiihara]

@ijuma I think it is a good practice to use the double quotes if we are unsure about whether there are spaces or not.

[ijuma]

Someone who is more of an expert in shell scripting should probably review this. Have you tested that this works correctly when the path is already quoted?

[Ishiihara]

We are currently running muckrake with this change. https://jenkins.confluent.io/job/system-test-confluent-platform-branch-builder/28/console

[granders]

@ijuma By "double-quoted", do you mean something like

CLASSPATH="first/path/to.jar:second/path/to.jar"
CLASSPATH="$CLASSPATH"  # this is fine - no change
bin/kafka-run-class.sh STUFF

If this is what you mean, this is not a problem. Double quotes as I understand it cause bash to interpret certain symbols as ordinary characters rather than program symbols to be parsed.

[granders]

@ijuma @granthenke
Regarding KAFKA-1508:

It would definitely be more robust to spaces in path names to have
"$(dirname "$0")" in all the shell scripts. However, we'd also be updating all shell scripts. It seems like a benign addition, but given that the issue has existed for almost 2 years without much clamor for change, I think we're ok without it? That said, everything I've experienced/read on this issue points to double quoting variables in bash as a generally more robust.

In general, given how surprising and quirky bash can be, and the fact that most people (including myself) have a very superficial knowledge of it, we should probably at the very least incorporate a bash linter like the one @gwenshap mentioned (I think this one: https://github.com/koalaman/shellcheck)

[ijuma]

Thanks for the explanation @granders

[granders]

This seems fine to me, unless we also want to also add the fix KAFKA-1508.
Kafka system tests pass on this branch (these use bin scripts quite a bit), as does a run of confluent platform tests which uses this branch of kafka.

[theduderog]

IMHO, since this is a major release, now is the time to quote all variable references. This script is effectively a Kafka API much like the other APIs. I think users are willing to accept some breaking changes if they happen for a good reason, not that this should break anything. Hopefully, people aren't already relying on the variables not being quoted but if they are, asking them to change that assumption is ok for a major release with the justification that not relying on subtle bash expansion rules is a much more robust API.

With a combination of quoting all variable references and checking for file existence before adding files to the classpath, I don't think there's any need to depend on whether nullglob is enabled or disabled. It shouldn't matter.

Specifically, a file exists check should be added to the should_include_file function so that only real files ever get added, regardless of glob expansion mode.

[gwenshap]

We are on a fourth (going on fifth) release candidate of a major release. We need to stabilize the release as much as possible.
IMO, not the time for "lets do a larger change since this is a major release"...

[theduderog]

Stabilizing is the priority as you said. I view this more as a security fix than a scope expansion. Not quoting variable references is like allow XSSing, SQL injection, etc. in a product. I think it's worth plugging such holes.

[granders]

@Ishiihara reverted the patch to dda32cc, and both of us have been doing a mix of manual and automated verification on this:
My verification is as follows:

• Full kafka system test suite against source build (all of the service classes directly or indirectly run kafka-run-class.sh to start services)
• Full CP system tests agains source build of this branch/commit
• Check external CLASSPATH behavior with wildcards with source build and a local releaseTarball build
• Compare equality of generated CLASSPATH with and without patch in source build
• Compare equality of generated CLASSPATH with and without patch in releaseTarball build
• Ran most of the connect CP system tests against release tarball (this exercises start script with external connectors)

[Ishiihara]

@granders @gwenshap @ijuma @theduderog Thanks for all the reviews and suggestions. I made the suggested modifications and add quotes to all variables in all scripts. However, with this change, we do see some unexpected behaviors:

1. In zookeeper-server-start.sh and kafka-server-start.sh, we set KAFKA_HEAP_OPTS with the following code snippet:

if [ "x$KAFKA_HEAP_OPTS" = "x" ]; then
   export KAFKA_HEAP_OPTS="-Xmx512M -Xms512M"
fi

If we double quote KAFKA_HEAP_OPTS in kafka-run-class.sh, Java treats "-Xmx512M -Xms512M" as one configuration and reports that it can not recognize that and fails to start JVM.
2. Same issue with $EXTRA_ARGS in kakfa-server-start.sh

To avoid unexpected behavior, we decided to roll back to dda32cc
as this commit passes both Kafka system test and muckrake tests.

To ensure that the changes are working @granders and I did a couple of manual tests. The things I did includes the following:

1. Run every scripts under bin directory with source build and ensure that they are running property
2. Run every scripts under the release tar ball and ensure that they are running property
3. Passing in external CLASSPATH with wildcard and ensures that it is included in the final CLASSPATH.

[ijuma]

In line 98, we quote $CLASSPATH and $file separately while here we quote both variables together. Is it because of the *?

[granders]

I believe the two forms are equivalent

[ijuma]

Thanks for the thorough testing. There were a couple of cosmetic questions, but changing that would invalidate part of the testing and given the brittleness of this code, I think we should just merge as is.

LGTM.

[ijuma]

Merged to trunk and 0.10.0 branches.