From c2a2688b93b1d6156350688dbff5cb18bfad029b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Mon, 7 Aug 2017 15:47:16 +0300 Subject: [PATCH] Fix duplicate listener checks Only the protocol, port and address of the listener were used to check if a listener exists. The check should also use the name of the listener to be sure that each name is unique. Expanded tests to check that the creation of duplicate listeners is detected. Did minor improvements to related test code. --- include/maxscale/service.h | 2 +- maxctrl/test/cluster.js | 8 +++- maxctrl/test/createdestroy.js | 15 +++++++ server/core/config.cc | 4 +- server/core/config_runtime.cc | 8 ++-- server/core/service.cc | 9 ++-- server/core/test/rest-api/test/service.js | 52 ++++++++++++++++------- server/core/test/testservice.cc | 2 +- 8 files changed, 71 insertions(+), 29 deletions(-) diff --git a/include/maxscale/service.h b/include/maxscale/service.h index 2168191be..9217a3652 100644 --- a/include/maxscale/service.h +++ b/include/maxscale/service.h @@ -247,7 +247,7 @@ bool serviceHasBackend(SERVICE *service, SERVER *server); * @param port Listener port * @return True if service has the listener */ -bool serviceHasListener(SERVICE *service, const char *protocol, +bool serviceHasListener(SERVICE* service, const char* name, const char* protocol, const char* address, unsigned short port); /** diff --git a/maxctrl/test/cluster.js b/maxctrl/test/cluster.js index 06ea5eb97..c39cd625a 100644 --- a/maxctrl/test/cluster.js +++ b/maxctrl/test/cluster.js @@ -145,6 +145,7 @@ describe('Cluster Command Internals', function() { } cluster.haveExtraServices(a, b, 'test1', 'test2').should.be.rejected + cluster.haveExtraServices(b, a, 'test2', 'test1').should.be.rejected expect(cluster.haveExtraServices(a, a, 'test1', 'test2')).to.equal(undefined) expect(cluster.haveExtraServices(b, b, 'test1', 'test2')).to.equal(undefined) }) @@ -212,12 +213,17 @@ describe('Cluster Sync', function() { // As the listeners cannot be truly deleted, since there's no code for actually closing a socket at runtime, // we do the listener tests last - it('sync listener creation', function() { + it('sync listener creation/deletion', function() { return doCommand('create listener RW-Split-Router my-listener-2 5999 --hosts 127.0.0.1:8990') // As both MaxScales are on the same machine, both can't listen on the same port. The sync should fail due to this .then(() => doCommand('cluster sync 127.0.0.1:8990 --hosts 127.0.0.1:8989').should.be.rejected) // Create the listener on the second MaxScale to avoid it being synced later on .then(() => doCommand('create listener RW-Split-Router my-listener-2 5998 --hosts 127.0.0.1:8989')) + // Sync after creation should succeed + .then(() => doCommand('cluster sync 127.0.0.1:8990 --hosts 127.0.0.1:8989')) + // Destroy the created server, should succeed + .then(() => doCommand('destroy listener RW-Split-Router my-listener-2')) + .then(() => doCommand('cluster sync 127.0.0.1:8990 --hosts 127.0.0.1:8989')) }) after(stopDoubleMaxScale) diff --git a/maxctrl/test/createdestroy.js b/maxctrl/test/createdestroy.js index 67a7c6b2b..8f45ff876 100644 --- a/maxctrl/test/createdestroy.js +++ b/maxctrl/test/createdestroy.js @@ -110,11 +110,26 @@ describe("Create/Destroy Commands", function() { .should.be.fulfilled }) + it('create already existing listener', function() { + return doCommand('create listener RW-Split-Router my-listener 7890') + .should.be.rejected + }) + + it('create listener with already used port', function() { + return doCommand('create listener RW-Split-Router my-listener2 4567') + .should.be.rejected + }) + it('destroy listener', function() { return doCommand('destroy listener RW-Split-Router my-listener') .should.be.fulfilled }) + it('destroy static listener', function() { + return doCommand('destroy listener RW-Split-Router RW-Split-Listener') + .should.be.rejected + }) + it('create user', function() { return verifyCommand('create user testuser test', 'users/inet/testuser') diff --git a/server/core/config.cc b/server/core/config.cc index 981611169..74e8f3723 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -3283,7 +3283,7 @@ int create_new_listener(CONFIG_CONTEXT *obj) SSL_LISTENER *ssl_info = make_ssl_structure(obj, true, &error_count); if (socket) { - if (serviceHasListener(service, protocol, address, 0)) + if (serviceHasListener(service, obj->object, protocol, address, 0)) { MXS_ERROR("Listener '%s' for service '%s' already has a socket at '%s.", obj->object, service_name, socket); @@ -3298,7 +3298,7 @@ int create_new_listener(CONFIG_CONTEXT *obj) if (port) { - if (serviceHasListener(service, protocol, address, atoi(port))) + if (serviceHasListener(service, obj->object, protocol, address, atoi(port))) { MXS_ERROR("Listener '%s', for service '%s', already have port %s.", obj->object, diff --git a/server/core/config_runtime.cc b/server/core/config_runtime.cc index 8570eb9f3..23a3a677b 100644 --- a/server/core/config_runtime.cc +++ b/server/core/config_runtime.cc @@ -743,7 +743,7 @@ bool runtime_create_listener(SERVICE *service, const char *name, const char *add spinlock_acquire(&crt_lock); - if (!serviceHasListener(service, proto, addr, u_port)) + if (!serviceHasListener(service, name, proto, addr, u_port)) { SSL_LISTENER *ssl = NULL; @@ -807,10 +807,8 @@ bool runtime_destroy_listener(SERVICE *service, const char *name) } else { - rval = false; - MXS_WARNING("Listener '%s' was not created at runtime. Remove the " - "listener manually from the correct configuration file.", - name); + runtime_error("Listener '%s' was not created at runtime. Remove the listener " + "manually from the correct configuration file.", name); } } else diff --git a/server/core/service.cc b/server/core/service.cc index caf8e20f8..e897303db 100644 --- a/server/core/service.cc +++ b/server/core/service.cc @@ -776,7 +776,7 @@ SERV_LISTENER* serviceCreateListener(SERVICE *service, const char *name, const c * @param port The port to listen on * @return True if the protocol/port is already part of the service */ -bool serviceHasListener(SERVICE *service, const char *protocol, +bool serviceHasListener(SERVICE* service, const char* name, const char* protocol, const char* address, unsigned short port) { LISTENER_ITERATOR iter; @@ -785,9 +785,12 @@ bool serviceHasListener(SERVICE *service, const char *protocol, listener; listener = listener_iterator_next(&iter)) { if (listener_is_active(listener) && - strcmp(listener->protocol, protocol) == 0 && listener->port == port && + // Listener with same name exists + (strcmp(listener->name, name) == 0 || + // Listener listening on the same interface and port exists + ((strcmp(listener->protocol, protocol) == 0 && listener->port == port && ((address && listener->address && strcmp(listener->address, address) == 0) || - (address == NULL && listener->address == NULL))) + (address == NULL && listener->address == NULL)))))) { return true; } diff --git a/server/core/test/rest-api/test/service.js b/server/core/test/rest-api/test/service.js index df49759bc..f74196d21 100644 --- a/server/core/test/rest-api/test/service.js +++ b/server/core/test/rest-api/test/service.js @@ -63,28 +63,48 @@ describe("Service", function() { }) }); - it("create a listener", function() { - var listener = { - "links": { - "self": "http://localhost:8989/v1/services/RW-Split-Router/listeners" + const listener = { + "links": { + "self": "http://localhost:8989/v1/services/RW-Split-Router/listeners" + }, + "data": { + "attributes": { + "parameters": { + "port": 4012, + "protocol": "MySQLClient", + "authenticator": "MySQLAuth", + "address": "127.0.0.1" + } }, - "data": { - "attributes": { - "parameters": { - "port": 4012, - "protocol": "MySQLClient", - "authenticator": "MySQLAuth", - "address": "127.0.0.1" - } - }, - "id": "RW-Split-Listener-2", - "type": "listeners" - } + "id": "RW-Split-Listener-2", + "type": "listeners" } + } + it("create a listener", function() { return request.post(base_url + "/services/RW-Split-Router/listeners", {json: listener}) .should.be.fulfilled }); + it("create an already existing listener", function() { + return request.post(base_url + "/services/RW-Split-Router/listeners", {json: listener}) + .should.be.rejected + }); + + it("destroy a listener", function() { + return request.delete(base_url + "/services/RW-Split-Router/listeners/RW-Split-Listener-2") + .should.be.fulfilled + }); + + it("destroy a nonexistent listener", function() { + return request.delete(base_url + "/services/RW-Split-Router/listeners/I-bet-this-listener-exists") + .should.be.rejected + }); + + it("destroy a static listener", function() { + return request.delete(base_url + "/services/RW-Split-Router/listeners/RW-Split-Listener") + .should.be.rejected + }); + after(stopMaxScale) }); diff --git a/server/core/test/testservice.cc b/server/core/test/testservice.cc index 6b76d7d3f..d14d3a4fa 100644 --- a/server/core/test/testservice.cc +++ b/server/core/test/testservice.cc @@ -73,7 +73,7 @@ test1() ss_info_dassert(serviceCreateListener(service, "TestProtocol", "testprotocol", "localhost", 9876, "MySQLAuth", NULL, NULL), "Add Protocol should succeed"); - ss_info_dassert(0 != serviceHasListener(service, "testprotocol", "localhost", 9876), + ss_info_dassert(0 != serviceHasListener(service, "TestProtocol", "testprotocol", "localhost", 9876), "Service should have new protocol as requested"); return 0;