From 4d7e2467ae61a4aba3cfaa689af31a047fa789d3 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 11:40:27 +0200 Subject: [PATCH 01/10] fix(report/handlers): accept more semver versions The current problem we have, specifically, is that since v1.2.0 the mobile apps emit full semver including also the build. This requires several parts of our infrastructure to improve the regexp being used so to validate also this kind of input. See also: measurement-kit/measurement-kit#1388 An earlier version of this diff was blessed by @hellais on Slack and since then I just changed comments. --- oonib/report/handlers.py | 13 ++++++++++++- oonib/tests/test_report.py | 23 ++++++++++++++++++----- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/oonib/report/handlers.py b/oonib/report/handlers.py index f8e626a9..49165755 100644 --- a/oonib/report/handlers.py +++ b/oonib/report/handlers.py @@ -81,7 +81,18 @@ def parseUpdateReportRequest(request, report_id=None): def validateHeader(header): - version_string = re.compile(r"^[0-9A-Za-z_\-\.]+$") + # Version-number-regex context + # ---------------------------- + # + # We are updating our software to always use semver, but for historical + # reasons there are also cases where we use '0.1', so better to be + # very liberal an accept anything version-ish in here. + # + # Live regexp testing: https://regex101.com/r/zhnfFl/3 + # + # See also: github.com/measurement-kit/measurement-kit/pull/1388 + version_string = re.compile(r"^[0-9A-Za-z_.+-]+$") + name = re.compile(r"^[a-zA-Z0-9_\- ]+$") probe_asn = re.compile(r"^AS[0-9]+$") probe_cc = re.compile(r"^[A-Z]{2}$") diff --git a/oonib/tests/test_report.py b/oonib/tests/test_report.py index 2623e76c..7796f74d 100644 --- a/oonib/tests/test_report.py +++ b/oonib/tests/test_report.py @@ -14,6 +14,19 @@ from oonib.report.api import reportAPI from oonib.tests.handler_helpers import HandlerTestCase, mock_initialize, MockTime +# Version number testing +# ---------------------- +# +# We need to test an array of version numbers ranging from super compliant +# with semver (mobile apps since v1.2.0) to very basic (e.g. 0.1, as used +# in some cases by ooni-probe test implementations). +# +# To emphasize this, I have refactored this regress test to put the +# various version numbers we use here at the beginning of file. +FULL_SEMVER_VERSION = "1.7.0-beta.1+11.7" +BASIC_SEMVER_VERSION = "1.7.0" +SIMPLE_VERSION = "1.0" + sample_report_entry = { 'agent': 'agent', 'input': 'http://example.com', @@ -42,7 +55,7 @@ 'probe_city': None, 'probe_ip': '127.0.0.1', 'software_name': 'ooniprobe', - 'software_version': '1.1.0', + 'software_version': FULL_SEMVER_VERSION, 'test_start_time': '2016-01-01 12:34:56', 'test_name': 'fake_test', 'test_version': '0.1.0' @@ -60,18 +73,18 @@ probe_city: null probe_ip: 127.0.0.1 software_name: ooniprobe -software_version: 1.1.0 +software_version: %s test_start_time: '2016-01-01 12:34:56' test_name: fake_test test_version: 0.1.0 ... -""" +""" % BASIC_SEMVER_VERSION dummy_data = { 'software_name': 'ooni-test', - 'software_version': '0.1', + 'software_version': SIMPLE_VERSION, 'test_name': 'some-test', - 'test_version': '0.1', + 'test_version': SIMPLE_VERSION, 'probe_asn': 'AS0', 'probe_cc': 'ZZ', 'test_start_time': '2016-01-01 12:34:56' From 87c8b0c226e2754b35f5d4666fe37215fdb81f63 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 12:25:25 +0200 Subject: [PATCH 02/10] attempt to repair regress test --- .travis.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 8e196342..4386f282 100644 --- a/.travis.yml +++ b/.travis.yml @@ -37,7 +37,8 @@ script: - openssl x509 -req -days 365 -in server.csr -signkey private.key -out certificate.crt - test -f "private.key.org" && rm -f private.key.org - test -f "server.csr" && rm -f server.csr - - chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib + - ./bin/oonib && sleep 30 && kill -TERM `cat oonib.pid` + #- chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib - echo "Build successful." - $(which coverage) run $(which trial) oonib after_success: From 03f1028148579cc56b44ff6aac38696a5a551028 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 12:29:36 +0200 Subject: [PATCH 03/10] fix problem with keyboard and chair --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 4386f282..d5a11dbe 100644 --- a/.travis.yml +++ b/.travis.yml @@ -37,7 +37,7 @@ script: - openssl x509 -req -days 365 -in server.csr -signkey private.key -out certificate.crt - test -f "private.key.org" && rm -f private.key.org - test -f "server.csr" && rm -f server.csr - - ./bin/oonib && sleep 30 && kill -TERM `cat oonib.pid` + - ./bin/oonib& && sleep 30 && kill -TERM `cat oonib.pid` #- chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib - echo "Build successful." - $(which coverage) run $(which trial) oonib From 9e0eca426cacf7ec4e301fd612ed25d9c824e22a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 12:32:53 +0200 Subject: [PATCH 04/10] another approach --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index d5a11dbe..6be5cf82 100644 --- a/.travis.yml +++ b/.travis.yml @@ -37,7 +37,9 @@ script: - openssl x509 -req -days 365 -in server.csr -signkey private.key -out certificate.crt - test -f "private.key.org" && rm -f private.key.org - test -f "server.csr" && rm -f server.csr - - ./bin/oonib& && sleep 30 && kill -TERM `cat oonib.pid` + - ./bin/oonib & + - sleep 3 + - kill -TERM `cat oonib.pid` #- chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib - echo "Build successful." - $(which coverage) run $(which trial) oonib From ec91e00d53ea57413c7cb5301fc2d747433ee7d9 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 12:36:45 +0200 Subject: [PATCH 05/10] improve over previous --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 6be5cf82..c6001a6a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -38,7 +38,9 @@ script: - test -f "private.key.org" && rm -f private.key.org - test -f "server.csr" && rm -f server.csr - ./bin/oonib & - - sleep 3 + - sleep 30 + - ps auxww|grep oonib|grep -v grep + - cat oonib.pid - kill -TERM `cat oonib.pid` #- chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib - echo "Build successful." From 9bb6db5e2a453911873e6fa069f12a85466473fe Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 12:39:31 +0200 Subject: [PATCH 06/10] also print pid after the event --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index c6001a6a..81a557e9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -42,6 +42,7 @@ script: - ps auxww|grep oonib|grep -v grep - cat oonib.pid - kill -TERM `cat oonib.pid` + - ps auxww|grep oonib|grep -v grep #- chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib - echo "Build successful." - $(which coverage) run $(which trial) oonib From 7852de586fbf512aac2eb3b4432f9e6463e7df93 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 12:46:07 +0200 Subject: [PATCH 07/10] make sure it is killed --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 81a557e9..0424b205 100644 --- a/.travis.yml +++ b/.travis.yml @@ -42,6 +42,7 @@ script: - ps auxww|grep oonib|grep -v grep - cat oonib.pid - kill -TERM `cat oonib.pid` + - sleep 3 - ps auxww|grep oonib|grep -v grep #- chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib - echo "Build successful." From 0b08f53c4e718741bebbdace65edc783aed3df45 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 12:52:00 +0200 Subject: [PATCH 08/10] Finish off PR --- .travis.test.sh | 35 ----------------------------------- .travis.yml | 17 +++++++++++++---- 2 files changed, 13 insertions(+), 39 deletions(-) delete mode 100755 .travis.test.sh diff --git a/.travis.test.sh b/.travis.test.sh deleted file mode 100755 index d7f5092d..00000000 --- a/.travis.test.sh +++ /dev/null @@ -1,35 +0,0 @@ -#!/bin/bash -############################################################################## -# -# .travis.test.sh -# ------------------- -# Run $2 for $1 seconds and then kill the process. -# -# :authors: Isis Agora Lovecruft, 0x2cdb8b35 -# :date: 21 April 2013 -# :version: 0.0.1 -############################################################################## - -set -vx -- - -function killitwithfire () { - trap - ALRM - kill -ALRM $prog 2>/dev/null - kill -9 $! 2>/dev/null && exit 0 -} - -function waitforit () { - trap "killitwithfire" ALRM - sleep $1& wait - kill -ALRM $$ -} - -waitforit $1& prog=$! ; shift ; -trap "killitwithfire" ALRM INT -"$@"& wait $! -RET=$? -if [[ "$(ps -ef | awk -v pid=$prog '$2==pid{print}{}')" != "" ]]; then - kill -ALRM $prog - wait $prog -fi -exit $RET diff --git a/.travis.yml b/.travis.yml index 0424b205..94001348 100644 --- a/.travis.yml +++ b/.travis.yml @@ -27,8 +27,10 @@ install: - pip install -r requirements.txt - pip install coveralls - python setup.py install -# command to run tests, e.g. python setup.py test + script: + + # 1. prepare for running oonib - cp oonib.conf.example oonib.conf - openssl genrsa -des3 -passout pass:oonib -out private.key 4096 - openssl req -batch -passin pass:oonib -new -key private.key -out server.csr @@ -37,15 +39,22 @@ script: - openssl x509 -req -days 365 -in server.csr -signkey private.key -out certificate.crt - test -f "private.key.org" && rm -f private.key.org - test -f "server.csr" && rm -f server.csr + + # 2. smoke test oonib: does it startup, does it react to SIGTERM? - ./bin/oonib & - sleep 30 - ps auxww|grep oonib|grep -v grep - cat oonib.pid - kill -TERM `cat oonib.pid` - - sleep 3 + # The original script was much more robust but made travis segfault and I + # think that it's a good indication of failure to respond to the `TERM` + # signal also the case where the travis build is killed after a timeout of + # ten seconds, so we can afford doing simple things here. + - wait `cat oonib.pid` - ps auxww|grep oonib|grep -v grep - #- chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib - - echo "Build successful." + + # 3. run regress tests - $(which coverage) run $(which trial) oonib + after_success: - coveralls --rcfile=".coveragerc" From dbf01b1ee260a024d51c97bf67f65084c8db6ab3 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 13:01:30 +0200 Subject: [PATCH 09/10] rewrite the test --- .travis.yml | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/.travis.yml b/.travis.yml index 94001348..ff8a9310 100644 --- a/.travis.yml +++ b/.travis.yml @@ -42,16 +42,23 @@ script: # 2. smoke test oonib: does it startup, does it react to SIGTERM? - ./bin/oonib & + # Notes on smoke testing + # ---------------------- + # + # 1. The original smoke testing procedure was much more robust but travis + # started segfaulting and, upon investigating that, it occurred to me + # that having a simplified procedure like the one below would also work. + # + # Specifically, if the process does not exit after `TERM`, my understanding + # is that the build will hang until travis kills it. This would fail the + # build and so we would know that the deamon doesn't handle `TERM`. + # + # 2. I need to copy the pidfile because it is deleted by the daemon when + # it exits, as such, using the original pidfile is racy. + - cp oonib.pid oonib-persist.pid - sleep 30 - - ps auxww|grep oonib|grep -v grep - - cat oonib.pid - - kill -TERM `cat oonib.pid` - # The original script was much more robust but made travis segfault and I - # think that it's a good indication of failure to respond to the `TERM` - # signal also the case where the travis build is killed after a timeout of - # ten seconds, so we can afford doing simple things here. - - wait `cat oonib.pid` - - ps auxww|grep oonib|grep -v grep + - kill -TERM `cat oonib-persist.pid` + - wait `cat oonib-persist.pid` # 3. run regress tests - $(which coverage) run $(which trial) oonib From d09c1c59e09743a3986ef27ca4d31f6878125b80 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 26 Sep 2017 13:03:40 +0200 Subject: [PATCH 10/10] exchange the order of operations to avoid a race --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index ff8a9310..4503c5a3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -55,8 +55,8 @@ script: # # 2. I need to copy the pidfile because it is deleted by the daemon when # it exits, as such, using the original pidfile is racy. - - cp oonib.pid oonib-persist.pid - sleep 30 + - cp oonib.pid oonib-persist.pid - kill -TERM `cat oonib-persist.pid` - wait `cat oonib-persist.pid`