Skip to content
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

[aws-cpp-sdk-core]: allow disabling IMDS in ClientConfiguration default constructor #2612

Closed
grrtrr opened this issue Aug 2, 2023 · 3 comments
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue

Comments

@grrtrr
Copy link
Contributor

grrtrr commented Aug 2, 2023

Problem Description

The ClientConfiguration and its sub-classes allow disabling IMDS lookup in all but the default constructor:

        struct AWS_CORE_API ClientConfiguration
        {
            ClientConfiguration(); // Does IMDS look-up by default, also in sub-classes.
            ClientConfiguration(const char* profileName, bool shouldDisableIMDS = false);
            explicit ClientConfiguration(bool useSmartDefaults, const char* defaultMode = "legacy", bool shouldDisableIMDS = false);
        }

In the base class, profileName can be set to nullptr, but not in the sub-classes: e.g. in S3CrtClientConfiguration, the const char *inputProfileName gets converted to a std::string (which does not like nullptr).

Hence the following use-case will always cause IMDS look-up:

        Aws::S3Crt::ClientConfiguration config;   // Default constructor
        config.scheme = defaults.scheme;
        config.endpointOverride = defaults.endpointOverride;
        config.connectTimeoutMs = 30000;
        // ...

This is because the default constructor hard-codes IMDS lookup:

// src/aws-cpp-sdk-core/source/client/ClientConfiguration.cpp
ClientConfiguration::ClientConfiguration()
{
    this->disableIMDS = false;  // <=== HERE
    // ...

    if (!this->disableIMDS &&
        region.empty() &&
        Aws::Utils::StringUtils::ToLower(Aws::Environment::GetEnv("AWS_EC2_METADATA_DISABLED").c_str()) != "true")
    {
        auto client = Aws::Internal::GetEC2MetadataClient();
        if (client)
        {
            region = client->GetCurrentRegion();  // <== IMDS look-up
        }
    }
    // ...
}

For an earlier record of the problems (slowness) that unnecessary IMDS look-up causes, please see #1440.

What to do

For ClientConfiguration, GenericClientConfiguration, and Aws::S3Crt::ClientConfiguration, please provide a shouldDisableIMDS default argument like the 2 other constructors:

        struct AWS_CORE_API ClientConfiguration
        {
            explicit ClientConfiguration(bool shouldDisableIMDS = false) {
               this->disableIMDS = shouldDisableIMDS;
               setLegacyClientConfigurationParameters(*this);
               // ...
            }
        }
@grrtrr grrtrr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 2, 2023
@yasminetalby yasminetalby self-assigned this Aug 2, 2023
@yasminetalby
Copy link
Contributor

Hello @grrtrr ,

Thank you very much for your submission.
This is a reasonable request and would be a great addition to the SDK.
I will mark this as p2 and post updates regarding implementation/release here.

Thank you very much for your time and feedback.

Sincerely,

Yasmine

@yasminetalby yasminetalby added p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Aug 2, 2023
@yasminetalby yasminetalby mentioned this issue Aug 4, 2023
11 tasks
@jmklix
Copy link
Member

jmklix commented Jun 28, 2024

Fixed with this PR: #2618

@jmklix jmklix closed this as completed Jun 28, 2024
Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

3 participants