Network acl support #1098

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
7 participants
@jsmartin
Contributor

jsmartin commented Nov 6, 2012

This adds support from network acls. I was not able to find a method AssociateNetworkAcl on this page: http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/query-apis.html

@garnaat

This comment has been minimized.

Show comment Hide comment
@garnaat

garnaat Nov 6, 2012

Member

This PR looks good to me. What do you say @jamesls ?

Member

garnaat commented Nov 6, 2012

This PR looks good to me. What do you say @jamesls ?

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Nov 6, 2012

Contributor

Any insight into how we can get AssociateNetworkAcl exposed and documented?

Contributor

jsmartin commented Nov 6, 2012

Any insight into how we can get AssociateNetworkAcl exposed and documented?

@jamesls

This comment has been minimized.

Show comment Hide comment
@jamesls

jamesls Nov 6, 2012

Member

I'm not too familiar with this particular functionality of VPC, but the code itself looks good to me.

Member

jamesls commented Nov 6, 2012

I'm not too familiar with this particular functionality of VPC, but the code itself looks good to me.

boto/vpc/__init__.py
+ }
+
+ result = self.get_object('SubnetNetworkAclAssociation', params, ResultSet)
+ return result.associationId

This comment has been minimized.

Show comment Hide comment
@garnaat

garnaat Nov 7, 2012

Member

I don't see this request in the EC2 documentation.

@garnaat

garnaat Nov 7, 2012

Member

I don't see this request in the EC2 documentation.

@garnaat

This comment has been minimized.

Show comment Hide comment
@garnaat

garnaat Nov 7, 2012

Member

I guess I don't really understand the question about exposing additional requests. This page:

http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/OperationList-query.html

lists the requests by category. If you go to the "Network ACLs" section, you can see all of the available requests associated with network ACL's. There aren't any others to expose. But the PR seems to be referencing some requests that aren't on this list.

Are you saying these undocumented requests are actually working for you?

Member

garnaat commented Nov 7, 2012

I guess I don't really understand the question about exposing additional requests. This page:

http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/OperationList-query.html

lists the requests by category. If you go to the "Network ACLs" section, you can see all of the available requests associated with network ACL's. There aren't any others to expose. But the PR seems to be referencing some requests that aren't on this list.

Are you saying these undocumented requests are actually working for you?

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Nov 7, 2012

Contributor

No, it doesn't work for me. I stubbed it out because it seemed like it should be there. There is a cmd line tool for it: http://docs.amazonwebservices.com/AWSEC2/latest/CommandLineReference/ApiReference-cmd-ReplaceNetworkAclAssociation.html, also it is an available option in the AWS Web GUI. Is it possible it's only available to Amazon branded tools?

Contributor

jsmartin commented Nov 7, 2012

No, it doesn't work for me. I stubbed it out because it seemed like it should be there. There is a cmd line tool for it: http://docs.amazonwebservices.com/AWSEC2/latest/CommandLineReference/ApiReference-cmd-ReplaceNetworkAclAssociation.html, also it is an available option in the AWS Web GUI. Is it possible it's only available to Amazon branded tools?

@garnaat

This comment has been minimized.

Show comment Hide comment
@garnaat

garnaat Nov 7, 2012

Member

No, I don't think so. I suspect the command line tool is using some combination of the existing API and custom logic to achieve the desired result. Similarly for the console. I'll try to find out more information. Thanks.

Member

garnaat commented Nov 7, 2012

No, I don't think so. I suspect the command line tool is using some combination of the existing API and custom logic to achieve the desired result. Similarly for the console. I'll try to find out more information. Thanks.

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Nov 8, 2012

Contributor

I found the proper action in the API reference. I swear it wasn't there before :). Anyhow it's called ReplaceNetworkAclAssociation and I'll try and get it updated soon.

Contributor

jsmartin commented Nov 8, 2012

I found the proper action in the API reference. I swear it wasn't there before :). Anyhow it's called ReplaceNetworkAclAssociation and I'll try and get it updated soon.

James Martin
Added support for associating and disassociating network ACLs and
subnets.
Added supoprt for replacing network acl entries.
@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Nov 9, 2012

Contributor

So I was able to get network acl associations implemented, but am hitting another stumbling block. In the http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/ApiReference-query-DescribeNetworkAcls.html call, a response with the network portRange will appear as:

   <item>
     <ruleNumber>110</ruleNumber>
     <protocol>6</protocol>
     <ruleAction>allow</ruleAction>
     <egress>false</egress>
     <cidrBlock>0.0.0.0/0</cidrBlock>
     <portRange>
       <from>80</from>
       <to>80</to>
     </portRange>
   </item>

and I can't quite figure out how to break that apart in my NetworkAclEntry class.

Contributor

jsmartin commented Nov 9, 2012

So I was able to get network acl associations implemented, but am hitting another stumbling block. In the http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/ApiReference-query-DescribeNetworkAcls.html call, a response with the network portRange will appear as:

   <item>
     <ruleNumber>110</ruleNumber>
     <protocol>6</protocol>
     <ruleAction>allow</ruleAction>
     <egress>false</egress>
     <cidrBlock>0.0.0.0/0</cidrBlock>
     <portRange>
       <from>80</from>
       <to>80</to>
     </portRange>
   </item>

