From 95bf0043efb5d91af82e1ce5f730a9eb74b8f702 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Fri, 1 Sep 2017 11:53:18 +1000 Subject: [PATCH] EtcdConfig: allow slashes in config key names Allowing slashes in config key names allows us to trivially support the proposed hierarchical structure with a single EtcdConfig object, by fetching values with the relevant prefixes on startup. Bug: T156924 Change-Id: Ica0914e61baba9c0462481925be15d79b66dc342 --- autoload.php | 1 + includes/config/EtcdConfig.php | 67 +++++++++++++---- includes/config/EtcdConfigParseError.php | 4 ++ .../includes/config/EtcdConfigTest.php | 72 ++++++++++++++++++- 4 files changed, 126 insertions(+), 18 deletions(-) create mode 100644 includes/config/EtcdConfigParseError.php diff --git a/autoload.php b/autoload.php index 3a2ae10007..9d205cb576 100644 --- a/autoload.php +++ b/autoload.php @@ -437,6 +437,7 @@ $wgAutoloadLocalClasses = [ 'EraseArchivedFile' => __DIR__ . '/maintenance/eraseArchivedFile.php', 'ErrorPageError' => __DIR__ . '/includes/exception/ErrorPageError.php', 'EtcdConfig' => __DIR__ . '/includes/config/EtcdConfig.php', + 'EtcdConfigParseError' => __DIR__ . '/includes/config/EtcdConfigParseError.php', 'EventRelayer' => __DIR__ . '/includes/libs/eventrelayer/EventRelayer.php', 'EventRelayerGroup' => __DIR__ . '/includes/EventRelayerGroup.php', 'EventRelayerKafka' => __DIR__ . '/includes/libs/eventrelayer/EventRelayerKafka.php', diff --git a/includes/config/EtcdConfig.php b/includes/config/EtcdConfig.php index d7dc45a537..0ec21cb9b7 100644 --- a/includes/config/EtcdConfig.php +++ b/includes/config/EtcdConfig.php @@ -228,7 +228,7 @@ class EtcdConfig implements Config, LoggerAwareInterface { // Retrieve all the values under the MediaWiki config directory list( $rcode, $rdesc, /* $rhdrs */, $rbody, $rerr ) = $this->http->run( [ 'method' => 'GET', - 'url' => "{$this->protocol}://{$address}/v2/keys/{$this->directory}/", + 'url' => "{$this->protocol}://{$address}/v2/keys/{$this->directory}/?recursive=true", 'headers' => [ 'content-type' => 'application/json' ] ] ); @@ -240,28 +240,65 @@ class EtcdConfig implements Config, LoggerAwareInterface { empty( $terminalCodes[$rcode] ) ]; } + try { + return [ $this->parseResponse( $rbody ), null, false ]; + } catch ( EtcdConfigParseError $e ) { + return [ null, $e->getMessage(), false ]; + } + } + /** + * Parse a response body, throwing EtcdConfigParseError if there is a validation error + * + * @param string $rbody + * @return array + */ + protected function parseResponse( $rbody ) { $info = json_decode( $rbody, true ); - if ( $info === null || !isset( $info['node']['nodes'] ) ) { - return [ null, "Unexpected JSON response; missing 'nodes' list.", false ]; + if ( $info === null ) { + throw new EtcdConfigParseError( "Error unserializing JSON response." ); + } + if ( !isset( $info['node'] ) || !is_array( $info['node'] ) ) { + throw new EtcdConfigParseError( + "Unexpected JSON response: Missing or invalid node at top level." ); } - $config = []; - foreach ( $info['node']['nodes'] as $node ) { + $this->parseDirectory( '', $info['node'], $config ); + return $config; + } + + /** + * Recursively parse a directory node and populate the array passed by + * reference, throwing EtcdConfigParseError if there is a validation error + * + * @param string $dirName The relative directory name + * @param array $dirNode The decoded directory node + * @param array &$config The output array + */ + protected function parseDirectory( $dirName, $dirNode, &$config ) { + if ( !isset( $dirNode['nodes'] ) ) { + throw new EtcdConfigParseError( + "Unexpected JSON response in dir '$dirName'; missing 'nodes' list." ); + } + if ( !is_array( $dirNode['nodes'] ) ) { + throw new EtcdConfigParseError( + "Unexpected JSON response in dir '$dirName'; 'nodes' is not an array." ); + } + + foreach ( $dirNode['nodes'] as $node ) { + $baseName = basename( $node['key'] ); + $fullName = $dirName === '' ? $baseName : "$dirName/$baseName"; if ( !empty( $node['dir'] ) ) { - continue; // skip directories - } + $this->parseDirectory( $fullName, $node, $config ); + } else { + $value = $this->unserialize( $node['value'] ); + if ( !is_array( $value ) || !array_key_exists( 'val', $value ) ) { + throw new EtcdConfigParseError( "Failed to parse value for '$fullName'." ); + } - $name = basename( $node['key'] ); - $value = $this->unserialize( $node['value'] ); - if ( !is_array( $value ) || !array_key_exists( 'val', $value ) ) { - return [ null, "Failed to parse value for '$name'.", false ]; + $config[$fullName] = $value['val']; } - - $config[$name] = $value['val']; } - - return [ $config, null, false ]; } /** diff --git a/includes/config/EtcdConfigParseError.php b/includes/config/EtcdConfigParseError.php new file mode 100644 index 0000000000..cab90a8ef8 --- /dev/null +++ b/includes/config/EtcdConfigParseError.php @@ -0,0 +1,4 @@ + [ + '200 OK - Empty dir' => [ 'http' => [ 'code' => 200, 'reason' => 'OK', @@ -395,7 +395,8 @@ class EtcConfigTest extends PHPUnit_Framework_TestCase { ], [ 'key' => '/example/sub', - 'dir' => true + 'dir' => true, + 'nodes' => [], ], [ 'key' => '/example/bar', @@ -410,6 +411,68 @@ class EtcConfigTest extends PHPUnit_Framework_TestCase { false // retry ], ], + '200 OK - Recursive' => [ + 'http' => [ + 'code' => 200, + 'reason' => 'OK', + 'headers' => [], + 'body' => json_encode( [ 'node' => [ 'nodes' => [ + [ + 'key' => '/example/a', + 'dir' => true, + 'nodes' => [ + [ + 'key' => 'b', + 'value' => json_encode( [ 'val' => true ] ), + ], + [ + 'key' => 'c', + 'value' => json_encode( [ 'val' => false ] ), + ], + ], + ], + ] ] ] ), + 'error' => '', + ], + 'expect' => [ + [ 'a/b' => true, 'a/c' => false ], // data + null, + false // retry + ], + ], + '200 OK - Missing nodes at second level' => [ + 'http' => [ + 'code' => 200, + 'reason' => 'OK', + 'headers' => [], + 'body' => json_encode( [ 'node' => [ 'nodes' => [ + [ + 'key' => '/example/a', + 'dir' => true, + ], + ] ] ] ), + 'error' => '', + ], + 'expect' => [ + null, + "Unexpected JSON response in dir 'a'; missing 'nodes' list.", + false // retry + ], + ], + '200 OK - Correctly encoded garbage response' => [ + 'http' => [ + 'code' => 200, + 'reason' => 'OK', + 'headers' => [], + 'body' => json_encode( [ 'foo' => 'bar' ] ), + 'error' => '', + ], + 'expect' => [ + null, + "Unexpected JSON response: Missing or invalid node at top level.", + false // retry + ], + ], '200 OK - Bad value' => [ 'http' => [ 'code' => 200, @@ -453,7 +516,7 @@ class EtcConfigTest extends PHPUnit_Framework_TestCase { ], 'expect' => [ null, // data - "Unexpected JSON response; missing 'nodes' list.", + "Error unserializing JSON response.", false // retry ], ], @@ -491,6 +554,9 @@ class EtcConfigTest extends PHPUnit_Framework_TestCase { /** * @covers EtcdConfig::fetchAllFromEtcdServer * @covers EtcdConfig::unserialize + * @covers EtcdConfig::parseResponse + * @covers EtcdConfig::parseDirectory + * @covers EtcdConfigParseError * @dataProvider provideFetchFromServer */ public function testFetchFromServer( array $httpResponse, array $expected ) { -- 2.20.1