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 5c98c8fb82e0ff64fa0923f87371e956e6b7d575..ccdeafe3f7d92dc7e0d5c0893e64adb23f6326a3 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 f315068da873f0d949961e177b9e5166bdee145b..533a590e69015b45a6218d7e70770b382450ffaa 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 0761dbd99284067769617302191ccea8caf932dc..a6894375a41aa0e7842633308550a26526529cf6 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 efd991d3446a63057f62c5c572b7820c9684a3de..717989f936424de16a86c0167b88bddb827d56a1 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 233cda28f0725092a8780580479e02b57b79495c..54fad0ed83153af48062466b10f99f6b7f49990e 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 367a1d8d89f3abd326ec9d64e4dddd9b2befb067..8a917f340e7132460fca69ae96bd0edf8d286e4d 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) {