You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "lordgamez (via GitHub)" <gi...@apache.org> on 2025/08/28 15:40:59 UTC
[PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
lordgamez opened a new pull request, #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016
- Add credential configuration strategy property with from properties, default credentials, managed identity, and workload identity options
- Add managed identity client id property
- Do not allow SAS Token and Account Key to be used together
https://issues.apache.org/jira/browse/MINIFICPP-2612
----------------------------
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
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 MINIFICPP-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?
### For code changes:
- [ ] 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?
- [ ] If applicable, have you updated the NOTICE file?
### 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 results 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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2378951626
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -29,6 +29,10 @@ void AzureStorageCredentialsService::initialize() {
}
void AzureStorageCredentialsService::onEnable() {
+ auto credential_configuration_strategy_str = getProperty(CredentialConfigurationStrategy.name).value_or(std::string{magic_enum::enum_name(CredentialConfigurationStrategyOption::FromProperties)});
+ if (auto credential_configuration_strategy = magic_enum::enum_cast<CredentialConfigurationStrategyOption>(credential_configuration_strategy_str)) {
+ credentials_.setCredentialConfigurationStrategy(*credential_configuration_strategy);
+ }
Review Comment:
I would put an `else` branch here, and either throw or at least log an error: the user has set an invalid value.
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2368801960
##########
cmake/AzureSdkCpp.cmake:
##########
@@ -55,3 +55,5 @@ FetchContent_Declare(asdkext
)
FetchContent_MakeAvailable(asdkext)
+
+add_dependencies(azure-storage-common LibXml2::LibXml2)
Review Comment:
I had some build failures of the azure library because it could not find the libxml2 library, as it was not built yet at that moment.
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2352024651
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -39,13 +43,13 @@ void AzureStorageCredentialsService::onEnable() {
credentials_.setSasToken(*sas_token);
}
if (auto common_storage_account_endpoint_suffix = getProperty(CommonStorageAccountEndpointSuffix.name)) {
- credentials_.setEndpontSuffix(*common_storage_account_endpoint_suffix);
+ credentials_.setEndpointSuffix(*common_storage_account_endpoint_suffix);
}
if (auto connection_String = getProperty(ConnectionString.name)) {
credentials_.setConnectionString(*connection_String);
}
- if (auto use_managed_identity_credentials = getProperty(UseManagedIdentityCredentials.name) | utils::andThen(parsing::parseBool)) {
- credentials_.setUseManagedIdentityCredentials(*use_managed_identity_credentials);
+ if (auto managed_identity_client_id = getProperty(ManagedIdentityClientId.name)) {
+ credentials_.setManagedIdentityClientId(*managed_identity_client_id);
Review Comment:
Sure, added it as a note in the description
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2308193434
##########
extensions/azure/utils/AzureEnums.h:
##########
@@ -19,11 +19,39 @@
*/
#pragma once
+#include "magic_enum.hpp"
+
namespace org::apache::nifi::minifi::azure {
enum class EntityTracking {
none,
timestamps
};
+enum class CredentialConfigurationStrategyOption {
+ fromProperties,
+ defaultCredential,
+ managedIdentity,
+ workloadIdentity
+};
Review Comment:
I'd prefer to keep the enum members as UpperCamelCase or UPPER_SNAKE_CASE
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.h:
##########
@@ -42,33 +43,44 @@ class AzureStorageCredentialsService : public core::controller::ControllerServic
.build();
EXTENSIONAPI static constexpr auto StorageAccountKey = core::PropertyDefinitionBuilder<>::createProperty("Storage Account Key")
.withDescription("The storage account key. This is an admin-like password providing access to every container in this account. "
- "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies.")
+ "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies "
+ "if Credential Configuration Strategy is set to From Properties. If set, SAS Token must be empty.")
Review Comment:
I'd prefer to keep continuation indentations = two normal levels of indentation, and not aligned with whatever from the previous lines.
Why?
- saves horizontal space
- prevents refactor/rename from messing up the formatting
- still lets us differentiate between normal indentation and continuation of the previous thing
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -29,6 +29,10 @@ void AzureStorageCredentialsService::initialize() {
}
void AzureStorageCredentialsService::onEnable() {
+ if (auto credential_configuration_strategy = magic_enum::enum_cast<CredentialConfigurationStrategyOption>(
+ getProperty(CredentialConfigurationStrategy.name).value_or(std::string{magic_enum::enum_name(CredentialConfigurationStrategyOption::fromProperties)}))) {
Review Comment:
Instead of breaking the line in two, I'd extract parts of it to keep it simpler
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2329841490
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.h:
##########
@@ -42,33 +43,44 @@ class AzureStorageCredentialsService : public core::controller::ControllerServic
.build();
EXTENSIONAPI static constexpr auto StorageAccountKey = core::PropertyDefinitionBuilder<>::createProperty("Storage Account Key")
.withDescription("The storage account key. This is an admin-like password providing access to every container in this account. "
- "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies.")
+ "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies "
+ "if Credential Configuration Strategy is set to From Properties. If set, SAS Token must be empty.")
.isSensitive(true)
.build();
EXTENSIONAPI static constexpr auto SASToken = core::PropertyDefinitionBuilder<>::createProperty("SAS Token")
- .withDescription("Shared Access Signature token. Specify either SAS Token (recommended) or Storage Account Key together with Storage Account Name if Managed Identity is not used.")
+ .withDescription("Shared Access Signature token. Specify either SAS Token (recommended) or Storage Account Key together with Storage Account Name "
+ "if Credential Configuration Strategy is set to From Properties. If set, Storage Account Key must be empty.")
.isSensitive(true)
.build();
EXTENSIONAPI static constexpr auto CommonStorageAccountEndpointSuffix = core::PropertyDefinitionBuilder<>::createProperty("Common Storage Account Endpoint Suffix")
.withDescription("Storage accounts in public Azure always use a common FQDN suffix. Override this endpoint suffix with a "
"different suffix in certain circumstances (like Azure Stack or non-public Azure regions).")
.build();
EXTENSIONAPI static constexpr auto ConnectionString = core::PropertyDefinitionBuilder<>::createProperty("Connection String")
- .withDescription("Connection string used to connect to Azure Storage service. This overrides all other set credential properties if Managed Identity is not used.")
+ .withDescription("Connection string used to connect to Azure Storage service. This overrides all other set credential properties "
+ "if Credential Configuration Strategy is set to From Properties.")
.build();
- EXTENSIONAPI static constexpr auto UseManagedIdentityCredentials = core::PropertyDefinitionBuilder<>::createProperty("Use Managed Identity Credentials")
- .withDescription("If true Managed Identity credentials will be used together with the Storage Account Name for authentication.")
+ EXTENSIONAPI static constexpr auto CredentialConfigurationStrategy =
+ core::PropertyDefinitionBuilder<magic_enum::enum_count<CredentialConfigurationStrategyOption>()>::createProperty("Credential Configuration Strategy")
+ .withDescription("The strategy to use for credential configuration. If set to From Properties, the credentials are parsed from the SAS Token, Storage Account Key, "
+ "and Connection String properties. In other cases, the selected Azure identity source is used.")
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/fa8b04f30e3b383886bd9004babe21fa327ac070
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -39,13 +43,13 @@ void AzureStorageCredentialsService::onEnable() {
credentials_.setSasToken(*sas_token);
}
if (auto common_storage_account_endpoint_suffix = getProperty(CommonStorageAccountEndpointSuffix.name)) {
- credentials_.setEndpontSuffix(*common_storage_account_endpoint_suffix);
+ credentials_.setEndpointSuffix(*common_storage_account_endpoint_suffix);
}
if (auto connection_String = getProperty(ConnectionString.name)) {
credentials_.setConnectionString(*connection_String);
}
- if (auto use_managed_identity_credentials = getProperty(UseManagedIdentityCredentials.name) | utils::andThen(parsing::parseBool)) {
- credentials_.setUseManagedIdentityCredentials(*use_managed_identity_credentials);
+ if (auto managed_identity_client_id = getProperty(ManagedIdentityClientId.name)) {
+ credentials_.setManagedIdentityClientId(*managed_identity_client_id);
Review Comment:
Replacing the old `Use Managed Identity Credentials` property with the new `Credential Configuration Strategy` is a breaking change, adding the new `Managed Identity Client Id` property is new a feature for using user assigned managed identity, that is not a breaking change just an additional option.
##########
extensions/azure/storage/AzureBlobStorageClient.cpp:
##########
@@ -56,13 +56,13 @@ AzureBlobStorageClient::AzureBlobStorageClient() {
}
Azure::Storage::Blobs::BlobContainerClient AzureBlobStorageClient::createClient(const AzureStorageCredentials &credentials, const std::string &container_name) {
- if (credentials.getUseManagedIdentityCredentials()) {
- auto storage_client = Azure::Storage::Blobs::BlobServiceClient(
- "https://" + credentials.getStorageAccountName() + ".blob." + credentials.getEndpointSuffix(), std::make_shared<Azure::Identity::ManagedIdentityCredential>());
- return storage_client.GetBlobContainerClient(container_name);
- } else {
+ if (credentials.getCredentialConfigurationStrategy() == CredentialConfigurationStrategyOption::FromProperties) {
return Azure::Storage::Blobs::BlobContainerClient::CreateFromConnectionString(credentials.buildConnectionString(), container_name);
}
+
+ auto storage_client = Azure::Storage::Blobs::BlobServiceClient("https://" + credentials.getStorageAccountName() + ".blob." + credentials.getEndpointSuffix(),
+ credentials.createAzureTokenCredential());
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/fa8b04f30e3b383886bd9004babe21fa327ac070
##########
extensions/azure/storage/AzureDataLakeStorageClient.cpp:
##########
@@ -42,14 +42,14 @@ std::unique_ptr<Azure::Storage::Files::DataLake::DataLakeFileSystemClient> Azure
options.Retry.MaxRetries = gsl::narrow<int32_t>(*number_of_retries);
}
- if (credentials.getUseManagedIdentityCredentials()) {
- auto datalake_service_client = Azure::Storage::Files::DataLake::DataLakeServiceClient(
- "https://" + credentials.getStorageAccountName() + ".dfs." + credentials.getEndpointSuffix(), std::make_shared<Azure::Identity::ManagedIdentityCredential>(), options);
- return std::make_unique<Azure::Storage::Files::DataLake::DataLakeFileSystemClient>(datalake_service_client.GetFileSystemClient(file_system_name));
- } else {
+ if (credentials.getCredentialConfigurationStrategy() == CredentialConfigurationStrategyOption::FromProperties) {
return std::make_unique<Azure::Storage::Files::DataLake::DataLakeFileSystemClient>(
- Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
+ Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/fa8b04f30e3b383886bd9004babe21fa327ac070
##########
extensions/azure/storage/AzureDataLakeStorageClient.cpp:
##########
@@ -42,14 +42,14 @@ std::unique_ptr<Azure::Storage::Files::DataLake::DataLakeFileSystemClient> Azure
options.Retry.MaxRetries = gsl::narrow<int32_t>(*number_of_retries);
}
- if (credentials.getUseManagedIdentityCredentials()) {
- auto datalake_service_client = Azure::Storage::Files::DataLake::DataLakeServiceClient(
- "https://" + credentials.getStorageAccountName() + ".dfs." + credentials.getEndpointSuffix(), std::make_shared<Azure::Identity::ManagedIdentityCredential>(), options);
- return std::make_unique<Azure::Storage::Files::DataLake::DataLakeFileSystemClient>(datalake_service_client.GetFileSystemClient(file_system_name));
- } else {
+ if (credentials.getCredentialConfigurationStrategy() == CredentialConfigurationStrategyOption::FromProperties) {
return std::make_unique<Azure::Storage::Files::DataLake::DataLakeFileSystemClient>(
- Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
+ Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
}
+
+ auto datalake_service_client = Azure::Storage::Files::DataLake::DataLakeServiceClient(
+ "https://" + credentials.getStorageAccountName() + ".dfs." + credentials.getEndpointSuffix(), credentials.createAzureTokenCredential(), options);
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/fa8b04f30e3b383886bd9004babe21fa327ac070
##########
extensions/azure/storage/AzureStorageCredentials.cpp:
##########
@@ -90,16 +102,37 @@ std::string AzureStorageCredentials::buildConnectionString() const {
}
bool AzureStorageCredentials::isValid() const {
- return (getUseManagedIdentityCredentials() && !getStorageAccountName().empty()) ||
- (!getUseManagedIdentityCredentials() && !buildConnectionString().empty());
+ return (credential_configuration_strategy_ != CredentialConfigurationStrategyOption::FromProperties && !getStorageAccountName().empty()) ||
+ (credential_configuration_strategy_ == CredentialConfigurationStrategyOption::FromProperties && !buildConnectionString().empty());
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/fa8b04f30e3b383886bd9004babe21fa327ac070
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2315284954
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.h:
##########
@@ -42,33 +43,44 @@ class AzureStorageCredentialsService : public core::controller::ControllerServic
.build();
EXTENSIONAPI static constexpr auto StorageAccountKey = core::PropertyDefinitionBuilder<>::createProperty("Storage Account Key")
.withDescription("The storage account key. This is an admin-like password providing access to every container in this account. "
- "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies.")
+ "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies "
+ "if Credential Configuration Strategy is set to From Properties. If set, SAS Token must be empty.")
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/dfc0162574dd581cce887cffc943a5e57d6c92e4
##########
extensions/azure/utils/AzureEnums.h:
##########
@@ -19,11 +19,39 @@
*/
#pragma once
+#include "magic_enum.hpp"
+
namespace org::apache::nifi::minifi::azure {
enum class EntityTracking {
none,
timestamps
};
+enum class CredentialConfigurationStrategyOption {
+ fromProperties,
+ defaultCredential,
+ managedIdentity,
+ workloadIdentity
+};
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/dfc0162574dd581cce887cffc943a5e57d6c92e4
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -29,6 +29,10 @@ void AzureStorageCredentialsService::initialize() {
}
void AzureStorageCredentialsService::onEnable() {
+ if (auto credential_configuration_strategy = magic_enum::enum_cast<CredentialConfigurationStrategyOption>(
+ getProperty(CredentialConfigurationStrategy.name).value_or(std::string{magic_enum::enum_name(CredentialConfigurationStrategyOption::fromProperties)}))) {
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/dfc0162574dd581cce887cffc943a5e57d6c92e4
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2368789434
##########
extensions/azure/tests/ListAzureDataLakeStorageTests.cpp:
##########
@@ -43,7 +43,7 @@ class ListAzureDataLakeStorageTestsFixture {
auto mock_data_lake_storage_client = std::make_unique<MockDataLakeStorageClient>();
mock_data_lake_storage_client_ptr_ = mock_data_lake_storage_client.get();
auto uuid = utils::IdGenerator::getIdGenerator()->generate();
- auto impl = std::unique_ptr<minifi::azure::processors::ListAzureDataLakeStorage>(
+ auto impl = std::unique_ptr<minifi::azure::processors::ListAzureDataLakeStorage>( // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
Unfortunately no due to the constructor overload being private and only the friend class `ListAzureDataLakeStorageTestsFixture` can instantiate it, the `unique_ptr` cannot.
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2352024651
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -39,13 +43,13 @@ void AzureStorageCredentialsService::onEnable() {
credentials_.setSasToken(*sas_token);
}
if (auto common_storage_account_endpoint_suffix = getProperty(CommonStorageAccountEndpointSuffix.name)) {
- credentials_.setEndpontSuffix(*common_storage_account_endpoint_suffix);
+ credentials_.setEndpointSuffix(*common_storage_account_endpoint_suffix);
}
if (auto connection_String = getProperty(ConnectionString.name)) {
credentials_.setConnectionString(*connection_String);
}
- if (auto use_managed_identity_credentials = getProperty(UseManagedIdentityCredentials.name) | utils::andThen(parsing::parseBool)) {
- credentials_.setUseManagedIdentityCredentials(*use_managed_identity_credentials);
+ if (auto managed_identity_client_id = getProperty(ManagedIdentityClientId.name)) {
+ credentials_.setManagedIdentityClientId(*managed_identity_client_id);
Review Comment:
Sure, added it as a note in the descrpition
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2329841908
##########
extensions/azure/storage/AzureStorageCredentials.cpp:
##########
@@ -90,16 +102,37 @@ std::string AzureStorageCredentials::buildConnectionString() const {
}
bool AzureStorageCredentials::isValid() const {
- return (getUseManagedIdentityCredentials() && !getStorageAccountName().empty()) ||
- (!getUseManagedIdentityCredentials() && !buildConnectionString().empty());
+ return (credential_configuration_strategy_ != CredentialConfigurationStrategyOption::FromProperties && !getStorageAccountName().empty()) ||
+ (credential_configuration_strategy_ == CredentialConfigurationStrategyOption::FromProperties && !buildConnectionString().empty());
+}
+
+std::shared_ptr<Azure::Core::Credentials::TokenCredential> AzureStorageCredentials::createAzureTokenCredential() const {
+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> credential;
+ if (credential_configuration_strategy_ == CredentialConfigurationStrategyOption::ManagedIdentity) {
+ if (managed_identity_client_id_.empty()) {
+ Azure::Identity::ManagedIdentityCredentialOptions options;
+ options.IdentityId = Azure::Identity::ManagedIdentityId::SystemAssigned();
+ credential = std::make_shared<Azure::Identity::ManagedIdentityCredential>(options);
+ } else {
+ Azure::Identity::ManagedIdentityCredentialOptions options;
+ options.IdentityId = Azure::Identity::ManagedIdentityId::FromUserAssignedClientId(managed_identity_client_id_);
+ credential = std::make_shared<Azure::Identity::ManagedIdentityCredential>(options);
+ }
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/fa8b04f30e3b383886bd9004babe21fa327ac070
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm closed pull request #2016: MINIFICPP-2614 Add more authentication options to Azure processors
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2352012434
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -39,13 +43,13 @@ void AzureStorageCredentialsService::onEnable() {
credentials_.setSasToken(*sas_token);
}
if (auto common_storage_account_endpoint_suffix = getProperty(CommonStorageAccountEndpointSuffix.name)) {
- credentials_.setEndpontSuffix(*common_storage_account_endpoint_suffix);
+ credentials_.setEndpointSuffix(*common_storage_account_endpoint_suffix);
}
if (auto connection_String = getProperty(ConnectionString.name)) {
credentials_.setConnectionString(*connection_String);
}
- if (auto use_managed_identity_credentials = getProperty(UseManagedIdentityCredentials.name) | utils::andThen(parsing::parseBool)) {
- credentials_.setUseManagedIdentityCredentials(*use_managed_identity_credentials);
+ if (auto managed_identity_client_id = getProperty(ManagedIdentityClientId.name)) {
+ credentials_.setManagedIdentityClientId(*managed_identity_client_id);
Review Comment:
Could you highlight this in the PR description? Hopefully it makes it into the merge commit message.
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -39,13 +43,13 @@ void AzureStorageCredentialsService::onEnable() {
credentials_.setSasToken(*sas_token);
}
if (auto common_storage_account_endpoint_suffix = getProperty(CommonStorageAccountEndpointSuffix.name)) {
- credentials_.setEndpontSuffix(*common_storage_account_endpoint_suffix);
+ credentials_.setEndpointSuffix(*common_storage_account_endpoint_suffix);
}
if (auto connection_String = getProperty(ConnectionString.name)) {
credentials_.setConnectionString(*connection_String);
}
- if (auto use_managed_identity_credentials = getProperty(UseManagedIdentityCredentials.name) | utils::andThen(parsing::parseBool)) {
- credentials_.setUseManagedIdentityCredentials(*use_managed_identity_credentials);
+ if (auto managed_identity_client_id = getProperty(ManagedIdentityClientId.name)) {
+ credentials_.setManagedIdentityClientId(*managed_identity_client_id);
Review Comment:
Could you highlight the breaking change in the PR description? Hopefully it makes it into the merge commit message.
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2352309301
##########
cmake/AzureSdkCpp.cmake:
##########
@@ -55,3 +55,5 @@ FetchContent_Declare(asdkext
)
FetchContent_MakeAvailable(asdkext)
+
+add_dependencies(azure-storage-common LibXml2::LibXml2)
Review Comment:
why is this necessary?
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2379485860
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -29,6 +29,10 @@ void AzureStorageCredentialsService::initialize() {
}
void AzureStorageCredentialsService::onEnable() {
+ auto credential_configuration_strategy_str = getProperty(CredentialConfigurationStrategy.name).value_or(std::string{magic_enum::enum_name(CredentialConfigurationStrategyOption::FromProperties)});
+ if (auto credential_configuration_strategy = magic_enum::enum_cast<CredentialConfigurationStrategyOption>(credential_configuration_strategy_str)) {
+ credentials_.setCredentialConfigurationStrategy(*credential_configuration_strategy);
+ }
Review Comment:
Updated in https://github.com/apache/nifi-minifi-cpp/pull/2016/commits/376db99250281bef25f1a23378630b001809d2b2
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2368006469
##########
extensions/azure/tests/ListAzureDataLakeStorageTests.cpp:
##########
@@ -43,7 +43,7 @@ class ListAzureDataLakeStorageTestsFixture {
auto mock_data_lake_storage_client = std::make_unique<MockDataLakeStorageClient>();
mock_data_lake_storage_client_ptr_ = mock_data_lake_storage_client.get();
auto uuid = utils::IdGenerator::getIdGenerator()->generate();
- auto impl = std::unique_ptr<minifi::azure::processors::ListAzureDataLakeStorage>(
+ auto impl = std::unique_ptr<minifi::azure::processors::ListAzureDataLakeStorage>( // NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks)
Review Comment:
does make_unique not work here?
--
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
Re: [PR] MINIFICPP-2614 Add more authentication options to Azure processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2016:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2016#discussion_r2315916546
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.h:
##########
@@ -42,33 +43,44 @@ class AzureStorageCredentialsService : public core::controller::ControllerServic
.build();
EXTENSIONAPI static constexpr auto StorageAccountKey = core::PropertyDefinitionBuilder<>::createProperty("Storage Account Key")
.withDescription("The storage account key. This is an admin-like password providing access to every container in this account. "
- "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies.")
+ "It is recommended one uses Shared Access Signature (SAS) token instead for fine-grained control with policies "
+ "if Credential Configuration Strategy is set to From Properties. If set, SAS Token must be empty.")
.isSensitive(true)
.build();
EXTENSIONAPI static constexpr auto SASToken = core::PropertyDefinitionBuilder<>::createProperty("SAS Token")
- .withDescription("Shared Access Signature token. Specify either SAS Token (recommended) or Storage Account Key together with Storage Account Name if Managed Identity is not used.")
+ .withDescription("Shared Access Signature token. Specify either SAS Token (recommended) or Storage Account Key together with Storage Account Name "
+ "if Credential Configuration Strategy is set to From Properties. If set, Storage Account Key must be empty.")
.isSensitive(true)
.build();
EXTENSIONAPI static constexpr auto CommonStorageAccountEndpointSuffix = core::PropertyDefinitionBuilder<>::createProperty("Common Storage Account Endpoint Suffix")
.withDescription("Storage accounts in public Azure always use a common FQDN suffix. Override this endpoint suffix with a "
"different suffix in certain circumstances (like Azure Stack or non-public Azure regions).")
.build();
EXTENSIONAPI static constexpr auto ConnectionString = core::PropertyDefinitionBuilder<>::createProperty("Connection String")
- .withDescription("Connection string used to connect to Azure Storage service. This overrides all other set credential properties if Managed Identity is not used.")
+ .withDescription("Connection string used to connect to Azure Storage service. This overrides all other set credential properties "
+ "if Credential Configuration Strategy is set to From Properties.")
.build();
- EXTENSIONAPI static constexpr auto UseManagedIdentityCredentials = core::PropertyDefinitionBuilder<>::createProperty("Use Managed Identity Credentials")
- .withDescription("If true Managed Identity credentials will be used together with the Storage Account Name for authentication.")
+ EXTENSIONAPI static constexpr auto CredentialConfigurationStrategy =
+ core::PropertyDefinitionBuilder<magic_enum::enum_count<CredentialConfigurationStrategyOption>()>::createProperty("Credential Configuration Strategy")
+ .withDescription("The strategy to use for credential configuration. If set to From Properties, the credentials are parsed from the SAS Token, Storage Account Key, "
+ "and Connection String properties. In other cases, the selected Azure identity source is used.")
Review Comment:
```suggestion
.withDescription("The strategy to use for credential configuration. If set to From Properties, the credentials are parsed from the SAS Token, Storage Account Key, "
"and Connection String properties. In other cases, the selected Azure identity source is used.")
```
##########
extensions/azure/controllerservices/AzureStorageCredentialsService.cpp:
##########
@@ -39,13 +43,13 @@ void AzureStorageCredentialsService::onEnable() {
credentials_.setSasToken(*sas_token);
}
if (auto common_storage_account_endpoint_suffix = getProperty(CommonStorageAccountEndpointSuffix.name)) {
- credentials_.setEndpontSuffix(*common_storage_account_endpoint_suffix);
+ credentials_.setEndpointSuffix(*common_storage_account_endpoint_suffix);
}
if (auto connection_String = getProperty(ConnectionString.name)) {
credentials_.setConnectionString(*connection_String);
}
- if (auto use_managed_identity_credentials = getProperty(UseManagedIdentityCredentials.name) | utils::andThen(parsing::parseBool)) {
- credentials_.setUseManagedIdentityCredentials(*use_managed_identity_credentials);
+ if (auto managed_identity_client_id = getProperty(ManagedIdentityClientId.name)) {
+ credentials_.setManagedIdentityClientId(*managed_identity_client_id);
Review Comment:
this is a breaking change, right?
##########
extensions/azure/storage/AzureDataLakeStorageClient.cpp:
##########
@@ -42,14 +42,14 @@ std::unique_ptr<Azure::Storage::Files::DataLake::DataLakeFileSystemClient> Azure
options.Retry.MaxRetries = gsl::narrow<int32_t>(*number_of_retries);
}
- if (credentials.getUseManagedIdentityCredentials()) {
- auto datalake_service_client = Azure::Storage::Files::DataLake::DataLakeServiceClient(
- "https://" + credentials.getStorageAccountName() + ".dfs." + credentials.getEndpointSuffix(), std::make_shared<Azure::Identity::ManagedIdentityCredential>(), options);
- return std::make_unique<Azure::Storage::Files::DataLake::DataLakeFileSystemClient>(datalake_service_client.GetFileSystemClient(file_system_name));
- } else {
+ if (credentials.getCredentialConfigurationStrategy() == CredentialConfigurationStrategyOption::FromProperties) {
return std::make_unique<Azure::Storage::Files::DataLake::DataLakeFileSystemClient>(
- Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
+ Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
}
+
+ auto datalake_service_client = Azure::Storage::Files::DataLake::DataLakeServiceClient(
+ "https://" + credentials.getStorageAccountName() + ".dfs." + credentials.getEndpointSuffix(), credentials.createAzureTokenCredential(), options);
Review Comment:
```suggestion
auto datalake_service_client = Azure::Storage::Files::DataLake::DataLakeServiceClient(
"https://" + credentials.getStorageAccountName() + ".dfs." + credentials.getEndpointSuffix(), credentials.createAzureTokenCredential(), options);
```
##########
extensions/azure/storage/AzureDataLakeStorageClient.cpp:
##########
@@ -42,14 +42,14 @@ std::unique_ptr<Azure::Storage::Files::DataLake::DataLakeFileSystemClient> Azure
options.Retry.MaxRetries = gsl::narrow<int32_t>(*number_of_retries);
}
- if (credentials.getUseManagedIdentityCredentials()) {
- auto datalake_service_client = Azure::Storage::Files::DataLake::DataLakeServiceClient(
- "https://" + credentials.getStorageAccountName() + ".dfs." + credentials.getEndpointSuffix(), std::make_shared<Azure::Identity::ManagedIdentityCredential>(), options);
- return std::make_unique<Azure::Storage::Files::DataLake::DataLakeFileSystemClient>(datalake_service_client.GetFileSystemClient(file_system_name));
- } else {
+ if (credentials.getCredentialConfigurationStrategy() == CredentialConfigurationStrategyOption::FromProperties) {
return std::make_unique<Azure::Storage::Files::DataLake::DataLakeFileSystemClient>(
- Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
+ Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
Review Comment:
```suggestion
Azure::Storage::Files::DataLake::DataLakeFileSystemClient::CreateFromConnectionString(credentials.buildConnectionString(), file_system_name, options));
```
##########
extensions/azure/storage/AzureBlobStorageClient.cpp:
##########
@@ -56,13 +56,13 @@ AzureBlobStorageClient::AzureBlobStorageClient() {
}
Azure::Storage::Blobs::BlobContainerClient AzureBlobStorageClient::createClient(const AzureStorageCredentials &credentials, const std::string &container_name) {
- if (credentials.getUseManagedIdentityCredentials()) {
- auto storage_client = Azure::Storage::Blobs::BlobServiceClient(
- "https://" + credentials.getStorageAccountName() + ".blob." + credentials.getEndpointSuffix(), std::make_shared<Azure::Identity::ManagedIdentityCredential>());
- return storage_client.GetBlobContainerClient(container_name);
- } else {
+ if (credentials.getCredentialConfigurationStrategy() == CredentialConfigurationStrategyOption::FromProperties) {
return Azure::Storage::Blobs::BlobContainerClient::CreateFromConnectionString(credentials.buildConnectionString(), container_name);
}
+
+ auto storage_client = Azure::Storage::Blobs::BlobServiceClient("https://" + credentials.getStorageAccountName() + ".blob." + credentials.getEndpointSuffix(),
+ credentials.createAzureTokenCredential());
Review Comment:
```suggestion
auto storage_client = Azure::Storage::Blobs::BlobServiceClient("https://" + credentials.getStorageAccountName() + ".blob." + credentials.getEndpointSuffix(),
credentials.createAzureTokenCredential());
```
##########
extensions/azure/storage/AzureStorageCredentials.cpp:
##########
@@ -90,16 +102,37 @@ std::string AzureStorageCredentials::buildConnectionString() const {
}
bool AzureStorageCredentials::isValid() const {
- return (getUseManagedIdentityCredentials() && !getStorageAccountName().empty()) ||
- (!getUseManagedIdentityCredentials() && !buildConnectionString().empty());
+ return (credential_configuration_strategy_ != CredentialConfigurationStrategyOption::FromProperties && !getStorageAccountName().empty()) ||
+ (credential_configuration_strategy_ == CredentialConfigurationStrategyOption::FromProperties && !buildConnectionString().empty());
Review Comment:
```suggestion
return (credential_configuration_strategy_ != CredentialConfigurationStrategyOption::FromProperties && !getStorageAccountName().empty()) ||
(credential_configuration_strategy_ == CredentialConfigurationStrategyOption::FromProperties && !buildConnectionString().empty());
```
##########
extensions/azure/storage/AzureStorageCredentials.cpp:
##########
@@ -90,16 +102,37 @@ std::string AzureStorageCredentials::buildConnectionString() const {
}
bool AzureStorageCredentials::isValid() const {
- return (getUseManagedIdentityCredentials() && !getStorageAccountName().empty()) ||
- (!getUseManagedIdentityCredentials() && !buildConnectionString().empty());
+ return (credential_configuration_strategy_ != CredentialConfigurationStrategyOption::FromProperties && !getStorageAccountName().empty()) ||
+ (credential_configuration_strategy_ == CredentialConfigurationStrategyOption::FromProperties && !buildConnectionString().empty());
+}
+
+std::shared_ptr<Azure::Core::Credentials::TokenCredential> AzureStorageCredentials::createAzureTokenCredential() const {
+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> credential;
+ if (credential_configuration_strategy_ == CredentialConfigurationStrategyOption::ManagedIdentity) {
+ if (managed_identity_client_id_.empty()) {
+ Azure::Identity::ManagedIdentityCredentialOptions options;
+ options.IdentityId = Azure::Identity::ManagedIdentityId::SystemAssigned();
+ credential = std::make_shared<Azure::Identity::ManagedIdentityCredential>(options);
+ } else {
+ Azure::Identity::ManagedIdentityCredentialOptions options;
+ options.IdentityId = Azure::Identity::ManagedIdentityId::FromUserAssignedClientId(managed_identity_client_id_);
+ credential = std::make_shared<Azure::Identity::ManagedIdentityCredential>(options);
+ }
Review Comment:
```suggestion
credential = std::make_shared<Azure::Identity::ManagedIdentityCredential>(Azure::Identity::ManagedIdentityCredentialOptions{
.IdentityId = managed_identity_client_id_.empty()
? Azure::Identity::ManagedIdentityId::SystemAssigned()
: Azure::Identity::ManagedIdentityId::FromUserAssignedClientId(managed_identity_client_id_)
});
```
or
```suggestion
Azure::Identity::ManagedIdentityCredentialOptions options;
if (managed_identity_client_id_.empty()) {
options.IdentityId = Azure::Identity::ManagedIdentityId::SystemAssigned();
} else {
options.IdentityId = Azure::Identity::ManagedIdentityId::FromUserAssignedClientId(managed_identity_client_id_);
}
credential = std::make_shared<Azure::Identity::ManagedIdentityCredential>(options);
```
--
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