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 VPC security group to RDS resource #937

Closed
wants to merge 2 commits into from

Conversation

alexeiskachykhin
Copy link

This pull request introduces an ability to specify VPC security groups assigned to an RDS instance.

}
return { attr_to_kwarg[attr] : attrs[attr] for attr in attrs.keys() }

def _compare_instance_id(self, instance_id):
# take care when comparing instance ids, as aws lowercases and converts to unicode
return unicode(self.rds_dbinstance_id).lower() == unicode(instance_id).lower()

def fetch_security_group_resources(self, config):
def fetch_security_group_resources(self, config, type, idSelector):
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: idSelector -> id_selector

vpcSecurityGroups = [ resources.ec2SecurityGroups.test-rds-sg ];
};

resources.ec2SecurityGroups.test-rds-sg =
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should work on new AWS accounts that use default vpc but for EC2 classic accounts the sg won't be a vpc one ?
I think you could add a vpc as resource here and pass it to vpcId attribute of the security group ?

Copy link
Author

Choose a reason for hiding this comment

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

@AmineChikhaoui this turns out to be trickier than that. Running from EC2-VPC account if I introduce a new VPC and assign a security group to that VPC then I also have to ensure that an RDS instance is created in the same VPC. To be able to do that we need to introduce DB Subnet Group resource (which currently is not available with NixOps) and extend RDS resource to support DB Subnet Group assignment. It is a lot of work but in a real production setup we need this functionality anyway, otherwise we will be locked to a default VPC.

With that said, do you think this work needs to be done in this or separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

@alexeiskachykhin I think for this PR you could start by adding an option for dbSubnetGroup that takes a string first so that we can create an rds dbinstance in a VPC != default. Later a separate resource can be implemented.

@betaboon
Copy link

betaboon commented Aug 6, 2018

@alexeiskachykhin i have vpc-security-groups for rds-instances (including db-subnet-groups) implemented on my fork.
i have to clean it up and get it into a PR tho.

if you're interested https://github.com/MapCaseGmbH/nixops/tree/pr-rds-vpc-securitygroup

@grahamc
Copy link
Member

grahamc commented Mar 26, 2020

Hello!

Thank you for this PR.

In the past several months, some major changes have taken place in
NixOps:

  1. Backends have been removed, preferring a plugin-based architecture.
    Here are some of them:

  2. NixOps Core has been updated to be Python 3 only, and at the
    same time, MyPy type hints have been added and are now strictly
    required during CI.

This is all accumulating in to what I hope will be a NixOps 2.0
release
. There is a tracking issue for that:
#1242 . It is possible that
more core changes will be made to NixOps for this release, with a
focus on simplifying NixOps core and making it easier to use and work
on.

My hope is that by adding types and more thorough automated testing,
it will be easier for contributors to make improvements, and for
contributions like this one to merge in the future.

However, because of the major changes, it has become likely that this
PR cannot merge right now as it is. The backlog of now-unmergable PRs
makes it hard to see which ones are being kept up to date.

If you would like to see this merge, please bring it up to date with
master and reopen it
. If the or mypy type checking fails, please
correct any issues and then reopen it. I will be looking primarily at
open PRs whose tests are all green.

Thank you again for the work you've done here, I am sorry to be
closing it now.

Graham

@grahamc grahamc closed this Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants