exceptionfactory commented on a change in pull request #4843:
URL: https://github.com/apache/nifi/pull/4843#discussion_r582955753
##########
File path:
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureDataLakeStorageProcessor.java
##########
@@ -140,17 +150,39 @@ public static DataLakeServiceClient
getStorageClient(PropertyContext context, Fl
storageClient = new
DataLakeServiceClientBuilder().endpoint(endpoint).sasToken(sasToken)
.buildClient();
} else if (accessToken != null) {
- TokenCredential credential = tokenRequestContext ->
Mono.just(accessToken);
+ final TokenCredential credential = tokenRequestContext ->
Mono.just(accessToken);
storageClient = new
DataLakeServiceClientBuilder().endpoint(endpoint).credential(credential)
- .buildClient();
- } else if(useManagedIdentity){
- final ManagedIdentityCredential misCrendential = new
ManagedIdentityCredentialBuilder()
- .build();
- storageClient = new DataLakeServiceClientBuilder()
- .endpoint(endpoint)
- .credential(misCrendential)
- .buildClient();
+ .buildClient();
+ } else if (useManagedIdentity) {
+ final ManagedIdentityCredential misCredential = new
ManagedIdentityCredentialBuilder()
+ .build();
+ storageClient = new DataLakeServiceClientBuilder()
+ .endpoint(endpoint)
+ .credential(misCredential)
+ .buildClient();
+ } else if (servicePrincipalTenantId != null &&
servicePrincipalClientId != null && servicePrincipalClientSecret != null) {
+ final ClientSecretCredential credential = new
ClientSecretCredentialBuilder()
+ .tenantId(servicePrincipalTenantId)
+ .clientId(servicePrincipalClientId)
+ .clientSecret(servicePrincipalClientSecret)
+ .build();
+
+ storageClient = new DataLakeServiceClientBuilder()
+ .endpoint(endpoint)
+ .credential(credential)
+ .buildClient();
+ } else if (servicePrincipalTenantId != null &&
servicePrincipalClientId != null && servicePrincipalClientCertificatePath !=
null && servicePrincipalClientCertificatePassword != null) {
Review comment:
Using `StringUtils.isNoneBlank()` would make this a bit more concise:
```suggestion
} else if (StringUtils.isNoneBlank(servicePrincipalTenantId,
servicePrincipalClientId, servicePrincipalClientCertificatePath,
servicePrincipalClientCertificatePassword)) {
```
##########
File path:
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -74,12 +75,56 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_TENANT_ID = new
PropertyDescriptor.Builder()
+ .name("service-principal-tenant-id")
+ .displayName("Service Principal Tenant ID")
+ .description("Tenant ID of the Azure Active Directory hosting the
Service Principal. The property is required when Service Principal
authentication is used.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_ID = new
PropertyDescriptor.Builder()
+ .name("service-principal-client-id")
+ .displayName("Service Principal Client ID")
+ .description("Client ID (or Application ID) of the
Client/Application having the Service Principal. The property is required when
Service Principal authentication is used. " +
+ "Also 'Service Principal Client Secret' or 'Service
Principal Client Certificate' must be specified in this case.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_SECRET =
new PropertyDescriptor.Builder()
+ .name("service-principal-client-Secret")
+ .displayName("Service Principal Client Secret")
+ .description("Password of the Client/Application.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor
SERVICE_PRINCIPAL_CLIENT_CERTIFICATE = new PropertyDescriptor.Builder()
+ .name("service-principal-client-certificate")
+ .displayName("Service Principal Client Certificate")
+ .description("SSL Context Service referencing the keystore with
the client certificate of the Client/Application. Only PKCS12 (.pfx) keystore
type is supported. " +
+ "The keystore must contain a single key and the password
of the keystore and the key must be the same.")
+ .identifiesControllerService(SSLContextService.class)
Review comment:
The property name is somewhat confusing in light of this property
referencing the `SSLContextService`. Given that SSLContextService is not
really used to provide an `SSLContext` object, what about changing this
property to be just the file path? That would also avoid the need for
introducing the additional dependency on `nifi-ssl-context-service-api`. In
that case, the File Path Validator could be used to ensure that the file exists.
##########
File path:
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -74,12 +75,56 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_TENANT_ID = new
PropertyDescriptor.Builder()
+ .name("service-principal-tenant-id")
+ .displayName("Service Principal Tenant ID")
+ .description("Tenant ID of the Azure Active Directory hosting the
Service Principal. The property is required when Service Principal
authentication is used.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
Review comment:
If this property is considered sensitive, should it support expression
language? Retrieving a sensitive property from flow file attributes would
expose the value in provenance events. This same question applies to the other
properties marked as sensitive.
##########
File path:
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -74,12 +75,56 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_TENANT_ID = new
PropertyDescriptor.Builder()
+ .name("service-principal-tenant-id")
+ .displayName("Service Principal Tenant ID")
+ .description("Tenant ID of the Azure Active Directory hosting the
Service Principal. The property is required when Service Principal
authentication is used.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_ID = new
PropertyDescriptor.Builder()
+ .name("service-principal-client-id")
+ .displayName("Service Principal Client ID")
+ .description("Client ID (or Application ID) of the
Client/Application having the Service Principal. The property is required when
Service Principal authentication is used. " +
+ "Also 'Service Principal Client Secret' or 'Service
Principal Client Certificate' must be specified in this case.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_SECRET =
new PropertyDescriptor.Builder()
+ .name("service-principal-client-Secret")
+ .displayName("Service Principal Client Secret")
+ .description("Password of the Client/Application.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
Review comment:
Based on the approach of some other components, it seems best to avoid
supporting expression language for password properties. This can still be
parameterized using parameter contexts, which preserves the ability to use
sensitive property encryption, and also supports passwords that may look like
expression language strings.
##########
File path:
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -74,12 +75,56 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_TENANT_ID = new
PropertyDescriptor.Builder()
+ .name("service-principal-tenant-id")
+ .displayName("Service Principal Tenant ID")
+ .description("Tenant ID of the Azure Active Directory hosting the
Service Principal. The property is required when Service Principal
authentication is used.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_ID = new
PropertyDescriptor.Builder()
+ .name("service-principal-client-id")
+ .displayName("Service Principal Client ID")
+ .description("Client ID (or Application ID) of the
Client/Application having the Service Principal. The property is required when
Service Principal authentication is used. " +
+ "Also 'Service Principal Client Secret' or 'Service
Principal Client Certificate' must be specified in this case.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_SECRET =
new PropertyDescriptor.Builder()
+ .name("service-principal-client-Secret")
+ .displayName("Service Principal Client Secret")
+ .description("Password of the Client/Application.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+
.expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor
SERVICE_PRINCIPAL_CLIENT_CERTIFICATE = new PropertyDescriptor.Builder()
+ .name("service-principal-client-certificate")
+ .displayName("Service Principal Client Certificate")
+ .description("SSL Context Service referencing the keystore with
the client certificate of the Client/Application. Only PKCS12 (.pfx) keystore
type is supported. " +
Review comment:
In light of the description and the Azure SDK allowing only PKCS12
certificates, it would be helpful to add a check in the `customValidate` method
to ensure that the file provided is actually a PKCS12. Leveraging the Azure
`ClientCertificateCredentialBuilder` to load the certificate with the password
provided would be a good way to ensure that the configured properties meet the
requirements described.
##########
File path:
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -97,16 +142,38 @@
boolean sasTokenSet =
StringUtils.isNotBlank(validationContext.getProperty(AzureStorageUtils.PROP_SAS_TOKEN).getValue());
boolean useManagedIdentitySet =
validationContext.getProperty(USE_MANAGED_IDENTITY).asBoolean();
- if (!onlyOneSet(accountKeySet, sasTokenSet, useManagedIdentitySet)) {
- StringJoiner options = new StringJoiner(", ")
- .add(AzureStorageUtils.ACCOUNT_KEY.getDisplayName())
- .add(AzureStorageUtils.PROP_SAS_TOKEN.getDisplayName())
- .add(USE_MANAGED_IDENTITY.getDisplayName());
+ boolean servicePrincipalTenantIdSet =
StringUtils.isNotBlank(validationContext.getProperty(SERVICE_PRINCIPAL_TENANT_ID).getValue());
+ boolean servicePrincipalClientIdSet =
StringUtils.isNotBlank(validationContext.getProperty(SERVICE_PRINCIPAL_CLIENT_ID).getValue());
+ boolean servicePrincipalClientSecretSet =
StringUtils.isNotBlank(validationContext.getProperty(SERVICE_PRINCIPAL_CLIENT_SECRET).getValue());
+ boolean servicePrincipalClientCertificateSet =
validationContext.getProperty(SERVICE_PRINCIPAL_CLIENT_CERTIFICATE).isSet();
+
+ boolean servicePrincipalSet = servicePrincipalTenantIdSet ||
servicePrincipalClientIdSet || servicePrincipalClientSecretSet ||
servicePrincipalClientCertificateSet;
Review comment:
Instead of using the presence of any of these properties to imply
Service Principal Authentication, what about introducing one more property
named something like `Authentication Type`? The value of that property could
take one of an enumerated list of values. With that property in place, the
remaining Service Principal properties could use the `dependsOn` feature of
Property Descriptors. This would provide a cleaner user experience and should
also make the determination of authentication type easier to follow.
----------------------------------------------------------------
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:
us...@infra.apache.org
