You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by ro...@apache.org on 2013/04/29 18:45:19 UTC
svn commit: r1477190 - in /qpid/trunk/qpid/java:
broker/src/main/java/org/apache/qpid/server/model/adapter/
broker/src/test/java/org/apache/qpid/server/configuration/startup/
systests/src/main/java/org/apache/qpid/systest/rest/
Author: robbie
Date: Mon Apr 29 16:45:19 2013
New Revision: 1477190
URL: http://svn.apache.org/r1477190
Log:
QPID-4785: relax restrictions on editing/deleting active ports outwith management-mode
Modified:
qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AmqpPortAdapter.java
qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java
qpid/trunk/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java
qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/PortRestTest.java
qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/QpidRestTestCase.java
Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AmqpPortAdapter.java
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AmqpPortAdapter.java?rev=1477190&r1=1477189&r2=1477190&view=diff
==============================================================================
--- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AmqpPortAdapter.java (original)
+++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AmqpPortAdapter.java Mon Apr 29 16:45:19 2013
@@ -193,17 +193,6 @@ public class AmqpPortAdapter extends Por
return null;
}
- @Override
- protected void changeAttributes(Map<String, Object> attributes)
- {
- if (_transport != null)
- {
- throw new IllegalStateException("Port " + getAttribute(PORT)
- + " is already opened. Start broker in management mode to change a port");
- }
- super.changeAttributes(MapValueConverter.convert(attributes, ATTRIBUTE_TYPES));
- }
-
class ServerNetworkTransportConfiguration implements NetworkTransportConfiguration
{
private final InetSocketAddress _bindingSocketAddress;
Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java?rev=1477190&r1=1477189&r2=1477190&view=diff
==============================================================================
--- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java (original)
+++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java Mon Apr 29 16:45:19 2013
@@ -160,7 +160,8 @@ public class BrokerAdapter extends Abstr
private StatisticsAdapter _statistics;
private final Map<String, VirtualHost> _vhostAdapters = new HashMap<String, VirtualHost>();
- private final Map<Integer, Port> _portAdapters = new HashMap<Integer, Port>();
+ private final Map<UUID, Port> _portAdapters = new HashMap<UUID, Port>();
+ private final Map<Port, Integer> _stillInUsePortNumbers = new HashMap<Port, Integer>();
private final Map<UUID, AuthenticationProvider> _authenticationProviders = new HashMap<UUID, AuthenticationProvider>();
private final Map<String, GroupProvider> _groupProviders = new HashMap<String, GroupProvider>();
private final Map<UUID, ConfiguredObject> _plugins = new HashMap<UUID, ConfiguredObject>();
@@ -445,20 +446,6 @@ public class BrokerAdapter extends Abstr
}
}
- private void addPort(Port port)
- {
- synchronized (_portAdapters)
- {
- int portNumber = port.getPort();
- if(_portAdapters.containsKey(portNumber))
- {
- throw new IllegalArgumentException("Cannot add port " + port + " because port number " + portNumber + " already configured");
- }
- _portAdapters.put(portNumber, port);
- }
- port.addChangeListener(this);
- }
-
/**
* Called when adding a new port via the management interface
*/
@@ -467,15 +454,49 @@ public class BrokerAdapter extends Abstr
Port port = _portFactory.createPort(UUID.randomUUID(), this, attributes);
addPort(port);
- //AMQP ports are disable during ManagementMode, and the management
- //plugins can currently only start ports at broker startup and
- //not when they are newly created via the management interfaces.
- boolean quiesce = isManagementMode() || !(port instanceof AmqpPortAdapter);
+ //1. AMQP ports are disabled during ManagementMode.
+ //2. The management plugins can currently only start ports at broker startup and
+ // not when they are newly created via the management interfaces.
+ //3. When active ports are deleted, or their port numbers updated, the broker must be
+ // restarted for it to take effect so we can't reuse port numbers until it is.
+ boolean quiesce = isManagementMode() || !(port instanceof AmqpPortAdapter) || isPreviouslyUsedPortNumber(port);
+
port.setDesiredState(State.INITIALISING, quiesce ? State.QUIESCED : State.ACTIVE);
return port;
}
+ private void addPort(Port port)
+ {
+ synchronized (_portAdapters)
+ {
+ int portNumber = port.getPort();
+ String portName = port.getName();
+ UUID portId = port.getId();
+
+ for(Port p : _portAdapters.values())
+ {
+ if(portNumber == p.getPort())
+ {
+ throw new IllegalConfigurationException("Can't add port " + portName + " because port number " + portNumber + " is already configured for port " + p.getName());
+ }
+
+ if(portName == p.getName())
+ {
+ throw new IllegalConfigurationException("Can't add Port because one with name " + portName + " already exists");
+ }
+
+ if(portId == p.getId())
+ {
+ throw new IllegalConfigurationException("Can't add Port because one with id " + portId + " already exists");
+ }
+ }
+
+ _portAdapters.put(port.getId(), port);
+ }
+ port.addChangeListener(this);
+ }
+
private AccessControlProvider createAccessControlProvider(Map<String, Object> attributes)
{
AccessControlProvider accessControlProvider = null;
@@ -771,17 +792,24 @@ public class BrokerAdapter extends Abstr
return super.getAttribute(name);
}
- private boolean deletePort(Port portAdapter)
+ private boolean deletePort(State oldState, Port portAdapter)
{
Port removedPort = null;
synchronized (_portAdapters)
{
- removedPort = _portAdapters.remove(portAdapter.getPort());
+ removedPort = _portAdapters.remove(portAdapter.getId());
}
if (removedPort != null)
{
removedPort.removeChangeListener(this);
+
+ if(oldState == State.ACTIVE)
+ {
+ //Record the originally used port numbers of previously-active ports being deleted, to ensure
+ //when creating new ports we don't try to re-bind a port number that we are currently still using
+ recordPreviouslyUsedPortNumberIfNecessary(removedPort, removedPort.getPort());
+ }
}
return removedPort != null;
@@ -907,7 +935,7 @@ public class BrokerAdapter extends Abstr
}
else if(object instanceof Port)
{
- childDeleted = deletePort((Port)object);
+ childDeleted = deletePort(oldState, (Port)object);
}
else if(object instanceof VirtualHost)
{
@@ -948,7 +976,15 @@ public class BrokerAdapter extends Abstr
@Override
public void attributeSet(ConfiguredObject object, String attributeName, Object oldAttributeValue, Object newAttributeValue)
{
- // no-op
+ if(object instanceof Port)
+ {
+ //Record all the originally used port numbers of active ports, to ensure that when
+ //creating new ports we don't try to re-bind a port number that we are still using
+ if(attributeName == Port.PORT && object.getActualState() == State.ACTIVE)
+ {
+ recordPreviouslyUsedPortNumberIfNecessary((Port) object, (Integer)oldAttributeValue);
+ }
+ }
}
private void addPlugin(ConfiguredObject plugin)
@@ -1193,4 +1229,18 @@ public class BrokerAdapter extends Abstr
return new ArrayList<AccessControlProvider>(_accessControlProviders.values());
}
}
+
+ private void recordPreviouslyUsedPortNumberIfNecessary(Port port, Integer portNumber)
+ {
+ //If we haven't previously recorded its original port number, record it now
+ if(!_stillInUsePortNumbers.containsKey(port))
+ {
+ _stillInUsePortNumbers.put(port, portNumber);
+ }
+ }
+
+ private boolean isPreviouslyUsedPortNumber(Port port)
+ {
+ return _stillInUsePortNumbers.containsValue(port.getPort());
+ }
}
Modified: qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java?rev=1477190&r1=1477189&r2=1477190&view=diff
==============================================================================
--- qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java (original)
+++ qpid/trunk/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java Mon Apr 29 16:45:19 2013
@@ -309,7 +309,7 @@ public class PortAdapter extends Abstrac
State state = _state.get();
if (desiredState == State.DELETED)
{
- if (state == State.STOPPED || state == State.QUIESCED)
+ if (state == State.INITIALISING || state == State.ACTIVE || state == State.STOPPED || state == State.QUIESCED)
{
return _state.compareAndSet(state, State.DELETED);
}
@@ -322,7 +322,15 @@ public class PortAdapter extends Abstrac
{
if ((state == State.INITIALISING || state == State.QUIESCED) && _state.compareAndSet(state, State.ACTIVE))
{
- onActivate();
+ try
+ {
+ onActivate();
+ }
+ catch(RuntimeException e)
+ {
+ _state.compareAndSet(State.ACTIVE, state);
+ throw e;
+ }
return true;
}
else
@@ -371,15 +379,27 @@ public class PortAdapter extends Abstrac
@Override
protected void changeAttributes(Map<String, Object> attributes)
{
- if (getActualState() == State.ACTIVE && !_broker.isManagementMode())
+ Map<String, Object> converted = MapValueConverter.convert(attributes, ATTRIBUTE_TYPES);
+
+ Map<String, Object> merged = generateEffectiveAttributes(converted);
+
+ String newName = (String) merged.get(NAME);
+ if(!getName().equals(newName))
{
- throw new IllegalStateException("Cannot change attributes for an active port outside of Management Mode");
+ throw new IllegalConfigurationException("Changing the port name is not allowed");
}
- Map<String, Object> converted = MapValueConverter.convert(attributes, ATTRIBUTE_TYPES);
- Map<String, Object> merged = new HashMap<String, Object>(getDefaultAttributes());
- merged.putAll(getActualAttributes());
- merged.putAll(converted);
+ Integer newPort = (Integer) merged.get(PORT);
+ if(getPort() != newPort)
+ {
+ for(Port p : _broker.getPorts())
+ {
+ if(p.getPort() == newPort)
+ {
+ throw new IllegalConfigurationException("Port number " + newPort + " is already in use by port " + p.getName());
+ }
+ }
+ }
@SuppressWarnings("unchecked")
Collection<Transport> transports = (Collection<Transport>)merged.get(TRANSPORTS);
Modified: qpid/trunk/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java?rev=1477190&r1=1477189&r2=1477190&view=diff
==============================================================================
--- qpid/trunk/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java (original)
+++ qpid/trunk/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java Mon Apr 29 16:45:19 2013
@@ -47,6 +47,7 @@ import org.apache.qpid.server.model.KeyS
import org.apache.qpid.server.model.Plugin;
import org.apache.qpid.server.model.Port;
import org.apache.qpid.server.model.TrustStore;
+import org.apache.qpid.server.model.UUIDGenerator;
import org.apache.qpid.server.model.VirtualHost;
import org.apache.qpid.server.model.adapter.AccessControlProviderFactory;
import org.apache.qpid.server.model.adapter.AuthenticationProviderFactory;
@@ -196,11 +197,13 @@ public class BrokerRecovererTest extends
//Add a couple ports
ConfigurationEntry portEntry1 = mock(ConfigurationEntry.class);
Port port1 = mock(Port.class);
+ when(port1.getId()).thenReturn(UUIDGenerator.generateRandomUUID());
when(port1.getName()).thenReturn("port1");
when(port1.getPort()).thenReturn(5671);
when(port1.getAttribute(Port.AUTHENTICATION_PROVIDER)).thenReturn("authenticationProvider1");
ConfigurationEntry portEntry2 = mock(ConfigurationEntry.class);
Port port2 = mock(Port.class);
+ when(port2.getId()).thenReturn(UUIDGenerator.generateRandomUUID());
when(port2.getName()).thenReturn("port2");
when(port2.getPort()).thenReturn(5672);
when(port2.getAttribute(Port.AUTHENTICATION_PROVIDER)).thenReturn("authenticationProvider2");
Modified: qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/PortRestTest.java
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/PortRestTest.java?rev=1477190&r1=1477189&r2=1477190&view=diff
==============================================================================
--- qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/PortRestTest.java (original)
+++ qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/PortRestTest.java Mon Apr 29 16:45:19 2013
@@ -124,8 +124,9 @@ public class PortRestTest extends QpidRe
Asserts.assertPortAttributes(port, State.ACTIVE);
// try to add a second RMI port
+ portName = portName + "2";
attributes = new HashMap<String, Object>();
- attributes.put(Port.NAME, portName + 2);
+ attributes.put(Port.NAME, portName);
attributes.put(Port.PORT, findFreePort());
attributes.put(Port.PROTOCOLS, Collections.singleton(Protocol.RMI));
@@ -162,28 +163,11 @@ public class PortRestTest extends QpidRe
attributes.put(Port.PROTOCOLS, Collections.singleton(Protocol.AMQP_0_9_1));
responseCode = getRestTestHelper().submitRequest("/rest/port/" + portName, "PUT", attributes);
- assertEquals("Port cannot be updated in non management mode", 409, responseCode);
- }
-
- public void testPutUpdateOpenedAmqpPortFails() throws Exception
- {
- Map<String, Object> port = getRestTestHelper().getJsonAsSingletonList("/rest/port/" + TestBrokerConfiguration.ENTRY_NAME_AMQP_PORT);
- Integer portValue = (Integer)port.get(Port.PORT);
-
- port.put(Port.PORT, findFreePort());
-
- int responseCode = getRestTestHelper().submitRequest("/rest/port/" + TestBrokerConfiguration.ENTRY_NAME_AMQP_PORT, "PUT", port);
- assertEquals("Unexpected response code for port update", 409, responseCode);
-
- port = getRestTestHelper().getJsonAsSingletonList("/rest/port/" + TestBrokerConfiguration.ENTRY_NAME_AMQP_PORT);
- assertEquals("Port has been changed", portValue, port.get(Port.PORT));
+ assertEquals("Unexpected response code for port update", 200, responseCode);
}
public void testUpdatePortTransportFromTCPToSSLWhenKeystoreIsConfigured() throws Exception
{
- restartBrokerInManagementMode();
- getRestTestHelper().setManagementModeCredentials();
-
String portName = TestBrokerConfiguration.ENTRY_NAME_AMQP_PORT;
Map<String, Object> attributes = new HashMap<String, Object>();
attributes.put(Port.NAME, portName);
@@ -193,9 +177,6 @@ public class PortRestTest extends QpidRe
int responseCode = getRestTestHelper().submitRequest("/rest/port/" + portName, "PUT", attributes);
assertEquals("Transport has not been changed to SSL " , 200, responseCode);
- restartBroker();
- getRestTestHelper().setUsernameAndPassword("webadmin", "webadmin");
-
Map<String, Object> port = getRestTestHelper().getJsonAsSingletonList("/rest/port/" + portName);
@SuppressWarnings("unchecked")
@@ -209,9 +190,6 @@ public class PortRestTest extends QpidRe
public void testUpdateTransportFromTCPToSSLWithoutKeystoreConfiguredFails() throws Exception
{
- restartBrokerInManagementMode();
- getRestTestHelper().setManagementModeCredentials();
-
String portName = TestBrokerConfiguration.ENTRY_NAME_AMQP_PORT;
Map<String, Object> attributes = new HashMap<String, Object>();
attributes.put(Port.NAME, portName);
@@ -235,17 +213,12 @@ public class PortRestTest extends QpidRe
int responseCode = getRestTestHelper().submitRequest("/rest/port/" + portName, "PUT", attributes);
assertEquals("SSL port was not added", 201, responseCode);
- restartBrokerInManagementMode();
- getRestTestHelper().setManagementModeCredentials();
-
attributes.put(Port.NEED_CLIENT_AUTH, true);
attributes.put(Port.WANT_CLIENT_AUTH, true);
responseCode = getRestTestHelper().submitRequest("/rest/port/" + portName, "PUT", attributes);
assertEquals("Attributes for need/want client auth are not set", 200, responseCode);
- restartBroker();
- getRestTestHelper().setUsernameAndPassword("webadmin", "webadmin");
Map<String, Object> port = getRestTestHelper().getJsonAsSingletonList("/rest/port/" + portName);
assertEquals("Unexpected " + Port.NEED_CLIENT_AUTH, true, port.get(Port.NEED_CLIENT_AUTH));
assertEquals("Unexpected " + Port.WANT_CLIENT_AUTH, true, port.get(Port.WANT_CLIENT_AUTH));
@@ -255,9 +228,6 @@ public class PortRestTest extends QpidRe
assertEquals("Unexpected auth provider", new HashSet<String>(Arrays.asList(TestBrokerConfiguration.ENTRY_NAME_SSL_TRUSTSTORE)),
new HashSet<String>(trustStores));
- restartBrokerInManagementMode();
- getRestTestHelper().setManagementModeCredentials();
-
attributes = new HashMap<String, Object>();
attributes.put(Port.NAME, portName);
attributes.put(Port.TRANSPORTS, Collections.singleton(Transport.TCP));
@@ -274,8 +244,6 @@ public class PortRestTest extends QpidRe
responseCode = getRestTestHelper().submitRequest("/rest/port/" + portName, "PUT", attributes);
assertEquals("Should be able to change transport to TCP ", 200, responseCode);
- restartBroker();
- getRestTestHelper().setUsernameAndPassword("webadmin", "webadmin");
port = getRestTestHelper().getJsonAsSingletonList("/rest/port/" + portName);
assertEquals("Unexpected " + Port.NEED_CLIENT_AUTH, false, port.get(Port.NEED_CLIENT_AUTH));
assertEquals("Unexpected " + Port.WANT_CLIENT_AUTH, false, port.get(Port.WANT_CLIENT_AUTH));
@@ -288,9 +256,6 @@ public class PortRestTest extends QpidRe
public void testUpdateSettingWantNeedCertificateFailsForNonSSLPort() throws Exception
{
- restartBrokerInManagementMode();
- getRestTestHelper().setManagementModeCredentials();
-
String portName = TestBrokerConfiguration.ENTRY_NAME_AMQP_PORT;
Map<String, Object> attributes = new HashMap<String, Object>();
attributes.put(Port.NAME, portName);
@@ -307,9 +272,6 @@ public class PortRestTest extends QpidRe
public void testUpdatePortAuthenticationProvider() throws Exception
{
- restartBrokerInManagementMode();
- getRestTestHelper().setManagementModeCredentials();
-
String portName = TestBrokerConfiguration.ENTRY_NAME_AMQP_PORT;
Map<String, Object> attributes = new HashMap<String, Object>();
attributes.put(Port.NAME, portName);
Modified: qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/QpidRestTestCase.java
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/QpidRestTestCase.java?rev=1477190&r1=1477189&r2=1477190&view=diff
==============================================================================
--- qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/QpidRestTestCase.java (original)
+++ qpid/trunk/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/QpidRestTestCase.java Mon Apr 29 16:45:19 2013
@@ -106,10 +106,4 @@ public class QpidRestTestCase extends Qp
{
return _restTestHelper;
}
-
- protected void restartBrokerInManagementMode() throws Exception
- {
- stopBroker();
- startBroker(0, true);
- }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org