Merge pull request #1055 from github/timrogers/require-aws-region

Require the AWS region to be specified if using AWS S3 for blob storage
github · Jul 10, 2023 · d5bcc2b · d5bcc2b
2 parents e455609 + 40ef791
commit d5bcc2b
Show file tree

Hide file tree

Showing 13 changed files with 26 additions and 48 deletions.
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -1,3 +1,4 @@
+- __BREAKING CHANGE:__ Require the AWS region to always be specified with the `--aws-region` argument or `AWS_REGION` environment variable if using AWS S3 for blob storage. Previously, this was optional (with a warning) if you weren't specifying an AWS session token.
 - __BREAKING CHANGE:__ Drop support for deprecated `AWS_ACCESS_KEY` and `AWS_SECRET_KEY` environment variables in `gh gei` and `gh bbs2gh`. The AWS S3 credentials can now only be configured using the industry-standard `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` variables or command line arguments.
 - __BREAKING CHANGE__: Require the Bitbucket Server URL, project key and repo to always be provided for `bbs2gh migrate-repo`, even if using the upload-and-migrate (`--archive-path`) or migrate-only (`--archive-url`) flows
 - Increase timeouts in archive uploads to AWS to prevent timeouts during large uploads
diff --git a/src/Octoshift/Services/AwsApi.cs b/src/Octoshift/Services/AwsApi.cs
@@ -13,27 +13,22 @@ namespace OctoshiftCLI.Services;
 public class AwsApi : IDisposable
 {
     private const int AUTHORIZATION_TIMEOUT_IN_HOURS = 48;
-    private static readonly RegionEndpoint DefaultRegionEndpoint = RegionEndpoint.USEast1;
 
     private readonly ITransferUtility _transferUtility;
 
+    public AwsApi(ITransferUtility transferUtility) => _transferUtility = transferUtility;
+
 #pragma warning disable CA2000
     public AwsApi(string awsAccessKeyId, string awsSecretAccessKey, string awsRegion = null, string awsSessionToken = null)
         : this(new TransferUtility(BuildAmazonS3Client(awsAccessKeyId, awsSecretAccessKey, awsRegion, awsSessionToken)))
 #pragma warning restore CA2000
     {
     }
 
-    internal AwsApi(ITransferUtility transferUtility) => _transferUtility = transferUtility;
-
     private static AmazonS3Client BuildAmazonS3Client(string awsAccessKeyId, string awsSecretAccessKey, string awsRegion, string awsSessionToken)
     {
-        var regionEndpoint = DefaultRegionEndpoint;
-        if (awsRegion.HasValue())
-        {
-            regionEndpoint = GetRegionEndpoint(awsRegion);
-            AWSConfigsS3.UseSignatureVersion4 = true;
-        }
+        var regionEndpoint = awsRegion.IsNullOrWhiteSpace() ? null : GetRegionEndpoint(awsRegion);
+        AWSConfigsS3.UseSignatureVersion4 = true;
 
         var creds = awsSessionToken.HasValue()
             ? (AWSCredentials)new SessionAWSCredentials(awsAccessKeyId, awsSecretAccessKey, awsSessionToken)

diff --git a/src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs b/src/OctoshiftCLI.IntegrationTests/BbsToGithub.cs
@@ -17,6 +17,7 @@ public sealed class BbsToGithub : IDisposable
 
     private const string SSH_KEY_FILE = "ssh_key.pem";
     private const string AWS_BUCKET_NAME = "github-dev";
+    private const string AWS_REGION = "us-east-1";
 
     private readonly ITestOutputHelper _output;
     private readonly OctoLogger _logger;
@@ -115,7 +116,7 @@ await retryPolicy.Retry(async () =>
         {
             _tokens.Add("AWS_ACCESS_KEY_ID", Environment.GetEnvironmentVariable("AWS_ACCESS_KEY_ID"));
             _tokens.Add("AWS_SECRET_ACCESS_KEY", Environment.GetEnvironmentVariable("AWS_SECRET_ACCESS_KEY"));
-            archiveUploadOptions = $" --aws-bucket-name {AWS_BUCKET_NAME}";
+            archiveUploadOptions = $" --aws-bucket-name {AWS_BUCKET_NAME} --aws-region {AWS_REGION}";
         }
 
         await _targetHelper.RunBbsCliMigration(

diff --git a/src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs b/src/OctoshiftCLI.Tests/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs
@@ -34,6 +34,7 @@ public class MigrateRepoCommandHandlerTests
         private const string AWS_ACCESS_KEY_ID = "aws-access-key-id";
         private const string AWS_SECRET_ACCESS_KEY = "aws-secret-access-key";
         private const string AWS_SESSION_TOKEN = "aws-session-token";
+        private const string AWS_REGION = "eu-west-1";
         private const string AZURE_STORAGE_CONNECTION_STRING = "azure-storage-connection-string";
 
         private const string BBS_HOST = "our-bbs-server.com";
@@ -214,6 +215,7 @@ public async Task Happy_Path_Generate_Archive_Ssh_Download_Aws_Upload_And_Ingest
                 AwsBucketName = AWS_BUCKET_NAME,
                 AwsAccessKey = AWS_ACCESS_KEY_ID,
                 AwsSecretKey = AWS_SECRET_ACCESS_KEY,
+                AwsRegion = AWS_REGION,
                 GithubOrg = GITHUB_ORG,
                 GithubRepo = GITHUB_REPO,
                 GithubPat = GITHUB_PAT,
@@ -672,6 +674,7 @@ public async Task Uses_Aws_If_Credentials_Are_Passed()
                 AwsAccessKey = AWS_ACCESS_KEY_ID,
                 AwsSecretKey = AWS_SECRET_ACCESS_KEY,
                 AwsBucketName = AWS_BUCKET_NAME,
+                AwsRegion = AWS_REGION,
                 QueueOnly = true,
             };
 
@@ -755,7 +758,7 @@ await _handler.Invoking(async x => await x.Handle(new MigrateRepoCommandArgs
         }
 
         [Fact]
-        public async Task It_Throws_When_Aws_Session_Token_Is_Provided_But_Aws_Region_Is_Not()
+        public async Task It_Throws_When_Aws_Bucket_Name_Is_Provided_But_No_Aws_Region()
         {
             await _handler.Invoking(async x => await x.Handle(new MigrateRepoCommandArgs
             {
@@ -769,7 +772,7 @@ await _handler.Invoking(async x => await x.Handle(new MigrateRepoCommandArgs
             }))
                 .Should()
                 .ThrowAsync<OctoshiftCliException>()
-                .WithMessage("*--aws-region*AWS_REGION*--aws-session-token*AWS_SESSION_TOKEN*");
+                .WithMessage("Either --aws-region or AWS_REGION environment variable must be set.");
         }
 
         [Fact]

diff --git a/src/OctoshiftCLI.Tests/gei/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs b/src/OctoshiftCLI.Tests/gei/Commands/MigrateRepo/MigrateRepoCommandHandlerTests.cs
@@ -1119,6 +1119,7 @@ public async Task It_Uses_Aws_If_Arguments_Are_Included()
             var awsAccessKeyId = "awsAccessKeyId";
             var awsSecretAccessKey = "awsSecretAccessKey";
             var awsBucketName = "awsBucketName";
+            var awsRegion = "eu-west-1";
             var archiveUrl = $"https://s3.amazonaws.com/{awsBucketName}/archive.tar";
 
             _mockTargetGithubApi.Setup(x => x.GetOrganizationId(TARGET_ORG).Result).Returns(githubOrgId);
@@ -1181,6 +1182,7 @@ public async Task It_Uses_Aws_If_Arguments_Are_Included()
                 AwsBucketName = awsBucketName,
                 AwsAccessKey = awsAccessKeyId,
                 AwsSecretKey = awsSecretAccessKey,
+                AwsRegion = awsRegion,
                 Wait = true
             };
 
@@ -1250,7 +1252,7 @@ await _handler.Invoking(async x => await x.Handle(new MigrateRepoCommandArgs
         }
 
         [Fact]
-        public async Task Ghes_When_Aws_Session_Token_Is_Provided_But_No_Aws_Region_Throws()
+        public async Task Ghes_When_Aws_Bucket_Name_Is_Provided_But_No_Aws_Region_Throws()
         {
             _mockGhesVersionChecker.Setup(m => m.AreBlobCredentialsRequired(GHES_API_URL)).ReturnsAsync(true);
 
@@ -1268,7 +1270,7 @@ await _handler.Invoking(async x => await x.Handle(new MigrateRepoCommandArgs
             }))
                 .Should()
                 .ThrowAsync<OctoshiftCliException>()
-                .WithMessage("*--aws-region*AWS_REGION*--aws-session-token*AWS_SESSION_TOKEN*");
+                .WithMessage("Either --aws-region or AWS_REGION environment variable must be set.");
         }
 
         [Fact]

diff --git a/src/bbs2gh/Commands/GenerateScript/GenerateScriptCommand.cs b/src/bbs2gh/Commands/GenerateScript/GenerateScriptCommand.cs
@@ -106,8 +106,7 @@ public GenerateScriptCommand() : base(
     public Option<string> AwsRegion { get; } = new(
         name: "--aws-region",
         description: "If using AWS, the AWS region. If not provided, it will be read from AWS_REGION environment variable. " +
-                     "Defaults to us-east-1 if neither the argument nor the environment variable is set. " +
-                     "In a future release, you will be required to set an AWS region if using AWS S3 as your blob storage provider.");
+                     "Required if using AWS.");
 
     public Option<bool> Verbose { get; } = new("--verbose");
 

diff --git a/src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommand.cs b/src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommand.cs
@@ -116,8 +116,7 @@ public MigrateRepoCommand() : base(
     public Option<string> AwsRegion { get; } = new(
         name: "--aws-region",
         description: "If using AWS, the AWS region. If not provided, it will be read from AWS_REGION environment variable. " +
-                     "Defaults to us-east-1 if neither the argument nor the environment variable is set. " +
-                     "In a future release, you will be required to set an AWS region if using AWS S3 as your blob storage provider.");
+                     "Required if using AWS.");
 
     public Option<string> GithubOrg { get; } = new("--github-org");
 

diff --git a/src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs b/src/bbs2gh/Commands/MigrateRepo/MigrateRepoCommandHandler.cs
@@ -244,9 +244,6 @@ private async Task ImportArchive(MigrateRepoCommandArgs args, string archiveUrl
 
     private string GetAwsRegion(MigrateRepoCommandArgs args) => args.AwsRegion.HasValue() ? args.AwsRegion : _environmentVariableProvider.AwsRegion(false);
 
-    private string GetAwsSessionToken(MigrateRepoCommandArgs args) =>
-        args.AwsSessionToken.HasValue() ? args.AwsSessionToken : _environmentVariableProvider.AwsSessionToken(false);
-
     private string GetAzureStorageConnectionString(MigrateRepoCommandArgs args) => args.AzureStorageConnectionString.HasValue()
         ? args.AzureStorageConnectionString
         : _environmentVariableProvider.AzureStorageConnectionString(false);
@@ -325,16 +322,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())
-            {
-                throw new OctoshiftCliException(
-                    "--aws-region or AWS_REGION environment variable must be provided with --aws-session-token or AWS_SESSION_TOKEN environment variable.");
-            }
-
-            if (!GetAwsRegion(args).HasValue())
+            if (GetAwsRegion(args).IsNullOrWhiteSpace())
             {
-                _log.LogWarning("Specifying an AWS region with the --aws-region argument or AWS_REGION environment variable is currently not required, " +
-                                "but will be required in a future release. Defaulting to us-east-1.");
+                throw new OctoshiftCliException("Either --aws-region or AWS_REGION environment variable must be set.");
             }
         }
     }

diff --git a/src/bbs2gh/Factories/AwsApiFactory.cs b/src/bbs2gh/Factories/AwsApiFactory.cs
@@ -16,7 +16,7 @@ public virtual AwsApi Create(string awsRegion = null, string awsAccessKeyId = nu
         awsAccessKeyId ??= _environmentVariableProvider.AwsAccessKeyId();
         awsSecretAccessKey ??= _environmentVariableProvider.AwsSecretAccessKey();
         awsSessionToken ??= _environmentVariableProvider.AwsSessionToken(false);
-        awsRegion ??= _environmentVariableProvider.AwsRegion(false);
+        awsRegion ??= _environmentVariableProvider.AwsRegion();
 
         return new AwsApi(awsAccessKeyId, awsSecretAccessKey, awsRegion, awsSessionToken);
     }

diff --git a/src/gei/Commands/GenerateScript/GenerateScriptCommand.cs b/src/gei/Commands/GenerateScript/GenerateScriptCommand.cs
@@ -85,8 +85,7 @@ public GenerateScriptCommand() : base(
         public Option<string> AwsRegion { get; } = new("--aws-region")
         {
             Description = "If using AWS, the AWS region. If not provided, it will be read from AWS_REGION environment variable. " +
-                          "Defaults to us-east-1 if neither the argument nor the environment variable is set. " +
-                          "In a future release, you will be required to set an AWS region if using AWS S3 as your blob storage provider."
+                          "Required if using AWS."
         };
 
         public Option<bool> Verbose { get; } = new("--verbose");

diff --git a/src/gei/Commands/MigrateRepo/MigrateRepoCommand.cs b/src/gei/Commands/MigrateRepo/MigrateRepoCommand.cs
@@ -97,8 +97,7 @@ public MigrateRepoCommand() : base(
         public Option<string> AwsRegion { get; } = new("--aws-region")
         {
             Description = "If using AWS, the AWS region. If not provided, it will be read from AWS_REGION environment variable. " +
-                          "Defaults to us-east-1 if neither the argument nor the environment variable is set. " +
-                          "In a future release, you will be required to set an AWS region if using AWS S3 as your blob storage provider."
+                          "Required if using AWS."
         };
         public Option<bool> NoSslVerify { get; } = new("--no-ssl-verify")
         {

diff --git a/src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs b/src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs
@@ -373,16 +373,9 @@ private void ValidateGHESOptions(MigrateRepoCommandArgs args, bool cloudCredenti
                 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())
             {
-                throw new OctoshiftCliException(
-                    "--aws-region or AWS_REGION environment variable must be provided with --aws-session-token or AWS_SESSION_TOKEN environment variable.");
-            }
-
-            if (!GetAwsRegion(args).HasValue())
-            {
-                _log.LogWarning("Specifying an AWS region with the --aws-region argument or AWS_REGION environment variable is currently not required, " +
-                                "but will be required in a future release. Defaulting to us-east-1.");
+                throw new OctoshiftCliException("Either --aws-region or AWS_REGION environment variable must be set.");
             }
         }
         else if (new[] { args.AwsAccessKey, args.AwsSecretKey, args.AwsSessionToken, args.AwsRegion }.Any(x => x.HasValue()))
@@ -397,9 +390,6 @@ private void ValidateGHESOptions(MigrateRepoCommandArgs args, bool cloudCredenti
 
     private string GetAwsRegion(MigrateRepoCommandArgs args) => args.AwsRegion.HasValue() ? args.AwsRegion : _environmentVariableProvider.AwsRegion(false);
 
-    private string GetAwsSessionToken(MigrateRepoCommandArgs args) =>
-        args.AwsSessionToken.HasValue() ? args.AwsSessionToken : _environmentVariableProvider.AwsSessionToken(false);
-
     private string GetAzureStorageConnectionString(MigrateRepoCommandArgs args) => args.AzureStorageConnectionString.HasValue()
         ? args.AzureStorageConnectionString
         : _environmentVariableProvider.AzureStorageConnectionString(false);

diff --git a/src/gei/Factories/AwsApiFactory.cs b/src/gei/Factories/AwsApiFactory.cs
@@ -16,7 +16,7 @@ public virtual AwsApi Create(string awsRegion = null, string awsAccessKeyId = nu
         awsAccessKeyId ??= _environmentVariableProvider.AwsAccessKeyId();
         awsSecretAccessKey ??= _environmentVariableProvider.AwsSecretAccessKey();
         awsSessionToken ??= _environmentVariableProvider.AwsSessionToken(false);
-        awsRegion ??= _environmentVariableProvider.AwsRegion(false);
+        awsRegion ??= _environmentVariableProvider.AwsRegion();
 
         return new AwsApi(awsAccessKeyId, awsSecretAccessKey, awsRegion, awsSessionToken);
     }