CSHARP-3740: Add native support for AWS IAM Roles for service accounts, EKS in particular. #947
Conversation
…s, EKS in particular.
DmitryLukyanov
force-pushed
the
csharp3740_3
branch
from
a60019f to
8d11d50
Compare
| exit 0 | ||
| fi | ||
| PROJECT_DIRECTORY=${PROJECT_DIRECTORY} OS=$OS ASSERT_NO_URI_CREDS=true evergreen/run-mongodb-aws-test.sh | ||
|
|
There was a problem hiding this comment.
Why are we running run-mongodb-aws-test.sh twice, once without AWS_ROLE_SESSION_NAME set and once with it set? If we are testing two different scenarios, we should probably separate this into two separate functions. Otherwise if one fails, it will not be immediately obvious which one. We would have to check whether AWS_ROLE_SESSION_NAME is set or not to understand which test case failed. Thoughts?
There was a problem hiding this comment.
There was a problem hiding this comment.
into two separate functions.
both of these steps require the same precondition in the first step, so I would say it's not too wrong running these ones in the same scope.
it will not be immediately obvious which one
given that it's almost the same scenarios, I would not expect failing them differently one from another. But even if so, EG provides a way to know what particular step was failed. For example pay attention on (step 9.2.. here
| @@ -9,7 +9,7 @@ set -o errexit # Exit the script with error if any of the commands fail | |||
| # | |||
| # Environment variables used as output: | |||
| # AWS_TESTS_ENABLED Allows runnings AWS tests | |||
| # AWS_ECS_TEST Allows runnings EVS tests | |||
| # AWS_ECS_ENABLED Allows runnings ECS tests | |||
There was a problem hiding this comment.
Allows runnings => Allows running
| _awsHttpClientHelper = new AwsHttpClientHelper(httpClientWrapper); | ||
| _environmentVariableProvider = Ensure.IsNotNull(environmentVariableProvider, nameof(environmentVariableProvider)); | ||
| var creds = FallbackCredentialsFactory.GetCredentials(); | ||
| var immutableCredentias = creds.GetCredentials(); |
There was a problem hiding this comment.
immutableCredentias => immutableCredentials
| return false; | ||
| } | ||
| var creds = FallbackCredentialsFactory.GetCredentials(); | ||
| var immutableCredentias = await creds.GetCredentialsAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
immutableCredentias => immutableCredentials
| using System.Security; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Amazon.Runtime; |
There was a problem hiding this comment.
Shouldn't switching to use the AWS SDK be done in the context of https://jira.mongodb.org/browse/CSHARP-4390?
There was a problem hiding this comment.
If the AWS SDK is responsible for caching, do we need any of this caching logic that was introduced in https://jira.mongodb.org/browse/CSHARP-4273?
There was a problem hiding this comment.
Shouldn't switching to use the AWS SDK be done in the context of https://jira.mongodb.org/browse/CSHARP-4390?
Better to do it now, otherwise we will need implementing custom aws-web-identity-credentials functionality and then simply remove it in the next ticket. I consider the scope of CSHARP-4390 like replacement this logic on this (I already tried to include that logic here, but with no success yet, so I defer it until 4390)
There was a problem hiding this comment.
If the AWS SDK is responsible for caching, do we need any of this caching logic that was introduced in https://jira.mongodb.org/browse/CSHARP-4273?
yes. it's used in azure kms
There was a problem hiding this comment.
Good point about Azure KMS.
While I'm not enthusiastic about splitting the CSHARP-4390 work between these two tickets, I understand your reasoning. It doesn't make sense to implement something in this ticket to only revert it in the next. Accepted.
| { | ||
| return null; | ||
| // returns cached credentials source immediately. Only if cached source unavailable, makes quite heavy steps |
There was a problem hiding this comment.
See https://github.com/aws/aws-sdk-net/blob/master/sdk/src/Core/Amazon.Runtime/Credentials/FallbackCredentialsFactory.cs#L127.
A full lookup for sources happens only first time and after failure. It's still not ideal, but maybe we can apply current solution now and create a ticket to track possible improvement (including submitting a PR to their repo)?
…s, EKS in particular. (mongodb#947)
Asked a question about possible thead safe issue: aws/aws-sdk-net#2487