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 2026/10/01 11:40:32 UTC
[PR] MINIFICPP-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
lordgamez opened a new pull request, #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269
- Deprecate authentication options in AWS and Azure processors in favor of credential services
- Use providers instead of AWS credentials snapshots for automatic refresh of temporary credentials
- Fix setEnvironmentVariable on Windows to also update CRT environment variables
System tests were done manually with different authentication method on an AWS EC2 instance.
https://issues.apache.org/jira/browse/MINIFICPP-2909
---------------------
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4155281659
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,6 +107,7 @@ bool Environment::unsetEnvironmentVariable(const char* name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
+ _putenv_s(name, "");
success = SetEnvironmentVariableA(name, nullptr);
Review Comment:
Updated handling in https://github.com/apache/nifi-minifi-cpp/pull/2269/commits/8c9fa6e5c8d72369ac85f2df3fe2fa6279b84aea
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4156779503
##########
.github/references/ubuntu_22_04_clang_arm_manifest.json:
##########
@@ -444,7 +444,7 @@
},
"Access Key": {
"name": "Access Key",
- "description": "AWS account access key",
+ "description": "AWS account access key. DEPRECATED, please use AWS Credentials Provider service instead.",
Review Comment:
I'd put DEPRECATED as the first word of the description.
##########
libminifi/test/libtest/unit/TestUtils.h:
##########
@@ -69,6 +71,41 @@ std::string getFileContent(const std::filesystem::path& file_name);
void makeFileOrDirectoryNotWritable(const std::filesystem::path& file_name);
void makeFileOrDirectoryWritable(const std::filesystem::path& file_name);
+/**
+ * Sets an environment variable for the lifetime of the object, and restores its previous value (or unsets it, if it was
+ * not set before) on destruction. Pass std::nullopt as the value to make sure the variable is not set in the scope.
+ * Environment variables are process-global, so tests that rely on them need to clean up after themselves.
Review Comment:
I don't agree with the second part and the implication. Envvars are process-global, but tests could also just leave them dirty, and let the process eventually die.
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,7 +107,13 @@ bool Environment::unsetEnvironmentVariable(const char* name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
- success = SetEnvironmentVariableA(name, nullptr);
+ const bool crt_success = _putenv_s(name, "") == 0;
+ bool windows_success = SetEnvironmentVariableA(name, nullptr) != 0;
+ if (!windows_success && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
+ windows_success = true;
+ }
+
+ success = crt_success && windows_success;
Review Comment:
why do we need to do two ways of setting the envvar? Can you explain what this fixes?
##########
libminifi/test/unit/EnvironmentUtilsTests.cpp:
##########
@@ -89,6 +90,16 @@ TEST_CASE("unsetenv existing", "[unsetenv]") {
REQUIRE(!utils::Environment::getEnvironmentVariable("UNSETENV2"));
}
+TEST_CASE("setenv and unsetenv are visible to getenv", "[setenv][unsetenv]") {
+ REQUIRE(true == utils::Environment::setEnvironmentVariable("GETENVSYNC", "test"));
+ const char* const value = std::getenv("GETENVSYNC");
+ REQUIRE(value != nullptr);
+ CHECK(std::string{value} == "test");
+
+ REQUIRE(true == utils::Environment::unsetEnvironmentVariable("GETENVSYNC"));
+ CHECK(std::getenv("GETENVSYNC") == nullptr);
+}
+
Review Comment:
setenv is not thread-safe, so this test can only be run in single-threaded processes.
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4167066675
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,7 +107,13 @@ bool Environment::unsetEnvironmentVariable(const char* name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
- success = SetEnvironmentVariableA(name, nullptr);
+ const bool crt_success = _putenv_s(name, "") == 0;
+ bool windows_success = SetEnvironmentVariableA(name, nullptr) != 0;
+ if (!windows_success && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
+ windows_success = true;
+ }
+
+ success = crt_success && windows_success;
Review Comment:
Currently on Windows our `setEnvironmentVariable` util uses both `SetEnvironmentVariableA` which sets the process's environment variables and `_putenv_s` that sets the C runtime's environment variables. `unsetEnvironmentVariable` only used `SetEnvironmentVariableA` variable to unset an envvar which left the CRT level variable in place and when a library like AWS used `getenv` for getting a variable it found it even though we wanted to unset it, this change keeps the 2 variable tables in sync and makes the `setEnvironmentVariable` and `unsetEnvironmentVariable` utility functions mirror each other.
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4181554121
##########
libminifi/test/libtest/unit/TestUtils.h:
##########
@@ -69,6 +71,41 @@ std::string getFileContent(const std::filesystem::path& file_name);
void makeFileOrDirectoryNotWritable(const std::filesystem::path& file_name);
void makeFileOrDirectoryWritable(const std::filesystem::path& file_name);
+/**
+ * Sets an environment variable for the lifetime of the object, and restores its previous value (or unsets it, if it was
+ * not set before) on destruction. Pass std::nullopt as the value to make sure the variable is not set in the scope.
+ * Environment variables are process-global, so tests that rely on them need to clean up after themselves.
Review Comment:
For example the default credential chains goes through the available credential options like access keys, profile credentials, instance profile, session tokens, and these credentials options can also be set in environment variables. We expect a specific credential to be used when we run a test and if we set another one in a previous test it could be prioritized and found in the default chain instead of the one we expect to be found, so it's better to clear all of them at the beginning especially as the order of the tests is randomized.
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4155166679
##########
extensions/aws/controllerservices/AWSCredentialsService.cpp:
##########
@@ -28,25 +32,49 @@ void AWSCredentialsService::initialize() {
}
void AWSCredentialsService::onEnable() {
+ AWSCredentialsProviderSettings settings;
if (const auto access_key = getProperty(AccessKey.name)) {
- aws_credentials_provider_.setAccessKey(*access_key);
+ settings.access_key = *access_key;
}
if (const auto secret_key = getProperty(SecretKey.name)) {
- aws_credentials_provider_.setSecretKey(*secret_key);
+ settings.secret_key = *secret_key;
}
if (const auto credentials_file = getProperty(CredentialsFile.name)) {
- aws_credentials_provider_.setCredentialsFile(*credentials_file);
+ settings.credentials_file = *credentials_file;
+ }
+ if (const auto profile_name = getProperty(ProfileName.name)) {
+ settings.profile_name = *profile_name;
+ }
+ if (const auto sso_profile_name = getProperty(SSOProfileName.name)) {
+ settings.sso_profile_name = *sso_profile_name;
}
- if (const auto use_credentials = getProperty(UseDefaultCredentials.name) | minifi::utils::andThen(parsing::parseBool)) {
- aws_credentials_provider_.setUseDefaultCredentials(*use_credentials);
+ if (const auto use_default_credentials = getProperty(UseDefaultCredentials.name) | minifi::utils::andThen(parsing::parseBool); use_default_credentials && *use_default_credentials) {
+ settings.credential_configuration_strategy = CredentialConfigurationStrategyOption::DefaultCredentials;
Review Comment:
This is an improvement as having this fallback would cause confusion which credentials are used. Setting the Use Default Credentials property should override the credentials set in the properties and in case it cannot be retrieved it should not fail silently.
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4167186021
##########
libminifi/test/unit/EnvironmentUtilsTests.cpp:
##########
@@ -89,6 +90,16 @@ TEST_CASE("unsetenv existing", "[unsetenv]") {
REQUIRE(!utils::Environment::getEnvironmentVariable("UNSETENV2"));
}
+TEST_CASE("setenv and unsetenv are visible to getenv", "[setenv][unsetenv]") {
+ REQUIRE(true == utils::Environment::setEnvironmentVariable("GETENVSYNC", "test"));
+ const char* const value = std::getenv("GETENVSYNC");
+ REQUIRE(value != nullptr);
+ CHECK(std::string{value} == "test");
+
+ REQUIRE(true == utils::Environment::unsetEnvironmentVariable("GETENVSYNC"));
+ CHECK(std::getenv("GETENVSYNC") == nullptr);
+}
+
Review Comment:
Catch2 doesn't run tests in parallel, so this test case runs independently on a single thread. No multiple threads are present when `std::getenv` is running, so this should not be a problem, or did you refer to something else?
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4181690025
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,7 +107,13 @@ bool Environment::unsetEnvironmentVariable(const char* name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
- success = SetEnvironmentVariableA(name, nullptr);
+ const bool crt_success = _putenv_s(name, "") == 0;
+ bool windows_success = SetEnvironmentVariableA(name, nullptr) != 0;
+ if (!windows_success && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
+ windows_success = true;
+ }
+
+ success = crt_success && windows_success;
Review Comment:
These are two separate APIs for using the environment variables as stated in this stack overflow topic and the answer lists the corresponding Microsoft documentation https://stackoverflow.com/a/69199802
The docs of `getenv` state that "getenv operates only on the data structures accessible to the run-time library and not on the environment "segment" created for the process by the operating system. " so it is different from what `SetEnvironmentVariableA` operates on.
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4167087974
##########
libminifi/test/libtest/unit/TestUtils.h:
##########
@@ -69,6 +71,41 @@ std::string getFileContent(const std::filesystem::path& file_name);
void makeFileOrDirectoryNotWritable(const std::filesystem::path& file_name);
void makeFileOrDirectoryWritable(const std::filesystem::path& file_name);
+/**
+ * Sets an environment variable for the lifetime of the object, and restores its previous value (or unsets it, if it was
+ * not set before) on destruction. Pass std::nullopt as the value to make sure the variable is not set in the scope.
+ * Environment variables are process-global, so tests that rely on them need to clean up after themselves.
Review Comment:
As multiple tests use different environment variables in a single test suite we need to clear them after each test case to cleanup between test cases not just at the end of the process.
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4154987476
##########
extensions/aws/controllerservices/AWSCredentialsService.cpp:
##########
@@ -28,25 +32,49 @@ void AWSCredentialsService::initialize() {
}
void AWSCredentialsService::onEnable() {
+ AWSCredentialsProviderSettings settings;
if (const auto access_key = getProperty(AccessKey.name)) {
- aws_credentials_provider_.setAccessKey(*access_key);
+ settings.access_key = *access_key;
}
if (const auto secret_key = getProperty(SecretKey.name)) {
- aws_credentials_provider_.setSecretKey(*secret_key);
+ settings.secret_key = *secret_key;
}
if (const auto credentials_file = getProperty(CredentialsFile.name)) {
- aws_credentials_provider_.setCredentialsFile(*credentials_file);
+ settings.credentials_file = *credentials_file;
+ }
+ if (const auto profile_name = getProperty(ProfileName.name)) {
+ settings.profile_name = *profile_name;
+ }
+ if (const auto sso_profile_name = getProperty(SSOProfileName.name)) {
+ settings.sso_profile_name = *sso_profile_name;
}
- if (const auto use_credentials = getProperty(UseDefaultCredentials.name) | minifi::utils::andThen(parsing::parseBool)) {
- aws_credentials_provider_.setUseDefaultCredentials(*use_credentials);
+ if (const auto use_default_credentials = getProperty(UseDefaultCredentials.name) | minifi::utils::andThen(parsing::parseBool); use_default_credentials && *use_default_credentials) {
+ settings.credential_configuration_strategy = CredentialConfigurationStrategyOption::DefaultCredentials;
Review Comment:
The deprecated flag previously tried the default chain and then fell back to this service's Access Key/Secret Key or Credentials File settings. Converting it to the strict `DefaultCredentials` strategy removes that fallback, breaking existing controller-service configurations whenever the default chain is unavailable. Keep the legacy flag distinguishable in the factory so its old fallback order remains intact; the new strategy can retain strict source selection.
##########
extensions/aws/processors/AwsProcessor.cpp:
##########
@@ -32,36 +32,35 @@
namespace org::apache::nifi::minifi::aws::processors {
-std::optional<Aws::Auth::AWSCredentials> AwsProcessor::getAWSCredentialsFromControllerService(core::ProcessContext& context) const {
+std::shared_ptr<Aws::Auth::AWSCredentialsProvider> AwsProcessor::getAWSCredentialsProviderFromControllerService(core::ProcessContext& context) const {
if (auto service = minifi::utils::parseOptionalControllerService<controllers::AWSCredentialsService>(context, AWSCredentialsProviderService, getUUID())) {
- return service->getAWSCredentials();
+ return service->getAWSCredentialsProvider();
}
logger_->log_debug("AWS credentials service could not be found");
- return std::nullopt;
+ return nullptr;
}
-std::optional<Aws::Auth::AWSCredentials> AwsProcessor::getAWSCredentials(core::ProcessContext& context) {
- auto service_cred = getAWSCredentialsFromControllerService(context);
- if (service_cred) {
+std::shared_ptr<Aws::Auth::AWSCredentialsProvider> AwsProcessor::getAWSCredentialsProvider(core::ProcessContext& context) {
+ if (auto service_credentials_provider = getAWSCredentialsProviderFromControllerService(context)) {
logger_->log_info("AWS Credentials successfully set from controller service");
- return service_cred;
+ return service_credentials_provider;
}
- aws::AWSCredentialsProvider aws_credentials_provider;
+ aws::AWSCredentialsProviderSettings settings;
if (const auto access_key = context.getProperty(AccessKey.name)) {
- aws_credentials_provider.setAccessKey(*access_key);
+ settings.access_key = *access_key;
}
if (const auto secret_key = context.getProperty(SecretKey.name)) {
- aws_credentials_provider.setSecretKey(*secret_key);
+ settings.secret_key = *secret_key;
}
if (const auto credentials_file = context.getProperty(CredentialsFile.name)) {
- aws_credentials_provider.setCredentialsFile(*credentials_file);
+ settings.credentials_file = *credentials_file;
}
- if (const auto use_credentials = context.getProperty(UseDefaultCredentials.name) | minifi::utils::andThen(parsing::parseBool)) {
- aws_credentials_provider.setUseDefaultCredentials(*use_credentials);
+ if (const auto use_default_credentials = context.getProperty(UseDefaultCredentials.name) | minifi::utils::andThen(parsing::parseBool); use_default_credentials && *use_default_credentials) {
+ settings.credential_configuration_strategy = CredentialConfigurationStrategyOption::DefaultCredentials;
Review Comment:
This changes the deprecated `Use Default Credentials=true` behavior: the removed provider tried the default chain first and then fell back to the configured access/secret keys or credentials file when that chain returned nothing. Mapping the flag directly to the strict `DefaultCredentials` strategy skips those fallbacks, so existing processor configurations can now fail to schedule. Preserve the legacy fallback semantics separately from the new explicit strategy (and add a regression test for a missing default-chain credential with configured keys/file).
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,6 +107,7 @@ bool Environment::unsetEnvironmentVariable(const char* name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
+ _putenv_s(name, "");
success = SetEnvironmentVariableA(name, nullptr);
Review Comment:
The CRT update result is discarded, so this function can return success when `SetEnvironmentVariableA` succeeds but `_putenv_s` fails, leaving `std::getenv` unsynchronized—the condition this change is intended to prevent. Capture both results without short-circuiting and report success only when both environment stores were cleared.
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4167620752
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,7 +107,13 @@ bool Environment::unsetEnvironmentVariable(const char* name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
- success = SetEnvironmentVariableA(name, nullptr);
+ const bool crt_success = _putenv_s(name, "") == 0;
+ bool windows_success = SetEnvironmentVariableA(name, nullptr) != 0;
+ if (!windows_success && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
+ windows_success = true;
+ }
+
+ success = crt_success && windows_success;
Review Comment:
Well, the `_putenv_s, _wputenv_s, _tputenv_s` page indicated that narrow _environ and wide string _wenviron environments are separate. Is it possible that the syscalls widen the narrow string and modify _wenviron?
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/putenv-s-wputenv-s?view=msvc-170
https://learn.microsoft.com/en-us/cpp/c-runtime-library/environ-wenviron?view=msvc-170
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4167620752
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,7 +107,13 @@ bool Environment::unsetEnvironmentVariable(const char* name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
- success = SetEnvironmentVariableA(name, nullptr);
+ const bool crt_success = _putenv_s(name, "") == 0;
+ bool windows_success = SetEnvironmentVariableA(name, nullptr) != 0;
+ if (!windows_success && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
+ windows_success = true;
+ }
+
+ success = crt_success && windows_success;
Review Comment:
Well, the `_putenv_s, _wputenv_s, _tputenv_s` page indicated that narrow _environ and wide string _wenviron environments are separate. Is it possible that the syscalls widen the narrow string and modify _wenviron?
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4155171301
##########
extensions/aws/processors/AwsProcessor.cpp:
##########
@@ -32,36 +32,35 @@
namespace org::apache::nifi::minifi::aws::processors {
-std::optional<Aws::Auth::AWSCredentials> AwsProcessor::getAWSCredentialsFromControllerService(core::ProcessContext& context) const {
+std::shared_ptr<Aws::Auth::AWSCredentialsProvider> AwsProcessor::getAWSCredentialsProviderFromControllerService(core::ProcessContext& context) const {
if (auto service = minifi::utils::parseOptionalControllerService<controllers::AWSCredentialsService>(context, AWSCredentialsProviderService, getUUID())) {
- return service->getAWSCredentials();
+ return service->getAWSCredentialsProvider();
}
logger_->log_debug("AWS credentials service could not be found");
- return std::nullopt;
+ return nullptr;
}
-std::optional<Aws::Auth::AWSCredentials> AwsProcessor::getAWSCredentials(core::ProcessContext& context) {
- auto service_cred = getAWSCredentialsFromControllerService(context);
- if (service_cred) {
+std::shared_ptr<Aws::Auth::AWSCredentialsProvider> AwsProcessor::getAWSCredentialsProvider(core::ProcessContext& context) {
+ if (auto service_credentials_provider = getAWSCredentialsProviderFromControllerService(context)) {
logger_->log_info("AWS Credentials successfully set from controller service");
- return service_cred;
+ return service_credentials_provider;
}
- aws::AWSCredentialsProvider aws_credentials_provider;
+ aws::AWSCredentialsProviderSettings settings;
if (const auto access_key = context.getProperty(AccessKey.name)) {
- aws_credentials_provider.setAccessKey(*access_key);
+ settings.access_key = *access_key;
}
if (const auto secret_key = context.getProperty(SecretKey.name)) {
- aws_credentials_provider.setSecretKey(*secret_key);
+ settings.secret_key = *secret_key;
}
if (const auto credentials_file = context.getProperty(CredentialsFile.name)) {
- aws_credentials_provider.setCredentialsFile(*credentials_file);
+ settings.credentials_file = *credentials_file;
}
- if (const auto use_credentials = context.getProperty(UseDefaultCredentials.name) | minifi::utils::andThen(parsing::parseBool)) {
- aws_credentials_provider.setUseDefaultCredentials(*use_credentials);
+ if (const auto use_default_credentials = context.getProperty(UseDefaultCredentials.name) | minifi::utils::andThen(parsing::parseBool); use_default_credentials && *use_default_credentials) {
+ settings.credential_configuration_strategy = CredentialConfigurationStrategyOption::DefaultCredentials;
Review Comment:
Same comment: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4155166679
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4167727174
##########
libminifi/test/libtest/unit/TestUtils.h:
##########
@@ -69,6 +71,41 @@ std::string getFileContent(const std::filesystem::path& file_name);
void makeFileOrDirectoryNotWritable(const std::filesystem::path& file_name);
void makeFileOrDirectoryWritable(const std::filesystem::path& file_name);
+/**
+ * Sets an environment variable for the lifetime of the object, and restores its previous value (or unsets it, if it was
+ * not set before) on destruction. Pass std::nullopt as the value to make sure the variable is not set in the scope.
+ * Environment variables are process-global, so tests that rely on them need to clean up after themselves.
Review Comment:
If they use different envvars, then the different tests are not stepping on each other's toes. Am I missing something?
--
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-2909 Add AWS credential configuration strategies to AWSCredentialsService [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2269:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2269#discussion_r4167590610
##########
core-framework/common/src/utils/Environment.cpp:
##########
@@ -107,7 +107,13 @@ bool Environment::unsetEnvironmentVariable(const char* name) {
Environment::accessEnvironment([&success, name](){
#ifdef WIN32
- success = SetEnvironmentVariableA(name, nullptr);
+ const bool crt_success = _putenv_s(name, "") == 0;
+ bool windows_success = SetEnvironmentVariableA(name, nullptr) != 0;
+ if (!windows_success && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
+ windows_success = true;
+ }
+
+ success = crt_success && windows_success;
Review Comment:
Are you sure that there are two "environment variable" storages per process, one accessed through syscalls and one through the CRT? Do you have any sources for this information? I couldn't find any indication in the microsoft docs that show this to be the case, so I don't see why we need to do two different actions that are each supposed to be achieving the same outcome.
--
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