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.

Sign up for GitHub

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

Jump to bottom

[WIP] Ec2 security groups: make SourceSecurityGroupOwnerId optional #457

Closed
wants to merge 4 commits into from
Closed

[WIP] Ec2 security groups: make SourceSecurityGroupOwnerId optional #457

wants to merge 4 commits into from

Conversation

moretea
Copy link
Contributor

@moretea moretea commented Jun 22, 2016

This would fix #456.

I'd like some feedback on how to proceed (see source code comment below) and squash the commits into one commit before merging.

moretea added 3 commits June 22, 2016 19:17
@moretea
Allow *not* setting SourceSecurityGrroupOwnerId.
b080972 
According to the documentation [1], the parameter
"SourceSecurityGrroupOwnerId" does not have to present. It is only
requied if the security group is in a different account.

[1] http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_AuthorizeSecurityGroupIngress.html
@moretea
Add test for ec2 security groups
a2d0ed9
@moretea
WIP: must retry something. Need feedback
c9813f2
moretea
moretea reviewed Jun 22, 2016
View reviewed changes
nixops/resources/ec2_security_group.py
grp.authorize(ip_protocol=rule[0], from_port=rule[1], to_port=rule[2], cidr_ip=rule[3])
# ^^^^ SOMETHING HERE TO RETRY
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this fails, because it could not find the previous security group. It might take some time before it is created, so we should probably retry this a few times.

@moretea
Copy link
Contributor Author

moretea commented Jun 22, 2016

Unfortunately, this does not fix the error.

[nix-shell:~/code/nixops]$ AWS_ACCESS_KEY_ID=cs python tests.py -x tests/functional/test_ec2_security_groups.py 
ssh-security-group> creating EC2 security group ‘charon-72d0eef3-38a6-11e6-8121-208984d3285c-ssh-security-group’...
server-access.....> creating EC2 security group ‘charon-72d0eef3-38a6-11e6-8121-208984d3285c-server-access’...
can-access-server.> creating EC2 security group ‘can-access-server’...
my-key-pair.......> uploading EC2 key pair ‘charon-72d0eef3-38a6-11e6-8121-208984d3285c-my-key-pair’...
ssh-security-group> adding new rules to EC2 security group ‘charon-72d0eef3-38a6-11e6-8121-208984d3285c-ssh-security-group’...
server-access.....> adding new rules to EC2 security group ‘charon-72d0eef3-38a6-11e6-8121-208984d3285c-server-access’...
Emy-key-pair.......> deleting EC2 key pair ‘charon-72d0eef3-38a6-11e6-8121-208984d3285c-my-key-pair’...
can-access-server.> deleting EC2 security group `can-access-server' ID `sg-8386f5f8'...
ssh-security-group> deleting EC2 security group `charon-72d0eef3-38a6-11e6-8121-208984d3285c-ssh-security-group' ID `sg-8286f5f9'...
server-access.....> deleting EC2 security group `charon-72d0eef3-38a6-11e6-8121-208984d3285c-server-access' ID `sg-8c86f5f7'...
deployment ‘72d0eef3-38a6-11e6-8121-208984d3285c’ destroyed

======================================================================
ERROR: tests.functional.test_ec2_security_groups.TestEc2SecurityGroups.test_deploy
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/nix/store/pd0h15a41ij3v8wdlayarg5y2y4k718p-python2.7-nose-1.3.7/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/maarten/code/nixops/tests/functional/test_ec2_security_groups.py", line 18, in test_deploy
    self.depl.deploy()
  File "/home/maarten/code/nixops/nixops/deployment.py", line 971, in deploy
    self._deploy(**kwargs)
  File "/home/maarten/code/nixops/nixops/deployment.py", line 928, in _deploy
    nixops.parallel.run_tasks(nr_workers=-1, tasks=self.active_resources.itervalues(), worker_fun=worker)
  File "/home/maarten/code/nixops/nixops/parallel.py", line 74, in run_tasks
    raise MultipleExceptions(exceptions)
MultipleExceptions: Multiple exceptions: EC2ResponseError: 400 Bad Request
<?xml version="1.0" encoding="UTF-8"?>
<Response><Errors><Error><Code>InvalidGroup.NotFound</Code><Message>The security group 'charon-72d0eef3-38a6-11e6-8121-208984d3285c-server-access' does not exist in default VPC 'vpc-95223af0'</Message></Error></Errors><RequestID>d82b899e-81d1-45a9-9ce0-f44f015b396d</RequestID></Response>, EC2ResponseError: 400 Bad Request
<?xml version="1.0" encoding="UTF-8"?>
<Response><Errors><Error><Code>InvalidGroup.NotFound</Code><Message>The security group 'charon-72d0eef3-38a6-11e6-8121-208984d3285c-ssh-security-group' does not exist in default VPC 'vpc-xxxx'</Message></Error></Errors><RequestID>3f987728-a8d9-4c4b-be9c-2ac07753fc90</RequestID></Response>

@domenkozar domenkozar changed the title Ec2 security groups [WIP] Ec2 security groups Jun 27, 2016
@domenkozar
Copy link
Member

I've added WIP until the bug is really fixed

@domenkozar domenkozar added the bug label Jun 27, 2016
@domenkozar domenkozar changed the title [WIP] Ec2 security groups [WIP] Ec2 security groups: make SourceSecurityGroupOwnerId optional Jun 27, 2016
@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
Reviewers
No reviews
Assignees
No one assigned
Labels
backends/ec2 bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow creating ec2 security groups without group owner id
3 participants
@moretea @domenkozar @grahamc