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/06 00:54:46 UTC
[jclouds] JCLOUDS-267. Add SecurityGroupExtension support for Nova.
(#144)
You can merge this Pull Request by running:
git pull https://github.com/abayer/jclouds-1 sg-ext-nova
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/144
-- Commit Summary --
* JCLOUDS-267. Add SecurityGroupExtension support for Nova.
-- File Changes --
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/config/NovaComputeServiceContextModule.java (14)
A apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/extensions/NovaSecurityGroupExtension.java (426)
A apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/functions/NovaSecurityGroupInZoneToSecurityGroup.java (73)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/functions/NovaSecurityGroupToSecurityGroup.java (13)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/zonescoped/SecurityGroupInZone.java (2)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/extensions/SecurityGroupApi.java (2)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/predicates/SecurityGroupPredicates.java (139)
A apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/compute/extensions/NovaSecurityGroupExtensionExpectTest.java (423)
A apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/compute/extensions/NovaSecurityGroupExtensionLiveTest.java (35)
A apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/compute/functions/NovaSecurityGroupInZoneToSecurityGroupTest.java (93)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/compute/functions/NovaSecurityGroupToSecurityGroupTest.java (89)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/predicates/SecurityGroupPredicatesTest.java (75)
A apis/openstack-nova/src/test/resources/securitygroup_details_extension.json (34)
A apis/openstack-nova/src/test/resources/securitygroup_details_extension_norules.json (10)
A apis/openstack-nova/src/test/resources/securitygroup_list_extension.json (51)
A apis/openstack-nova/src/test/resources/securitygrouprule_created_cidr.json (13)
A apis/openstack-nova/src/test/resources/securitygrouprule_created_group.json (14)
A apis/openstack-nova/src/test/resources/server_with_security_groups_extension.json (1)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/144.patch
https://github.com/jclouds/jclouds/pull/144.diff
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> +import com.google.common.base.Supplier;
> +import com.google.common.cache.LoadingCache;
> +import com.google.common.collect.ImmutableSet;
> +import com.google.common.collect.Iterables;
> +import com.google.common.collect.Multimap;
> +import com.google.common.util.concurrent.ListeningExecutorService;
> +
> +/**
> + * An extension to compute service to allow for the manipulation of {@link org.jclouds.compute.domain.SecurityGroup}s. Implementation
> + * is optional by providers.
> + *
> + * @author Andrew Bayer
> + */
> +public class NovaSecurityGroupExtension implements SecurityGroupExtension {
> +
> + protected final NovaApi client;
I have a tendency not to change them, but I'll clean this one up, since it's new and all.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353281
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Test
> + public void testApplyWithCidr() {
> +
> + NovaSecurityGroupInZoneToSecurityGroup parser = createGroupParser();
> +
> + SecurityGroupInZone origGroup = new SecurityGroupInZone(securityGroupWithCidr(), zone.getId());
> +
> + SecurityGroup newGroup = parser.apply(origGroup);
> +
> + assertEquals(newGroup.getId(), origGroup.getSecurityGroup().getId());
> + assertEquals(newGroup.getProviderId(), origGroup.getSecurityGroup().getId());
> + assertEquals(newGroup.getName(), origGroup.getSecurityGroup().getName());
> + assertEquals(newGroup.getOwnerId(), origGroup.getSecurityGroup().getTenantId());
> + assertEquals(newGroup.getIpPermissions(), ImmutableSet.copyOf(transform(origGroup.getSecurityGroup().getRules(), ruleConverter)));
> + assertEquals(newGroup.getLocation().getId(), origGroup.getZone());
assertEquals(boolean actual, boolean expected)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344802
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> +
> +
> + @Test
> + public void testApplyWithGroup() {
> + NovaSecurityGroupInZoneToSecurityGroup parser = createGroupParser();
> +
> + SecurityGroupInZone origGroup = new SecurityGroupInZone(securityGroupWithGroup(), zone.getId());
> +
> + SecurityGroup newGroup = parser.apply(origGroup);
> +
> + assertEquals(newGroup.getId(), origGroup.getSecurityGroup().getId());
> + assertEquals(newGroup.getProviderId(), origGroup.getSecurityGroup().getId());
> + assertEquals(newGroup.getName(), origGroup.getSecurityGroup().getName());
> + assertEquals(newGroup.getOwnerId(), origGroup.getSecurityGroup().getTenantId());
> + assertEquals(newGroup.getIpPermissions(), ImmutableSet.copyOf(transform(origGroup.getSecurityGroup().getRules(), ruleConverter)));
> + assertEquals(newGroup.getLocation().getId(), origGroup.getZone());
Ah, like so. Then yes, correct.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6354023
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + * @return predicate that matches endPort
> + */
> + public static Predicate<SecurityGroupRule> ruleEndPort(final int endPort) {
> + checkNotNull(endPort, "endPort must be defined");
> +
> + return new Predicate<SecurityGroupRule>() {
> + @Override
> + public boolean apply(SecurityGroupRule ext) {
> + return endPort == ext.getToPort();
> + }
> +
> + @Override
> + public String toString() {
> + return "ruleEndPort(" + endPort + ")";
> + }
> + };
Not enough duplication - I thought about it, but it felt like more hassle to have to worry about specifying which port rule was which than to have some very braindead dupe code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353495
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
I honestly have no idea what happened there. I'm gonna run that test a bunch of times locally and see if I can reproduce it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24416190
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + URI.create("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/os-security-groups/160")).headers(
> + ImmutableMultimap.<String, String> builder().put("Accept", "application/json").put("X-Auth-Token",
> + authToken).build()).build();
> +
> + HttpResponse getSecurityGroupResponse = HttpResponse.builder().statusCode(200).payload(
> + payloadFromResource("/securitygroup_details_extension.json")).build();
> +
> + Builder<HttpRequest, HttpResponse> requestResponseMap = ImmutableMap.<HttpRequest, HttpResponse> builder();
> + requestResponseMap.put(keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess);
> + requestResponseMap.put(extensionsOfNovaRequest, extensionsOfNovaResponse);
> + requestResponseMap.put(getSecurityGroup, getSecurityGroupResponse).build();
> +
> + SecurityGroupExtension extension = requestsSendResponses(requestResponseMap.build()).getSecurityGroupExtension().get();
> +
> + SecurityGroup group = extension.getSecurityGroupById(zone + "/160");
> + assertEquals("160", group.getId());
`assertEquals(boolean actual, boolean expected)`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344767
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!serverApi.isPresent() || !sgApi.isPresent()) {
> + return ImmutableSet.of();
> + }
> +
> + ServerWithSecurityGroups instance = serverApi.get().get(instanceId);
> + if (instance == null) {
> + return ImmutableSet.of();
> + }
> +
> + Set<String> groupNames = instance.getSecurityGroupNames();
> + Set<? extends SecurityGroupInZone> rawGroups =
> + sgApi.get().list().filter(nameIn(groupNames)).transform(groupToGroupInZone(zone)).toSet();
> +
> + return ImmutableSet.copyOf(transform(filter(rawGroups, notNull()), groupConverter));
OK
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353997
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + * @return predicate that matches endPort
> + */
> + public static Predicate<SecurityGroupRule> ruleEndPort(final int endPort) {
> + checkNotNull(endPort, "endPort must be defined");
> +
> + return new Predicate<SecurityGroupRule>() {
> + @Override
> + public boolean apply(SecurityGroupRule ext) {
> + return endPort == ext.getToPort();
> + }
> +
> + @Override
> + public String toString() {
> + return "ruleEndPort(" + endPort + ")";
> + }
> + };
Worth combining the start and end port rules into a "portRule", or not enough duplication to worry about that..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344745
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #691](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/691/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24617578
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #205](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/205/) UNSTABLE
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/144#issuecomment-24100984
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + @Override
> + public Set<SecurityGroup> listSecurityGroupsForNode(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
> + String zone = zoneAndId.getZone();
> + String instanceId = zoneAndId.getId();
> +
> + Optional<? extends ServerWithSecurityGroupsApi> serverApi = client.getServerWithSecurityGroupsExtensionForZone(zone);
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!serverApi.isPresent() || !sgApi.isPresent()) {
> + return ImmutableSet.of();
> + }
> +
> + ServerWithSecurityGroups instance = serverApi.get().get(instanceId);
> + if (instance == null) {
Is this a "normal" thing, or something we could/should warn about?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344639
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + ServerWithSecurityGroups instance = serverApi.get().get(instanceId);
> + if (instance == null) {
> + return ImmutableSet.of();
> + }
> +
> + Set<String> groupNames = instance.getSecurityGroupNames();
> + Set<? extends SecurityGroupInZone> rawGroups =
> + sgApi.get().list().filter(nameIn(groupNames)).transform(groupToGroupInZone(zone)).toSet();
> +
> + return ImmutableSet.copyOf(transform(filter(rawGroups, notNull()), groupConverter));
> + }
> +
> + @Override
> + public SecurityGroup getSecurityGroupById(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353367
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #666 UNSTABLE
> jclouds-pull-requests #205 UNSTABLE
These both look unrelated.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24221424
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #681](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/681/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24420374
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + return null;
> + }
> +
> + SecurityGroupInZone rawGroup = new SecurityGroupInZone(sgApi.get().get(groupId), zone);
> +
> + return groupConverter.apply(rawGroup);
> + }
> +
> + @Override
> + public SecurityGroup createSecurityGroup(String name, Location location) {
> + String zone = location.getId();
> + if (zone != null) {
> + return createSecurityGroup(name, zone);
> + } else {
> + return null;
> + }
'k.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353369
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #220](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/220/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24420317
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + payloadFromResource("/securitygroup_list.json")).build();
> +
> +
> + Builder<HttpRequest, HttpResponse> requestResponseMap = ImmutableMap.<HttpRequest, HttpResponse> builder();
> + requestResponseMap.put(keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess);
> + requestResponseMap.put(extensionsOfNovaRequest, extensionsOfNovaResponse);
> + requestResponseMap.put(list, listResponse).build();
> +
> + SecurityGroupExtension extension = requestsSendResponses(requestResponseMap.build()).getSecurityGroupExtension().get();
> +
> + Set<SecurityGroup> groups = extension.listSecurityGroupsInLocation(new LocationBuilder()
> + .scope(LocationScope.ZONE)
> + .id(zone)
> + .description("zone")
> + .build());
> + assertEquals(1, groups.size());
`assertEquals(boolean actual, boolean expected)`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344760
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!serverApi.isPresent() || !sgApi.isPresent()) {
> + return ImmutableSet.of();
> + }
> +
> + ServerWithSecurityGroups instance = serverApi.get().get(instanceId);
> + if (instance == null) {
> + return ImmutableSet.of();
> + }
> +
> + Set<String> groupNames = instance.getSecurityGroupNames();
> + Set<? extends SecurityGroupInZone> rawGroups =
> + sgApi.get().list().filter(nameIn(groupNames)).transform(groupToGroupInZone(zone)).toSet();
> +
> + return ImmutableSet.copyOf(transform(filter(rawGroups, notNull()), groupConverter));
Not sure how much value that'd provide over what it's got there already - in some cases, it's FluentIterable stuff, and so is going to be in its own little world anyway.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353348
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + groupId);
> + }
> + }
> +
> + return getSecurityGroupById(ZoneAndId.fromZoneAndId(zone, id).slashEncode());
> + }
> +
> + @Override
> + public SecurityGroup addIpPermission(IpProtocol protocol, int startPort, int endPort,
> + Multimap<String, String> tenantIdGroupNamePairs,
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + String zone = group.getLocation().getId();
> + String id = group.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
No null check for `zone`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344682
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> So I have one theory that the @Named(TIMEOUT_SECURITYGROUP_PRESENT) is causing problems
Interesting! Could that indeed cause only test failures in *one* of three runs?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24418134
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + return null;
> + }
> +
> + SecurityGroupInZone rawGroup = new SecurityGroupInZone(sgApi.get().get(groupId), zone);
> +
> + return groupConverter.apply(rawGroup);
> + }
> +
> + @Override
> + public SecurityGroup createSecurityGroup(String name, Location location) {
> + String zone = location.getId();
> + if (zone != null) {
> + return createSecurityGroup(name, zone);
> + } else {
> + return null;
> + }
As in places above, use
```
if (zone == null) {
return null;
}
return createSecurityGroup(name, zone);
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344657
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #678](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/678/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24414431
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + protected Function<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup, SecurityGroup> baseConverter;
> + protected final Supplier<Map<String, Location>> locationIndex;
> +
> + @Inject
> + public NovaSecurityGroupInZoneToSecurityGroup(Function<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup, SecurityGroup> baseConverter,
> + Supplier<Map<String, Location>> locationIndex) {
> + this.baseConverter = checkNotNull(baseConverter, "baseConverter");
> + this.locationIndex = checkNotNull(locationIndex, "locationIndex");
> + }
> +
> + @Override
> + public SecurityGroup apply(SecurityGroupInZone group) {
> + SecurityGroupBuilder builder = SecurityGroupBuilder.fromSecurityGroup(baseConverter.apply(group.getSecurityGroup()));
> +
> + Location zone = locationIndex.get().get(group.getZone());
> + checkState(zone != null, "location %s not in locationIndex: %s", group.getZone(), locationIndex.get());
`checkArgument` or `checkNotNull`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344711
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-pull-requests #217 UNSTABLE
Timing related [test failure](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/org.apache.jclouds.api$openstack-nova/217/testReport/junit/org.jclouds.openstack.nova.v2_0.compute/NovaComputeServiceExpectTest/testCreateNodeWithGeneratedKeyPair/). Don't think it's related, but since it's in Nova, could you just take a look, @abayer ?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24415092
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + @Override
> + public SecurityGroup getSecurityGroupById(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
> + String zone = zoneAndId.getZone();
> + String groupId = zoneAndId.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!sgApi.isPresent()) {
> + return null;
> + }
> +
> + SecurityGroupInZone rawGroup = new SecurityGroupInZone(sgApi.get().get(groupId), zone);
> +
> + return groupConverter.apply(rawGroup);
Let's leave it then ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353984
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> +
> + assertNotNull(origGroup);
> + SecurityGroup newGroup = extension.addIpPermission(IpProtocol.TCP,
> + 22,
> + 22,
> + emptyMultimap(),
> + emptyStringSet(),
> + ImmutableSet.of("admin/11111"),
> + origGroup);
> +
> + assertNotNull(newGroup);
> + }
> +
> + private Multimap<String, String> emptyMultimap() {
> + return LinkedHashMultimap.create();
> + }
Any need for these methods, rather than just calling LinkedHashMultimap.create() directly?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344791
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #662](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/662/) UNSTABLE
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/144#issuecomment-23909932
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> public void testnameEqualsWhenEqual() {
> - assert nameEquals("jclouds").apply(ref);
> + assertTrue(nameEquals("jclouds").apply(ref));
Just something to give more overview in the logs? E.g.
```
assertTrue(nameEquals("jclouds").apply(ref), "Expected 'jclouds' as the name of " + ref);
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6354085
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
So I have one theory that the @Named(TIMEOUT_SECURITYGROUP_PRESENT) is causing problems - it's being reused to describe two different things. So I'm gonna push a change that names the securityGroupEventualConsistencyDelay something else.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24416768
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + payloadFromResource("/securitygroup_list_extension.json")).build();
> +
> + Builder<HttpRequest, HttpResponse> requestResponseMap = ImmutableMap.<HttpRequest, HttpResponse> builder();
> + requestResponseMap.put(keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess);
> + requestResponseMap.put(extensionsOfNovaRequest, extensionsOfNovaResponse);
> + requestResponseMap.put(create, createResponse);
> + requestResponseMap.put(list, listResponse).build();
> +
> + SecurityGroupExtension extension = requestsSendResponses(requestResponseMap.build()).getSecurityGroupExtension().get();
> +
> + SecurityGroup group = extension.createSecurityGroup("test", new LocationBuilder()
> + .scope(LocationScope.ZONE)
> + .id(zone)
> + .description("zone")
> + .build());
> + assertEquals("160", group.getId());
`assertEquals(boolean actual, boolean expected)`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344770
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + return ImmutableSet.of();
> + }
> + return listSecurityGroupsInLocation(zone);
> + }
> +
> + public Set<SecurityGroup> listSecurityGroupsInLocation(String zone) {
> + Iterable<? extends SecurityGroupInZone> rawGroups = pollSecurityGroupsByZone(zone);
> + Iterable<SecurityGroup> groups = transform(filter(rawGroups, notNull()),
> + groupConverter);
> + return ImmutableSet.copyOf(groups);
> + }
> +
> + @Override
> + public Set<SecurityGroup> listSecurityGroupsForNode(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
[minor] Combine these two lines?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344630
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.inject.Inject;
> +
> +
> +/**
> + * A function for transforming a Nova-specific SecurityGroup into a generic
> + * SecurityGroup object.
> + *
> + * @author Andrew Bayer
> + */
> +@Singleton
> +public class NovaSecurityGroupInZoneToSecurityGroup implements Function<SecurityGroupInZone, SecurityGroup> {
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + protected Function<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup, SecurityGroup> baseConverter;
`final`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344706
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #201](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/201/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-23910075
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
Looks happy enough. +1 from me - we can keep an eye out on Nova test failures.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24439372
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> +
> + assertNotNull(origGroup);
> + SecurityGroup newGroup = extension.addIpPermission(IpProtocol.TCP,
> + 22,
> + 22,
> + emptyMultimap(),
> + emptyStringSet(),
> + ImmutableSet.of("admin/11111"),
> + origGroup);
> +
> + assertNotNull(newGroup);
> + }
> +
> + private Multimap<String, String> emptyMultimap() {
> + return LinkedHashMultimap.create();
> + }
Oh, wait, no, now I remember - LinkedHashMultimap.<String,String>create() and its ilk everywhere annoyed me. Mind if I keep it as is?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353602
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> +import com.google.inject.Inject;
> +
> +
> +/**
> + * A function for transforming a Nova-specific SecurityGroup into a generic
> + * SecurityGroup object.
> + *
> + * @author Andrew Bayer
> + */
> +@Singleton
> +public class NovaSecurityGroupInZoneToSecurityGroup implements Function<SecurityGroupInZone, SecurityGroup> {
> + @Resource
> + @Named(ComputeServiceConstants.COMPUTE_LOGGER)
> + protected Logger logger = Logger.NULL;
> +
> + protected Function<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup, SecurityGroup> baseConverter;
duh, yeah. finalizing!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353439
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + ImmutableMultimap.<String, String> builder().put("Accept", "application/json").put("X-Auth-Token",
> + authToken).build()).build();
> +
> + HttpResponse listResponse = HttpResponse.builder().statusCode(200).payload(
> + payloadFromResource("/securitygroup_list.json")).build();
> +
> +
> + Builder<HttpRequest, HttpResponse> requestResponseMap = ImmutableMap.<HttpRequest, HttpResponse> builder();
> + requestResponseMap.put(keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess);
> + requestResponseMap.put(extensionsOfNovaRequest, extensionsOfNovaResponse);
> + requestResponseMap.put(list, listResponse).build();
> +
> + SecurityGroupExtension extension = requestsSendResponses(requestResponseMap.build()).getSecurityGroupExtension().get();
> +
> + Set<SecurityGroup> groups = extension.listSecurityGroups();
> + assertEquals(1, groups.size());
TestNG uses `assertEquals(boolean actual, boolean expected)`, so `assertEquals(groups.size(), 1);`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344757
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> +
> + }
> + }
> +
> + return getSecurityGroupById(ZoneAndId.fromZoneAndId(zone, id).slashEncode());
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpProtocol protocol, int startPort, int endPort,
> + Multimap<String, String> tenantIdGroupNamePairs,
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + String zone = group.getLocation().getId();
> + String id = group.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
*whistles innocently*
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353437
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + String groupId = zoneAndId.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!sgApi.isPresent()) {
> + return false;
> + }
> +
> + if (sgApi.get().get(groupId) != null) {
> + sgApi.get().delete(groupId);
> + // TODO: test this clear happens
> + groupCreator.invalidate(new ZoneSecurityGroupNameAndPorts(zone, groupId, ImmutableSet.<Integer> of()));
> + return true;
> + }
> +
> + return false;
Use same pattern as above?
```
if (... == null) {
// do null case
}
// do non-null case
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344668
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + protected Function<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup, SecurityGroup> baseConverter;
> + protected final Supplier<Map<String, Location>> locationIndex;
> +
> + @Inject
> + public NovaSecurityGroupInZoneToSecurityGroup(Function<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup, SecurityGroup> baseConverter,
> + Supplier<Map<String, Location>> locationIndex) {
> + this.baseConverter = checkNotNull(baseConverter, "baseConverter");
> + this.locationIndex = checkNotNull(locationIndex, "locationIndex");
> + }
> +
> + @Override
> + public SecurityGroup apply(SecurityGroupInZone group) {
> + SecurityGroupBuilder builder = SecurityGroupBuilder.fromSecurityGroup(baseConverter.apply(group.getSecurityGroup()));
> +
> + Location zone = locationIndex.get().get(group.getZone());
> + checkState(zone != null, "location %s not in locationIndex: %s", group.getZone(), locationIndex.get());
the checkState seemed to be the convention in other converters, so I kept it here. Totally open to changing it, but not sure what the alternative would be.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353457
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + URI.create("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/os-security-groups/160")).headers(
> + ImmutableMultimap.<String, String> builder().put("Accept", "application/json").put("X-Auth-Token",
> + authToken).build()).build();
> +
> + HttpResponse getSecurityGroupResponse = HttpResponse.builder().statusCode(200).payload(
> + payloadFromResource("/securitygroup_details_extension.json")).build();
> +
> + Builder<HttpRequest, HttpResponse> requestResponseMap = ImmutableMap.<HttpRequest, HttpResponse> builder();
> + requestResponseMap.put(keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess);
> + requestResponseMap.put(extensionsOfNovaRequest, extensionsOfNovaResponse);
> + requestResponseMap.put(getSecurityGroup, getSecurityGroupResponse);
> + requestResponseMap.put(delete, deleteResponse).build();
> +
> + SecurityGroupExtension extension = requestsSendResponses(requestResponseMap.build()).getSecurityGroupExtension().get();
> +
> + assertTrue(extension.removeSecurityGroup(zone + "/160"));
okiedokie
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353551
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #217](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/217/) UNSTABLE
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/144#issuecomment-24414424
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> +
> + }
> + }
> +
> + return getSecurityGroupById(ZoneAndId.fromZoneAndId(zone, id).slashEncode());
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpProtocol protocol, int startPort, int endPort,
> + Multimap<String, String> tenantIdGroupNamePairs,
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + String zone = group.getLocation().getId();
> + String id = group.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
No null check for `zone`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344690
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> +
> + assertNotNull(origGroup);
> + SecurityGroup newGroup = extension.addIpPermission(IpProtocol.TCP,
> + 22,
> + 22,
> + emptyMultimap(),
> + emptyStringSet(),
> + ImmutableSet.of("admin/11111"),
> + origGroup);
> +
> + assertNotNull(newGroup);
> + }
> +
> + private Multimap<String, String> emptyMultimap() {
> + return LinkedHashMultimap.create();
> + }
Of course not!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6354010
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + assert ruleStartPort(10).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleStartPortWhenNotEqual() {
> + assert !ruleStartPort(50).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleEndPortWhenEqual() {
> + assert ruleEndPort(20).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleEndPortWhenNotEqual() {
> + assert !ruleEndPort(50).apply(ruleRef);
Though I don't really have a good message for 'em. So I'll leave that out.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353674
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + @Override
> + public SecurityGroup getSecurityGroupById(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
> + String zone = zoneAndId.getZone();
> + String groupId = zoneAndId.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!sgApi.isPresent()) {
> + return null;
> + }
> +
> + SecurityGroupInZone rawGroup = new SecurityGroupInZone(sgApi.get().get(groupId), zone);
> +
> + return groupConverter.apply(rawGroup);
Is "groupInZone2group" or something a bit more descriptive than "groupConverter"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344647
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + sgApi.get().delete(groupId);
> + // TODO: test this clear happens
> + groupCreator.invalidate(new ZoneSecurityGroupNameAndPorts(zone, groupId, ImmutableSet.<Integer> of()));
> + return true;
> + }
> +
> + return false;
> + }
> +
> +
> + @Override
> + public SecurityGroup addIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String zone = group.getLocation().getId();
> + String id = group.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
The only way it could end up null is if you pass a SecurityGroup created from another cloud than Nova - any Nova-created SG is gonna have the zone set up.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353422
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + protected Function<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup, SecurityGroup> baseConverter;
> + protected final Supplier<Map<String, Location>> locationIndex;
> +
> + @Inject
> + public NovaSecurityGroupInZoneToSecurityGroup(Function<org.jclouds.openstack.nova.v2_0.domain.SecurityGroup, SecurityGroup> baseConverter,
> + Supplier<Map<String, Location>> locationIndex) {
> + this.baseConverter = checkNotNull(baseConverter, "baseConverter");
> + this.locationIndex = checkNotNull(locationIndex, "locationIndex");
> + }
> +
> + @Override
> + public SecurityGroup apply(SecurityGroupInZone group) {
> + SecurityGroupBuilder builder = SecurityGroupBuilder.fromSecurityGroup(baseConverter.apply(group.getSecurityGroup()));
> +
> + Location zone = locationIndex.get().get(group.getZone());
> + checkState(zone != null, "location %s not in locationIndex: %s", group.getZone(), locationIndex.get());
Ah, I guess it's not an input item in this case, so can see the point here...thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353976
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #230](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/230/) UNSTABLE
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/144#issuecomment-24617508
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
I'm gonna be nervous until I'm sure - it's just close enough to stuff that I did add that I'd like to be sure.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24416442
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + ImmutableMultimap.<String, String> builder().put("Accept", "application/json").put("X-Auth-Token",
> + authToken).build()).build();
> +
> + HttpResponse listResponse = HttpResponse.builder().statusCode(200).payload(
> + payloadFromResource("/securitygroup_list.json")).build();
> +
> +
> + Builder<HttpRequest, HttpResponse> requestResponseMap = ImmutableMap.<HttpRequest, HttpResponse> builder();
> + requestResponseMap.put(keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess);
> + requestResponseMap.put(extensionsOfNovaRequest, extensionsOfNovaResponse);
> + requestResponseMap.put(list, listResponse).build();
> +
> + SecurityGroupExtension extension = requestsSendResponses(requestResponseMap.build()).getSecurityGroupExtension().get();
> +
> + Set<SecurityGroup> groups = extension.listSecurityGroups();
> + assertEquals(1, groups.size());
whoopsie. Fixing all o' these.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353504
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + @Override
> + public SecurityGroup getSecurityGroupById(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
> + String zone = zoneAndId.getZone();
> + String groupId = zoneAndId.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!sgApi.isPresent()) {
> + return null;
> + }
> +
> + SecurityGroupInZone rawGroup = new SecurityGroupInZone(sgApi.get().get(groupId), zone);
> +
> + return groupConverter.apply(rawGroup);
I was going with convention - I use groupConverter in the other SecurityGroupExtension implementations. But I'm flexible - up to you.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353360
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
I'm just wondering if it might have somehow created a race condition where some other test rewrites it depending on ordering or something like that. Eh, we'll see.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24419126
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> +
> + assertNotNull(origGroup);
> + SecurityGroup newGroup = extension.addIpPermission(IpProtocol.TCP,
> + 22,
> + 22,
> + emptyMultimap(),
> + emptyStringSet(),
> + ImmutableSet.of("admin/11111"),
> + origGroup);
> +
> + assertNotNull(newGroup);
> + }
> +
> + private Multimap<String, String> emptyMultimap() {
> + return LinkedHashMultimap.create();
> + }
Probably not. I think I just got annoyed at line lengths at one point. Undoing!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353568
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + String groupId = zoneAndId.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!sgApi.isPresent()) {
> + return false;
> + }
> +
> + if (sgApi.get().get(groupId) != null) {
> + sgApi.get().delete(groupId);
> + // TODO: test this clear happens
> + groupCreator.invalidate(new ZoneSecurityGroupNameAndPorts(zone, groupId, ImmutableSet.<Integer> of()));
> + return true;
> + }
> +
> + return false;
...I can never remember the preferred pattern, y'see... =) Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353397
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.common.base.Supplier;
> +import com.google.common.cache.LoadingCache;
> +import com.google.common.collect.ImmutableSet;
> +import com.google.common.collect.Iterables;
> +import com.google.common.collect.Multimap;
> +import com.google.common.util.concurrent.ListeningExecutorService;
> +
> +/**
> + * An extension to compute service to allow for the manipulation of {@link org.jclouds.compute.domain.SecurityGroup}s. Implementation
> + * is optional by providers.
> + *
> + * @author Andrew Bayer
> + */
> +public class NovaSecurityGroupExtension implements SecurityGroupExtension {
> +
> + protected final NovaApi client;
Are we calling these "client" or "api" now..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344610
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + @Override
> + public Set<SecurityGroup> listSecurityGroupsForNode(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
> + String zone = zoneAndId.getZone();
> + String instanceId = zoneAndId.getId();
> +
> + Optional<? extends ServerWithSecurityGroupsApi> serverApi = client.getServerWithSecurityGroupsExtensionForZone(zone);
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!serverApi.isPresent() || !sgApi.isPresent()) {
> + return ImmutableSet.of();
> + }
> +
> + ServerWithSecurityGroups instance = serverApi.get().get(instanceId);
> + if (instance == null) {
I'm not sure. I think we don't need to bother warning, since if someone's trying to open ports on a cloud that doesn't have security groups, well, they don't need to worry about opening ports. =)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353332
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
> +
> + if (!serverApi.isPresent() || !sgApi.isPresent()) {
> + return ImmutableSet.of();
> + }
> +
> + ServerWithSecurityGroups instance = serverApi.get().get(instanceId);
> + if (instance == null) {
> + return ImmutableSet.of();
> + }
> +
> + Set<String> groupNames = instance.getSecurityGroupNames();
> + Set<? extends SecurityGroupInZone> rawGroups =
> + sgApi.get().list().filter(nameIn(groupNames)).transform(groupToGroupInZone(zone)).toSet();
> +
> + return ImmutableSet.copyOf(transform(filter(rawGroups, notNull()), groupConverter));
The "filter-then-convert" pattern appears in a couple of places...factor out into a method or function?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344642
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + @Override
> + public String toString() {
> + return "cidr(" + cidr + ")";
> + }
> + };
> + }
> +
> + /**
> + * matches a security group rule by the security group it allows
> + *
> + * @param group
> + * @return predicate that matches group
> + */
> + public static Predicate<SecurityGroupRule> ruleGroup(final String group) {
Rename to `groupName` - makes the comparison with getGroup().getName() clearer
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344730
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + authToken).build()).build();
> +
> + HttpResponse listResponse = HttpResponse.builder().statusCode(200).payload(
> + payloadFromResource("/securitygroup_list.json")).build();
> +
> +
> + Builder<HttpRequest, HttpResponse> requestResponseMap = ImmutableMap.<HttpRequest, HttpResponse> builder();
> + requestResponseMap.put(keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess);
> + requestResponseMap.put(extensionsOfNovaRequest, extensionsOfNovaResponse);
> + requestResponseMap.put(serverReq, serverResponse);
> + requestResponseMap.put(list, listResponse).build();
> +
> + SecurityGroupExtension extension = requestsSendResponses(requestResponseMap.build()).getSecurityGroupExtension().get();
> +
> + Set<SecurityGroup> groups = extension.listSecurityGroupsForNode(zone + "/8d0a6ca5-8849-4b3d-b86e-f24c92490ebb");
> + assertEquals(1, groups.size());
`assertEquals(boolean actual, boolean expected)`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344762
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + groupId);
> + }
> + }
> +
> + return getSecurityGroupById(ZoneAndId.fromZoneAndId(zone, id).slashEncode());
> + }
> +
> + @Override
> + public SecurityGroup addIpPermission(IpProtocol protocol, int startPort, int endPort,
> + Multimap<String, String> tenantIdGroupNamePairs,
> + Iterable<String> ipRanges,
> + Iterable<String> groupIds, SecurityGroup group) {
> + String zone = group.getLocation().getId();
> + String id = group.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
See above.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353429
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + assert ruleStartPort(10).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleStartPortWhenNotEqual() {
> + assert !ruleStartPort(50).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleEndPortWhenEqual() {
> + assert ruleEndPort(20).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleEndPortWhenNotEqual() {
> + assert !ruleEndPort(50).apply(ruleRef);
Why `assert` here rather than `assertTrue(..., message)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344808
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + .fromPort(startPort)
> + .toPort(endPort)
> + .build(),
> + groupId);
> + }
> + }
> +
> + return getSecurityGroupById(ZoneAndId.fromZoneAndId(zone, id).slashEncode());
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String zone = group.getLocation().getId();
> + String id = group.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
Above. =)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353431
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> +
> +
> + @Test
> + public void testApplyWithGroup() {
> + NovaSecurityGroupInZoneToSecurityGroup parser = createGroupParser();
> +
> + SecurityGroupInZone origGroup = new SecurityGroupInZone(securityGroupWithGroup(), zone.getId());
> +
> + SecurityGroup newGroup = parser.apply(origGroup);
> +
> + assertEquals(newGroup.getId(), origGroup.getSecurityGroup().getId());
> + assertEquals(newGroup.getProviderId(), origGroup.getSecurityGroup().getId());
> + assertEquals(newGroup.getName(), origGroup.getSecurityGroup().getName());
> + assertEquals(newGroup.getOwnerId(), origGroup.getSecurityGroup().getTenantId());
> + assertEquals(newGroup.getIpPermissions(), ImmutableSet.copyOf(transform(origGroup.getSecurityGroup().getRules(), ruleConverter)));
> + assertEquals(newGroup.getLocation().getId(), origGroup.getZone());
assertEquals(boolean actual, boolean expected)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344801
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + .fromPort(startPort)
> + .toPort(endPort)
> + .build(),
> + groupId);
> + }
> + }
> +
> + return getSecurityGroupById(ZoneAndId.fromZoneAndId(zone, id).slashEncode());
> + }
> +
> + @Override
> + public SecurityGroup removeIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String zone = group.getLocation().getId();
> + String id = group.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
No null check for `zone`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344684
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #666](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/666/) UNSTABLE
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/144#issuecomment-24100166
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> +
> +
> + @Test
> + public void testApplyWithGroup() {
> + NovaSecurityGroupInZoneToSecurityGroup parser = createGroupParser();
> +
> + SecurityGroupInZone origGroup = new SecurityGroupInZone(securityGroupWithGroup(), zone.getId());
> +
> + SecurityGroup newGroup = parser.apply(origGroup);
> +
> + assertEquals(newGroup.getId(), origGroup.getSecurityGroup().getId());
> + assertEquals(newGroup.getProviderId(), origGroup.getSecurityGroup().getId());
> + assertEquals(newGroup.getName(), origGroup.getSecurityGroup().getName());
> + assertEquals(newGroup.getOwnerId(), origGroup.getSecurityGroup().getTenantId());
> + assertEquals(newGroup.getIpPermissions(), ImmutableSet.copyOf(transform(origGroup.getSecurityGroup().getRules(), ruleConverter)));
> + assertEquals(newGroup.getLocation().getId(), origGroup.getZone());
Those are right, aren't they? newGroup's the actual, origGroup's the expected.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353634
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + sgApi.get().delete(groupId);
> + // TODO: test this clear happens
> + groupCreator.invalidate(new ZoneSecurityGroupNameAndPorts(zone, groupId, ImmutableSet.<Integer> of()));
> + return true;
> + }
> +
> + return false;
> + }
> +
> +
> + @Override
> + public SecurityGroup addIpPermission(IpPermission ipPermission, SecurityGroup group) {
> + String zone = group.getLocation().getId();
> + String id = group.getId();
> +
> + Optional<? extends SecurityGroupApi> sgApi = client.getSecurityGroupExtensionForZone(zone);
No null check for `zone` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344676
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + ServerWithSecurityGroups instance = serverApi.get().get(instanceId);
> + if (instance == null) {
> + return ImmutableSet.of();
> + }
> +
> + Set<String> groupNames = instance.getSecurityGroupNames();
> + Set<? extends SecurityGroupInZone> rawGroups =
> + sgApi.get().list().filter(nameIn(groupNames)).transform(groupToGroupInZone(zone)).toSet();
> +
> + return ImmutableSet.copyOf(transform(filter(rawGroups, notNull()), groupConverter));
> + }
> +
> + @Override
> + public SecurityGroup getSecurityGroupById(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
[minor] combine the two lines?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344649
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> + URI.create("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/os-security-groups/160")).headers(
> + ImmutableMultimap.<String, String> builder().put("Accept", "application/json").put("X-Auth-Token",
> + authToken).build()).build();
> +
> + HttpResponse getSecurityGroupResponse = HttpResponse.builder().statusCode(200).payload(
> + payloadFromResource("/securitygroup_details_extension.json")).build();
> +
> + Builder<HttpRequest, HttpResponse> requestResponseMap = ImmutableMap.<HttpRequest, HttpResponse> builder();
> + requestResponseMap.put(keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess);
> + requestResponseMap.put(extensionsOfNovaRequest, extensionsOfNovaResponse);
> + requestResponseMap.put(getSecurityGroup, getSecurityGroupResponse);
> + requestResponseMap.put(delete, deleteResponse).build();
> +
> + SecurityGroupExtension extension = requestsSendResponses(requestResponseMap.build()).getSecurityGroupExtension().get();
> +
> + assertTrue(extension.removeSecurityGroup(zone + "/160"));
`assertTrue(..., "Expected removeSecurityGroup to be successful")`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6344777
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + assert ruleStartPort(10).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleStartPortWhenNotEqual() {
> + assert !ruleStartPort(50).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleEndPortWhenEqual() {
> + assert ruleEndPort(20).apply(ruleRef);
> + }
> +
> + @Test
> + public void testRuleEndPortWhenNotEqual() {
> + assert !ruleEndPort(50).apply(ruleRef);
'cos I cut and pasted, I think. Changing to assertTrue.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353644
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds #383](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/383/) 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/144#issuecomment-23909795
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + }
> +
> + @Override
> + public String toString() {
> + return "cidr(" + cidr + ")";
> + }
> + };
> + }
> +
> + /**
> + * matches a security group rule by the security group it allows
> + *
> + * @param group
> + * @return predicate that matches group
> + */
> + public static Predicate<SecurityGroupRule> ruleGroup(final String group) {
done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353463
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Bayer <no...@github.com>.
> + return ImmutableSet.of();
> + }
> + return listSecurityGroupsInLocation(zone);
> + }
> +
> + public Set<SecurityGroup> listSecurityGroupsInLocation(String zone) {
> + Iterable<? extends SecurityGroupInZone> rawGroups = pollSecurityGroupsByZone(zone);
> + Iterable<SecurityGroup> groups = transform(filter(rawGroups, notNull()),
> + groupConverter);
> + return ImmutableSet.copyOf(groups);
> + }
> +
> + @Override
> + public Set<SecurityGroup> listSecurityGroupsForNode(String id) {
> + checkNotNull(id, "id");
> + ZoneAndId zoneAndId = ZoneAndId.fromSlashEncoded(id);
Done.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144/files#r6353315
Re: [jclouds] JCLOUDS-267. Add SecurityGroupExtension support for
Nova. (#144)
Posted by Andrew Phillips <no...@github.com>.
> I'm gonna run that test a bunch of times locally and see if I can reproduce it.
>From my perspective, that shouldn't be necessary. As long as we feel that code isn't affected by this change, I'll take this as one of our usual timing-related test failures.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/144#issuecomment-24416311