bbende commented on a change in pull request #4613:
URL: https://github.com/apache/nifi/pull/4613#discussion_r512947724
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/state/manager/StandardStateManagerProvider.java
##########
@@ -217,12 +228,17 @@ private static StateProvider createStateProvider(final
File configFile, final Sc
propertyMap.put(descriptor, new
StandardPropertyValue(entry.getValue(),null, parameterLookup,
variableRegistry));
}
- final SSLContext sslContext;
- try {
- sslContext =
SslContextFactory.createSslContext(StandardTlsConfiguration.fromNiFiProperties(properties));
- } catch (TlsException e) {
- logger.error("Encountered an error configuring TLS for state
manager: ", e);
- throw new IllegalStateException("Error configuring TLS for state
manager", e);
+ // add TLS properties if the state provider is a TLS secured Zookeeper
+ if(properties.isZooKeeperClientSecure() &&
providerConfig.getClassName().equals(ZooKeeperStateProvider.class.getName())) {
+ propertyMap.put(new
PropertyDescriptor.Builder().name(NiFiProperties.ZOOKEEPER_CLIENT_SECURE).build(),
+ new StandardPropertyValue("true", null, parameterLookup,
variableRegistry));
+
+ if(properties.isZooKeeperTlsConfigurationPresent()) {
+ StandardTlsConfiguration zooKeeperTlsConfiguration =
StandardTlsConfiguration.zooKeeperTlsFromNiFiProperties(properties);
+ addTlsPropertiesForProvider(propertyMap,
zooKeeperTlsConfiguration, parameterLookup, variableRegistry);
+ } else {
+ addTlsPropertiesForProvider(propertyMap,
standardTlsConfiguration, parameterLookup, variableRegistry);
+ }
Review comment:
I'm not sure we want `StandardStateManagerProvider` to perform specific
logic based on knowing the implementation. I know there are very few
implementations, and technically the ZK implementation lives in the framework,
but it feels like leaking details between the API and the implementations.
The way the API is setup, the `StateProvider` extends
`ConfigurableComponent` which defines `List<PropertyDescriptor>
getPropertyDescriptors()`, this is how the implementation specifies the
configuration it needs. The framework then goes through each of these property
descriptors and gets the value from the config in `state-management.xml` and
passes it in the init context to say "here is the config you asked for". In
this case, the component didn't define these descriptors, but the framework is
deciding it needs them.
I can see two alternative options worth considering...
1) Provide a general mechanism for implementations to be given an instance
of `NiFiProperties`. This way the ZK implementation can make the decision about
which properties to look at.
We took this approach with our
Authorizer/UserGroupProvider/AccessPolicyProvider API. There is an annotation
`@AuthorizerContext` which the framework looks for on methods or member
variables of the implementing class and indicates dependency injection is
needed.
https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-authorizer/src/main/java/org/apache/nifi/authorization/AuthorizerFactoryBean.java#L395-L451
2) If we don't want to pass around `NiFIProperties`, we could introduce a
new config/context class as part of `StateProviderInitializationContext`,
similar to how you passed in SSLContext, but this would be just the
configuration values for NiFI's keystore/truststore.
A similar example is the `KerberosContext` which was created to pass
around the service principal/keytab from nifi.properties:
https://github.com/apache/nifi/blob/main/nifi-api/src/main/java/org/apache/nifi/kerberos/KerberosContext.java
The downside with this option is I think it would only make sense to
pass around the main NiFi TLS properties in `TLSContext`, but then we'd still
be left needing to duplicate the ZK TLS config in the case when its not using
NiFI's main TLS properties, but we already do that for the connection string.
Thoughts?
----------------------------------------------------------------
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]
