turcsanyip commented on a change in pull request #4249:
URL: https://github.com/apache/nifi/pull/4249#discussion_r424967588
##########
File path:
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureDataLakeStorageProcessor.java
##########
@@ -118,9 +136,14 @@
.build();
private static final List<PropertyDescriptor> PROPERTIES =
Collections.unmodifiableList(
- Arrays.asList(AbstractAzureDataLakeStorageProcessor.ACCOUNT_NAME,
AbstractAzureDataLakeStorageProcessor.ACCOUNT_KEY,
- AbstractAzureDataLakeStorageProcessor.SAS_TOKEN,
AbstractAzureDataLakeStorageProcessor.FILESYSTEM,
- AbstractAzureDataLakeStorageProcessor.DIRECTORY,
AbstractAzureDataLakeStorageProcessor.FILE));
+ Arrays.asList(AbstractAzureDataLakeStorageProcessor.ACCOUNT_NAME,
+ AbstractAzureDataLakeStorageProcessor.ACCOUNT_KEY,
+ AbstractAzureDataLakeStorageProcessor.SAS_TOKEN,
+ AbstractAzureDataLakeStorageProcessor.ENDPOINT_SUFFIX,
Review comment:
As I mentioned in https://github.com/apache/nifi/pull/4265 (Blob/Queue
processors Endpoint Suffix), I think we should keep the credential properties
together on the UI.
Unlike the Blob/Queue processors, there is one more credential property
here, the Use Azure Managed Identity.
So I would move the suffix property after it (or leave it at the bottom
where it was earlier).
As a general rule, it is preferred to add only 1 feature in a jira ticket
and NIFI-7409 is about the Managed Identity support. So this Endpoint Suffix is
an additional change here and it would rather belong to NIFI-7434 T think.
Mixing multiple features together makes the review more complicated. Please
keep it in mind in the future.
##########
File path:
nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureDataLakeStorageProcessor.java
##########
@@ -134,17 +157,35 @@
public static Collection<ValidationResult>
validateCredentialProperties(final ValidationContext validationContext) {
final List<ValidationResult> results = new ArrayList<>();
+
+ final boolean useManagedIdentity =
validationContext.getProperty(USE_MANAGED_IDENTITY).asBoolean();
final String accountName =
validationContext.getProperty(ACCOUNT_NAME).getValue();
- final String accountKey =
validationContext.getProperty(ACCOUNT_KEY).getValue();
- final String sasToken =
validationContext.getProperty(SAS_TOKEN).getValue();
-
- if (StringUtils.isNotBlank(accountName)
- && ((StringUtils.isNotBlank(accountKey) &&
StringUtils.isNotBlank(sasToken)) || (StringUtils.isBlank(accountKey) &&
StringUtils.isBlank(sasToken)))) {
- results.add(new ValidationResult.Builder().subject("Azure Storage
Credentials").valid(false)
- .explanation("either " + ACCOUNT_NAME.getDisplayName() + "
with " + ACCOUNT_KEY.getDisplayName() +
- " or " + ACCOUNT_NAME.getDisplayName() + " with "
+ SAS_TOKEN.getDisplayName() +
- " must be specified, not both")
+ final boolean accountKeyIsSet =
validationContext.getProperty(ACCOUNT_KEY).isSet();
+ final boolean sasTokenIsSet =
validationContext.getProperty(SAS_TOKEN).isSet();
+
+ if(useManagedIdentity){
+ if(accountKeyIsSet || sasTokenIsSet) {
+ final String msg = String.format(
+ "('%s') and ('%s' or '%s') fields cannot be set at the
same time.",
+ USE_MANAGED_IDENTITY.getDisplayName(),
+ ACCOUNT_KEY.getDisplayName(),
+ SAS_TOKEN.getDisplayName()
+ );
+ results.add(new
ValidationResult.Builder().subject("Credentials
config").valid(false).explanation(msg).build());
+ }
+ } else {
+ final String accountKey =
validationContext.getProperty(ACCOUNT_KEY).getValue();
+ final String sasToken =
validationContext.getProperty(SAS_TOKEN).getValue();
+ if (StringUtils.isNotBlank(accountName) &&
((StringUtils.isNotBlank(accountKey) && StringUtils.isNotBlank(sasToken))
+ || (StringUtils.isBlank(accountKey) &&
StringUtils.isBlank(sasToken)))) {
+ final String msg = String.format("either " +
ACCOUNT_NAME.getDisplayName() + " with " + ACCOUNT_KEY.getDisplayName() +
+ " or " + ACCOUNT_NAME.getDisplayName() + " with " +
SAS_TOKEN.getDisplayName() +
+ " must be specified, not both"
+ );
+ results.add(new
ValidationResult.Builder().subject("Credentials Config").valid(false)
+ .explanation(msg)
.build());
+ }
Review comment:
When I was testing the validation, I noticed 2 things:
- when none of the Account Key, SAS Token or Managed Identity properties is
specified (which is the initial state of the processor), then the error message
says that _"either Storage Account Name with Storage Account Key or Storage
Account Name with SAS Token must be specified"_, which is not entirely correct
because Managed Identity could also be set
- the validation error message for the user could be corrected and also
simplified at same time
What we really need to check:
- Account Key must be specified, but it is already checked via
required=true, so we don't need to check it again in customValidate()
- exactly one of the Account Key, SAS Token or Managed Identity properties
must be set, I think it would be enough to check this condition in
customValidate(), and in this way we can provide a proper error message to the
user
----------------------------------------------------------------
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]
