-
Notifications
You must be signed in to change notification settings - Fork 166
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
Initial attempt at new RDS blueprint framework #86
Changes from 1 commit
97c748d
4c1702e
3af78d3
1349377
7fd73b9
ca923d5
47a0dd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,336 @@ | ||
from troposphere import ( | ||
Ref, ec2, Output, GetAtt, Not, Equals, Condition, And, Join, If | ||
) | ||
from troposphere.rds import DBInstance, DBSubnetGroup | ||
from troposphere.route53 import RecordSetType | ||
|
||
from ..base import Blueprint | ||
|
||
RDS_ENGINES = ["MySQL", "oracle-se1", "oracle-se", "oracle-ee", "sqlserver-ee", | ||
"sqlserver-se", "sqlserver-ex", "sqlserver-web", "postgres"] | ||
|
||
# Resource name constants | ||
SUBNET_GROUP = "RDSSubnetGroup" | ||
SECURITY_GROUP = "RDSSecurityGroup" | ||
DBINSTANCE_NO_IOPS = "RDSDBInstanceNoIops" | ||
DBINSTANCE_WITH_IOPS = "RDSDBInstanceWithIops" | ||
DNS_RECORD = "DBInstanceDnsRecord" | ||
|
||
|
||
def common_parameters(): | ||
""" Return common parameters for all RDS stacks. | ||
|
||
Returns: | ||
dict: A dictionary of parameter definitions. | ||
""" | ||
|
||
parameters = { | ||
"VpcId": { | ||
"type": "AWS::EC2::VPC::Id", | ||
"description": "Vpc Id"}, | ||
"Subnets": { | ||
"type": "List<AWS::EC2::Subnet::Id>", | ||
"description": "Subnets to deploy RDS instance in."}, | ||
"InstanceType": { | ||
"type": "String", | ||
"description": "AWS RDS Instance Type", | ||
"default": "db.m3.large"}, | ||
"AllowMajorVersionUpgrade": { | ||
"type": "String", | ||
"description": "Set to 'true' to allow major version " | ||
"upgrades.", | ||
"default": "false", | ||
"allowed_values": ["true", "false"] | ||
}, | ||
"AutoMinorVersionUpgrade": { | ||
"type": "String", | ||
"description": "Set to 'true' to allow minor version upgrades " | ||
"during maintenance windows.", | ||
"default": "false", | ||
"allowed_values": ["true", "false"] | ||
}, | ||
"AllocatedStorage": { | ||
"type": "Number", | ||
"description": "Space, in GB, to allocate to RDS instance. If " | ||
"IOPS is set below, this must be a minimum of " | ||
"100 and must be at least 1/10th the IOPs " | ||
"setting.", | ||
"default": "10"}, | ||
"StorageEncrypted": { | ||
"type": "String", | ||
"description": "Set to 'false' to disable encrypted storage.", | ||
"default": "true", | ||
"allowed_values": ["true", "false"] | ||
}, | ||
"IOPS": { | ||
"type": "Number", | ||
"description": "If set, uses provisioned IOPS for the " | ||
"database. Note: This must be no more than " | ||
"10x of AllocatedStorage. Minimum: 1000", | ||
"max_value": 10000, | ||
"default": "0"}, | ||
"InternalZoneId": { | ||
"type": "String", | ||
"default": "", | ||
"description": "Internal zone Id, if you have one."}, | ||
"InternalZoneName": { | ||
"type": "String", | ||
"default": "", | ||
"description": "Internal zone name, if you have one."}, | ||
"InternalHostname": { | ||
"type": "String", | ||
"default": "", | ||
"description": "Internal domain name, if you have one."}, | ||
} | ||
|
||
return parameters | ||
|
||
|
||
class MasterInstance(Blueprint): | ||
""" Blueprint for a generic Master RDS Database Instance. | ||
|
||
Subclasses should be created for each RDS engine for better validation of | ||
things like engine version. | ||
""" | ||
|
||
ENGINE = None | ||
|
||
def _get_engine_versions(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if something should be overridden, i usually don't put an underscore since it implies some sort of public api |
||
"""Used by engine specific subclasses - returns valid engine versions. | ||
|
||
Should only be overridden if the class variable ENGINE is defined on | ||
the class. | ||
|
||
Return: | ||
list: A list of valid engine versions for the given engine. | ||
""" | ||
return [] | ||
|
||
def _get_parameters(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where is this value used? is this from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - there was a change a little while back for this, to make it easy/possible to override Parameter definitions in subclasses. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea, i would just make it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So really the definition of private method that I've always followed is any method that won't be called directly by anything but the class itself, not whether it might get overriden in subclasses. I don't plan on fixing this in this update :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well the subclass overrides it to conform to a specific interface for interacting with the parent class. the underscore implies "i can do whatever i want with this because i told you it could change by having an underscore". if i edited the base blueprint and removed/changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, the bikeshed is BLUE! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hahah - I think that's honestly just a personal interpretation of In general I don't think anyone would ever want to call |
||
master_parameters = { | ||
"EngineVersion": { | ||
"type": "String", | ||
"description": "Database engine version for the RDS Instance.", | ||
}, | ||
"BackupRetentionPeriod": { | ||
"type": "Number", | ||
"description": "Number of days to retain database backups.", | ||
"min_value": 0, | ||
"default": 7, | ||
"max_value": 35, | ||
"constraint_description": "Must be between 0-35.", | ||
}, | ||
"MasterUser": { | ||
"type": "String", | ||
"description": "Name of the master user in the db.", | ||
"default": "dbuser"}, | ||
"MasterUserPassword": { | ||
"type": "String", | ||
"no_echo": "true", | ||
"description": "Master user password."}, | ||
"PreferredBackupWindow": { | ||
"type": "String", | ||
"description": "A (minimum 30 minute) window in HH:MM-HH:MM " | ||
"format in UTC for backups. Default: 3am-4am " | ||
"PST", | ||
"default": "11:00-12:00"}, | ||
"DatabaseName": { | ||
"type": "String", | ||
"description": "Initial db to create in database."}, | ||
"MultiAZ": { | ||
"type": "String", | ||
"description": "Set to 'false' to disable MultiAZ support.", | ||
"default": "true"}, | ||
} | ||
engine_versions = self._get_engine_versions() | ||
if engine_versions: | ||
master_parameters['EngineVersion']['allowed_values'] = \ | ||
engine_versions | ||
|
||
if not self.ENGINE: | ||
master_parameters['Engine'] = { | ||
"type": "String", | ||
"description": "Database engine for the RDS Instance.", | ||
"allowed_values": RDS_ENGINES | ||
} | ||
else: | ||
if self.ENGINE not in RDS_ENGINES: | ||
raise ValueError("ENGINE must be one of: %s" % | ||
", ".join(RDS_ENGINES)) | ||
|
||
# Merge common parameters w/ master only parameters | ||
parameters = common_parameters().update(master_parameters) | ||
|
||
return parameters | ||
|
||
def create_conditions(self): | ||
self.template.add_condition( | ||
"HasInternalZone", | ||
Not(Equals(Ref("InternalZoneId"), ""))) | ||
self.template.add_condition( | ||
"HasInternalZoneName", | ||
Not(Equals(Ref("InternalZoneName"), ""))) | ||
self.template.add_condition( | ||
"HasInternalHostname", | ||
Not(Equals(Ref("InternalHostname"), ""))) | ||
self.template.add_condition( | ||
"CreateInternalHostname", | ||
And(Condition("HasInternalZone"), | ||
Condition("HasInternalZoneName"), | ||
Condition("HasInternalHostname"))) | ||
self.template.add_condition( | ||
"ProvisionedIOPS", | ||
Not(Equals(Ref("IOPS"), "0"))) | ||
self.template.add_condition( | ||
"NoProvisionedIOPS", | ||
Equals(Ref("IOPS"), "0")) | ||
|
||
def create_subnet_group(self): | ||
t = self.template | ||
t.add_resource( | ||
DBSubnetGroup( | ||
SUBNET_GROUP, | ||
DBSubnetGroupDescription="%s VPC subnet group." % self.name, | ||
SubnetIds=Ref("PrivateSubnets"))) | ||
|
||
def create_security_group(self): | ||
t = self.template | ||
sg = t.add_resource( | ||
ec2.SecurityGroup( | ||
SECURITY_GROUP, | ||
GroupDescription="%s RDS security group" % self.name, | ||
VpcId=Ref("VpcId"))) | ||
t.add_output(Output("SecurityGroup", Value=Ref(sg))) | ||
|
||
def get_db_instance(self): | ||
return If("ProvisionedIOPS", DBINSTANCE_WITH_IOPS, | ||
DBINSTANCE_NO_IOPS) | ||
|
||
def get_db_endpoint(self): | ||
endpoint = GetAtt(self.get_db_instance(), "Endpoint.Address") | ||
return endpoint | ||
|
||
def create_rds(self): | ||
t = self.template | ||
# Non-provisioned iops database | ||
t.add_resource( | ||
DBInstance( | ||
DBINSTANCE_NO_IOPS, | ||
Condition="NoProvisionedIOPS", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would pull out all of these common parameters so i don't have to duplicate specifying them in each There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hard/ugly to do? How would you do it? Just args/*kwargs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i was just thinking like: common = {
'AllocatedStorage': Ref("AllocatedStorage"),
'AllowMajorVersionUpgrade': Ref("AllowMajorVersionUpgrade"),
'AutoMinorVersionUpgrade': Ref("AutoMinorVersionUpgrade"),
'BackupRetentionPeriod': Ref("BackupRetentionPeriod"),
'DBName': Ref("DatabaseName"),
'DBInstanceClass': Ref("InstanceType"),
'DBSubnetGroupName': Ref(SUBNET_GROUP),
'Engine': self.ENGINE or Ref("Engine"),
'EngineVersion': Ref("EngineVersion"),
'MasterUsername': Ref("MasterUser"),
'MasterUserPassword': Ref("MasterUserPassword"),
'MultiAZ': Ref("MultiAZ"),
'PreferredBackupWindow': Ref("PreferredBackupWindow"),
'VPCSecurityGroups': [Ref(SECURITY_GROUP), ],
'StorageEncrypted': Ref("StorageEncrypted"),
}
t.add_resource(
DBInstance(
DBINSTANCE_NO_IOPS,
Condition="NoProvisionedIOPS",
**common
)
)
t.add_resource(
DBInstance(
DBINSTANCE_WITH_IOPS,
Condition="ProvisionedIOPS",
StorageEncrypted=Ref("StorageEncrypted"),
StorageType="io1",
Iops=Ref("IOPS"),
**common
)
) which also has the added benefit of letting me see whats different about the two. not sure if thats 100% accurate just a rough copy paste |
||
AllocatedStorage=Ref("AllocatedStorage"), | ||
AllowMajorVersionUpgrade=Ref("AllowMajorVersionUpgrade"), | ||
AutoMinorVersionUpgrade=Ref("AutoMinorVersionUpgrade"), | ||
BackupRetentionPeriod=Ref("BackupRetentionPeriod"), | ||
DBName=Ref("DatabaseName"), | ||
DBInstanceClass=Ref("InstanceType"), | ||
DBSubnetGroupName=Ref(SUBNET_GROUP), | ||
Engine=self.ENGINE or Ref("Engine"), | ||
EngineVersion=Ref("EngineVersion"), | ||
MasterUsername=Ref("MasterUser"), | ||
MasterUserPassword=Ref("MasterUserPassword"), | ||
MultiAZ=Ref("MultiAZ"), | ||
PreferredBackupWindow=Ref("PreferredBackupWindow"), | ||
StorageEncrypted=Ref("StorageEncrypted"), | ||
VPCSecurityGroups=[Ref(SECURITY_GROUP), ])) | ||
|
||
t.add_resource( | ||
DBInstance( | ||
DBINSTANCE_WITH_IOPS, | ||
Condition="ProvisionedIOPS", | ||
AllocatedStorage=Ref("AllocatedStorage"), | ||
AllowMajorVersionUpgrade=Ref("AllowMajorVersionUpgrade"), | ||
AutoMinorVersionUpgrade=Ref("AutoMinorVersionUpgrade"), | ||
BackupRetentionPeriod=Ref("BackupRetentionPeriod"), | ||
DBName=Ref("DatabaseName"), | ||
DBInstanceClass=Ref("InstanceType"), | ||
DBSubnetGroupName=Ref(SUBNET_GROUP), | ||
Engine=self.ENGINE or Ref("Engine"), | ||
EngineVersion=Ref("EngineVersion"), | ||
MasterUsername=Ref("MasterUser"), | ||
MasterUserPassword=Ref("MasterUserPassword"), | ||
MultiAZ=Ref("MultiAZ"), | ||
PreferredBackupWindow=Ref("PreferredBackupWindow"), | ||
VPCSecurityGroups=[Ref(SECURITY_GROUP), ], | ||
StorageEncrypted=Ref("StorageEncrypted"), | ||
StorageType="io1", | ||
Iops=Ref("IOPS"))) | ||
|
||
def create_dns_records(self): | ||
t = self.template | ||
endpoint = self.get_db_endpoint() | ||
|
||
# Setup CNAME to db | ||
t.add_resource( | ||
RecordSetType( | ||
DNS_RECORD, | ||
# Appends a "." to the end of the domain | ||
HostedZoneId=Ref("InternalZoneId"), | ||
Comment="RDS DB CNAME Record", | ||
Name=Join(".", [Ref("InternalHostname"), | ||
Ref("InternalZoneName")]), | ||
Type="CNAME", | ||
TTL="120", | ||
ResourceRecords=[endpoint], | ||
Condition="CreateInternalHostname")) | ||
|
||
def create_db_outputs(self): | ||
t = self.template | ||
t.add_output(Output("DBAddress", Value=self.get_db_endpoint())) | ||
t.add_output(Output("DBInstance", Value=Ref(self.get_db_instance()))) | ||
t.add_output( | ||
Output( | ||
"DBCname", | ||
Condition="CreateInternalHostname", | ||
Value=Ref(DNS_RECORD))) | ||
|
||
def create_template(self): | ||
self.create_conditions() | ||
self.create_subnet_group() | ||
self.create_security_group() | ||
self.create_rds() | ||
self.create_dns_records() | ||
self.create_db_outputs() | ||
|
||
|
||
class ReadReplica(MasterInstance): | ||
""" Blueprint for a Read replica RDS Database Instance. """ | ||
def _get_parameters(self): | ||
parameters = common_parameters() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just do:
and have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are parameters that come from the MasterInstance that shouldn't be used in ReadReplicas. A better design might be to have a BaseInstance type or something, and have MasterInstance & ReadReplica both subclass off of it, rather than ReadReplica being based on MasterInstance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see yea, not a big deal, you're just calling |
||
parameters['MasterDatabaseId'] = { | ||
"type": "String", | ||
"description": "ID of the master database to create a read " | ||
"replica of."} | ||
return parameters | ||
|
||
def create_rds(self): | ||
t = self.template | ||
# Non-provisioned iops database | ||
t.add_resource( | ||
DBInstance( | ||
DBINSTANCE_NO_IOPS, | ||
Condition="NoProvisionedIOPS", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here re: duplication of db parameters |
||
SourceDBInstanceIdentifier=Ref("MasterDatabaseId"), | ||
AllocatedStorage=Ref("AllocatedStorage"), | ||
AllowMajorVersionUpgrade=Ref("AllowMajorVersionUpgrade"), | ||
AutoMinorVersionUpgrade=Ref("AutoMinorVersionUpgrade"), | ||
DBInstanceClass=Ref("InstanceType"), | ||
DBSubnetGroupName=Ref(SUBNET_GROUP), | ||
StorageEncrypted=Ref("StorageEncrypted"), | ||
VPCSecurityGroups=[Ref(SECURITY_GROUP), ])) | ||
|
||
t.add_resource( | ||
DBInstance( | ||
DBINSTANCE_WITH_IOPS, | ||
Condition="ProvisionedIOPS", | ||
SourceDBInstanceIdentifier=Ref("MasterDatabaseId"), | ||
AllocatedStorage=Ref("AllocatedStorage"), | ||
AllowMajorVersionUpgrade=Ref("AllowMajorVersionUpgrade"), | ||
AutoMinorVersionUpgrade=Ref("AutoMinorVersionUpgrade"), | ||
DBInstanceClass=Ref("InstanceType"), | ||
DBSubnetGroupName=Ref(SUBNET_GROUP), | ||
VPCSecurityGroups=[Ref(SECURITY_GROUP), ], | ||
StorageEncrypted=Ref("StorageEncrypted"), | ||
StorageType="io1", | ||
Iops=Ref("IOPS"))) | ||
|
||
t.add_output(Output("DBAddress", Value=self.get_db_endpoint())) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
from stacker.blueprints.rds import base | ||
|
||
|
||
class MasterInstance(base.MasterInstance): | ||
ENGINE = "MySQL" | ||
|
||
def _get_engine_versions(self): | ||
return ['5.1.73a', '5.1.73b', '5.5.40', '5.5.40a', '5.5.40b', '5.5.41', | ||
'5.5.42', '5.6.19a', '5.6.19b', '5.6.21', '5.6.21b', '5.6.22', | ||
'5.6.23'] | ||
|
||
|
||
class ReadReplica(base.ReadReplica): | ||
ENGINE = "MySQL" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
from stacker.blueprints.rds import base | ||
|
||
|
||
class MasterInstance(base.MasterInstance): | ||
ENGINE = "postgres" | ||
|
||
def _get_engine_versions(self): | ||
return ['9.3.1', '9.3.2', '9.3.3', '9.3.5', '9.3.6', '9.4.1'] | ||
|
||
|
||
class ReadReplica(base.ReadReplica): | ||
ENGINE = "postgres" |
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.
should we add the snapshot parameter you were talking about too?
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.
I haven't yet figured out if that's the right thing to do here or not. There's a lot of 'if this parameter is specified, these parameters shouldn't be' which I handled in Read Replicas by just removing the unused/disallowed ones. Not sure what the right case should be here. If you have any ideas, let me know.
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 could just leave that up to private stacks but make it so we can pass those parameters to the db instance troposphere object