From 930fa4e39fd2023342599d4422111070bee42b79 Mon Sep 17 00:00:00 2001 From: Ad Schellevis Date: Tue, 4 Jun 2024 16:12:10 +0200 Subject: [PATCH] Firewall: Aliases / generic MVC - performance improvments, closes https://github.com/opnsense/core/issues/7509 This commit improves alias save/validate performance by fixing two main issues: 1) Faster unique constraint handling using caching, in order to prevent nested loops we should cache our results while still in the validation cycle. This required an attribute to count validation cycles so we know the model could not have changed in between, getValidationSequence() helps to reach that goal 2) Alias::getByName() more optimistic caching, also prevening nested loops when locating aliases. Although this is slighly more optimistic than the previous construction, in practice when validating data the set won't change, if there is a risk of mutations, we should flush the set using the "flush" parameter (same as before, but a bit more sensitive). --- .../OPNsense/Firewall/Api/AliasController.php | 2 +- .../app/models/OPNsense/Base/BaseModel.php | 19 +++++- .../Base/Constraints/UniqueConstraint.php | 66 ++++++++++++------- .../app/models/OPNsense/Firewall/Alias.php | 31 ++++----- .../Base/Constraints/UniqueConstraintTest.php | 66 +++++++++---------- 5 files changed, 106 insertions(+), 78 deletions(-) diff --git a/src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php b/src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php index ac840ea7d..797133c15 100644 --- a/src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php +++ b/src/opnsense/mvc/app/controllers/OPNsense/Firewall/Api/AliasController.php @@ -405,7 +405,7 @@ class AliasController extends ApiMutableModelControllerBase foreach ($data['aliases']['alias'] as $uuid => $content) { $type = !empty($content['type']) ? $content['type'] : ""; if (is_array($content) && !empty($content['name']) && $type != 'internal') { - $node = $this->getModel()->getByName($content['name']); + $node = $this->getModel()->getByName($content['name'], true); if ($node == null) { $node = $this->getModel()->aliases->alias->Add(); $result['new'] += 1; diff --git a/src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php b/src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php index 2908c1d6a..26daf64d2 100644 --- a/src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php +++ b/src/opnsense/mvc/app/models/OPNsense/Base/BaseModel.php @@ -91,6 +91,11 @@ abstract class BaseModel */ private $internalMissingUuids = false; + /** + * @var int internal validation sequence (number of times validation has run) + */ + private int $internalValidationSequence = 0; + /** * If the model needs a custom initializer, override this init() method * Default behaviour is to do nothing in this init. @@ -110,7 +115,7 @@ abstract class BaseModel if ($xmlNode->count() == 0) { $result = (string)$xmlNode; } else { - $result = array(); + $result = []; foreach ($xmlNode->children() as $childNode) { // item keys can be overwritten using value attributes if (!isset($childNode->attributes()['value'])) { @@ -467,6 +472,16 @@ abstract class BaseModel return str_ends_with($this->internal_mountpoint, '+') && strpos($this->internal_mountpoint, "//") !== 0; } + /** + * Return the number of times performValidation() has been called. + * This can be practical if validations need to cache outcomes which are consistent for the full validation + * sequence. + * @return int + */ + public function getValidationSequence() + { + return $this->internalValidationSequence; + } /** * validate full model using all fields and data in a single (1 deep) array @@ -480,6 +495,8 @@ abstract class BaseModel $validation_data = array(); $all_nodes = $this->internalData->getFlatNodes(); + $this->internalValidationSequence++; + foreach ($all_nodes as $key => $node) { if ($validateFullModel || $node->isFieldChanged()) { $node_validators = $node->getValidators(); diff --git a/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php b/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php index 7047d09a4..7da128561 100644 --- a/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php +++ b/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/UniqueConstraint.php @@ -36,6 +36,9 @@ namespace OPNsense\Base\Constraints; */ class UniqueConstraint extends BaseConstraint { + private static $itemmap = []; + private static $validationSequence = 0; + /** * Executes validation * @@ -51,38 +54,53 @@ class UniqueConstraint extends BaseConstraint if (!$node->isRequired() && empty((string)$node)) { return true; } - $containerNode = $node; - $nodeName = $node->getInternalXMLTagName(); - $parentNode = $node->getParentNode(); - $level = 0; - // dive into parent - while ($containerNode != null && !$containerNode->isArrayType()) { - $containerNode = $containerNode->getParentNode(); - $level++; + $mdl = $node->getParentModel(); + if ($mdl === null || $mdl->getValidationSequence() != static::$validationSequence) { + // reset cache, new validation round. + static::$validationSequence = $mdl !== null ? $mdl->getValidationSequence() : 0; + static::$itemmap = []; } - if ($containerNode != null && $level == 2) { - // collect (additional) key fields - $keyFields = array($nodeName); - $keyFields = array_unique(array_merge($keyFields, $this->getOptionValueList('addFields'))); - // calculate the key for this node - $nodeKey = ''; - foreach ($keyFields as $field) { - $nodeKey .= $fieldSeparator . $parentNode->$field; + $keyFields = array_unique( + array_merge( + [$node->getInternalXMLTagName()], + $this->getOptionValueList('addFields') + ) + ); + asort($keyFields); + $nodeKey = implode('|', $keyFields); + $parentNode = $node->getParentNode(); + // calculate the key for this node + if (!isset(static::$itemmap[$nodeKey])) { + static::$itemmap[$nodeKey] = []; + $level = 0; + // dive into parent + $containerNode = $node; + while ($containerNode != null && !$containerNode->isArrayType()) { + $containerNode = $containerNode->getParentNode(); + $level++; } - // when an ArrayField is found in range, traverse nodes and compare keys - foreach ($containerNode->iterateItems() as $item) { - if ($item !== $parentNode) { - $itemKey = ''; + if ($containerNode != null && $level == 2) { + // when an ArrayField is found in range, traverse nodes and compare keys + foreach ($containerNode->iterateItems() as $item) { + $itemValue = ''; foreach ($keyFields as $field) { - $itemKey .= $fieldSeparator . $item->$field; + $itemValue .= $fieldSeparator . $item->$field; } - if ($itemKey == $nodeKey) { - $this->appendMessage($validator, $attribute); - return false; + if (empty(static::$itemmap[$nodeKey][$itemValue])) { + static::$itemmap[$nodeKey][$itemValue] = 0; } + static::$itemmap[$nodeKey][$itemValue]++ ; } } } + $nodeValue = ''; + foreach ($keyFields as $field) { + $nodeValue .= $fieldSeparator . $parentNode->$field; + } + if ((static::$itemmap[$nodeKey][$nodeValue] ?? 0) > 1) { + $this->appendMessage($validator, $attribute); + return false; + } } return true; } diff --git a/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php b/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php index f2e78cae3..bc164d3b9 100644 --- a/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php +++ b/src/opnsense/mvc/app/models/OPNsense/Firewall/Alias.php @@ -217,28 +217,21 @@ class Alias extends BaseModel public function getByName($name, $flush = false) { if ($flush) { - // Flush false negative results (searched earlier, didn't exist at the time) as positive matches will - // always be resolved. + // Flush cache in case model data has been changed. $this->aliasReferenceCache = []; } - // cache alias uuid references, but always validate existence before return. + if (empty($this->aliasReferenceCache)) { + // cache alias uuid references, but always validate existence before return. + foreach ($this->aliases->alias->iterateItems() as $uuid => $alias) { + if ((string)$alias->name == $name) { + $this->aliasReferenceCache[(string)$alias->name] = $alias; + } + } + } if (isset($this->aliasReferenceCache[$name])) { - $uuid = $this->aliasReferenceCache[$name]; - if ($uuid === false) { - return null; - } - $alias = $this->getNodeByReference('aliases.alias.' . $uuid); - if ($alias != null && (string)$alias->name == $name) { - return $alias; - } + return $this->aliasReferenceCache[$name]; + } else { + return null; } - foreach ($this->aliases->alias->iterateItems() as $uuid => $alias) { - if ((string)$alias->name == $name) { - $this->aliasReferenceCache[$name] = $uuid; - return $alias; - } - } - $this->aliasReferenceCache[$name] = false; - return null; } } diff --git a/src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php b/src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php index 9a55a7ffa..daac90ba9 100644 --- a/src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php +++ b/src/opnsense/mvc/tests/app/models/OPNsense/Base/Constraints/UniqueConstraintTest.php @@ -34,9 +34,8 @@ use OPNsense\Base\FieldTypes\TextField; class UniqueTestContainer extends ArrayField { - private $uniqueConstraints = []; - private $valuesRequired = false; - private $internalNodes = []; + private bool $valuesRequired = false; + private array $internalNodes = []; /** * @param $nodes a single node or an array of nodes @@ -48,47 +47,48 @@ class UniqueTestContainer extends ArrayField { // UniqueConstraint requires a depth of 2, so add a container node $container = new ContainerField(); - $constraint = new UniqueConstraint(); - $this->addChildNode('UniqueTest', $container); - $addFields = []; - $fields_added = false; + $idx = count($this->internalNodes); + $this->addChildNode('item'. $idx, $container); foreach ($nodes as $name => $value) { $node = new TextField(null, $name); $node->setRequired($this->valuesRequired ? "Y" : "N"); - if ($name === array_key_first($nodes)) { - $constraint->setOption('node', $node); - $constraint->setOption('name', $name); - $constraint->setOption('ValidationMessage', 'Validation Failed'); - } else { - $addFields[] = $name; - $fields_added = true; - } - $node->setValue($value); + $node->setValue((string)$value); $container->addChildNode($name, $node); - $this->internalNodes[] = $node; } - - if ($fields_added) { - $constraint->setOption('addFields', $addFields); - } - - $this->uniqueConstraints[] = $constraint; + $this->internalNodes[] = $nodes; } public function setRequired($required) { $this->valuesRequired = $required; - foreach ($this->internalNodes as $node) { - /* cover earlier set nodes */ - $node->setRequired($this->valuesRequired ? "Y" : "N"); + foreach ($this->iterateItems() as $records) { + foreach ($records->iterateItems() as $node) { + /* cover earlier set nodes */ + $node->setRequired($this->valuesRequired ? "Y" : "N"); + } } } public function validate() { + $uniqueConstraints = []; $validator = new \OPNsense\Base\Validation(); - foreach ($this->uniqueConstraints as $idx => $constraint) { + foreach ($this->internalNodes as $idx => $nodes) { + $addFields = []; + $constraint = new UniqueConstraint(); + foreach ($nodes as $name => $value) { + if ($name === array_key_first($nodes)) { + $constraint->setOption('node', $this->{'item'. $idx}->$name); + $constraint->setOption('name', $name); + $constraint->setOption('ValidationMessage', 'Validation Failed'); + } else { + $addFields[] = $name; + } + } + if ($addFields) { + $constraint->setOption('addFields', $addFields); + } $validator->add($idx, $constraint); } $msgs = $validator->validate([]); @@ -111,7 +111,8 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $container->addNode(['unique_test' => 'value1']); $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + + $this->assertEquals(2, count($msgs)); } public function testMultipleNonEqualAndEqualValues() @@ -126,7 +127,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $container->addNode(['unique_test' => 'value1', 'unique_test2' => 'value2']); $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + $this->assertEquals(2, count($msgs)); } public function testEmptyValuesNotRequiredAndRequired() @@ -143,7 +144,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + $this->assertEquals(2, count($msgs)); } public function testMultipleEmptyValuesNotRequiredAndRequired() @@ -151,7 +152,6 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $container = new UniqueTestContainer(); $container->addNode(['unique_test' => '', 'unique_test2' => '']); $container->addNode(['unique_test' => '', 'unique_test2' => '']); - $msgs = $container->validate(); $this->assertEquals(0, count($msgs)); @@ -160,7 +160,7 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + $this->assertEquals(2, count($msgs)); } public function testFirstValueEmptyPassAll() @@ -177,6 +177,6 @@ class UniqueConstraintTest extends \PHPUnit\Framework\Testcase $msgs = $container->validate(); - $this->assertEquals(1, count($msgs)); + $this->assertEquals(2, count($msgs)); } }