Implement SSO Credentials Provider Part 2 #191
Conversation
waahm7
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## token-provider-profile #191 +/- ##
=========================================================
Coverage ? 79.50%
=========================================================
Files ? 33
Lines ? 5527
Branches ? 0
=========================================================
Hits ? 4394
Misses ? 1133
Partials ? 0
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
waahm7
changed the title
| @@ -73,13 +73,15 @@ struct aws_auth_http_system_vtable { | |||
| enum aws_parse_credentials_expiration_format { | |||
| AWS_PCEF_STRING_ISO_8601_DATE, | |||
| AWS_PCEF_NUMBER_UNIX_EPOCH, | |||
| AWS_PCEF_NUMBER_UNIX_EPOCH_MS, | |||
There was a problem hiding this comment.
So, what's AWS_PCEF_NUMBER_UNIX_EPOCH stands for, ns?
There was a problem hiding this comment.
it stands for epoch seconds.
There was a problem hiding this comment.
So, can we change AWS_PCEF_NUMBER_UNIX_EPOCH to AWS_PCEF_NUMBER_UNIX_EPOCH_SEC to be more clear?
There was a problem hiding this comment.
Seems like unix epoch just stands for sec from googling. So, it's fine.
source/credentials_provider_sso.c
Outdated
| struct aws_byte_buf *endpoint, | ||
| const struct aws_string *region) { | ||
| if (!allocator || !endpoint || !region) { | ||
| return AWS_ERROR_INVALID_ARGUMENT; |
There was a problem hiding this comment.
return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
As it's an internal helper function, I'll only handle error from region not set and crash on others are missing:
AWS_PRECONDITION(allocator);
AWS_PRECONDITION(endpoint);
source/credentials_provider_sso.c
Outdated
| } | ||
|
|
||
| struct sso_user_data *sso_user_data = user_data; | ||
| if (sso_user_data->status_code == AWS_OP_SUCCESS) { |
There was a problem hiding this comment.
trivial: we use AWS_OP_SUCCESS as return code, and AWS_ERROR_SUCCESS as error code.
And why you are check the status_code to be 0? To make sure it was not set before?
Oh, I see, the on_response_headers will be invoked more than once, but the on_response_header_block_done will be invoked only once, should we use that instead?
There was a problem hiding this comment.
Or, you can just get the status code from complete callback.
There was a problem hiding this comment.
Thanks, moved it to complete callback.
| /* Request headers. */ | ||
| AWS_STATIC_STRING_FROM_LITERAL(s_sso_token_header, "x-amz-sso_bearer_token"); | ||
| AWS_STATIC_STRING_FROM_LITERAL(s_sso_user_agent_header, "User-Agent"); | ||
| AWS_STATIC_STRING_FROM_LITERAL(s_sso_user_agent_header_value, "CRTAuthSSOCredentialsProvider"); |
There was a problem hiding this comment.
Do we have an agreement on the user_agent value?
There was a problem hiding this comment.
No, do we have to talk to someone?
There was a problem hiding this comment.
I am not sure, there is a SDK metrics team, which may need this info?
source/credentials_provider_sso.c
Outdated
| } | ||
| if (user_data->connection) { | ||
| struct aws_credentials_provider_sso_impl *provider_impl = user_data->provider->impl; | ||
| provider_impl->function_table->aws_http_connection_manager_release_connection( |
There was a problem hiding this comment.
this function can fail, maybe just assert on the return code.
source/credentials_provider_sso.c
Outdated
| /* | ||
| * If we can retry the request based on error response, retry it, otherwise, call the finalize function. | ||
| */ | ||
| if (user_data->error_code) { |
There was a problem hiding this comment.
should we also try to retry based on the status code?
| if (user_data->error_code) { | |
| if (user_data->error_code || user_data->status_code!=AWS_HTTP_STATUS_CODE_200_OK) { |
There was a problem hiding this comment.
We are already doing that as user_data->error_code will be set if http status code was not 200. I have changed it to your suggestion as it is more clear. thanks.
There was a problem hiding this comment.
Oh, I missed the part you set the error code based on status code. It's fine, it just couple line above, we can keep what you did before.
source/credentials_provider_sso.c
Outdated
| } | ||
| static void s_on_retry_ready(struct aws_retry_token *token, int error_code, void *wrapped_user_data); | ||
|
|
||
| static void s_on_stream_complete_fn(struct aws_http_stream *stream, int error_code, void *data) { |
There was a problem hiding this comment.
super trivial: I would prefer to have the callbacks listed as the order it get invoked, header->body->complete
There was a problem hiding this comment.
Most of the functions are listed in a bottom-to-top order fashion, which makes it easier to navigate the whole file from bottom to top. For example, the functions are listed as follows: acquireConnection -> tokenCallback -> send query -> incoming body -> stream complete -> finalize query.
source/credentials_provider_sso.c
Outdated
| AWS_LS_AUTH_CREDENTIALS_PROVIDER, | ||
| "(id=%p): failed to register operation success: %s", | ||
| (void *)sso_query_context->provider, | ||
| aws_error_str(aws_last_error())); |
There was a problem hiding this comment.
Well, if this function fails, we basically just log the error and everything else will keep moving as succeed.
I don't think this function can ever fail from the current impl, should we just change to assert on the return value?
Co-authored-by: Dengke Tang <dengket@amazon.com>
Co-authored-by: Dengke Tang <dengket@amazon.com>
| @@ -73,13 +73,15 @@ struct aws_auth_http_system_vtable { | |||
| enum aws_parse_credentials_expiration_format { | |||
| AWS_PCEF_STRING_ISO_8601_DATE, | |||
| AWS_PCEF_NUMBER_UNIX_EPOCH, | |||
| AWS_PCEF_NUMBER_UNIX_EPOCH_MS, | |||
There was a problem hiding this comment.
Seems like unix epoch just stands for sec from googling. So, it's fine.
Description of changes:
Todos:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.