[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #961: MINIFICPP-1440 PublishKafka processor's "security protocol" should ha…

[GitBox <gi...@apache.org>]

szaszm commented on a change in pull request #961:
URL: https://github.com/apache/nifi-minifi-cpp/pull/961#discussion_r544326007



##########
File path: extensions/librdkafka/PublishKafka.cpp
##########
@@ -126,7 +126,11 @@ const core::Property PublishKafka::CompressCodec(
 const core::Property PublishKafka::MaxFlowSegSize(
     core::PropertyBuilder::createProperty("Max Flow Segment Size")->withDescription("Maximum flow content payload segment size for the kafka record. 0 B means unlimited.")
         ->isRequired(false)->withDefaultValue<core::DataSizeValue>("0 B")->build());
-const core::Property PublishKafka::SecurityProtocol("Security Protocol", "Protocol used to communicate with brokers", "");
+const core::Property PublishKafka::SecurityProtocol(
+        core::PropertyBuilder::createProperty("Security Protocol")
+        ->withDescription("Protocol used to communicate with brokers")
+        ->withAllowableValues<std::string>({SECURITY_PROTOCOL_PLAINTEXT, SECURITY_PROTOCOL_SSL, SECURITY_PROTOCOL_SASL_PLAINTEXT, SECURITY_PROTOCOL_SASL_SSL})

Review comment:
       When looking at `PublishKafka.cpp:686`, it seems like only `SECURITY_PROTOCOL_SSL` is actually supported, and the default empty value is interpreted as plaintext.
   
   I suggest restricting the set of "allowable values" to plaintext and ssl, and handling plaintext near `PublishKafka.cpp:687`. I think doing nothing is a valid handling of plaintext.




----------------------------------------------------------------
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:
users@infra.apache.org