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

Require the AWS region to be specified if using AWS S3 for blob storage #1055

Merged
merged 6 commits into from
Jul 10, 2023

Conversation

timrogers
Copy link
Contributor

@timrogers timrogers commented Jul 6, 2023

A few months ago, we changed the CLI to allow an AWS region to be specified when using AWS S3 for blob storage in gh gei and gh bbs2gh.

We added support for specifying an AWS region because:

  • Without a region, we have to default to using AWS V2 signatures, and these do not work in all regions, causing migrations to fail with a confusing error message
  • You can't use AWS temporary credentials (i.e. a session token) without a region

For backwards compatibility, we made the region optional - unless you're using an AWS session token specified with --aws-session-token or AWS_SESSION_TOKEN, in which case the region is mandatory. If you don't specify it, we allow you to continue, but print a warning saying that providing a region is recommended and will soon be mandatory.

This updates the behaviour, completing our breaking change, making it mandatory to specify the session token.

  • Did you write/update appropriate tests
  • Release notes updated (if appropriate)
  • Appropriate logging output
  • Issue linked
  • Docs updated (or issue created)
  • New package licenses are added to ThirdPartyNotices.txt (if applicable)

src/Octoshift/Services/AwsApi.cs Show resolved Hide resolved
@@ -325,16 +325,9 @@ private void ValidateUploadOptions(MigrateRepoCommandArgs args)
throw new OctoshiftCliException("Either --aws-secret-key or AWS_SECRET_ACCESS_KEY environment variable must be set.");
}

if (GetAwsSessionToken(args).HasValue() && GetAwsRegion(args).IsNullOrWhiteSpace())
if (GetAwsRegion(args).IsNullOrWhiteSpace())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem like we should have to have this validation here because AwsApiFactory will validate that the region is set when it is initialized. But in the tests, there are some cases which skip that path and this validation gets run.

@timrogers
Copy link
Contributor Author

Fixes #857.

@timrogers timrogers force-pushed the timrogers/require-aws-region branch from 3a4cb82 to 17dd230 Compare July 6, 2023 16:48
@timrogers
Copy link
Contributor Author

The tests pass for me locally, but we get failures like this in CI:

[xUnit.net 00:00:03.13]     OctoshiftCLI.Tests.GithubEnterpriseImporter.Commands.MigrateRepo.MigrateRepoCommandHandlerTests.It_Uses_Aws_If_Arguments_Are_Included [FAIL]
  Failed OctoshiftCLI.Tests.GithubEnterpriseImporter.Commands.MigrateRepo.MigrateRepoCommandHandlerTests.It_Uses_Aws_If_Arguments_Are_Included [970 ms]
  Error Message:
   Amazon.Runtime.AmazonClientException : No RegionEndpoint or ServiceURL configured
  Stack Trace:
     at Amazon.Runtime.ClientConfig.Validate()
   at Amazon.S3.AmazonS3Config.Validate()
   at Amazon.Runtime.AmazonServiceClient..ctor(AWSCredentials credentials, ClientConfig config)
   at Amazon.S3.AmazonS3Client..ctor(AWSCredentials credentials, AmazonS3Config clientConfig)
   at OctoshiftCLI.Services.AwsApi.BuildAmazonS3Client(String awsAccessKeyId, String awsSecretAccessKey, String awsRegion, String awsSessionToken) in D:\a\gh-gei\gh-gei\src\Octoshift\Services\AwsApi.cs:line 43
   at OctoshiftCLI.Services.AwsApi..ctor(String awsAccessKeyId, String awsSecretAccessKey, String awsRegion, String awsSessionToken) in D:\a\gh-gei\gh-gei\src\Octoshift\Services\AwsApi.cs:line 21
   at Castle.Proxies.AwsApiProxy..ctor(IInterceptor[] , String awsAccessKeyId, String awsSecretAccessKey, String awsRegion, String awsSessionToken)

I'm not sure how to debug that.

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Unit Test Results

781 tests   781 ✔️  24s ⏱️
    1 suites      0 💤
    1 files        0

Results for commit 40ef791.

♻️ This comment has been updated with latest results.

@dylan-smith
Copy link
Collaborator

The tests pass for me locally, but we get failures like this in CI:

[xUnit.net 00:00:03.13]     OctoshiftCLI.Tests.GithubEnterpriseImporter.Commands.MigrateRepo.MigrateRepoCommandHandlerTests.It_Uses_Aws_If_Arguments_Are_Included [FAIL]
  Failed OctoshiftCLI.Tests.GithubEnterpriseImporter.Commands.MigrateRepo.MigrateRepoCommandHandlerTests.It_Uses_Aws_If_Arguments_Are_Included [970 ms]
  Error Message:
   Amazon.Runtime.AmazonClientException : No RegionEndpoint or ServiceURL configured
  Stack Trace:
     at Amazon.Runtime.ClientConfig.Validate()
   at Amazon.S3.AmazonS3Config.Validate()
   at Amazon.Runtime.AmazonServiceClient..ctor(AWSCredentials credentials, ClientConfig config)
   at Amazon.S3.AmazonS3Client..ctor(AWSCredentials credentials, AmazonS3Config clientConfig)
   at OctoshiftCLI.Services.AwsApi.BuildAmazonS3Client(String awsAccessKeyId, String awsSecretAccessKey, String awsRegion, String awsSessionToken) in D:\a\gh-gei\gh-gei\src\Octoshift\Services\AwsApi.cs:line 43
   at OctoshiftCLI.Services.AwsApi..ctor(String awsAccessKeyId, String awsSecretAccessKey, String awsRegion, String awsSessionToken) in D:\a\gh-gei\gh-gei\src\Octoshift\Services\AwsApi.cs:line 21
   at Castle.Proxies.AwsApiProxy..ctor(IInterceptor[] , String awsAccessKeyId, String awsSecretAccessKey, String awsRegion, String awsSessionToken)

I'm not sure how to debug that.

When I run it in a codespace I see the same failing tests. You might not be seeing it locally because you have some AWS env vars set that the CI doesn't?

image

@timrogers
Copy link
Contributor Author

That's a great point. I definitely do. I'll give it a go on a Codespace on Monday. Another reason why clean, separated environments are a good idea...!

@timrogers timrogers marked this pull request as ready for review July 9, 2023 19:10
@timrogers timrogers requested a review from a team July 10, 2023 13:40
@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
Octoshift 85% 75% 1205
bbs2gh 79% 75% 638
ado2gh 84% 82% 617
gei 80% 75% 520
Summary 83% (6532 / 7856) 77% (1526 / 1992) 2980

Copy link
Collaborator

@synthead synthead left a comment

Choose a reason for hiding this comment

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

LGTM!

@timrogers timrogers merged commit d5bcc2b into main Jul 10, 2023
@timrogers timrogers deleted the timrogers/require-aws-region branch July 10, 2023 18:54
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