exceptionfactory commented on code in PR #10498:
URL: https://github.com/apache/nifi/pull/10498#discussion_r2506937804
##########
nifi-extension-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java:
##########
@@ -463,9 +476,42 @@ protected Collection<ValidationResult>
customValidate(ValidationContext validati
.valid(false)
.build());
}
+ } else if (blobStorageAuthenticationStrategy ==
BlobStorageAuthenticationStrategy.OAUTH2_CLIENT_CREDENTIALS) {
+ if (!blobOauthProviderSet) {
+ results.add(new ValidationResult.Builder()
+
.subject(BLOB_STORAGE_OAUTH2_ACCESS_TOKEN_PROVIDER.getDisplayName())
+ .explanation(String.format("%s must be set when %s
is %s.",
+
BLOB_STORAGE_OAUTH2_ACCESS_TOKEN_PROVIDER.getDisplayName(),
+
BLOB_STORAGE_AUTHENTICATION_STRATEGY.getDisplayName(),
+
BlobStorageAuthenticationStrategy.OAUTH2_CLIENT_CREDENTIALS.getDisplayName()))
+ .valid(false)
+ .build());
+ }
+
+ if (StringUtils.isNotBlank(storageAccountKey)) {
+ results.add(new ValidationResult.Builder()
+ .subject(STORAGE_ACCOUNT_KEY.getDisplayName())
+ .explanation(String.format("%s must not be set
when %s is %s.",
Review Comment:
```suggestion
.explanation("%s must not be set when %s is
%s".formatted(
```
##########
nifi-extension-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java:
##########
@@ -543,10 +589,35 @@ protected EventProcessorClient createClient(final
ProcessContext context) {
if (checkpointStrategy == CheckpointStrategy.AZURE_BLOB_STORAGE) {
final String containerName =
defaultIfBlank(context.getProperty(STORAGE_CONTAINER_NAME).evaluateAttributeExpressions().getValue(),
eventHubName);
- final String storageConnectionString =
createStorageConnectionString(context);
- final BlobContainerClientBuilder blobContainerClientBuilder = new
BlobContainerClientBuilder()
- .connectionString(storageConnectionString)
- .containerName(containerName);
+ final String storageAccountName =
context.getProperty(STORAGE_ACCOUNT_NAME).evaluateAttributeExpressions().getValue();
+ final String domainName = getStorageDomainName(serviceBusEndpoint);
+ final BlobStorageAuthenticationStrategy
blobStorageAuthenticationStrategy =
+
context.getProperty(BLOB_STORAGE_AUTHENTICATION_STRATEGY).asAllowableValue(BlobStorageAuthenticationStrategy.class);
+
+ if (blobStorageAuthenticationStrategy == null) {
+ throw new IllegalArgumentException("Blob Storage
Authentication Strategy must be specified");
+ }
+
+ final BlobContainerClientBuilder blobContainerClientBuilder = new
BlobContainerClientBuilder();
+
+ final Runnable storageCredentialConfigurator = switch
(blobStorageAuthenticationStrategy) {
Review Comment:
I'm not following why this is a `Runnable` and then invoked immediately in
this same thread, is there a particular reason for this approach?
##########
nifi-extension-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java:
##########
@@ -463,9 +476,42 @@ protected Collection<ValidationResult>
customValidate(ValidationContext validati
.valid(false)
.build());
}
+ } else if (blobStorageAuthenticationStrategy ==
BlobStorageAuthenticationStrategy.OAUTH2_CLIENT_CREDENTIALS) {
+ if (!blobOauthProviderSet) {
+ results.add(new ValidationResult.Builder()
+
.subject(BLOB_STORAGE_OAUTH2_ACCESS_TOKEN_PROVIDER.getDisplayName())
+ .explanation(String.format("%s must be set when %s
is %s.",
+
BLOB_STORAGE_OAUTH2_ACCESS_TOKEN_PROVIDER.getDisplayName(),
+
BLOB_STORAGE_AUTHENTICATION_STRATEGY.getDisplayName(),
+
BlobStorageAuthenticationStrategy.OAUTH2_CLIENT_CREDENTIALS.getDisplayName()))
+ .valid(false)
+ .build());
+ }
+
+ if (StringUtils.isNotBlank(storageAccountKey)) {
+ results.add(new ValidationResult.Builder()
+ .subject(STORAGE_ACCOUNT_KEY.getDisplayName())
+ .explanation(String.format("%s must not be set
when %s is %s.",
+ STORAGE_ACCOUNT_KEY.getDisplayName(),
+
BLOB_STORAGE_AUTHENTICATION_STRATEGY.getDisplayName(),
+
BlobStorageAuthenticationStrategy.OAUTH2_CLIENT_CREDENTIALS.getDisplayName()))
+ .valid(false)
+ .build());
+ }
+
+ if (StringUtils.isNotBlank(storageSasToken)) {
+ results.add(new ValidationResult.Builder()
+ .subject(STORAGE_SAS_TOKEN.getDisplayName())
+ .explanation(String.format("%s must not be set
when %s is %s.",
Review Comment:
```suggestion
.explanation("%s must not be set when %s is
%s".formatted(
```
##########
nifi-extension-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/shared/azure/eventhubs/AzureEventHubAuthenticationStrategy.java:
##########
@@ -23,7 +23,8 @@
*/
public enum AzureEventHubAuthenticationStrategy implements DescribedValue {
MANAGED_IDENTITY("Managed Identity", "Authenticate using the Managed
Identity of the hosting Azure resource."),
- SHARED_ACCESS_SIGNATURE("Shared Access Signature", "Authenticate using the
Shared Access Policy name and key.");
+ SHARED_ACCESS_SIGNATURE("Shared Access Signature", "Authenticate using the
Shared Access Policy name and key."),
+ OAUTH2_CLIENT_CREDENTIALS("OAuth2", "Authenticate using an OAuth2 Access
Token Provider backed by an Entra registered application.");
Review Comment:
Is this limited to the `Client Credentials` flow? Either the display name
should read `OAuth2 Client Credentials`, or the enum should be changed, since
the Token Provider can be configured with other types.
##########
nifi-extension-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java:
##########
@@ -463,9 +476,42 @@ protected Collection<ValidationResult>
customValidate(ValidationContext validati
.valid(false)
.build());
}
+ } else if (blobStorageAuthenticationStrategy ==
BlobStorageAuthenticationStrategy.OAUTH2_CLIENT_CREDENTIALS) {
+ if (!blobOauthProviderSet) {
+ results.add(new ValidationResult.Builder()
+
.subject(BLOB_STORAGE_OAUTH2_ACCESS_TOKEN_PROVIDER.getDisplayName())
+ .explanation(String.format("%s must be set when %s
is %s.",
Review Comment:
```suggestion
.explanation("%s must be set when %s is
%s".formatted(
```
##########
nifi-extension-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java:
##########
@@ -256,6 +259,13 @@ public class ConsumeAzureEventHub extends
AbstractSessionFactoryProcessor implem
.required(true)
.dependsOn(CHECKPOINT_STRATEGY,
CheckpointStrategy.AZURE_BLOB_STORAGE)
.build();
+ static final PropertyDescriptor BLOB_STORAGE_OAUTH2_ACCESS_TOKEN_PROVIDER
= new PropertyDescriptor.Builder()
+ .name("Blob Storage OAuth2 Access Token Provider")
Review Comment:
To make this a bit shorter, what do you think about removing `Blob` and
`OAuth2`?
```suggestion
.name("Storage Access Token Provider")
```
##########
nifi-extension-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java:
##########
@@ -543,10 +589,35 @@ protected EventProcessorClient createClient(final
ProcessContext context) {
if (checkpointStrategy == CheckpointStrategy.AZURE_BLOB_STORAGE) {
final String containerName =
defaultIfBlank(context.getProperty(STORAGE_CONTAINER_NAME).evaluateAttributeExpressions().getValue(),
eventHubName);
- final String storageConnectionString =
createStorageConnectionString(context);
- final BlobContainerClientBuilder blobContainerClientBuilder = new
BlobContainerClientBuilder()
- .connectionString(storageConnectionString)
- .containerName(containerName);
+ final String storageAccountName =
context.getProperty(STORAGE_ACCOUNT_NAME).evaluateAttributeExpressions().getValue();
+ final String domainName = getStorageDomainName(serviceBusEndpoint);
+ final BlobStorageAuthenticationStrategy
blobStorageAuthenticationStrategy =
+
context.getProperty(BLOB_STORAGE_AUTHENTICATION_STRATEGY).asAllowableValue(BlobStorageAuthenticationStrategy.class);
+
+ if (blobStorageAuthenticationStrategy == null) {
+ throw new IllegalArgumentException("Blob Storage
Authentication Strategy must be specified");
+ }
Review Comment:
Should this property be required to avoid the need for a `null` check?
##########
nifi-extension-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/shared/azure/eventhubs/AzureEventHubComponent.java:
##########
@@ -43,6 +44,14 @@ public interface AzureEventHubComponent {
.required(true)
.expressionLanguageSupported(ExpressionLanguageScope.NONE)
.build();
+ PropertyDescriptor OAUTH2_ACCESS_TOKEN_PROVIDER = new
PropertyDescriptor.Builder()
+ .name("Event Hubs OAuth2 Access Token Provider")
Review Comment:
What do you think about removing `OAuth2` to make this shorter?
```suggestion
.name("Event Hubs Access Token Provider")
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
