You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2014/10/10 01:30:28 UTC
[jclouds] Allow to configure CIDR exclusion blocks (#567)
It could be useful to be able to express exclusion CIDR blocks when configuring firewalls. For example, CloudSigma allows to configure firewall rules with something like `!192.168.0.0/24` to allow traffic from any IP address outside that range.
This change should also simplify a lot that provider and unlock https://github.com/jclouds/jclouds-labs/pull/70
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds cidr-exclusion
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/567
-- Commit Summary --
* Allow to configure CIDR exclusion blocks
-- File Changes --
M apis/ec2/src/main/java/org/jclouds/ec2/util/IpPermissions.java (2)
M apis/ec2/src/test/java/org/jclouds/ec2/xml/DescribeSecurityGroupsResponseHandlerTest.java (8)
M compute/src/main/java/org/jclouds/net/domain/IpPermission.java (73)
M compute/src/main/java/org/jclouds/net/util/IpPermissions.java (25)
M compute/src/test/java/org/jclouds/net/util/IpPermissionsTest.java (39)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/567.patch
https://github.com/jclouds/jclouds/pull/567.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -128,8 +130,17 @@ public IpPermissions originatingFromCidrBlock(String cidrIp) {
> }
>
> public IpPermissions originatingFromCidrBlocks(Iterable<String> cidrIps) {
> - return new IpPermissions(getIpProtocol(), getFromPort(), getToPort(),
> - ImmutableMultimap.<String, String> of(), ImmutableSet.<String> of(), cidrIps);
> + return new IpPermissions(getIpProtocol(), getFromPort(), getToPort(), ImmutableMultimap.<String, String> of(),
> + ImmutableSet.<String> of(), cidrIps, ImmutableSet.<String> of());
> + }
> +
> + public IpPermissions originatingFromAnyButCidrBlock(String excludedCidrIp) {
Proposals for a better name for this method are welcome :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567/files#r18681345
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [e5fb0b6](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=e5fb0b607d763530647a1f97848f95158cb88ffb). Also changed the order of the tests I added to the `BaseSecurityGroupExtensionLiveTest` to avoid skipping other tests if the CIDR exclusion blocks are not supported.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58747463
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -173,6 +173,54 @@ public void testRemoveIpPermission() {
> }
>
> @Test(groups = { "integration", "live" }, singleThreaded = true, dependsOnMethods = "testRemoveIpPermission")
> + public void testAddIpPermissionWithCidrExclusionGroup() {
> + skipIfSecurityGroupsNotSupported();
> +
> + ComputeService computeService = view.getComputeService();
> +
> + Optional<SecurityGroupExtension> securityGroupExtension = computeService.getSecurityGroupExtension();
> + assertTrue(securityGroupExtension.isPresent(), "security group extension was not present");
I'm not sure why this is configured this way, but the skip method reads [this variable](https://github.com/jclouds/jclouds/blob/master/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java#L67), which is not set by checking the presence of the extension. Seems like something that can be rethought and refactored in an upcoming PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567/files#r18749900
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #187](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/187/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58593970
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Adrian Cole <no...@github.com>.
Are there any other providers that support this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58594754
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Andrew Phillips <no...@github.com>.
> + ComputeService computeService = view.getComputeService();
> +
> + Optional<SecurityGroupExtension> securityGroupExtension = computeService.getSecurityGroupExtension();
> + assertTrue(securityGroupExtension.isPresent(), "security group extension was not present");
> + if (!securityGroupExtension.get().supportsExclusionCidrBlocks()) {
> + throw new SkipException("Test cannot run without CIDR exclusion groups available.");
> + }
> +
> + Optional<SecurityGroup> optGroup = getGroup(securityGroupExtension.get());
> + assertTrue(optGroup.isPresent());
> + SecurityGroup group = optGroup.get();
> +
> + IpPermission cidrExclusionPermission = createCidrExclusionPermission();
> + Set<IpPermission> expectedPermissions = ImmutableSet.of(cidrExclusionPermission);
> +
> + SecurityGroup securityGriupWithExclusion = securityGroupExtension.get().addIpPermission(cidrExclusionPermission, group);
Name cleaned up in 3e082c8
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567/files#r18745271
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Andrew Phillips <no...@github.com>.
> @@ -217,6 +245,14 @@ public int getToPort() {
> return cidrBlocks;
> }
>
> + /**
> + * source of traffic is a all but this exclusionCidrBlocks
Just to clarify: this means that any incoming traffic that matches _one of_ the exclusion blocks should be blocked?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567/files#r18745196
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Andrew Phillips <no...@github.com>.
> @@ -279,4 +279,9 @@ public boolean supportsPortRangesForGroups() {
> return false;
> }
>
> + @Override
> + public boolean supportsExclusionCidrBlocks() {
> + return false;
> + }
> +
Indenting cleaned up in 3e082c8
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567/files#r18745270
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Andrew Phillips <no...@github.com>.
> @@ -173,6 +173,54 @@ public void testRemoveIpPermission() {
> }
>
> @Test(groups = { "integration", "live" }, singleThreaded = true, dependsOnMethods = "testRemoveIpPermission")
> + public void testAddIpPermissionWithCidrExclusionGroup() {
> + skipIfSecurityGroupsNotSupported();
> +
> + ComputeService computeService = view.getComputeService();
> +
> + Optional<SecurityGroupExtension> securityGroupExtension = computeService.getSecurityGroupExtension();
> + assertTrue(securityGroupExtension.isPresent(), "security group extension was not present");
Stupid question...do we need this if we already have `skipIfSecurityGroupsNotSupported` at the beginning?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567/files#r18745199
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-6 #191](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-6/191/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58721612
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Adrian Cole <no...@github.com>.
+1 if you tag the new methods beta. we should do a sweep to make sure this is common, but that can happen after use.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58596045
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Ignasi Barrera <no...@github.com>.
>Are there any other providers that support this? We should do a sweep to make sure this is common, but that can happen after use.
AFAIK there are no other providers that support this. However, I see value in adding this to this portable class:
Without it, it would be really hard (if not impossible) to properly implement the `SecurityGroupExtension` for CloudSigma: a rule that says `!192.168.0.0/24` should translate in our portable model into a set of rules, one for each CIDR block that is not in that range. Although this can be done (and @Kentzo from CloudSigma already came up with [an algorithm to do that](https://gist.github.com/nacx/94e39a2eb4d2dd92a1fc)), the inverse process should already be done: given a set of rules, check if they correspond to a "negation one".
This can be done, but then there is the problem that the `SecurityGroupExtension` allows you to, for example, remove a single `IpPermission`. If a "negation" rule translates into a "set" of rules, then a user could try to just remove one of them, and that makes really really hard to keep a state that is consistent with the provider.
For this reasons, I think it is a reasonable trade-off. Having this in the portable interface, even if it is not widely supported seems harmless to me. It doesn't break any existing provider, and semantically makes sense. I see it the same way as other fields in this class such as the security groups or tenants. They're not supported in a majority of providers, but without them, the `SecurityGroupExtension` couldn't be properly implemented the ones that support that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58625746
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Adrian Cole <no...@github.com>.
The harm I'm not sure you are aware of is that people supply a value to
this, and it isn't implemented. Without changing existing providers to
throw, or some algo to make it work, a user calling this could be making a
disasterous mistake in policy assumption.
That said, this is probably an already broken contract. For example there
are probably othet parts of this class that are silently ignored. But then
again.. broken windows shouldn't justify new ones.
Anyway, you make the call. If it were me, I would add a disclaimer or Beta
and who supports this on the method until we actually sort out how to
address the others. That said, I don't feel strongly enough to say don't
merge as I do see the pragmatic value in allowing one provider to escape
labs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58689192
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Adrian Cole <no...@github.com>.
doc notes or updating the supported thing to default to false on this are both perfectly adequate.
+1
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58700298
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1280](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1280/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58724016
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Ignasi Barrera <no...@github.com>.
I didn't pretend to justify adding it because there already exists a precedent.
That said, I've just seen that the `SecurityGroupExtension` already provides several methods to [check the supported features](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/extensions/SecurityGroupExtension.java#L162-L181). Perhaps it is not the best mechanism to provide a feature support matrix, but I think that adding the corresponding method to check if the exclusion groups are supported will be consistent with the existing approach and allow users to write portable code without making incorrect assumptions. We can discuss later what would be the best approach to check for supported features, and refactor the existing one, if needed.
If you are ok with this, I'll address your comments about the beta annotation and the method name, and add the corresponding "supports" method. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58699743
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Ignasi Barrera <no...@github.com>.
Changes applied. Once merged, this may require also minor changes in the downstream repos to implement the new method in the `SecurityGroupExtension`.
Also note that I've implemented with `supported = false` all existing implementations except the stub one, to make sure that the generic tests I've added to the `BaseSecurityGroupExtensionLiveTest` work as expected.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58721738
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1772](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1772/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58609728
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Adrian Cole <no...@github.com>.
> @@ -128,8 +130,17 @@ public IpPermissions originatingFromCidrBlock(String cidrIp) {
> }
>
> public IpPermissions originatingFromCidrBlocks(Iterable<String> cidrIps) {
> - return new IpPermissions(getIpProtocol(), getFromPort(), getToPort(),
> - ImmutableMultimap.<String, String> of(), ImmutableSet.<String> of(), cidrIps);
> + return new IpPermissions(getIpProtocol(), getFromPort(), getToPort(), ImmutableMultimap.<String, String> of(),
> + ImmutableSet.<String> of(), cidrIps, ImmutableSet.<String> of());
> + }
> +
> + public IpPermissions originatingFromAnyButCidrBlock(String excludedCidrIp) {
exceptOriginatingCidrBlock ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567/files#r18681736
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -217,6 +245,14 @@ public int getToPort() {
> return cidrBlocks;
> }
>
> + /**
> + * source of traffic is a all but this exclusionCidrBlocks
Yes!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567/files#r18749888
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1276](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1276/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58595745
Re: [jclouds] Allow to configure CIDR exclusion blocks (#567)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1776](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1776/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/567#issuecomment-58722123