From abb862db8817c0fde495479436d7ce5d722acb95 Mon Sep 17 00:00:00 2001 From: Karen Etheridge Date: Thu, 26 Sep 2019 14:02:07 -0700 Subject: [PATCH 1/2] properly handle a report using a different device's system_uuid If we do not query for the device properly, the wrong device will get updated; this might also cause a database explosion if the system_uuid and/or serial_number is already in use by another device. (cherry picked from commit ed07cf58ae196fd55cefd171a9a097d705f07507) ..with conflict resolution, which originally comes from v3.0.0-a6. --- lib/Conch/Controller/DeviceReport.pm | 42 ++++++++++++++++++---------- t/integration/device-reports.t | 14 ++++++++++ 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/lib/Conch/Controller/DeviceReport.pm b/lib/Conch/Controller/DeviceReport.pm index 4c0e59f06..ca620a908 100644 --- a/lib/Conch/Controller/DeviceReport.pm +++ b/lib/Conch/Controller/DeviceReport.pm @@ -83,19 +83,28 @@ sub process ($c) { : $existing_device ? $existing_device->uptime_since : undef; - my $device = $c->db_devices->update_or_create({ - id => $c->stash('device_id'), - system_uuid => $unserialized_report->{system_uuid}, - hardware_product_id => $hw->id, - state => $unserialized_report->{state}, - health => 'unknown', - last_seen => \'now()', - uptime_since => $uptime, - hostname => $unserialized_report->{os}{hostname}, - updated => \'now()', - deactivated => undef, + my $device = $c->txn_wrapper(sub ($c) { + $c->db_devices->update_or_create({ + id => $c->stash('device_id'), + system_uuid => $unserialized_report->{system_uuid}, + hardware_product_id => $hw->id, + state => $unserialized_report->{state}, + health => 'unknown', + last_seen => \'now()', + uptime_since => $uptime, + hostname => $unserialized_report->{os}{hostname}, + updated => \'now()', + deactivated => undef, + }, + { key => 'primary' }); }); + if (not $device) { + return $c->status(400, { error => 'could not process report for device ' + .$c->stash('device_id') + .($c->stash('exception') ? ': '.(split(/\n/, $c->stash('exception'), 2))[0] : '') }); + } + $c->log->debug('Creating device report'); my $device_report = $device->create_related('device_reports', { report => $c->req->text, # this is the raw json string @@ -130,7 +139,9 @@ sub process ($c) { device => $c->db_ro_devices->find($device->id), device_report => $device_report, ); - return $c->status(400, { error => 'no validations ran' }) if not $validation_state; + return $c->status(400, { error => 'no validations ran' + .($c->stash('exception') ? ': '.(split(/\n/, $c->stash('exception'), 2))[0] : '') }) + if not $validation_state; $c->log->debug('Validations ran with result: '.$validation_state->status); # calculate the device health based on the validation results. @@ -430,7 +441,8 @@ sub validate_report ($c) { hostname => $unserialized_report->{os}{hostname}, updated => \'now()', deactivated => undef, - }); + }, + { key => 'primary' }); # we do not call _record_device_configuration, because no validations # should be using that information, instead choosing to respect the report data. @@ -448,7 +460,9 @@ sub validate_report ($c) { die 'rollback: device used for report validation should not be persisted'; }); - return $c->status(400, { error => 'no validations ran' }) if not @validation_results; + return $c->status(400, { error => 'no validations ran' + .($c->stash('exception') ? ': '.(split(/\n/, $c->stash('exception'), 2))[0] : '') }) + if not @validation_results; $c->status(200, { device_id => $unserialized_report->{serial_number}, diff --git a/t/integration/device-reports.t b/t/integration/device-reports.t index ee566258e..07104218d 100644 --- a/t/integration/device-reports.t +++ b/t/integration/device-reports.t @@ -79,4 +79,18 @@ subtest 'run report without an existing device' => sub { }); }; +subtest 'system_uuid collisions' => sub { + my $report_data = Mojo::JSON::from_json($report); + $report_data->{serial_number} = 'i_was_here_first'; + + my $existing_device = $t->generate_fixtures('device', { id => 'i_was_here_first' }); + + $t->post_ok('/device_report', json => $report_data) + ->status_is(400) + ->json_cmp_deeply({ error => re(qr/no validations ran: .*duplicate key value violates unique constraint "device_system_uuid_key"/) }); + + $t->post_ok('/device/i_was_here_first', json => $report_data) + ->json_cmp_deeply({ error => re(qr/could not process report for device i_was_here_first.*duplicate key value violates unique constraint "device_system_uuid_key"/) }); +}; + done_testing; From 2d1eb9ec03b53aa2ee159dedf788461fe6a305ab Mon Sep 17 00:00:00 2001 From: Karen Etheridge Date: Wed, 15 Apr 2020 10:10:16 -0700 Subject: [PATCH 2/2] also set both conflicting devices to the error state for better detection This resolves Rollbar items 3490, 3491 occurring 2020-04-15. --- lib/Conch/Controller/DeviceReport.pm | 8 ++++++++ t/integration/device-reports.t | 6 ++++++ 2 files changed, 14 insertions(+) diff --git a/lib/Conch/Controller/DeviceReport.pm b/lib/Conch/Controller/DeviceReport.pm index ca620a908..a298d8bfe 100644 --- a/lib/Conch/Controller/DeviceReport.pm +++ b/lib/Conch/Controller/DeviceReport.pm @@ -100,6 +100,14 @@ sub process ($c) { }); if (not $device) { + if (my $serial_device = $c->db_devices->find({ id => $c->stash('device_id') })) { + $serial_device->health('error'); + $serial_device->update({ updated => \'now()' }) if $serial_device->is_changed; + } + if (my $system_uuid_device = $c->db_devices->find({ system_uuid => $unserialized_report->{system_uuid} })) { + $system_uuid_device->health('error'); + $system_uuid_device->update({ updated => \'now()' }) if $system_uuid_device->is_changed; + } return $c->status(400, { error => 'could not process report for device ' .$c->stash('device_id') .($c->stash('exception') ? ': '.(split(/\n/, $c->stash('exception'), 2))[0] : '') }); diff --git a/t/integration/device-reports.t b/t/integration/device-reports.t index 07104218d..d86c991c9 100644 --- a/t/integration/device-reports.t +++ b/t/integration/device-reports.t @@ -91,6 +91,12 @@ subtest 'system_uuid collisions' => sub { $t->post_ok('/device/i_was_here_first', json => $report_data) ->json_cmp_deeply({ error => re(qr/could not process report for device i_was_here_first.*duplicate key value violates unique constraint "device_system_uuid_key"/) }); + + $existing_device->discard_changes; + is($existing_device->health, 'error', 'existing device had health set to error'); + + my $test_device = $t->app->db_devices->find('TEST'); + is($test_device->health, 'error', 'TEST device had health set to error as well'); }; done_testing;