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/01 19:00:38 UTC
[GitHub] [nifi] sjyang18 opened a new pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
sjyang18 opened a new pull request #4249:
URL: https://github.com/apache/nifi/pull/4249
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
Azure managed identity support to Azure Datalake processors; NIFI-7409
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [X] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [X] Has your PR been rebased against the latest commit within the target branch (typically `master`)?
- [X] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [X] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [X] Have you written or updated unit tests to verify your changes?
- [X] Have you verified that the full build is successful on both JDK 8 and JDK 11?
- [X] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [X] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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
[GitHub] [nifi] sjyang18 commented on a change in pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
Posted by GitBox <gi...@apache.org>.
sjyang18 commented on a change in pull request #4249:
URL: https://github.com/apache/nifi/pull/4249#discussion_r427045233
##########
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:
I have changed the order. I will follow the general rule next time.
----------------------------------------------------------------
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
[GitHub] [nifi] sjyang18 commented on a change in pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
Posted by GitBox <gi...@apache.org>.
sjyang18 commented on a change in pull request #4249:
URL: https://github.com/apache/nifi/pull/4249#discussion_r424810149
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureDataLakeStorageProcessor.java
##########
@@ -31,6 +31,8 @@
import com.azure.storage.common.StorageSharedKeyCredential;
import com.azure.storage.file.datalake.DataLakeServiceClient;
import com.azure.storage.file.datalake.DataLakeServiceClientBuilder;
+import com.azure.identity.ManagedIdentityCredential;
+import com.azure.identity.ManagedIdentityCredentialBuilder;
Review comment:
done
----------------------------------------------------------------
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
[GitHub] [nifi] sjyang18 commented on a change in pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
Posted by GitBox <gi...@apache.org>.
sjyang18 commented on a change in pull request #4249:
URL: https://github.com/apache/nifi/pull/4249#discussion_r424789021
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/pom.xml
##########
@@ -74,13 +96,24 @@
<groupId>com.azure</groupId>
<artifactId>azure-storage-file-datalake</artifactId>
<version>12.0.1</version>
+ <exclusions>
+ <exclusion>
+ <groupId>com.azure</groupId>
+ <artifactId>azure-core</artifactId>
+ </exclusion>
+ </exclusions>
</dependency>
<!-- overriding jackson-core in azure-storage -->
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-core</artifactId>
<version>2.10.3</version>
</dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.dataformat</groupId>
+ <artifactId>jackson-dataformat-xml</artifactId>
+ <version>2.10.3</version>
+ </dependency>
Review comment:
fixed.
----------------------------------------------------------------
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
[GitHub] [nifi] sjyang18 commented on a change in pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
Posted by GitBox <gi...@apache.org>.
sjyang18 commented on a change in pull request #4249:
URL: https://github.com/apache/nifi/pull/4249#discussion_r424788693
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureDataLakeStorageProcessor.java
##########
@@ -134,17 +157,22 @@
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")
- .build());
+
+ if(!useManagedIdentity) {
+ 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")
+ .build());
+ }
Review comment:
fixed
----------------------------------------------------------------
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
[GitHub] [nifi] turcsanyip commented on a change in pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [nifi] sjyang18 commented on a change in pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
Posted by GitBox <gi...@apache.org>.
sjyang18 commented on a change in pull request #4249:
URL: https://github.com/apache/nifi/pull/4249#discussion_r427044249
##########
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:
changed the logic as you suggested.
----------------------------------------------------------------
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
[GitHub] [nifi] asfgit closed pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4249:
URL: https://github.com/apache/nifi/pull/4249
----------------------------------------------------------------
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
[GitHub] [nifi] sjyang18 commented on a change in pull request #4249: NIFI-7409: Azure managed identity support to Azure Datalake processors
Posted by GitBox <gi...@apache.org>.
sjyang18 commented on a change in pull request #4249:
URL: https://github.com/apache/nifi/pull/4249#discussion_r424788950
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureDataLakeStorageProcessor.java
##########
@@ -134,17 +157,22 @@
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")
- .build());
+
+ if(!useManagedIdentity) {
Review comment:
fixed.
----------------------------------------------------------------
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