You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by geomacy <gi...@git.apache.org> on 2016/11/10 15:30:04 UTC
[GitHub] brooklyn-server pull request #425: Do not merge; for review only; network ex...
GitHub user geomacy opened a pull request:
https://github.com/apache/brooklyn-server/pull/425
Do not merge; for review only; network experiments.
Some experimental code relating to a proposal to add security 'networks' into Brooklyn.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/geomacy/brooklyn-server network-investigations
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/425.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #425
----
commit d1ba1f2e11093a2c9486a396aed059aad496ee01
Author: Geoff Macartney <ge...@cloudsoftcorp.com>
Date: 2016-11-08T21:37:48Z
WIP
commit db25519f5aebedb636e4f6761fae8174f0749ff7
Author: Geoff Macartney <ge...@cloudsoftcorp.com>
Date: 2016-11-10T12:19:39Z
Add JcloudsLocationSecurityGroupEditor utility.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r89302809
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -219,60 +226,62 @@ private SecurityGroup getSecurityGroup(final String nodeId, final SecurityGroupE
* the changes may not be picked up by later customizations, meaning the same rule could possibly be
* added twice, which would fail. A finer grained mechanism would be preferable here, but
* we have no access to the information required, so this brute force serializing is required.
+ * TODO investigate whether this can be improved. Can the synchronization be moved to
+ * the class org.apache.brooklyn.location.jclouds.networking.SecurityGroupEditor?
*
* @param location Location to gain permissions
* @param permissions The set of permissions to be applied to the location
*/
- public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
- ComputeService computeService = location.getParent().getComputeService();
- addPermissionsToLocationAndReturnSecurityGroup(computeService, location, permissions);
+ public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsMachineLocation location,
+ final Iterable<IpPermission> permissions) {
+ addPermissionsToLocationAndReturnSecurityGroup(location, permissions);
return this;
}
- public Collection<SecurityGroup> addPermissionsToLocationAndReturnSecurityGroup(ComputeService computeService, final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+ public Collection<SecurityGroup> addPermissionsToLocationAndReturnSecurityGroup(
+ final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+
synchronized (JcloudsLocationSecurityGroupCustomizer.class) {
- String nodeId = location.getNode().getId();
- return addPermissionsToLocation(permissions, nodeId, computeService).values();
+ return addPermissionsInternal(permissions, location).values();
}
}
+
/**
- * Removes the given security group permissions from the given node with the given compute service.
+ * Removes the given security group permissions from the given node.
* <p>
* Takes no action if the compute service does not have a security group extension.
- * @param permissions The set of permissions to be removed from the location
- * @param location Location to remove permissions from
+ * @param location Location of the node to remove permissions from
+ * @param permissions The set of permissions to be removed from the node
*/
- public void removePermissionsFromLocation(final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+ public void removePermissionsFromLocation(JcloudsMachineLocation location, Iterable<IpPermission> permissions) {
+
synchronized (JcloudsLocationSecurityGroupCustomizer.class) {
- ComputeService computeService = location.getParent().getComputeService();
- String nodeId = location.getNode().getId();
- removePermissionsFromLocation(permissions, nodeId, computeService);
+ removePermissionsInternal(location, permissions);
}
}
/**
- * Removes the given security group permissions from the given node with the given compute service.
+ * Removes the given security group permissions from the given node.
* <p>
* Takes no action if the compute service does not have a security group extension.
+ * @param location Location of the node to remove permissions from
* @param permissions The set of permissions to be removed from the node
- * @param nodeId The id of the node to update
- * @param computeService The compute service to use to apply the changes
*/
- @VisibleForTesting
- void removePermissionsFromLocation(Iterable<IpPermission> permissions, final String nodeId, ComputeService computeService) {
- if (!computeService.getSecurityGroupExtension().isPresent()) {
+ private void removePermissionsInternal(JcloudsMachineLocation location, Iterable<IpPermission> permissions) {
+ ComputeService computeService = location.getParent().getComputeService();
+ String nodeId = location.getNode().getId();
+
+ final Optional<SecurityGroupExtension> securityApi = computeService.getSecurityGroupExtension();
+ if (!securityApi.isPresent()) {
--- End diff --
Could check with the editor instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88352921
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+/**
+ * Utility for manipulating security groups.
+ *
+ * For convenience, constructors take an argument of a {@link ComputeService} and extract from it the
+ * information needed to edit groups. However, not all compute services support the security group extension,
+ * so if the editor is being created for a compute service where it is not known in advance then users
+ * should query {@link #hasServiceSupport()} before calling the methods to edit security groups.
+ */
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+ public static final java.lang.String JCLOUDS_PREFIX = "jclouds#";
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor for editor that will retry operations upon exceptions.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request (e.g. to add a security group
+ * or a rule) if the attempted operation throws a Throwable.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
+ this.securityApi = this.computeService.getSecurityGroupExtension(); // TODO surely check for isPresent else throw?
+ this.isExceptionRetryable = predicate;
+ }
+
+ /**
+ * Constructor for editor that never retries requests if the attempted operation fails.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService) {
+ this(location, computeService, Predicates.alwaysFalse());
+ }
+
+
+ /**
+ * Flag to indicate whether the given compute service has support for security groups.
+ */
+ public boolean hasServiceSupport() {
+ return securityApi.isPresent();
+ }
+
+ /**
+ * Get the location in which security groups will be created or searched.
+ */
+ public Location getLocation() {
+ return location;
+ }
+
+ /**
+ * Get the location id from the compute service (e.g. "aws-ec2").
+ */
+ public String getLocationId() {
+ return locationId;
+ }
+
+ public Set<SecurityGroup> getSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ /**
+ * Create the security group. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. This method is idempotent.
+ * @param name Name of the group to create
+ * @return The created group.
+ */
+ public SecurityGroup createSecurityGroup(final String name) {
+
+ LOG.debug("Creating security group {} in {}", name, location);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ return securityApi.get().createSecurityGroup(name, location);
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ /**
+ * Removes a security group and its permissions.
+ * @return true if the group was found and removed.
+ */
+ public Boolean removeSecurityGroup(final SecurityGroup group) {
+
+ LOG.debug("Removing security group {} in {}", group.getName(), location);
+ Callable<Boolean> callable = new Callable<Boolean>() {
+ @Override
+ public Boolean call() throws Exception {
+ return securityApi.get().removeSecurityGroup(group.getId());
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ public Set<SecurityGroup> listSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ public Iterable<SecurityGroup> findSecurityGroupsMatching(Predicate predicate) {
+ final Set<SecurityGroup> locationGroups = securityApi.get().listSecurityGroupsInLocation(location);
+ return Iterables.filter(locationGroups, predicate);
+ }
+
+ /**
+ * @see #findSecurityGroupByName(String)
+ */
+ public static class AmbiguousGroupName extends IllegalArgumentException {
+ public AmbiguousGroupName(String s) {
+ super(s);
+ }
+ }
+
+ /**
+ * Find a security group with the given name. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. For convenience this method accepts names either with or without the prefix.
+ * @param name Name of the group to find.
+ * @return An optional of the group.
+ * @throws AmbiguousGroupName in the unexpected case that the cloud returns more than one matching group.
+ */
+ public Optional<SecurityGroup> findSecurityGroupByName(final String name) {
+ final String query = name.startsWith(JCLOUDS_PREFIX) ? name : JCLOUDS_PREFIX + name;
+ final Iterable<SecurityGroup> groupsMatching = findSecurityGroupsMatching(new Predicate<SecurityGroup>() {
+ @Override
+ public boolean apply(final SecurityGroup input) {
+ return input.getName().equals(query);
+ }
+ });
+ final ImmutableList<SecurityGroup> matches = ImmutableList.copyOf(groupsMatching);
+ if (matches.size() == 0) {
+ return Optional.absent();
+ } else if (matches.size() == 1) {
+ return Optional.of(matches.get(0));
+ } else {
+ throw new AmbiguousGroupName("Unexpected result of multiple groups matching " + name);
+ }
+ }
+
+ /**
+ * Add permissions to the security group, using {@link #addPermission(SecurityGroup, IpPermission)}.
+ * @param group The group to update
+ * @param permissions The new permissions
+ * @return The updated group with the added permissions.
+ */
+ public SecurityGroup addPermissions(final SecurityGroup group, final Iterable<IpPermission> permissions) {
+ SecurityGroup lastGroup = group;
+ for (IpPermission permission : permissions) {
+ lastGroup = addPermission(group, permission);
+ }
+ // TODO any of the calls could return null because of duplicate record. Fetch the new SG state ourselves in this case
+ return lastGroup;
+ }
+
+ /**
+ * Add a permission to the security group. This operation is idempotent (will return the group unmodified if the
+ * permission already exists on it).
+ * @param group The group to update
+ * @param permissions The new permissions
+ * @return The updated group with the added permissions.
+ */
+ public SecurityGroup addPermission(final SecurityGroup group, final IpPermission permission) {
+ LOG.debug("Adding permission to security group {}: {}", group.getName(), permission);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ try {
+ return securityApi.get().addIpPermission(permission, group);
+ } catch (Exception e) {
+ Exceptions.propagateIfFatal(e);
+
+ if (isDuplicate(e)) {
+ return group;
+ }
+
+ throw Exceptions.propagate(e);
+ }
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ @Deprecated // TODO improve this - shouldn't have AWS specifics in here
+ private boolean isDuplicate(Exception e) {
+ // Sometimes AWSResponseException is wrapped in an IllegalStateException
+ AWSResponseException cause = Exceptions.getFirstThrowableOfType(e, AWSResponseException.class);
+ if (cause != null) {
+ if ("InvalidPermission.Duplicate".equals(cause.getError().getCode())) {
+ return true;
+ }
+ }
+
+ if (e.toString().contains("already exists")) {
+ return true;
+ }
+
+ return false;
+ }
+
+
+ public SecurityGroup removePermission(final SecurityGroup group, final IpPermission permission) {
+ LOG.debug("Removing permission from security group {}: {}", group.getName(), permission);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ return securityApi.get().removeIpPermission(permission, group);
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ public SecurityGroup removePermissions(SecurityGroup group, final Iterable<IpPermission> permissions) {
+ for (IpPermission permission : permissions) {
+ group = removePermission(group, permission);
+ }
+ return group;
+ }
+
+ /**
+ * Runs the given callable. Repeats until the operation succeeds or {@link #isExceptionRetryable} indicates
+ * that the request cannot be retried.
+ */
+ protected <T> T runOperationWithRetry(Callable<T> operation) {
+ int backoff = 64;
+ Exception lastException = null;
+ for (int retries = 0; retries < 100; retries++) { // TODO this will try for about 2.0e+21 years; maybe not try so hard?
+ try {
+ return operation.call();
+ } catch (Exception e) {
+ lastException = e;
+ if (isExceptionRetryable.apply(e)) {
+ LOG.debug("Attempt #{} failed to add security group: {}", retries + 1, e.getMessage());
+ try {
+ Thread.sleep(backoff);
--- End diff --
Can we use our `Repeater`? It is very configurable, and gives us logging for free.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88501073
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -223,56 +230,64 @@ private SecurityGroup getSecurityGroup(final String nodeId, final SecurityGroupE
* @param location Location to gain permissions
* @param permissions The set of permissions to be applied to the location
*/
- public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
- ComputeService computeService = location.getParent().getComputeService();
- addPermissionsToLocationAndReturnSecurityGroup(computeService, location, permissions);
+ public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsMachineLocation location,
+ final Iterable<IpPermission> permissions) {
+ addPermissionsToLocationAndReturnSecurityGroup(location, permissions);
return this;
}
- public Collection<SecurityGroup> addPermissionsToLocationAndReturnSecurityGroup(ComputeService computeService, final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+ public Collection<SecurityGroup> addPermissionsToLocationAndReturnSecurityGroup(
+ final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+
synchronized (JcloudsLocationSecurityGroupCustomizer.class) {
--- End diff --
Should this lock be moved to the editor?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r89302475
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -223,56 +230,64 @@ private SecurityGroup getSecurityGroup(final String nodeId, final SecurityGroupE
* @param location Location to gain permissions
* @param permissions The set of permissions to be applied to the location
*/
- public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
- ComputeService computeService = location.getParent().getComputeService();
- addPermissionsToLocationAndReturnSecurityGroup(computeService, location, permissions);
+ public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsMachineLocation location,
+ final Iterable<IpPermission> permissions) {
+ addPermissionsToLocationAndReturnSecurityGroup(location, permissions);
return this;
}
- public Collection<SecurityGroup> addPermissionsToLocationAndReturnSecurityGroup(ComputeService computeService, final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+ public Collection<SecurityGroup> addPermissionsToLocationAndReturnSecurityGroup(
+ final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+
synchronized (JcloudsLocationSecurityGroupCustomizer.class) {
--- End diff --
I'm asking because of [this](https://github.com/apache/brooklyn-server/blob/master/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java#L217-L221) comment. Looks like the same restrictions will apply on the synchronizer.
Agree it can be tackled in a separate PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Do not merge; for review only; network ex...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r87448337
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -320,19 +319,22 @@ void removePermissionsFromLocation(Iterable<IpPermission> permissions, final Str
* look for the uniqueSecurityGroup rather than the shared securityGroup.
*
* @param nodeId The id of the node in question
- * @param locationId The id of the location in question
- * @param securityApi The API to use to list security groups
+ * @param groupEditor The id of the node in question
* @return the security group unique to the given node, or null if one could not be determined.
*/
- private SecurityGroup getUniqueSecurityGroupForNodeCachingSharedGroupIfPreviouslyUnknown(String nodeId, String locationId, SecurityGroupExtension securityApi) {
- Set<SecurityGroup> groupsOnNode = securityApi.listSecurityGroupsForNode(nodeId);
+ private SecurityGroup getUniqueSecurityGroupForNodeCachingSharedGroupIfPreviouslyUnknown(String nodeId,
+ final JcloudsLocationSecurityGroupEditor groupEditor) {
--- End diff --
Thank you for breaking those long signatures :)
If there was one styling change to force I'd choose a line length limit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #425: Factor security group code out into separate uti...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/425
LGTM, merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88352692
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+/**
+ * Utility for manipulating security groups.
+ *
+ * For convenience, constructors take an argument of a {@link ComputeService} and extract from it the
+ * information needed to edit groups. However, not all compute services support the security group extension,
+ * so if the editor is being created for a compute service where it is not known in advance then users
+ * should query {@link #hasServiceSupport()} before calling the methods to edit security groups.
+ */
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+ public static final java.lang.String JCLOUDS_PREFIX = "jclouds#";
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor for editor that will retry operations upon exceptions.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request (e.g. to add a security group
+ * or a rule) if the attempted operation throws a Throwable.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
--- End diff --
The `computeService` is only used for logging, and for getting the initial securitygroupExension.
The `locationId` is only used for the `getLocationId()` method, and for logging.
The `locationId` is also a bit confusing in Brooklyn, given we have a location class in Brooklyn as well. Its value is compared against "aws-ec2" - perhaps we should call it the `providerId`?
I wonder if we should pass less in - and perhaps expect the caller to have done the `computeservice.getSecurityGroupExtension().get()` call for us.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Do not merge; for review only; network ex...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r87568111
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -456,17 +457,16 @@ public boolean apply(final SecurityGroup input) {
*
*
* @param groupName The name of the security group to create
- * @param location The location in which the security group will be created
* @param securityApi The API to use to create the security group
*
* @return the created security group
*/
- private SecurityGroup createBaseSecurityGroupInLocation(String groupName, Location location, SecurityGroupExtension securityApi) {
- SecurityGroup group = addSecurityGroupInLocation(groupName, location, securityApi);
+ private SecurityGroup createBaseSecurityGroupInLocation(String groupName, JcloudsLocationSecurityGroupEditor groupEditor) {
+ SecurityGroup group = groupEditor.createSecurityGroup(groupName).get();
String groupId = group.getProviderId();
int fromPort = 0;
- if (isOpenstackNova(location)) {
+ if (isOpenstackNova(groupEditor.getLocation())) {
groupId = group.getId();
--- End diff --
We should report these inconsistencies to jclouds.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88763504
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+/**
+ * Utility for manipulating security groups.
+ *
+ * For convenience, constructors take an argument of a {@link ComputeService} and extract from it the
+ * information needed to edit groups. However, not all compute services support the security group extension,
+ * so if the editor is being created for a compute service where it is not known in advance then users
+ * should query {@link #hasServiceSupport()} before calling the methods to edit security groups.
+ */
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+ public static final java.lang.String JCLOUDS_PREFIX = "jclouds#";
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor for editor that will retry operations upon exceptions.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request (e.g. to add a security group
+ * or a rule) if the attempted operation throws a Throwable.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
+ this.securityApi = this.computeService.getSecurityGroupExtension(); // TODO surely check for isPresent else throw?
+ this.isExceptionRetryable = predicate;
+ }
+
+ /**
+ * Constructor for editor that never retries requests if the attempted operation fails.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService) {
+ this(location, computeService, Predicates.alwaysFalse());
+ }
+
+
+ /**
+ * Flag to indicate whether the given compute service has support for security groups.
+ */
+ public boolean hasServiceSupport() {
+ return securityApi.isPresent();
+ }
+
+ /**
+ * Get the location in which security groups will be created or searched.
+ */
+ public Location getLocation() {
+ return location;
+ }
+
+ /**
+ * Get the location id from the compute service (e.g. "aws-ec2").
+ */
+ public String getLocationId() {
+ return locationId;
+ }
+
+ public Set<SecurityGroup> getSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ /**
+ * Create the security group. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. This method is idempotent.
+ * @param name Name of the group to create
+ * @return The created group.
+ */
+ public SecurityGroup createSecurityGroup(final String name) {
+
+ LOG.debug("Creating security group {} in {}", name, location);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ return securityApi.get().createSecurityGroup(name, location);
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ /**
+ * Removes a security group and its permissions.
+ * @return true if the group was found and removed.
+ */
+ public Boolean removeSecurityGroup(final SecurityGroup group) {
+
+ LOG.debug("Removing security group {} in {}", group.getName(), location);
+ Callable<Boolean> callable = new Callable<Boolean>() {
+ @Override
+ public Boolean call() throws Exception {
+ return securityApi.get().removeSecurityGroup(group.getId());
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ public Set<SecurityGroup> listSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ public Iterable<SecurityGroup> findSecurityGroupsMatching(Predicate predicate) {
+ final Set<SecurityGroup> locationGroups = securityApi.get().listSecurityGroupsInLocation(location);
+ return Iterables.filter(locationGroups, predicate);
+ }
+
+ /**
+ * @see #findSecurityGroupByName(String)
+ */
+ public static class AmbiguousGroupName extends IllegalArgumentException {
+ public AmbiguousGroupName(String s) {
+ super(s);
+ }
+ }
+
+ /**
+ * Find a security group with the given name. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. For convenience this method accepts names either with or without the prefix.
+ * @param name Name of the group to find.
+ * @return An optional of the group.
+ * @throws AmbiguousGroupName in the unexpected case that the cloud returns more than one matching group.
+ */
+ public Optional<SecurityGroup> findSecurityGroupByName(final String name) {
+ final String query = name.startsWith(JCLOUDS_PREFIX) ? name : JCLOUDS_PREFIX + name;
+ final Iterable<SecurityGroup> groupsMatching = findSecurityGroupsMatching(new Predicate<SecurityGroup>() {
+ @Override
+ public boolean apply(final SecurityGroup input) {
+ return input.getName().equals(query);
+ }
+ });
+ final ImmutableList<SecurityGroup> matches = ImmutableList.copyOf(groupsMatching);
+ if (matches.size() == 0) {
+ return Optional.absent();
+ } else if (matches.size() == 1) {
+ return Optional.of(matches.get(0));
+ } else {
+ throw new AmbiguousGroupName("Unexpected result of multiple groups matching " + name);
+ }
+ }
+
+ /**
+ * Add permissions to the security group, using {@link #addPermission(SecurityGroup, IpPermission)}.
+ * @param group The group to update
+ * @param permissions The new permissions
+ * @return The updated group with the added permissions.
+ */
+ public SecurityGroup addPermissions(final SecurityGroup group, final Iterable<IpPermission> permissions) {
+ SecurityGroup lastGroup = group;
+ for (IpPermission permission : permissions) {
+ lastGroup = addPermission(group, permission);
+ }
+ // TODO any of the calls could return null because of duplicate record. Fetch the new SG state ourselves in this case
--- End diff --
Leftover comment, will remove.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r89303465
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---
@@ -219,60 +226,62 @@ private SecurityGroup getSecurityGroup(final String nodeId, final SecurityGroupE
* the changes may not be picked up by later customizations, meaning the same rule could possibly be
* added twice, which would fail. A finer grained mechanism would be preferable here, but
* we have no access to the information required, so this brute force serializing is required.
+ * TODO investigate whether this can be improved. Can the synchronization be moved to
+ * the class org.apache.brooklyn.location.jclouds.networking.SecurityGroupEditor?
*
* @param location Location to gain permissions
* @param permissions The set of permissions to be applied to the location
*/
- public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
- ComputeService computeService = location.getParent().getComputeService();
- addPermissionsToLocationAndReturnSecurityGroup(computeService, location, permissions);
+ public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsMachineLocation location,
+ final Iterable<IpPermission> permissions) {
+ addPermissionsToLocationAndReturnSecurityGroup(location, permissions);
return this;
}
- public Collection<SecurityGroup> addPermissionsToLocationAndReturnSecurityGroup(ComputeService computeService, final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+ public Collection<SecurityGroup> addPermissionsToLocationAndReturnSecurityGroup(
+ final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+
synchronized (JcloudsLocationSecurityGroupCustomizer.class) {
- String nodeId = location.getNode().getId();
- return addPermissionsToLocation(permissions, nodeId, computeService).values();
+ return addPermissionsInternal(permissions, location).values();
}
}
+
/**
- * Removes the given security group permissions from the given node with the given compute service.
+ * Removes the given security group permissions from the given node.
* <p>
* Takes no action if the compute service does not have a security group extension.
- * @param permissions The set of permissions to be removed from the location
- * @param location Location to remove permissions from
+ * @param location Location of the node to remove permissions from
+ * @param permissions The set of permissions to be removed from the node
*/
- public void removePermissionsFromLocation(final JcloudsMachineLocation location, final Iterable<IpPermission> permissions) {
+ public void removePermissionsFromLocation(JcloudsMachineLocation location, Iterable<IpPermission> permissions) {
+
synchronized (JcloudsLocationSecurityGroupCustomizer.class) {
- ComputeService computeService = location.getParent().getComputeService();
- String nodeId = location.getNode().getId();
- removePermissionsFromLocation(permissions, nodeId, computeService);
+ removePermissionsInternal(location, permissions);
}
}
/**
- * Removes the given security group permissions from the given node with the given compute service.
+ * Removes the given security group permissions from the given node.
* <p>
* Takes no action if the compute service does not have a security group extension.
+ * @param location Location of the node to remove permissions from
* @param permissions The set of permissions to be removed from the node
- * @param nodeId The id of the node to update
- * @param computeService The compute service to use to apply the changes
*/
- @VisibleForTesting
- void removePermissionsFromLocation(Iterable<IpPermission> permissions, final String nodeId, ComputeService computeService) {
- if (!computeService.getSecurityGroupExtension().isPresent()) {
+ private void removePermissionsInternal(JcloudsMachineLocation location, Iterable<IpPermission> permissions) {
+ ComputeService computeService = location.getParent().getComputeService();
+ String nodeId = location.getNode().getId();
+
+ final Optional<SecurityGroupExtension> securityApi = computeService.getSecurityGroupExtension();
+ if (!securityApi.isPresent()) {
--- End diff --
nvm, that's no longer editor's responsibility
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/425
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88353667
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+/**
+ * Utility for manipulating security groups.
+ *
+ * For convenience, constructors take an argument of a {@link ComputeService} and extract from it the
+ * information needed to edit groups. However, not all compute services support the security group extension,
+ * so if the editor is being created for a compute service where it is not known in advance then users
+ * should query {@link #hasServiceSupport()} before calling the methods to edit security groups.
+ */
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+ public static final java.lang.String JCLOUDS_PREFIX = "jclouds#";
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor for editor that will retry operations upon exceptions.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request (e.g. to add a security group
+ * or a rule) if the attempted operation throws a Throwable.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
+ this.securityApi = this.computeService.getSecurityGroupExtension(); // TODO surely check for isPresent else throw?
+ this.isExceptionRetryable = predicate;
+ }
+
+ /**
+ * Constructor for editor that never retries requests if the attempted operation fails.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService) {
+ this(location, computeService, Predicates.alwaysFalse());
+ }
+
+
+ /**
+ * Flag to indicate whether the given compute service has support for security groups.
+ */
+ public boolean hasServiceSupport() {
+ return securityApi.isPresent();
+ }
+
+ /**
+ * Get the location in which security groups will be created or searched.
+ */
+ public Location getLocation() {
+ return location;
+ }
+
+ /**
+ * Get the location id from the compute service (e.g. "aws-ec2").
+ */
+ public String getLocationId() {
+ return locationId;
+ }
+
+ public Set<SecurityGroup> getSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ /**
+ * Create the security group. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. This method is idempotent.
+ * @param name Name of the group to create
+ * @return The created group.
+ */
+ public SecurityGroup createSecurityGroup(final String name) {
+
+ LOG.debug("Creating security group {} in {}", name, location);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ return securityApi.get().createSecurityGroup(name, location);
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ /**
+ * Removes a security group and its permissions.
+ * @return true if the group was found and removed.
+ */
+ public Boolean removeSecurityGroup(final SecurityGroup group) {
+
+ LOG.debug("Removing security group {} in {}", group.getName(), location);
+ Callable<Boolean> callable = new Callable<Boolean>() {
+ @Override
+ public Boolean call() throws Exception {
+ return securityApi.get().removeSecurityGroup(group.getId());
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ public Set<SecurityGroup> listSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ public Iterable<SecurityGroup> findSecurityGroupsMatching(Predicate predicate) {
+ final Set<SecurityGroup> locationGroups = securityApi.get().listSecurityGroupsInLocation(location);
+ return Iterables.filter(locationGroups, predicate);
+ }
+
+ /**
+ * @see #findSecurityGroupByName(String)
+ */
+ public static class AmbiguousGroupName extends IllegalArgumentException {
+ public AmbiguousGroupName(String s) {
+ super(s);
+ }
+ }
+
+ /**
+ * Find a security group with the given name. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. For convenience this method accepts names either with or without the prefix.
+ * @param name Name of the group to find.
+ * @return An optional of the group.
+ * @throws AmbiguousGroupName in the unexpected case that the cloud returns more than one matching group.
+ */
+ public Optional<SecurityGroup> findSecurityGroupByName(final String name) {
+ final String query = name.startsWith(JCLOUDS_PREFIX) ? name : JCLOUDS_PREFIX + name;
+ final Iterable<SecurityGroup> groupsMatching = findSecurityGroupsMatching(new Predicate<SecurityGroup>() {
+ @Override
+ public boolean apply(final SecurityGroup input) {
+ return input.getName().equals(query);
+ }
+ });
+ final ImmutableList<SecurityGroup> matches = ImmutableList.copyOf(groupsMatching);
+ if (matches.size() == 0) {
+ return Optional.absent();
+ } else if (matches.size() == 1) {
+ return Optional.of(matches.get(0));
+ } else {
+ throw new AmbiguousGroupName("Unexpected result of multiple groups matching " + name);
+ }
+ }
+
+ /**
+ * Add permissions to the security group, using {@link #addPermission(SecurityGroup, IpPermission)}.
+ * @param group The group to update
+ * @param permissions The new permissions
+ * @return The updated group with the added permissions.
+ */
+ public SecurityGroup addPermissions(final SecurityGroup group, final Iterable<IpPermission> permissions) {
+ SecurityGroup lastGroup = group;
+ for (IpPermission permission : permissions) {
+ lastGroup = addPermission(group, permission);
+ }
+ // TODO any of the calls could return null because of duplicate record. Fetch the new SG state ourselves in this case
--- End diff --
How could it return null? I thought `addPermission()` would return the group that was passed in if we get the "InvalidPermission.Duplicate" error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88353338
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+/**
+ * Utility for manipulating security groups.
+ *
+ * For convenience, constructors take an argument of a {@link ComputeService} and extract from it the
+ * information needed to edit groups. However, not all compute services support the security group extension,
+ * so if the editor is being created for a compute service where it is not known in advance then users
+ * should query {@link #hasServiceSupport()} before calling the methods to edit security groups.
+ */
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+ public static final java.lang.String JCLOUDS_PREFIX = "jclouds#";
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor for editor that will retry operations upon exceptions.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request (e.g. to add a security group
+ * or a rule) if the attempted operation throws a Throwable.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
+ this.securityApi = this.computeService.getSecurityGroupExtension(); // TODO surely check for isPresent else throw?
+ this.isExceptionRetryable = predicate;
+ }
+
+ /**
+ * Constructor for editor that never retries requests if the attempted operation fails.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService) {
+ this(location, computeService, Predicates.alwaysFalse());
+ }
+
+
+ /**
+ * Flag to indicate whether the given compute service has support for security groups.
+ */
+ public boolean hasServiceSupport() {
+ return securityApi.isPresent();
+ }
+
+ /**
+ * Get the location in which security groups will be created or searched.
+ */
+ public Location getLocation() {
+ return location;
+ }
+
+ /**
+ * Get the location id from the compute service (e.g. "aws-ec2").
+ */
+ public String getLocationId() {
+ return locationId;
+ }
+
+ public Set<SecurityGroup> getSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ /**
+ * Create the security group. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. This method is idempotent.
+ * @param name Name of the group to create
+ * @return The created group.
+ */
+ public SecurityGroup createSecurityGroup(final String name) {
+
+ LOG.debug("Creating security group {} in {}", name, location);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ return securityApi.get().createSecurityGroup(name, location);
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ /**
+ * Removes a security group and its permissions.
+ * @return true if the group was found and removed.
+ */
+ public Boolean removeSecurityGroup(final SecurityGroup group) {
+
+ LOG.debug("Removing security group {} in {}", group.getName(), location);
+ Callable<Boolean> callable = new Callable<Boolean>() {
+ @Override
+ public Boolean call() throws Exception {
+ return securityApi.get().removeSecurityGroup(group.getId());
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ public Set<SecurityGroup> listSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ public Iterable<SecurityGroup> findSecurityGroupsMatching(Predicate predicate) {
+ final Set<SecurityGroup> locationGroups = securityApi.get().listSecurityGroupsInLocation(location);
+ return Iterables.filter(locationGroups, predicate);
+ }
+
+ /**
+ * @see #findSecurityGroupByName(String)
+ */
+ public static class AmbiguousGroupName extends IllegalArgumentException {
+ public AmbiguousGroupName(String s) {
+ super(s);
+ }
+ }
+
+ /**
+ * Find a security group with the given name. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. For convenience this method accepts names either with or without the prefix.
+ * @param name Name of the group to find.
+ * @return An optional of the group.
+ * @throws AmbiguousGroupName in the unexpected case that the cloud returns more than one matching group.
+ */
+ public Optional<SecurityGroup> findSecurityGroupByName(final String name) {
+ final String query = name.startsWith(JCLOUDS_PREFIX) ? name : JCLOUDS_PREFIX + name;
+ final Iterable<SecurityGroup> groupsMatching = findSecurityGroupsMatching(new Predicate<SecurityGroup>() {
+ @Override
+ public boolean apply(final SecurityGroup input) {
+ return input.getName().equals(query);
+ }
+ });
+ final ImmutableList<SecurityGroup> matches = ImmutableList.copyOf(groupsMatching);
+ if (matches.size() == 0) {
+ return Optional.absent();
+ } else if (matches.size() == 1) {
+ return Optional.of(matches.get(0));
+ } else {
+ throw new AmbiguousGroupName("Unexpected result of multiple groups matching " + name);
--- End diff --
Ah, ignore me - it's an exact name match, rather than a predicate!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Do not merge; for review only; network ex...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r87450045
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,223 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.apache.brooklyn.util.guava.Maybe;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request to add a security group
+ * or a rule after an throwable is thrown.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate<Exception> predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
--- End diff --
Isn't the `computeService` extracted from the `location`? Why not pass only the `location` in.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #425: Factor security group code out into separate uti...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/425
Changes made in response to the above comments, squashed and rebased against master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Do not merge; for review only; network ex...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r87810853
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,223 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.apache.brooklyn.util.guava.Maybe;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request to add a security group
+ * or a rule after an throwable is thrown.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate<Exception> predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
--- End diff --
The compute service is actually obtained direct from the ComputeServiceRegistry, [here](https://github.com/apache/brooklyn-server/blob/master/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java#L664). This uses the `setup` created using the flags that were passed into `JcloudsLocation.obtain()`. If, from inside the editor code, I just call `location.getComputeService()` to get the compute service, the call sequence will pass in the config bag of the `config()` of the location. While I don't really think that the value this obtains will be different from the value obtained using the flags from `obtain()`, I couldn't rule it out, so to preserve the semantics I kept the `computeService` here as a separate argument.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #425: Do not merge; for review only; network experimen...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/425
TODO add handling of exceptions, to make operations idempotent
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88660968
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+/**
+ * Utility for manipulating security groups.
+ *
+ * For convenience, constructors take an argument of a {@link ComputeService} and extract from it the
+ * information needed to edit groups. However, not all compute services support the security group extension,
+ * so if the editor is being created for a compute service where it is not known in advance then users
+ * should query {@link #hasServiceSupport()} before calling the methods to edit security groups.
+ */
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+ public static final java.lang.String JCLOUDS_PREFIX = "jclouds#";
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor for editor that will retry operations upon exceptions.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request (e.g. to add a security group
+ * or a rule) if the attempted operation throws a Throwable.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
--- End diff --
Have given this and the previous comment further thought and discussion with @neykov. The choice here seems to be between
1. going "higher level" and trying to pass in a Brooklyn class to the constructor, from which it could extract the location and compute service. This would make the API JClouds-unaware, and turn the class into a more widely re-usable tool for manipulating security-group-like-things however they were implemented.
2. going "lower level" and leaving it up to the caller to get the required objects - security group extension and location, with a resultingly simpler API for this class.
While the former might be nicer to have, it looks like it would require a bit of rework in the `JcloudsLocationSecurityGroupCustomizer` and/or `JCloudsLocation` classes, as there are a couple of paths through them that require this functionality, and these get the two required objects from different sources - specifically, one path gets the location from a Template, the other from the actual node's machine location (via `JcloudsMachineLocation`). This seems too much to take on for the sake of this PR.
In short, I'll Keep it Simple and go with option 2.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #425: Factor security group code out into separ...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/425#discussion_r88763287
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupEditor.java ---
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.location.jclouds.networking;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.jclouds.aws.AWSResponseException;
+import org.jclouds.compute.ComputeService;
+import org.jclouds.compute.domain.SecurityGroup;
+import org.jclouds.compute.extensions.SecurityGroupExtension;
+import org.jclouds.domain.Location;
+import org.jclouds.net.domain.IpPermission;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+/**
+ * Utility for manipulating security groups.
+ *
+ * For convenience, constructors take an argument of a {@link ComputeService} and extract from it the
+ * information needed to edit groups. However, not all compute services support the security group extension,
+ * so if the editor is being created for a compute service where it is not known in advance then users
+ * should query {@link #hasServiceSupport()} before calling the methods to edit security groups.
+ */
+public class JcloudsLocationSecurityGroupEditor {
+
+ private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupEditor.class);
+ public static final java.lang.String JCLOUDS_PREFIX = "jclouds#";
+
+ private final Location location;
+ private final ComputeService computeService;
+ private final Optional<SecurityGroupExtension> securityApi;
+ private final String locationId;
+ private final Predicate<Exception> isExceptionRetryable;
+
+ /**
+ * Constructor for editor that will retry operations upon exceptions.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ * @param predicate A predicate indicating whether the customiser can retry a request (e.g. to add a security group
+ * or a rule) if the attempted operation throws a Throwable.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService, Predicate predicate) {
+ this.location = location;
+ this.computeService = computeService;
+ this.locationId = this.computeService.getContext().unwrap().getId();
+ this.securityApi = this.computeService.getSecurityGroupExtension(); // TODO surely check for isPresent else throw?
+ this.isExceptionRetryable = predicate;
+ }
+
+ /**
+ * Constructor for editor that never retries requests if the attempted operation fails.
+ * @param location JClouds location where security groups will be managed.
+ * @param computeService The JClouds compute service instance.
+ */
+ public JcloudsLocationSecurityGroupEditor(Location location, ComputeService computeService) {
+ this(location, computeService, Predicates.alwaysFalse());
+ }
+
+
+ /**
+ * Flag to indicate whether the given compute service has support for security groups.
+ */
+ public boolean hasServiceSupport() {
+ return securityApi.isPresent();
+ }
+
+ /**
+ * Get the location in which security groups will be created or searched.
+ */
+ public Location getLocation() {
+ return location;
+ }
+
+ /**
+ * Get the location id from the compute service (e.g. "aws-ec2").
+ */
+ public String getLocationId() {
+ return locationId;
+ }
+
+ public Set<SecurityGroup> getSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ /**
+ * Create the security group. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. This method is idempotent.
+ * @param name Name of the group to create
+ * @return The created group.
+ */
+ public SecurityGroup createSecurityGroup(final String name) {
+
+ LOG.debug("Creating security group {} in {}", name, location);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ return securityApi.get().createSecurityGroup(name, location);
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ /**
+ * Removes a security group and its permissions.
+ * @return true if the group was found and removed.
+ */
+ public Boolean removeSecurityGroup(final SecurityGroup group) {
+
+ LOG.debug("Removing security group {} in {}", group.getName(), location);
+ Callable<Boolean> callable = new Callable<Boolean>() {
+ @Override
+ public Boolean call() throws Exception {
+ return securityApi.get().removeSecurityGroup(group.getId());
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ public Set<SecurityGroup> listSecurityGroupsForNode(final String nodeId) {
+ return securityApi.get().listSecurityGroupsForNode(nodeId);
+ }
+
+ public Iterable<SecurityGroup> findSecurityGroupsMatching(Predicate predicate) {
+ final Set<SecurityGroup> locationGroups = securityApi.get().listSecurityGroupsInLocation(location);
+ return Iterables.filter(locationGroups, predicate);
+ }
+
+ /**
+ * @see #findSecurityGroupByName(String)
+ */
+ public static class AmbiguousGroupName extends IllegalArgumentException {
+ public AmbiguousGroupName(String s) {
+ super(s);
+ }
+ }
+
+ /**
+ * Find a security group with the given name. As we use jclouds, groups are created with names prefixed
+ * with {@link #JCLOUDS_PREFIX}. For convenience this method accepts names either with or without the prefix.
+ * @param name Name of the group to find.
+ * @return An optional of the group.
+ * @throws AmbiguousGroupName in the unexpected case that the cloud returns more than one matching group.
+ */
+ public Optional<SecurityGroup> findSecurityGroupByName(final String name) {
+ final String query = name.startsWith(JCLOUDS_PREFIX) ? name : JCLOUDS_PREFIX + name;
+ final Iterable<SecurityGroup> groupsMatching = findSecurityGroupsMatching(new Predicate<SecurityGroup>() {
+ @Override
+ public boolean apply(final SecurityGroup input) {
+ return input.getName().equals(query);
+ }
+ });
+ final ImmutableList<SecurityGroup> matches = ImmutableList.copyOf(groupsMatching);
+ if (matches.size() == 0) {
+ return Optional.absent();
+ } else if (matches.size() == 1) {
+ return Optional.of(matches.get(0));
+ } else {
+ throw new AmbiguousGroupName("Unexpected result of multiple groups matching " + name);
+ }
+ }
+
+ /**
+ * Add permissions to the security group, using {@link #addPermission(SecurityGroup, IpPermission)}.
+ * @param group The group to update
+ * @param permissions The new permissions
+ * @return The updated group with the added permissions.
+ */
+ public SecurityGroup addPermissions(final SecurityGroup group, final Iterable<IpPermission> permissions) {
+ SecurityGroup lastGroup = group;
+ for (IpPermission permission : permissions) {
+ lastGroup = addPermission(group, permission);
+ }
+ // TODO any of the calls could return null because of duplicate record. Fetch the new SG state ourselves in this case
+ return lastGroup;
+ }
+
+ /**
+ * Add a permission to the security group. This operation is idempotent (will return the group unmodified if the
+ * permission already exists on it).
+ * @param group The group to update
+ * @param permissions The new permissions
+ * @return The updated group with the added permissions.
+ */
+ public SecurityGroup addPermission(final SecurityGroup group, final IpPermission permission) {
+ LOG.debug("Adding permission to security group {}: {}", group.getName(), permission);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ try {
+ return securityApi.get().addIpPermission(permission, group);
+ } catch (Exception e) {
+ Exceptions.propagateIfFatal(e);
+
+ if (isDuplicate(e)) {
+ return group;
+ }
+
+ throw Exceptions.propagate(e);
+ }
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ @Deprecated // TODO improve this - shouldn't have AWS specifics in here
+ private boolean isDuplicate(Exception e) {
+ // Sometimes AWSResponseException is wrapped in an IllegalStateException
+ AWSResponseException cause = Exceptions.getFirstThrowableOfType(e, AWSResponseException.class);
+ if (cause != null) {
+ if ("InvalidPermission.Duplicate".equals(cause.getError().getCode())) {
+ return true;
+ }
+ }
+
+ if (e.toString().contains("already exists")) {
+ return true;
+ }
+
+ return false;
+ }
+
+
+ public SecurityGroup removePermission(final SecurityGroup group, final IpPermission permission) {
+ LOG.debug("Removing permission from security group {}: {}", group.getName(), permission);
+ Callable<SecurityGroup> callable = new Callable<SecurityGroup>() {
+ @Override
+ public SecurityGroup call() throws Exception {
+ return securityApi.get().removeIpPermission(permission, group);
+ }
+ };
+ return runOperationWithRetry(callable);
+ }
+
+ public SecurityGroup removePermissions(SecurityGroup group, final Iterable<IpPermission> permissions) {
+ for (IpPermission permission : permissions) {
+ group = removePermission(group, permission);
+ }
+ return group;
+ }
+
+ /**
+ * Runs the given callable. Repeats until the operation succeeds or {@link #isExceptionRetryable} indicates
+ * that the request cannot be retried.
+ */
+ protected <T> T runOperationWithRetry(Callable<T> operation) {
+ int backoff = 64;
+ Exception lastException = null;
+ for (int retries = 0; retries < 100; retries++) { // TODO this will try for about 2.0e+21 years; maybe not try so hard?
+ try {
+ return operation.call();
+ } catch (Exception e) {
+ lastException = e;
+ if (isExceptionRetryable.apply(e)) {
+ LOG.debug("Attempt #{} failed to add security group: {}", retries + 1, e.getMessage());
+ try {
+ Thread.sleep(backoff);
--- End diff --
I had a go at this, but it's complicated by the fact that you have to return the result of the `operation`, plus `Repeater` doesn't have an exact equivalent of `isExceptionRetryable`, so you have to capture the exception in the `repeat` body and save it for the `until` to look at. It's doable but didn't seem compellingly simpler than what's there now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---