-
Notifications
You must be signed in to change notification settings - Fork 5
Feat/data retention backup #907
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
base: development
Are you sure you want to change the base?
Feat/data retention backup #907
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
f9a8670
to
c59119e
Compare
'backup:CopyTargets': [self.cross_account_ssn_backup_vault.backup_vault_arn] | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the generic cross_account_backup_vault shouldn't it?
@@ -26,8 +26,38 @@ | |||
"workspace_id": "T01234567" | |||
} | |||
] | |||
}, | |||
"backup_policies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be backing up our beta environment data. As it is intended to be ephemeral in nature, since it will likely have real SSNs mixed in with test data, which we don't want to backup for non-prod purposes
@@ -147,6 +147,7 @@ def __init__( | |||
self.pipeline_environment_context = self.ssm_context['environments']['pipeline'] | |||
self.connection_arn = self.pipeline_environment_context['connection_arn'] | |||
self.github_repo_string = self.ssm_context['github_repo_string'] | |||
self.backup_config = self.ssm_context['backup_config'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be optional in the base pipeline, since beta should not include it. We can enforce it in the prod and test pipelines, and throw an exception if not found, since it is mandatory for those.
@@ -21,6 +21,7 @@ def __init__( | |||
app_name: str, | |||
environment_name: str, | |||
environment_context: dict, | |||
backup_config: dict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be optional, since not every environment will have backups
backup_config: dict, | |
backup_config: dict | None, |
@@ -50,6 +51,7 @@ def __init__( | |||
app_name: str, | |||
environment_name: str, | |||
environment_context: dict, | |||
backup_config: dict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
backup_config: dict, | |
backup_config: dict | None, |
def _add_data_resources( | ||
self, removal_policy: RemovalPolicy, backup_infrastructure_stack: BackupInfrastructureStack | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these will need to be updated to account for an optional backup_infrastructure_stack
def _add_data_resources( | |
self, removal_policy: RemovalPolicy, backup_infrastructure_stack: BackupInfrastructureStack | |
): | |
def _add_data_resources( | |
self, removal_policy: RemovalPolicy, backup_infrastructure_stack: BackupInfrastructureStack | None | |
): |
class _AppSynthesizer: | ||
""" | ||
A helper class to cache apps based on context. | ||
This is useful to avoid re-synthesizing the app for each test. | ||
""" | ||
|
||
def __init__(self): | ||
super().__init__() | ||
self._cached_apps: dict[str, CompactConnectApp] = {} | ||
|
||
def get_app(self, context: Mapping) -> CompactConnectApp: | ||
context_hash = self._get_context_hash(context) | ||
if context_hash not in self._cached_apps.keys(): | ||
self._cached_apps[context_hash] = CompactConnectApp(context=context) | ||
return self._cached_apps[context_hash] | ||
|
||
def _get_context_hash(self, context: Mapping) -> str: | ||
return hash(json.dumps(context, sort_keys=True)) | ||
|
||
|
||
_app_synthesizer = _AppSynthesizer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat!
def _check_backend_stage_resource_counts(self, stage: BackendStage): | ||
""" | ||
Check resource counts for all stacks in a BackendStage and emit warnings/errors. | ||
|
||
Emits a warning if any stack has more than 400 resources. | ||
Fails the test if any stack has more than 475 resources. | ||
|
||
:param stage: The BackendStage containing stacks to check | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this!
class BackupsApp(App): | ||
def __init__(self, *args, **kwargs): | ||
""" | ||
An app for deploying data retention backup destination infrastructure in the backup account. | ||
This application creates the centralized backup infrastructure that receives and stores | ||
backup copies from CompactConnect environment accounts: | ||
- Cross-account backup vaults (general and SSN-specific) | ||
- Customer-managed KMS keys for backup encryption | ||
- Organization-level access policies for secure cross-account backup operations | ||
Environment account backup infrastructure (local vaults, IAM roles, backup plans) | ||
is managed by the CompactConnect application deployment. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly, it was mentioned that if these vaults don't exist in the secondary backup account for whatever reason, the application pipeline deployments will still succeed, but the backup processes will silently fail. If this is true, is there a way we can add alerting for this in prod? We will need to know if the backup processes are failing for whatever reason.
AWSTemplateFormatVersion: "2010-09-09" | ||
|
||
Description: > | ||
This template creates the IAM role required for the "How to secure recovery with cross-account backup and cross-Region copy using AWS Backuap" blog in each member account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is a little confusing, and has some typos. Can we find a way to reword this?
Description: > | ||
This template creates backup vault(cabvault) required for the "How to secure recovery with cross-account backup and cross-Region copy using AWS Backup" blog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this is needed for a blog? Or is it copied over from a blog that demonstrated this setup?
We are moving towards a setup where test and prod will have their own respective secondary accounts to which we will replicate their backups to. This updates the readme to reflect that change.
Description List
Testing List
pytest
We have the option to remove the backup account app and only back up data direct to the secondary environment account vault when that work is ready. Alternately, we could do the direct-to-secondary copy where applicable but use the backup account for pre-prod environments without a secondary deployment.
Deployment
control-tower
app to management account (for new SCP)backup
app to backup account (if we keep it)Closes #246