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

Add support for AWS EBS Snapshot Lock mode #1005

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions barman/clients/cloud_backup.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
UnrecoverableHookScriptError,
)
from barman.postgres import PostgreSQLConnection
from barman.utils import check_backup_name, check_positive, check_size, force_str
from barman.utils import check_aws_snapshot_lock_duration_range, check_aws_snapshot_lock_cool_off_period_range, check_backup_name, check_positive, check_size, check_timestamp, force_str

_find_space = re.compile(r"[\s]").search

Expand Down Expand Up @@ -419,6 +419,26 @@ def parse_arguments(args=None):
"timing out (default: 3600 seconds)",
type=check_positive,
)
s3_arguments.add_argument(
"--aws-snapshot-lock-mode",
help="The lock mode to apply to the snapshot. Allowed values: 'governance'|'compliance'.",
choices=["governance", "compliance"],
)
s3_arguments.add_argument(
"--aws-snapshot-lock-duration",
help="The duration (in days) for which the snapshot should be locked. Must be between 1 and 36500. To lock a snapshopt, you must specify either this argument or --aws-snapshot-lock-expiration-date, but not both.",
type=check_aws_snapshot_lock_duration_range,
)
s3_arguments.add_argument(
"--aws-snapshot-lock-cool-off-period",
help="Specifies the cool-off period (in hours) for a snapshot locked in 'compliance' mode, allowing you to unlock or modify lock settings after it is locked. Must be between 1 and 72 hours. To lock the snapshot immediately without a cool-off period, leave this option unset.",
type=check_aws_snapshot_lock_cool_off_period_range,
)
s3_arguments.add_argument(
"--aws-snapshot-lock-expiration-date",
help="The expiration date for a locked snapshot in the format YYYY-MM-DDThh:mm:ss.sssZ. To lock a snapshot, you must specify either this argument or --aws-snapshot-lock-duration, but not both.",
type=check_timestamp
)
azure_arguments.add_argument(
"--encryption-scope",
help="The name of an encryption scope defined in the Azure Blob Storage "
Expand All @@ -434,7 +454,14 @@ def parse_arguments(args=None):
help="The name of the Azure resource group to which the compute instance and "
"disks defined by the --snapshot-instance and --snapshot-disk arguments belong.",
)
return parser.parse_args(args=args)

parsed_args = parser.parse_args(args=args)

# Perform mutual exclusivity check
if parsed_args.aws_snapshot_lock_duration is not None and parsed_args.aws_snapshot_lock_expiration_date is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Move and refactor this in the _validate_config function in this module.

Copy link
Contributor

Choose a reason for hiding this comment

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

As this is only about mutually exclusiveness, we can also leverage of argparse to handle that for us. Something like this:

    s3_lock_target_group = s3_arguments.add_mutually_exclusive_group()
    s3_lock_target_group.add_argument(
        "--aws-snapshot-lock-duration",
        help="The duration (in days) for which the snapshot should be locked. Must be between 1 and 36500. To lock a snapshopt, you must specify either this argument or --aws-snapshot-lock-expiration-date, but not both.",
        type=check_aws_snapshot_lock_duration_range,
    )
    s3_lock_target_group.add_argument(
        "--aws-snapshot-lock-expiration-date",
        help="The expiration date for a locked snapshot in the format YYYY-MM-DDThh:mm:ss.sssZ. To lock a snapshot, you must specify either this argument or --aws-snapshot-lock-duration, but not both.",
        type=check_timestamp
    )

If you attempt to specify both, argparse would throw an error:

barman-cloud-backup: error: argument --aws-snapshot-lock-expiration-date: not allowed with argument --aws-snapshot-lock-duration

parser.error("You must specify either --aws-snapshot-lock-duration or --aws-snapshot-lock-expiration-date, but not both.")

return parsed_args


if __name__ == "__main__":
Expand Down
8 changes: 8 additions & 0 deletions barman/cloud_providers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ def get_snapshot_interface(config):
config.aws_profile,
config.aws_region,
config.aws_await_snapshots_timeout,
config.aws_snapshot_lock_mode,
config.aws_snapshot_lock_duration,
config.aws_snapshot_lock_cool_off_period,
config.aws_snapshot_lock_expiration_date,
]
return AwsCloudSnapshotInterface(*args)
else:
Expand Down Expand Up @@ -253,6 +257,10 @@ def get_snapshot_interface_from_server_config(server_config):
server_config.aws_profile,
server_config.aws_region,
server_config.aws_await_snapshots_timeout,
server_config.aws_snapshot_lock_mode,
server_config.aws_snapshot_lock_duration,
server_config.aws_snapshot_lock_cool_off_period,
server_config.aws_snapshot_lock_expiration_date,
)
else:
raise CloudProviderUnsupported(
Expand Down
84 changes: 81 additions & 3 deletions barman/cloud_providers/aws_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -463,21 +463,41 @@ class AwsCloudSnapshotInterface(CloudSnapshotInterface):
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-creating-snapshot.html
"""

def __init__(self, profile_name=None, region=None, await_snapshots_timeout=3600):
def __init__(
self,
profile_name=None,
region=None,
await_snapshots_timeout=3600,
lock_mode=None,
lock_duration=None,
lock_cool_off_period=None,
lock_expiration_date=None
):
"""
Creates the client necessary for creating and managing snapshots.
:param str profile_name: AWS auth profile identifier.
:param str region: The AWS region in which snapshot resources are located.
:param int await_snapshots_timeout: The maximum time in seconds to wait for
snapshots to complete.
:param str lock_mode: The lock mode to apply to the snapshot.
:param int lock_duration: The duration (in days) for which the snapshot
should be locked.
:param int lock_cool_off_period: The cool-off period (in hours) for the snapshot.
:param str lock_expiration_date: The expiration date for the snapshot in the format
YYYY-MM-DDThh:mm:ss.sssZ.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
YYYY-MM-DDThh:mm:ss.sssZ.
``YYYY-MM-DDThh:mm:ss.sssZ``.

Sphinx recommendation for literals.

"""

self.session = boto3.Session(profile_name=profile_name)
# If a specific region was provided then this overrides any region which may be
# defined in the profile
self.region = region or self.session.region_name
self.ec2_client = self.session.client("ec2", region_name=self.region)
self.await_snapshots_timeout = await_snapshots_timeout
self.lock_mode = lock_mode
self.lock_duration = lock_duration
self.lock_cool_off_period = lock_cool_off_period
self.lock_expiration_date = lock_expiration_date

def _get_waiter_config(self):
delay = 15
Expand Down Expand Up @@ -761,10 +781,12 @@ def take_snapshot_backup(self, backup_info, instance_identifier, volumes):
snapshot_name, snapshot_resp = self._create_snapshot(
backup_info, volume_identifier, volume_metadata.id
)

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

snapshots.append(
AwsSnapshotMetadata(
snapshot_id=snapshot_resp["SnapshotId"],
snapshot_name=snapshot_name,
snapshot_lock_mode=self.lock_mode,
device_name=attached_volumes[0]["DeviceName"],
mount_options=volume_metadata.mount_options,
mount_point=volume_metadata.mount_point,
Expand All @@ -783,14 +805,57 @@ def take_snapshot_backup(self, backup_info, instance_identifier, volumes):
WaiterConfig=self._get_waiter_config(),
)

# Apply lock on snapshots if lock mode is specified
if self.lock_mode:
self._lock_snapshots(
snapshots,
self.lock_mode,
self.lock_duration,
self.lock_cool_off_period,
self.lock_expiration_date
)

backup_info.snapshots_info = AwsSnapshotsInfo(
snapshots=snapshots,
region=self.region,
# All snapshots will have the same OwnerId so we get it from the last
# snapshot response.
account_id=snapshot_resp["OwnerId"],

)

def _lock_snapshots(self, snapshots, lock_mode, lock_duration, lock_cool_off_period, lock_expiration_date):
Copy link
Contributor

@andremagui andremagui Oct 22, 2024

Choose a reason for hiding this comment

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

This should be revisited and refactored.

  • This should lock just one snapshot.
  • The values should not be checked but passed to the the client method call. The checks will be done higher in the code flow, so they will come to this part as they should be. Also, pay attention that if this fails somehow, the backup will also fail. The snapshot will be created though.
  • The response of the request to lock the snapshot has all the information you need to output information. Do not manipulate yourself the values in the response object, because if you use LockDuration OR LockExpiresOn, each one has a different response. If the field is not present in the response object, the lock is successful but the backup fails.
  • Use output.info() instead of logging.info(). This can be imported from barman import output.

To summarize, try to use this method to lock one snapshot, pass all the arguments, lock the snapshot and output information dynamically from the response object. In line 768, where we have for volume_identifier, volume_metadata in volumes.items():, you should lock snapshot as they are created.

Also, you should add a mutually exclusive test in the beginning of the backup process for fields that cannot be passed together, like duration and expiration_date OR governance mode with cool_off period. You did it for the client, but its not being checked when using this with a barman server


I can give you two examples of response object that i have from my tests:

LockDuration

{'SnapshotId': 'snap-00adc6c28f422e98c', 'LockState': 'governance', 'LockDuration': 1, 'LockCreatedOn': datetime.datetime(2024, 10, 22, 17, 0, 49, 253000, tzinfo=tzlocal()), 'LockExpiresOn': datetime.datetime(2024, 10, 23, 17, 0, 49, 253000, tzinfo=tzlocal()), 'LockDurationStartTime': datetime.datetime(2024, 10, 22, 17, 0, 49, 253000, tzinfo=tzlocal()), 'ResponseMetadata': {'RequestId': '3f8c3e22-d1b2b2da0', 'HTTPStatusCode': 200, 'HTTPHeaders': {'x-amzn-requestid': '3f8c3e22-d1b2-b2da0', 'cache-control': 'no-cache, no-store', 'strict-transport-security': 'max-age=31536000; includeSubDomains', 'vary': 'accept-encoding', 'content-type': 'text/xml;charset=UTF-8', 'transfer-encoding': 'chunked', 'date': 'Tue, 22 Oct 2024 17:00:49 GMT', 'server': 'AmazonEC2'}, 'RetryAttempts': 0}}

LockExpiresOn

{'SnapshotId': 'snap-0d83456985415270c', 'LockState': 'compliance-cooloff', 'CoolOffPeriod': 3, 'CoolOffPeriodExpiresOn': datetime.datetime(2024, 10, 22, 19, 55, 3, 461000, tzinfo=tzlocal()), 'LockCreatedOn': datetime.datetime(2024, 10, 22, 16, 55, 3, 461000, tzinfo=tzlocal()), 'LockExpiresOn': datetime.datetime(2024, 10, 24, 15, 53, 35, 457000, tzinfo=tzlocal()), 'LockDurationStartTime': datetime.datetime(2024, 10, 22, 16, 55, 3, 461000, tzinfo=tzlocal()), 'ResponseMetadata': {'RequestId': 'bac96fa3-008405f8d79', 'HTTPStatusCode': 200, 'HTTPHeaders': {'x-amzn-requestid': 'bac969e2e-6808405f8d79', 'cache-control': 'no-cache, no-store', 'strict-transport-security': 'max-age=31536000; includeSubDomains', 'vary': 'accept-encoding', 'content-type': 'text/xml;charset=UTF-8', 'transfer-encoding': 'chunked', 'date': 'Tue, 22 Oct 2024 16:55:02 GMT', 'server': 'AmazonEC2'}, 'RetryAttempts': 0}}

lock_snapshot_default_args = {
"LockMode": lock_mode
}

if lock_duration:
lock_snapshot_default_args["LockDuration"] = lock_duration

if lock_cool_off_period:
lock_snapshot_default_args["CoolOffPeriod"] = lock_cool_off_period

if lock_expiration_date:
lock_snapshot_default_args["ExpirationDate"] = lock_expiration_date

for snapshot in snapshots:
lock_snapshot_args = lock_snapshot_default_args.copy()
lock_snapshot_args["SnapshotId"] = snapshot.identifier

resp = self.ec2_client.lock_snapshot(**lock_snapshot_args)

logging.info(
"Snapshot %s locked in state '%s' (lock duration: %s days, cool-off period: %s hours, "
"cool-off period expires on: %s, lock expires on: %s, lock duration time: %s)",
snapshot.identifier,
resp["LockState"],
resp["LockDuration"],
resp["CoolOffPeriod"],
resp["CoolOffPeriodExpiresOn"],
resp["LockExpiresOn"],
resp["LockDurationStartTime"]
)
Comment on lines +847 to +857
Copy link
Contributor

Choose a reason for hiding this comment

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

In accordance with what @andremagui said, I think this is the biggest problem here. It seems the response from AWS is not always the same depending on the request parameters you pass, so you should ideally not rely on their response for logging.

E.g. if i run barman-cloud-backup with --aws-snapshot-lock-mode governance --aws-snapshot-lock-duration 1 i get:

2024-10-23 14:14:14,636 [3122] ERROR: Backup failed uploading data ('CoolOffPeriod')

Because CoolOffPeriod is not present in the response:

{'SnapshotId': 'snap-0a9e1d413ef5a9254', 'LockState': 'governance', 'LockDuration': 1, 'LockCreatedOn': datetime.datetime(2024, 10, 23, 14, 14, 14, 594000, tzinfo=tzlocal()), 'LockExpiresOn': datetime.datetime(2024, 10, 24, 14, 14, 14, 594000, tzinfo=tzlocal()), 'LockDurationStartTime': datetime.datetime(2024, 10, 23, 14, 14, 14, 594000, tzinfo=tzlocal()), 'ResponseMetadata': {'RequestId': '91c97b91-9dde-4d38-a652-0607ff9574f8', 'HTTPStatusCode': 200, 'HTTPHeaders': {'x-amzn-requestid': '91c97b91-9dde-4d38-a652-0607ff9574f8', 'cache-control': 'no-cache, no-store', 'strict-transport-security': 'max-age=31536000; includeSubDomains', 'vary': 'accept-encoding', 'content-type': 'text/xml;charset=UTF-8', 'transfer-encoding': 'chunked', 'date': 'Wed, 23 Oct 2024 14:14:14 GMT', 'server': 'AmazonEC2'}, 'RetryAttempts': 0}}

The snapshot itself was created and locked but the process errored out and the backup is not completed successfully in Barman.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to use a formatted message, we should at least use resp.get to return default values when the keys are not present.


def _delete_snapshot(self, snapshot_id):
"""
Delete the specified snapshot.
Expand Down Expand Up @@ -824,6 +889,16 @@ def delete_snapshot_backup(self, backup_info):
snapshot.identifier,
backup_info.backup_id,
)

