You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/05/14 10:58:25 UTC
[GitHub] [nifi] turcsanyip commented on a change in pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
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:
users@infra.apache.org