Skip to content

Conversation

@pvillard31
Copy link
Contributor

@pvillard31 pvillard31 commented Oct 30, 2025

Summary

NIFI-15161 - Add support for Azure Federated Identity Credentials in Storage components + EventHub components

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using ./mvnw clean install -P contrib-check
    • JDK 21
    • JDK 25

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@pvillard31 pvillard31 changed the title NIFI-15161 - Add support for Azure Federated Identity Credentials in Storage components NIFI-15161 - Add support for Azure Federated Identity Credentials Nov 27, 2025
@pvillard31
Copy link
Contributor Author

Added support for EventHub components as well.

Screenshot 2025-11-27 at 17 21 01 Screenshot 2025-11-27 at 17 19 05 Screenshot 2025-11-27 at 17 18 54 Screenshot 2025-11-27 at 17 18 43

Comment on lines 552 to 562
} else if (blobStorageAuthenticationStrategy == BlobStorageAuthenticationStrategy.IDENTITY_FEDERATION) {
if (StringUtils.isNotBlank(storageAccountKey)) {
results.add(new ValidationResult.Builder()
.subject(STORAGE_ACCOUNT_KEY.getDisplayName())
.explanation("%s must not be set when %s is %s."
.formatted(STORAGE_ACCOUNT_KEY.getDisplayName(),
BLOB_STORAGE_AUTHENTICATION_STRATEGY.getDisplayName(),
BlobStorageAuthenticationStrategy.IDENTITY_FEDERATION.getDisplayName()))
.valid(false)
.build());
}
Copy link
Contributor

@turcsanyip turcsanyip Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pvillard31 Thanks for addig identity federation support in Azure processors. My plan is to review it in more detail.

Just an initial question: Are these extensive validation steps really needed? I mean the authentication related properties are controlled by the Authentication Strategy property: only the relevant properties are shown and it does not matter what values the other properties contain. If we keep the validation steps as well, then the user will get warnings related to properties that are not visible and have no effect when the processor runs.

For example:

  1. The user chooses Authentication Strategy = Storage Account Key and fills in Storage Account Key property
  2. Then they change their mind and select Authentication Strategy = Identity Federation and provide the configuration
  3. After Apply, they get a validation error saying "Storage Account Key must not be set when Authentication Strategy is Identity Federation."
  4. So they have to go back to the other option, clear the fields, and choose the desired option again

I believe it is not the ideal user experience. However, there may be other advantages to keeping the customValidate() rules that I'm not aware of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you're absolutely right that it's better to provide a good UX experience and rely on dependsOn/required. I pushed a commit to simplify all of this.

@turcsanyip
Copy link
Contributor

@pvillard31 On a more detailed review, I would suggest the following changes:

  1. Using ClientAssertionCredential: StandardAzureIdentityFederationTokenProvider performs direct HTTP requests to exchange the client assertion token for an access token. The Azure client lib provides a higher level API via ClientAssertionCredential (similar to IdentityPoolCredentials in GCP) which is more convenient and robust to achieve the same functionality.
  2. AzureIdentityFederationTokenProvider should not extend OAuth2AccessTokenProvider: Being an OAuth2AccessTokenProvider implementation, the controller service could be referenced in any processor that has OAuth dependency (e.g. QuerySalesforceObject) where the Azure-specific service is not appropriate. This relates to point (1) as well because if the service is refactored to use the credential object instead of the raw access token, then its interface will be changed and does not follow OAuth2AccessTokenProvider anymore.
  3. Due to the changes in AzureIdentityFederationTokenProvider functionality and weight, a simple util class might be enough for building the credential object from component properties. No strong opinions here though.
  4. Separating the Identity Federation and the Access Token options both on the UI and in the code: The components in the current implementation refer the new authentication strategy with different names: Identity Federation, OAuth2 Access Token or Access Token. Using a consistent name like Identity Federation or Workload Identity Federation would provide a better UX. I would not combine it with Access Token at code level either, mostly in the light of point (1) which changes the raw access token to a credential object.

@pvillard31
Copy link
Contributor Author

Thanks for the review and the suggestions @turcsanyip, this is definitely making things more consistent and cleaner, appreciate the time you spent on this! I pushed commits to address your points one by one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants