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 2021/06/08 07:07:29 UTC
[GitHub] [nifi] timeabarna opened a new pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
timeabarna opened a new pull request #5136:
URL: https://github.com/apache/nifi/pull/5136
ConsumeAzureEventHub NiFi processors need to support storage SAS token authentication
https://issues.apache.org/jira/browse/NIFI-8668
#### Description of PR
Enables using SAS token for storage account
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] 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.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] 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:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] 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:
- [ ] 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] jfrazee commented on pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
jfrazee commented on pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#issuecomment-875841218
@timeabarna Let me see if I have it right. We know the existing one doesn't evaluate the expression because it could be attributes and evaluating those could produce nonsense because the attributes don't exist at the time of evaluation so we're adding a new one that does evaluate.
But it should still be possible to just have something like the following and push the responsibility on to the user:
```java
public static Validator createRegexMatchingValidator(final Pattern pattern) {
return createRegexMatchingValidator(pattern, false);
}
```
And:
```java
public static Validator createRegexMatchingValidator(final Pattern pattern, final boolean evaluateExpressions) {
return new Validator() {
@Override
public ValidationResult validate(final String subject, final String value, final ValidationContext context) {
String input = value;
if (context.isExpressionLanguageSupported(subject) && context.isExpressionLanguagePresent(value)) {
if (evaluateExpressions) {
try {
input = property.evaluateAttributeExpressions().getValue();
} catch (final Exception e) {
return ...
}
} else {
return new ValidationResult.Builder().subject(subject).input(input).explanation("Expression Language Present").valid(true).build();
}
}
final boolean matches = pattern.matcher(input).matches();
return new ValidationResult.Builder() ...
}
};
}
```
Is there a reason this doesn't work?
Also, I think the new EL variant might evaluate for `ExpressionLanguageScope.NONE` since it doesn't check for `context.isExpressionLanguageSupported(subject)`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r647887457
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -332,6 +345,26 @@ public void setWriterFactory(RecordSetWriterFactory writerFactory) {
.valid(false)
.build());
}
+
+ if (storageAccountKey == null && storageSasToken == null) {
+ results.add(new ValidationResult.Builder()
+ .subject(String.format("%s or %s",
+ STORAGE_ACCOUNT_KEY.getDisplayName(), STORAGE_SAS_TOKEN.getDisplayName()))
+ .explanation(String.format("either %s or %s should be set.",
+ STORAGE_ACCOUNT_KEY.getDisplayName(), STORAGE_SAS_TOKEN.getDisplayName()))
+ .valid(false)
+ .build());
+ }
+
+ if (storageAccountKey != null && storageSasToken != null) {
Review comment:
Should this use StringUtils.isNotBlank() to confirm that the properties are both not null and not blank?
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -228,7 +229,16 @@
.sensitive(true)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
- .required(true)
+ .required(false)
+ .build();
+ static final PropertyDescriptor STORAGE_SAS_TOKEN = new PropertyDescriptor.Builder()
+ .name("storage-sas-token")
+ .displayName("Storage SAS Token")
+ .description("The Azure Storage SAS token to store event hub consumer group state. Always starts with a ? character.")
Review comment:
If the value should always start with a `?` character, does it make sense to use a Regular Expression pattern matching validator for this property?
--
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 pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#issuecomment-895129013
@jfrazee, @exceptionfactory Thanks for the reviews.
Merging to main...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#issuecomment-884665781
@jfrazee Hello Joey, thanks again for your help regarding this review. Do you have anything else to discuss? Can this item be merged?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna edited a comment on pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna edited a comment on pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#issuecomment-875379175
@jfrazee createRegexMatchingValidator() has been set for properties having expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES). As far as I know it is not possible to differentiate between flowfile attributes and expression language during validation this is why createRegexMatchingValidator() gives valid for all EL cases. createRegexMatchingValidatorWithEL() on the other hand should resolve EL and evaluate it. Thanks for point this out I've added a comment above createRegexMatchingValidatorWithEL() to explain it should not be used with flow file attributes. Added the unit tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r648226847
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -91,7 +91,8 @@
})
public class ConsumeAzureEventHub extends AbstractSessionFactoryProcessor {
- private static final String FORMAT_STORAGE_CONNECTION_STRING = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_ACCOUNT_KEY = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_SAS_TOKEN = "BlobEndpoint=https://%s.blob.core.windows.net/;SharedAccessSignature=%s";
Review comment:
https://issues.apache.org/jira/browse/NIFI-8677 has been opened for endpoint suffix support.
--
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] exceptionfactory commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r648507938
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -332,6 +347,26 @@ public void setWriterFactory(RecordSetWriterFactory writerFactory) {
.valid(false)
.build());
}
+
+ if (!StringUtils.isNotBlank(storageAccountKey) && !StringUtils.isNotBlank(storageSasToken)) {
Review comment:
Agreed.
--
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 #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r647320815
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -91,7 +91,8 @@
})
public class ConsumeAzureEventHub extends AbstractSessionFactoryProcessor {
- private static final String FORMAT_STORAGE_CONNECTION_STRING = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_KEY = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_TOKEN = "BlobEndpoint=https://%s.blob.core.windows.net/;SharedAccessSignature=%s";
Review comment:
To make it more straightforward, I would suggest using `FOR_ACCOUNT_KEY` and `FOR_SAS_TOKEN` suffixes.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -332,6 +345,24 @@ public void setWriterFactory(RecordSetWriterFactory writerFactory) {
.valid(false)
.build());
}
+
+ if (storageAccountKey == null && storageSasToken == null) {
+ results.add(new ValidationResult.Builder()
+ .subject("Storage Account Key or Storage SAS Token")
Review comment:
`getDisplayName()` could be used as for the explanation below.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/processors/azure/eventhub/TestConsumeAzureEventHub.java
##########
@@ -132,12 +135,42 @@ public void testProcessorConfigValidityWithManagedIdentityFlag() throws Initiali
testRunner.assertValid();
}
+ @Test
+ public void testProcessorConfigValidityWithNeitherStorageKeyNorTokenSet() {
+ TestRunner testRunner = TestRunners.newTestRunner(processor);
+ testRunner.setProperty(ConsumeAzureEventHub.EVENT_HUB_NAME,eventHubName);
+ testRunner.assertNotValid();
+ testRunner.setProperty(ConsumeAzureEventHub.NAMESPACE,namespaceName);
+ testRunner.assertNotValid();
+ testRunner.setProperty(ConsumeAzureEventHub.ACCESS_POLICY_NAME, policyName);
+ testRunner.setProperty(ConsumeAzureEventHub.POLICY_PRIMARY_KEY, policyKey);
+ testRunner.assertNotValid();
+ testRunner.setProperty(ConsumeAzureEventHub.STORAGE_ACCOUNT_NAME, storageAccountName);
+ testRunner.assertNotValid();
+ }
+
+ @Test
+ public void testProcessorConfigValidityWithBothStorageKeyAndTokenSet() {
+ TestRunner testRunner = TestRunners.newTestRunner(processor);
+ testRunner.setProperty(ConsumeAzureEventHub.EVENT_HUB_NAME,eventHubName);
+ testRunner.assertNotValid();
+ testRunner.setProperty(ConsumeAzureEventHub.NAMESPACE,namespaceName);
+ testRunner.assertNotValid();
+ testRunner.setProperty(ConsumeAzureEventHub.ACCESS_POLICY_NAME, policyName);
+ testRunner.setProperty(ConsumeAzureEventHub.POLICY_PRIMARY_KEY, policyKey);
+ testRunner.assertNotValid();
+ testRunner.setProperty(ConsumeAzureEventHub.STORAGE_ACCOUNT_NAME, storageAccountName);
+ testRunner.setProperty(ConsumeAzureEventHub.STORAGE_ACCOUNT_KEY, storageAccountKey);
+ testRunner.setProperty(ConsumeAzureEventHub.STORAGE_SAS_TOKEN, storageSasToken);
+ testRunner.assertNotValid();
+ }
+
Review comment:
It would be good to have some tests for the valid cases as well. (when Storage Account Key specified only, and when Storage SAS Token only).
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/processors/azure/eventhub/TestConsumeAzureEventHub.java
##########
@@ -150,7 +183,7 @@ public void testReceivedApplicationProperties() throws Exception {
@Test
public void testReceiveOne() throws Exception {
- final Iterable<EventData> eventDataList = Arrays.asList(EventData.create("one".getBytes(StandardCharsets.UTF_8)));
+ final Iterable<EventData> eventDataList = Collections.singletonList(EventData.create("one" .getBytes(StandardCharsets.UTF_8)));
Review comment:
Typo / wrong formatting: `"one".getBytes()`
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -332,6 +345,24 @@ public void setWriterFactory(RecordSetWriterFactory writerFactory) {
.valid(false)
.build());
}
+
+ if (storageAccountKey == null && storageSasToken == null) {
+ results.add(new ValidationResult.Builder()
+ .subject("Storage Account Key or Storage SAS Token")
+ .explanation(String.format("Either %s or %s should be set.",
Review comment:
The explanation will be concatenated into the middle of the validation failure message so it should start with lower case (_"...is invalid because either..."_)
--
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] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r647991025
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -91,7 +91,8 @@
})
public class ConsumeAzureEventHub extends AbstractSessionFactoryProcessor {
- private static final String FORMAT_STORAGE_CONNECTION_STRING = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_ACCOUNT_KEY = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_SAS_TOKEN = "BlobEndpoint=https://%s.blob.core.windows.net/;SharedAccessSignature=%s";
Review comment:
@jfrazee At the moment ConsumeAzureEventHub processor does not support special regions, so this is why it is not included in this PR. During AzureCheckpointLeaseManager initialisation CloudStorageAccount is created by the parse(storageConnectionString). Parse is using tryConfigureServiceAccount() method to instantiate CloudStrageAccount using public CloudStorageAccount(StorageCredentials storageCredentials, StorageUri blobStorageUri, StorageUri queueStorageUri, StorageUri tableStorageUri, StorageUri fileStorageUri) constructor. This constructor sets endPointSuffix to null and as we are not having endpointSuffix in our connectionStrings the url always be "core windows.net"
It is a good idea though and can be added by a separate request.
--
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] exceptionfactory commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r648506809
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -332,6 +347,26 @@ public void setWriterFactory(RecordSetWriterFactory writerFactory) {
.valid(false)
.build());
}
+
+ if (!StringUtils.isNotBlank(storageAccountKey) && !StringUtils.isNotBlank(storageSasToken)) {
Review comment:
Based on the previous approach, I believe just `StringUtils.isNotBlank()` should be used:
```suggestion
if (StringUtils.isNotBlank(storageAccountKey) && StringUtils.isNotBlank(storageSasToken)) {
```
--
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] Lehel44 commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r648499618
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -332,6 +347,26 @@ public void setWriterFactory(RecordSetWriterFactory writerFactory) {
.valid(false)
.build());
}
+
+ if (!StringUtils.isNotBlank(storageAccountKey) && !StringUtils.isNotBlank(storageSasToken)) {
Review comment:
I think !StringUtils::isNotBlank can be replaced with StringUtils::isBlank.
--
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] jfrazee commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r647440433
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -91,7 +91,8 @@
})
public class ConsumeAzureEventHub extends AbstractSessionFactoryProcessor {
- private static final String FORMAT_STORAGE_CONNECTION_STRING = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_ACCOUNT_KEY = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_SAS_TOKEN = "BlobEndpoint=https://%s.blob.core.windows.net/;SharedAccessSignature=%s";
Review comment:
Putting `blob.core.windows.net` explicitly in this means that this isn't going to work for any special regions including US Government, China, and Germany. Probably need to find a way to add endpoint suffix support to this.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -228,7 +229,16 @@
.sensitive(true)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
- .required(true)
+ .required(false)
+ .build();
+ static final PropertyDescriptor STORAGE_SAS_TOKEN = new PropertyDescriptor.Builder()
+ .name("storage-sas-token")
+ .displayName("Storage SAS Token")
+ .description("The Azure Storage SAS token to store event hub consumer group state. Always starts with a ? character.")
+ .sensitive(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
Review comment:
```suggestion
.description("The Azure Storage SAS token to store Event Hub consumer group state. Always starts with a ? character.")
.sensitive(true)
.addValidator(StandardValidators.NON_BLANK_VALIDATOR)
```
--
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] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r648000100
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -228,7 +229,16 @@
.sensitive(true)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
- .required(true)
+ .required(false)
+ .build();
+ static final PropertyDescriptor STORAGE_SAS_TOKEN = new PropertyDescriptor.Builder()
+ .name("storage-sas-token")
+ .displayName("Storage SAS Token")
+ .description("The Azure Storage SAS token to store event hub consumer group state. Always starts with a ? character.")
+ .sensitive(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
Review comment:
Thanks @jfreeze code has been modified accordingly.
--
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] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r647998357
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -332,6 +345,26 @@ public void setWriterFactory(RecordSetWriterFactory writerFactory) {
.valid(false)
.build());
}
+
+ if (storageAccountKey == null && storageSasToken == null) {
+ results.add(new ValidationResult.Builder()
+ .subject(String.format("%s or %s",
+ STORAGE_ACCOUNT_KEY.getDisplayName(), STORAGE_SAS_TOKEN.getDisplayName()))
+ .explanation(String.format("either %s or %s should be set.",
+ STORAGE_ACCOUNT_KEY.getDisplayName(), STORAGE_SAS_TOKEN.getDisplayName()))
+ .valid(false)
+ .build());
+ }
+
+ if (storageAccountKey != null && storageSasToken != null) {
Review comment:
Thanks @exceptionfactory code has been modified accordingly.
--
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] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r648010388
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -228,7 +229,16 @@
.sensitive(true)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
- .required(true)
+ .required(false)
+ .build();
+ static final PropertyDescriptor STORAGE_SAS_TOKEN = new PropertyDescriptor.Builder()
+ .name("storage-sas-token")
+ .displayName("Storage SAS Token")
+ .description("The Azure Storage SAS token to store event hub consumer group state. Always starts with a ? character.")
Review comment:
Thanks @exceptionfactory code has been modified accordingly.
--
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 #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5136:
URL: https://github.com/apache/nifi/pull/5136
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#issuecomment-875379175
@jfrazee createRegexMatchingValidator() has been set for properties having expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES). As far as I know it is not possible to differentiate between flowfile attributes and expression language during validation this is why createRegexMatchingValidator() gives valid for all EL cases. createRegexMatchingValidatorWithEL() on the other hand should resolve EL and evaluate it. Thanks for point this out I've added a comment above createRegexMatchingValidatorWithEL() to explain it should not be used with flow file attributes. Working on the unit tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] jfrazee commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
jfrazee commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r664874346
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java
##########
@@ -667,6 +667,46 @@ public ValidationResult validate(final String subject, final String input, final
};
}
+ public static Validator createRegexMatchingValidatorWithEL(final Pattern pattern, final String validationMessage) {
Review comment:
@timeabarna I don't think I understand why we need both `createRegexMatchingValidator()` and `createRegexMatchingValidatorWithEL()`. If `createRegexMatchingValidator()` isn't working with parameters wouldn't it work to just make the improvement you've made in the `WithEL` variant?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r647991025
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -91,7 +91,8 @@
})
public class ConsumeAzureEventHub extends AbstractSessionFactoryProcessor {
- private static final String FORMAT_STORAGE_CONNECTION_STRING = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_ACCOUNT_KEY = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_SAS_TOKEN = "BlobEndpoint=https://%s.blob.core.windows.net/;SharedAccessSignature=%s";
Review comment:
@jfrazee At the moment ConsumeAzureEventHub processor does not support special regions, so this is why it is not included in this PR.
It is a good idea though and can be added by a separate request.
--
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] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r647991025
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -91,7 +91,8 @@
})
public class ConsumeAzureEventHub extends AbstractSessionFactoryProcessor {
- private static final String FORMAT_STORAGE_CONNECTION_STRING = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_ACCOUNT_KEY = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_SAS_TOKEN = "BlobEndpoint=https://%s.blob.core.windows.net/;SharedAccessSignature=%s";
Review comment:
@jfrazee At the moment ConsumeAzureEventHub processor does not support special regions, so this is why it is not included in this PR. During AzureCheckpointLeaseManager initialisation CloudStorageAccount is created by the parse(storageConnectionString). Parse is using tryConfigureServiceAccount() method to instantiate CloudStrageAccount using public CloudStorageAccount(StorageCredentials storageCredentials, StorageUri blobStorageUri, StorageUri queueStorageUri, StorageUri tableStorageUri, StorageUri fileStorageUri) constructor. This constructor sets endPointSuffix to null so the url always be "core windows.net"
It is a good idea though and can be added by a separate request.
--
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] timeabarna commented on pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#issuecomment-856686623
@turcsanyip Thanks for the review, code modified according to your recomendations.
--
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] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r648874627
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -332,6 +347,26 @@ public void setWriterFactory(RecordSetWriterFactory writerFactory) {
.valid(false)
.build());
}
+
+ if (!StringUtils.isNotBlank(storageAccountKey) && !StringUtils.isNotBlank(storageSasToken)) {
Review comment:
Thanks @Lehel44 code updated accordingly.
--
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] timeabarna commented on pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#issuecomment-876326442
@jfrazee I've modified the validator accordingly
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r663655732
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java
##########
@@ -667,6 +667,46 @@ public ValidationResult validate(final String subject, final String input, final
};
}
+ public static Validator createRegexMatchingValidatorWithEL(final Pattern pattern, final String validationMessage) {
+ return new Validator() {
+ @Override
+ public ValidationResult validate(final String subject, final String input, final ValidationContext context) {
+ final PropertyValue prefixProperty = context.newPropertyValue(input);
+ final String prefix;
Review comment:
@turcsanyip thanks for your comment, renamed it to propertyValue
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r647331646
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -91,7 +91,8 @@
})
public class ConsumeAzureEventHub extends AbstractSessionFactoryProcessor {
- private static final String FORMAT_STORAGE_CONNECTION_STRING = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_KEY = "DefaultEndpointsProtocol=https;AccountName=%s;AccountKey=%s";
+ private static final String FORMAT_STORAGE_CONNECTION_STRING_FOR_TOKEN = "BlobEndpoint=https://%s.blob.core.windows.net/;SharedAccessSignature=%s";
Review comment:
Modified accordingly
--
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] timeabarna commented on a change in pull request #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
timeabarna commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r654146623
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java
##########
@@ -667,6 +667,46 @@ public ValidationResult validate(final String subject, final String input, final
};
}
+ public static Validator createRegexMatchingValidatorWithEL(final Pattern pattern, final String validationMessage) {
Review comment:
@exceptionfactory Thanks for your help
--
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 #5136: NIFI-8668 ConsumeAzureEventHub NiFi processors need to support storag…
Posted by GitBox <gi...@apache.org>.
turcsanyip commented on a change in pull request #5136:
URL: https://github.com/apache/nifi/pull/5136#discussion_r661709627
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/processor/util/StandardValidators.java
##########
@@ -667,6 +667,46 @@ public ValidationResult validate(final String subject, final String input, final
};
}
+ public static Validator createRegexMatchingValidatorWithEL(final Pattern pattern, final String validationMessage) {
+ return new Validator() {
+ @Override
+ public ValidationResult validate(final String subject, final String input, final ValidationContext context) {
+ final PropertyValue prefixProperty = context.newPropertyValue(input);
+ final String prefix;
Review comment:
@timeabarna I retested the processor with a simple flow and it works properly.
A minor question: What does "prefix" mean in this context?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org