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:
[email protected]
