You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by al...@apache.org on 2014/11/28 19:23:27 UTC

[1/5] incubator-brooklyn git commit: Use shared PortForwardManager, and improved interface

Repository: incubator-brooklyn
Updated Branches:
  refs/heads/master a8203c1ed -> dea4a5195


Use shared PortForwardManager, and improved interface

- Improve the PortForwardManager interface.
  Deprecate the old code.
- Change PortForwardManager to be a location.
- Add PortForwardManagerLoationResolver for retrieving shared instance.
- Support configuring the starting port number for PortForwardManager.


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/804c9eb6
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/804c9eb6
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/804c9eb6

Branch: refs/heads/master
Commit: 804c9eb625a935d75ff35b4342026294081a58ff
Parents: 068de70
Author: Aled Sage <al...@gmail.com>
Authored: Fri Nov 21 11:48:22 2014 +0000
Committer: Aled Sage <al...@gmail.com>
Committed: Fri Nov 28 08:31:56 2014 +0000

----------------------------------------------------------------------
 .../brooklyn/management/ManagementContext.java  |   5 +-
 .../location/access/PortForwardManager.java     | 235 ++++++++---
 .../access/PortForwardManagerAuthority.java     | 230 +----------
 .../access/PortForwardManagerClient.java        | 256 +++++++++++-
 .../location/access/PortForwardManagerImpl.java | 408 +++++++++++++++++++
 .../PortForwardManagerLocationResolver.java     |  89 ++++
 .../brooklyn/location/access/PortMapping.java   |  38 +-
 .../basic/AbstractLocationResolver.java         |   2 +-
 .../internal/AbstractManagementContext.java     |   2 +-
 .../NonDeploymentManagementContext.java         |   1 -
 .../services/brooklyn.location.LocationResolver |   1 +
 .../PortForwardManagerLocationResolverTest.java |  83 ++++
 .../access/PortForwardManagerRebindTest.java    |  26 +-
 .../location/jclouds/JcloudsLocation.java       |  15 +-
 .../JcloudsPortForwarderExtension.java          |  11 +
 .../java/brooklyn/entity/java/JmxSupport.java   |   1 -
 16 files changed, 1067 insertions(+), 336 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/api/src/main/java/brooklyn/management/ManagementContext.java
----------------------------------------------------------------------
diff --git a/api/src/main/java/brooklyn/management/ManagementContext.java b/api/src/main/java/brooklyn/management/ManagementContext.java
index 221e3c9..b45ecef 100644
--- a/api/src/main/java/brooklyn/management/ManagementContext.java
+++ b/api/src/main/java/brooklyn/management/ManagementContext.java
@@ -219,10 +219,11 @@ public interface ManagementContext {
      */
     void removePropertiesReloadListener(PropertiesReloadListener listener);
 
-    /** Active entitlements checker instance. */
+    /**
+     * Active entitlements checker instance.
+     */
     EntitlementManager getEntitlementManager();
  
-
     /** As {@link #lookup(String, Class)} but not constraining the return type */
     public BrooklynObject lookup(String id);
     

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/location/access/PortForwardManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManager.java b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
index 3d8aeb4..7a20157 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManager.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
@@ -20,87 +20,71 @@ package brooklyn.location.access;
 
 import java.util.Collection;
 
+import brooklyn.config.ConfigKey;
+import brooklyn.entity.basic.ConfigKeys;
 import brooklyn.location.Location;
 
 import com.google.common.annotations.Beta;
 import com.google.common.net.HostAndPort;
 
 /**
- * Records port mappings against public IP addresses with given identifiers.
- * <p>
- * To use, create a new authoritative instance (e.g. {@link PortForwardManagerAuthority}) which will live in one
- * canonical place, then set config to be a client (e.g. {@link PortForwardManagerClient} which delegates to the
- * primary instance) so the authority is shared among all communicating parties but only persisted in one place.
- * <p>
- * One Location side (e.g. a software process in a VM) can request ({@link #acquirePublicPort(String, Location, int)})
- * an unused port on a firewall / public IP address. It may then go on actually to talk to that firewall/IP to
- * provision the forwarding rule.
- * <p>
- * Subsequently the other side can use this class {@link #lookup(Location, int)} if it knows the
- * location and private port it wishes to talk to.
- * <p>
+ * Acts as a registry for existing port mappings (e.g. the public endpoints for accessing specific
+ * ports on private VMs). This could be using DNAT, or iptables port-forwarding, or Docker port-mapping 
+ * via the host, or any other port mapping approach.
+ * 
+ * Also controls the allocation of ports via {@link #acquirePublicPort(String)}
+ * (e.g. for port-mapping with DNAT, then which port to use for the public side).
+ * 
+ * Implementations typically will not know anything about what the firewall/IP actually is, they just 
+ * handle a unique identifier for it.
+ * 
+ * To use, see {@link PortForwardManagerLocationResolver}, with code such as 
+ * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
+ * 
  * Implementations typically will not know anything about what the firewall/IP actually is, they just handle a
  * unique identifier for it. It is recommended, however, to {@link #recordPublicIpHostname(String, String)} an
  * accessible hostname with the identifier. This is required in order to use {@link #lookup(Location, int)}.
  */
 @Beta
-public interface PortForwardManager {
+public interface PortForwardManager extends Location {
 
-    /**
-     * Reserves a unique public port on the given publicIpId.
-     * <p>
-     * Often followed by {@link #associate(String, int, Location, int)}
-     * to enable {@link #lookup(Location, int)}.
-     */
-    public int acquirePublicPort(String publicIpId);
+    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
+            "scope",
+            "The scope that this applies to, defaulting to global",
+            "global");
 
-    /** Returns old mapping if it existed, null if it is new. */
-    public PortMapping acquirePublicPortExplicit(String publicIpId, int port);
+    @Beta
+    public static final ConfigKey<Integer> PORT_FORWARD_MANAGER_STARTING_PORT = ConfigKeys.newIntegerConfigKey(
+            "brooklyn.portForwardManager.startingPort",
+            "The starting port for assigning port numbers, such as for DNAT",
+            11000);
 
-    /** Returns the port mapping for a given publicIpId and public port. */
-    public PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort);
+    public String getScope();
 
-    /** Returns the subset of port mappings associated with a given public IP ID. */
-    public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId);
-
-    /** Clears the given port mapping, returning the mapping if there was one. */
-    public PortMapping forgetPortMapping(String publicIpId, int publicPort);
-    
-    /** @see #forgetPortMapping(String, int) */
-    public boolean forgetPortMapping(PortMapping m);
-
-    // -----------------
-    
     /**
-     * Records a public hostname or address to be associated with the given publicIpId for lookup purposes.
+     * Reserves a unique public port on the given publicIpId.
      * <p>
-     * Conceivably this may have to be access-location specific.
+     * Often followed by {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
+     * to enable {@link #lookup(String, int)} or {@link #lookup(Location, int)} respectively.
      */
-    public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress);
-
-    /** Returns a recorded public hostname or address. */
-    public String getPublicIpHostname(String publicIpId);
-    
-    /** Clears a previous call to {@link #recordPublicIpHostname(String, String)}. */
-    public boolean forgetPublicIpHostname(String publicIpId);
+    public int acquirePublicPort(String publicIpId);
 
     /**
-     * Returns the public host and port for use accessing the given mapping.
+     * Records a location and private port against a public endpoint (ip and port),
+     * to support {@link #lookup(Location, int)}.
      * <p>
-     * Conceivably this may have to be access-location specific.
+     * Superfluous if {@link #acquirePublicPort(String, Location, int)} was used,
+     * but strongly recommended if {@link #acquirePublicPortExplicit(String, int)} was used
+     * e.g. if the location is not known ahead of time.
      */
-    public HostAndPort getPublicHostAndPort(PortMapping m);
+    public void associate(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort);
 
-    // -----------------
-    
     /**
-     * Reserves a unique public port for the purpose of forwarding to the given target,
-     * associated with a given location for subsequent lookup purpose.
-     * <p>
-     * If already allocated, returns the previously allocated.
+     * Records a mapping for pubilcIpId:privatePort to a public endpoint, such that it can
+     * subsequently be looked up using {@link #lookup(String, int)}.
      */
-    public int acquirePublicPort(String publicIpId, Location l, int privatePort);
-
+    public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort);
+    
     /**
      * Returns the public ip hostname and public port for use contacting the given endpoint.
      * <p>
@@ -115,7 +99,59 @@ public interface PortForwardManager {
      * @see #recordPublicIpHostname(String, String)
      */
     public HostAndPort lookup(Location l, int privatePort);
+
+    /**
+     * Returns the public endpoint (host and port) for use contacting the given endpoint.
+     * 
+     * Expects a previous call to {@link #associate(String, HostAndPort, int)}, to register
+     * the endpoint.
+     * 
+     * Will return null if there has not been a public endpoint associated with this pairing.
+     */
+    public HostAndPort lookup(String publicIpId, int privatePort);
+
+    /** 
+     * Clears the given port mapping, returning the true if there was a match.
+     */
+    public boolean forgetPortMapping(String publicIpId, int publicPort);
+    
+    /** 
+     * Clears the port mappings associated with the given location, returning true if there were any matches.
+     */
+    public boolean forgetPortMappings(Location location);
+    
+    /**
+     * Returns true if this implementation is a client which is immutable/safe for serialization
+     * i.e. it delegates to something on an entity or location elsewhere.
+     */
+    public boolean isClient();
+    
+    public String toVerboseString();
+
     
+    ///////////////////////////////////////////////////////////////////////////////////
+    // Deprecated
+    ///////////////////////////////////////////////////////////////////////////////////
+
+    /**
+     * Reserves a unique public port for the purpose of forwarding to the given target,
+     * associated with a given location for subsequent lookup purpose.
+     * <p>
+     * If already allocated, returns the previously allocated.
+     * 
+     * @deprecated since 0.7.0; use {@link #acquirePublicPort(String)}, and then use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
+     */
+    @Deprecated
+    public int acquirePublicPort(String publicIpId, Location l, int privatePort);
+
+    /** 
+     * Returns old mapping if it existed, null if it is new.
+     * 
+     * @deprecated since 0.7.0; use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
+     */
+    @Deprecated
+    public PortMapping acquirePublicPortExplicit(String publicIpId, int port);
+
     /**
      * Records a location and private port against a publicIp and public port,
      * to support {@link #lookup(Location, int)}.
@@ -123,19 +159,90 @@ public interface PortForwardManager {
      * Superfluous if {@link #acquirePublicPort(String, Location, int)} was used,
      * but strongly recommended if {@link #acquirePublicPortExplicit(String, int)} was used
      * e.g. if the location is not known ahead of time.
+     * 
+     * @deprecated Use {@link #associate(String, HostAndPort, Location, int)}
      */
+    @Deprecated
     public void associate(String publicIpId, int publicPort, Location l, int privatePort);
 
-    /** Returns the subset of port mappings associated with a given location. */
-    public Collection<PortMapping> getLocationPublicIpIds(Location l);
-        
-    /** Returns the mapping to a given private port, or null if none. */
-    public PortMapping getPortMappingWithPrivateSide(Location l, int privatePort);
+    /**
+     * Records a public hostname or address to be associated with the given publicIpId for lookup purposes.
+     * <p>
+     * Conceivably this may have to be access-location specific.
+     * 
+     * @deprecated Use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
+     */
+    @Deprecated
+    public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress);
 
     /**
-     * Returns true if this implementation is a client which is immutable/safe for serialization
-     * i.e. it delegates to something on an entity or location elsewhere.
+     * Returns a recorded public hostname or address.
+     * 
+     * @deprecated Use {@link #lookup(String, int)} or {@link #lookup(Location, int)}
      */
-    public boolean isClient();
+    @Deprecated
+    public String getPublicIpHostname(String publicIpId);
     
+    /**
+     * Clears a previous call to {@link #recordPublicIpHostname(String, String)}.
+     * 
+     * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMapping(Location, int)}
+     */
+    @Deprecated
+    public boolean forgetPublicIpHostname(String publicIpId);
+
+
+    ///////////////////////////////////////////////////////////////////////////////////
+    // Deprecated; just internal
+    ///////////////////////////////////////////////////////////////////////////////////
+
+    /** 
+     * Returns the port mapping for a given publicIpId and public port.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort);
+
+    /** 
+     * Returns the subset of port mappings associated with a given public IP ID.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId);
+
+    /** 
+     * @see #forgetPortMapping(String, int)
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public boolean forgetPortMapping(PortMapping m);
+
+    /**
+     * Returns the public host and port for use accessing the given mapping.
+     * <p>
+     * Conceivably this may have to be access-location specific.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public HostAndPort getPublicHostAndPort(PortMapping m);
+
+    /** 
+     * Returns the subset of port mappings associated with a given location.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public Collection<PortMapping> getLocationPublicIpIds(Location l);
+        
+    /** 
+     * Returns the mapping to a given private port, or null if none.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public PortMapping getPortMappingWithPrivateSide(Location l, int privatePort);
 }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/location/access/PortForwardManagerAuthority.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerAuthority.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerAuthority.java
index 9f7b1cf..98786e9 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManagerAuthority.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerAuthority.java
@@ -18,234 +18,10 @@
  */
 package brooklyn.location.access;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.atomic.AtomicInteger;
