Restore RDS DB from snapshot #120

tewfik-ghariani · 2020-11-25T21:07:38Z

This PR introduces the possibility of restoring an RDS DB from snapshot.
Given that the RDS DB instance restored is automatically placed in the default SG, added the support for immediately modifying it to apply the submitted VPC SG

Additionally, the RDS module has been converted to boto3 instead of boto2

Other changes

RDS Subnet group: fix the destroy operation
Example file of RDS DB in VPC
Blackify
Upgrading boto3 version and re-locking poetry

cc @grahamc, @adisbladis, @RaitoBezarius

RaitoBezarius

+1 on handling better errors and a new test, so thank you! Otherwise, it would be awesome to cut this PR into 3 PRs:

(1) Boto3 port.
(2) RDS snapshot feature.
(3) Unrelated minor changes like those on RDS subnets.

Anyway, except those points, LGTM.

RaitoBezarius · 2020-11-26T23:39:32Z

nixops_aws/resources/ec2_rds_dbinstance.py

@@ -28,8 +30,8 @@ class EC2RDSDbInstanceDefinition(nixops.resources.ResourceDefinition):

    config: Ec2RdsDbinstanceOptions
    subnet_group: Optional[str]
-    vpc_security_groups: Optional[Sequence[str]] = None
-    rds_dbinstance_security_groups: Optional[Sequence[str]] = None
+    vpc_security_groups: Optional[Sequence[str]] = []


So, is it still relevant to keep Optional ? Same for the line below.

It is still relevant of course, the 'vpc_security_groups' can be omitted since it's optional but by default it would have the empty array value.

RaitoBezarius · 2020-11-26T23:41:18Z

nixops_aws/resources/ec2_rds_dbinstance.py

-
-            self.rds_dbinstance_port = int(dbinstance.endpoint[1])
-            self.rds_dbinstance_endpoint = "%s:%d" % dbinstance.endpoint
+            self.rds_dbinstance_id = dbinstance["DBInstanceIdentifier"]


I'd have abstracted the AWS strings into a String Enum enum.Enum, rather than multiplying hardcoded references to it, that way, it's easy to change them in all the relevant places. But it can be done in a further PR IMHO.

RaitoBezarius · 2020-11-26T23:43:13Z

nixops_aws/resources/rds_db_subnet_group.py

+    def __init__(self, name: str, config: nixops.resources.ResourceEval):
+        super(RDSDbSubnetGroupDefinition, self).__init__(name, config)
+
+        self.group_name: str = self.config.name


I thought it was mostly about RDS snapshot & boto 3 port, is this change really required in this PR?

This change is to mainly fix the destroy operation bug related to the subnet-group resource but also to streamline the resource definition and follow the standard

RaitoBezarius · 2020-11-26T23:44:51Z

nixops_aws/resources/rds_db_subnet_group.py

@@ -35,8 +49,8 @@ class RDSDbSubnetGroupState(nixops.resources.ResourceState[RDSDbSubnetGroupDefin
    region = nixops.util.attr_property("region", None)
    description = nixops.util.attr_property("description", None)
    subnet_ids = nixops.util.attr_property("subnet_ids", [], "json")
+    access_key_id = nixops.util.attr_property("accessKeyId", None)


Why is it required now?

It was moved from line 39 to this place so that in the new line 83, we would access it from the defn directly instead of from the config

tewfik-ghariani

Thanks for the review @RaitoBezarius

Actually I split my commits in a way that each change would be viewed on its own to make the review easier

(1) Boto3 port. fb5ea12
(2) RDS snapshot feature. 576b701
(3) Unrelated minor changes like those on RDS subnets. :

Example file d5d85fb
Fix destroy operation of subnet-group a5c0355
Upgrade boto3 version eb996c9
Black : b049ebc

tewfik-ghariani · 2020-11-27T18:03:18Z

nixops_aws/resources/ec2_rds_dbinstance.py

@@ -28,8 +30,8 @@ class EC2RDSDbInstanceDefinition(nixops.resources.ResourceDefinition):

    config: Ec2RdsDbinstanceOptions
    subnet_group: Optional[str]
-    vpc_security_groups: Optional[Sequence[str]] = None
-    rds_dbinstance_security_groups: Optional[Sequence[str]] = None
+    vpc_security_groups: Optional[Sequence[str]] = []


It is still relevant of course, the 'vpc_security_groups' can be omitted since it's optional but by default it would have the empty array value.

tewfik-ghariani · 2020-11-27T18:06:21Z

nixops_aws/resources/rds_db_subnet_group.py

+    def __init__(self, name: str, config: nixops.resources.ResourceEval):
+        super(RDSDbSubnetGroupDefinition, self).__init__(name, config)
+
+        self.group_name: str = self.config.name


This change is to mainly fix the destroy operation bug related to the subnet-group resource but also to streamline the resource definition and follow the standard

tewfik-ghariani · 2020-11-27T18:07:34Z

nixops_aws/resources/rds_db_subnet_group.py

@@ -35,8 +49,8 @@ class RDSDbSubnetGroupState(nixops.resources.ResourceState[RDSDbSubnetGroupDefin
    region = nixops.util.attr_property("region", None)
    description = nixops.util.attr_property("description", None)
    subnet_ids = nixops.util.attr_property("subnet_ids", [], "json")
+    access_key_id = nixops.util.attr_property("accessKeyId", None)


It was moved from line 39 to this place so that in the new line 83, we would access it from the defn directly instead of from the config

tewfik-ghariani added 6 commits November 25, 2020 02:22

RDS DB using boto3

fb5ea12

RDS Subnet group: fix the destroy operation

a5c0355

Blackify

b049ebc

Upgrading boto3 version

eb996c9

Restoring RDS DB from Snapshot

576b701

Example of RDS DB in VPC

d5d85fb

manari-oumaima approved these changes Nov 26, 2020

View reviewed changes

type null for snap

caf31ab

RaitoBezarius reviewed Nov 26, 2020

View reviewed changes

tewfik-ghariani commented Nov 27, 2020

View reviewed changes

tewfik-ghariani and others added 4 commits November 28, 2020 03:11

Handling DBSnapshotNotFoundFault exception

e8d49a6

updating old param names

6e0d338

Bump install-nix-action to v12

a854bd2

Fixing github ci

8085e19

tewfik-ghariani mentioned this pull request Oct 12, 2021

WIP: RDS: add engineVersion attribute #150

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore RDS DB from snapshot #120

Restore RDS DB from snapshot #120

tewfik-ghariani commented Nov 25, 2020

RaitoBezarius left a comment

RaitoBezarius Nov 26, 2020

tewfik-ghariani Nov 27, 2020

RaitoBezarius Nov 26, 2020

RaitoBezarius Nov 26, 2020

tewfik-ghariani Nov 27, 2020

RaitoBezarius Nov 26, 2020

tewfik-ghariani Nov 27, 2020

tewfik-ghariani left a comment

tewfik-ghariani Nov 27, 2020

tewfik-ghariani Nov 27, 2020

tewfik-ghariani Nov 27, 2020

Restore RDS DB from snapshot #120

Are you sure you want to change the base?

Restore RDS DB from snapshot #120

Conversation

tewfik-ghariani commented Nov 25, 2020

RaitoBezarius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tewfik-ghariani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment