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

Added a new pattern Resilience Backup Restore using Native AWS Services #156

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

prabaksa
Copy link

Issue #, if available:

Description of changes: Added a new pattern Resilience Backup Restore using Native AWS Services . Added files under bin, docs and lib folders. Changed CDK Version from 2.99.1 to 2.107.0 in package.json

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@shapirov103
Copy link
Contributor

@elamaran11 please review at your convenience.
@prabaksa I will update the libraries to use the latest CDK, you will need to merge that change.

@elamaran11
Copy link
Contributor

@elamaran11 please review at your convenience. @prabaksa I will update the libraries to use the latest CDK, you will need to merge that change.

Sounds good!

@elamaran11
Copy link
Contributor

@prabaksa Is the PR fully ready to review. Last time we spoke, i thought you were working on Architectural changes?

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@prabaksa AWSome work. Some high level feedback:

  1. How are we simulating failure and demonstrate the same? We should simulate failure and show the restore process.
  2. Are you planning to run sample workload as we discussed? We need show a workload like yelb, wordpress with some state in EBS restored
  3. package.json should be updated with latest blueprints version, cdk and all other dependencies
  4. I think you should have single documentation demonstrating the complete DR flow.
  5. Also the diagram in restore process is not correct, it should DR region running the workload.
  6. destroy "*" should destroy all stack.

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@prabaksa Overall looks great. AWSome work. I have some minor tactical feedback please fix this and i can run it once and merge. Also please see GH actions failing due to doc links. Please fix it all and i can run and merge.

@@ -0,0 +1,9 @@
import { configureApp } from '../lib/common/construct-utils';
import ResilienceBRAWSConstruct from '../lib/resilience/backup_restore/backup/aws';
Copy link
Contributor

Choose a reason for hiding this comment

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

ResilienceBRAWSConstruct doesnt look like standard convention. What is BRAWS?

Copy link
Author

Choose a reason for hiding this comment

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

Backup/Restore AWS

@@ -0,0 +1,9 @@
import { configureApp } from '../lib/common/construct-utils';
import ResilienceBRAWSConstruct from '../lib/resilience/backup_restore/restore/aws';
Copy link
Contributor

Choose a reason for hiding this comment

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

ResilienceBRAWSConstruct doesnt look like standard convention. What is BRAWS?

@@ -1,3 +1,10 @@
{
"app": "npx ts-node dist/lib/common/default-main.js"
"app": "npx ts-node dist/lib/common/default-main.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets remove cdk.json changes. This should go in the doc.

Copy link
Author

Choose a reason for hiding this comment

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

ok

export interface backupStackProps extends cdk.StackProps {
primaryKeyArn: string,
drbackupVault: backup.IBackupVault
//drbackupVault: { "backupVaultArn": string, backupVaultName: string, env: {"account": string , region: string}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented?

Copy link
Author

Choose a reason for hiding this comment

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

I will remove this, It is not required anymore.

super(scope, id, props );
const account = process.env.CDK_DEFAULT_ACCOUNT!;
const region = blueprints.utils.valueFromContext(scope, "resilience-backup-restore-aws.primary.region", undefined);
//const drregion = blueprints.utils.valueFromContext(scope, "resilience-backup-restore-aws.dr.region", undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented?

Copy link
Author

Choose a reason for hiding this comment

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

not used in this construct, will remove this earlier since everything was on a single file i had defined all the variables in the file.

super(scope, id, props );
const account = process.env.CDK_DEFAULT_ACCOUNT!;
const region = blueprints.utils.valueFromContext(scope, "resilience-backup-restore-aws.primary.region", undefined);
//const drregion = blueprints.utils.valueFromContext(scope, "resilience-backup-restore-aws.dr.region", undefined);
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.


const backupStackProps = {
primaryKeyArn: props.primaryKeyArn,
//drbackupVault: { "backupVaultArn": drbackupVault.attrArn, backupVaultName: drbackupVault.attrName, env: {"account": process.env.CDK_DEFAULT_ACCOUNT! , region: drregion}}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove



export default class ResilienceBRAWSConstruct {
constructor(scope: Construct, id: string, props?: cdk.StackProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

'id' is defined but never used. Allowed unused args must match /^_/u



export default class ResilienceBRAWSConstruct {
constructor(scope: Construct, id: string, props?: cdk.StackProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

'props' is defined but never used. Allowed unused args must match /^_/u

pendingWindowInDays: 30
});

const kmsAlias = new kms.CfnAlias(stack, 'KMSAlias', {
Copy link
Contributor

Choose a reason for hiding this comment

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

'kmsAlias'

Copy link
Contributor

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

One more Comment. We need to Bootstrap the Argo right. We need an env and env maps to teams and we will have separate ArgoConfig. Check this example -

function createArgoAddonConfig(environment: string, repoUrl: string): blueprints.ArgoCDAddOn {

new blueprints.addons.KubeProxyAddOn(),
new blueprints.addons.AwsLoadBalancerControllerAddOn(),
new blueprints.addons.ArgoCDAddOn({
bootstrapRepo: {
Copy link
Contributor

Choose a reason for hiding this comment

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

We dont bootstrap Argo like this. We need an env and env maps to teams and we will have separate ArgoConfig. Check this example -

function createArgoAddonConfig(environment: string, repoUrl: string): blueprints.ArgoCDAddOn {

new blueprints.addons.KubeProxyAddOn(),
new blueprints.addons.AwsLoadBalancerControllerAddOn(),
new blueprints.addons.ArgoCDAddOn({
bootstrapRepo: {
Copy link
Contributor

Choose a reason for hiding this comment

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

We dont bootstrap Argo like this. We need an env and env maps to teams and we will have separate ArgoConfig. Check this example -

function createArgoAddonConfig(environment: string, repoUrl: string): blueprints.ArgoCDAddOn {

Copy link
Contributor

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

@prabaksa do you mind throwing a 30 min meeting to discuss the design here? I see a bunch of CDK stacks outside of the blueprints and also shell scripts. The objective of the meeting is to review the design and find the opps to improve customer experience.

Copy link

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label May 13, 2024
Copy link

Pull request closed due to inactivity.

@github-actions github-actions bot closed this Jul 12, 2024
@elamaran11 elamaran11 removed the stale label Aug 14, 2024
@elamaran11 elamaran11 reopened this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants