Merge pull request #825 from EnterpriseDB/BAR-29-minor-aws-snapshot-f…

…ixes

BAR-29: Minor AWS snapshot fixes

barman/cloud_providers/__init__.py

@@ -307,11 +307,16 @@ def get_snapshot_interface_from_backup_info(backup_info, config=None):
     elif backup_info.snapshots_info.provider == "aws":
         from barman.cloud_providers.aws_s3 import AwsCloudSnapshotInterface
 
+        # When creating a snapshot interface for existing backups we use the region
+        # from the backup_info, unless a region is set in the config in which case the
+        # config region takes precedence.
         region = None
         profile = None
         if config is not None and hasattr(config, "aws_region"):
             region = config.aws_region
             profile = config.aws_profile
+        if region is None:
+            region = backup_info.snapshots_info.region
         return AwsCloudSnapshotInterface(profile, region)
     else:
         raise CloudProviderUnsupported(

doc/manual/28-snapshots.en.md

@@ -92,6 +92,7 @@ The following additional prerequisites apply to snapshot backups on AWS:
 The following permissions are required:
 
 - `ec2:CreateSnapshot`
+- `ec2:CreateTags`
 - `ec2:DeleteSnapshot`
 - `ec2:DescribeSnapshots`
 - `ec2:DescribeInstances`
@@ -147,6 +148,10 @@ azure_resource_group = AZURE_RESOURCE_GROUP
 
 #### Configuration for AWS snapshots
 
+When specifying `snapshot_instance` or `snapshot_disks`, Barman will accept either the instance/volume ID which was assigned to the resource by AWS *or* a name.
+If a name is used then Barman will query AWS to find resources with a matching `Name` tag.
+If zero or multiple matching resources are found then Barman will exit with an error.
+
 The following optional parameters can be set when using AWS:
 
 ``` ini

tests/test_cloud_snapshot_interface.py

@@ -61,6 +61,7 @@ class TestGetSnapshotInterface(object):
             ("aws", AwsCloudSnapshotInterface),
             ("azure", AzureCloudSnapshotInterface),
             ("gcp", GcpCloudSnapshotInterface),
+            ("unsupportedcloud", None),
         ],
     )
     @mock.patch("barman.cloud_providers._get_azure_credential")
@@ -136,6 +137,7 @@ def test_from_config_azure_no_subscription_id(self):
             ("aws", AwsCloudSnapshotInterface),
             ("azure", AzureCloudSnapshotInterface),
             ("gcp", GcpCloudSnapshotInterface),
+            ("unsupportedcloud", None),
         ],
     )
     @mock.patch("barman.cloud_providers.aws_s3.boto3")
@@ -205,7 +207,7 @@ def test_from_backup_info_azure_no_subscription_id(self):
         mock_backup_info = mock.Mock(
             snapshots_info=mock.Mock(provider="azure", subscription_id=None)
         )
-        # WHEN get snapshot_interface_from_server_config is called
+        # WHEN get snapshot_interface_from_backup_info is called
         with pytest.raises(ConfigurationException) as exc:
             get_snapshot_interface_from_backup_info(mock_backup_info, mock_config)
         # THEN the expected exception is raised
@@ -214,6 +216,46 @@ def test_from_backup_info_azure_no_subscription_id(self):
             in str(exc.value)
         )
 
+    @pytest.mark.parametrize(
+        ("config_region", "backup_info_region", "expected_region"),
+        (
+            # If neither config nor backup_info have a region we expect None
+            (None, None, None),
+            # If the config has a region but backup_info does not then we expect the
+            # region in the config
+            ("config-region", None, "config-region"),
+            # If the backup_info has a region but the config does not then we expect
+            # the region in the backup_info
+            (None, "backup-info-region", "backup-info-region"),
+            # If both config and backup_info have a region we expect the region in the
+            # config
+            ("config-region", "backup-info-region", "config-region"),
+        ),
+    )
+    @mock.patch("barman.cloud_providers.aws_s3.boto3")
+    def test_from_backup_info_aws_region(
+        self, mock_boto3, config_region, backup_info_region, expected_region
+    ):
+        """
+        Verify that the region is taken from the backup_info but can be overridden by
+        the config.
+        """
+        # GIVEN a server config with the aws snapshot provider and the specified region
+        mock_config = mock.Mock(snapshot_provider="aws", aws_region=config_region)
+        # AND a backup info with the specified region
+        mock_backup_info = mock.Mock(
+            snapshots_info=mock.Mock(provider="aws", region=backup_info_region)
+        )
+        # AND the session has no default region name
+        mock_boto3.Session.return_value.region_name = None
+
+        # WHEN get snapshot_interface_from_backup_info is called
+        snapshot_interface = get_snapshot_interface_from_backup_info(
+            mock_backup_info, mock_config
+        )
+        # THEN the snapshot interface has the expected region
+        assert snapshot_interface.region == expected_region
+
     @pytest.mark.parametrize(
         ("cloud_provider", "interface_cls"),
         [
@@ -223,6 +265,7 @@ def test_from_backup_info_azure_no_subscription_id(self):
                 AzureCloudSnapshotInterface,
             ),
             ("google-cloud-storage", GcpCloudSnapshotInterface),
+            ("unsupportedcloud", None),
         ],
     )
     @mock.patch("barman.cloud_providers._get_azure_credential")
@@ -2051,7 +2094,7 @@ def test_get_attached_volumes_disk_not_found(self):
         expected_volumes = [d["name"] for d in disks[:-1]]
         assert set(attached_volumes.keys()) == set(expected_volumes)
 
-    def test_get_attached_volumes_disk_not_attaached(self):
+    def test_get_attached_volumes_disk_not_attached(self):
         """
         Verify that a SnapshotBackupException is raised if a disk is not attached
         to the instance.
@@ -2235,15 +2278,15 @@ class TestAzureVolumeMetadata(object):
             # If no attachment metadata or disk metadata is passed then we expect init
             # to succeed but the lun and location to be None.
             (None, None, None, None),
-            # If the lun is set in the attachment metadata then we eexpect it to be
+            # If the lun is set in the attachment metadata then we expect it to be
             # set in the VolumeMetadata instance.
             (
                 mock.Mock(lun="10"),
                 None,
                 "10",
                 None,
             ),
-            # If the location is set in the attachment metadata then we eexpect it to
+            # If the location is set in the attachment metadata then we expect it to
             # be set in the VolumeMetadata instance.
             (None, mock.Mock(location="uksouth"), None, "uksouth"),
             # If lun and location are set in the attachment metadata then we expect

tests/test_executor.py

@@ -1712,7 +1712,7 @@ def mock_resolve_mounted_volume(mock_volume, disk_name, _cmd):
                 mock_resolve_mounted_volume, mock_volume, disk
             )
 
-        # WHEN find_missing_and_unmounted_disks is called for all expected disksts
+        # WHEN find_missing_and_unmounted_disks is called for all expected disks
         instance_name = "instance1"
         snapshot_disks = (
             expected_missing_disks + expected_unmounted_disks + expected_mounted_disks