if snapshot.snapshot_lock_mode is not None:
resp = self.ec2_client.describe_locked_snapshots(
SnapshotIds=[snapshot.identifier],
)

if resp["Snapshots"] and resp["Snapshots"][0]["LockState"] != "expired":
logging.warning("Skipping deletion of snapshot %s as it not expired yet", snapshot.identifier)
continue

Comment on lines +893 to +901
Copy link
Contributor

Choose a reason for hiding this comment

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

This has do be done earlier in the code. By doing it here you are only avoiding the deletion of the snapshot itself, but the backup metadata in the s3 bucket is still deleted. This would make Barman lose track of the backup while the snapshot itself will still exist.

E.g.

  1. I have a backup 20241023T135554 in my catalog:
$ barman-cloud-backup-list s3://barman-testing-snapshot-tags pg-server
Backup ID           End Time                 Begin Wal                     Archival Status  Name                                                  
20241023T135554     2024-10-23 13:56:10      000000010000000000000024
  1. I try to delete it and receive your message saying it was skipped.
$ barman-cloud-backup-delete s3://barman-testing-snapshot-tags pg-server -b 20241023T135554
2024-10-23 13:57:17,634 [2735] WARNING: Skipping deletion of snapshot snap-03c42da5c8a2f2ce7 as it not expired yet
  1. However, its metadata was deleted from s3, so Barman is no longer aware of it.
