-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15161 - Add support for Azure Federated Identity Credentials #10482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ea35791 to
9547faf
Compare
9547faf to
1bfeb33
Compare
| } 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()); | ||
| } |
There was a problem hiding this comment.
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:
- The user chooses
Authentication Strategy = Storage Account Keyand fills inStorage Account Keyproperty - Then they change their mind and select
Authentication Strategy = Identity Federationand provide the configuration - After Apply, they get a validation error saying "Storage Account Key must not be set when Authentication Strategy is Identity Federation."
- 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.
There was a problem hiding this comment.
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.




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
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation