Add an Option to Disable v2 to v1 Fallback #218
Conversation
source/aws_imds_client.c
Outdated
| @@ -509,7 +513,8 @@ static void s_client_on_token_response(struct imds_user_data *user_data) { | |||
| * we should fall back to insecure request. Otherwise, we should use | |||
| * token in following requests. | |||
| */ | |||
| if (user_data->status_code != AWS_HTTP_STATUS_CODE_200_OK || user_data->current_result.len == 0) { | |||
| if (!user_data->ec2_metadata_v1_disabled && | |||
There was a problem hiding this comment.
I would prefer to be more clear about this:
| if (!user_data->ec2_metadata_v1_disabled && | |
| if (user_data->status_code != AWS_HTTP_STATUS_CODE_200_OK || user_data->current_result.len == 0) { | |
| if (user_data->ec2_metadata_v1_disabled) { | |
| //XXX failed to fetch creds and fallback disabled | |
| } | |
| else { | |
| //XXX Fallback to retry | |
| } | |
| } |
There was a problem hiding this comment.
Thanks, I have refactored the code to the following.
if(success){
} else if(fallback disabled){
// error out
} else {
fallback to v1.
}
source/aws_imds_client.c
Outdated
| @@ -533,7 +538,10 @@ static void s_client_on_token_response(struct imds_user_data *user_data) { | |||
| uint64_t expire_timestamp = aws_add_u64_saturating( | |||
| current, aws_timestamp_convert(s_imds_token_ttl_secs, AWS_TIMESTAMP_SECS, AWS_TIMESTAMP_NANOS, NULL)); | |||
| s_update_token_safely( | |||
| user_data->client, cursor.len == 0 ? NULL : &user_data->imds_token, cursor.len != 0, expire_timestamp); | |||
| user_data->client, | |||
| cursor.len == 0 ? NULL : &user_data->imds_token, | |||
There was a problem hiding this comment.
we checked that user_data->current_result is not empty and we have a 200, should we just error out if the cursor here is still empty?
There was a problem hiding this comment.
I have added an assert, as it should not be empty at this point.
| /* | ||
| * If true, fallback from v2 to v1 will be disabled for all cases | ||
| */ | ||
| bool ec2_metadata_v1_disabled; |
There was a problem hiding this comment.
ec2_metadata_v1_disabled, but you can still set the imds_version to V1 manually.
And, it's already part of IMDS options, I think we can skip the ec2_metadata part
Maybe v1_fallback_disabled?
There was a problem hiding this comment.
ec2_metadata_v1_disabled will be used cross SDKs along with the relevant env variable.
Co-authored-by: Michael Graeb <[email protected]>
Co-authored-by: Michael Graeb <[email protected]>
Co-authored-by: Michael Graeb <[email protected]>
Co-authored-by: Michael Graeb <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #218 +/- ##
==========================================
+ Coverage 79.61% 79.62% +0.01%
==========================================
Files 33 33
Lines 5773 5776 +3
==========================================
+ Hits 4596 4599 +3
Misses 1177 1177 ☔ View full report in Codecov by Sentry. |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.