$ barman-cloud-backup-list s3://barman-testing-snapshot-tags pg-server
Backup ID           End Time                 Begin Wal                     Archival Status  Name 
$ barman-cloud-backup-delete s3://barman-testing-snapshot-tags pg-server -b 20241023T135554
2024-10-23 13:57:34,720 [2736] WARNING: Backup 20241023T135554 does not exist

I believe you would have to handle this in cloud_backup_delete.py in _delete_backup. Also in backup.py in delete_backup_data for when running it through barman standard server instead of cloud-barman-backup-delete script.

self._delete_snapshot(snapshot.identifier)

def get_attached_volumes(
Expand Down Expand Up @@ -1003,11 +1078,11 @@ class AwsSnapshotMetadata(SnapshotMetadata):
"""
Specialization of SnapshotMetadata for AWS EBS snapshots.
Stores the device_name, snapshot_id and snapshot_name in the provider-specific
Stores the device_name, snapshot_id, snapshot_name and snapshot_lock_mode in the provider-specific
field.
"""

_provider_fields = ("device_name", "snapshot_id", "snapshot_name")
_provider_fields = ("device_name", "snapshot_id", "snapshot_name", "snapshot_lock_mode")

def __init__(
self,
Expand All @@ -1016,6 +1091,7 @@ def __init__(
device_name=None,
snapshot_id=None,
snapshot_name=None,
snapshot_lock_mode=None,
):
"""
Constructor saves additional metadata for AWS snapshots.
Expand All @@ -1027,12 +1103,14 @@ def __init__(
:param str device_name: The device name used in the AWS API.
:param str snapshot_id: The snapshot ID used in the AWS API.
:param str snapshot_name: The snapshot name stored in the `Name` tag.
:param str snapshot_lock_mode: The mode with which the snapshot has been locked, if set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:param str snapshot_lock_mode: The mode with which the snapshot has been locked, if set.
:param str snapshot_lock_mode: The mode with which the snapshot has been locked (``governance`` or ``compliance``), if set.

:param str project: The AWS project name.
"""
super(AwsSnapshotMetadata, self).__init__(mount_options, mount_point)
self.device_name = device_name
self.snapshot_id = snapshot_id
self.snapshot_name = snapshot_name
self.snapshot_lock_mode = snapshot_lock_mode

@property
def identifier(self):
Expand Down
22 changes: 22 additions & 0 deletions barman/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,17 @@ def parse_time_interval(value):
return time_delta


def parse_datetime(value):
"""
Parse a string, transforming it in a datetime object.
Accepted format: YYYY-MM-DDThh:mm:ss.sssZ
:param str value: the string to evaluate
Comment on lines +237 to +240
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Parse a string, transforming it in a datetime object.
Accepted format: YYYY-MM-DDThh:mm:ss.sssZ
:param str value: the string to evaluate
Parse a string, transforming it in a datetime object.
Accepted format: ``YYYY-MM-DDThh:mm:ss.sssZ``
:param str value: the string to evaluate
:return: *value* parsed as :class:`datetime.datetime`.

"""

return datetime.datetime.strptime(value, "%Y-%m-%dT%H:%M:%S.%fZ")
Comment on lines +235 to +243
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little confusing in the sense that it is parsing a string to datetime but it is constraining it to a specific format, which is in fact just for AWS in this case.

Suggested change
def parse_datetime(value):
"""
Parse a string, transforming it in a datetime object.
Accepted format: YYYY-MM-DDThh:mm:ss.sssZ
:param str value: the string to evaluate
"""
return datetime.datetime.strptime(value, "%Y-%m-%dT%H:%M:%S.%fZ")
def parse_aws_datetime(value):
"""
Parse a string, transforming it in a datetime object.
Accepted format: YYYY-MM-DDThh:mm:ss.sssZ
:param str value: the string to evaluate
"""
return check_aws_expiration_date_format

Copy link
Contributor

Choose a reason for hiding this comment

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

Any ideas here @gustabowill? Maybe in the future we could re-think this if we have other formats.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep it as it is for now as this date format is very common.

I think you mistyped the return statement of your suggestion @andremagui.



def parse_si_suffix(value):
"""
Parse a string, transforming it into integer and multiplying by
Expand Down Expand Up @@ -489,6 +500,10 @@ class ServerConfig(BaseConfig):
"archiver_batch_size",
"autogenerate_manifest",
"aws_await_snapshots_timeout",
"aws_snapshot_lock_mode",
"aws_snapshot_lock_duration",
"aws_snapshot_lock_cool_off_period",
"aws_snapshot_lock_expiration_date",
"aws_profile",
"aws_region",
"azure_credential",
Expand Down Expand Up @@ -587,6 +602,10 @@ class ServerConfig(BaseConfig):
"archiver_batch_size",
"autogenerate_manifest",
"aws_await_snapshots_timeout",
"aws_snapshot_lock_mode",
"aws_snapshot_lock_duration",
"aws_snapshot_lock_cool_off_period",
"aws_snapshot_lock_expiration_date",
"aws_profile",
"aws_region",
"azure_credential",
Expand Down Expand Up @@ -710,6 +729,9 @@ class ServerConfig(BaseConfig):
"archiver_batch_size": int,
"autogenerate_manifest": parse_boolean,
"aws_await_snapshots_timeout": int,
"aws_snapshot_lock_duration": int,
"aws_snapshot_lock_cool_off_period": int,
"aws_snapshot_lock_expiration_date": parse_datetime,
Comment on lines +732 to +734
Copy link
Contributor

Choose a reason for hiding this comment

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

We could somehow leverage of the utils functions that you created for validating the CLI arguments of barman-cloud, to validate all the new configuration options (including the lock mode).

"backup_compression": parse_backup_compression,
"backup_compression_format": parse_backup_compression_format,
"backup_compression_level": int,
Expand Down
48 changes: 48 additions & 0 deletions barman/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,44 @@ def check_positive(value):
return int_value


def check_aws_snapshot_lock_duration_range(value):
"""
Check for AWS Snapshot Lock duration range option
:param value: str containing the value to check
"""
if value is None:
return None
try:
int_value = int(value)
except Exception:
raise ArgumentTypeError("'%s' is not a valid input" % value)

if int_value < 1 or int_value > 36500:
raise ArgumentTypeError("'%s' is outside supported range of 1-36500 days" % value)

Comment on lines +742 to +744
Copy link
Contributor

@andremagui andremagui Oct 22, 2024

Choose a reason for hiding this comment

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

Suggested change
if int_value < 1 or int_value > 36500:
raise ArgumentTypeError("'%s' is outside supported range of 1-36500 days" % value)
if not 1 <= int_value <= 36500:
raise ArgumentTypeError(f"Value must be between 1 and 36,500 days.")

This way is more efficient because the value is compared only once in a single operation and It directly expresses the idea that the value should fall within a specific range.

return int_value


def check_aws_snapshot_lock_cool_off_period_range(value):
"""
Check for AWS Snapshot Lock cool-off period range option
:param value: str containing the value to check
"""
if value is None:
return None
try:
int_value = int(value)
except Exception:
raise ArgumentTypeError("'%s' is not a valid input" % value)

if int_value < 1 or int_value > 72:
raise ArgumentTypeError("'%s' is outside supported range of 1-72 hours" % value)
Comment on lines +761 to +762
Copy link
Contributor

@andremagui andremagui Oct 22, 2024

Choose a reason for hiding this comment

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

Suggested change
if int_value < 1 or int_value > 72:
raise ArgumentTypeError("'%s' is outside supported range of 1-72 hours" % value)
if not 1 <= int_value <= 72:
raise ArgumentTypeError("Value must be between 1 and 72 hours.")

same here.


return int_value


def check_tli(value):
"""
Check for a positive integer option, and also make "current" and "latest" acceptable values
Expand Down Expand Up @@ -783,6 +821,16 @@ def check_size(value):
return int_value


def check_timestamp(value):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def check_timestamp(value):
def check_aws_expiration_date_format(value):

More specific name to the job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add docstrings like the other aws checks

try:
# Attempt to parse the input date string into a datetime object
return datetime.datetime.strptime(value, "%Y-%m-%dT%H:%M:%S.%fZ")
except ValueError:
raise ArgumentTypeError(
"Invalid expiration date: '%s'. Expected format is 'YYYY-MM-DDThh:mm:ss.sssZ'." % value
)


def check_backup_name(backup_name):
"""
Verify that a backup name is not a backup ID or reserved identifier.
Expand Down
1 change: 1 addition & 0 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -1761,6 +1761,7 @@ def test_help_output(self, minimal_parser, capsys):
if sys.version_info < (3, 10):
options_label = "optional arguments"
expected_output = self._expected_help_output.format(options_label=options_label)

Copy link
Contributor

Choose a reason for hiding this comment

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

revert or remove this

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

assert expected_output == out


Expand Down
Loading