You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Andrew Bayer <no...@github.com> on 2013/09/07 01:13:37 UTC
[jclouds] Adding a set of ports to IpPermission (#145)
This is to enable translating GCE's Firewall.Rules, which support
RangeSets of ports, into a set of ports, so that IpPermission can
accurately reflect GCE and other possible usecases.
You can merge this Pull Request by running:
git pull https://github.com/abayer/jclouds-1 ippermission-tweak-for-gce
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/145
-- Commit Summary --
* Adding a set of ports to IpPermission
-- File Changes --
M compute/src/main/java/org/jclouds/net/domain/IpPermission.java (61)
M compute/src/main/java/org/jclouds/net/util/IpPermissions.java (14)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/145.patch
https://github.com/jclouds/jclouds/pull/145.diff
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Phillips <no...@github.com>.
> - public String apply(String input) {
> - checkArgument(isCidrFormat(input),
> - "input %s is not a valid CIDR",
> - input);
> - return input;
> - }
> - }));
> + new Function<String, String>() {
> + @Override
> + public String apply(String input) {
> + checkArgument(isCidrFormat(input),
> + "input %s is not a valid CIDR",
> + input);
> + return input;
> + }
> + }));
Wow...if I'm getting this right, the "transform" simply validates each entry? In that case perhaps simply write:
```
for (String cidrBlock : cidrBlocks) {
checkArgument(isCidrFormat(cirdBlock), "%s is not a valid CIDR", cirdBlock);
}
Iterables.addAll(this.cidrBlocks, cidrBlocks);
return this;
```
? Intent seems much clearer here to me...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6228647
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
Good 'nuff. I've got a lot of work to go on this anyway, so ignore this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-23975970
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #207](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/207/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24117273
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
> - public String apply(String input) {
> - checkArgument(isCidrFormat(input),
> - "input %s is not a valid CIDR",
> - input);
> - return input;
> - }
> - }));
> + new Function<String, String>() {
> + @Override
> + public String apply(String input) {
> + checkArgument(isCidrFormat(input),
> + "input %s is not a valid CIDR",
> + input);
> + return input;
> + }
> + }));
Makes sense to me. Will do.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6250171
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #386](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/386/) 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/145#issuecomment-23975913
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Phillips <no...@github.com>.
> public IpPermission(IpProtocol ipProtocol, int fromPort, int toPort,
> - Multimap<String, String> tenantIdGroupNamePairs, Iterable<String> groupIds, Iterable<String> cidrBlocks) {
> + Multimap<String, String> tenantIdGroupNamePairs, Iterable<String> groupIds,
> + Iterable<String> cidrBlocks, RangeSet<Integer> ports) {
Also keep a backwards-compatible constructor? Or are we OK with the breaking change here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6228657
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #663](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/663/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-23975987
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #206](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/206/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24106077
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #664](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/664/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24024356
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Phillips <no...@github.com>.
> @@ -131,7 +133,8 @@ 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);
> + ImmutableMultimap.<String, String> of(), ImmutableSet.<String> of(), cidrIps,
> + ImmutableSet.<Integer> of());
Fomatting?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6228665
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Adrian Cole <no...@github.com>.
perhaps a use-case test of this? right now, it is parsing, just curious about actually using it (builder or otherwise)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24615535
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #203](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/203/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24024370
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #668](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/668/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24117474
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #202](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/202/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-23976062
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Phillips <no...@github.com>.
> @@ -241,8 +289,8 @@ public String toString() {
>
> protected ToStringHelper string() {
> return Objects.toStringHelper("").add("ipProtocol", ipProtocol).add("fromPort", fromPort).add("toPort", toPort)
> - .add("tenantIdGroupNamePairs", tenantIdGroupNamePairs).add("groupIds", groupIds).add("cidrBlocks",
> - cidrBlocks);
> + .add("tenantIdGroupNamePairs", tenantIdGroupNamePairs).add("groupIds", groupIds)
> + .add("cidrBlocks", cidrBlocks).add("ports", ports);
Formatting
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6228663
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
> @@ -163,6 +197,7 @@ public IpPermission(IpProtocol ipProtocol, int fromPort, int toPort,
> this.ipProtocol = checkNotNull(ipProtocol, "ipProtocol");
> this.groupIds = ImmutableSet.copyOf(checkNotNull(groupIds, "groupIds"));
> this.cidrBlocks = ImmutableSet.copyOf(checkNotNull(cidrBlocks, "cidrBlocks"));
> + this.ports = ports == null ? TreeRangeSet.<Integer>create() : ports;
Works for me.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6250204
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
Yeah, lobbing this for now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24613004
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
> @@ -241,8 +289,8 @@ public String toString() {
>
> protected ToStringHelper string() {
> return Objects.toStringHelper("").add("ipProtocol", ipProtocol).add("fromPort", fromPort).add("toPort", toPort)
> - .add("tenantIdGroupNamePairs", tenantIdGroupNamePairs).add("groupIds", groupIds).add("cidrBlocks",
> - cidrBlocks);
> + .add("tenantIdGroupNamePairs", tenantIdGroupNamePairs).add("groupIds", groupIds)
> + .add("cidrBlocks", cidrBlocks).add("ports", ports);
...stupid indents.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6250214
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #667](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/667/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24105817
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #665](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/665/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24025715
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #388](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/388/) 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/145#issuecomment-24025687
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #204](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/204/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24025783
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPort(Integer port) {
> + this.ports.add(singleton(checkNotNull(port, "port")));
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPortRange(Integer start, Integer end) {
> + checkState(checkNotNull(start, "start") < checkNotNull(end, "end"),
> + "start of range must be lower than end of range");
"...but was [%d, %d]", start, end)"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6228654
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Phillips <no...@github.com>.
> @@ -216,6 +251,18 @@ public int getToPort() {
> return cidrBlocks;
> }
>
> + /**
> + * Each entry must be either an integer or a range. If not specified, connections through any port are allowed.
> + * Example inputs include: ["22"], ["80,"443"], and ["12345-12349"].
> + * <p/>
> + * It is an error to specify this for any protocol that isn't UDP or TCP.
> + *
> + * @return An optional list of ports which are allowed.
What does "optional" mean here? If it means "can be null", perhaps make that explicit?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6228661
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPort(Integer port) {
> + this.ports.add(singleton(checkNotNull(port, "port")));
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPortRange(Integer start, Integer end) {
> + checkState(checkNotNull(start, "start") < checkNotNull(end, "end"),
> + "start of range must be lower than end of range");
Will do.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6250181
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
> @@ -216,6 +251,18 @@ public int getToPort() {
> return cidrBlocks;
> }
>
> + /**
> + * Each entry must be either an integer or a range. If not specified, connections through any port are allowed.
> + * Example inputs include: ["22"], ["80,"443"], and ["12345-12349"].
> + * <p/>
> + * It is an error to specify this for any protocol that isn't UDP or TCP.
> + *
> + * @return An optional list of ports which are allowed.
Will do.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6250209
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Adrian Cole <no...@github.com>.
> - "input %s is not a valid CIDR",
> - input);
> - return input;
> - }
> - }));
> + for (String cidrBlock : cidrBlocks) {
> + checkArgument(isCidrFormat(cidrBlock), "%s is not a valid CIDR", cidrBlock);
> + }
> + Iterables.addAll(this.cidrBlocks, cidrBlocks);
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPort(Integer port) {
easier to just make port an int? (removes checkNull etc)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6415048
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
> public IpPermission(IpProtocol ipProtocol, int fromPort, int toPort,
> - Multimap<String, String> tenantIdGroupNamePairs, Iterable<String> groupIds, Iterable<String> cidrBlocks) {
> + Multimap<String, String> tenantIdGroupNamePairs, Iterable<String> groupIds,
> + Iterable<String> cidrBlocks, RangeSet<Integer> ports) {
I'm fairly ok with the breaking change, since this is just a tweak of an already significant change from 1.6 anyway.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6250200
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPort(Integer port) {
> + this.ports.add(singleton(checkNotNull(port, "port")));
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPortRange(Integer start, Integer end) {
> + checkState(checkNotNull(start, "start") < checkNotNull(end, "end"),
> + "start of range must be lower than end of range");
(and I'm copying that same tweak over to GCE's Firewall.Rule, where this snippet came from)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6250324
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPort(Integer port) {
> + this.ports.add(singleton(checkNotNull(port, "port")));
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPortRange(Integer start, Integer end) {
> + checkState(checkNotNull(start, "start") < checkNotNull(end, "end"),
okiedokie.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6250176
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Bayer <no...@github.com>.
FYI, I may ditch this. I'm realizing that it may not actually be viable to squeeze GCE's Firewalls into the SecurityGroup abstraction, at which point this isn't worth doing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145#issuecomment-24120730
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Phillips <no...@github.com>.
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPort(Integer port) {
> + this.ports.add(singleton(checkNotNull(port, "port")));
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPortRange(Integer start, Integer end) {
> + checkState(checkNotNull(start, "start") < checkNotNull(end, "end"),
`checkArgument`? This doesn't seem like a case for `IllegalStateException`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6228651
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Adrian Cole <no...@github.com>.
> + Iterables.addAll(this.cidrBlocks, cidrBlocks);
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPort(Integer port) {
> + this.ports.add(singleton(checkNotNull(port, "port")));
> + return this;
> + }
> +
> + /**
> + * @see IpPermission#getPorts()
> + */
> + public Builder addPortRange(Integer start, Integer end) {
same here
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6415055
Re: [jclouds] Adding a set of ports to IpPermission (#145)
Posted by Andrew Phillips <no...@github.com>.
> @@ -163,6 +197,7 @@ public IpPermission(IpProtocol ipProtocol, int fromPort, int toPort,
> this.ipProtocol = checkNotNull(ipProtocol, "ipProtocol");
> this.groupIds = ImmutableSet.copyOf(checkNotNull(groupIds, "groupIds"));
> this.cidrBlocks = ImmutableSet.copyOf(checkNotNull(cidrBlocks, "cidrBlocks"));
> + this.ports = ports == null ? TreeRangeSet.<Integer>create() : ports;
Rather than make this nullable, how about having a legacy constructor that would pass an empty RangeSet (rather than null). Then we could hopefully use the same `copyOf` construction.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/145/files#r6228660