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/19 01:58:28 UTC
[jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
You can merge this Pull Request by running:
git pull https://github.com/abayer/jclouds-1 jclouds-287
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/152
-- Commit Summary --
* JCLOUDS-287. Add SecurityGroupExtension support to CloudStack.
-- File Changes --
M apis/cloudstack/src/main/java/org/jclouds/cloudstack/compute/config/CloudStackComputeServiceContextModule.java (17)
A apis/cloudstack/src/main/java/org/jclouds/cloudstack/compute/extensions/CloudStackSecurityGroupExtension.java (302)
M apis/cloudstack/src/main/java/org/jclouds/cloudstack/compute/functions/IngressRuleToIpPermission.java (9)
M apis/cloudstack/src/main/java/org/jclouds/cloudstack/features/SecurityGroupApi.java (1)
M apis/cloudstack/src/main/java/org/jclouds/cloudstack/predicates/SecurityGroupPredicates.java (79)
A apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/extensions/CloudStackSecurityGroupExtensionExpectTest.java (703)
A apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/extensions/CloudStackSecurityGroupExtensionLiveTest.java (43)
M apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/functions/IngressRuleToIpPermissionTest.java (2)
M apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/functions/ZoneToLocationTest.java (8)
M apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/SecurityGroupApiTest.java (2)
M apis/cloudstack/src/test/java/org/jclouds/cloudstack/predicates/SecurityGroupPredicatesTest.java (34)
A apis/cloudstack/src/test/resources/deletesecuritygroupresponse.json (1)
A apis/cloudstack/src/test/resources/getsecuritygroupresponse_extension_byid.json (1)
A apis/cloudstack/src/test/resources/getsecuritygroupresponse_extension_byid_empty.json (1)
A apis/cloudstack/src/test/resources/getsecuritygroupresponse_extension_byid_with_cidr.json (1)
A apis/cloudstack/src/test/resources/getsecuritygroupresponse_extension_byid_with_group.json (1)
A apis/cloudstack/src/test/resources/listzonesresponse_single.json (1)
A apis/cloudstack/src/test/resources/revokesecuritygroupingressresponse.json (2)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/152.patch
https://github.com/jclouds/jclouds/pull/152.diff
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> +
> + @BeforeClass(groups = { "integration", "live" })
> + public void setupContext() {
> + super.setupContext();
> +
> + CloudStackApi api = view.unwrapApi(CloudStackApi.class);
> + zone = Iterables.find(api.getZoneApi().listZones(), new Predicate<Zone>() {
> +
> + @Override
> + public boolean apply(Zone arg0) {
> + return arg0.isSecurityGroupsEnabled();
> + }
> +
> + });
> + if (zone == null)
> + securityGroupsSupported = false;
Tweaked a bit, but yeah, done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385420
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @BeforeClass(groups = { "integration", "live" })
> + public void setupContext() {
> + super.setupContext();
> +
> + CloudStackApi api = view.unwrapApi(CloudStackApi.class);
> + zone = Iterables.find(api.getZoneApi().listZones(), new Predicate<Zone>() {
> +
> + @Override
> + public boolean apply(Zone arg0) {
> + return arg0.isSecurityGroupsEnabled();
> + }
> +
> + });
> + if (zone == null)
> + securityGroupsSupported = false;
I know it's evil and all, but...
```
for (Zone zone : api.getZoneApi().listZones()) {
if (zone.isSecurityGroupsEnabled()) {
return true;
}
}
return false;
```
seems a little easier to read and understand?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377803
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #258](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/258/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-25383580
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + permBuilder.toPort(endPort);
> + permBuilder.tenantIdGroupNamePairs(tenantIdGroupNamePairs);
> + permBuilder.cidrBlocks(ipRanges);
> + permBuilder.groupIds(groupIds);
> +
> + return addIpPermission(permBuilder.build(), group);
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String id = checkNotNull(group.getId(), "group.getId()");
> +
> + org.jclouds.cloudstack.domain.SecurityGroup rawGroup = api.getSecurityGroupApi()
> + .getSecurityGroup(id);
> +
> + if (ipPermission.getCidrBlocks().size() > 0) {
`!ipPermission.getCidrBlocks().isEmpty()`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377192
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + @Override
> + public boolean supportsTenantIdGroupIdPairs() {
> + return false;
> + }
> +
> + @Override
> + public boolean supportsGroupIds() {
> + return false;
> + }
> +
> + @Override
> + public boolean supportsPortRangesForGroups() {
> + return false;
> + }
> +
> + protected Iterable<? extends org.jclouds.cloudstack.domain.SecurityGroup> pollSecurityGroups() {
Ah, it was, but now is not. Removing!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385377
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
>
> - if (securityGroupExtension.isPresent()) {
> - Optional<SecurityGroup> group = getGroup(securityGroupExtension.get());
> + if (securityGroupExtension.isPresent()) {
Should we ever have `securityGroupsSupported` but `securityGroupExtension.isAbsent()`? Or would that be a bug?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7378074
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.compute.extensions.SecurityGroupExtension;
> +import org.jclouds.compute.functions.GroupNamingConvention;
> +import org.jclouds.domain.Location;
> +import org.jclouds.net.domain.IpPermission;
> +import org.jclouds.net.domain.IpProtocol;
> +
> +import com.google.common.base.Function;
> +import com.google.common.base.Predicate;
> +import com.google.common.base.Supplier;
> +import com.google.common.cache.LoadingCache;
> +import com.google.common.collect.ImmutableSet;
> +import com.google.common.collect.Multimap;
> +
> +/**
> + * An extension to compute service to allow for the manipulation of {@link org.jclouds.compute.domain.SecurityGroup}s. Implementation
> + * is optional by providers.
"This is an optional extension whose support may vary from provider to provider." or something like that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7376691
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + for (IngressRule rule : filter(rawGroup.getIngressRules(),
> + ruleCidrMatches(ipPermission.getIpProtocol().toString(),
> + ipPermission.getFromPort(),
> + ipPermission.getToPort(),
> + ipPermission.getCidrBlocks()))) {
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
> + }
> + }
> +
> + if (ipPermission.getTenantIdGroupNamePairs().size() > 0) {
> + for (IngressRule rule : filter(rawGroup.getIngressRules(),
> + ruleGroupMatches(ipPermission.getIpProtocol().toString(),
> + ipPermission.getFromPort(),
> + ipPermission.getToPort(),
> + ipPermission.getTenantIdGroupNamePairs()))) {
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
Again, `try...catch`es around `revokeIngressRule` here and above to prevent leaving a half-deleted state?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377216
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> +
> + return getSecurityGroupById(id);
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpProtocol protocol, int startPort, int endPort,
> + Multimap<String, String> tenantIdGroupNamePairs,
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + IpPermission.Builder permBuilder = IpPermission.builder();
> + permBuilder.ipProtocol(protocol);
> + permBuilder.fromPort(startPort);
> + permBuilder.toPort(endPort);
> + permBuilder.tenantIdGroupNamePairs(tenantIdGroupNamePairs);
> + permBuilder.cidrBlocks(ipRanges);
> + permBuilder.groupIds(groupIds);
Nope, as above.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385372
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + return new Predicate<IngressRule>() {
> + @Override
> + public boolean apply(IngressRule rule) {
> + return protocol.equals(rule.getProtocol())
> + && startPort == rule.getStartPort()
> + && endPort == rule.getEndPort()
> + && accountGroupNames.containsEntry(rule.getAccount(), rule.getSecurityGroupName());
> + }
> +
> + @Override
> + public String toString() {
> + return "ruleGroupMatches(" + protocol
> + + "," + startPort
> + + "," + endPort
> + + ",[" + accountGroupNames
> + + "])";
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385396
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + IpPermission.Builder permBuilder = IpPermission.builder();
> + permBuilder.ipProtocol(protocol);
> + permBuilder.fromPort(startPort);
> + permBuilder.toPort(endPort);
> + permBuilder.tenantIdGroupNamePairs(tenantIdGroupNamePairs);
> + permBuilder.cidrBlocks(ipRanges);
> + permBuilder.groupIds(groupIds);
> +
> + return addIpPermission(permBuilder.build(), group);
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String id = checkNotNull(group.getId(), "group.getId()");
Yeah, adding the ipPermission null check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385357
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
See https://github.com/jclouds/jclouds/pull/195 for follow up changes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-27610129
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + for (Location location : locations.get()) {
> + groupCreator.invalidate(ZoneSecurityGroupNamePortsCidrs.builder()
> + .zone(location.getId())
> + .name(group.getName())
> + .build());
> + }
> +
> + return true;
> + }
> +
> + return false;
> + }
> +
> + @Override
> + public SecurityGroup addIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String id = checkNotNull(group.getId(), "group.getId()");
A group can not have a null ID. But a check for nullity of the group is a good idea.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385320
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #736](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/736/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-25663074
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> @@ -310,6 +325,7 @@ public boolean apply(SecurityGroup input) {
> // instance is still floating around in EC2. - abayer, 6/14/13
> @Test(groups = { "integration", "live" }, singleThreaded = true, dependsOnMethods = "testAddIpPermissionsFromSpec")
> public void testDeleteSecurityGroup() {
> + skipIfSecurityGroupsNotSupported();
Replace the invocations of `skipIfSecurityGroupsNotSupported` with a `@DataProvider` a la e.g. https://github.com/jclouds/jclouds/pull/175?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7378012
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
> + }
> +
> + api.getSecurityGroupApi().deleteSecurityGroup(id);
> + // TODO find something better here maybe - hard to map zones to groups
> + for (Location location : locations.get()) {
> + groupCreator.invalidate(ZoneSecurityGroupNamePortsCidrs.builder()
> + .zone(location.getId())
> + .name(group.getName())
> + .build());
> + }
> +
> + return true;
> + }
> +
> + return false;
Use above "special case" pattern?
```
if (group == null) {
return false;
}
for ...
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377070
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + String id = checkNotNull(group.getId(), "group.getId()");
> +
> + org.jclouds.cloudstack.domain.SecurityGroup rawGroup = api.getSecurityGroupApi()
> + .getSecurityGroup(id);
> +
> + if (ipPermission.getCidrBlocks().size() > 0) {
> + for (IngressRule rule : filter(rawGroup.getIngressRules(),
> + ruleCidrMatches(ipPermission.getIpProtocol().toString(),
> + ipPermission.getFromPort(),
> + ipPermission.getToPort(),
> + ipPermission.getCidrBlocks()))) {
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
> + }
> + }
> +
> + if (ipPermission.getTenantIdGroupNamePairs().size() > 0) {
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385365
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> See #195 for follow up changes.
Thanks, @abayer!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-27637576
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + for (Location location : locations.get()) {
> + groupCreator.invalidate(ZoneSecurityGroupNamePortsCidrs.builder()
> + .zone(location.getId())
> + .name(group.getName())
> + .build());
> + }
> +
> + return true;
> + }
> +
> + return false;
> + }
> +
> + @Override
> + public SecurityGroup addIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String id = checkNotNull(group.getId(), "group.getId()");
Can a group have a null ID? And do we need to check for `group` being `null` here, too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377139
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #274](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/274/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-25663057
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> +
> + return getSecurityGroupById(id);
> + }
> +
> + @Override
> + public SecurityGroup addIpPermission(IpProtocol protocol, int startPort, int endPort,
> + Multimap<String, String> tenantIdGroupNamePairs,
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + IpPermission.Builder permBuilder = IpPermission.builder();
> + permBuilder.ipProtocol(protocol);
> + permBuilder.fromPort(startPort);
> + permBuilder.toPort(endPort);
> + permBuilder.tenantIdGroupNamePairs(tenantIdGroupNamePairs);
> + permBuilder.cidrBlocks(ipRanges);
> + permBuilder.groupIds(groupIds);
Any `null` checks needed here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377173
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + return new Predicate<IngressRule>() {
> + @Override
> + public boolean apply(IngressRule rule) {
> + return protocol.equals(rule.getProtocol())
> + && startPort == rule.getStartPort()
> + && endPort == rule.getEndPort()
> + && cidrs.contains(rule.getCIDR());
> + }
> +
> + @Override
> + public String toString() {
> + return "ruleCidrMatches(" + protocol
> + + "," + startPort
> + + "," + endPort
> + + ",[" + cidrs
> + + "])";
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385393
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> +
> + return getSecurityGroupById(id);
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpProtocol protocol, int startPort, int endPort,
> + Multimap<String, String> tenantIdGroupNamePairs,
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + IpPermission.Builder permBuilder = IpPermission.builder();
> + permBuilder.ipProtocol(protocol);
> + permBuilder.fromPort(startPort);
> + permBuilder.toPort(endPort);
> + permBuilder.tenantIdGroupNamePairs(tenantIdGroupNamePairs);
> + permBuilder.cidrBlocks(ipRanges);
> + permBuilder.groupIds(groupIds);
Any `null` checks needed here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377251
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + .zone(location.getId())
> + .name(group.getName())
> + .build());
> + }
> +
> + return true;
> + }
> +
> + return false;
> + }
> +
> + @Override
> + public SecurityGroup addIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String id = checkNotNull(group.getId(), "group.getId()");
> +
> + if (ipPermission.getCidrBlocks().size() > 0) {
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385349
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + @Override
> + public boolean supportsTenantIdGroupIdPairs() {
> + return false;
> + }
> +
> + @Override
> + public boolean supportsGroupIds() {
> + return false;
> + }
> +
> + @Override
> + public boolean supportsPortRangesForGroups() {
> + return false;
> + }
> +
> + protected Iterable<? extends org.jclouds.cloudstack.domain.SecurityGroup> pollSecurityGroups() {
Stupid question, but is this used anywhere?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377640
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
>
> - if (securityGroupExtension.isPresent()) {
> - Optional<SecurityGroup> group = getGroup(securityGroupExtension.get());
> + if (securityGroupExtension.isPresent()) {
securityGroupsSupported is a CloudStack thing - so CloudStack will always have the SecurityGroupExtension, but not all CloudStack zones will support security groups. If that makes any sense.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385445
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> +import org.jclouds.compute.extensions.SecurityGroupExtension;
> +import org.jclouds.compute.functions.GroupNamingConvention;
> +import org.jclouds.domain.Location;
> +import org.jclouds.net.domain.IpPermission;
> +import org.jclouds.net.domain.IpProtocol;
> +
> +import com.google.common.base.Function;
> +import com.google.common.base.Predicate;
> +import com.google.common.base.Supplier;
> +import com.google.common.cache.LoadingCache;
> +import com.google.common.collect.ImmutableSet;
> +import com.google.common.collect.Multimap;
> +
> +/**
> + * An extension to compute service to allow for the manipulation of {@link org.jclouds.compute.domain.SecurityGroup}s. Implementation
> + * is optional by providers.
What's wrong with what's there? I can change if need be, but that's the same boilerplate I've been using on all the SG extensions...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385262
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + for (IngressRule rule : filter(rawGroup.getIngressRules(),
> + ruleCidrMatches(ipPermission.getIpProtocol().toString(),
> + ipPermission.getFromPort(),
> + ipPermission.getToPort(),
> + ipPermission.getCidrBlocks()))) {
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
> + }
> + }
> +
> + if (ipPermission.getTenantIdGroupNamePairs().size() > 0) {
> + for (IngressRule rule : filter(rawGroup.getIngressRules(),
> + ruleGroupMatches(ipPermission.getIpProtocol().toString(),
> + ipPermission.getFromPort(),
> + ipPermission.getToPort(),
> + ipPermission.getTenantIdGroupNamePairs()))) {
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
Again, still nto sure what'll get thrown. =)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385370
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> +
> + return getSecurityGroupById(id);
> + }
> +
> + @Override
> + public SecurityGroup addIpPermission(IpProtocol protocol, int startPort, int endPort,
> + Multimap<String, String> tenantIdGroupNamePairs,
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + IpPermission.Builder permBuilder = IpPermission.builder();
> + permBuilder.ipProtocol(protocol);
> + permBuilder.fromPort(startPort);
> + permBuilder.toPort(endPort);
> + permBuilder.tenantIdGroupNamePairs(tenantIdGroupNamePairs);
> + permBuilder.cidrBlocks(ipRanges);
> + permBuilder.groupIds(groupIds);
Not really - the builder itself will barf if there are bad nulls.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385354
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
Got a chance to run the live tests and they worked with a minor tweak, so we're live.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-25661767
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #232](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/232/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-24710834
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + String id = checkNotNull(group.getId(), "group.getId()");
> +
> + org.jclouds.cloudstack.domain.SecurityGroup rawGroup = api.getSecurityGroupApi()
> + .getSecurityGroup(id);
> +
> + if (ipPermission.getCidrBlocks().size() > 0) {
> + for (IngressRule rule : filter(rawGroup.getIngressRules(),
> + ruleCidrMatches(ipPermission.getIpProtocol().toString(),
> + ipPermission.getFromPort(),
> + ipPermission.getToPort(),
> + ipPermission.getCidrBlocks()))) {
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
> + }
> + }
> +
> + if (ipPermission.getTenantIdGroupNamePairs().size() > 0) {
`!ipPermission.getTenantIdGroupNamePairs().isEmpty()`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377200
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #735](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/735/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-25663075
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + return new Predicate<IngressRule>() {
> + @Override
> + public boolean apply(IngressRule rule) {
> + return protocol.equals(rule.getProtocol())
> + && startPort == rule.getStartPort()
> + && endPort == rule.getEndPort()
> + && cidrs.contains(rule.getCIDR());
> + }
> +
> + @Override
> + public String toString() {
> + return "ruleCidrMatches(" + protocol
> + + "," + startPort
> + + "," + endPort
> + + ",[" + cidrs
> + + "])";
Add the names of the fields/properties to the `toString`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377714
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + .zone(location.getId())
> + .name(group.getName())
> + .build());
> + }
> +
> + return true;
> + }
> +
> + return false;
> + }
> +
> + @Override
> + public SecurityGroup addIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String id = checkNotNull(group.getId(), "group.getId()");
> +
> + if (ipPermission.getCidrBlocks().size() > 0) {
`!ipPermission.getCidrBlocks().isEmpty()`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377151
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #693](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/693/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-24710830
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> @@ -310,6 +325,7 @@ public boolean apply(SecurityGroup input) {
> // instance is still floating around in EC2. - abayer, 6/14/13
> @Test(groups = { "integration", "live" }, singleThreaded = true, dependsOnMethods = "testAddIpPermissionsFromSpec")
> public void testDeleteSecurityGroup() {
> + skipIfSecurityGroupsNotSupported();
I...don't know how to do that or what that means. =)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385431
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #719](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/719/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152#issuecomment-25384074
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + IpPermission.Builder permBuilder = IpPermission.builder();
> + permBuilder.ipProtocol(protocol);
> + permBuilder.fromPort(startPort);
> + permBuilder.toPort(endPort);
> + permBuilder.tenantIdGroupNamePairs(tenantIdGroupNamePairs);
> + permBuilder.cidrBlocks(ipRanges);
> + permBuilder.groupIds(groupIds);
> +
> + return addIpPermission(permBuilder.build(), group);
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String id = checkNotNull(group.getId(), "group.getId()");
`null` checks for ipPermission and group?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377188
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
> + }
> +
> + api.getSecurityGroupApi().deleteSecurityGroup(id);
> + // TODO find something better here maybe - hard to map zones to groups
> + for (Location location : locations.get()) {
> + groupCreator.invalidate(ZoneSecurityGroupNamePortsCidrs.builder()
> + .zone(location.getId())
> + .name(group.getName())
> + .build());
> + }
> +
> + return true;
> + }
> +
> + return false;
Will do.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385286
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Bayer <no...@github.com>.
> + return groupConverter.apply(groupCreator.apply(zoneAndName));
> + }
> +
> + @Override
> + public boolean removeSecurityGroup(String id) {
> + checkNotNull(id, "id");
> +
> + org.jclouds.cloudstack.domain.SecurityGroup group =
> + api.getSecurityGroupApi().getSecurityGroup(id);
> +
> + if (group != null) {
> + for (IngressRule rule : group.getIngressRules()) {
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
> + }
> +
> + api.getSecurityGroupApi().deleteSecurityGroup(id);
If I could figure out what exception would get thrown by it failing...hrm...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7385311
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + return groupConverter.apply(groupCreator.apply(zoneAndName));
> + }
> +
> + @Override
> + public boolean removeSecurityGroup(String id) {
> + checkNotNull(id, "id");
> +
> + org.jclouds.cloudstack.domain.SecurityGroup group =
> + api.getSecurityGroupApi().getSecurityGroup(id);
> +
> + if (group != null) {
> + for (IngressRule rule : group.getIngressRules()) {
> + jobComplete.apply(api.getSecurityGroupApi().revokeIngressRule(rule.getId()));
> + }
> +
> + api.getSecurityGroupApi().deleteSecurityGroup(id);
Do we need some `try...catch`ing here to ensure we don't leave a half-deleted state if this call fails? Some for the `revokeIngressRule` calls above, I guess?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377107
Re: [jclouds] JCLOUDS-287. Add SecurityGroupExtension support to
CloudStack. (#152)
Posted by Andrew Phillips <no...@github.com>.
> + return new Predicate<IngressRule>() {
> + @Override
> + public boolean apply(IngressRule rule) {
> + return protocol.equals(rule.getProtocol())
> + && startPort == rule.getStartPort()
> + && endPort == rule.getEndPort()
> + && accountGroupNames.containsEntry(rule.getAccount(), rule.getSecurityGroupName());
> + }
> +
> + @Override
> + public String toString() {
> + return "ruleGroupMatches(" + protocol
> + + "," + startPort
> + + "," + endPort
> + + ",[" + accountGroupNames
> + + "])";
Add the names of the fields/properties to the `toString`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/152/files#r7377720