-
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import brooklyn.entity.Entity;
-import brooklyn.entity.basic.EntityInternal;
-import brooklyn.location.Location;
-
-import com.google.common.net.HostAndPort;
 
 /**
- * This implementation is not very efficient, and currently has a cap of about 50000 rules.
- * (TODO improve the efficiency and scale)
+ * @deprecated since 0.7.0; use {@link PortForwardManagerImpl}
  */
-public class PortForwardManagerAuthority implements PortForwardManager {
-
-    private static final Logger log = LoggerFactory.getLogger(PortForwardManagerAuthority.class);
-    
-    protected Entity owningEntity;
-    
-    protected final Map<String,PortMapping> mappings = new LinkedHashMap<String,PortMapping>();
-    
-    protected final Map<String,String> publicIpIdToHostname = new LinkedHashMap<String,String>();
-    
-    // horrible hack -- see javadoc above
-    AtomicInteger portReserved = new AtomicInteger(11000);
-
-    public PortForwardManagerAuthority() {
-    }
-    
-    public PortForwardManagerAuthority(Entity owningEntity) {
-        this.owningEntity = owningEntity;
-    }
-    
-    public synchronized void injectOwningEntity(Entity owningEntity) {
-        if (this.owningEntity!=null && owningEntity!=null && !this.owningEntity.equals(owningEntity))
-            throw new IllegalStateException("Cannot set owningEntity for "+this+" to "+owningEntity+" when it is already "+this.owningEntity);
-        this.owningEntity = owningEntity;
-        onChanged();
-    }
-    
-    /** {@inheritDoc} */
-    @Override
-    public int acquirePublicPort(String publicIpId) {
-        int port;
-        synchronized (this) {
-            // far too simple -- see javadoc above
-            port = portReserved.incrementAndGet();
-            
-            PortMapping mapping = new PortMapping(publicIpId, port, null, -1);
-            log.debug("allocating public port "+port+" at "+publicIpId+" (no association info yet)");
-            
-            mappings.put(makeKey(publicIpId, port), mapping);
-        }
-        onChanged();
-        return port;
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public PortMapping acquirePublicPortExplicit(String publicIpId, int port) {
-        PortMapping mapping = new PortMapping(publicIpId, port, null, -1);
-        log.debug("assigning explicit public port "+port+" at "+publicIpId);
-        PortMapping result = mappings.put(makeKey(publicIpId, port), mapping);
-        onChanged();
-        return result;
-    }
-
-    protected String makeKey(String publicIpId, int publicPort) {
-        return publicIpId+":"+publicPort;
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public synchronized PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort) {
-        return mappings.get(makeKey(publicIpId, publicPort));
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public synchronized Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId) {
-        List<PortMapping> result = new ArrayList<PortMapping>();
-        for (PortMapping m: mappings.values())
-            if (publicIpId.equals(m.publicIpId)) result.add(m);
-        return result;
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public PortMapping forgetPortMapping(String publicIpId, int publicPort) {
-        PortMapping result;
-        synchronized (this) {
-            result = mappings.remove(makeKey(publicIpId, publicPort));
-            log.debug("clearing port mapping for "+publicIpId+":"+publicPort+" - "+result);
-        }
-        if (result != null) onChanged();
-        return result;
-    }
-    
-    /** {@inheritDoc} */
-    @Override
-    public boolean forgetPortMapping(PortMapping m) {
-        return (forgetPortMapping(m.publicIpId, m.publicPort) != null);
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress) {
-        log.debug("recording public IP "+publicIpId+" associated with "+hostnameOrPublicIpAddress);
-        synchronized (publicIpIdToHostname) {
-            String old = publicIpIdToHostname.put(publicIpId, hostnameOrPublicIpAddress);
-            if (old!=null && !old.equals(hostnameOrPublicIpAddress))
-                log.warn("Changing hostname recorded against public IP "+publicIpId+"; from "+old+" to "+hostnameOrPublicIpAddress);
-        }
-        onChanged();
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public String getPublicIpHostname(String publicIpId) {
-        synchronized (publicIpIdToHostname) {
-            return publicIpIdToHostname.get(publicIpId);
-        }
-    }
-    
-    /** {@inheritDoc} */
-    @Override
-    public boolean forgetPublicIpHostname(String publicIpId) {
-        log.debug("forgetting public IP "+publicIpId+" association");
-        boolean result;
-        synchronized (publicIpIdToHostname) {
-            result = (publicIpIdToHostname.remove(publicIpId) != null);
-        }
-        onChanged();
-        return result;
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public HostAndPort getPublicHostAndPort(PortMapping m) {
-        String hostname = getPublicIpHostname(m.publicIpId);
-        if (hostname==null)
-            throw new IllegalStateException("No public hostname associated with "+m.publicIpId+" (mapping "+m+")");
-        return HostAndPort.fromParts(hostname, m.publicPort);
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public int acquirePublicPort(String publicIpId, Location l, int privatePort) {
-        int publicPort;
-        synchronized (this) {
-            PortMapping old = getPortMappingWithPrivateSide(l, privatePort);
-            // only works for 1 public IP ID per location (which is the norm)
-            if (old!=null && old.publicIpId.equals(publicIpId)) {
-                log.debug("request to acquire public port at "+publicIpId+" for "+l+":"+privatePort+", reusing old assignment "+old);
-                return old.getPublicPort();
-            }
-            
-            publicPort = acquirePublicPort(publicIpId);
-            log.debug("request to acquire public port at "+publicIpId+" for "+l+":"+privatePort+", allocating "+publicPort);
-            associateImpl(publicIpId, publicPort, l, privatePort);
-        }
-        onChanged();
-        return publicPort;
-    }
-
-    /** {@inheritDoc} */
-    @Override
-    public synchronized HostAndPort lookup(Location l, int privatePort) {
-        for (PortMapping m: mappings.values()) {
-            if (l.equals(m.target) && privatePort==m.privatePort)
-                return getPublicHostAndPort(m);
-        }
-        return null;
-    }
-    
-    /** {@inheritDoc} */
-    @Override
-    public synchronized void associate(String publicIpId, int publicPort, Location l, int privatePort) {
-        synchronized (this) {
-            associateImpl(publicIpId, publicPort, l, privatePort);
-        }
-        onChanged();
-    }
-
-    protected void associateImpl(String publicIpId, int publicPort, Location l, int privatePort) {
-        PortMapping mapping = getPortMappingWithPublicSide(publicIpId, publicPort);
-        log.debug("associating public port "+publicPort+" on "+publicIpId+" with private port "+privatePort+" at "+l+" ("+mapping+")");
-        if (mapping==null)
-            throw new IllegalStateException("No record of port mapping for "+publicIpId+":"+publicPort);
-        PortMapping mapping2 = new PortMapping(publicIpId, publicPort, l, privatePort);
-        mappings.put(makeKey(mapping.publicIpId, mapping.publicPort), mapping2);
-    }
-
-    /** returns the subset of port mappings associated with a given location */
-    public synchronized Collection<PortMapping> getLocationPublicIpIds(Location l) {
-        List<PortMapping> result = new ArrayList<PortMapping>();
-        for (PortMapping m: mappings.values())
-            if (l.equals(m.getTarget())) result.add(m);
-        return result;
-    }
-        
-    public synchronized PortMapping getPortMappingWithPrivateSide(Location l, int privatePort) {
-        for (PortMapping m: mappings.values())
-            if (l.equals(m.getTarget()) && privatePort==m.privatePort) return m;
-        return null;
-    }
-
-    @Override
-    public String toString() {
-        return getClass().getName()+"["+mappings+"]";
-    }
-
-    @Override
-    public boolean isClient() {
-        return false;
-    }
-
-    protected void onChanged() {
-        if (owningEntity != null) {
-            ((EntityInternal)owningEntity).requestPersist();
-        }
-    }
+@Deprecated
+public class PortForwardManagerAuthority extends PortForwardManagerImpl {
 }
-

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java
index d849564..f651a9c 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java
@@ -19,7 +19,10 @@
 package brooklyn.location.access;
 
 import java.util.Collection;
+import java.util.Map;
 
+import brooklyn.config.ConfigKey;
+import brooklyn.config.ConfigKey.HasConfigKey;
 import brooklyn.entity.Entity;
 import brooklyn.event.AttributeSensor;
 import brooklyn.location.Location;
@@ -29,6 +32,9 @@ import com.google.common.base.Preconditions;
 import com.google.common.base.Supplier;
 import com.google.common.net.HostAndPort;
 
+/**
+ * @deprecated since 0.7.0; just use the {@link PortForwardManager}, or a direct reference to its impl {@link PortForwardManagerImpl}
+ */
 public class PortForwardManagerClient implements PortForwardManager {
 
     protected final Supplier<PortForwardManager> delegateSupplier;
@@ -89,69 +95,275 @@ public class PortForwardManagerClient implements PortForwardManager {
         return _delegate;
     }
 
+    @Override
     public int acquirePublicPort(String publicIpId) {
         return getDelegate().acquirePublicPort(publicIpId);
     }
 
-    public PortMapping acquirePublicPortExplicit(String publicIpId, int port) {
-        return getDelegate().acquirePublicPortExplicit(publicIpId, port);
+    @Override
+    public void associate(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort) {
+        getDelegate().associate(publicIpId, publicEndpoint, l, privatePort);
     }
 
-    public PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort) {
-        return getDelegate().getPortMappingWithPublicSide(publicIpId, publicPort);
+    @Override
+    public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort) {
+        getDelegate().associate(publicIpId, publicEndpoint, privatePort);
     }
 
-    public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId) {
-        return getDelegate().getPortMappingWithPublicIpId(publicIpId);
+    @Override
+    public HostAndPort lookup(Location l, int privatePort) {
+        return getDelegate().lookup(l, privatePort);
+    }
+
+    @Override
+    public HostAndPort lookup(String publicIpId, int privatePort) {
+        return getDelegate().lookup(publicIpId, privatePort);
     }
 
-    public PortMapping forgetPortMapping(String publicIpId, int publicPort) {
+    @Override
+    public boolean forgetPortMapping(String publicIpId, int publicPort) {
         return getDelegate().forgetPortMapping(publicIpId, publicPort);
     }
 
-    public boolean forgetPortMapping(PortMapping m) {
-        return getDelegate().forgetPortMapping(m);
+    @Override
+    public boolean forgetPortMappings(Location location) {
+        return getDelegate().forgetPortMappings(location);
+    }
+
+    @Override
+    public String getId() {
+        return getDelegate().getId();
+    }    
+
+    @Override
+    public String getScope() {
+        return getDelegate().getScope();
+    }    
+
+    @Override
+    public boolean isClient() {
+        return true;
     }
 
+    @Override
+    public String toVerboseString() {
+        return getClass().getName()+"[wrapping="+getDelegate().toVerboseString()+"]";
+    }
+    
+    
+    ///////////////////////////////////////////////////////////////////////////////////
+    // Deprecated
+    ///////////////////////////////////////////////////////////////////////////////////
+
+    /**
+     * Reserves a unique public port for the purpose of forwarding to the given target,
+     * associated with a given location for subsequent lookup purpose.
+     * <p>
+     * If already allocated, returns the previously allocated.
+     * 
+     * @deprecated since 0.7.0; use {@link #acquirePublicPort(String)}, and then use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
+     */
+    @Deprecated
+    public int acquirePublicPort(String publicIpId, Location l, int privatePort) {
+        return getDelegate().acquirePublicPort(publicIpId, l, privatePort);
+    }
+
+    /** 
+     * Returns old mapping if it existed, null if it is new.
+     * 
+     * @deprecated since 0.7.0; use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
+     */
+    @Deprecated
+    public PortMapping acquirePublicPortExplicit(String publicIpId, int publicPort) {
+        return getDelegate().acquirePublicPortExplicit(publicIpId, publicPort);
+    }
+
+    /**
+     * Records a location and private port against a publicIp and public port,
+     * to support {@link #lookup(Location, int)}.
+     * <p>
+     * Superfluous if {@link #acquirePublicPort(String, Location, int)} was used,
+     * but strongly recommended if {@link #acquirePublicPortExplicit(String, int)} was used
+     * e.g. if the location is not known ahead of time.
+     * 
+     * @deprecated Use {@link #associate(String, HostAndPort, Location, int)}
+     */
+    @Deprecated
+    public void associate(String publicIpId, int publicPort, Location l, int privatePort) {
+        getDelegate().associate(publicIpId, publicPort, l, privatePort);
+    }
+
+    /**
+     * Records a public hostname or address to be associated with the given publicIpId for lookup purposes.
+     * <p>
+     * Conceivably this may have to be access-location specific.
+     * 
+     * @deprecated Use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
+     */
+    @Deprecated
     public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress) {
         getDelegate().recordPublicIpHostname(publicIpId, hostnameOrPublicIpAddress);
     }
 
+    /**
+     * Returns a recorded public hostname or address.
+     * 
+     * @deprecated Use {@link #lookup(String, int)} or {@link #lookup(Location, int)}
+     */
+    @Deprecated
     public String getPublicIpHostname(String publicIpId) {
         return getDelegate().getPublicIpHostname(publicIpId);
     }
-
+    
+    /**
+     * Clears a previous call to {@link #recordPublicIpHostname(String, String)}.
+     * 
+     * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMapping(Location, int)}
+     */
+    @Deprecated
     public boolean forgetPublicIpHostname(String publicIpId) {
         return getDelegate().forgetPublicIpHostname(publicIpId);
     }
 
-    public HostAndPort getPublicHostAndPort(PortMapping m) {
-        return getDelegate().getPublicHostAndPort(m);
+
+    ///////////////////////////////////////////////////////////////////////////////////
+    // Deprecated; just internal
+    ///////////////////////////////////////////////////////////////////////////////////
+
+    /** 
+     * Returns the port mapping for a given publicIpId and public port.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort) {
+        return getDelegate().getPortMappingWithPublicSide(publicIpId, publicPort);
     }
 
-    public int acquirePublicPort(String publicIpId, Location l, int privatePort) {
-        return getDelegate().acquirePublicPort(publicIpId, l, privatePort);
+    /** 
+     * Returns the subset of port mappings associated with a given public IP ID.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId) {
+        return getDelegate().getPortMappingWithPublicIpId(publicIpId);
     }
 
-    public HostAndPort lookup(Location l, int privatePort) {
-        return getDelegate().lookup(l, privatePort);
+    /** 
+     * @see #forgetPortMapping(String, int)
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public boolean forgetPortMapping(PortMapping m) {
+        return getDelegate().forgetPortMapping(m);
     }
 
-    public void associate(String publicIpId, int publicPort, Location l, int privatePort) {
-        getDelegate().associate(publicIpId, publicPort, l, privatePort);
+    /**
+     * Returns the public host and port for use accessing the given mapping.
+     * <p>
+     * Conceivably this may have to be access-location specific.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
+    public HostAndPort getPublicHostAndPort(PortMapping m) {
+        return getDelegate().getPublicHostAndPort(m);
     }
 
+    /** 
+     * Returns the subset of port mappings associated with a given location.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
     public Collection<PortMapping> getLocationPublicIpIds(Location l) {
         return getDelegate().getLocationPublicIpIds(l);
     }
-
+        
+    /** 
+     * Returns the mapping to a given private port, or null if none.
+     * 
+     * @deprecated since 0.7.0; this method will be internal only
+     */
+    @Deprecated
     public PortMapping getPortMappingWithPrivateSide(Location l, int privatePort) {
         return getDelegate().getPortMappingWithPrivateSide(l, privatePort);
     }
     
     @Override
-    public boolean isClient() {
-        return true;
+    public String toString() {
+        return getClass().getName()+"[id="+getId()+"]";
+    }
+
+    @Override
+    public String getDisplayName() {
+        return getDelegate().getDisplayName();
+    }
+
+    @Override
+    public Location getParent() {
+        return getDelegate().getParent();
+    }
+
+    @Override
+    public Collection<Location> getChildren() {
+        return getDelegate().getChildren();
+    }
+
+    @Override
+    public void setParent(Location newParent) {
+        throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public boolean containsLocation(Location potentialDescendent) {
+        return getDelegate().containsLocation(potentialDescendent);
+    }
+
+    @Override
+    public <T> T getConfig(ConfigKey<T> key) {
+        return getDelegate().getConfig(key);
+    }
+
+    @Override
+    public <T> T getConfig(HasConfigKey<T> key) {
+        return getDelegate().getConfig(key);
+    }
+
+    @Override
+    public boolean hasConfig(ConfigKey<?> key, boolean includeInherited) {
+        return getDelegate().hasConfig(key, includeInherited);
+    }
+
+    @Override
+    public Map<String, Object> getAllConfig(boolean includeInherited) {
+        return getDelegate().getAllConfig(includeInherited);
+    }
+
+    @Override
+    public boolean hasExtension(Class<?> extensionType) {
+        return getDelegate().hasExtension(extensionType);
+    }
+
+    @Override
+    public <T> T getExtension(Class<T> extensionType) {
+        return getDelegate().getExtension(extensionType);
+    }
+
+    @Override
+    public String getCatalogItemId() {
+        return getDelegate().getCatalogItemId();
+    }
+
+    @Override
+    public TagSupport tags() {
+        return getDelegate().tags();
+    }
+
+    @Override
+    public TagSupport getTagSupport() {
+        return getDelegate().getTagSupport();
     }
-    
 }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
new file mode 100644
index 0000000..759eccb
--- /dev/null
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
@@ -0,0 +1,408 @@
+/*
+ * 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 brooklyn.location.access;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import brooklyn.config.ConfigKey;
+import brooklyn.entity.basic.ConfigKeys;
+import brooklyn.entity.rebind.BasicLocationRebindSupport;
+import brooklyn.entity.rebind.RebindContext;
+import brooklyn.entity.rebind.RebindSupport;
+import brooklyn.location.Location;
+import brooklyn.location.basic.AbstractLocation;
+import brooklyn.mementos.LocationMemento;
+import brooklyn.util.collections.MutableMap;
+
+import com.google.common.annotations.Beta;
+import com.google.common.base.Objects.ToStringHelper;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.net.HostAndPort;
+
+@SuppressWarnings("serial")
+public class PortForwardManagerImpl extends AbstractLocation implements PortForwardManager {
+
+    // TODO This implementation is not efficient, and currently has a cap of about 50000 rules.
+    // Need to improve the efficiency and scale.
+    // A quick win could be to use a different portReserved counter for each publicIpId,
+    // when calling acquirePublicPort?
+    
+    // TODO Callers need to be more careful in acquirePublicPort for which ports are actually in use.
+    // If multiple apps sharing the same public-ip (e.g. in the same vcloud-director vOrg) then they 
+    // must not allocate the same public port (so can share the same PortForwardManagerAuthority
+    // via PortForwardManager.Factory.sharedInstance).
+    // However, this still doesn't check if the port is *actually* available. For example, if a
+    // different Brooklyn instance is also deploying there then we can get port conflicts, or if 
+    // some ports in that range are already in use (e.g. due to earlier dev/test runs) then this
+    // will not be respected. Callers should probably figure out the port number themselves, but
+    // that also leads to concurrency issues.
+
+    // TODO The publicIpId means different things to different callers:
+    //  - In acquirePublicPort() it is (often?) an identifier of the actual public ip.
+    //  - In later calls to associate(), it is (often?) an identifier for the target machine
+    //    such as the jcloudsMachine.getJcloudsId().
+    
+    // TODO Should PortForwardManager be changed to extend AbstractLocation, or is this wrapper ok?
+    
+    private static final Logger log = LoggerFactory.getLogger(PortForwardManagerImpl.class);
+    
+    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
+            "scope",
+            "The scope that this applies to, defaulting to global",
+            "global");
+
+    @Beta
+    public static final ConfigKey<Integer> PORT_FORWARD_MANAGER_STARTING_PORT = ConfigKeys.newIntegerConfigKey(
+            "brooklyn.portForwardManager.startingPort",
+            "The starting port for assigning port numbers, such as for DNAT",
+            11000);
+    
+    protected final Map<String,PortMapping> mappings = new LinkedHashMap<String,PortMapping>();
+    
+    @Deprecated
+    protected final Map<String,String> publicIpIdToHostname = new LinkedHashMap<String,String>();
+    
+    // horrible hack -- see javadoc above
+    private final AtomicInteger portReserved = new AtomicInteger(11000);
+
+    public PortForwardManagerImpl() {
+        super();
+        if (isLegacyConstruction()) {
+            log.warn("Deprecated construction of "+PortForwardManagerImpl.class.getName()+"; instead use location resolver");
+        }
+    }
+    
+    @Override
+    public void init() {
+        super.init();
+        Integer portStartingPoint;
+        Object rawPort = getAllConfigBag().getStringKey(PORT_FORWARD_MANAGER_STARTING_PORT.getName());
+        if (rawPort != null) {
+            portStartingPoint = getConfig(PORT_FORWARD_MANAGER_STARTING_PORT);
+        } else {
+            portStartingPoint = getManagementContext().getConfig().getConfig(PORT_FORWARD_MANAGER_STARTING_PORT);
+        }
+        portReserved.set(portStartingPoint);
+        log.debug(this+" set initial port to "+portStartingPoint);
+    }
+
+    // TODO Need to use attributes for these so they are persisted (once a location is an entity),
+    // rather than this deprecated approach of custom fields.
+    @Override
+    public RebindSupport<LocationMemento> getRebindSupport() {
+        return new BasicLocationRebindSupport(this) {
+            @Override public LocationMemento getMemento() {
+                return getMementoWithProperties(MutableMap.<String,Object>of(
+                        "mappings", mappings, 
+                        "portReserved", portReserved.get(), 
+                        "publicIpIdToHostname", publicIpIdToHostname));
+            }
+            @Override
+            protected void doReconstruct(RebindContext rebindContext, LocationMemento memento) {
+                super.doReconstruct(rebindContext, memento);
+                mappings.putAll((Map<String, PortMapping>) memento.getCustomField("mappings"));
+                portReserved.set((Integer)memento.getCustomField("portReserved"));
+                publicIpIdToHostname.putAll((Map<String, String>)memento.getCustomField("publicIpIdToHostname"));
+            }
+        };
+    }
+    
+    @Override
+    public int acquirePublicPort(String publicIpId) {
+        int port;
+        synchronized (this) {
+            // far too simple -- see javadoc above
+            port = getNextPort();
+            
+            // TODO When delete deprecated code, stop registering PortMapping until associate() is called
+            PortMapping mapping = new PortMapping(publicIpId, port, null, -1);
+            log.debug(this+" allocating public port "+port+" on "+publicIpId+" (no association info yet)");
+            
+            mappings.put(makeKey(publicIpId, port), mapping);
+        }
+        onChanged();
+        return port;
+    }
+
+    protected int getNextPort() {
+        // far too simple -- see javadoc above
+        return portReserved.incrementAndGet();
+    }
+    
+    @Override
+    public void associate(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort) {
+        associateImpl(publicIpId, publicEndpoint, l, privatePort);
+    }
+
+    @Override
+    public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort) {
+        associateImpl(publicIpId, publicEndpoint, null, privatePort);
+    }
+
+    protected void associateImpl(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort) {
+        synchronized (this) {
+            String publicIp = publicEndpoint.getHostText();
+            int publicPort = publicEndpoint.getPort();
+            recordPublicIpHostname(publicIpId, publicIp);
+            PortMapping mapping = new PortMapping(publicIpId, publicEndpoint, l, privatePort);
+            PortMapping oldMapping = getPortMappingWithPublicSide(publicIpId, publicPort);
+            log.debug(this+" associating public "+publicEndpoint+" on "+publicIpId+" with private port "+privatePort+" at "+l+" ("+mapping+")"
+                    +(oldMapping == null ? "" : " (overwriting "+oldMapping+" )"));
+            mappings.put(makeKey(publicIpId, publicPort), mapping);
+        }
+        onChanged();
+    }
+
+    @Override
+    public synchronized HostAndPort lookup(Location l, int privatePort) {
+        for (PortMapping m: mappings.values()) {
+            if (l.equals(m.target) && privatePort == m.privatePort)
+                return getPublicHostAndPort(m);
+        }
+        return null;
+    }
+    
+    @Override
+    public synchronized HostAndPort lookup(String publicIpId, int privatePort) {
+        for (PortMapping m: mappings.values()) {
+            if (publicIpId.equals(m.publicIpId) && privatePort==m.privatePort)
+                return getPublicHostAndPort(m);
+        }
+        return null;
+    }
+    
+    @Override
+    public boolean forgetPortMapping(String publicIpId, int publicPort) {
+        PortMapping old;
+        synchronized (this) {
+            old = mappings.remove(makeKey(publicIpId, publicPort));
+            log.debug("cleared port mapping for "+publicIpId+":"+publicPort+" - "+old);
+        }
+        if (old != null) onChanged();
+        return (old != null);
+    }
+    
+    @Override
+    public boolean forgetPortMappings(Location l) {
+        List<PortMapping> result = Lists.newArrayList();
+        synchronized (this) {
+            for (Iterator<PortMapping> iter = result.iterator(); iter.hasNext();) {
+                PortMapping m = iter.next();
+                if (l.equals(m.target)) {
+                    iter.remove();
+                    result.add(m);
+                }
+            }
+        }
+        log.debug("cleared all port mappings for "+l+" - "+result);
+        if (!result.isEmpty()) {
+            onChanged();
+        }
+        return !result.isEmpty();
+    }
+    
+    @Override
+    protected ToStringHelper string() {
+        return super.string().add("scope", getScope()).add("mappingsSize", mappings.size());
+    }
+
+    @Override
+    public String toVerboseString() {
+        return string().add("mappings", mappings).toString();
+    }
+
+    @Override
+    public String getScope() {
+        return checkNotNull(getConfig(SCOPE), "scope");
+    }
+
+    @Override
+    public boolean isClient() {
+        return false;
+    }
+
+    protected String makeKey(String publicIpId, int publicPort) {
+        return publicIpId+":"+publicPort;
+    }
+
+    
+    ///////////////////////////////////////////////////////////////////////////////////
+    // Internal state, for generating memento
+    ///////////////////////////////////////////////////////////////////////////////////
+
+    public List<PortMapping> getPortMappings() {
+        synchronized (this) {
+            return ImmutableList.copyOf(mappings.values());
+        }
+    }
+    
+    public Map<String, Integer> getPortCounters() {
+        return ImmutableMap.of("global", portReserved.get());
+    }
+
+    
+    ///////////////////////////////////////////////////////////////////////////////////
+    // Deprecated
+    ///////////////////////////////////////////////////////////////////////////////////
+
+    @Override
+    @Deprecated
+    public PortMapping acquirePublicPortExplicit(String publicIpId, int port) {
+        PortMapping mapping = new PortMapping(publicIpId, port, null, -1);
+        log.debug("assigning explicit public port "+port+" at "+publicIpId);
+        PortMapping result = mappings.put(makeKey(publicIpId, port), mapping);
+        onChanged();
+        return result;
+    }
+
+    @Override
+    @Deprecated
+    public boolean forgetPortMapping(PortMapping m) {
+        return forgetPortMapping(m.publicIpId, m.publicPort);
+    }
+
+    @Override
+    @Deprecated
+    public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress) {
+        log.debug("recording public IP "+publicIpId+" associated with "+hostnameOrPublicIpAddress);
+        synchronized (publicIpIdToHostname) {
+            String old = publicIpIdToHostname.put(publicIpId, hostnameOrPublicIpAddress);
+            if (old!=null && !old.equals(hostnameOrPublicIpAddress))
+                log.warn("Changing hostname recorded against public IP "+publicIpId+"; from "+old+" to "+hostnameOrPublicIpAddress);
+        }
+        onChanged();
+    }
+
+    @Override
+    @Deprecated
+    public String getPublicIpHostname(String publicIpId) {
+        synchronized (publicIpIdToHostname) {
+            return publicIpIdToHostname.get(publicIpId);
+        }
+    }
+    
+    @Override
+    @Deprecated
+    public boolean forgetPublicIpHostname(String publicIpId) {
+        log.debug("forgetting public IP "+publicIpId+" association");
+        boolean result;
+        synchronized (publicIpIdToHostname) {
+            result = (publicIpIdToHostname.remove(publicIpId) != null);
+        }
+        onChanged();
+        return result;
+    }
+
+    @Override
+    @Deprecated
+    public int acquirePublicPort(String publicIpId, Location l, int privatePort) {
+        int publicPort;
+        synchronized (this) {
+            PortMapping old = getPortMappingWithPrivateSide(l, privatePort);
+            // only works for 1 public IP ID per location (which is the norm)
+            if (old!=null && old.publicIpId.equals(publicIpId)) {
+                log.debug("request to acquire public port at "+publicIpId+" for "+l+":"+privatePort+", reusing old assignment "+old);
+                return old.getPublicPort();
+            }
+            
+            publicPort = acquirePublicPort(publicIpId);
+            log.debug("request to acquire public port at "+publicIpId+" for "+l+":"+privatePort+", allocating "+publicPort);
+            associateImpl(publicIpId, publicPort, l, privatePort);
+        }
+        onChanged();
+        return publicPort;
+    }
+
+    @Override
+    @Deprecated
+    public void associate(String publicIpId, int publicPort, Location l, int privatePort) {
+        synchronized (this) {
+            associateImpl(publicIpId, publicPort, l, privatePort);
+        }
+        onChanged();
+    }
+
+    protected void associateImpl(String publicIpId, int publicPort, Location l, int privatePort) {
+        synchronized (this) {
+            PortMapping mapping = new PortMapping(publicIpId, publicPort, l, privatePort);
+            PortMapping oldMapping = getPortMappingWithPublicSide(publicIpId, publicPort);
+            log.debug("associating public port "+publicPort+" on "+publicIpId+" with private port "+privatePort+" at "+l+" ("+mapping+")"
+                    +(oldMapping == null ? "" : " (overwriting "+oldMapping+" )"));
+            mappings.put(makeKey(publicIpId, publicPort), mapping);
+        }
+    }
+
+    ///////////////////////////////////////////////////////////////////////////////////
+    // Internal only; make protected when deprecated interface method removed
+    ///////////////////////////////////////////////////////////////////////////////////
+
+    @Override
+    public HostAndPort getPublicHostAndPort(PortMapping m) {
+        if (m.publicEndpoint == null) {
+            String hostname = getPublicIpHostname(m.publicIpId);
+            if (hostname==null)
+                throw new IllegalStateException("No public hostname associated with "+m.publicIpId+" (mapping "+m+")");
+            return HostAndPort.fromParts(hostname, m.publicPort);
+        } else {
+            return m.publicEndpoint;
+        }
+    }
+
+    @Override
+    public synchronized PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort) {
+        return mappings.get(makeKey(publicIpId, publicPort));
+    }
+
+    @Override
+    public synchronized Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId) {
+        List<PortMapping> result = new ArrayList<PortMapping>();
+        for (PortMapping m: mappings.values())
+            if (publicIpId.equals(m.publicIpId)) result.add(m);
+        return result;
+    }
+
+    /** returns the subset of port mappings associated with a given location */
+    @Override
+    public synchronized Collection<PortMapping> getLocationPublicIpIds(Location l) {
+        List<PortMapping> result = new ArrayList<PortMapping>();
+        for (PortMapping m: mappings.values())
+            if (l.equals(m.getTarget())) result.add(m);
+        return result;
+    }
+
+    @Override
+    public synchronized PortMapping getPortMappingWithPrivateSide(Location l, int privatePort) {
+        for (PortMapping m: mappings.values())
+            if (l.equals(m.getTarget()) && privatePort==m.privatePort) return m;
+        return null;
+    }
+}

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/location/access/PortForwardManagerLocationResolver.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerLocationResolver.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerLocationResolver.java
new file mode 100644
index 0000000..e56d1c0
--- /dev/null
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerLocationResolver.java
@@ -0,0 +1,89 @@
+/*
+ * 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 brooklyn.location.access;
+
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import brooklyn.location.Location;
+import brooklyn.location.LocationSpec;
+import brooklyn.location.basic.AbstractLocationResolver;
+import brooklyn.location.basic.LocationConfigUtils;
+import brooklyn.location.basic.LocationInternal;
+import brooklyn.location.basic.LocationPredicates;
+import brooklyn.util.config.ConfigBag;
+
+import com.google.common.base.Optional;
+import com.google.common.base.Predicates;
+import com.google.common.collect.Iterables;
+
+public class PortForwardManagerLocationResolver extends AbstractLocationResolver {
+
+    private static final Logger LOG = LoggerFactory.getLogger(PortForwardManagerLocationResolver.class);
+
+    public static final String PREFIX = "portForwardManager";
+
+    @Override
+    public String getPrefix() {
+        return PREFIX;
+    }
+
+    @Override
+    public Location newLocationFromString(Map locationFlags, String spec, brooklyn.location.LocationRegistry registry) {
+        ConfigBag config = extractConfig(locationFlags, spec, registry);
+        Map globalProperties = registry.getProperties();
+        String namedLocation = (String) locationFlags.get(LocationInternal.NAMED_SPEC_NAME.getName());
+        String scope = config.get(PortForwardManager.SCOPE);
+
+        Optional<Location> result = Iterables.tryFind(managementContext.getLocationManager().getLocations(), 
+                Predicates.and(
+                        Predicates.instanceOf(PortForwardManager.class), 
+                        LocationPredicates.configEqualTo(PortForwardManager.SCOPE, scope)));
+        
+        if (result.isPresent()) {
+            return result.get();
+        } else {
+            PortForwardManager loc = managementContext.getLocationManager().createLocation(LocationSpec.create(PortForwardManagerImpl.class)
+                    .configure(config.getAllConfig())
+                    .configure(LocationConfigUtils.finalAndOriginalSpecs(spec, locationFlags, globalProperties, namedLocation)));
+            
+            if (LOG.isDebugEnabled()) LOG.debug("Created "+loc+" for scope "+scope);
+            return loc;
+        }
+    }
+
+    @Override
+    protected Class<? extends Location> getLocationType() {
+        return PortForwardManager.class;
+    }
+
+    @Override
+    protected SpecParser getSpecParser() {
+        return new AbstractLocationResolver.SpecParser(getPrefix()).setExampleUsage("\"portForwardManager\" or \"portForwardManager(scope=global)\"");
+    }
+    
+    @Override
+    protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, brooklyn.location.LocationRegistry registry) {
+        ConfigBag config = super.extractConfig(locationFlags, spec, registry);
+        config.putAsStringKeyIfAbsent("name", "localhost");
+        return config;
+    }
+}

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/location/access/PortMapping.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortMapping.java b/core/src/main/java/brooklyn/location/access/PortMapping.java
index 0092a44..85f49ce 100644
--- a/core/src/main/java/brooklyn/location/access/PortMapping.java
+++ b/core/src/main/java/brooklyn/location/access/PortMapping.java
@@ -19,28 +19,37 @@
 package brooklyn.location.access;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import brooklyn.location.Location;
 
 import com.google.common.base.Objects;
-
-import brooklyn.location.Location;
+import com.google.common.net.HostAndPort;
 
 public class PortMapping {
+
+    final String publicIpId;
+    final HostAndPort publicEndpoint;
+    final int publicPort;
+
+    final Location target;
+    final int privatePort;
+    // TODO CIDR's ?
+
+    public PortMapping(String publicIpId, HostAndPort publicEndpoint, Location target, int privatePort) {
+        this.publicIpId = checkNotNull(publicIpId, "publicIpId");
+        this.publicEndpoint = checkNotNull(publicEndpoint, "publicEndpoint");
+        this.publicPort = publicEndpoint.getPort();
+        this.target = target;
+        this.privatePort = privatePort;
+    }
     
     public PortMapping(String publicIpId, int publicPort, Location target, int privatePort) {
-        super();
         this.publicIpId = checkNotNull(publicIpId, "publicIpId");
+        this.publicEndpoint = null;
         this.publicPort = publicPort;
         this.target = target;
         this.privatePort = privatePort;
     }
 
-    final String publicIpId;
-    final int publicPort;
-
-    final Location target;
-    final int privatePort;
-    // CIDR's ?
-
     public int getPublicPort() {
         return publicPort;
     }
@@ -48,14 +57,19 @@ public class PortMapping {
     public Location getTarget() {
         return target;
     }
+    
     public int getPrivatePort() {
         return privatePort;
     }
     
     @Override
     public String toString() {
-        return Objects.toStringHelper(this).add("public", publicIpId+":"+publicPort).
-                add("private", target+":"+privatePort).toString();
+        return Objects.toStringHelper(this)
+                .add("publicIpId", publicIpId+":"+publicPort)
+                .add("publicEndpoint", (publicEndpoint == null ? publicPort : publicEndpoint))
+                .add("targetLocation", target)
+                .add("targetPort", privatePort)
+                .toString();
     }
     
     @Override

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/location/basic/AbstractLocationResolver.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/basic/AbstractLocationResolver.java b/core/src/main/java/brooklyn/location/basic/AbstractLocationResolver.java
index 132edfb..2fe1769 100644
--- a/core/src/main/java/brooklyn/location/basic/AbstractLocationResolver.java
+++ b/core/src/main/java/brooklyn/location/basic/AbstractLocationResolver.java
@@ -131,7 +131,7 @@ public abstract class AbstractLocationResolver implements LocationResolver {
         
         public SpecParser(String prefix) {
             this.prefix = prefix;
-            pattern = Pattern.compile("("+prefix.toLowerCase()+"|"+prefix.toUpperCase()+")" + "(:)?" + "(\\((.*)\\))?$");
+            pattern = Pattern.compile("("+prefix+"|"+prefix.toLowerCase()+"|"+prefix.toUpperCase()+")" + "(:)?" + "(\\((.*)\\))?$");
         }
         
         public SpecParser(String prefix, Pattern pattern) {

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java b/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java
index 52714eb..ab156b6 100644
--- a/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java
+++ b/core/src/main/java/brooklyn/management/internal/AbstractManagementContext.java
@@ -148,7 +148,7 @@ public abstract class AbstractManagementContext implements ManagementContextInte
     protected DownloadResolverManager downloadsManager;
 
     protected EntitlementManager entitlementManager;
-    
+
     private final BrooklynStorage storage;
 
     private volatile boolean running = true;

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java b/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java
index c04a31c..3066720 100644
--- a/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java
+++ b/core/src/main/java/brooklyn/management/internal/NonDeploymentManagementContext.java
@@ -600,5 +600,4 @@ public class NonDeploymentManagementContext implements ManagementContextInternal
             throw new IllegalStateException("Non-deployment context "+NonDeploymentManagementContext.this+" is not valid for this operation.");
         }
     }
-    
 }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/main/resources/META-INF/services/brooklyn.location.LocationResolver
----------------------------------------------------------------------
diff --git a/core/src/main/resources/META-INF/services/brooklyn.location.LocationResolver b/core/src/main/resources/META-INF/services/brooklyn.location.LocationResolver
index 10bb0ce..74df3da 100644
--- a/core/src/main/resources/META-INF/services/brooklyn.location.LocationResolver
+++ b/core/src/main/resources/META-INF/services/brooklyn.location.LocationResolver
@@ -5,3 +5,4 @@ brooklyn.location.basic.ByonLocationResolver
 brooklyn.location.basic.SingleMachineLocationResolver
 brooklyn.location.basic.HostLocationResolver
 brooklyn.location.basic.MultiLocationResolver
+brooklyn.location.access.PortForwardManagerLocationResolver
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java b/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
new file mode 100644
index 0000000..cc2677a
--- /dev/null
+++ b/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
@@ -0,0 +1,83 @@
+/*
+ * 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 brooklyn.location.access;
+
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import brooklyn.entity.basic.Entities;
+import brooklyn.location.Location;
+import brooklyn.management.internal.LocalManagementContext;
+import brooklyn.test.entity.LocalManagementContextForTests;
+
+public class PortForwardManagerLocationResolverTest {
+
+    private LocalManagementContext managementContext;
+
+    @BeforeMethod(alwaysRun=true)
+    public void setUp() throws Exception {
+        managementContext = LocalManagementContextForTests.newInstance();
+    }
+    
+    @AfterMethod(alwaysRun=true)
+    public void tearDown() throws Exception {
+        if (managementContext != null) Entities.destroyAll(managementContext);
+    }
+    
+    @Test
+    public void testReturnsSameInstanceBasedOnScope() {
+        Location global1 = resolve("portForwardManager()"); // defaults to global
+        Location global2 = resolve("portForwardManager()");
+        Location global3 = resolve("portForwardManager(scope=global)");
+        assertSame(global1, global2);
+        assertSame(global1, global3);
+        
+        Location a1 = resolve("portForwardManager(scope=a)");
+        Location a2 = resolve("portForwardManager(scope=a)");
+        assertSame(a1, a2);
+        assertNotSame(global1, a1);
+        
+        Location b1 = resolve("portForwardManager(scope=b)");
+        assertNotSame(global1, b1);
+        assertNotSame(a1, b1);
+    }
+
+    private Location resolve(String val) {
+        Location l = managementContext.getLocationRegistry().resolve(val);
+        Assert.assertNotNull(l);
+        return l;
+    }
+    
+    private void assertSame(Location loc1, Location loc2) {
+        Assert.assertNotNull(loc1);
+        Assert.assertTrue(loc1 instanceof PortForwardManager, "loc1="+loc1);
+        Assert.assertSame(loc1, loc2);
+    }
+    
+    private void assertNotSame(Location loc1, Location loc2) {
+        Assert.assertNotNull(loc1);
+        Assert.assertNotNull(loc2);
+        Assert.assertTrue(loc2 instanceof PortForwardManager, "loc1="+loc1);
+        Assert.assertTrue(loc2 instanceof PortForwardManager, "loc2="+loc2);
+        Assert.assertNotSame(loc1, loc2);
+        Assert.assertNotEquals(((PortForwardManager)loc1).getId(), ((PortForwardManager)loc2).getId());
+    }
+}

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java b/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java
index 913e92d..3c5cb14 100644
--- a/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java
+++ b/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java
@@ -76,6 +76,28 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
 
         // We first wait for persisted, to ensure that it is the PortForwardManager.onChanged that is causing persistence.
         RebindTestUtils.waitForPersisted(origApp);
+        origPortForwardManager.associate(publicIpId, HostAndPort.fromParts(publicAddress, 40080), origSimulatedMachine, 80);
+     
+        newApp = rebind();
+        
+        TestEntity newEntity = (TestEntity) Iterables.find(newApp.getChildren(), Predicates.instanceOf(TestEntity.class));
+        Location newSimulatedMachine = newApp.getManagementContext().getLocationManager().getLocation(origSimulatedMachine.getId());
+        PortForwardManager newPortForwardManager = newEntity.getConfig(MyEntity.PORT_FORWARD_MANAGER);
+        
+        assertEquals(newPortForwardManager.lookup(newSimulatedMachine, 80), HostAndPort.fromParts(publicAddress, 40080));
+        assertEquals(newPortForwardManager.lookup(publicIpId, 80), HostAndPort.fromParts(publicAddress, 40080));
+    }
+    
+    @Test
+    public void testHostAndPortTransformingEnricherLegacy() throws Exception {
+        String publicIpId = "5.6.7.8";
+        String publicAddress = "5.6.7.8";
+
+        TestEntity origEntity = origApp.createAndManageChild(EntitySpec.create(TestEntity.class).impl(MyEntity.class));
+        PortForwardManager origPortForwardManager = origEntity.getConfig(MyEntity.PORT_FORWARD_MANAGER);
+
+        // We first wait for persisted, to ensure that it is the PortForwardManager.onChanged that is causing persistence.
+        RebindTestUtils.waitForPersisted(origApp);
         origPortForwardManager.recordPublicIpHostname(publicIpId, publicAddress);
         origPortForwardManager.acquirePublicPortExplicit(publicIpId, 40080);
         origPortForwardManager.associate(publicIpId, 40080, origSimulatedMachine, 80);
@@ -88,6 +110,7 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
         
         assertEquals(newPortForwardManager.getPublicIpHostname(publicIpId), publicAddress);
         assertEquals(newPortForwardManager.lookup(newSimulatedMachine, 80), HostAndPort.fromParts(publicAddress, 40080));
+        assertEquals(newPortForwardManager.lookup(publicIpId, 80), HostAndPort.fromParts(publicAddress, 40080));
     }
     
     public static class MyEntity extends TestEntityImpl {
@@ -99,8 +122,7 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
             super.init();
             
             if (getConfig(PORT_FORWARD_MANAGER) == null) {
-                PortForwardManagerAuthority pfm = new PortForwardManagerAuthority();
-                pfm.injectOwningEntity(this);
+                PortForwardManager pfm = (PortForwardManager) getManagementContext().getLocationRegistry().resolve("portForwardManager(scope=global)");
                 setAttribute(PORT_FORWARD_MANAGER_LIVE, pfm);
                 setConfig(PORT_FORWARD_MANAGER, PortForwardManagerClient.fromAttributeOnEntity(this, PORT_FORWARD_MANAGER_LIVE));
             }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java
----------------------------------------------------------------------
diff --git a/locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java b/locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java
index 26dc4ef..e695bea 100644
--- a/locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java
+++ b/locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java
@@ -664,6 +664,17 @@ public class JcloudsLocation extends AbstractCloudMachineProvisioningLocation im
                 sshMachineLocation.template = template;
             }
 
+            if (usePortForwarding && sshHostAndPortOverride.isPresent()) {
+                // Now that we have the sshMachineLocation, we can associate the port-forwarding address with it.
+                PortForwardManager portForwardManager = setup.get(PORT_FORWARDING_MANAGER);
+                if (portForwardManager != null) {
+                    portForwardManager.associate(node.getId(), sshHostAndPortOverride.get(), sshMachineLocation, node.getLoginPort());
+                } else {
+                    LOG.warn("No port-forward manager for {} so could not associate {} -> {} for {}",
+                            new Object[] {this, node.getLoginPort(), sshHostAndPortOverride, sshMachineLocation});
+                }
+            }
+
             if ("docker".equals(this.getProvider())) {
                 Map<Integer, Integer> portMappings = JcloudsUtil.dockerPortMappingsFor(this, node.getId());
                 PortForwardManager portForwardManager = setup.get(PORT_FORWARDING_MANAGER);
@@ -671,9 +682,7 @@ public class JcloudsLocation extends AbstractCloudMachineProvisioningLocation im
                     for(Integer containerPort : portMappings.keySet()) {
                         Integer hostPort = portMappings.get(containerPort);
                         String dockerHost = sshMachineLocation.getSshHostAndPort().getHostText();
-                        portForwardManager.recordPublicIpHostname(node.getId(), dockerHost);
-                        portForwardManager.acquirePublicPortExplicit(node.getId(), hostPort);
-                        portForwardManager.associate(node.getId(), hostPort, sshMachineLocation, containerPort);
+                        portForwardManager.associate(node.getId(), HostAndPort.fromParts(dockerHost, hostPort), sshMachineLocation, containerPort);
                     }
                 } else {
                     LOG.warn("No port-forward manager for {} so could not associate docker port-mappings for {}",

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsPortForwarderExtension.java
----------------------------------------------------------------------
diff --git a/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsPortForwarderExtension.java b/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsPortForwarderExtension.java
index 1e9b992..e3921b8 100644
--- a/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsPortForwarderExtension.java
+++ b/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsPortForwarderExtension.java
@@ -20,6 +20,8 @@ package brooklyn.location.jclouds.networking;
 
 import org.jclouds.compute.domain.NodeMetadata;
 
+import brooklyn.location.access.BrooklynAccessUtils;
+import brooklyn.location.access.PortForwardManager;
 import brooklyn.util.net.Cidr;
 import brooklyn.util.net.Protocol;
 
@@ -28,5 +30,14 @@ import com.google.common.net.HostAndPort;
 
 public interface JcloudsPortForwarderExtension {
 
+    /**
+     * Opens port forwarding (e.g. DNAT or iptables port-forwarding) to reach the given given 
+     * target port on this node (from the given cidr).
+     * 
+     * This should also register the port with the {@link PortForwardManager}, via 
+     * {@code portForwardManager.associate(node.getId(), result, targetPort)} so that
+     * subsequent calls to {@link BrooklynAccessUtils#getBrooklynAccessibleAddress(brooklyn.entity.Entity, int)}
+     * will know about the mapped port.
+     */
     public HostAndPort openPortForwarding(NodeMetadata node, int targetPort, Optional<Integer> optionalPublicPort, Protocol protocol, Cidr accessingCidr);
 }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/804c9eb6/software/base/src/main/java/brooklyn/entity/java/JmxSupport.java
----------------------------------------------------------------------
diff --git a/software/base/src/main/java/brooklyn/entity/java/JmxSupport.java b/software/base/src/main/java/brooklyn/entity/java/JmxSupport.java
index bddd85f..85dfd35 100644
--- a/software/base/src/main/java/brooklyn/entity/java/JmxSupport.java
+++ b/software/base/src/main/java/brooklyn/entity/java/JmxSupport.java
@@ -287,7 +287,6 @@ public class JmxSupport implements UsesJmx {
             result.put(JmxmpAgent.JMXMP_PORT_PROPERTY, jmxRemotePort);
             // with JMXMP don't try to tell it the hostname -- it isn't needed for JMXMP, and if specified
             // it will break if the hostname we see is not known at the server, e.g. a forwarding public IP
-            // (should not be present, but remove just to be sure)
             result.remove("java.rmi.server.hostname");
             break;
         case JMX_RMI_CUSTOM_AGENT:


[3/5] incubator-brooklyn git commit: PortForwardManager: incorporate review comments

Posted by al...@apache.org.
PortForwardManager: incorporate review comments


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/d2f96a21
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/d2f96a21
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/d2f96a21

Branch: refs/heads/master
Commit: d2f96a21dbc7b8e6afbe859296e1dc2588bbde3b
Parents: 4a41504
Author: Aled Sage <al...@gmail.com>
Authored: Fri Nov 28 12:18:57 2014 +0000
Committer: Aled Sage <al...@gmail.com>
Committed: Fri Nov 28 12:20:31 2014 +0000

----------------------------------------------------------------------
 .../location/access/PortForwardManager.java     | 18 +++---
 .../location/access/PortForwardManagerImpl.java | 60 +++++++++-----------
 .../PortForwardManagerLocationResolverTest.java |  2 +-
 3 files changed, 39 insertions(+), 41 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d2f96a21/core/src/main/java/brooklyn/location/access/PortForwardManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManager.java b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
index 40f76c5..e80c28a 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManager.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
@@ -41,13 +41,17 @@ import com.google.common.net.HostAndPort;
  * To use, see {@link PortForwardManagerLocationResolver}, with code such as 
  * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
  * 
- * Implementations typically will not know anything about what the firewall/IP actually is, they just handle a
- * unique identifier for it. It is recommended, however, to {@link #recordPublicIpHostname(String, String)} an
- * accessible hostname with the identifier. This is required in order to use {@link #lookup(Location, int)}.
+ * @see PortForwardManagerImpl for implementation notes and considerations.
  */
 @Beta
 public interface PortForwardManager extends Location {
 
+    /**
+     * The intention is that there is one PortForwardManager instance per "scope". If you 
+     * use global, then it will be a shared instance (for that management context). If you 
+     * pass in your own name (e.g. "docker-fjie3") then it will shared with just any other
+     * places that use that same location spec (e.g. {@code portForwardManager(scope=docker-fjie3)}).
+     */
     public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
             "scope",
             "The scope that this applies to, defaulting to global",
@@ -80,7 +84,7 @@ public interface PortForwardManager extends Location {
     public void associate(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort);
 
     /**
-     * Records a mapping for pubilcIpId:privatePort to a public endpoint, such that it can
+     * Records a mapping for publicIpId:privatePort to a public endpoint, such that it can
      * subsequently be looked up using {@link #lookup(String, int)}.
      */
     public void associate(String publicIpId, HostAndPort publicEndpoint, int privatePort);
@@ -111,7 +115,7 @@ public interface PortForwardManager extends Location {
     public HostAndPort lookup(String publicIpId, int privatePort);
 
     /** 
-     * Clears the given port mapping, returning the true if there was a match.
+     * Clears the given port mapping, returning true if there was a match.
      */
     public boolean forgetPortMapping(String publicIpId, int publicPort);
     
@@ -180,7 +184,7 @@ public interface PortForwardManager extends Location {
     /**
      * Clears a previous call to {@link #recordPublicIpHostname(String, String)}.
      * 
-     * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMapping(Location, int)}
+     * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMappings(Location)}
      */
     @Deprecated
     public boolean forgetPublicIpHostname(String publicIpId);
@@ -216,7 +220,7 @@ public interface PortForwardManager extends Location {
     public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId);
 
     /** 
-     * @see #forgetPortMapping(String, int)
+     * @see {@link #forgetPortMapping(String, int)} and {@link #forgetPortMappings(Location)}
      * 
      * @deprecated since 0.7.0; this method will be internal only
      */

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d2f96a21/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
index 5b8c072..21d7b03 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
@@ -48,44 +48,38 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 import com.google.common.net.HostAndPort;
 
+/**
+ * 
+ * @author aled
+ *
+ * TODO This implementation is not efficient, and currently has a cap of about 50000 rules.
+ * Need to improve the efficiency and scale.
+ * A quick win could be to use a different portReserved counter for each publicIpId,
+ * when calling acquirePublicPort?
+ * 
+ * TODO Callers need to be more careful in acquirePublicPort for which ports are actually in use.
+ * If multiple apps sharing the same public-ip (e.g. in the same vcloud-director vOrg) then they 
+ * must not allocate the same public port (e.g. ensure they share the same PortForwardManager
+ * by using the same scope in 
+ * {@code managementContext.getLocationRegistry().resolve("portForwardManager(scope=global)")}.
+ * However, this still doesn't check if the port is *actually* available. For example, if a
+ * different Brooklyn instance is also deploying there then we can get port conflicts, or if 
+ * some ports in that range are already in use (e.g. due to earlier dev/test runs) then this
+ * will not be respected. Callers should probably figure out the port number themselves, but
+ * that also leads to concurrency issues.
+ * 
+ * TODO The publicIpId means different things to different callers:
+ * <ul>
+ *   <li> In acquirePublicPort() it is (often?) an identifier of the actual public ip.
+ *   <li> In later calls to associate(), it is (often?) an identifier for the target machine
+ *        such as the jcloudsMachine.getJcloudsId().
+ * </ul>
+ */
 @SuppressWarnings("serial")
 public class PortForwardManagerImpl extends AbstractLocation implements PortForwardManager {
 
-    // TODO This implementation is not efficient, and currently has a cap of about 50000 rules.
-    // Need to improve the efficiency and scale.
-    // A quick win could be to use a different portReserved counter for each publicIpId,
-    // when calling acquirePublicPort?
-    
-    // TODO Callers need to be more careful in acquirePublicPort for which ports are actually in use.
-    // If multiple apps sharing the same public-ip (e.g. in the same vcloud-director vOrg) then they 
-    // must not allocate the same public port (so can share the same PortForwardManagerAuthority
-    // via PortForwardManager.Factory.sharedInstance).
-    // However, this still doesn't check if the port is *actually* available. For example, if a
-    // different Brooklyn instance is also deploying there then we can get port conflicts, or if 
-    // some ports in that range are already in use (e.g. due to earlier dev/test runs) then this
-    // will not be respected. Callers should probably figure out the port number themselves, but
-    // that also leads to concurrency issues.
-
-    // TODO The publicIpId means different things to different callers:
-    //  - In acquirePublicPort() it is (often?) an identifier of the actual public ip.
-    //  - In later calls to associate(), it is (often?) an identifier for the target machine
-    //    such as the jcloudsMachine.getJcloudsId().
-    
-    // TODO Should PortForwardManager be changed to extend AbstractLocation, or is this wrapper ok?
-    
     private static final Logger log = LoggerFactory.getLogger(PortForwardManagerImpl.class);
     
-    public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
-            "scope",
-            "The scope that this applies to, defaulting to global",
-            "global");
-
-    @Beta
-    public static final ConfigKey<Integer> PORT_FORWARD_MANAGER_STARTING_PORT = ConfigKeys.newIntegerConfigKey(
-            "brooklyn.portForwardManager.startingPort",
-            "The starting port for assigning port numbers, such as for DNAT",
-            11000);
-    
     protected final Map<String,PortMapping> mappings = new LinkedHashMap<String,PortMapping>();
     
     @Deprecated

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/d2f96a21/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java b/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
index cc2677a..a699bbc 100644
--- a/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
+++ b/core/src/test/java/brooklyn/location/access/PortForwardManagerLocationResolverTest.java
@@ -75,7 +75,7 @@ public class PortForwardManagerLocationResolverTest {
     private void assertNotSame(Location loc1, Location loc2) {
         Assert.assertNotNull(loc1);
         Assert.assertNotNull(loc2);
-        Assert.assertTrue(loc2 instanceof PortForwardManager, "loc1="+loc1);
+        Assert.assertTrue(loc1 instanceof PortForwardManager, "loc1="+loc1);
         Assert.assertTrue(loc2 instanceof PortForwardManager, "loc2="+loc2);
         Assert.assertNotSame(loc1, loc2);
         Assert.assertNotEquals(((PortForwardManager)loc1).getId(), ((PortForwardManager)loc2).getId());


[5/5] incubator-brooklyn git commit: This closes #353

Posted by al...@apache.org.
This closes #353


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/dea4a519
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/dea4a519
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/dea4a519

Branch: refs/heads/master
Commit: dea4a51954e24d371ea83e4e8a03e9f92e2e64f6
Parents: a8203c1 1dcaab1
Author: Aled Sage <al...@gmail.com>
Authored: Fri Nov 28 18:23:14 2014 +0000
Committer: Aled Sage <al...@gmail.com>
Committed: Fri Nov 28 18:23:14 2014 +0000

----------------------------------------------------------------------
 .../brooklyn/management/ManagementContext.java  |   5 +-
 .../location/access/PortForwardManager.java     | 242 ++++++++---
 .../access/PortForwardManagerAuthority.java     | 230 +---------
 .../access/PortForwardManagerClient.java        | 272 +++++++++++-
 .../location/access/PortForwardManagerImpl.java | 430 +++++++++++++++++++
 .../PortForwardManagerLocationResolver.java     |  89 ++++
 .../brooklyn/location/access/PortMapping.java   |  38 +-
 .../basic/AbstractLocationResolver.java         |   2 +-
 .../internal/AbstractManagementContext.java     |   2 +-
 .../NonDeploymentManagementContext.java         |   1 -
 .../services/brooklyn.location.LocationResolver |   1 +
 .../PortForwardManagerLocationResolverTest.java |  83 ++++
 .../access/PortForwardManagerRebindTest.java    |  63 ++-
 .../location/access/PortForwardManagerTest.java | 157 +++++++
 .../location/jclouds/JcloudsLocation.java       |  15 +-
 .../JcloudsPortForwarderExtension.java          |  11 +
 .../java/brooklyn/entity/java/JmxSupport.java   |   1 -
 17 files changed, 1297 insertions(+), 345 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/dea4a519/locations/jclouds/src/main/java/brooklyn/location/jclouds/JcloudsLocation.java
----------------------------------------------------------------------

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/dea4a519/software/base/src/main/java/brooklyn/entity/java/JmxSupport.java
----------------------------------------------------------------------


[2/5] incubator-brooklyn git commit: PortForwardManager tests and fixes:

Posted by al...@apache.org.
PortForwardManager tests and fixes:

- Add tests for PortForwardManager
- Deprecate isClient()
- AcquirePort uses the first port in range as well (i.e. use
  `getAndIncrement()` rather than `incrementAndGet()`)
- Fix forgetPortMappings(Location)


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/4a415043
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/4a415043
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/4a415043

Branch: refs/heads/master
Commit: 4a415043228820d49ed6fa42692bfa76117264d2
Parents: 804c9eb
Author: Aled Sage <al...@gmail.com>
Authored: Fri Nov 28 11:49:57 2014 +0000
Committer: Aled Sage <al...@gmail.com>
Committed: Fri Nov 28 11:49:57 2014 +0000

----------------------------------------------------------------------
 .../location/access/PortForwardManager.java     |  15 +-
 .../access/PortForwardManagerClient.java        |  26 ++-
 .../location/access/PortForwardManagerImpl.java |   4 +-
 .../access/PortForwardManagerRebindTest.java    |  39 +++--
 .../location/access/PortForwardManagerTest.java | 157 +++++++++++++++++++
 5 files changed, 216 insertions(+), 25 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/4a415043/core/src/main/java/brooklyn/location/access/PortForwardManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManager.java b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
index 7a20157..40f76c5 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManager.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
@@ -120,12 +120,6 @@ public interface PortForwardManager extends Location {
      */
     public boolean forgetPortMappings(Location location);
     
-    /**
-     * Returns true if this implementation is a client which is immutable/safe for serialization
-     * i.e. it delegates to something on an entity or location elsewhere.
-     */
-    public boolean isClient();
-    
     public String toVerboseString();
 
     
@@ -191,6 +185,15 @@ public interface PortForwardManager extends Location {
     @Deprecated
     public boolean forgetPublicIpHostname(String publicIpId);
 
+    /**
+     * Returns true if this implementation is a client which is immutable/safe for serialization
+     * i.e. it delegates to something on an entity or location elsewhere.
+     * 
+     * @deprecated since 0.7.0; no need to separate client-proxy from impl
+     */
+    @Deprecated
+    public boolean isClient();
+    
 
     ///////////////////////////////////////////////////////////////////////////////////
     // Deprecated; just internal

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/4a415043/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java
index f651a9c..f3767fd 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerClient.java
@@ -35,8 +35,11 @@ import com.google.common.net.HostAndPort;
 /**
  * @deprecated since 0.7.0; just use the {@link PortForwardManager}, or a direct reference to its impl {@link PortForwardManagerImpl}
  */
+@Deprecated
 public class PortForwardManagerClient implements PortForwardManager {
 
+    private static final long serialVersionUID = -295204304305332895L;
+    
     protected final Supplier<PortForwardManager> delegateSupplier;
     private transient volatile PortForwardManager _delegate;
     
@@ -141,11 +144,6 @@ public class PortForwardManagerClient implements PortForwardManager {
     }    
 
     @Override
-    public boolean isClient() {
-        return true;
-    }
-
-    @Override
     public String toVerboseString() {
         return getClass().getName()+"[wrapping="+getDelegate().toVerboseString()+"]";
     }
@@ -163,6 +161,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated since 0.7.0; use {@link #acquirePublicPort(String)}, and then use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
      */
+    @Override
     @Deprecated
     public int acquirePublicPort(String publicIpId, Location l, int privatePort) {
         return getDelegate().acquirePublicPort(publicIpId, l, privatePort);
@@ -173,6 +172,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated since 0.7.0; use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
      */
+    @Override
     @Deprecated
     public PortMapping acquirePublicPortExplicit(String publicIpId, int publicPort) {
         return getDelegate().acquirePublicPortExplicit(publicIpId, publicPort);
@@ -188,6 +188,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated Use {@link #associate(String, HostAndPort, Location, int)}
      */
+    @Override
     @Deprecated
     public void associate(String publicIpId, int publicPort, Location l, int privatePort) {
         getDelegate().associate(publicIpId, publicPort, l, privatePort);
@@ -200,6 +201,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated Use {@link #associate(String, HostAndPort, int)} or {@link #associate(String, HostAndPort, Location, int)}
      */
+    @Override
     @Deprecated
     public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress) {
         getDelegate().recordPublicIpHostname(publicIpId, hostnameOrPublicIpAddress);
@@ -210,6 +212,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated Use {@link #lookup(String, int)} or {@link #lookup(Location, int)}
      */
+    @Override
     @Deprecated
     public String getPublicIpHostname(String publicIpId) {
         return getDelegate().getPublicIpHostname(publicIpId);
@@ -220,11 +223,18 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated Use {@link #forgetPortMapping(String, int)} or {@link #forgetPortMapping(Location, int)}
      */
+    @Override
     @Deprecated
     public boolean forgetPublicIpHostname(String publicIpId) {
         return getDelegate().forgetPublicIpHostname(publicIpId);
     }
 
+    @Override
+    @Deprecated
+    public boolean isClient() {
+        return true;
+    }
+
 
     ///////////////////////////////////////////////////////////////////////////////////
     // Deprecated; just internal
@@ -235,6 +245,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated since 0.7.0; this method will be internal only
      */
+    @Override
     @Deprecated
     public PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort) {
         return getDelegate().getPortMappingWithPublicSide(publicIpId, publicPort);
@@ -245,6 +256,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated since 0.7.0; this method will be internal only
      */
+    @Override
     @Deprecated
     public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId) {
         return getDelegate().getPortMappingWithPublicIpId(publicIpId);
@@ -255,6 +267,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated since 0.7.0; this method will be internal only
      */
+    @Override
     @Deprecated
     public boolean forgetPortMapping(PortMapping m) {
         return getDelegate().forgetPortMapping(m);
@@ -267,6 +280,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated since 0.7.0; this method will be internal only
      */
+    @Override
     @Deprecated
     public HostAndPort getPublicHostAndPort(PortMapping m) {
         return getDelegate().getPublicHostAndPort(m);
@@ -277,6 +291,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated since 0.7.0; this method will be internal only
      */
+    @Override
     @Deprecated
     public Collection<PortMapping> getLocationPublicIpIds(Location l) {
         return getDelegate().getLocationPublicIpIds(l);
@@ -287,6 +302,7 @@ public class PortForwardManagerClient implements PortForwardManager {
      * 
      * @deprecated since 0.7.0; this method will be internal only
      */
+    @Override
     @Deprecated
     public PortMapping getPortMappingWithPrivateSide(Location l, int privatePort) {
         return getDelegate().getPortMappingWithPrivateSide(l, privatePort);

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/4a415043/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
index 759eccb..5b8c072 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
@@ -155,7 +155,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
 
     protected int getNextPort() {
         // far too simple -- see javadoc above
-        return portReserved.incrementAndGet();
+        return portReserved.getAndIncrement();
     }
     
     @Override
@@ -215,7 +215,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     public boolean forgetPortMappings(Location l) {
         List<PortMapping> result = Lists.newArrayList();
         synchronized (this) {
-            for (Iterator<PortMapping> iter = result.iterator(); iter.hasNext();) {
+            for (Iterator<PortMapping> iter = mappings.values().iterator(); iter.hasNext();) {
                 PortMapping m = iter.next();
                 if (l.equals(m.target)) {
                     iter.remove();

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/4a415043/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java b/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java
index 3c5cb14..5ad0296 100644
--- a/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java
+++ b/core/src/test/java/brooklyn/location/access/PortForwardManagerRebindTest.java
@@ -19,8 +19,7 @@
 package brooklyn.location.access;
 
 import static org.testng.Assert.assertEquals;
-
-import java.util.Map;
+import static org.testng.Assert.assertNotEquals;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -39,7 +38,6 @@ import brooklyn.location.LocationSpec;
 import brooklyn.location.basic.SshMachineLocation;
 import brooklyn.test.entity.TestEntity;
 import brooklyn.test.entity.TestEntityImpl;
-import brooklyn.util.flags.SetFromFlag;
 import brooklyn.util.net.Networking;
 
 import com.google.common.base.Predicates;
@@ -50,11 +48,9 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
 
     private static final Logger log = LoggerFactory.getLogger(PortForwardManagerRebindTest.class);
 
-    private Map<HostAndPort, HostAndPort> portMapping;
     private String machineAddress = "1.2.3.4";
     private SshMachineLocation origSimulatedMachine;
 
-    
     @Override
     @BeforeMethod(alwaysRun=true)
     public void setUp() throws Exception {
@@ -67,7 +63,7 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
     }
     
     @Test
-    public void testHostAndPortTransformingEnricher() throws Exception {
+    public void testAssociationPreservedOnRebind() throws Exception {
         String publicIpId = "5.6.7.8";
         String publicAddress = "5.6.7.8";
 
@@ -80,6 +76,7 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
      
         newApp = rebind();
         
+        // After rebind, confirm that lookups still work
         TestEntity newEntity = (TestEntity) Iterables.find(newApp.getChildren(), Predicates.instanceOf(TestEntity.class));
         Location newSimulatedMachine = newApp.getManagementContext().getLocationManager().getLocation(origSimulatedMachine.getId());
         PortForwardManager newPortForwardManager = newEntity.getConfig(MyEntity.PORT_FORWARD_MANAGER);
@@ -89,7 +86,7 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
     }
     
     @Test
-    public void testHostAndPortTransformingEnricherLegacy() throws Exception {
+    public void testAssociationPreservedOnRebindLegacy() throws Exception {
         String publicIpId = "5.6.7.8";
         String publicAddress = "5.6.7.8";
 
@@ -104,6 +101,7 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
      
         newApp = rebind();
         
+        // After rebind, confirm that lookups still work
         TestEntity newEntity = (TestEntity) Iterables.find(newApp.getChildren(), Predicates.instanceOf(TestEntity.class));
         Location newSimulatedMachine = newApp.getManagementContext().getLocationManager().getLocation(origSimulatedMachine.getId());
         PortForwardManager newPortForwardManager = newEntity.getConfig(MyEntity.PORT_FORWARD_MANAGER);
@@ -113,6 +111,27 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
         assertEquals(newPortForwardManager.lookup(publicIpId, 80), HostAndPort.fromParts(publicAddress, 40080));
     }
     
+    @Test
+    public void testAcquirePortCounterPreservedOnRebindLegacy() throws Exception {
+        String publicIpId = "5.6.7.8";
+
+        TestEntity origEntity = origApp.createAndManageChild(EntitySpec.create(TestEntity.class).impl(MyEntity.class));
+        PortForwardManager origPortForwardManager = origEntity.getConfig(MyEntity.PORT_FORWARD_MANAGER);
+
+        // We first wait for persisted, to ensure that it is the PortForwardManager.onChanged that is causing persistence.
+        RebindTestUtils.waitForPersisted(origApp);
+        int acquiredPort = origPortForwardManager.acquirePublicPort(publicIpId);
+     
+        newApp = rebind();
+        
+        // After rebind, confirm that lookups still work
+        TestEntity newEntity = (TestEntity) Iterables.find(newApp.getChildren(), Predicates.instanceOf(TestEntity.class));
+        PortForwardManager newPortForwardManager = newEntity.getConfig(MyEntity.PORT_FORWARD_MANAGER);
+        
+        int acquiredPort2 = newPortForwardManager.acquirePublicPort(publicIpId);
+        assertNotEquals(acquiredPort, acquiredPort2);
+    }
+    
     public static class MyEntity extends TestEntityImpl {
         public static final ConfigKey<PortForwardManager> PORT_FORWARD_MANAGER = ConfigKeys.newConfigKey(PortForwardManager.class, "myentity.portForwardManager");
         public static final AttributeSensor<PortForwardManager> PORT_FORWARD_MANAGER_LIVE = Sensors.newSensor(PortForwardManager.class, "myentity.portForwardManager.live");
@@ -124,12 +143,8 @@ public class PortForwardManagerRebindTest extends RebindTestFixtureWithApp {
             if (getConfig(PORT_FORWARD_MANAGER) == null) {
                 PortForwardManager pfm = (PortForwardManager) getManagementContext().getLocationRegistry().resolve("portForwardManager(scope=global)");
                 setAttribute(PORT_FORWARD_MANAGER_LIVE, pfm);
-                setConfig(PORT_FORWARD_MANAGER, PortForwardManagerClient.fromAttributeOnEntity(this, PORT_FORWARD_MANAGER_LIVE));
+                setConfig(PORT_FORWARD_MANAGER, pfm);
             }
         }
-        
-        @SetFromFlag("myconfigflagname")
-        public static final ConfigKey<String> MY_CONFIG_WITH_FLAGNAME = ConfigKeys.newStringConfigKey("myentity.myconfigwithflagname");
     }
-    
 }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/4a415043/core/src/test/java/brooklyn/location/access/PortForwardManagerTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/brooklyn/location/access/PortForwardManagerTest.java b/core/src/test/java/brooklyn/location/access/PortForwardManagerTest.java
new file mode 100644
index 0000000..62c4f65
--- /dev/null
+++ b/core/src/test/java/brooklyn/location/access/PortForwardManagerTest.java
@@ -0,0 +1,157 @@
+/*
+ * 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 brooklyn.location.access;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+import static org.testng.Assert.assertNull;
+
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import brooklyn.config.BrooklynProperties;
+import brooklyn.entity.BrooklynAppUnitTestSupport;
+import brooklyn.entity.basic.Entities;
+import brooklyn.location.LocationSpec;
+import brooklyn.location.basic.SshMachineLocation;
+import brooklyn.test.entity.LocalManagementContextForTests;
+import brooklyn.util.net.Networking;
+
+import com.google.common.net.HostAndPort;
+
+public class PortForwardManagerTest extends BrooklynAppUnitTestSupport {
+
+    private static final Logger log = LoggerFactory.getLogger(PortForwardManagerTest.class);
+
+    private Map<HostAndPort, HostAndPort> portMapping;
+    private SshMachineLocation machine1;
+    private SshMachineLocation machine2;
+    private PortForwardManager pfm;
+    
+    @Override
+    @BeforeMethod(alwaysRun=true)
+    public void setUp() throws Exception {
+        super.setUp();
+
+        pfm = (PortForwardManager) mgmt.getLocationRegistry().resolve("portForwardManager(scope=global)");
+
+        machine1 = mgmt.getLocationManager().createLocation(LocationSpec.create(SshMachineLocation.class)
+                .configure("address", Networking.getInetAddressWithFixedName("1.2.3.4"))
+                .configure("port", 1234)
+                .configure("user", "myuser"));
+        machine2 = mgmt.getLocationManager().createLocation(LocationSpec.create(SshMachineLocation.class)
+                .configure("address", Networking.getInetAddressWithFixedName("1.2.3.5"))
+                .configure("port", 1234)
+                .configure("user", "myuser"));
+    }
+    
+    @Test
+    public void testAssociateWithLocation() throws Exception {
+        String publicIpId = "myipid";
+        String publicAddress = "5.6.7.8";
+
+        pfm.associate(publicIpId, HostAndPort.fromParts(publicAddress, 40080), machine1, 80);
+     
+        assertEquals(pfm.lookup(machine1, 80), HostAndPort.fromParts(publicAddress, 40080));
+        assertEquals(pfm.lookup(publicIpId, 80), HostAndPort.fromParts(publicAddress, 40080));
+    }
+    
+    @Test
+    public void testAssociateWithoutLocation() throws Exception {
+        String publicIpId = "myipid";
+        String publicAddress = "5.6.7.8";
+
+        pfm.associate(publicIpId, HostAndPort.fromParts(publicAddress, 40080), 80);
+     
+        assertEquals(pfm.lookup(publicIpId, 80), HostAndPort.fromParts(publicAddress, 40080));
+        assertNull(pfm.lookup(machine1, 80));
+    }
+    
+    @Test
+    public void testAcquirePortDoesNotReturnDuplicate() throws Exception {
+        String publicIpId = "myipid";
+
+        int port1 = pfm.acquirePublicPort(publicIpId);
+        int port2 = pfm.acquirePublicPort(publicIpId);
+        assertNotEquals(port1, port2);
+    }
+    
+    @Test
+    public void testAcquirePortRespectsStartingPortNumber() throws Exception {
+        BrooklynProperties props = BrooklynProperties.Factory.newEmpty();
+        props.put(PortForwardManager.PORT_FORWARD_MANAGER_STARTING_PORT, 1234);
+        LocalManagementContextForTests mgmt2 = new LocalManagementContextForTests(props);
+        try {
+            PortForwardManager pfm2 = (PortForwardManager) mgmt2.getLocationRegistry().resolve("portForwardManager(scope=global)");
+            int port = pfm2.acquirePublicPort("myipid");
+            assertEquals(port, 1234);
+        } finally {
+            Entities.destroyAll(mgmt2);
+        }
+    }
+    
+    @Test
+    public void testForgetPortMapping() throws Exception {
+        String publicIpId = "myipid";
+        String publicAddress = "5.6.7.8";
+
+        pfm.associate(publicIpId, HostAndPort.fromParts(publicAddress, 40080), machine1, 80);
+        pfm.associate(publicIpId, HostAndPort.fromParts(publicAddress, 40081), machine1, 81);
+        pfm.forgetPortMapping(publicIpId, 40080);
+        
+        assertNull(pfm.lookup(publicIpId, 80));
+        assertNull(pfm.lookup(machine1, 80));
+        assertEquals(pfm.lookup(publicIpId, 81), HostAndPort.fromParts(publicAddress, 40081));
+        assertEquals(pfm.lookup(publicIpId, 81), HostAndPort.fromParts(publicAddress, 40081));
+    }
+    
+    @Test
+    public void testForgetPortMappingsOfMachine() throws Exception {
+        String publicIpId = "myipid";
+        String publicIpId2 = "myipid2";
+        String publicAddress = "5.6.7.8";
+
+        pfm.associate(publicIpId, HostAndPort.fromParts(publicAddress, 40080), machine1, 80);
+        pfm.associate(publicIpId, HostAndPort.fromParts(publicAddress, 40081), machine1, 81);
+        pfm.associate(publicIpId2, HostAndPort.fromParts(publicAddress, 40082), machine2, 80);
+        pfm.forgetPortMappings(machine1);
+        
+        assertNull(pfm.lookup(machine1, 80));
+        assertNull(pfm.lookup(machine1, 81));
+        assertNull(pfm.lookup(publicIpId, 80));
+        assertEquals(pfm.lookup(machine2, 80), HostAndPort.fromParts(publicAddress, 40082));
+    }
+    
+    @Test
+    public void testAssociateLegacy() throws Exception {
+        String publicIpId = "myipid";
+        String publicAddress = "5.6.7.8";
+
+        pfm.acquirePublicPortExplicit(publicIpId, 40080);
+        pfm.recordPublicIpHostname(publicIpId, publicAddress);
+        pfm.associate(publicIpId, 40080, machine1, 80);
+        
+        assertEquals(pfm.lookup(publicIpId, 80), HostAndPort.fromParts(publicAddress, 40080));
+        assertEquals(pfm.lookup(machine1, 80), HostAndPort.fromParts(publicAddress, 40080));
+    }
+}


[4/5] incubator-brooklyn git commit: PortForwardManager: incorporate review comments

Posted by al...@apache.org.
PortForwardManager: incorporate review comments

- for synchronisation


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/1dcaab17
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/1dcaab17
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/1dcaab17

Branch: refs/heads/master
Commit: 1dcaab1752ee8f3572dc4e6f2132241be4e82af6
Parents: d2f96a2
Author: Aled Sage <al...@gmail.com>
Authored: Fri Nov 28 13:56:35 2014 +0000
Committer: Aled Sage <al...@gmail.com>
Committed: Fri Nov 28 17:16:19 2014 +0000

----------------------------------------------------------------------
 .../location/access/PortForwardManager.java     |   4 +
 .../location/access/PortForwardManagerImpl.java | 108 ++++++++++++-------
 2 files changed, 72 insertions(+), 40 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1dcaab17/core/src/main/java/brooklyn/location/access/PortForwardManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManager.java b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
index e80c28a..00bcb75 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManager.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManager.java
@@ -52,6 +52,10 @@ public interface PortForwardManager extends Location {
      * pass in your own name (e.g. "docker-fjie3") then it will shared with just any other
      * places that use that same location spec (e.g. {@code portForwardManager(scope=docker-fjie3)}).
      */
+    // TODO Note: using name "scope" rather than "brooklyn.portForwardManager.scope" so that location spec 
+    // "portForwardManager(scope=global)" works, rather than having to do 
+    // portForwardManager(brooklyn.portForwardManager.scope=global).
+    // The config being read by the PortForwardManagerLocationResolver doesn't respect @SetFromFlag("scope").
     public static final ConfigKey<String> SCOPE = ConfigKeys.newStringConfigKey(
             "scope",
             "The scope that this applies to, defaulting to global",

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/1dcaab17/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
index 21d7b03..7d30a69 100644
--- a/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
+++ b/core/src/main/java/brooklyn/location/access/PortForwardManagerImpl.java
@@ -31,8 +31,6 @@ import java.util.concurrent.atomic.AtomicInteger;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import brooklyn.config.ConfigKey;
-import brooklyn.entity.basic.ConfigKeys;
 import brooklyn.entity.rebind.BasicLocationRebindSupport;
 import brooklyn.entity.rebind.RebindContext;
 import brooklyn.entity.rebind.RebindSupport;
@@ -41,7 +39,6 @@ import brooklyn.location.basic.AbstractLocation;
 import brooklyn.mementos.LocationMemento;
 import brooklyn.util.collections.MutableMap;
 
-import com.google.common.annotations.Beta;
 import com.google.common.base.Objects.ToStringHelper;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -88,6 +85,8 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     // horrible hack -- see javadoc above
     private final AtomicInteger portReserved = new AtomicInteger(11000);
 
+    private final Object mutex = new Object();
+    
     public PortForwardManagerImpl() {
         super();
         if (isLegacyConstruction()) {
@@ -115,10 +114,16 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     public RebindSupport<LocationMemento> getRebindSupport() {
         return new BasicLocationRebindSupport(this) {
             @Override public LocationMemento getMemento() {
+                Map<String, PortMapping> mappingsCopy;
+                Map<String,String> publicIpIdToHostnameCopy;
+                synchronized (mutex) {
+                    mappingsCopy = ImmutableMap.copyOf(mappings);
+                    publicIpIdToHostnameCopy = ImmutableMap.copyOf(publicIpIdToHostname);
+                }
                 return getMementoWithProperties(MutableMap.<String,Object>of(
-                        "mappings", mappings, 
+                        "mappings", mappingsCopy, 
                         "portReserved", portReserved.get(), 
-                        "publicIpIdToHostname", publicIpIdToHostname));
+                        "publicIpIdToHostname", publicIpIdToHostnameCopy));
             }
             @Override
             protected void doReconstruct(RebindContext rebindContext, LocationMemento memento) {
@@ -133,7 +138,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     @Override
     public int acquirePublicPort(String publicIpId) {
         int port;
-        synchronized (this) {
+        synchronized (mutex) {
             // far too simple -- see javadoc above
             port = getNextPort();
             
@@ -163,7 +168,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     }
 
     protected void associateImpl(String publicIpId, HostAndPort publicEndpoint, Location l, int privatePort) {
-        synchronized (this) {
+        synchronized (mutex) {
             String publicIp = publicEndpoint.getHostText();
             int publicPort = publicEndpoint.getPort();
             recordPublicIpHostname(publicIpId, publicIp);
@@ -177,27 +182,31 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     }
 
     @Override
-    public synchronized HostAndPort lookup(Location l, int privatePort) {
-        for (PortMapping m: mappings.values()) {
-            if (l.equals(m.target) && privatePort == m.privatePort)
-                return getPublicHostAndPort(m);
+    public HostAndPort lookup(Location l, int privatePort) {
+        synchronized (mutex) {
+            for (PortMapping m: mappings.values()) {
+                if (l.equals(m.target) && privatePort == m.privatePort)
+                    return getPublicHostAndPort(m);
+            }
         }
         return null;
     }
     
     @Override
-    public synchronized HostAndPort lookup(String publicIpId, int privatePort) {
-        for (PortMapping m: mappings.values()) {
-            if (publicIpId.equals(m.publicIpId) && privatePort==m.privatePort)
-                return getPublicHostAndPort(m);
-        }
+    public HostAndPort lookup(String publicIpId, int privatePort) {
+        synchronized (mutex) {
+            for (PortMapping m: mappings.values()) {
+                if (publicIpId.equals(m.publicIpId) && privatePort==m.privatePort)
+                    return getPublicHostAndPort(m);
+            }
+}
         return null;
     }
     
     @Override
     public boolean forgetPortMapping(String publicIpId, int publicPort) {
         PortMapping old;
-        synchronized (this) {
+        synchronized (mutex) {
             old = mappings.remove(makeKey(publicIpId, publicPort));
             log.debug("cleared port mapping for "+publicIpId+":"+publicPort+" - "+old);
         }
@@ -208,7 +217,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     @Override
     public boolean forgetPortMappings(Location l) {
         List<PortMapping> result = Lists.newArrayList();
-        synchronized (this) {
+        synchronized (mutex) {
             for (Iterator<PortMapping> iter = mappings.values().iterator(); iter.hasNext();) {
                 PortMapping m = iter.next();
                 if (l.equals(m.target)) {
@@ -217,7 +226,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
                 }
             }
         }
-        log.debug("cleared all port mappings for "+l+" - "+result);
+        if (log.isDebugEnabled()) log.debug("cleared all port mappings for "+l+" - "+result);
         if (!result.isEmpty()) {
             onChanged();
         }
@@ -226,12 +235,20 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     
     @Override
     protected ToStringHelper string() {
-        return super.string().add("scope", getScope()).add("mappingsSize", mappings.size());
+        int size;
+        synchronized (mutex) {
+            size = mappings.size();
+        }
+        return super.string().add("scope", getScope()).add("mappingsSize", size);
     }
 
     @Override
     public String toVerboseString() {
-        return string().add("mappings", mappings).toString();
+        String mappingsStr;
+        synchronized (mutex) {
+            mappingsStr = mappings.toString();
+        }
+        return string().add("mappings", mappingsStr).toString();
     }
 
     @Override
@@ -254,7 +271,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     ///////////////////////////////////////////////////////////////////////////////////
 
     public List<PortMapping> getPortMappings() {
-        synchronized (this) {
+        synchronized (mutex) {
             return ImmutableList.copyOf(mappings.values());
         }
     }
@@ -273,7 +290,10 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     public PortMapping acquirePublicPortExplicit(String publicIpId, int port) {
         PortMapping mapping = new PortMapping(publicIpId, port, null, -1);
         log.debug("assigning explicit public port "+port+" at "+publicIpId);
-        PortMapping result = mappings.put(makeKey(publicIpId, port), mapping);
+        PortMapping result;
+        synchronized (mutex) {
+            result = mappings.put(makeKey(publicIpId, port), mapping);
+        }
         onChanged();
         return result;
     }
@@ -288,7 +308,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     @Deprecated
     public void recordPublicIpHostname(String publicIpId, String hostnameOrPublicIpAddress) {
         log.debug("recording public IP "+publicIpId+" associated with "+hostnameOrPublicIpAddress);
-        synchronized (publicIpIdToHostname) {
+        synchronized (mutex) {
             String old = publicIpIdToHostname.put(publicIpId, hostnameOrPublicIpAddress);
             if (old!=null && !old.equals(hostnameOrPublicIpAddress))
                 log.warn("Changing hostname recorded against public IP "+publicIpId+"; from "+old+" to "+hostnameOrPublicIpAddress);
@@ -299,7 +319,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     @Override
     @Deprecated
     public String getPublicIpHostname(String publicIpId) {
-        synchronized (publicIpIdToHostname) {
+        synchronized (mutex) {
             return publicIpIdToHostname.get(publicIpId);
         }
     }
@@ -309,7 +329,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     public boolean forgetPublicIpHostname(String publicIpId) {
         log.debug("forgetting public IP "+publicIpId+" association");
         boolean result;
-        synchronized (publicIpIdToHostname) {
+        synchronized (mutex) {
             result = (publicIpIdToHostname.remove(publicIpId) != null);
         }
         onChanged();
@@ -320,7 +340,7 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     @Deprecated
     public int acquirePublicPort(String publicIpId, Location l, int privatePort) {
         int publicPort;
-        synchronized (this) {
+        synchronized (mutex) {
             PortMapping old = getPortMappingWithPrivateSide(l, privatePort);
             // only works for 1 public IP ID per location (which is the norm)
             if (old!=null && old.publicIpId.equals(publicIpId)) {
@@ -339,14 +359,14 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     @Override
     @Deprecated
     public void associate(String publicIpId, int publicPort, Location l, int privatePort) {
-        synchronized (this) {
+        synchronized (mutex) {
             associateImpl(publicIpId, publicPort, l, privatePort);
         }
         onChanged();
     }
 
     protected void associateImpl(String publicIpId, int publicPort, Location l, int privatePort) {
-        synchronized (this) {
+        synchronized (mutex) {
             PortMapping mapping = new PortMapping(publicIpId, publicPort, l, privatePort);
             PortMapping oldMapping = getPortMappingWithPublicSide(publicIpId, publicPort);
             log.debug("associating public port "+publicPort+" on "+publicIpId+" with private port "+privatePort+" at "+l+" ("+mapping+")"
@@ -372,31 +392,39 @@ public class PortForwardManagerImpl extends AbstractLocation implements PortForw
     }
 
     @Override
-    public synchronized PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort) {
-        return mappings.get(makeKey(publicIpId, publicPort));
+    public PortMapping getPortMappingWithPublicSide(String publicIpId, int publicPort) {
+        synchronized (mutex) {
+            return mappings.get(makeKey(publicIpId, publicPort));
+        }
     }
 
     @Override
-    public synchronized Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId) {
+    public Collection<PortMapping> getPortMappingWithPublicIpId(String publicIpId) {
         List<PortMapping> result = new ArrayList<PortMapping>();
-        for (PortMapping m: mappings.values())
-            if (publicIpId.equals(m.publicIpId)) result.add(m);
+        synchronized (mutex) {
+            for (PortMapping m: mappings.values())
+                if (publicIpId.equals(m.publicIpId)) result.add(m);
+        }
         return result;
     }
 
     /** returns the subset of port mappings associated with a given location */
     @Override
-    public synchronized Collection<PortMapping> getLocationPublicIpIds(Location l) {
+    public Collection<PortMapping> getLocationPublicIpIds(Location l) {
         List<PortMapping> result = new ArrayList<PortMapping>();
-        for (PortMapping m: mappings.values())
-            if (l.equals(m.getTarget())) result.add(m);
+        synchronized (mutex) {
+            for (PortMapping m: mappings.values())
+                if (l.equals(m.getTarget())) result.add(m);
+        }
         return result;
     }
 
     @Override
-    public synchronized PortMapping getPortMappingWithPrivateSide(Location l, int privatePort) {
-        for (PortMapping m: mappings.values())
-            if (l.equals(m.getTarget()) && privatePort==m.privatePort) return m;
+    public PortMapping getPortMappingWithPrivateSide(Location l, int privatePort) {
+        synchronized (mutex) {
+            for (PortMapping m: mappings.values())
+                if (l.equals(m.getTarget()) && privatePort==m.privatePort) return m;
+        }
         return null;
     }
 }