and I can't quite figure out how to break that apart in my NetworkAclEntry class.

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Nov 9, 2012

Contributor

Ok, that should do it :) .

Contributor

jsmartin commented Nov 9, 2012

Ok, that should do it :) .

@garnaat

This comment has been minimized.

Show comment Hide comment
@garnaat

garnaat Dec 3, 2012

Member

This looks good to me. What do you think, @jamesls ?

Member

garnaat commented Dec 3, 2012

This looks good to me. What do you think, @jamesls ?

@shredder12

This comment has been minimized.

Show comment Hide comment
@shredder12

shredder12 May 5, 2013

Hi all. What's the status of this PR?

Hi all. What's the status of this PR?

@israelshirk

This comment has been minimized.

Show comment Hide comment
@israelshirk

israelshirk May 16, 2013

This would be super-useful. Is there anything else on it that needs some effort? I'll be giving it a try tonight or tomorrow, if I find anything needed to get it in sync with more recent Boto I'll add it.

This would be super-useful. Is there anything else on it that needs some effort? I'll be giving it a try tonight or tomorrow, if I find anything needed to get it in sync with more recent Boto I'll add it.

@shredder12

This comment has been minimized.

Show comment Hide comment
@shredder12

shredder12 May 16, 2013

@israelshirk Yes. It works fine. I've been using it for a while. It'd be good indeed if its merged.

@israelshirk Yes. It works fine. I've been using it for a while. It'd be good indeed if its merged.

@e-gineer

This comment has been minimized.

Show comment Hide comment
@e-gineer

e-gineer May 21, 2013

I'm also relying on this and have found it to work well. FYI, I run it as a pull on top of the latest boto and it merges without any problem. Would be great to see it merged.

I'm also relying on this and have found it to work well. FYI, I run it as a pull on top of the latest boto and it merges without any problem. Would be great to see it merged.

@ryno75

This comment has been minimized.

Show comment Hide comment
@ryno75

ryno75 Jul 17, 2013

+1
Yes, getting this merged into the main branch would be awesome.

ryno75 commented Jul 17, 2013

+1
Yes, getting this merged into the main branch would be awesome.

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Jul 18, 2013

Contributor

Folks,

I asked in IRC why this was never merged and it was told it needs some tests. I don't have the time to write the tests for it, so anyone who is interested in getting this merged, please add some tests!

Contributor

jsmartin commented Jul 18, 2013

Folks,

I asked in IRC why this was never merged and it was told it needs some tests. I don't have the time to write the tests for it, so anyone who is interested in getting this merged, please add some tests!

@israelshirk

This comment has been minimized.

Show comment Hide comment
@israelshirk

israelshirk Jul 18, 2013

@jsmartin would that just be something comparable to the existing unit tests (ie, checking the message being sent and the message parsing), or would it be an integration test? Thanks!

@jsmartin would that just be something comparable to the existing unit tests (ie, checking the message being sent and the message parsing), or would it be an integration test? Thanks!

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Jul 19, 2013

Contributor

@israelshirk I think if you take it up with the mailing list you'll get a better response. I didn't get a response on IRC, just "it needs tests".

Contributor

jsmartin commented Jul 19, 2013

@israelshirk I think if you take it up with the mailing list you'll get a better response. I didn't get a response on IRC, just "it needs tests".

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Jul 19, 2013

Contributor

Just got an update on IRC:

daniellindsley: jsm: The XML parsing classes could be unit tests. Unfortunately, for the methods in "init.py", they either need mocking (to be unit) or integration

Contributor

jsmartin commented Jul 19, 2013

Just got an update on IRC:

daniellindsley: jsm: The XML parsing classes could be unit tests. Unfortunately, for the methods in "init.py", they either need mocking (to be unit) or integration

@israelshirk

This comment has been minimized.

Show comment Hide comment
@israelshirk

israelshirk Jul 19, 2013

Thanks for the assist @jsmartin, I'll hop on getting some tests written in the next few days.

Thanks for the assist @jsmartin, I'll hop on getting some tests written in the next few days.

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Jul 19, 2013

Contributor

@israelshirk awesome, thanks!

Contributor

jsmartin commented Jul 19, 2013

@israelshirk awesome, thanks!

@jsmartin

This comment has been minimized.

Show comment Hide comment
@jsmartin

jsmartin Sep 18, 2013

Contributor

@israelshirk did you make any progress with the tests?

Contributor

jsmartin commented Sep 18, 2013

@israelshirk did you make any progress with the tests?

@israelshirk

This comment has been minimized.

Show comment Hide comment
@israelshirk

israelshirk Sep 18, 2013

Sadly, no - have been slammed with some new projects. You're welcome to claim if you're free.

On Sep 18, 2013, at 3:42 PM, jsmartin notifications@github.com wrote:

@israelshirk did you make any progress with the tests?


Reply to this email directly or view it on GitHub.

Sadly, no - have been slammed with some new projects. You're welcome to claim if you're free.

On Sep 18, 2013, at 3:42 PM, jsmartin notifications@github.com wrote:

@israelshirk did you make any progress with the tests?


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment