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

Support Endpoint Override for CredentialsProviders #263

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

waahm7
Copy link
Contributor

@waahm7 waahm7 commented Feb 4, 2025

Issue #, if available:
#257
awslabs/aws-crt-swift#311

Description of changes:
Support endpoint override for credentials providers.

The order of resolution for configured endpoint is as follows:

  1. The value provided by a service-specific environment variable, AWS_ENDPOINT_URL_<SERVICE>. <SERVICE> here is the uppercased version of corresponding service identifier in this official list.
  2. The value provided by the global endpoint environment variable, AWS_ENDPOINT_URL.
  3. The value provided by a service-specific parameter from a services definition section referenced in a profile in the shared configuration file. The name of the service must match the identifier in this official list. E.g.:
[profile profile-with-services]
services = dev
endpoint_url = http://localhost:5567

[services dev]
sts = 
  endpoint_url = https://play.min.io:9000
  1. The value provided by the global parameter from a profile in the shared configuration file. E.g.:
[profile profile-with-services]
services = dev
endpoint_url = http://localhost:5567

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 92.95775% with 5 lines in your changes missing coverage. Please review.

Project coverage is 80.61%. Comparing base (b513db4) to head (dcb8868).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
source/credentials_provider_sts.c 85.71% 3 Missing ⚠️
source/credentials_utils.c 95.83% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #263      +/-   ##
==========================================
+ Coverage   80.50%   80.61%   +0.11%     
==========================================
  Files          33       33              
  Lines        6099     6140      +41     
==========================================
+ Hits         4910     4950      +40     
- Misses       1189     1190       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

/* check environment variable first */
struct aws_string *region = aws_credentials_provider_resolve_region_from_env(allocator);
if (region != NULL && region->len > 0) {
return region;
}

if (profile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a few lines up, we can leak the region string if it's ""

Copy link
Contributor

Choose a reason for hiding this comment

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

the aws_credentials_provider_resolve_region_from_env() function has the same bug

Copy link
Contributor

Choose a reason for hiding this comment

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

should we just have an alternate get-env-var function that won't return empty strings?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh man, if we're adding new get-env functions, we can fix the terrible API of the current one (bad because it did out-values to handle reporting OOM which we don't do anymore, and bad because it didn't just take char * which forces some users to allocate/cleanup a needless aws_string)

struct aws_string *aws_getenv_nonempty(struct aws_allocator *, const char *); // NULL if missing or ""
struct aws_string *aws_getenv_raw(struct aws_allocator *, const char *); // may be ""

*/
AWS_AUTH_API
int aws_credentials_provider_construct_regional_endpoint(
int aws_credentials_provider_construct_endpoint(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not proposing an options-struct because this is private

But if someone moves this out of private/ and is like "WHY SHOULD I CHANGE IT YOU ALREADY APPROVED THE EXISTING CODE"

then I can point at this comment here and say "NUH UH"

s_sts_service_env_name,
s_sts_service_name,
profile_collection,
profile)) {
goto cleanup;
Copy link
Contributor

Choose a reason for hiding this comment

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

if something goes wrong here, we end up setting out_region but not out_endpoint?

is that failure? or is that something we expect to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logically, it won't happen since if we resolve the region, we can construct the endpoint. But even if it does happen in the future, we have a fallback: We will check that out_endpoint is not set and then use the global STS endpoint.

Comment on lines 745 to 750
*out_region = s_resolve_region(allocator, profile);

if (aws_credentials_provider_construct_endpoint(
allocator,
out_endpoint,
*out_region,
Copy link
Contributor

Choose a reason for hiding this comment

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

code clarity: *out_region could be NULL at this point....
and I see that aws_credentials_provider_construct_endpoint() only requires region sometimes

if region isn't always required, add a comment here to that effect

but if it is always required, maybe early-out if s_resolve_region() returns NULL

source/credentials_utils.c Outdated Show resolved Hide resolved
on_finish:
aws_byte_buf_clean_up(&service_endpoint_buf);
aws_string_destroy(service_endpoint_str);
return out_endpoint;
Copy link
Contributor

Choose a reason for hiding this comment

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

there are a lot of paths where NULL is returned, but no aws_raise_error() happened. Is that OK for this helper function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, NULL means that there was no endpoint_override configured.

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.

3 participants