From d2bae5b76e1c1c1b72a164671b096ef824f1f160 Mon Sep 17 00:00:00 2001 From: Franco Fichtner Date: Thu, 5 Apr 2018 14:31:17 +0000 Subject: [PATCH] net/relayd: style in previous, test alignments * Test validations NotEmpty, translations and text improvements will otherwise get in the way. We know that validation failed for a particular value already. * Behaviour on an installed OPNsense seems slightly different. * Whitespace sweep and PSR2 code style * Translate custom validation messages CC: @fbrendel There is a race happening sometimes... There was 1 failure: 1) tests\OPNsense\Relayd\Api\RelaydTest::testStatusController Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'empty' +'active (1 hosts)' /usr/plugins/net/relayd/src/opnsense/mvc/tests/app/compound/OPNsense/Relayd/RelaydTest.php:428 --- net/relayd/Makefile | 5 + .../Relayd/Api/SettingsController.php | 140 +++++++++++++----- .../compound/OPNsense/Relayd/RelaydTest.php | 118 ++++++++------- 3 files changed, 169 insertions(+), 94 deletions(-) diff --git a/net/relayd/Makefile b/net/relayd/Makefile index e8c04b47a..0826605d2 100644 --- a/net/relayd/Makefile +++ b/net/relayd/Makefile @@ -5,4 +5,9 @@ PLUGIN_DEPENDS= relayd PLUGIN_COMMENT= Relayd Load Balancer PLUGIN_MAINTAINER= frank.brendel@eurolog.com +test: + @cd /usr/local/opnsense/mvc/tests && \ + phpunit --configuration PHPunit.xml \ + ${.CURDIR}/src/opnsense/mvc/tests + .include "../../Mk/plugins.mk" diff --git a/net/relayd/src/opnsense/mvc/app/controllers/OPNsense/Relayd/Api/SettingsController.php b/net/relayd/src/opnsense/mvc/app/controllers/OPNsense/Relayd/Api/SettingsController.php index 7a790542e..6e30e2cdb 100644 --- a/net/relayd/src/opnsense/mvc/app/controllers/OPNsense/Relayd/Api/SettingsController.php +++ b/net/relayd/src/opnsense/mvc/app/controllers/OPNsense/Relayd/Api/SettingsController.php @@ -50,7 +50,7 @@ class SettingsController extends ApiControllerBase * list with valid model node types */ private $nodeTypes = array('general', 'host', 'tablecheck', 'table', 'protocol', 'virtualserver'); - + /** * initialize object properties */ @@ -58,7 +58,7 @@ class SettingsController extends ApiControllerBase { $this->mdlRelayd = new Relayd(); } - + /** * query relayd settings * @param $nodeType @@ -73,7 +73,7 @@ class SettingsController extends ApiControllerBase if ($nodeType == 'general') { $node = $this->mdlRelayd->getNodeByReference($nodeType); } else { - if($uuid != null) { + if ($uuid != null) { $node = $this->mdlRelayd->getNodeByReference($nodeType . '.' . $uuid); } else { $node = $this->mdlRelayd->$nodeType->Add(); @@ -96,72 +96,89 @@ class SettingsController extends ApiControllerBase */ public function setAction($nodeType = null, $uuid = null) { - $result = array("result" => "failed", "validations" => array()); - if ($this->request->isPost() && $this->request->hasPost("relayd") && $nodeType != null) { + $result = array('result' => 'failed', 'validations' => array()); + if ($this->request->isPost() && $this->request->hasPost('relayd') && $nodeType != null) { $this->validateNodeType($nodeType); if ($nodeType == 'general') { $node = $this->mdlRelayd->getNodeByReference($nodeType); } else { - if($uuid != null) { + if ($uuid != null) { $node = $this->mdlRelayd->getNodeByReference($nodeType . '.' . $uuid); } else { $node = $this->mdlRelayd->$nodeType->Add(); } } if ($node != null) { - $relaydInfo = $this->request->getPost("relayd"); - + $relaydInfo = $this->request->getPost('relayd'); + // perform plugin specific validations if ($nodeType == 'virtualserver') { - // preset defaults for validations - if(empty($relaydInfo[$nodeType]['type'])) { + if (empty($relaydInfo[$nodeType]['type'])) { $relaydInfo[$nodeType]['type'] = $node->type->__toString(); } - if(empty($relaydInfo[$nodeType]['transport_tablemode'])) { + if (empty($relaydInfo[$nodeType]['transport_tablemode'])) { $relaydInfo[$nodeType]['transport_tablemode'] = $node->transport_tablemode->__toString(); } - if(empty($relaydInfo[$nodeType]['backuptransport_tablemode'])) { - $relaydInfo[$nodeType]['backuptransport_tablemode'] = $node->backuptransport_tablemode->__toString(); + if (empty($relaydInfo[$nodeType]['backuptransport_tablemode'])) { + $relaydInfo[$nodeType]['backuptransport_tablemode'] = + $node->backuptransport_tablemode->__toString(); } - + if ($relaydInfo[$nodeType]['type'] == 'redirect') { if ($relaydInfo[$nodeType]['transport_tablemode'] != 'least-states' && $relaydInfo[$nodeType]['transport_tablemode'] != 'roundrobin') { - $result["validations"]['relayd.virtualserver.transport_tablemode'] = "Scheduler '" . $relaydInfo[$nodeType]['transport_tablemode'] . "' not supported for redirects."; + $result['validations']['relayd.virtualserver.transport_tablemode'] = sprintf( + gettext('Scheduler "%s" not supported for redirects.'), + $relaydInfo[$nodeType]['transport_tablemode'] + ); } if ($relaydInfo[$nodeType]['backuptransport_tablemode'] != 'least-states' && $relaydInfo[$nodeType]['backuptransport_tablemode'] != 'roundrobin') { - $result["validations"]['relayd.virtualserver.backuptransport_tablemode'] = "Scheduler '" . $relaydInfo[$nodeType]['backuptransport_tablemode'] . "' not supported for redirects."; + $result['validations']['relayd.virtualserver.backuptransport_tablemode'] = sprintf( + gettext('Scheduler "%s" not supported for redirects.'), + $relaydInfo[$nodeType]['backuptransport_tablemode'] + ); } } if ($relaydInfo[$nodeType]['type'] == 'relay') { if ($relaydInfo[$nodeType]['transport_tablemode'] == 'least-states') { - $result["validations"]['relayd.virtualserver.transport_tablemode'] = "Scheduler '" . $relaydInfo[$nodeType]['transport_tablemode'] . "' not supported for relays."; + $result['validations']['relayd.virtualserver.transport_tablemode'] = sprintf( + gettext('Scheduler "%s" not supported for relays.'), + $relaydInfo[$nodeType]['transport_tablemode'] + ); } if ($relaydInfo[$nodeType]['backuptransport_tablemode'] == 'least-states') { - $result["validations"]['relayd.virtualserver.backuptransport_tablemode'] = "Scheduler '" . $relaydInfo[$nodeType]['backuptransport_tablemode'] . "' not supported for relays."; + $result['validations']['relayd.virtualserver.backuptransport_tablemode'] = sprintf( + gettext('Scheduler "%s" not supported for relays.'), + $relaydInfo[$nodeType]['backuptransport_tablemode'] + ); } } } elseif ($nodeType == 'tablecheck') { switch ($relaydInfo[$nodeType]['type']) { case 'send': if (empty($relaydInfo[$nodeType]['expect'])) { - $result["validations"]['relayd.tablecheck.expect'] = "Expect Pattern cannot be empty."; + $result['validations']['relayd.tablecheck.expect'] = + gettext('Expect Pattern cannot be empty.'); } break; case 'script': - if(empty($relaydInfo[$nodeType]['path'])) { - $result["validations"]['relayd.tablecheck.path'] = "Script path cannot be empty."; + if (empty($relaydInfo[$nodeType]['path'])) { + $result['validations']['relayd.tablecheck.path'] = + gettext('Script path cannot be empty.'); } break; case 'http': - if(empty($relaydInfo[$nodeType]['path'])) { - $result["validations"]['relayd.tablecheck.path'] = "Path cannot be empty."; + if (empty($relaydInfo[$nodeType]['path'])) { + $result['validations']['relayd.tablecheck.path'] = + gettext('Path cannot be empty.'); } - if(empty($relaydInfo[$nodeType]['code']) && empty($relaydInfo[$nodeType]['digest'])) { - $result["validations"]['relayd.tablecheck.code'] = "Provide one of Response Code or Message Digest."; - $result["validations"]['relayd.tablecheck.digest'] = "Provide one of Response Code or Message Digest."; + if (empty($relaydInfo[$nodeType]['code']) && empty($relaydInfo[$nodeType]['digest'])) { + $result['validations']['relayd.tablecheck.code'] = + gettext('Provide one of Response Code or Message Digest.'); + $result['validations']['relayd.tablecheck.digest'] = + gettext('Provide one of Response Code or Message Digest.'); } break; } @@ -178,7 +195,9 @@ class SettingsController extends ApiControllerBase $result['result'] = 'ok'; $this->mdlRelayd->serializeToConfig(); Config::getInstance()->save(); - if ($nodeType == 'general' && $relaydInfo['general']['enabled'] == '0') { + if ($nodeType == 'general' && + isset($relaydInfo[$nodeType]['enabled']) && + $relaydInfo[$nodeType]['enabled'] == '0') { $svcRelayd = new ServiceController(); $result = $svcRelayd->stopAction(); } @@ -207,23 +226,65 @@ class SettingsController extends ApiControllerBase // delete relations switch ($nodeType) { case 'host': - $this->deleteRelations('table', 'hosts', $uuid, 'host', $nodeName, $mdlRelayd); + $this->deleteRelations( + 'table', + 'hosts', + $uuid, + 'host', + $nodeName, + $this->mdlRelayd + ); break; case 'tablecheck': - $this->deleteRelations('virtualserver', 'transport_tablecheck', $uuid, 'tablecheck', $nodeName, $mdlRelayd); - $this->deleteRelations('virtualserver', 'backuptransport_tablecheck', $uuid, 'tablecheck', $nodeName, $mdlRelayd); + $this->deleteRelations( + 'virtualserver', + 'transport_tablecheck', + $uuid, + 'tablecheck', + $nodeName, + $this->mdlRelayd + ); + $this->deleteRelations( + 'virtualserver', + 'backuptransport_tablecheck', + $uuid, + 'tablecheck', + $nodeName, + $this->mdlRelayd + ); break; case 'table': - $this->deleteRelations('virtualserver', 'transport_table', $uuid, 'table', $nodeName, $mdlRelayd); - $this->deleteRelations('virtualserver', 'backuptransport_table', $uuid, 'table', $nodeName, $mdlRelayd); + $this->deleteRelations( + 'virtualserver', + 'transport_table', + $uuid, + 'table', + $nodeName, + $this->mdlRelayd + ); + $this->deleteRelations( + 'virtualserver', + 'backuptransport_table', + $uuid, + 'table', + $nodeName, + $this->mdlRelayd + ); break; case 'protocol': - $this->deleteRelations('virtualserver', 'protocol', $uuid, 'protocol', $nodeName, $mdlRelayd); + $this->deleteRelations( + 'virtualserver', + 'protocol', + $uuid, + 'protocol', + $nodeName, + $this->mdlRelayd + ); break; } $this->mdlRelayd->serializeToConfig(); Config::getInstance()->save(); - $result["result"] = "ok"; + $result['result'] = 'ok'; } } } @@ -284,8 +345,13 @@ class SettingsController extends ApiControllerBase * @param &$mdlRelayd * @throws \Exception */ - private function deleteRelations($nodeType = null, $nodeField = null, $relUuid = null, $relNodeType = null, $relNodeName = null) - { + private function deleteRelations( + $nodeType = null, + $nodeField = null, + $relUuid = null, + $relNodeType = null, + $relNodeName = null + ) { $nodes = $this->mdlRelayd->$nodeType->getNodes(); // get nodes with relations foreach ($nodes as $nodeUuid => $node) { @@ -301,7 +367,7 @@ class SettingsController extends ApiControllerBase $nodeRels = ltrim($nodeRels, ','); $this->mdlRelayd->setNodeByReference($refField, $nodeRels); if ($relNode->isEmptyAndRequired()) { - $nodeName = $this->mdlRelayd->getNodeByReference($nodeType . '.' . $nodeUuid . '.name')->__toString(); + $nodeName = $this->mdlRelayd->getNodeByReference("{$nodeType}.{$nodeUuid}.name")->__toString(); throw new \Exception("Cannot delete $relNodeType '$relNodeName' from $nodeType '$nodeName'"); } } diff --git a/net/relayd/src/opnsense/mvc/tests/app/compound/OPNsense/Relayd/RelaydTest.php b/net/relayd/src/opnsense/mvc/tests/app/compound/OPNsense/Relayd/RelaydTest.php index 715b4fd32..5450c97c0 100644 --- a/net/relayd/src/opnsense/mvc/tests/app/compound/OPNsense/Relayd/RelaydTest.php +++ b/net/relayd/src/opnsense/mvc/tests/app/compound/OPNsense/Relayd/RelaydTest.php @@ -1,44 +1,40 @@ assertInstanceOf('\OPNsense\Relayd\Api\SettingsController', self::$setRelayd); - $this->expectException(Exception); + $this->expectException(\Exception::class); $response = self::$setRelayd->getAction('wrong_node_type'); $testConfig = []; $response = self::$setRelayd->getAction('general'); @@ -124,7 +120,7 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('general'); $this->assertCount(1, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.general.interval'], 'Check interval must be greater than 0'); + $this->assertNotEmpty($response['validations']['relayd.general.interval']); // set correct interval and incorrect timeout (s. testServiceController) $_POST = array('relayd' => ['general' => ['interval' => '10', 'timeout' => 86400, 'enabled' => '0']]); @@ -145,7 +141,7 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('host'); $this->assertCount(1, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.host.name'], 'Should be a string between 1 and 255 characters. Allowed characters are letters and numbers as well as underscore, minus, dot and space.'); + $this->assertNotEmpty($response['validations']['relayd.host.name']); $this->cleanupNodes('host'); // check mask @@ -153,8 +149,8 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('host'); $this->assertCount(2, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.host.name'], 'Should be a string between 1 and 255 characters. Allowed characters are letters and numbers as well as underscore, minus, dot and space.'); - $this->assertEquals($response['validations']['relayd.host.address'], 'Please specify a valid servername or IP address.'); + $this->assertNotEmpty($response['validations']['relayd.host.name']); + $this->assertNotEmpty($response['validations']['relayd.host.address']); $this->cleanupNodes('host'); // create host for ServiceControllerTest @@ -176,17 +172,19 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('table'); $this->assertCount(2, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.table.name'], 'Should be a string between 1 and 255 characters. Allowed characters are letters and numbers as well as underscore, minus, dot and space.'); - $this->assertEquals($response['validations']['relayd.table.hosts'], 'Host not found'); + $this->assertNotEmpty($response['validations']['relayd.table.name']); + $this->assertNotEmpty($response['validations']['relayd.table.hosts']); $this->cleanupNodes('table'); // create table for ServiceControllerTest $_POST = array('current' => '1', 'rowCount' => '7', 'searchPhrase' => 'testHost'); $response = self::$setRelayd->searchAction('host'); $this->assertArrayHasKey('total', $response); - $_POST = array('relayd' => ['table' => ['name' => 'testTable', 'enabled' => 1, 'hosts' => $response['rows'][0]['uuid']]]); + $_POST = array('relayd' => [ + 'table' => ['name' => 'testTable', 'enabled' => 1, 'hosts' => $response['rows'][0]['uuid']] + ]); $response = self::$setRelayd->setAction('table'); - } + } /** * test setAction for tablechecks @@ -202,8 +200,8 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('tablecheck'); $this->assertCount(2, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.tablecheck.name'], 'Should be a string between 1 and 255 characters. Allowed characters are letters and numbers as well as underscore, minus, dot and space.'); - $this->assertEquals($response['validations']['relayd.tablecheck.type'], 'option not in list'); + $this->assertNotEmpty($response['validations']['relayd.tablecheck.name']); + $this->assertNotEmpty($response['validations']['relayd.tablecheck.type']); $this->cleanupNodes('tablecheck'); // type 'send' without 'expect' @@ -211,7 +209,7 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('tablecheck'); $this->assertCount(1, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.tablecheck.expect'], 'Expect Pattern cannot be empty.'); + $this->assertNotEmpty($response['validations']['relayd.tablecheck.expect']); $this->cleanupNodes('tablecheck'); // type 'script' without 'path' @@ -219,23 +217,25 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('tablecheck'); $this->assertCount(1, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.tablecheck.path'], 'Script path cannot be empty.'); + $this->assertNotEmpty($response['validations']['relayd.tablecheck.path']); $this->cleanupNodes('tablecheck'); // type 'http' without 'code' and 'digest' - $_POST = array('relayd' => ['tablecheck' => ['name' => 'testTableCheck', 'type' => 'http', 'path' => 'http://www.example.com']]); + $_POST = array('relayd' => [ + 'tablecheck' => ['name' => 'testTableCheck', 'type' => 'http', 'path' => 'http://www.example.com'] + ]); $response = self::$setRelayd->setAction('tablecheck'); $this->assertCount(2, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.tablecheck.code'], 'Provide one of Response Code or Message Digest.'); - $this->assertEquals($response['validations']['relayd.tablecheck.digest'], 'Provide one of Response Code or Message Digest.'); + $this->assertNotEmpty($response['validations']['relayd.tablecheck.code']); + $this->assertNotEmpty($response['validations']['relayd.tablecheck.digest']); $this->cleanupNodes('tablecheck'); // create tablecheck for ServiceControllerTest $_POST = array('relayd' => [ 'tablecheck' => [ - 'name' => 'testTableCheck', - 'type' => 'http', + 'name' => 'testTableCheck', + 'type' => 'http', 'path' => '/', 'host' => 'localhost', 'code' => '403', @@ -258,12 +258,14 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('protocol'); $this->assertCount(2, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.protocol.name'], 'Should be a string between 1 and 255 characters. Allowed characters are letters and numbers as well as underscore, minus, dot and space.'); - $this->assertEquals($response['validations']['relayd.protocol.type'], 'option not in list'); + $this->assertNotEmpty($response['validations']['relayd.protocol.name']); + $this->assertNotEmpty($response['validations']['relayd.protocol.type']); $this->cleanupNodes('protocol'); // create protocol for ServiceControllerTest - $_POST = array('relayd' => ['protocol' => ['name' => 'testProtocol', 'type' => 'tcp', 'options' => 'nodelay, socket buffer 65536']]); + $_POST = array('relayd' => [ + 'protocol' => ['name' => 'testProtocol', 'type' => 'tcp', 'options' => 'nodelay, socket buffer 65536'] + ]); $response = self::$setRelayd->setAction('protocol'); $this->assertEquals($response['result'], 'ok'); } @@ -274,7 +276,7 @@ class RelaydTest extends \PHPUnit\Framework\TestCase * @depends testReset */ public function testSetVirtualServer() - { + { $_SERVER['REQUEST_METHOD'] = 'POST'; // search table and tablecheck @@ -300,11 +302,11 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('virtualserver'); $this->assertCount(5, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.virtualserver.name'], 'Should be a string between 1 and 255 characters. Allowed characters are letters and numbers as well as underscore, minus, dot and space.'); - $this->assertEquals($response['validations']['relayd.virtualserver.listen_address'], 'Please specify a valid servername or IP address.'); - $this->assertEquals($response['validations']['relayd.virtualserver.listen_startport'], 'A valid Port number must be specified.'); - $this->assertEquals($response['validations']['relayd.virtualserver.transport_table'], 'Table not found'); - $this->assertEquals($response['validations']['relayd.virtualserver.transport_tablecheck'], 'Table check not found'); + $this->assertNotEmpty($response['validations']['relayd.virtualserver.name']); + $this->assertNotEmpty($response['validations']['relayd.virtualserver.listen_address']); + $this->assertNotEmpty($response['validations']['relayd.virtualserver.listen_startport']); + $this->assertNotEmpty($response['validations']['relayd.virtualserver.transport_table']); + $this->assertNotEmpty($response['validations']['relayd.virtualserver.transport_tablecheck']); $this->cleanupNodes('virtualserver'); // wrong tablemodes, missing ModelRelationField targets @@ -320,7 +322,7 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('virtualserver'); $this->assertCount(1, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.virtualserver.transport_tablemode'], 'Scheduler \'least-states\' not supported for relays.'); + $this->assertNotEmpty($response['validations']['relayd.virtualserver.transport_tablemode']); $this->cleanupNodes('virtualserver'); // wron scheduler, missing protocol @@ -341,8 +343,8 @@ class RelaydTest extends \PHPUnit\Framework\TestCase $response = self::$setRelayd->setAction('virtualserver'); $this->assertCount(2, $response['validations']); $this->assertEquals($response['result'], 'failed'); - $this->assertEquals($response['validations']['relayd.virtualserver.backuptransport_tablemode'], 'Scheduler \'random\' not supported for redirects.'); - $this->assertEquals($response['validations']['relayd.virtualserver.protocol'], 'Protocol not found'); + $this->assertNotEmpty($response['validations']['relayd.virtualserver.backuptransport_tablemode']); + $this->assertNotEmpty($response['validations']['relayd.virtualserver.protocol']); $this->cleanupNodes('virtualserver'); // create virtualserver for ServiceControllerTest @@ -378,7 +380,10 @@ class RelaydTest extends \PHPUnit\Framework\TestCase // generate template and test it by Relayd $response = $svcRelayd->configtestAction(); $this->assertEquals($response['template'], 'OK'); - $this->assertEquals($response['result'], "global timeout exceeds interval\ntable timeout exceeds interval: testTable:443"); + $this->assertEquals( + $response['result'], + "global timeout exceeds interval\ntable timeout exceeds interval: testTable:443" + ); $_POST = array('relayd' => ['general' => ['timeout' => '200']]); $response = self::$setRelayd->setAction('general'); $this->assertEquals($response['result'], 'ok'); @@ -406,7 +411,6 @@ class RelaydTest extends \PHPUnit\Framework\TestCase // status $response = $svcRelayd->statusAction(); $this->assertEquals($response['status'], 'running'); - } /**