jfrazee commented on a change in pull request #4265: URL: https://github.com/apache/nifi/pull/4265#discussion_r430022873
########## File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java ########## @@ -85,6 +85,22 @@ .sensitive(true) .build(); + public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder() + .name("storage-endpoint-suffix") + .displayName("Storage Endpoint Suffix") + .description( + "Storage accounts in public Azure always use a common FQDN suffix. " + + "Override this endpoint suffix with a different suffix in certain circumsances (like Azure Stack or non-public Azure regions). " + + "The preferred way is to configure them through a controller service specified in the Storage Credentials property. " + + "The controller service can provide a common/shared configuration for multiple/all Azure processors. Furthermore, the credentials " + + "can also be looked up dynamically with the 'Lookup' version of the service.") + .addValidator(StandardValidators.NON_EMPTY_VALIDATOR) Review comment: I think the precedent for these is to use a dropdown (`allowableValues()`) and there is a list of these in [`client-runtime`](https://github.com/Azure/autorest-clientruntime-for-java/blob/23e3142db1f7fbcd8a871ed791c1a806285ee81c/azure-client-runtime/src/main/java/com/microsoft/azure/AzureEnvironment.java#L144), which we already pull in, but it doesn't include Azure Stack so we'd need to add that. Are there any (un)expected drawbacks to doing this? ########## File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java ########## @@ -85,6 +86,20 @@ .sensitive(true) .build(); + public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder() + .name("storage-endpoint-suffix") + .displayName("Common Storage Account Endpoint Suffix") + .description( + "Storage accounts in public Azure always use a common FQDN suffix. " + + "Override this endpoint suffix with a different suffix in certain circumsances (like Azure Stack or non-public Azure regions). " + + "The preferred way is to configure them through a controller service specified in the Storage Credentials property. " + + "The controller service can provide a common/shared configuration for multiple/all Azure processors. Furthermore, the credentials " + + "can also be looked up dynamically with the 'Lookup' version of the service.") + .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES) + .required(false) Review comment: If we don't use `allowableValues()` this will need a validator. ########## File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/processors/azure/storage/queue/GetAzureQueueStorageIT.java ########## @@ -39,6 +39,7 @@ public void setUp() throws StorageException { cloudQueue.addMessage(new CloudQueueMessage("Dummy Message 1"), 604800, 0, null, null); cloudQueue.addMessage(new CloudQueueMessage("Dummy Message 2"), 604800, 0, null, null); cloudQueue.addMessage(new CloudQueueMessage("Dummy Message 3"), 604800, 0, null, null); + Review comment: Can you remove this since nothing else was changed in this file? ########## File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-services-api/src/main/java/org/apache/nifi/services/azure/storage/AzureStorageCredentialsDetails.java ########## @@ -22,17 +22,24 @@ private final String storageAccountName; + private final String storageSuffix; + private final StorageCredentials storageCredentials; - public AzureStorageCredentialsDetails(String storageAccountName, StorageCredentials storageCredentials) { + public AzureStorageCredentialsDetails(String storageAccountName, String storageSuffix, StorageCredentials storageCredentials) { Review comment: Since this is in an API NAR and it's a public constructor we have to leave the old one too. I'd suggest marking the two parameter one `@Deprecated` and having it call the new one with `.blob.core.windows.net` so it has the same behavior. ########## File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-services-api/src/main/java/org/apache/nifi/services/azure/storage/AzureStorageCredentialsDetails.java ########## @@ -22,17 +22,24 @@ private final String storageAccountName; + private final String storageSuffix; + private final StorageCredentials storageCredentials; - public AzureStorageCredentialsDetails(String storageAccountName, StorageCredentials storageCredentials) { + public AzureStorageCredentialsDetails(String storageAccountName, String storageSuffix, StorageCredentials storageCredentials) { Review comment: Since this is in an API NAR and it's a public constructor we have to leave the old one too. I'd suggest marking the two parameter one `@Deprecated` and having it call the new one with `.blob.core.windows.net` or null so it has the same behavior. ########## File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java ########## @@ -85,6 +86,20 @@ .sensitive(true) .build(); + public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder() + .name("storage-endpoint-suffix") + .displayName("Common Storage Account Endpoint Suffix") + .description( + "Storage accounts in public Azure always use a common FQDN suffix. " + + "Override this endpoint suffix with a different suffix in certain circumsances (like Azure Stack or non-public Azure regions). " + + "The preferred way is to configure them through a controller service specified in the Storage Credentials property. " + + "The controller service can provide a common/shared configuration for multiple/all Azure processors. Furthermore, the credentials " + + "can also be looked up dynamically with the 'Lookup' version of the service.") + .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES) Review comment: If I read everything correctly in [`AzureStorageUtils`](https://github.com/apache/nifi/blob/56fc89fcdea0e3146f0fe4a28904995fbad3646d/nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java#L159-L169), _Storage Credentials_ service takes precedence over _Storage Account Name_ and _Storage Account Key_ or _SAS Token_. Those are validated as either-ors in the processor though so the behavior is predictable. What happens though if we set _Endpoint Suffix_ on both the processor and the service? I think it's going to use what's on the service, which could be confusing. And what if you use an expression on the service but it's blank? I don't think it'll default to what's set in the processor, since if the service is set it'll always use the service. So it's not a fallback either. *If* this is correct I think there are a couple of options: 1. Validate that either _Storage Credentials_ service or _Endpoint Suffix_ are in use and not both. 2. Create some kind of fallback behavior in [`createStorageCredentialsDetails`](https://github.com/apache/nifi/blob/56fc89fcdea0e3146f0fe4a28904995fbad3646d/nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java#L159) (use the endpoint suffix from the service if it's non-blank, otherwise fallback to the endpoint suffix from the processor context). 3. Make a note in the docs and do nothing. (2) is very flexible but pretty complicated. (3) seems like it'll lead people to make mistakes. ########## File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java ########## @@ -85,6 +85,22 @@ .sensitive(true) .build(); + public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder() + .name("storage-endpoint-suffix") + .displayName("Storage Endpoint Suffix") + .description( + "Storage accounts in public Azure always use a common FQDN suffix. " + + "Override this endpoint suffix with a different suffix in certain circumsances (like Azure Stack or non-public Azure regions). " + + "The preferred way is to configure them through a controller service specified in the Storage Credentials property. " + + "The controller service can provide a common/shared configuration for multiple/all Azure processors. Furthermore, the credentials " + + "can also be looked up dynamically with the 'Lookup' version of the service.") + .addValidator(StandardValidators.NON_EMPTY_VALIDATOR) Review comment: After thinking about it more, there is a use case that can't be satisfied using `allowableValues()`. The storage account, container, etc. can be pulled from FlowFile attributes so if the suffix is freeform, you can have the same processor(s) connecting across commercial and gov clouds based on attributes. I lean toward `allowableValues()` nonetheless because I'd be a little surprised if it's a common use case and it can be satisfied by just using two processors. Interested to hear from others on this though. ########## File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/AzureStorageUtils.java ########## @@ -85,6 +86,20 @@ .sensitive(true) .build(); + public static final PropertyDescriptor ENDPOINT_SUFFIX = new PropertyDescriptor.Builder() + .name("storage-endpoint-suffix") + .displayName("Common Storage Account Endpoint Suffix") + .description( + "Storage accounts in public Azure always use a common FQDN suffix. " + + "Override this endpoint suffix with a different suffix in certain circumsances (like Azure Stack or non-public Azure regions). " + + "The preferred way is to configure them through a controller service specified in the Storage Credentials property. " + + "The controller service can provide a common/shared configuration for multiple/all Azure processors. Furthermore, the credentials " + + "can also be looked up dynamically with the 'Lookup' version of the service.") + .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES) + .required(false) Review comment: @sjyang18 Re: Azure Stack endpoint, sounds good. Thanks for digging into it. Re: validation. I think it'd be helpful to account for the just whitespace or _Set empty string_ scenarios to help prevent mistakes. ---------------------------------------------------------------- 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