thenatog commented on pull request #4753: URL: https://github.com/apache/nifi/pull/4753#issuecomment-761161322
> @thenatog I've been able to verify that this is functional, both for the cluster and state management, but there's some behavioral stuff I want to sanity check. I don't think the intent of the code is to require doing either of these, but right now: > > * TLS is required for the embedded ZK when cluster TLS is enabled but NiFi won't try to connect securely unless `nifi.zookeeper.client.secure` is set to true in nifi.properties. > * Similarly, the embedded ZK won't actually run with TLS enabled unless `secureClientPort` is set in zookeeper.properties. It appears that `clientPort` is successfully removed but `secureClientPort` doesn't get added. > > Am I doing something wrong or is this the intended behavior? There are pros to not ignoring or overriding configuring but I think the upgrade is going to be frustrating and we might need to be warning more loudly. > > Just to re-up the discussion in one of the JIRAs, these were suggested for the embedded server. You have most of this but I'm having trouble validating the first and last bullet points. > > * NiFi restricts embedded ZK to only allow TLS connections if TLS configs are provided, avoiding the surprise to the user even if both options are supported simultaneously given the provided server configuration secureClientPort and clientPort (this property is exposed by default in our zookeeper.properties template) > * Add comments in zookeeper.properties template explaining the ports are mutually exclusive for embedded NiFi ZK > * Add docs in Admin Guide explaining changes from normal ZK operation > * Clarify that an external ZK is preferred but embedded ZK is provided as accelerator only (similar to TLS TK) > * Detect conflicting config & warn loudly but continue with TLS only Yes, I had noticed at least one issue with nifi.zookeeper.client.secure and was working on some code to rectify the issue but was finding it difficult. As you mentioned, I removed some code that overrode configuration from the previous PR because I don't believe it's consistent with the rest of NiFi. Generally, if something is not configured right, things won't start. I did not think we should take the connection string port and attach that as the secure port being used by the embedded server, and rather instead I think things should fail. I'll have another commit available soon which should address some of the above. ---------------------------------------------------------------- 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: [email protected]
