From 23df552784c0aef6bd27191eb7f6d0564021b3d2 Mon Sep 17 00:00:00 2001 From: Michael Wallace Date: Fri, 14 Jul 2023 16:50:50 +0100 Subject: [PATCH 1/5] Add `ec2:CreateTags` to AWS snapshot permissions --- doc/manual/28-snapshots.en.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/manual/28-snapshots.en.md b/doc/manual/28-snapshots.en.md index 629f530c4..7211f2e22 100644 --- a/doc/manual/28-snapshots.en.md +++ b/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` From bf6d1eb31119592c532b689e107faf1e1990ddc3 Mon Sep 17 00:00:00 2001 From: Michael Wallace Date: Wed, 19 Jul 2023 10:09:47 +0100 Subject: [PATCH 2/5] Add unsupported cloud provider unit tests Adds tests for unsupported cloud providers when creating a CloudSnapshotInterface. These were originally covered by the `aws` provider but since this is now supported we need the additional test cases in this commit to cover the handling unsupported providers. --- tests/test_cloud_snapshot_interface.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_cloud_snapshot_interface.py b/tests/test_cloud_snapshot_interface.py index 89d129656..68d20cc9c 100644 --- a/tests/test_cloud_snapshot_interface.py +++ b/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") @@ -223,6 +225,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") From 470e738e0fa39a3459d5017d3fa49c2e72517845 Mon Sep 17 00:00:00 2001 From: Michael Wallace Date: Wed, 19 Jul 2023 10:34:21 +0100 Subject: [PATCH 3/5] Use region in backup_info when creating AwsCloudSnapshotInterface Uses the region set in the `snapshots_info/provider` section of the backup_info when creating an `AwsCloudSnapshotInterface` from a backup_info. This does not take precedence over the region specified in the config, so it is possible for the user to force a different region to be used at recovery or deletion time. --- barman/cloud_providers/__init__.py | 5 ++++ tests/test_cloud_snapshot_interface.py | 40 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/barman/cloud_providers/__init__.py b/barman/cloud_providers/__init__.py index 2a77bae3a..9e2279b52 100644 --- a/barman/cloud_providers/__init__.py +++ b/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( diff --git a/tests/test_cloud_snapshot_interface.py b/tests/test_cloud_snapshot_interface.py index 68d20cc9c..996cf5659 100644 --- a/tests/test_cloud_snapshot_interface.py +++ b/tests/test_cloud_snapshot_interface.py @@ -216,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"), [ From 799acf0fda59fc5836cb47ab9f7b574a07a783a7 Mon Sep 17 00:00:00 2001 From: Michael Wallace Date: Wed, 19 Jul 2023 10:40:10 +0100 Subject: [PATCH 4/5] Fix comment errors in unit tests Fixes a number of typos and inaccuracies in comments in the unit tests. --- tests/test_cloud_snapshot_interface.py | 8 ++++---- tests/test_executor.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_cloud_snapshot_interface.py b/tests/test_cloud_snapshot_interface.py index 996cf5659..7b23f8f0f 100644 --- a/tests/test_cloud_snapshot_interface.py +++ b/tests/test_cloud_snapshot_interface.py @@ -207,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 @@ -2094,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. @@ -2278,7 +2278,7 @@ 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"), @@ -2286,7 +2286,7 @@ class TestAzureVolumeMetadata(object): "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 diff --git a/tests/test_executor.py b/tests/test_executor.py index ee9f877e9..0f2833717 100644 --- a/tests/test_executor.py +++ b/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 From a012ccd60772c6f399878a864b55faf867685c2b Mon Sep 17 00:00:00 2001 From: Michael Wallace Date: Wed, 19 Jul 2023 10:56:10 +0100 Subject: [PATCH 5/5] Explain either IDs or names can be used for AWS resources Adds a note to the manual explaining that either IDs or names can be used when specifying instances or volumes in AWS. --- doc/manual/28-snapshots.en.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/manual/28-snapshots.en.md b/doc/manual/28-snapshots.en.md index 7211f2e22..493334f96 100644 --- a/doc/manual/28-snapshots.en.md +++ b/doc/manual/28-snapshots.en.md @@ -148,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