Perf dashboard should support authentication via a slave password
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 20 Dec 2014 02:38:04 +0000 (02:38 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 20 Dec 2014 02:38:04 +0000 (02:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=139837

Reviewed by Andreas Kling.

For historical reasons, perf dashboard conflated builders and build slaves. As a result we ended up
having to add multiple builders with the same password when a single build slave is shared among them.

This patch introduces the concept of build_slave into the perf dashboard to end this madness.

* init-database.sql: Added build_slave table as well as references to it in builds and reports.

* public/admin/build-slaves.php: Added.

* public/admin/builders.php: Added the support for updating passwords.

* public/include/admin-header.php:
(update_field): Takes an extra argument when a new value needs to be supplied by the caller instead of
being retrieved from $_POST.
(AdministrativePage::render_table): Use array_get to retrieve a value out of the database row since
the raw may not exist (e.g. new_password).
(AdministrativePage::render_form_to_add): Added the support for post_insertion. Don't render the form
control here when this flag evaluates to TRUE.

* public/include/report-processor.php:
(ReportProcessor::process): Added the logic to authenticate with slaveName and slavePassword if those
values are present in the report. In addition, try authenticating builderName with slavePassword if
builderPassword is not specified. When neither password is specified, exit with BuilderNotFound.
Also insert the slave or the builder whichever is missing after we've successfully authenticated.
(ReportProcessor::construct_build_data): Takes a builder ID and an optional slave ID instead of
a builder row.
(ReportProcessor::store_report): Store the slave ID with the report.
(ReportProcessor::resolve_build_id): Exit with MismatchingBuildSlave when the slave associated with
the matching build is different from what's being reported.

* tests/api-report.js: Added a bunch of tests to test the new features of /api/report.
(.addSlave): Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@177614 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/init-database.sql
Websites/perf.webkit.org/public/admin/build-slaves.php [new file with mode: 0644]
Websites/perf.webkit.org/public/admin/builders.php
Websites/perf.webkit.org/public/include/admin-header.php
Websites/perf.webkit.org/public/include/report-processor.php
Websites/perf.webkit.org/tests/api-report.js

index 5c98c8f..ccdeafe 100644 (file)
@@ -1,3 +1,43 @@
+2014-12-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Perf dashboard should support authentication via a slave password
+        https://bugs.webkit.org/show_bug.cgi?id=139837
+
+        Reviewed by Andreas Kling.
+
+        For historical reasons, perf dashboard conflated builders and build slaves. As a result we ended up
+        having to add multiple builders with the same password when a single build slave is shared among them.
+
+        This patch introduces the concept of build_slave into the perf dashboard to end this madness.
+
+        * init-database.sql: Added build_slave table as well as references to it in builds and reports.
+
+        * public/admin/build-slaves.php: Added.
+
+        * public/admin/builders.php: Added the support for updating passwords.
+
+        * public/include/admin-header.php:
+        (update_field): Takes an extra argument when a new value needs to be supplied by the caller instead of
+        being retrieved from $_POST.
+        (AdministrativePage::render_table): Use array_get to retrieve a value out of the database row since
+        the raw may not exist (e.g. new_password).
+        (AdministrativePage::render_form_to_add): Added the support for post_insertion. Don't render the form
+        control here when this flag evaluates to TRUE.
+
+        * public/include/report-processor.php:
+        (ReportProcessor::process): Added the logic to authenticate with slaveName and slavePassword if those
+        values are present in the report. In addition, try authenticating builderName with slavePassword if
+        builderPassword is not specified. When neither password is specified, exit with BuilderNotFound.
+        Also insert the slave or the builder whichever is missing after we've successfully authenticated.
+        (ReportProcessor::construct_build_data): Takes a builder ID and an optional slave ID instead of
+        a builder row.
+        (ReportProcessor::store_report): Store the slave ID with the report.
+        (ReportProcessor::resolve_build_id): Exit with MismatchingBuildSlave when the slave associated with
+        the matching build is different from what's being reported.
+
+        * tests/api-report.js: Added a bunch of tests to test the new features of /api/report.
+        (.addSlave): Added.
+
 2014-12-18  Ryosuke Niwa  <rniwa@webkit.org>
 
         New perf dashboard should not duplicate author information in each commit
index f315068..533a590 100644 (file)
@@ -7,6 +7,7 @@ DROP TABLE builds CASCADE;
 DROP TABLE committers CASCADE;
 DROP TABLE commits CASCADE;
 DROP TABLE build_commits CASCADE;
+DROP TABLE build_slaves CASCADE;
 DROP TABLE builders CASCADE;
 DROP TABLE repositories CASCADE;
 DROP TABLE platforms CASCADE;
@@ -46,12 +47,18 @@ CREATE TABLE tracker_repositories (
 CREATE TABLE builders (
     builder_id serial PRIMARY KEY,
     builder_name varchar(64) NOT NULL UNIQUE,
-    builder_password_hash character(64) NOT NULL,
+    builder_password_hash character(64),
     builder_build_url varchar(1024));
 
+CREATE TABLE build_slaves (
+    slave_id serial PRIMARY KEY,
+    slave_name varchar(64) NOT NULL UNIQUE,
+    slave_password_hash character(64));
+
 CREATE TABLE builds (
     build_id serial PRIMARY KEY,
     build_builder integer REFERENCES builders ON DELETE CASCADE,
+    build_slave integer REFERENCES build_slaves ON DELETE CASCADE,
     build_number integer NOT NULL,
     build_time timestamp NOT NULL,
     build_latest_revision timestamp,
@@ -135,6 +142,7 @@ CREATE TABLE run_iterations (
 CREATE TABLE reports (
     report_id serial PRIMARY KEY,
     report_builder integer NOT NULL REFERENCES builders ON DELETE RESTRICT,
+    report_slave integer REFERENCES build_slaves ON DELETE RESTRICT,
     report_build_number integer,
     report_build integer REFERENCES builds,
     report_created_at timestamp NOT NULL DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC'),
diff --git a/Websites/perf.webkit.org/public/admin/build-slaves.php b/Websites/perf.webkit.org/public/admin/build-slaves.php
new file mode 100644 (file)
index 0000000..460b492
--- /dev/null
@@ -0,0 +1,35 @@
+<?php
+
+require('../include/admin-header.php');
+
+if ($db) {
+
+    if ($action == 'add') {
+        if ($db->insert_row('build_slaves', 'slave', array('name' => $_POST['name'], 'password_hash' => hash('sha256', $_POST['password'])))) {
+            notice('Inserted the new slave.');
+            regenerate_manifest();
+        } else
+            notice('Could not add the slave.');
+    } else if ($action == 'update') {
+        if (update_field('build_slaves', 'slave', 'name'))
+            regenerate_manifest();
+        else if (update_field('build_slaves', 'slave', 'password_hash', hash('sha256', $_POST['new_password'])))
+            regenerate_manifest();
+        else
+            notice('Invalid parameters.');
+    }
+
+    $page = new AdministrativePage($db, 'build_slaves', 'slave', array(
+        'name' => array('size' => 50, 'editing_mode' => 'string'),
+        'password_hash' => array(),
+        'password' => array('pre_insertion' => TRUE, 'editing_mode' => 'string'),
+        'new_password' => array('post_insertion' => TRUE, 'editing_mode' => 'string'),
+    ));
+
+    $page->render_table('name');
+    $page->render_form_to_add();
+}
+
+require('../include/admin-footer.php');
+
+?>
index 0761dbd..a689437 100644 (file)
@@ -14,6 +14,8 @@ if ($db) {
     } else if ($action == 'update') {
         if (update_field('builders', 'builder', 'name') || update_field('builders', 'builder', 'build_url'))
             regenerate_manifest();
+        else if (update_field('build_slaves', 'slave', 'password_hash', hash('sha256', $_POST['new_password'])))
+            regenerate_manifest();
         else
             notice('Invalid parameters.');
     }
@@ -22,6 +24,7 @@ if ($db) {
         'name' => array('size' => 50, 'editing_mode' => 'string'),
         'password_hash' => array(),
         'password' => array('pre_insertion' => TRUE, 'editing_mode' => 'string'),
+        'new_password' => array('post_insertion' => TRUE, 'editing_mode' => 'string'),
         'build_url' => array('label' => 'Build URL', 'size' => 100, 'editing_mode' => 'url'),
     ));
 
index efd991d..717989f 100644 (file)
@@ -18,6 +18,7 @@ require_once('manifest.php');
     <li><a href="/admin/tests">Tests</a></li>
     <li><a href="/admin/aggregators">Aggregators</a></li>
     <li><a href="/admin/builders">Builders</a></li>
+    <li><a href="/admin/build-slaves">Slaves</a></li>
     <li><a href="/admin/repositories">Repositories</a></li>
     <li><a href="/admin/bug-trackers">Bug Trackers</a></li>
 </ul>
@@ -56,18 +57,24 @@ function execute_query_and_expect_one_row_to_be_affected($query, $params, $succe
     return false;
 }
 
-function update_field($table, $prefix, $field_name) {
+function update_field($table, $prefix, $field_name, $new_value = NULL) {
     global $db;
 
-    if (!array_key_exists('id', $_POST) || (array_get($_POST, 'updated-column') != $field_name && !array_key_exists($field_name, $_POST)))
+    if (!array_key_exists('id', $_POST))
         return FALSE;
 
     $id = intval($_POST['id']);
     $prefixed_field_name = $prefix . '_' . $field_name;
     $id_field_name = $prefix . '_id';
 
+    if ($new_value == NULL) {
+        if (array_get($_POST, 'updated-column') != $field_name && !array_key_exists($field_name, $_POST))
+            return FALSE;
+        $new_value = array_get($_POST, $field_name);
+    }
+
     execute_query_and_expect_one_row_to_be_affected("UPDATE $table SET $prefixed_field_name = \$2 WHERE $id_field_name = \$1",
-        array($id, array_get($_POST, $field_name)),
+        array($id, $new_value),
         "Updated the $prefix $id",
         "Could not update $prefix $id");
 
@@ -226,7 +233,7 @@ END;
                         continue;
                     }
 
-                    $value = htmlspecialchars($row[$this->prefix . '_' . $name], ENT_QUOTES);
+                    $value = htmlspecialchars(array_get($row, $this->prefix . '_' . $name), ENT_QUOTES);
                     $editing_mode = array_get($this->column_info[$name], 'editing_mode');
                     if (!$editing_mode) {
                         echo "<td$rowspan_if_needed>$value</td>\n";
@@ -295,6 +302,8 @@ END;
             $editing_mode = array_get($this->column_info[$name], 'editing_mode');
             if (array_get($this->column_info[$name], 'custom') || !$editing_mode)
                 continue;
+            if (array_get($this->column_info[$name], 'post_insertion'))
+                continue;
 
             $label = htmlspecialchars($this->column_label($name));
             echo "<label>$label<br>\n";
index 233cda2..54fad0e 100644 (file)
@@ -39,16 +39,42 @@ class ReportProcessor {
         array_key_exists('buildTime', $report) or $this->exit_with_error('MissingBuildTime');
 
         $builder_info = array('name' => $report['builderName']);
-        if (!$existing_report_id)
-            $builder_info['password_hash'] = hash('sha256', $report['builderPassword']);
+        $slave_name = array_get($report, 'slaveName', NULL);
+        $slave_id = NULL;
+        if (!$existing_report_id) {
+            $hash = NULL;
+            if ($slave_name && array_key_exists('slavePassword', $report)) {
+                $hash = hash('sha256', $report['slavePassword']);
+                $slave = $this->db->select_first_row('build_slaves', 'slave', array('name' => $slave_name, 'password_hash' => $hash));
+                if ($slave)
+                    $slave_id = $slave['slave_id'];
+            } else if (array_key_exists('builderPassword', $report))
+                $hash = hash('sha256', $report['builderPassword']);
+
+            if (!$hash)
+                $this->exit_with_error('BuilderNotFound');
+            if (!$slave_id)
+                $builder_info['password_hash'] = $hash;
+        }
+
         if (array_key_exists('builderPassword', $report))
             unset($report['builderPassword']);
+        if (array_key_exists('slavePassword', $report))
+            unset($report['slavePassword']);
+
+        $builder_id = NULL;
+        if ($slave_id)
+            $builder_id = $this->db->select_or_insert_row('builders', 'builder', $builder_info);
+        else {
+            $builder = $this->db->select_first_row('builders', 'builder', $builder_info);
+            if (!$builder)
+                $this->exit_with_error('BuilderNotFound', array('name' => $builder_info['name']));
+            $builder_id = $builder['builder_id'];
+            if ($slave_name)
+                $slave_id = $this->db->select_or_insert_row('build_slaves', 'slave', array('name' => $slave_name));
+        }
 
-        $matched_builder = $this->db->select_first_row('builders', 'builder', $builder_info);
-        if (!$matched_builder)
-            $this->exit_with_error('BuilderNotFound', array('name' => $builder_info['name']));
-
-        $build_data = $this->construct_build_data($report, $matched_builder);
+        $build_data = $this->construct_build_data($report, $builder_id, $slave_id);
         if (!$existing_report_id)
             $this->store_report($report, $build_data);
 
@@ -67,16 +93,19 @@ class ReportProcessor {
         $this->runs->commit($platform_id, $build_id);
     }
 
-    private function construct_build_data($report, $builder) {
+    private function construct_build_data($report, $builder_id, $slave_id) {
         array_key_exists('buildNumber', $report) or $this->exit_with_error('MissingBuildNumber');
         array_key_exists('buildTime', $report) or $this->exit_with_error('MissingBuildTime');
 
-        return array('builder' => $builder['builder_id'], 'number' => $report['buildNumber'], 'time' => $report['buildTime']);
+        return array('builder' => $builder_id, 'slave' => $slave_id, 'number' => $report['buildNumber'], 'time' => $report['buildTime']);
     }
 
     private function store_report($report, $build_data) {
         assert(!$this->report_id);
-        $this->report_id = $this->db->insert_row('reports', 'report', array('builder' => $build_data['builder'], 'build_number' => $build_data['number'],
+        $this->report_id = $this->db->insert_row('reports', 'report', array(
+            'builder' => $build_data['builder'],
+            'slave' => $build_data['slave'],
+            'build_number' => $build_data['number'],
             'content' => json_encode($report)));
         if (!$this->report_id)
             $this->exit_with_error('FailedToStoreRunReport');
@@ -84,11 +113,15 @@ class ReportProcessor {
 
     private function resolve_build_id($build_data, $revisions) {
         // FIXME: This code has a race condition. See <rdar://problem/15876303>.
-        $results = $this->db->query_and_fetch_all("SELECT build_id FROM builds WHERE build_builder = $1 AND build_number = $2 AND build_time <= $3 AND build_time + interval '1 day' > $3",
+        $results = $this->db->query_and_fetch_all("SELECT build_id, build_slave FROM builds
+            WHERE build_builder = $1 AND build_number = $2 AND build_time <= $3 AND build_time + interval '1 day' > $3",
             array($build_data['builder'], $build_data['number'], $build_data['time']));
-        if ($results)
-            $build_id = $results[0]['build_id'];
-        else
+        if ($results) {
+            $first_result = $results[0];
+            if ($first_result['build_slave'] != $build_data['slave'])
+                $this->exit_with_error('MismatchingBuildSlave', array('storedBuild' => $results, 'reportedBuildData' => $build_data));
+            $build_id = $first_result['build_id'];
+        } else
             $build_id = $this->db->insert_row('builds', 'build', $build_data);
         if (!$build_id)
             $this->exit_with_error('FailedToInsertBuild', $build_data);
index 367a1d8..8a917f3 100644 (file)
@@ -3,6 +3,7 @@ describe("/api/report", function () {
         "buildNumber": "123",
         "buildTime": "2013-02-28T10:12:03.388304",
         "builderName": "someBuilder",
+        "slaveName": "someSlave",
         "builderPassword": "somePassword",
         "platform": "Mountain Lion",
         "tests": {},
@@ -16,11 +17,35 @@ describe("/api/report", function () {
             }
         }}];
 
+    var emptySlaveReport = [{
+        "buildNumber": "123",
+        "buildTime": "2013-02-28T10:12:03.388304",
+        "builderName": "someBuilder",
+        "builderPassword": "somePassword",
+        "slaveName": "someSlave",
+        "slavePassword": "otherPassword",
+        "platform": "Mountain Lion",
+        "tests": {},
+        "revisions": {
+            "OS X": {
+                "revision": "10.8.2 12C60"
+            },
+            "WebKit": {
+                "revision": "141977",
+                "timestamp": "2013-02-06T08:55:20.9Z"
+            }
+        }}];
+
     function addBuilder(report, callback) {
         queryAndFetchAll('INSERT INTO builders (builder_name, builder_password_hash) values ($1, $2)',
             [report[0].builderName, sha256(report[0].builderPassword)], callback);
     }
 
+    function addSlave(report, callback) {
+        queryAndFetchAll('INSERT INTO build_slaves (slave_name, slave_password_hash) values ($1, $2)',
+            [report[0].slaveName, sha256(report[0].slavePassword)], callback);
+    }
+
     it("should reject error when builder name is missing", function () {
         postJSON('/api/report/', [{"buildTime": "2013-02-28T10:12:03.388304"}], function (response) {
             assert.equal(response.statusCode, 200);
@@ -53,6 +78,28 @@ describe("/api/report", function () {
         });
     });
 
+    it("should reject a report without a builder password", function () {
+        addBuilder(emptyReport, function () {
+            var report = [{
+                "buildNumber": "123",
+                "buildTime": "2013-02-28T10:12:03.388304",
+                "builderName": "someBuilder",
+                "tests": {},
+                "revisions": {}}];
+            postJSON('/api/report/', report, function (response) {
+                assert.equal(response.statusCode, 200);
+                assert.notEqual(JSON.parse(response.responseText)['status'], 'OK');
+                assert.equal(JSON.parse(response.responseText)['failureStored'], false);
+                assert.equal(JSON.parse(response.responseText)['processedRuns'], 0);
+
+                queryAndFetchAll('SELECT COUNT(*) from reports', [], function (rows) {
+                    assert.equal(rows[0].count, 0);
+                    notifyDone();
+                });
+            });
+        });
+    });
+
     it("should store a report from a valid builder", function () {
         addBuilder(emptyReport, function () {
             postJSON('/api/report/', emptyReport, function (response) {
@@ -68,6 +115,41 @@ describe("/api/report", function () {
         });
     });
 
+    it("should treat the slave password as the builder password if there is no matching slave", function () {
+        addBuilder(emptyReport, function () {
+            emptyReport[0]['slavePassword'] = emptyReport[0]['builderPassword'];
+            delete emptyReport[0]['builderPassword'];
+            postJSON('/api/report/', emptyReport, function (response) {
+                emptyReport[0]['builderPassword'] = emptyReport[0]['slavePassword'];
+                delete emptyReport[0]['slavePassword'];
+
+                assert.equal(response.statusCode, 200);
+                assert.equal(JSON.parse(response.responseText)['status'], 'OK');
+                assert.equal(JSON.parse(response.responseText)['failureStored'], false);
+                assert.equal(JSON.parse(response.responseText)['processedRuns'], 1);
+                queryAndFetchAll('SELECT COUNT(*) from reports', [], function (rows) {
+                    assert.equal(rows[0].count, 1);
+                    notifyDone();
+                });
+            });
+        });
+    });
+
+    it("should store a report from a valid slave", function () {
+        addSlave(emptySlaveReport, function () {
+            postJSON('/api/report/', emptySlaveReport, function (response) {
+                assert.equal(response.statusCode, 200);
+                assert.equal(JSON.parse(response.responseText)['status'], 'OK');
+                assert.equal(JSON.parse(response.responseText)['failureStored'], false);
+                assert.equal(JSON.parse(response.responseText)['processedRuns'], 1);
+                queryAndFetchAll('SELECT COUNT(*) from reports', [], function (rows) {
+                    assert.equal(rows[0].count, 1);
+                    notifyDone();
+                });
+            });
+        });
+    });
+
     it("should store the builder name but not the builder password", function () {
         addBuilder(emptyReport, function () {
             postJSON('/api/report/', emptyReport, function (response) {
@@ -81,6 +163,28 @@ describe("/api/report", function () {
         });
     });
 
+    it("should add a slave if there isn't one and the report was authenticated by a builder", function () {
+        addBuilder(emptyReport, function () {
+            postJSON('/api/report/', emptyReport, function (response) {
+                queryAndFetchAll('SELECT * from build_slaves', [], function (rows) {
+                    assert.strictEqual(rows[0].slave_name, emptyReport[0].slaveName);
+                    notifyDone();
+                });
+            });
+        });
+    });
+
+    it("should add a builder if there isn't one and the report was authenticated by a slave", function () {
+        addSlave(emptySlaveReport, function () {
+            postJSON('/api/report/', emptySlaveReport, function (response) {
+                queryAndFetchAll('SELECT * from builders', [], function (rows) {
+                    assert.strictEqual(rows[0].builder_name, emptyReport[0].builderName);
+                    notifyDone();
+                });
+            });
+        });
+    });
+
     it("should add a build", function () {
         addBuilder(emptyReport, function () {
             postJSON('/api/report/', emptyReport, function (response) {