diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 871df24..13f4229 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -5,18 +5,53 @@ on: tags: - '*' -# Three independent channels. The GitHub release asset and the Docker image -# must never be hostage to Maven Central: for 4.9 the Central publish queue -# outlasted the plugin's poll window, the job "failed", and the release and -# docker steps were skipped while Central went on to publish anyway. +# Three independent publish channels, all gated on one `test` job. +# +# The independence is between the channels, not from the tests. The GitHub release +# asset and the Docker image must never be hostage to Maven Central: for 4.9 the +# Central publish queue outlasted the plugin's poll window, the job "failed", and +# the release and docker steps were skipped while Central went on to publish anyway. +# So they still do not depend on each other -- but every one of them now depends on +# `test`, because until it did, `native` and `central` had no `needs:` at all and +# both build with -DskipTests. A tagged commit with failing tests published native +# distributions and a Maven Central artifact; only `docker` was gated, and only +# incidentally, by depending on `build`. +# +# `test` runs `verify` rather than `test`, so the shaded-jar smoke test and JaCoCo +# run here too -- a release is the one place where the packaged artifact being +# runnable has to be checked before anything is uploaded. +# # Workflow permissions are least-privilege by default (see the maven-publish -# hardening); only the job that creates the GitHub release gets write access. +# hardening); only the jobs that create the GitHub release get write access. permissions: contents: read jobs: + # The gate. Nothing is published unless this passes. + test: + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + java-version: '17' + distribution: 'temurin' + cache: 'maven' + + # No versions:set: the tests do not depend on the version, and the smoke test + # resolves the jar through ${project.build.finalName} rather than globbing. + # -Dgpg.skip: signing belongs to the `central` job, which has the key. maven-gpg-plugin + # binds to verify, so without this the gate fails on a missing key rather than on a test. + - name: Verify + run: mvn -B verify -Dgpg.skip=true + build: runs-on: ubuntu-latest + needs: test permissions: contents: write # creates the release and uploads the jar asset @@ -69,6 +104,7 @@ jobs: # matrix. Each build is verified before upload, because a missing module fails at # runtime rather than at build time. native: + needs: test permissions: contents: write strategy: @@ -79,7 +115,10 @@ jobs: asset: testingbot-tunnel-linux-x64 - os: ubuntu-24.04-arm asset: testingbot-tunnel-linux-arm64 - - os: macos-13 + # macos-13 was retired by GitHub on 2025-12-04; macos-15-intel is the + # Intel runner that replaced it. jlink emits a runtime for the machine it + # runs on, so this entry is what makes the x64 macOS build x64. + - os: macos-15-intel asset: testingbot-tunnel-macos-x64 - os: macos-latest asset: testingbot-tunnel-macos-arm64 @@ -190,6 +229,7 @@ jobs: central: runs-on: ubuntu-latest + needs: test steps: - name: Checkout code @@ -212,7 +252,7 @@ jobs: TAG_VERSION=${GITHUB_REF#refs/tags/v} mvn versions:set -DnewVersion=${TAG_VERSION} -DgenerateBackupPoms=false - # Tests run in the build job; this job only deploys. + # Tests run in the `test` job this depends on; this job only deploys. - name: Deploy to Maven Central run: mvn -B clean deploy -DskipTests -Dgpg.passphrase=${{ secrets.GPG_PASSPHRASE }} env: @@ -222,7 +262,10 @@ jobs: docker: runs-on: ubuntu-latest - needs: build + # `test`, not `build`: the point was never to wait for the release to be created, + # only not to publish an image from a failing build. Naming the gate directly says + # that, and lets the image build alongside the release rather than after it. + needs: test steps: - name: Checkout code diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1a763ca..4867417 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -74,8 +74,14 @@ jobs: key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }} restore-keys: ${{ runner.os }}-m2 + # verify, not package: it adds the shaded-jar smoke test and the JaCoCo report, + # which is what the release gate runs. Building with `package` here meant the + # first time a release ever checked the packaged jar was actually runnable was + # after the tag had been pushed. + # -Dgpg.skip: maven-gpg-plugin binds `sign` to the verify phase for the Maven Central + # deploy, and there is no signing key here. Only the `central` release job needs it. - name: Build project - run: mvn clean package + run: mvn clean verify -Dgpg.skip=true - name: Upload build artifacts uses: actions/upload-artifact@v7 diff --git a/dist/build-runtime.sh b/dist/build-runtime.sh index 5f37726..cc213b9 100755 --- a/dist/build-runtime.sh +++ b/dist/build-runtime.sh @@ -14,8 +14,27 @@ HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" ROOT="$(cd "$HERE/.." && pwd)" JAVA_HOME="${JAVA_HOME:-$(/usr/libexec/java_home -v 17 2>/dev/null || dirname "$(dirname "$(readlink -f "$(command -v java)")")")}" -JAR="$(ls "$ROOT"/target/TestingBotTunnel-*-shaded.jar 2>/dev/null | head -1)" -[ -z "$JAR" ] && { echo "No shaded jar. Run: mvn package"; exit 1; } +# The newest shaded jar, and only when it is unambiguous. `ls | head -1` took the +# alphabetically first, so after a version bump without `mvn clean` it silently picked +# the *older* artifact -- 5.10 sorts before 5.9, and the run then verified the previous +# release while reporting the new one. +select_shaded_jar() { + local root="$1" + local jars=() + while IFS= read -r line; do jars+=("$line"); done < <( + ls -t "$root"/target/TestingBotTunnel-*-shaded.jar 2>/dev/null + ) + if [ "${#jars[@]}" -eq 0 ]; then + return 1 + fi + if [ "${#jars[@]}" -gt 1 ]; then + echo "Warning: several shaded jars in target/; using the newest ($(basename "${jars[0]}"))." >&2 + echo " Run 'mvn clean package' if that is not what you meant." >&2 + fi + printf '%s\n' "${jars[0]}" +} + +JAR="$(select_shaded_jar "$ROOT")" || { echo "No shaded jar. Run: mvn package"; exit 1; } case "$(uname -s)" in Darwin) OS=macos;; diff --git a/dist/verify-runtime.sh b/dist/verify-runtime.sh index c15e4f4..8afbac4 100755 --- a/dist/verify-runtime.sh +++ b/dist/verify-runtime.sh @@ -13,11 +13,28 @@ set -uo pipefail HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" DIST="${1:-}" if [ -z "$DIST" ]; then - DIST="$(ls -d "$HERE"/testingbot-tunnel-*-*/ 2>/dev/null | head -1)" + # -t: newest first. Alphabetical order picked an older build when several are present. + DIST="$(ls -dt "$HERE"/testingbot-tunnel-*-*/ 2>/dev/null | head -1)" fi [ -z "$DIST" ] && { echo "usage: verify-runtime.sh "; exit 1; } -LAUNCHER="$DIST/bin/testingbot-tunnel" -[ -x "$LAUNCHER" ] || { echo "launcher not found: $LAUNCHER"; exit 1; } +# build-runtime.sh emits a .cmd launcher on Windows and a shell script everywhere +# else. This looked only for the shell script and required it to be executable, so +# the Windows leg of the release matrix could never pass -- it failed at this line, +# before verifying anything, on every tagged build. +case "$(uname -s)" in + MINGW*|MSYS*|CYGWIN*) IS_WINDOWS=1 ;; + *) IS_WINDOWS=0 ;; +esac + +if [ "$IS_WINDOWS" = "1" ]; then + LAUNCHER="$DIST/bin/testingbot-tunnel.cmd" + # Not -x: a .cmd carries no Unix executable bit, and whether the MSYS layer + # synthesises one is not something to depend on. + [ -f "$LAUNCHER" ] || { echo "launcher not found: $LAUNCHER"; exit 1; } +else + LAUNCHER="$DIST/bin/testingbot-tunnel" + [ -x "$LAUNCHER" ] || { echo "launcher not found: $LAUNCHER"; exit 1; } +fi WORK="$(mktemp -d "${TMPDIR:-/tmp}/tb-runtime.XXXXXX")" PID="" @@ -41,7 +58,31 @@ ok() { PASS=$((PASS+1)); printf ' \033[32m✓\033[0m %s\n' "$1"; } bad() { FAIL=$((FAIL+1)); printf ' \033[31m✗\033[0m %s — %s\n' "$1" "$2"; } # Deliberately strip the environment so a system JDK cannot rescue a missing module. -run_isolated() { env -i HOME="$HOME" PATH=/usr/bin:/bin "$@"; } +# +# Kept as an array rather than only a function: the live-tunnel launch below has to +# invoke it directly, because backgrounding a shell function makes $! the wrapping +# subshell and killing that orphans the JVM underneath it, which then holds a slot +# against the account's concurrent-tunnel limit. +# +# TESTINGBOT_KEY and TESTINGBOT_SECRET are carried through when set. They were not, +# which made the credentialed section below unreachable by the means it tests for: +# the guard admitted a run because the variables were set, and then launched the +# tunnel with an environment that no longer contained them. Only a ~/.testingbot +# file ever actually exercised the live checks. +if [ "$IS_WINDOWS" = "1" ]; then + # env -i is not usable here: the launcher is a .cmd and needs the Windows command + # processor, which is located through the very environment that would be cleared. + # Strip what could actually rescue a missing module instead -- a system JDK on + # PATH or at JAVA_HOME -- which is what the isolation is for. + ISOLATED=(env "JAVA_HOME=" \ + "PATH=$(printf '%s' "$PATH" | tr ':' '\n' | grep -viE 'jdk|jre|/java' | paste -sd: -)") +else + ISOLATED=(env -i "HOME=$HOME" PATH=/usr/bin:/bin) +fi +[ -n "${TESTINGBOT_KEY:-}" ] && ISOLATED+=("TESTINGBOT_KEY=$TESTINGBOT_KEY") +[ -n "${TESTINGBOT_SECRET:-}" ] && ISOLATED+=("TESTINGBOT_SECRET=$TESTINGBOT_SECRET") + +run_isolated() { "${ISOLATED[@]}" "$@"; } echo "Verifying $(basename "$DIST")" @@ -59,11 +100,11 @@ if [ -z "${TESTINGBOT_KEY:-}" ] && [ ! -f "$HOME/.testingbot" ]; then echo " - live tunnel checks skipped (no credentials)" else MPORT="$(python3 -c 'import socket;s=socket.socket();s.bind(("127.0.0.1",0));print(s.getsockname()[1]);s.close()')" - # Launch directly rather than through run_isolated: backgrounding a shell function - # makes $! the wrapping subshell, and killing that orphans the JVM underneath it -- - # which then keeps holding a tunnel slot on the account. - env -i HOME="$HOME" PATH=/usr/bin:/bin \ - "$LAUNCHER" --readyfile "$WORK/ready" --metrics-port "$MPORT" > "$WORK/tunnel.log" 2>&1 & + # The array directly, not run_isolated: backgrounding a shell function makes $! the + # wrapping subshell, and killing that orphans the JVM underneath it -- which then + # keeps holding a tunnel slot on the account. + "${ISOLATED[@]}" "$LAUNCHER" --readyfile "$WORK/ready" --metrics-port "$MPORT" \ + > "$WORK/tunnel.log" 2>&1 & PID=$! for _ in $(seq 1 120); do [ -f "$WORK/ready" ] && break; kill -0 $PID 2>/dev/null || break; sleep 1; done diff --git a/e2e/run-e2e.sh b/e2e/run-e2e.sh index 556b729..b0815ef 100755 --- a/e2e/run-e2e.sh +++ b/e2e/run-e2e.sh @@ -33,8 +33,27 @@ ROOT="$(cd "$HERE/.." && pwd)" # shellcheck source=e2e/webdriver.sh source "$HERE/webdriver.sh" -JAR="$(ls "$ROOT"/target/TestingBotTunnel-*-shaded.jar 2>/dev/null | head -1)" -[ -z "$JAR" ] && { echo "No shaded jar found. Run: mvn package"; exit 1; } +# The newest shaded jar, and only when it is unambiguous. `ls | head -1` took the +# alphabetically first, so after a version bump without `mvn clean` it silently picked +# the *older* artifact -- 5.10 sorts before 5.9, and the run then verified the previous +# release while reporting the new one. +select_shaded_jar() { + local root="$1" + local jars=() + while IFS= read -r line; do jars+=("$line"); done < <( + ls -t "$root"/target/TestingBotTunnel-*-shaded.jar 2>/dev/null + ) + if [ "${#jars[@]}" -eq 0 ]; then + return 1 + fi + if [ "${#jars[@]}" -gt 1 ]; then + echo "Warning: several shaded jars in target/; using the newest ($(basename "${jars[0]}"))." >&2 + echo " Run 'mvn clean package' if that is not what you meant." >&2 + fi + printf '%s\n' "${jars[0]}" +} + +JAR="$(select_shaded_jar "$ROOT")" || { echo "No shaded jar found. Run: mvn package"; exit 1; } WORK="$(mktemp -d "${TMPDIR:-/tmp}/tb-e2e.XXXXXX")" MARKER="TB-E2E-$(date +%s)-$$" diff --git a/examples/docker-compose-prometheus-grafana/grafana/provisioning/dashboards/testingbot_tunnel.json b/examples/docker-compose-prometheus-grafana/grafana/provisioning/dashboards/testingbot_tunnel.json index 5aed870..a0a4f82 100644 --- a/examples/docker-compose-prometheus-grafana/grafana/provisioning/dashboards/testingbot_tunnel.json +++ b/examples/docker-compose-prometheus-grafana/grafana/provisioning/dashboards/testingbot_tunnel.json @@ -3,7 +3,10 @@ "list": [ { "builtIn": 1, - "datasource": {"type": "grafana", "uid": "-- Grafana --"}, + "datasource": { + "type": "grafana", + "uid": "-- Grafana --" + }, "enable": true, "hide": true, "iconColor": "rgba(0, 211, 255, 1)", @@ -21,37 +24,85 @@ "panels": [ { "collapsed": false, - "gridPos": {"h": 1, "w": 24, "x": 0, "y": 0}, + "gridPos": { + "h": 1, + "w": 24, + "x": 0, + "y": 0 + }, "id": 100, "panels": [], "title": "Overview", "type": "row" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { "mappings": [ - {"options": {"0": {"text": "DOWN", "color": "red"}, "1": {"text": "UP", "color": "green"}}, "type": "value"} + { + "options": { + "0": { + "text": "DOWN", + "color": "red" + }, + "1": { + "text": "UP", + "color": "green" + } + }, + "type": "value" + } ], - "thresholds": {"mode": "absolute", "steps": [{"color": "red", "value": null}, {"color": "green", "value": 1}]}, - "color": {"mode": "thresholds"} + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "red", + "value": null + }, + { + "color": "green", + "value": 1 + } + ] + }, + "color": { + "mode": "thresholds" + } } }, - "gridPos": {"h": 4, "w": 4, "x": 0, "y": 1}, + "gridPos": { + "h": 4, + "w": 4, + "x": 0, + "y": 1 + }, "id": 1, "options": { "colorMode": "background", "graphMode": "none", "justifyMode": "center", "orientation": "auto", - "reduceOptions": {"calcs": ["lastNotNull"], "fields": "", "values": false}, + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, "textMode": "value" }, "pluginVersion": "10.4.3", "targets": [ { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "expr": "max(testingbot_tunnel_up{job=~\"$job\", instance=~\"$instance\"})", "refId": "A" } @@ -60,27 +111,54 @@ "type": "stat" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "thresholds"}, - "thresholds": {"mode": "absolute", "steps": [{"color": "blue", "value": null}]} + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "blue", + "value": null + } + ] + } } }, - "gridPos": {"h": 4, "w": 8, "x": 4, "y": 1}, + "gridPos": { + "h": 4, + "w": 8, + "x": 4, + "y": 1 + }, "id": 2, "options": { "colorMode": "value", "graphMode": "none", "justifyMode": "auto", "orientation": "auto", - "reduceOptions": {"calcs": ["lastNotNull"], "fields": "", "values": false}, + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, "textMode": "value_and_name" }, "pluginVersion": "10.4.3", "targets": [ { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "expr": "testingbot_tunnel_info{job=~\"$job\", instance=~\"$instance\"}", "legendFormat": "v{{version}} id={{tunnel_id}} name={{identifier}}", "refId": "A" @@ -90,28 +168,55 @@ "type": "stat" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "thresholds"}, - "thresholds": {"mode": "absolute", "steps": [{"color": "green", "value": null}]}, + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "green", + "value": null + } + ] + }, "unit": "short" } }, - "gridPos": {"h": 4, "w": 4, "x": 12, "y": 1}, + "gridPos": { + "h": 4, + "w": 4, + "x": 12, + "y": 1 + }, "id": 3, "options": { "colorMode": "value", "graphMode": "area", "justifyMode": "center", "orientation": "auto", - "reduceOptions": {"calcs": ["lastNotNull"], "fields": "", "values": false}, + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, "textMode": "auto" }, "pluginVersion": "10.4.3", "targets": [ { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "expr": "sum(testingbot_active_connections{job=~\"$job\", instance=~\"$instance\"})", "refId": "A" } @@ -120,28 +225,55 @@ "type": "stat" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "thresholds"}, - "thresholds": {"mode": "absolute", "steps": [{"color": "blue", "value": null}]}, + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "blue", + "value": null + } + ] + }, "unit": "s" } }, - "gridPos": {"h": 4, "w": 4, "x": 16, "y": 1}, + "gridPos": { + "h": 4, + "w": 4, + "x": 16, + "y": 1 + }, "id": 4, "options": { "colorMode": "value", "graphMode": "none", "justifyMode": "center", "orientation": "auto", - "reduceOptions": {"calcs": ["lastNotNull"], "fields": "", "values": false}, + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, "textMode": "auto" }, "pluginVersion": "10.4.3", "targets": [ { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "expr": "max(testingbot_tunnel_uptime_seconds{job=~\"$job\", instance=~\"$instance\"})", "refId": "A" } @@ -150,28 +282,63 @@ "type": "stat" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "thresholds"}, - "thresholds": {"mode": "absolute", "steps": [{"color": "green", "value": null}, {"color": "orange", "value": 1}, {"color": "red", "value": 5}]}, + "color": { + "mode": "thresholds" + }, + "thresholds": { + "mode": "absolute", + "steps": [ + { + "color": "green", + "value": null + }, + { + "color": "orange", + "value": 1 + }, + { + "color": "red", + "value": 5 + } + ] + }, "unit": "short" } }, - "gridPos": {"h": 4, "w": 4, "x": 20, "y": 1}, + "gridPos": { + "h": 4, + "w": 4, + "x": 20, + "y": 1 + }, "id": 5, "options": { "colorMode": "value", "graphMode": "none", "justifyMode": "center", "orientation": "auto", - "reduceOptions": {"calcs": ["lastNotNull"], "fields": "", "values": false}, + "reduceOptions": { + "calcs": [ + "lastNotNull" + ], + "fields": "", + "values": false + }, "textMode": "auto" }, "pluginVersion": "10.4.3", "targets": [ { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "expr": "sum(testingbot_tunnel_reconnects_total{job=~\"$job\", instance=~\"$instance\"})", "refId": "A" } @@ -181,261 +348,1289 @@ }, { "collapsed": false, - "gridPos": {"h": 1, "w": 24, "x": 0, "y": 5}, + "gridPos": { + "h": 1, + "w": 24, + "x": 0, + "y": 5 + }, "id": 101, "panels": [], "title": "HTTP", "type": "row" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"axisCenteredZero": false, "axisColorMode": "text", "axisLabel": "", "axisPlacement": "auto", "barAlignment": 0, "drawStyle": "line", "fillOpacity": 10, "gradientMode": "none", "hideFrom": {"legend": false, "tooltip": false, "viz": false}, "lineInterpolation": "linear", "lineWidth": 1, "pointSize": 5, "scaleDistribution": {"type": "linear"}, "showPoints": "never", "spanNulls": false, "stacking": {"group": "A", "mode": "normal"}, "thresholdsStyle": {"mode": "off"}}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "axisCenteredZero": false, + "axisColorMode": "text", + "axisLabel": "", + "axisPlacement": "auto", + "barAlignment": 0, + "drawStyle": "line", + "fillOpacity": 10, + "gradientMode": "none", + "hideFrom": { + "legend": false, + "tooltip": false, + "viz": false + }, + "lineInterpolation": "linear", + "lineWidth": 1, + "pointSize": 5, + "scaleDistribution": { + "type": "linear" + }, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + }, + "thresholdsStyle": { + "mode": "off" + } + }, "unit": "reqps" } }, - "gridPos": {"h": 8, "w": 12, "x": 0, "y": 6}, + "gridPos": { + "h": 8, + "w": 12, + "x": 0, + "y": 6 + }, "id": 10, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(rate(testingbot_http_requests_total{method!=\"CONNECT\", code=~\"2..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "2xx", "refId": "A"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(rate(testingbot_http_requests_total{method!=\"CONNECT\", code=~\"3..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "3xx", "refId": "B"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(rate(testingbot_http_requests_total{method!=\"CONNECT\", code=~\"4..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "4xx", "refId": "C"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(rate(testingbot_http_requests_total{method!=\"CONNECT\", code=~\"5..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "5xx", "refId": "D"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(rate(testingbot_http_requests_total{method!=\"CONNECT\", code=~\"2..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "2xx", + "refId": "A" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(rate(testingbot_http_requests_total{method!=\"CONNECT\", code=~\"3..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "3xx", + "refId": "B" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(rate(testingbot_http_requests_total{method!=\"CONNECT\", code=~\"4..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "4xx", + "refId": "C" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(rate(testingbot_http_requests_total{method!=\"CONNECT\", code=~\"5..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "5xx", + "refId": "D" + } ], "title": "HTTP Request Rate (non-CONNECT)", "type": "timeseries" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 10, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never", "spanNulls": false, "stacking": {"group": "A", "mode": "normal"}}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 10, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + } + }, "unit": "reqps" } }, - "gridPos": {"h": 8, "w": 12, "x": 12, "y": 6}, + "gridPos": { + "h": 8, + "w": 12, + "x": 12, + "y": 6 + }, "id": 11, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(rate(testingbot_https_connect_total{code=~\"2..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "2xx", "refId": "A"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(rate(testingbot_https_connect_total{code=~\"4..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "4xx", "refId": "B"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(rate(testingbot_https_connect_total{code=~\"5..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "5xx", "refId": "C"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(rate(testingbot_https_connect_total{code=~\"2..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "2xx", + "refId": "A" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(rate(testingbot_https_connect_total{code=~\"4..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "4xx", + "refId": "B" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(rate(testingbot_https_connect_total{code=~\"5..\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "5xx", + "refId": "C" + } ], "title": "HTTPS CONNECT Rate", "type": "timeseries" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 0, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never", "spanNulls": false}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 0, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never", + "spanNulls": false + }, "unit": "s" } }, - "gridPos": {"h": 8, "w": 8, "x": 0, "y": 14}, + "gridPos": { + "h": 8, + "w": 8, + "x": 0, + "y": 14 + }, "id": 12, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.5, sum by (le) (rate(testingbot_http_request_duration_seconds_bucket{method!=\"CONNECT\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p50", "refId": "A"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.95, sum by (le) (rate(testingbot_http_request_duration_seconds_bucket{method!=\"CONNECT\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p95", "refId": "B"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.99, sum by (le) (rate(testingbot_http_request_duration_seconds_bucket{method!=\"CONNECT\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p99", "refId": "C"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.5, sum by (le) (rate(testingbot_http_request_duration_seconds_bucket{method!=\"CONNECT\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p50", + "refId": "A" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.95, sum by (le) (rate(testingbot_http_request_duration_seconds_bucket{method!=\"CONNECT\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p95", + "refId": "B" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.99, sum by (le) (rate(testingbot_http_request_duration_seconds_bucket{method!=\"CONNECT\", job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p99", + "refId": "C" + } ], "title": "HTTP Latency (non-CONNECT)", "type": "timeseries" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 0, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never", "spanNulls": false}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 0, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never", + "spanNulls": false + }, "unit": "s" } }, - "gridPos": {"h": 8, "w": 8, "x": 8, "y": 14}, + "gridPos": { + "h": 8, + "w": 8, + "x": 8, + "y": 14 + }, "id": 14, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.5, sum by (le) (rate(testingbot_https_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p50", "refId": "A"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.95, sum by (le) (rate(testingbot_https_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p95", "refId": "B"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.99, sum by (le) (rate(testingbot_https_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p99", "refId": "C"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.5, sum by (le) (rate(testingbot_https_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p50", + "refId": "A" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.95, sum by (le) (rate(testingbot_https_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p95", + "refId": "B" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.99, sum by (le) (rate(testingbot_https_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p99", + "refId": "C" + } ], "title": "HTTPS CONNECT Latency", "type": "timeseries" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 0, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never", "spanNulls": false}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 0, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never", + "spanNulls": false + }, "unit": "binBps" }, "description": "HTTP response bytes only. HTTPS CONNECT traffic flows through a raw socket relay and is not counted here." }, - "gridPos": {"h": 8, "w": 8, "x": 16, "y": 14}, + "gridPos": { + "h": 8, + "w": 8, + "x": 16, + "y": 14 + }, "id": 13, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "single"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "single" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(rate(testingbot_proxy_bytes_transferred_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval]))", "legendFormat": "response bytes/s", "refId": "A"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(rate(testingbot_proxy_bytes_transferred_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval]))", + "legendFormat": "response bytes/s", + "refId": "A" + } ], "title": "HTTP Response Throughput", "type": "timeseries" }, { "collapsed": false, - "gridPos": {"h": 1, "w": 24, "x": 0, "y": 22}, + "gridPos": { + "h": 1, + "w": 24, + "x": 0, + "y": 22 + }, "id": 102, "panels": [], "title": "Tunnel", "type": "row" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 20, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never"}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 20, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never" + }, "unit": "short" } }, - "gridPos": {"h": 8, "w": 12, "x": 0, "y": 23}, + "gridPos": { + "h": 8, + "w": 12, + "x": 0, + "y": 23 + }, "id": 20, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "single"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "single" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum(testingbot_active_connections{job=~\"$job\", instance=~\"$instance\"})", "legendFormat": "active", "refId": "A"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum(testingbot_active_connections{job=~\"$job\", instance=~\"$instance\"})", + "legendFormat": "active", + "refId": "A" + } ], "title": "Active Connections", "type": "timeseries" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 0, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never"}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 0, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never" + }, "unit": "s" } }, - "gridPos": {"h": 8, "w": 12, "x": 12, "y": 23}, + "gridPos": { + "h": 8, + "w": 12, + "x": 12, + "y": 23 + }, "id": 21, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.5, sum by (le) (rate(testingbot_tunnel_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p50", "refId": "A"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.95, sum by (le) (rate(testingbot_tunnel_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p95", "refId": "B"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "histogram_quantile(0.99, sum by (le) (rate(testingbot_tunnel_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", "legendFormat": "p99", "refId": "C"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.5, sum by (le) (rate(testingbot_tunnel_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p50", + "refId": "A" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.95, sum by (le) (rate(testingbot_tunnel_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p95", + "refId": "B" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "histogram_quantile(0.99, sum by (le) (rate(testingbot_tunnel_connect_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "p99", + "refId": "C" + } ], "title": "SSH Connect Latency", "type": "timeseries" }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "palette-classic" + }, + "custom": { + "axisCenteredZero": false, + "axisColorMode": "text", + "axisLabel": "", + "axisPlacement": "auto", + "barAlignment": 0, + "drawStyle": "line", + "fillOpacity": 10, + "gradientMode": "none", + "hideFrom": { + "legend": false, + "tooltip": false, + "viz": false + }, + "lineInterpolation": "linear", + "lineWidth": 1, + "pointSize": 5, + "scaleDistribution": { + "type": "linear" + }, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + }, + "thresholdsStyle": { + "mode": "off" + } + }, + "unit": "reqps" + } + }, + "gridPos": { + "h": 8, + "w": 12, + "x": 0, + "y": 31 + }, + "id": 22, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, + "targets": [ + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "editorMode": "code", + "expr": "testingbot_connections_current{job=~\"$job\", instance=~\"$instance\"}", + "legendFormat": "{{listener}}", + "range": true, + "refId": "A" + } + ], + "title": "Connections by listener", + "type": "timeseries", + "description": "Open connections per listener. Separating the Selenium relay from the proxy is what makes \"is the relay wedged?\" answerable." + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "palette-classic" + }, + "custom": { + "axisCenteredZero": false, + "axisColorMode": "text", + "axisLabel": "", + "axisPlacement": "auto", + "barAlignment": 0, + "drawStyle": "line", + "fillOpacity": 10, + "gradientMode": "none", + "hideFrom": { + "legend": false, + "tooltip": false, + "viz": false + }, + "lineInterpolation": "linear", + "lineWidth": 1, + "pointSize": 5, + "scaleDistribution": { + "type": "linear" + }, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + }, + "thresholdsStyle": { + "mode": "off" + } + }, + "unit": "reqps" + } + }, + "gridPos": { + "h": 8, + "w": 12, + "x": 12, + "y": 31 + }, + "id": 23, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, + "targets": [ + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "editorMode": "code", + "expr": "sum by (path, outcome) (rate(testingbot_dial_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval]))", + "legendFormat": "{{path}} {{outcome}}", + "range": true, + "refId": "A" + } + ], + "title": "Outbound dial rate by path", + "type": "timeseries", + "description": "Outbound connection attempts. Connection exhaustion and a target that has started refusing are both invisible without this." + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "palette-classic" + }, + "custom": { + "axisCenteredZero": false, + "axisColorMode": "text", + "axisLabel": "", + "axisPlacement": "auto", + "barAlignment": 0, + "drawStyle": "line", + "fillOpacity": 10, + "gradientMode": "none", + "hideFrom": { + "legend": false, + "tooltip": false, + "viz": false + }, + "lineInterpolation": "linear", + "lineWidth": 1, + "pointSize": 5, + "scaleDistribution": { + "type": "linear" + }, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + }, + "thresholdsStyle": { + "mode": "off" + } + }, + "unit": "s" + } + }, + "gridPos": { + "h": 8, + "w": 12, + "x": 0, + "y": 39 + }, + "id": 24, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, + "targets": [ + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "editorMode": "code", + "expr": "histogram_quantile(0.95, sum by (le, path) (rate(testingbot_dial_duration_seconds_bucket{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])))", + "legendFormat": "{{path}}", + "range": true, + "refId": "A" + } + ], + "title": "Outbound dial latency (p95)", + "type": "timeseries" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "palette-classic" + }, + "custom": { + "axisCenteredZero": false, + "axisColorMode": "text", + "axisLabel": "", + "axisPlacement": "auto", + "barAlignment": 0, + "drawStyle": "line", + "fillOpacity": 10, + "gradientMode": "none", + "hideFrom": { + "legend": false, + "tooltip": false, + "viz": false + }, + "lineInterpolation": "linear", + "lineWidth": 1, + "pointSize": 5, + "scaleDistribution": { + "type": "linear" + }, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + }, + "thresholdsStyle": { + "mode": "off" + } + }, + "unit": "Bps" + } + }, + "gridPos": { + "h": 8, + "w": 12, + "x": 12, + "y": 39 + }, + "id": 25, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, + "targets": [ + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "editorMode": "code", + "expr": "sum by (listener) (rate(testingbot_connection_bytes_received_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval]))", + "legendFormat": "rx {{listener}}", + "range": true, + "refId": "A" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "editorMode": "code", + "expr": "sum by (listener) (rate(testingbot_connection_bytes_sent_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval]))", + "legendFormat": "tx {{listener}}", + "range": true, + "refId": "B" + } + ], + "title": "Tunnel throughput by listener", + "type": "timeseries", + "description": "Counted at the connector, so unlike HTTP Response Throughput this includes tunnelled CONNECT and WebSocket bytes." + }, { "collapsed": false, - "gridPos": {"h": 1, "w": 24, "x": 0, "y": 31}, + "gridPos": { + "h": 1, + "w": 24, + "x": 0, + "y": 47 + }, "id": 103, "panels": [], "title": "Errors", "type": "row" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 30, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never", "spanNulls": false, "stacking": {"group": "A", "mode": "normal"}}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 30, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + } + }, "unit": "short" } }, - "gridPos": {"h": 8, "w": 12, "x": 0, "y": 32}, + "gridPos": { + "h": 8, + "w": 12, + "x": 0, + "y": 48 + }, "id": 30, - "options": {"legend": {"calcs": ["sum"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "sum" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum by (name) (rate(testingbot_errors_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "{{name}}", "refId": "A"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum by (name) (rate(testingbot_errors_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "{{name}}", + "refId": "A" + } ], "title": "Errors Rate by name", "type": "timeseries" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 30, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never", "spanNulls": false, "stacking": {"group": "A", "mode": "normal"}}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 30, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + } + }, "unit": "short" } }, - "gridPos": {"h": 8, "w": 12, "x": 12, "y": 32}, + "gridPos": { + "h": 8, + "w": 12, + "x": 12, + "y": 48 + }, "id": 31, - "options": {"legend": {"calcs": ["sum"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "sum" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum by (reason) (rate(testingbot_https_connect_errors_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", "legendFormat": "{{reason}}", "refId": "A"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum by (reason) (rate(testingbot_https_connect_errors_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])) or on() vector(0)", + "legendFormat": "{{reason}}", + "refId": "A" + } ], "title": "HTTPS CONNECT Errors by reason", "type": "timeseries" }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "fieldConfig": { + "defaults": { + "color": { + "mode": "palette-classic" + }, + "custom": { + "axisCenteredZero": false, + "axisColorMode": "text", + "axisLabel": "", + "axisPlacement": "auto", + "barAlignment": 0, + "drawStyle": "line", + "fillOpacity": 10, + "gradientMode": "none", + "hideFrom": { + "legend": false, + "tooltip": false, + "viz": false + }, + "lineInterpolation": "linear", + "lineWidth": 1, + "pointSize": 5, + "scaleDistribution": { + "type": "linear" + }, + "showPoints": "never", + "spanNulls": false, + "stacking": { + "group": "A", + "mode": "normal" + }, + "thresholdsStyle": { + "mode": "off" + } + }, + "unit": "reqps" + } + }, + "gridPos": { + "h": 8, + "w": 24, + "x": 0, + "y": 56 + }, + "id": 32, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, + "targets": [ + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "editorMode": "code", + "expr": "sum by (reason) (rate(testingbot_proxy_errors_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval]))", + "legendFormat": "{{reason}}", + "range": true, + "refId": "A" + } + ], + "title": "Proxy Errors by reason", + "type": "timeseries", + "description": "Classified proxy failures. The reason label matches the X-TestingBot-Error header on the response, so a spike here and a header in a support ticket name the same thing." + }, { "collapsed": false, - "gridPos": {"h": 1, "w": 24, "x": 0, "y": 40}, + "gridPos": { + "h": 1, + "w": 24, + "x": 0, + "y": 64 + }, "id": 104, "panels": [], "title": "Resources (JVM)", "type": "row" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 10, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never"}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 10, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never" + }, "unit": "bytes" } }, - "gridPos": {"h": 8, "w": 8, "x": 0, "y": 41}, + "gridPos": { + "h": 8, + "w": 8, + "x": 0, + "y": 65 + }, "id": 40, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "sum by (area) (jvm_memory_bytes_used{job=~\"$job\", instance=~\"$instance\"})", "legendFormat": "{{area}}", "refId": "A"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "sum by (area) (jvm_memory_bytes_used{job=~\"$job\", instance=~\"$instance\"})", + "legendFormat": "{{area}}", + "refId": "A" + } ], "title": "JVM Memory Used", "type": "timeseries" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 10, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never"}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 10, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never" + }, "unit": "percentunit" } }, - "gridPos": {"h": 8, "w": 8, "x": 8, "y": 41}, + "gridPos": { + "h": 8, + "w": 8, + "x": 8, + "y": 65 + }, "id": 41, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "single"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "single" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "rate(process_cpu_seconds_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])", "legendFormat": "cpu", "refId": "A"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "rate(process_cpu_seconds_total{job=~\"$job\", instance=~\"$instance\"}[$__rate_interval])", + "legendFormat": "cpu", + "refId": "A" + } ], "title": "Process CPU", "type": "timeseries" }, { - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "fieldConfig": { "defaults": { - "color": {"mode": "palette-classic"}, - "custom": {"drawStyle": "line", "fillOpacity": 10, "lineInterpolation": "linear", "lineWidth": 1, "showPoints": "never"}, + "color": { + "mode": "palette-classic" + }, + "custom": { + "drawStyle": "line", + "fillOpacity": 10, + "lineInterpolation": "linear", + "lineWidth": 1, + "showPoints": "never" + }, "unit": "short" } }, - "gridPos": {"h": 8, "w": 8, "x": 16, "y": 41}, + "gridPos": { + "h": 8, + "w": 8, + "x": 16, + "y": 65 + }, "id": 42, - "options": {"legend": {"calcs": ["mean", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true}, "tooltip": {"mode": "multi", "sort": "desc"}}, + "options": { + "legend": { + "calcs": [ + "mean", + "max" + ], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, + "tooltip": { + "mode": "multi", + "sort": "desc" + } + }, "targets": [ - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "jvm_threads_current{job=~\"$job\", instance=~\"$instance\"}", "legendFormat": "current", "refId": "A"}, - {"datasource": {"type": "prometheus", "uid": "$datasource"}, "expr": "jvm_threads_daemon{job=~\"$job\", instance=~\"$instance\"}", "legendFormat": "daemon", "refId": "B"} + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "jvm_threads_current{job=~\"$job\", instance=~\"$instance\"}", + "legendFormat": "current", + "refId": "A" + }, + { + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, + "expr": "jvm_threads_daemon{job=~\"$job\", instance=~\"$instance\"}", + "legendFormat": "daemon", + "refId": "B" + } ], "title": "JVM Threads", "type": "timeseries" @@ -443,11 +1638,18 @@ ], "refresh": "10s", "schemaVersion": 39, - "tags": ["testingbot", "tunnel"], + "tags": [ + "testingbot", + "tunnel" + ], "templating": { "list": [ { - "current": {"selected": false, "text": "Prometheus", "value": "Prometheus"}, + "current": { + "selected": false, + "text": "Prometheus", + "value": "Prometheus" + }, "hide": 0, "includeAll": false, "label": "Datasource", @@ -462,8 +1664,15 @@ "type": "datasource" }, { - "current": {"selected": false, "text": "All", "value": "$__all"}, - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "current": { + "selected": false, + "text": "All", + "value": "$__all" + }, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "definition": "label_values(testingbot_tunnel_uptime_seconds, job)", "hide": 0, "includeAll": true, @@ -471,7 +1680,10 @@ "multi": false, "name": "job", "options": [], - "query": {"query": "label_values(testingbot_tunnel_uptime_seconds, job)", "refId": "PrometheusVariableQueryEditor-VariableQuery"}, + "query": { + "query": "label_values(testingbot_tunnel_uptime_seconds, job)", + "refId": "PrometheusVariableQueryEditor-VariableQuery" + }, "refresh": 2, "regex": "", "skipUrlSync": false, @@ -479,8 +1691,15 @@ "type": "query" }, { - "current": {"selected": false, "text": "All", "value": "$__all"}, - "datasource": {"type": "prometheus", "uid": "$datasource"}, + "current": { + "selected": false, + "text": "All", + "value": "$__all" + }, + "datasource": { + "type": "prometheus", + "uid": "$datasource" + }, "definition": "label_values(testingbot_tunnel_uptime_seconds{job=~\"$job\"}, instance)", "hide": 0, "includeAll": true, @@ -488,7 +1707,10 @@ "multi": true, "name": "instance", "options": [], - "query": {"query": "label_values(testingbot_tunnel_uptime_seconds{job=~\"$job\"}, instance)", "refId": "PrometheusVariableQueryEditor-VariableQuery"}, + "query": { + "query": "label_values(testingbot_tunnel_uptime_seconds{job=~\"$job\"}, instance)", + "refId": "PrometheusVariableQueryEditor-VariableQuery" + }, "refresh": 2, "regex": "", "skipUrlSync": false, @@ -497,11 +1719,14 @@ } ] }, - "time": {"from": "now-30m", "to": "now"}, + "time": { + "from": "now-30m", + "to": "now" + }, "timepicker": {}, "timezone": "", "title": "TestingBot Tunnel", "uid": "testingbot-tunnel", "version": 1, "weekStart": "" -} +} \ No newline at end of file diff --git a/pom.xml b/pom.xml index 13f6a70..74209ff 100644 --- a/pom.xml +++ b/pom.xml @@ -160,6 +160,21 @@ + + org.apache.maven.plugins + maven-dependency-plugin + 3.11.0 + + + + resolve-agent-paths + process-test-classes + properties + + + org.apache.maven.plugins maven-surefire-plugin @@ -168,7 +183,11 @@ **/*Test.java - @{argLine} --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED + + @{argLine} -javaagent:${org.mockito:mockito-core:jar} --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED 1 false @@ -329,6 +348,41 @@ report + + + coverage-floor + verify + + check + + + + + BUNDLE + + + INSTRUCTION + COVEREDRATIO + 0.82 + + + BRANCH + COVEREDRATIO + 0.73 + + + LINE + COVEREDRATIO + 0.81 + + + + + + @@ -482,6 +536,38 @@ httpclient5 5.6.4 + + + org.apache.httpcomponents.core5 + httpcore5 + 5.4.3 + + + com.fasterxml.jackson.core + jackson-core + 2.22.2 + + + org.eclipse.jetty + jetty-client + + + org.eclipse.jetty + jetty-http + + + org.eclipse.jetty + jetty-io + + + org.eclipse.jetty + jetty-util + diff --git a/src/main/java/com/testingbot/tunnel/Api.java b/src/main/java/com/testingbot/tunnel/Api.java index cfc66e8..a7d706f 100644 --- a/src/main/java/com/testingbot/tunnel/Api.java +++ b/src/main/java/com/testingbot/tunnel/Api.java @@ -112,6 +112,19 @@ private RequestConfig defaultRequestConfig() { * could start -- so a SOCKS5 upstream proxy never worked at all. {@code http://host:port} * was broken the same way. */ + /** + * The control-plane client builder, for callers outside a live Api. + * + *

{@code --doctor} runs before an Api exists but has to reach TestingBot the same way one + * would, or it reports a route nobody uses. It built its own client and got it partly wrong: + * no SOCKS5 support, and no credentials for an authenticated proxy -- so on those networks it + * said "can not be reached" and exited 1 while the tunnel started perfectly, or reached the + * API by a path the tunnel would not have taken. + */ + static HttpClientBuilder controlPlaneBuilder(App app) { + return new Api(app).newBuilderWithProxy(); + } + private HttpClientBuilder newBuilderWithProxy() { HttpClientBuilder builder = httpClientBuilderSupplier.get(); builder.setDefaultRequestConfig(defaultRequestConfig()); @@ -393,9 +406,23 @@ private JsonNode _post(String url, List postData) throws Exceptio postRequest.setHeader("Authorization", "Basic " + encoding); postRequest.setEntity(new UrlEncodedFormEntity(postData)); - responseBody = httpClient.execute(postRequest, response -> - EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8) - ); + responseBody = httpClient.execute(postRequest, response -> { + // The status, not just the body. Without this a 500 carrying + // {"message":"failure"} parsed cleanly and was handed back as tunnel data, + // so an API outage read as a malformed response -- or worse, as a tunnel + // whose fields happened to be absent. _get has always checked; this is the + // path that creates the tunnel. + // + // The body is read either way and included: the API says why it refused, + // and the reason is the whole value of the message to the user. + String body = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8); + if (response.getCode() < 200 || response.getCode() >= 300) { + throw new RuntimeException("Failed : HTTP error code : " + + response.getCode() + + (body == null || body.isBlank() ? "" : " - " + body)); + } + return body; + }); } try { diff --git a/src/main/java/com/testingbot/tunnel/App.java b/src/main/java/com/testingbot/tunnel/App.java index 98fb17c..43c88eb 100644 --- a/src/main/java/com/testingbot/tunnel/App.java +++ b/src/main/java/com/testingbot/tunnel/App.java @@ -29,7 +29,18 @@ import ssh.TunnelPoller; public class App { - public static final Float VERSION = getVersionFromProperties(); + /** + * The release version as written, e.g. {@code 5.0} or {@code 5.10.2}. + * + *

Was a {@code Float}, which could not represent this project's own version scheme: + * {@code 5.10} became {@code 5.1f} and sorted below {@code 5.9}, and {@code 5.0.1} did not + * parse at all and silently became {@code 0.0}. Every use of it here is display, so a + * string is what was wanted throughout; {@link #RELEASE} does the comparing. + */ + public static final String VERSION = getVersionFromProperties(); + + /** {@link #VERSION} parsed for comparison, or null when the properties could not be read. */ + static final Version RELEASE = Version.parse(VERSION); private Api api; private String clientKey; private String clientSecret; @@ -120,22 +131,27 @@ public class App { private int sshPort = 0; private boolean shared = false; - private static Float getVersionFromProperties() { + private static String getVersionFromProperties() { try (InputStream input = App.class.getClassLoader().getResourceAsStream("version.properties")) { if (input == null) { - return 0.0f; + return "unknown"; } Properties prop = new Properties(); prop.load(input); String version = prop.getProperty("version"); - if (version != null) { - String numericVersion = version.replaceAll("-SNAPSHOT", "").replaceAll("[^0-9.]", ""); - return Float.parseFloat(numericVersion); + if (version != null && !version.isBlank()) { + // Returned as written. The stripping that used to happen here -- removing + // -SNAPSHOT and then every non-digit -- existed only to make Float.parseFloat + // accept the result, and it is what turned 5.0.1 into an unparseable "5.0.1" + // and then, via the catch below, into 0.0. + return version.trim(); } - } catch (IOException | NumberFormatException ex) { + } catch (IOException ex) { Logger.getLogger(App.class.getName()).log(Level.WARNING, "Could not read version from properties, using fallback", ex); } - return 0.0f; + // Not "0.0": that is not "unknown", it is "older than every release", which is what made + // the upgrade notice fire on every startup once the version stopped parsing. + return "unknown"; } /** Matches maven.compiler.release; the jar's class files cannot load below this. */ @@ -880,7 +896,8 @@ public static void main(String... args) throws Exception { ConsoleHandler handler = new ConsoleHandler(); // Read from the command line rather than the App, which does not exist yet: the // console handler is installed before anything is parsed into an App. - handler.setFormatter(logFormatterFor(requestedLogFormat(commandLine))); + activeLogFormat = requestedLogFormat(commandLine); + handler.setFormatter(logFormatterFor(activeLogFormat)); logger.addHandler(handler); if ("json".equalsIgnoreCase(requestedLogFormat(commandLine))) { // Sibling JUL loggers (HttpProxy, Doctor, SSHTunnel, the handlers) publish @@ -891,6 +908,14 @@ public static void main(String... args) throws Exception { : Logger.getLogger("").getHandlers()) { rootHandler.setFormatter(new JsonLogFormatter()); } + // And the other logging stack. This process logs through both: JUL for its own + // classes and SLF4J/logback for Jetty, Apache HC and the proxy handlers, and + // logback.xml pins its console appender to a text pattern. So --log-format json + // produced a stream that was JSON for some records and text for others -- which + // is not a format at all, and worse than plain text for the collector this + // option exists to serve. The file appender below already did this; the console + // is where almost every record actually goes. + jsonifyLogbackConsole((LoggerContext) LoggerFactory.getILoggerFactory()); } App app = new App(); @@ -978,20 +1003,32 @@ public static void main(String... args) throws Exception { return; } - System.out.println("----------------------------------------------------------------"); - System.out.println(" TestingBot Tunnel v" + App.VERSION + " "); - System.out.println(" Questions or suggestions, please visit https://testingbot.com "); - System.out.println("----------------------------------------------------------------"); + // Suppressed under --log-format json: four lines of ASCII art on stdout is four + // parse failures for a collector reading one object per line, and it is the very + // first thing it would meet. + if (!"json".equalsIgnoreCase(requestedLogFormat(commandLine))) { + System.out.println("----------------------------------------------------------------"); + System.out.println(" TestingBot Tunnel v" + App.VERSION + " "); + System.out.println(" Questions or suggestions, please visit https://testingbot.com "); + System.out.println("----------------------------------------------------------------"); + } else { + Logger.getLogger(App.class.getName()).log(Level.INFO, + "TestingBot Tunnel {0}", App.VERSION); + } applyCredentials(app, commandLine); applyOptions(app, commandLine); if (commandLine.hasOption("web")) { - new LocalWebServer(commandLine.getOptionValue("web"), app.getBindAddress()); + // Kept on the App so stop() can shut it down. It used to be constructed and + // dropped, which left it serving the directory for the life of the JVM. + app.localWebServer = new LocalWebServer( + commandLine.getOptionValue("web"), app.getBindAddress()); } app.init(); + app.commandLineClient = true; app.boot(); // The pid file lets an external supervisor stop this process; it is // only meaningful when running as a command line client. @@ -1011,10 +1048,34 @@ public static void main(String... args) throws Exception { System.err.println(parseException.getMessage()); System.exit(2); } catch (TunnelFailedException tunnelFailedException) { - System.err.println(tunnelFailedException.getMessage()); + // Under json this goes through the logger instead: JUL's console handler writes to + // stderr, so printing here would put a bare multi-line message in the middle of the + // JSON stream -- and this is a multi-line message, so it is several parse failures. + if ("json".equalsIgnoreCase(activeLogFormat)) { + Logger.getLogger(App.class.getName()).log(Level.SEVERE, + tunnelFailedException.getMessage()); + } else { + System.err.println(tunnelFailedException.getMessage()); + } System.exit(tunnelFailedException.getExitCode()); } } + /** + * True only for the {@code main()} client. An embedder's JVM is not ours to exit, so a + * terminal setup failure stops the tunnel and reports itself for them to observe, while the + * command line client exits with a status a supervisor can act on. + */ + private volatile boolean commandLineClient; + /** + * The format the console log stream is using, for code outside the parse block. + * + *

Static because the fatal-error path in main() runs from a catch that encloses argument + * parsing, so the CommandLine may not exist by then -- but the formatter has already been + * installed and the stream already has a shape that must be respected. + */ + private static volatile String activeLogFormat = "text"; + /** The --web directory server, or null when --web was not given. */ + private LocalWebServer localWebServer; private PidPoller pidPoller; private TunnelPoller poller; private HttpForwarder httpForwarder; @@ -1056,7 +1117,8 @@ public void run() { } TunnelMetrics.setTunnelUp(false); try { - System.out.println("Shutting down your personal Tunnel Server."); + Logger.getLogger(App.class.getName()).log(Level.INFO, + "Shutting down your personal Tunnel Server."); api.destroyTunnel(); } catch (Exception ex) { Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); @@ -1106,8 +1168,12 @@ public void boot() throws Exception { TunnelMetrics.setTunnelInfo(App.VERSION, this.tunnelID, this.tunnelIdentifier); - if (Float.parseFloat(tunnelData.get("version").asText()) > App.VERSION) { - System.err.println("A new version (" + tunnelData.get("version").asText() + ") is available for download at https://testingbot.com\nYou have version " + App.VERSION); + // Both sides have to parse before anyone is told to upgrade. An unreadable local + // version used to compare as 0.0 and so nagged on every single startup, and an + // unexpected value from the API would have thrown out of boot() entirely. + Version latest = Version.parse(tunnelData.path("version").asText(null)); + if (latest != null && RELEASE != null && RELEASE.isOlderThan(latest)) { + System.err.println("A new version (" + latest + ") is available for download at https://testingbot.com\nYou have version " + App.VERSION); } Logger.getLogger(App.class.getName()).log(Level.INFO, "Please wait while your personal Tunnel Server is being setup. Shouldn't take more than a minute.\nWhen the tunnel is ready you will see a message \"You may start your tests.\""); @@ -1137,6 +1203,38 @@ public void trackPid() { pidPoller = new PidPoller(this); } + /** + * Gives up on a tunnel that cannot be set up, from a thread with nobody to throw to. + * + *

The poller and {@link #tunnelReady} both run on timer threads, so a failure there used + * to be logged and dropped: the scheduler stopped, but the metrics server kept answering and + * the process stayed alive forever, never ready and no longer trying. Neither a supervisor + * nor a container could tell that from a tunnel still coming up. + * + *

Everything is released either way. The command line client then exits with a status; + * an embedder is left a stopped App whose {@code /readyz} says what happened. + */ + public void setupFailed(String reason, int exitCode) { + Logger.getLogger(App.class.getName()).log(Level.SEVERE, reason); + try { + stop(); + } catch (Exception cleanupFailed) { + Logger.getLogger(App.class.getName()).log(Level.WARNING, + "Cleanup after a failed setup did not complete", cleanupFailed); + } + // After stop(), which sets it false itself -- but stop() is also the ordinary teardown, + // so being explicit here keeps the reason for the gauge attached to this path. + TunnelMetrics.setTunnelUp(false); + if (commandLineClient) { + // Already logged above; printing it again would duplicate it under text and break + // the stream under json. + if (!"json".equalsIgnoreCase(activeLogFormat)) { + System.err.println(reason); + } + System.exit(exitCode); + } + } + public void stop() { TunnelMetrics.setTunnelUp(false); @@ -1159,6 +1257,11 @@ public void stop() { stopInsightServer(); + if (localWebServer != null) { + localWebServer.stop(); + localWebServer = null; + } + if (poller != null) { poller.cancel(); } @@ -1168,6 +1271,19 @@ public void stop() { pidPoller = null; } + // The ready file says "this tunnel is forwarding", so it must not outlive the tunnel. + // The shutdown hook below removes it when the JVM exits, which covers the command line + // client but not an explicit stop(): an embedder running a tunnel per job left a stale + // file claiming the previous job's tunnel was ready, and the reconnect monitor's + // stop()/boot() rebuild left one across the window where nothing was forwarding. + if (readyFile != null) { + File f = new File(readyFile); + if (f.exists() && !f.delete()) { + Logger.getLogger(App.class.getName()).log(Level.WARNING, + "Could not delete ready file: {0}", readyFile); + } + } + // Without this, an embedder that starts a tunnel per job leaks one // shutdown hook per App instance for the lifetime of the JVM. if (cleanupThread != null) { @@ -1183,7 +1299,8 @@ public void stop() { // normal path for an embedder cleaning up in a finally block. if (api != null) { try { - System.out.println("Shutting down your personal Tunnel Server."); + Logger.getLogger(App.class.getName()).log(Level.INFO, + "Shutting down your personal Tunnel Server."); api.destroyTunnel(); } catch (Exception ex) { Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); @@ -1206,11 +1323,19 @@ public void tunnelReady(JsonNode apiResponse) { Logger.getLogger(App.class.getName()).log(Level.INFO, "Successfully authenticated, setting up forwarding."); tunnel.createPortForwarding(); boolean healthy = this.startProxies(); - TunnelMetrics.setTunnelUp(true); + // Gated on the self-test, not merely on having got this far. /readyz means "the + // tunnel is forwarding", and every check startProxies() runs is a check that + // traffic will actually arrive -- the Selenium relay reaching the hub, the + // reverse forward reaching the local proxy, the proxy reaching the internet. A + // tunnel that failed one of those carries nothing, so reporting it ready told + // container probes and --readyfile integrations to send work to something that + // could not do any. It stays not-ready until a reconnect succeeds. + TunnelMetrics.setTunnelUp(healthy); if (healthy) { + writeReadyFile(); Logger.getLogger(App.class.getName()).log(Level.INFO, "The Tunnel is ready, ip: {0}\nYou may start your tests.", _serverIP); } else { - Logger.getLogger(App.class.getName()).log(Level.SEVERE, "The Tunnel is up (ip: {0}) but its self-test failed; tests may not work until this is resolved.", _serverIP); + Logger.getLogger(App.class.getName()).log(Level.SEVERE, "The Tunnel is up (ip: {0}) but its self-test failed, so it is not reporting ready; tests will not work until this is resolved.", _serverIP); } Logger.getLogger(App.class.getName()).log(Level.INFO, "To stop the tunnel, press CTRL+C"); } @@ -1219,12 +1344,15 @@ public void tunnelReady(JsonNode apiResponse) { // client can exit and an embedder can handle it throw tunnelFailedException; } catch (Exception ex) { - Logger.getLogger(App.class.getName()).log(Level.INFO, "Something went wrong while setting up the Tunnel."); - Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); + // Not merely logged: this runs on a timer thread when it is reached from the poller, + // so returning here left a process that was alive, unready and no longer trying. + Logger.getLogger(App.class.getName()).log(Level.SEVERE, "Something went wrong while setting up the Tunnel.", ex); + setupFailed("Could not set up the tunnel: " + ex.getMessage(), 1); } } - private boolean startProxies() { + /** Package-private so ReadinessGatingTest can drive a failing startup. */ + boolean startProxies() { boolean healthy = true; httpForwarder = new HttpForwarder(this); @@ -1249,23 +1377,43 @@ private boolean startProxies() { } } - if (this.readyFile != null) { - File f = new File(this.readyFile); - if (f.exists()) { - f.setLastModified(System.currentTimeMillis()); - } else { - try (FileWriter fw = new FileWriter(f.getAbsoluteFile()); - BufferedWriter bw = new BufferedWriter(fw)) { - bw.write("TestingBot Tunnel Ready"); - } catch (IOException ex) { - Logger.getLogger(App.class.getName()).log(Level.SEVERE, "Could not create readyfile. Please make sure the directory exists and we have permission write to this directory." , ex); - } - } - } - return healthy; } + /** + * Touches {@code --readyfile}. + * + *

Only from the healthy path. It used to be written at the end of startProxies() + * regardless of what those checks found, so a tunnel whose forwarding test had just failed + * still announced itself ready to whatever was waiting on the file. + */ + /** Package-private, for the readiness tests; the CLI assigns the field directly. */ + void setNoProxy(boolean noProxy) { + this.noProxy = noProxy; + } + + /** Package-private, for the readiness tests; the CLI assigns the field directly. */ + void setReadyFile(String readyFile) { + this.readyFile = readyFile; + } + + void writeReadyFile() { + if (this.readyFile == null) { + return; + } + File f = new File(this.readyFile); + if (f.exists()) { + f.setLastModified(System.currentTimeMillis()); + return; + } + try (FileWriter fw = new FileWriter(f.getAbsoluteFile()); + BufferedWriter bw = new BufferedWriter(fw)) { + bw.write("TestingBot Tunnel Ready"); + } catch (IOException ex) { + Logger.getLogger(App.class.getName()).log(Level.SEVERE, "Could not create readyfile. Please make sure the directory exists and we have permission write to this directory." , ex); + } + } + /** * Evaluates a PAC file against one URL and prints the outcome. * @@ -1407,15 +1555,14 @@ static void applyUpstreamProxyOptions(App app, CommandLine commandLine) throws P } static int readinessPort(CommandLine commandLine) throws ParseException { - String value = commandLine.getOptionValue("metrics-port"); - if (value == null) { + if (commandLine.getOptionValue("metrics-port") == null) { return DEFAULT_METRICS_PORT; } - try { - return Integer.parseInt(value.trim()); - } catch (NumberFormatException notANumber) { - throw new ParseException("Invalid --metrics-port value: " + value); - } + // port(), like every other port option. This checked the syntax but not the range, so + // --ready --metrics-port 99999 got past it and died in ReadinessProbe with an uncaught + // IllegalArgumentException and a stack trace -- from the one command whose entire + // contract is to exit 0 or 1 for a container probe to read. + return port(commandLine, "metrics-port"); } public void doctor() { @@ -1551,11 +1698,46 @@ public void setPacLocalSha256(String pacLocalSha256) { /** Loaded once and shared; null when --pac-local was not given. */ public synchronized com.testingbot.tunnel.pac.PacPolicy getPacPolicy() { if (pacPolicy == null && pacLocal != null) { - pacPolicy = com.testingbot.tunnel.pac.PacPolicy.load(pacLocal, pacLocalSha256); + pacPolicy = com.testingbot.tunnel.pac.PacPolicy.load( + pacLocal, pacLocalSha256, pacFetchOptions()); } return pacPolicy; } + /** + * How to reach a remote {@code --pac-local} document. + * + *

The fetch used to ignore both {@code --proxy} and {@code --cacert-file}, so on a + * proxy-only network the URL was unreachable and on a TLS-intercepting network the + * handshake failed against a CA the JVM has never seen -- the exact network + * {@code --cacert-file} exists for. Either way the tunnel refused to start over a document + * it had been told how to reach. + */ + com.testingbot.tunnel.pac.PacPolicy.FetchOptions pacFetchOptions() { + java.net.Proxy proxy = null; + com.testingbot.tunnel.proxy.ProxySpec spec = + com.testingbot.tunnel.proxy.ProxySpec.parse(getProxy()); + if (spec != null) { + proxy = new java.net.Proxy( + spec.isSocks5() ? java.net.Proxy.Type.SOCKS : java.net.Proxy.Type.HTTP, + new java.net.InetSocketAddress(spec.getHost(), spec.getPort())); + } + javax.net.ssl.SSLSocketFactory sslSocketFactory = null; + if (caCertificates != null) { + try { + sslSocketFactory = caCertificates.sslContext().getSocketFactory(); + } catch (java.security.GeneralSecurityException ex) { + // Not fatal here: the fetch still runs against the platform trust store, and if + // that is not enough it fails with a certificate error naming the real problem. + Logger.getLogger(App.class.getName()).log(Level.WARNING, + "Could not apply --cacert-file to the PAC fetch", ex); + } + } + return proxy == null && sslSocketFactory == null + ? null + : new com.testingbot.tunnel.pac.PacPolicy.FetchOptions(proxy, sslSocketFactory); + } + public String getProxyAuthScheme() { return proxyAuthScheme; } @@ -1620,6 +1802,33 @@ public void setLogFormat(String logFormat) { this.logFormat = logFormat == null ? "text" : logFormat; } + /** + * Replaces the encoder on logback's console appender with the JSON one. + * + *

Reaches into the configured appenders rather than adding another: logback.xml's STDOUT + * appender is what Jetty, Apache HC and the SLF4J-using proxy handlers write through, and + * adding a second would duplicate every record rather than reformat it. + */ + static void jsonifyLogbackConsole(LoggerContext loggerContext) { + ch.qos.logback.classic.Logger root = + loggerContext.getLogger(ch.qos.logback.classic.Logger.ROOT_LOGGER_NAME); + for (java.util.Iterator> + it = root.iteratorForAppenders(); it.hasNext(); ) { + ch.qos.logback.core.Appender appender = it.next(); + if (appender instanceof ch.qos.logback.core.ConsoleAppender console) { + JsonLogbackEncoder json = new JsonLogbackEncoder(); + json.setContext(loggerContext); + json.start(); + @SuppressWarnings("unchecked") + ch.qos.logback.core.ConsoleAppender typed = + (ch.qos.logback.core.ConsoleAppender) console; + typed.stop(); + typed.setEncoder(json); + typed.start(); + } + } + } + /** The formatter for {@code --log-format}, shared by the console and the log file. */ static java.util.logging.Formatter logFormatterFor(String logFormat) { return "json".equalsIgnoreCase(logFormat) ? new JsonLogFormatter() : new LogFormatter(); diff --git a/src/main/java/com/testingbot/tunnel/Doctor.java b/src/main/java/com/testingbot/tunnel/Doctor.java index 384f488..202c510 100644 --- a/src/main/java/com/testingbot/tunnel/Doctor.java +++ b/src/main/java/com/testingbot/tunnel/Doctor.java @@ -38,6 +38,16 @@ public final class Doctor { private boolean hasFailures = false; public Doctor(App app) { + this(app, defaultEndpoints()); + } + + /** + * @param uris what to reach; injected so the tests can exercise the configuration checks + * without leaving the machine. The constructor runs the checks, so every test + * that built a Doctor made four real internet requests -- slow, and failing in + * a sandbox or on a plane for reasons that have nothing to do with the test. + */ + Doctor(App app, ArrayList uris) { this.app = app; if (app.getJettyPort() <= 0) { // Only when none was configured. Overwriting it meant --doctor --localproxy 9999 @@ -45,18 +55,21 @@ public Doctor(App app) { // configured port being taken, or privileged -- was never tested. app.setFreeJettyPort(); } + performChecks(uris); + } + + /** The endpoints a real {@code --doctor} run checks. */ + static ArrayList defaultEndpoints() { ArrayList uris = new ArrayList<>(); try { uris.add(new URI("https://testingbot.com")); uris.add(new URI("http://hub.testingbot.com")); uris.add(new URI("https://api.testingbot.com/v1/browsers")); uris.add(new URI("https://www.google.com/")); - } catch (URISyntaxException e) { - Logger.getLogger(Doctor.class.getName()).log(Level.SEVERE, e.getMessage()); - hasFailures = true; + } catch (URISyntaxException impossible) { + Logger.getLogger(Doctor.class.getName()).log(Level.SEVERE, impossible.getMessage()); } - - performChecks(uris); + return uris; } public boolean hasFailures() { @@ -201,28 +214,13 @@ private boolean checkConnection(final URI uri) { .setResponseTimeout(Timeout.of(3, TimeUnit.SECONDS)) .build(); - org.apache.hc.client5.http.impl.classic.HttpClientBuilder builder = HttpClients.custom(); - com.testingbot.tunnel.proxy.ProxySpec spec = - com.testingbot.tunnel.proxy.ProxySpec.parse(app.getControlProxy()); - if (spec != null && !spec.isSocks5()) { - builder.setProxy(new org.apache.hc.core5.http.HttpHost( - "http", spec.getHost(), spec.getPort())); - } - if (app.getCaCertificates() != null) { - try { - builder.setConnectionManager( - org.apache.hc.client5.http.impl.io - .PoolingHttpClientConnectionManagerBuilder.create() - .setTlsSocketStrategy( - new org.apache.hc.client5.http.ssl.DefaultClientTlsStrategy( - app.getCaCertificates().sslContext())) - .build()); - } catch (java.security.GeneralSecurityException unusable) { - Logger.getLogger(Doctor.class.getName()).log(Level.WARNING, - "Could not apply --cacert-file to the connectivity check: {0}", - unusable.getMessage()); - } - } + // The same builder Api uses, rather than a second one that has to be kept in step. This + // was a local reimplementation that had drifted: it skipped SOCKS5 entirely and never + // supplied credentials for an authenticated proxy, so on those networks --doctor tested + // a route the tunnel does not take -- reporting a failure the tunnel would not hit, or a + // success by a path it would not use. + org.apache.hc.client5.http.impl.classic.HttpClientBuilder builder = + Api.controlPlaneBuilder(app); try (CloseableHttpClient client = builder .setDefaultRequestConfig(cfg).build()) { diff --git a/src/main/java/com/testingbot/tunnel/LocalWebServer.java b/src/main/java/com/testingbot/tunnel/LocalWebServer.java index 53f0d68..5ccf163 100644 --- a/src/main/java/com/testingbot/tunnel/LocalWebServer.java +++ b/src/main/java/com/testingbot/tunnel/LocalWebServer.java @@ -12,13 +12,28 @@ public class LocalWebServer { static final int PORT = 8080; + /** + * Held, not constructor-local. + * + *

It was local, so nothing could ever stop this server: it served an operator-chosen + * directory, with listing enabled, for the life of the JVM, and outlived the tunnel it was + * started alongside. For an embedder that also meant a leaked Jetty server and a held port + * per App, and it left port 8080 bound so the next run could not start one. + */ + private final Server server; + public LocalWebServer(String directoryPath, String bindAddress) { - Server server = new Server(); + this(directoryPath, bindAddress, PORT); + } + + /** @param port for tests, which cannot assume 8080 is free */ + LocalWebServer(String directoryPath, String bindAddress, int port) { + server = new Server(); // new Server(port) binds the wildcard address, which published an operator-chosen // directory -- with listing enabled, below -- to every host that could route here. ServerConnector connector = new ServerConnector(server); connector.setHost(bindAddress); - connector.setPort(PORT); + connector.setPort(port); server.addConnector(connector); ResourceHandler resource_handler = new ResourceHandler(); @@ -32,9 +47,28 @@ public LocalWebServer(String directoryPath, String bindAddress) { try { server.start(); - Logger.getLogger(LocalWebServer.class.getName()).log(Level.INFO, "Local webserver now running on {0}:{1}", new Object[]{bindAddress, PORT}); + Logger.getLogger(LocalWebServer.class.getName()).log(Level.INFO, "Local webserver now running on {0}:{1}", new Object[]{bindAddress, port}); } catch (Exception ex) { Logger.getLogger(LocalWebServer.class.getName()).log(Level.SEVERE, null, ex); } } + + /** The port actually bound, which differs from {@link #PORT} only in tests. */ + int getPort() { + return server.getConnectors().length == 0 ? -1 + : ((ServerConnector) server.getConnectors()[0]).getLocalPort(); + } + + boolean isRunning() { + return server.isRunning(); + } + + public void stop() { + try { + server.stop(); + } catch (Exception ex) { + Logger.getLogger(LocalWebServer.class.getName()).log(Level.WARNING, + "Could not stop the local webserver", ex); + } + } } diff --git a/src/main/java/com/testingbot/tunnel/PidPoller.java b/src/main/java/com/testingbot/tunnel/PidPoller.java index ed77476..ab42b54 100644 --- a/src/main/java/com/testingbot/tunnel/PidPoller.java +++ b/src/main/java/com/testingbot/tunnel/PidPoller.java @@ -96,7 +96,14 @@ public void cancel() { Runtime.getRuntime().removeShutdownHook(cleanupThread); } catch (IllegalStateException alreadyShuttingDown) { // The JVM is on its way down and will run the hook itself. + scheduler.cancel(); + return; } + // The hook was this file's only deletion, so removing it and stopping there left the + // pid file behind for a process that no longer exists. The next run then finds a pid + // file naming a dead process, and an embedder that starts a tunnel per job + // accumulates one per job. + deleteQuietly(pidFile); } scheduler.cancel(); } diff --git a/src/main/java/com/testingbot/tunnel/ReadinessProbe.java b/src/main/java/com/testingbot/tunnel/ReadinessProbe.java index d139492..591c83c 100644 --- a/src/main/java/com/testingbot/tunnel/ReadinessProbe.java +++ b/src/main/java/com/testingbot/tunnel/ReadinessProbe.java @@ -42,6 +42,12 @@ public static int probe(String host, int port, int timeoutMs) { ? "Tunnel is not ready yet." : "Unexpected status " + status + " from /readyz on port " + port + "."); return 1; + } catch (IllegalArgumentException unusable) { + // A port outside 1-65535 makes URI.create throw, which is not an IOException. The + // CLI rejects that before reaching here, but this method is public and the whole + // point of it is to return an exit code rather than a stack trace. + System.err.println("Cannot probe port " + port + ": " + unusable.getMessage()); + return 1; } catch (IOException unreachable) { System.err.println("Could not reach the tunnel's metrics port " + port + " on " + host + ": " + unreachable.getMessage() diff --git a/src/main/java/com/testingbot/tunnel/TunnelMetrics.java b/src/main/java/com/testingbot/tunnel/TunnelMetrics.java index 4099415..b664206 100644 --- a/src/main/java/com/testingbot/tunnel/TunnelMetrics.java +++ b/src/main/java/com/testingbot/tunnel/TunnelMetrics.java @@ -226,11 +226,11 @@ public static void init() { * forever. A long session then reported several tunnels as simultaneously active, and the * label set grew without bound. */ - public static void setTunnelInfo(float version, int tunnelId, String identifier) { + public static void setTunnelInfo(String version, int tunnelId, String identifier) { // Only ever one active tunnel per process. TUNNEL_INFO.clear(); TUNNEL_INFO.labels( - Float.toString(version), + version == null ? "unknown" : version, Integer.toString(tunnelId), identifier == null ? "" : identifier ).set(1.0); diff --git a/src/main/java/com/testingbot/tunnel/Version.java b/src/main/java/com/testingbot/tunnel/Version.java new file mode 100644 index 0000000..2b3657f --- /dev/null +++ b/src/main/java/com/testingbot/tunnel/Version.java @@ -0,0 +1,138 @@ +package com.testingbot.tunnel; + +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; + +/** + * A dotted release version, compared component by component. + * + *

This existed as {@code Float} and the arithmetic was wrong in three separate ways, all of + * which only appear once the project has more than nine minor releases or more than two + * components: + * + *

    + *
  • {@code 5.10} parses to {@code 5.1f}, which sorts below {@code 5.9}. The upgrade + * notice would stop appearing at exactly the point there was something to upgrade to.
    • + *
  • {@code 5.0.1} is not a float at all. {@code Float.parseFloat} threw, the exception was + * caught, and the version became {@code 0.0} -- so every server version looked newer and + * the notice appeared on every single startup.
    • + *
  • Floats are binary, so equality between two versions that should match was never + * something to rely on.
    • + *
+ * + *

Comparison is numeric per component, left to right, with a missing component read as zero, + * so {@code 5.1} and {@code 5.1.0} are equal. A pre-release suffix ({@code 5.1.0-SNAPSHOT}) + * sorts below the same version without one, which is what semver says and what makes a local + * snapshot correctly see the matching release as newer. + */ +public final class Version implements Comparable { + + private final List components; + private final String preRelease; + private final String display; + + private Version(List components, String preRelease, String display) { + this.components = components; + this.preRelease = preRelease; + this.display = display; + } + + /** + * @param text something like {@code 5}, {@code 5.1}, {@code 5.10.2} or {@code 5.1-SNAPSHOT} + * @return the parsed version, or null when there is no leading number to read -- the callers + * here are a startup notice and a metrics label, and neither is worth failing a + * tunnel over. Refusing to guess is the point: the previous code turned anything it + * could not read into 0.0, which is not "unknown" but "older than everything". + */ + public static Version parse(String text) { + if (text == null) { + return null; + } + String trimmed = text.trim(); + if (trimmed.isEmpty()) { + return null; + } + + // Split the pre-release suffix off first, so the numeric scan below does not have to + // know about it: "5.1.0-SNAPSHOT" and "5.1.0-rc.1" both leave "5.1.0". + String numeric = trimmed; + String suffix = null; + int dash = trimmed.indexOf('-'); + if (dash >= 0) { + numeric = trimmed.substring(0, dash); + suffix = trimmed.substring(dash + 1); + } + + List parsed = new ArrayList<>(3); + for (String part : numeric.split("\\.", -1)) { + if (part.isEmpty()) { + continue; + } + try { + parsed.add(Integer.valueOf(Integer.parseInt(part))); + } catch (NumberFormatException notANumber) { + // Stop at the first component that is not a number rather than dropping it and + // carrying on: in "5.1.x.3" the 3 does not mean what its position would imply. + break; + } + } + if (parsed.isEmpty()) { + return null; + } + return new Version(List.copyOf(parsed), suffix, trimmed); + } + + @Override + public int compareTo(Version other) { + int width = Math.max(components.size(), other.components.size()); + for (int i = 0; i < width; i++) { + // A missing component is zero, so 5.1 == 5.1.0 rather than one preceding the other. + int mine = i < components.size() ? components.get(i) : 0; + int theirs = i < other.components.size() ? other.components.get(i) : 0; + if (mine != theirs) { + return Integer.compare(mine, theirs); + } + } + if (Objects.equals(preRelease, other.preRelease)) { + return 0; + } + // A pre-release precedes the release it leads to; two different pre-releases of the same + // version are ordered by name, which is right for rc.1 < rc.2 and arbitrary but stable + // otherwise. + if (preRelease == null) { + return 1; + } + if (other.preRelease == null) { + return -1; + } + return preRelease.compareTo(other.preRelease); + } + + /** True when {@code other} is a release this one should be upgraded to. */ + public boolean isOlderThan(Version other) { + return other != null && compareTo(other) < 0; + } + + @Override + public boolean equals(Object o) { + return o instanceof Version && compareTo((Version) o) == 0; + } + + @Override + public int hashCode() { + // Trailing zeros do not affect comparison, so they must not affect the hash either: + // 5.1 and 5.1.0 are equal and have to agree here. + List trimmed = new ArrayList<>(components); + while (trimmed.size() > 1 && trimmed.get(trimmed.size() - 1) == 0) { + trimmed.remove(trimmed.size() - 1); + } + return Objects.hash(trimmed, preRelease); + } + + /** The version as it was written, suffix and all -- this is what users and labels want. */ + @Override + public String toString() { + return display; + } +} diff --git a/src/main/java/com/testingbot/tunnel/pac/PacPolicy.java b/src/main/java/com/testingbot/tunnel/pac/PacPolicy.java index 9d2d53c..74e4848 100644 --- a/src/main/java/com/testingbot/tunnel/pac/PacPolicy.java +++ b/src/main/java/com/testingbot/tunnel/pac/PacPolicy.java @@ -72,12 +72,28 @@ private record CachedResult(PacResult result, long decidedAtMs) { this.clock = clock; } + /** + * How to reach a PAC document that lives behind the network's own egress rules. + * + *

The fetch used a bare {@link HttpURLConnection}, so it ignored both {@code --proxy} and + * {@code --cacert-file}. On a proxy-only network the PAC URL was simply unreachable, and on + * a TLS-intercepting network -- which is the entire reason {@code --cacert-file} exists -- + * an {@code https} PAC URL failed the handshake against a CA the JVM has never seen. Either + * way the tunnel refused to start over a document it had been told how to reach. + * + * @param proxy the upstream proxy to fetch through, or null for a direct fetch + * @param sslSocketFactory the factory carrying any extra CAs, or null for the platform's + */ + public record FetchOptions(java.net.Proxy proxy, + javax.net.ssl.SSLSocketFactory sslSocketFactory) { + } + /** * @param location a file path, or an https URL * @throws PacException if it cannot be read or does not parse */ public static PacPolicy load(String location) { - return load(location, null); + return load(location, null, null); } /** @@ -86,7 +102,15 @@ public static PacPolicy load(String location) { * @throws PacException if it cannot be read, fails its digest, or does not parse */ public static PacPolicy load(String location, String expectedSha256) { - String text = read(location, expectedSha256); + return load(location, expectedSha256, null); + } + + /** + * @param options how to reach a remote document, or null to fetch it directly + * @throws PacException if it cannot be read, fails its digest, or does not parse + */ + public static PacPolicy load(String location, String expectedSha256, FetchOptions options) { + String text = read(location, expectedSha256, options); try { return new PacPolicy(new PacInterpreter(text), location); } catch (PacException invalid) { @@ -101,7 +125,7 @@ public static PacPolicy of(String script, String description) { return new PacPolicy(new PacInterpreter(script), description); } - private static String read(String location, String expectedSha256) { + private static String read(String location, String expectedSha256, FetchOptions options) { // Before anything is fetched. A pin that cannot match is a configuration error, and // discovering it only after the document has been pulled over cleartext means the // request went out anyway -- to exactly the network this pin exists to distrust. @@ -123,7 +147,14 @@ private static String read(String location, String expectedSha256) { } HttpURLConnection connection = null; try { - connection = (HttpURLConnection) URI.create(location).toURL().openConnection(); + java.net.Proxy proxy = options == null ? null : options.proxy(); + connection = (HttpURLConnection) (proxy == null + ? URI.create(location).toURL().openConnection() + : URI.create(location).toURL().openConnection(proxy)); + if (options != null && options.sslSocketFactory() != null + && connection instanceof javax.net.ssl.HttpsURLConnection https) { + https.setSSLSocketFactory(options.sslSocketFactory()); + } connection.setConnectTimeout(FETCH_TIMEOUT_MS); connection.setReadTimeout(FETCH_TIMEOUT_MS); // Not followed: HttpURLConnection follows by default and will not carry an @@ -159,7 +190,19 @@ private static String read(String location, String expectedSha256) { } } try { - byte[] body = Files.readAllBytes(Path.of(location)); + // The same cap the remote path applies. readAllBytes() had none, so a path that + // was not the small script it was meant to be -- a log, a device, the wrong file + // entirely -- was read into memory in full before anything looked at it. The limit + // is about what this process will hold, and that does not depend on where the + // bytes came from. + byte[] body; + try (java.io.InputStream in = Files.newInputStream(Path.of(location))) { + body = in.readNBytes(MAX_PAC_BYTES + 1); + } + if (body.length > MAX_PAC_BYTES) { + throw new PacException("PAC file " + location + " is larger than " + + MAX_PAC_BYTES + " bytes; refusing to load it"); + } // A digest given for a local file is checked too. It is not defending against the // network here, but it is the operator saying "this exact document", and silently // ignoring that would be worse than refusing it. @@ -228,10 +271,30 @@ static String sha256Hex(byte[] body) { } /** - * @return where {@code host} should be reached, or {@link PacResult#direct()} if the file - * fails at runtime -- a broken PAC file must not make the tunnel unusable + * @return where {@code host} should be reached, or null when the file could not be + * evaluated -- see {@link #resolveOrNull} for why that is not DIRECT */ public PacResult resolve(String url, String host) { + PacResult result = resolveOrNull(url, host); + return result == null ? PacResult.direct() : result; + } + + /** + * As {@link #resolve}, but says when it does not know. + * + *

A failed evaluation used to become {@link PacResult#direct()}. That is not a neutral + * default: on a network whose only sanctioned egress is a proxy, it takes traffic the + * operator routed deliberately and sends it straight out instead -- and it did so past a + * configured {@code --proxy} as well, which no reading of "fallback" covers. The component + * whose entire job is deciding where traffic goes was failing open. + * + *

So the callers are told, and each falls back to the static {@code --proxy} when there + * is one. Going direct remains the answer only when nothing else was configured, because + * then there is genuinely nowhere else to send it. + * + * @return null when the file threw; a real result otherwise + */ + public PacResult resolveOrNull(String url, String host) { if (host == null) { return PacResult.direct(); } @@ -252,9 +315,24 @@ public PacResult resolve(String url, String host) { try { result = PacResult.parse(interpreter.findProxyForUrl(url, host)); } catch (RuntimeException failure) { - LOG.log(Level.WARNING, "PAC evaluation failed for {0} ({1}); going direct", + LOG.log(Level.WARNING, + "PAC evaluation failed for {0} ({1}); falling back to the configured proxy, " + + "or direct if there is none", new Object[]{host, failure.getMessage()}); - result = PacResult.direct(); + // Deliberately not cached. A failure is usually about this evaluation -- a + // dnsResolve that timed out, say -- and caching it would hold the fallback route in + // place for a minute after the condition passed. + return null; + } + if (result.getEntries().size() > 1) { + // Said once per destination rather than silently: the list is a failover list, and + // only the first entry is used. An operator who wrote "PROXY a; PROXY b" is + // entitled to know that b will never be tried rather than discovering it during an + // outage of a. + LOG.log(Level.INFO, + "PAC returned {0} directives for {1}; using {2}. Failover to the remaining " + + "entries is not implemented.", + new Object[]{result.getEntries().size(), host, result.first()}); } if (cache.size() >= MAX_CACHE_ENTRIES) { cache.clear(); diff --git a/src/main/java/com/testingbot/tunnel/proxy/CustomConnectHandler.java b/src/main/java/com/testingbot/tunnel/proxy/CustomConnectHandler.java index b703b49..8a3633a 100644 --- a/src/main/java/com/testingbot/tunnel/proxy/CustomConnectHandler.java +++ b/src/main/java/com/testingbot/tunnel/proxy/CustomConnectHandler.java @@ -201,7 +201,12 @@ private ProxySpec upstreamFor(String host, int port) { return proxySpec; } com.testingbot.tunnel.pac.PacResult result = - pacPolicy.resolve("https://" + host + ":" + port + "/", host); + pacPolicy.resolveOrNull("https://" + host + ":" + port + "/", host); + if (result == null) { + // The file could not be evaluated. Falling through to --proxy rather than going + // direct: on a proxy-only network, direct is not a safe default, it is a bypass. + return proxySpec; + } if (result.first().isDirect()) { return null; } @@ -380,8 +385,11 @@ protected void connectToServer(Request request, String host, int port, Promise= 200 && code < 300; - } catch (NumberFormatException ex) { - return false; - } + return HttpStatusLine.isSuccessfulConnect(statusLine); } } diff --git a/src/main/java/com/testingbot/tunnel/proxy/HttpStatusLine.java b/src/main/java/com/testingbot/tunnel/proxy/HttpStatusLine.java new file mode 100644 index 0000000..de40586 --- /dev/null +++ b/src/main/java/com/testingbot/tunnel/proxy/HttpStatusLine.java @@ -0,0 +1,61 @@ +package com.testingbot.tunnel.proxy; + +/** + * Reads the status code out of an HTTP status line. + * + *

Both hand-rolled relays here have to interpret a response they read off the wire themselves + * rather than through Jetty's parser: {@link CustomConnectHandler} reads the upstream proxy's + * answer to a CONNECT, and {@link WebsocketHandler} reads the target's answer to an upgrade. + * + *

They did it differently. The CONNECT path parsed the line properly; the WebSocket path + * asked whether the first line {@code contains("101")}, which accepts {@code HTTP/1.1 2101}, a + * {@code 500} whose reason phrase mentions 101, and anything else with those three digits + * anywhere in it -- and then treated the connection as an established WebSocket, handing the + * client a 101 of our own and splicing it to a target that had refused. One parser, used by + * both, so the two cannot drift again. + */ +final class HttpStatusLine { + + /** No status could be read. Distinct from any real code, so callers need no second check. */ + static final int INVALID = -1; + + private HttpStatusLine() { + } + + /** + * @param statusLine e.g. {@code HTTP/1.1 101 Switching Protocols} + * @return the three-digit status, or {@link #INVALID} if this is not a status line + */ + static int parse(String statusLine) { + if (statusLine == null) { + return INVALID; + } + String[] parts = statusLine.trim().split(" ", 3); + if (parts.length < 2 || !parts[0].startsWith("HTTP/")) { + return INVALID; + } + // Exactly three digits. Integer.parseInt alone would accept "2101" and "+200", and it is + // the over-long case that the substring check used to let through. + String code = parts[1]; + if (code.length() != 3) { + return INVALID; + } + for (int i = 0; i < 3; i++) { + if (code.charAt(i) < '0' || code.charAt(i) > '9') { + return INVALID; + } + } + return Integer.parseInt(code); + } + + /** True for a 2xx, which is what a proxy answering CONNECT must send. */ + static boolean isSuccessfulConnect(String statusLine) { + int code = parse(statusLine); + return code >= 200 && code < 300; + } + + /** True only for {@code 101 Switching Protocols}. */ + static boolean isSwitchingProtocols(String statusLine) { + return parse(statusLine) == 101; + } +} diff --git a/src/main/java/com/testingbot/tunnel/proxy/TunnelProxyHandler.java b/src/main/java/com/testingbot/tunnel/proxy/TunnelProxyHandler.java index 5102e00..266daa2 100644 --- a/src/main/java/com/testingbot/tunnel/proxy/TunnelProxyHandler.java +++ b/src/main/java/com/testingbot/tunnel/proxy/TunnelProxyHandler.java @@ -17,6 +17,7 @@ import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Level; import java.util.logging.Logger; @@ -27,6 +28,7 @@ import org.eclipse.jetty.client.BasicAuthentication; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.HttpProxy; +import org.eclipse.jetty.client.Origin; import org.eclipse.jetty.client.ProxyConfiguration; import org.eclipse.jetty.client.Socks5; import org.eclipse.jetty.client.Socks5Proxy; @@ -77,6 +79,8 @@ public class TunnelProxyHandler extends ProxyHandler.Forward { private String proxyAuthHeaderValue; /** Host of the upstream HTTP proxy, or null when there is not one. */ private String upstreamHttpProxyHost; + /** The same proxy in full, so a chosen upstream can be compared against it. */ + private ProxySpec upstreamHttpProxySpec; private String upstreamProxy; private String upstreamProxyAuth; private String[] basicAuth; @@ -86,6 +90,8 @@ public class TunnelProxyHandler extends ProxyHandler.Forward { private ConnectToMap connectTo = ConnectToMap.none(); private LocalhostPolicy localhostPolicy = LocalhostPolicy.ALLOW; private com.testingbot.tunnel.pac.PacPolicy pacPolicy; + /** One entry per proxy --pac-local has named so far; see registerPacProxy. */ + private final Map pacProxies = new ConcurrentHashMap<>(); private long idleTimeoutMs = 120_000L; private long connectTimeoutMs = -1; @@ -126,6 +132,7 @@ public void setUpstreamProxy(String hostPort, String userPassword) { ProxySpec spec = ProxySpec.parse(hostPort); boolean httpProxy = spec != null && !spec.isSocks5(); this.upstreamHttpProxyHost = httpProxy ? spec.getHost() : null; + this.upstreamHttpProxySpec = httpProxy ? spec : null; if (httpProxy && userPassword != null && !userPassword.isEmpty()) { this.proxyAuthHeaderValue = "Basic " + java.util.Base64.getEncoder() .encodeToString(userPassword.getBytes(java.nio.charset.StandardCharsets.UTF_8)); @@ -145,10 +152,18 @@ public void setUpstreamProxy(String hostPort, String userPassword) { * never fire and every request through a Negotiate proxy failed. The CONNECT and SSH paths * were already pre-emptive; this makes the plain-HTTP path agree with them. */ - private String upstreamAuthorization() { + private String upstreamAuthorization(org.eclipse.jetty.client.Request proxyToServerRequest) { if (upstreamHttpProxyHost == null) { return null; } + if (pacPolicy != null && !goesToTheConfiguredProxy(proxyToServerRequest)) { + // The credential names one specific proxy. Under a PAC file this request may be + // going to a different one, or -- for a DIRECT answer -- straight to the origin, + // where the customer's proxy password would land in an arbitrary internet host's + // access log. Withheld unless --proxy is genuinely the chosen upstream, which it + // still is when the file routes there and when a failed evaluation falls back to it. + return null; + } if (proxyAuthenticator.isNegotiate()) { return proxyAuthenticator.authorizationValue(upstreamHttpProxyHost); } @@ -170,6 +185,23 @@ boolean isTunnelledToOrigin(org.eclipse.jetty.client.Request proxyToServerReques : proxyToServerRequest.getURI().getScheme()); } + /** True when the upstream chosen for this request is the proxy {@code --proxy} names. */ + private boolean goesToTheConfiguredProxy(org.eclipse.jetty.client.Request proxyToServerRequest) { + if (upstreamHttpProxySpec == null) { + return false; + } + String host = proxyToServerRequest.getHost(); + int port = proxyToServerRequest.getPort(); + if (port <= 0) { + port = HttpScheme.HTTPS.is(proxyToServerRequest.getScheme()) ? 443 : 80; + } + ProxySpec chosen = upstreamFor(host, port, proxyToServerRequest.getScheme()); + return chosen != null + && !chosen.isSocks5() + && chosen.getPort() == upstreamHttpProxySpec.getPort() + && chosen.getHost().equalsIgnoreCase(upstreamHttpProxySpec.getHost()); + } + /** Splits "user:password"; the password may itself contain colons. */ static String[] splitCredentials(String userPassword) { if (userPassword == null || userPassword.isEmpty()) { @@ -244,6 +276,140 @@ public void setPacPolicy(com.testingbot.tunnel.pac.PacPolicy pacPolicy) { this.pacPolicy = pacPolicy; } + /** + * The upstream proxy for this destination, or null to go direct. + * + *

The same rule {@code CustomConnectHandler} follows: --pac-local wins over --proxy for + * every host, because a PAC file is per-destination by definition and a static --proxy + * cannot be. Only the first directive is used; honouring the failover list would need a + * retry loop around jetty-client's exchange. + */ + ProxySpec upstreamFor(String host, int port, String scheme) { + if (pacPolicy == null) { + return ProxySpec.parse(upstreamProxy); + } + if (host == null) { + return null; + } + com.testingbot.tunnel.pac.PacResult result = + pacPolicy.resolveOrNull(scheme + "://" + host + ":" + port + "/", host); + if (result == null) { + // Could not evaluate: fall through to --proxy rather than direct, so a broken file + // does not silently bypass the network's only sanctioned egress. + return ProxySpec.parse(upstreamProxy); + } + if (result.first().isDirect()) { + return null; + } + return ProxySpec.parse(result.first().toProxySpec()); + } + + /** + * True when {@code host:port} is an upstream proxy rather than a destination. + * + *

jetty-client's resolver is handed whichever endpoint it is about to dial, and that is + * the proxy whenever one is in play. Two things have to know the difference: --connect-to, + * which describes destinations and must not move the proxy, and --localhost-policy, since a + * proxy on this machine's loopback is an ordinary setup and what it goes on to reach is its + * decision, not ours. + */ + private boolean isUpstreamProxyEndpoint(String host, int port) { + if (host == null) { + return false; + } + if (pacPolicy == null) { + ProxySpec spec = ProxySpec.parse(upstreamProxy); + return spec != null && spec.getPort() == port && spec.getHost().equalsIgnoreCase(host); + } + // Under PAC the set is whatever the file has named so far, which registerPacProxy has + // already recorded by the time anything is dialled through it. + String http = "http://" + host + ":" + port; + String socks = "socks5://" + host + ":" + port; + return pacProxies.containsKey(http) || pacProxies.containsKey(socks); + } + + /** The identity of a proxy for the registry below; null means DIRECT. */ + private static String proxyKey(ProxySpec spec) { + return spec == null ? null + : (spec.isSocks5() ? "socks5://" : "http://") + spec.getHost() + ":" + spec.getPort(); + } + + /** + * Makes sure jetty-client knows about the proxy PAC chose for this destination. + * + *

jetty-client picks an upstream by walking {@link ProxyConfiguration}'s list and taking + * the first {@code Proxy} whose {@code matches(Origin)} answers true -- a fixed address per + * entry, which a PAC file is not. So each distinct proxy the file has named gets one entry + * whose {@code matches} re-asks the PAC and claims only the origins routed to itself; a host + * the file sends DIRECT is claimed by none of them and dialled directly. + * + *

Registration happens here, on the request thread before the exchange is created, rather + * than from inside {@code matches()}: {@code match()} iterates the list, and growing it + * during that walk is how this would become an intermittent failure under load. + * + * @return the proxy key for this destination, used as the request tag + */ + private String registerPacProxy(String host, int port, String scheme) { + if (pacPolicy == null) { + return null; + } + ProxySpec chosen = upstreamFor(host, port, scheme); + String key = proxyKey(chosen); + if (key == null) { + return null; + } + pacProxies.computeIfAbsent(key, k -> { + ProxyConfiguration.Proxy proxy = chosen.isSocks5() + ? new PacSocks5Proxy(chosen, k) + : new PacHttpProxy(chosen, k); + // Credentials are deliberately not attached: --proxy-userpwd names a specific proxy, + // and a PAC file can name any host at all. Handing the customer's proxy password to + // whatever a fetched document nominates is the thing that check exists to prevent. + // The request comes back 407 and this line says why. + if (upstreamProxyAuth != null && !upstreamProxyAuth.isEmpty()) { + LOG.log(Level.WARNING, + "PAC selected proxy {0}, which is not the --proxy the credentials belong " + + "to; sending none. Expect 407 if it requires authentication.", k); + } + LOG.log(Level.INFO, "PAC routing plain HTTP via {0}", k); + getHttpClient().getProxyConfiguration().addProxy(proxy); + return proxy; + }); + return key; + } + + /** An HTTP proxy that claims exactly the origins the PAC file routes to it. */ + private final class PacHttpProxy extends HttpProxy { + private final String key; + + PacHttpProxy(ProxySpec spec, String key) { + super(spec.getHost(), spec.getPort()); + this.key = key; + } + + @Override + public boolean matches(Origin origin) { + return key.equals(proxyKey(upstreamFor(origin.getAddress().getHost(), + origin.getAddress().getPort(), origin.getScheme()))); + } + } + + /** As {@link PacHttpProxy}, for a {@code SOCKS} directive. */ + private final class PacSocks5Proxy extends Socks5Proxy { + private final String key; + + PacSocks5Proxy(ProxySpec spec, String key) { + super(spec.getHost(), spec.getPort()); + this.key = key; + } + + @Override + public boolean matches(Origin origin) { + return key.equals(proxyKey(upstreamFor(origin.getAddress().getHost(), + origin.getAddress().getPort(), origin.getScheme()))); + } + } + public void setLocalhostPolicy(LocalhostPolicy localhostPolicy) { this.localhostPolicy = localhostPolicy == null ? LocalhostPolicy.ALLOW : localhostPolicy; } @@ -377,7 +543,14 @@ protected void configureHttpClient(HttpClient client) { @Override public void resolve(String host, int port, java.util.Map context, org.eclipse.jetty.util.Promise> promise) { - ConnectToMap.Target target = connectTo.remap(host, port); + // Only a destination is remapped. With an upstream proxy jetty-client + // resolves the *proxy's* address here, and --connect-to describes where a + // named destination lives -- so applying it to the proxy pointed the + // connection itself somewhere else and left the destination alone, which a + // wildcard rule turned into "every request goes to this one address". + boolean toProxy = isUpstreamProxyEndpoint(host, port); + ConnectToMap.Target target = + toProxy ? new ConnectToMap.Target(host, port) : connectTo.remap(host, port); if (dnsResolver == null) { // Wrap the promise rather than the resolver: the platform resolver is // what produces the addresses, so the check has to sit between it and @@ -387,7 +560,7 @@ public void resolve(String host, int port, java.util.Map context @Override public void succeeded(List addresses) { try { - promise.succeeded(refuseLoopback(target.host(), addresses)); + promise.succeeded(refuseLoopback(target.host(), addresses, toProxy)); } catch (Throwable denied) { promise.failed(denied); } @@ -406,7 +579,7 @@ public void failed(Throwable x) { for (InetAddress address : dnsResolver.resolve(target.host())) { resolved.add(new InetSocketAddress(address, target.port())); } - promise.succeeded(refuseLoopback(target.host(), resolved)); + promise.succeeded(refuseLoopback(target.host(), resolved, toProxy)); } catch (Throwable x) { promise.failed(x); } @@ -423,12 +596,13 @@ public void failed(Throwable x) { * @throws LocalhostPolicy.Denied if any resolved address is loopback */ private List refuseLoopback(String host, - List addresses) { - // With an upstream proxy jetty-client resolves the *proxy's* origin here, not - // the destination, and a proxy on this machine's loopback is an ordinary - // setup. Judging that address refused every request under + List addresses, + boolean toProxy) { + // Skipped for a proxy endpoint: jetty-client resolves the *proxy's* origin + // here, not the destination, and a proxy on this machine's loopback is an + // ordinary setup. Judging that address refused every request under // --localhost-policy deny; what the proxy then reaches is its decision. - if (addresses != null && upstreamProxy == null) { + if (addresses != null && !toProxy) { for (InetSocketAddress address : addresses) { if (localhostPolicy.blocksAddress(address.getAddress())) { LOG.log(Level.INFO, "Localhost policy: refusing dial to {0} ({1})", @@ -460,7 +634,18 @@ private org.eclipse.jetty.util.SocketAddressResolver platformResolver() { client.setResponseBufferSize(CLIENT_BUFFER_SIZE); ProxySpec spec = ProxySpec.parse(upstreamProxy); - if (upstreamProxy != null && !upstreamProxy.isEmpty() && spec == null) { + if (pacPolicy != null) { + // --pac-local decides per destination, so nothing static is registered here: a + // --proxy entry added to this list would match every origin and be picked ahead of + // the PAC entries, which is exactly how the file gets ignored. upstreamFor() already + // treats --proxy as unreachable once a PAC file is loaded, matching the CONNECT + // path, so this is the same decision expressed in jetty-client's terms. + if (spec != null) { + LOG.log(Level.INFO, + "--pac-local is set, so --proxy is not used for plain HTTP; the PAC file " + + "decides per destination."); + } + } else if (upstreamProxy != null && !upstreamProxy.isEmpty() && spec == null) { LOG.log(Level.WARNING, "Invalid proxy format ''{0}''; expected host:port, http://host:port or socks5://host:port", upstreamProxy); @@ -642,9 +827,20 @@ protected org.eclipse.jetty.client.Request newProxyToServerRequest(Request clien if (pathQuery == null || pathQuery.isEmpty()) { pathQuery = "/"; } + // Before the request is created, so the proxy PAC chose is already in the client's + // configuration when jetty-client resolves the destination and asks which one matches. + String pacTag = registerPacProxy(newHttpURI.getHost(), port, newHttpURI.getScheme()); + org.eclipse.jetty.client.Request proxyRequest = getHttpClient().newRequest(newHttpURI.getHost(), port) .scheme(newHttpURI.getScheme()) + // The upstream is part of this destination's identity. jetty-client + // resolves the proxy once per Origin and caches the destination, and + // Origin's equality includes the tag -- so without this a PAC answer + // that changes (the file routes by time of day, and decisions expire + // after a minute) would keep using the destination built from the + // previous answer for as long as it stayed in the pool. + .tag(pacTag) .path(pathQuery) // Deliberately no Request.timeout(): that is the TOTAL length of the // request/response conversation, not an idle timeout, so it aborts a @@ -726,7 +922,7 @@ protected void addProxyHeaders(Request clientToProxyRequest, // and also with no attacker at all where a bumping upstream Squid re-issues requests // in that shape. Mirrors jetty-client's own HttpProxy.requiresTunnel test. if (!isTunnelledToOrigin(proxyToServerRequest)) { - String upstreamAuthorization = upstreamAuthorization(); + String upstreamAuthorization = upstreamAuthorization(proxyToServerRequest); if (upstreamAuthorization != null) { fields.put(HttpHeader.PROXY_AUTHORIZATION, upstreamAuthorization); } diff --git a/src/main/java/com/testingbot/tunnel/proxy/WebsocketHandler.java b/src/main/java/com/testingbot/tunnel/proxy/WebsocketHandler.java index c47052b..dbaebe1 100644 --- a/src/main/java/com/testingbot/tunnel/proxy/WebsocketHandler.java +++ b/src/main/java/com/testingbot/tunnel/proxy/WebsocketHandler.java @@ -120,7 +120,12 @@ ProxySpec upstreamFor(String host, int port) { } // ws:// is carried over HTTP, so that is the scheme a PAC file is asked about. com.testingbot.tunnel.pac.PacResult result = - pacPolicy.resolve("http://" + host + ":" + port + "/", host); + pacPolicy.resolveOrNull("http://" + host + ":" + port + "/", host); + if (result == null) { + // Could not evaluate: fall through to --proxy rather than direct, so a broken file + // does not silently bypass the network's only sanctioned egress. + return proxySpec; + } if (result.first().isDirect()) { return null; } @@ -350,11 +355,14 @@ protected void connectToServer(Request request, String host, int port, Promise 0 ? lines[0] : "empty response"; // Naming the proxy matters here: in get mode the answer usually comes from // the proxy declining to forward the upgrade, not from the target refusing diff --git a/src/main/java/ssh/CustomConnectionMonitor.java b/src/main/java/ssh/CustomConnectionMonitor.java index 4394f82..b0caee5 100644 --- a/src/main/java/ssh/CustomConnectionMonitor.java +++ b/src/main/java/ssh/CustomConnectionMonitor.java @@ -69,8 +69,11 @@ public void connectionLost(Throwable reason) { } private void scheduleRetry() { - scheduler.scheduleOnce("Reconnect-" + tunnel.getConnectionId(), this::attemptReconnect, - retryDelayMs); + scheduleRetry(this::attemptReconnect); + } + + private void scheduleRetry(Runnable task) { + scheduler.scheduleOnce("Reconnect-" + tunnel.getConnectionId(), task, retryDelayMs); } /** One reconnect attempt. Package-private so a test can drive it without the scheduler. */ @@ -121,11 +124,26 @@ private void onReconnected() { LOG.log(Level.SEVERE, String.format( "[%s] SSH reconnected, but the local proxy could not be restarted: %s", tunnel.getConnectionId(), proxyFailed.getMessage()), proxyFailed); - // The SSH session stays up; retry just the proxy rather than re-dialling it. - scheduleRetry(); + // Retry just the proxy. This used to call scheduleRetry(), whose task is + // attemptReconnect() -- which begins tunnel.stop(); tunnel.connect(). So the branch + // that exists to avoid re-dialling a healthy SSH session scheduled exactly that, + // every five seconds, for as long as the port stayed bound. + if (retryAttempts >= MAX_RETRIES) { + // Checked here too. Returning from attemptReconnect() through onReconnected() + // skips its own limit check, so a port that never frees up retried forever + // rather than falling through to the rebuild that would have released it. + giveUpAndRebuild(); + return; + } + scheduleRetry(this::retryLocalProxy); return; } + finishReconnect(); + } + + /** The part common to a clean reconnect and one that needed the proxy retried. */ + private void finishReconnect() { retrying.set(false); scheduler.cancel(); @@ -138,6 +156,32 @@ private void onReconnected() { retryAttempts = 0; } + /** + * Restarts the local proxy without touching the SSH session. + * + *

Reached when SSH is up and authenticated but the proxy could not bind -- typically its + * port has not been released yet. Everything else about the tunnel is healthy, so the + * expensive part is not repeated; only when the port never frees up does this fall through + * to the rebuild, which stops the whole App and so releases it. + */ + private void retryLocalProxy() { + retryAttempts += 1; + try { + host.startLocalProxy(); + } catch (RuntimeException proxyFailed) { + LOG.log(Level.WARNING, String.format( + "[%s] Local proxy restart attempt %d failed: %s", + tunnel.getConnectionId(), retryAttempts, proxyFailed.getMessage())); + if (retryAttempts >= MAX_RETRIES) { + giveUpAndRebuild(); + } else { + scheduleRetry(this::retryLocalProxy); + } + return; + } + finishReconnect(); + } + private void giveUpAndRebuild() { LOG.log(Level.WARNING, String.format( "[%s] Giving up retrying after %d attempts. Creating a new Tunnel Connection.", diff --git a/src/main/java/ssh/SSHTunnel.java b/src/main/java/ssh/SSHTunnel.java index 4a2e98b..8af7bce 100644 --- a/src/main/java/ssh/SSHTunnel.java +++ b/src/main/java/ssh/SSHTunnel.java @@ -339,22 +339,52 @@ public void run() { /** * Is the local forward for {@code sshPort} still in JSch's list? * - *

Extracted so it can be tested: the entries are free-form strings like - * {@code "4446:hub.testingbot.com:80"}, and a substring match on the port is looser than it - * looks -- 445 matches 4456 -- so the exact behaviour is worth pinning rather than assuming. + *

The entries look like {@code "4446:hub.testingbot.com:80"}, or with a bind address + * {@code "127.0.0.1:4446:hub.testingbot.com:80"}. The local port is what identifies the + * forward, so that is what is compared. + * + *

This was {@code contains(String.valueOf(sshPort))} on the whole entry, and a previous + * test documented the false positive it produces -- 445 matching 4456 -- as harmless. It is + * not: this answers "is my forward still there", and the monitor repairs it when the answer + * is no. A match against some other forward's port, or against a digit sequence in the + * destination host or the remote port, reports a forward that is gone as present, so the + * repair never runs and every request through the tunnel keeps failing. */ static boolean localForwardingActive(String[] forwardedPorts, int sshPort) { if (forwardedPorts == null) { return false; } - for (String port : forwardedPorts) { - if (port != null && port.contains(String.valueOf(sshPort))) { + for (String entry : forwardedPorts) { + if (entry != null && localPortOf(entry) == sshPort) { return true; } } return false; } + /** + * The local port from a JSch forwarding entry, or -1 when it cannot be read. + * + *

The port is the field before the destination host: first for {@code lport:host:rport}, + * second when a bind address is present. Distinguished by which one parses as a number -- + * a bind address never does, and a port always does. + */ + private static int localPortOf(String entry) { + String[] fields = entry.split(":"); + if (fields.length < 3) { + return -1; + } + // fields[0] is either the local port or a bind address. + for (int i = 0; i < 2 && i < fields.length; i++) { + try { + return Integer.parseInt(fields[i].trim()); + } catch (NumberFormatException notAPort) { + // a bind address; try the next field + } + } + return -1; + } + /** * Tracks reverse-forward health so each change is logged once rather than every 15 seconds. * diff --git a/src/main/java/ssh/TunnelPoller.java b/src/main/java/ssh/TunnelPoller.java index a88a474..702aa61 100644 --- a/src/main/java/ssh/TunnelPoller.java +++ b/src/main/java/ssh/TunnelPoller.java @@ -42,8 +42,32 @@ public void cancel() { scheduler.cancel(); } + /** + * Consecutive failed polls tolerated before the tunnel is given up on. + * + *

Not one. A poll is an HTTPS request to the API, and a single failed one means very + * little -- a DNS hiccup or a dropped connection while the tunnel server is still booting. + * Cancelling on the first exception turned any such blip into a process that stayed alive + * and never became ready. Five consecutive failures spans about 25 seconds, by which point + * it is not a blip. + */ + static final int MAX_CONSECUTIVE_ERRORS = 5; + + /** + * Stops polling and puts the app into its terminal failure state. + * + *

Cancelling the scheduler alone was the bug: it stopped the retries but told nothing + * else, so the process stayed up with a metrics server answering and a tunnel that would + * never be ready. + */ + private void giveUp(String reason) { + scheduler.cancel(); + app.setupFailed(reason, 1); + } + class PollTask implements Runnable { int counter = 0; + int consecutiveErrors = 0; @Override public void run() { @@ -53,11 +77,14 @@ public void run() { response = api.pollTunnel(tunnelID); if (this.counter > 80) { - Logger.getLogger(TunnelPoller.class.getName()).log(Level.SEVERE, "Unable to create tunnel, waited for 400 seconds. Please try again or check https://status.testingbot.com"); - scheduler.cancel(); + giveUp("Unable to create tunnel, waited for 400 seconds. Please try again or check https://status.testingbot.com"); return; } + // Reset only after a poll that actually returned: the count is of consecutive + // failures, and a run of them broken by one success is not the same thing. + this.consecutiveErrors = 0; + if (response.get("state").asText().equals("READY")) { scheduler.cancel(); app.tunnelReady(response); @@ -66,13 +93,22 @@ public void run() { Logger.getLogger(TunnelPoller.class.getName()).log(Level.INFO, "Current tunnel status: {0}", response.get("state").asText()); } } catch (TunnelFailedException tunnelFailedException) { - // the tunnel became ready but could not be set up; this runs on a - // timer thread so there is nobody to propagate to, report it here - scheduler.cancel(); - Logger.getLogger(TunnelPoller.class.getName()).log(Level.SEVERE, tunnelFailedException.getMessage()); + // The tunnel became ready but could not be set up. Not retryable -- the failure + // is in our own setup, not in the poll -- and this runs on a timer thread with + // nobody to propagate to, so it ends the tunnel here. + giveUp(tunnelFailedException.getMessage()); } catch (Exception ex) { - scheduler.cancel(); - Logger.getLogger(TunnelPoller.class.getName()).log(Level.SEVERE, "Unable to poll for tunnel status."); + // A failed poll is not a failed tunnel. The scheduler keeps running so the next + // tick retries, and only a sustained run of failures gives up. + this.consecutiveErrors += 1; + Logger.getLogger(TunnelPoller.class.getName()).log(Level.WARNING, + "Unable to poll for tunnel status ({0}/{1}): {2}", + new Object[]{this.consecutiveErrors, MAX_CONSECUTIVE_ERRORS, + ex.getMessage()}); + if (this.consecutiveErrors >= MAX_CONSECUTIVE_ERRORS) { + giveUp("Unable to poll for tunnel status after " + + MAX_CONSECUTIVE_ERRORS + " consecutive attempts: " + ex.getMessage()); + } } } } diff --git a/src/test/java/com/testingbot/tunnel/ApiTest.java b/src/test/java/com/testingbot/tunnel/ApiTest.java index e1b11f8..39dd579 100644 --- a/src/test/java/com/testingbot/tunnel/ApiTest.java +++ b/src/test/java/com/testingbot/tunnel/ApiTest.java @@ -218,12 +218,65 @@ void createTunnel_withServerError_shouldThrowException() throws Exception { api = createApiWithMockServer(); - // When/Then: Should throw exception (JSON parsing fails on non-JSON response) + // Now fails on the status. It used to fail only because the body was not JSON, which is + // why the case below went unnoticed: this test looked like coverage of "the API + // returned 500" and was really coverage of "the API returned something unparseable". assertThatThrownBy(() -> api.createTunnel()) .isInstanceOf(Exception.class) .hasMessageContaining("Could not start tunnel"); } + @Test + void createTunnel_withServerErrorCarryingValidJson_shouldThrowException() throws Exception { + // The gap. _post parsed the body without ever looking at the status, so a 500 whose + // body happened to be well-formed JSON was handed back as tunnel data. boot() then read + // fields off it -- state, id, ip -- and an API outage surfaced as a missing-field error + // somewhere further along, or as a tunnel that never came up for no stated reason. + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse() + .withStatus(500) + .withHeader("Content-Type", "application/json") + .withBody("{\"message\":\"failure\"}"))); + + api = createApiWithMockServer(); + + assertThatThrownBy(() -> api.createTunnel()) + .isInstanceOf(Exception.class) + .hasMessageContaining("500"); + } + + @Test + void createTunnel_withServerError_shouldReportWhatTheApiSaid() throws Exception { + // The body is read on the failure path too: the API states why it refused, and that + // reason is the entire value of the message to whoever has to act on it. + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse() + .withStatus(403) + .withHeader("Content-Type", "application/json") + .withBody("{\"error\":\"concurrent tunnel limit reached\"}"))); + + api = createApiWithMockServer(); + + assertThatThrownBy(() -> api.createTunnel()) + .hasMessageContaining("concurrent tunnel limit reached"); + } + + @Test + void createTunnel_with401_shouldThrowRatherThanReturnTheBody() throws Exception { + // Bad credentials are the most common real failure and the one most likely to return a + // tidy JSON envelope. + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse() + .withStatus(401) + .withHeader("Content-Type", "application/json") + .withBody("{\"error\":\"unauthorized\"}"))); + + api = createApiWithMockServer(); + + assertThatThrownBy(() -> api.createTunnel()) + .hasMessageContaining("401"); + } + @Test void pollTunnel_shouldReturnTunnelStatus() throws Exception { // Given: Mock server configured to respond diff --git a/src/test/java/com/testingbot/tunnel/DoctorEgressTest.java b/src/test/java/com/testingbot/tunnel/DoctorEgressTest.java new file mode 100644 index 0000000..116944a --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/DoctorEgressTest.java @@ -0,0 +1,112 @@ +package com.testingbot.tunnel; + +import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.core.WireMockConfiguration; + +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; +import org.apache.hc.client5.http.classic.methods.HttpHead; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.Base64; +import java.nio.charset.StandardCharsets; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * {@code --doctor} has to test the route the tunnel actually takes. + * + *

It built its own HTTP client, and that client had drifted from the one {@link Api} uses: it + * skipped SOCKS5 entirely, and it never supplied credentials for an authenticated proxy. So on + * exactly the networks {@code --doctor} exists to diagnose it tested something else -- reporting + * "can not be reached" for a tunnel that would have started, or reaching the API by a path the + * tunnel would not have used and calling that a pass. + * + *

Asserted through the shared builder rather than by running the whole doctor, because what + * changed is which client is built; running the checks would need the real TestingBot endpoints. + */ +class DoctorEgressTest { + + private WireMockServer proxy; + + @BeforeEach + void setUp() { + proxy = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + proxy.start(); + } + + @AfterEach + void tearDown() { + if (proxy != null && proxy.isRunning()) { + proxy.stop(); + } + } + + private App appWithProxy(String spec, String auth) { + App app = new App(); + app.setClientKey("test_key"); + app.setClientSecret("test_secret"); + app.setProxy(spec); + if (auth != null) { + app.setProxyAuth(auth); + } + return app; + } + + @Test + void theConnectivityCheckGoesThroughAnAuthenticatedProxy() throws Exception { + String expected = "Basic " + Base64.getEncoder() + .encodeToString("user:sesame".getBytes(StandardCharsets.UTF_8)); + proxy.stubFor(any(urlMatching(".*")) + .withHeader("Proxy-Authorization", equalTo(expected)) + .willReturn(aResponse().withStatus(200))); + proxy.stubFor(any(urlMatching(".*")) + .withHeader("Proxy-Authorization", absent()) + .willReturn(aResponse().withStatus(407) + .withHeader("Proxy-Authenticate", "Basic realm=\"t\""))); + + App app = appWithProxy("127.0.0.1:" + proxy.port(), "user:sesame"); + + HttpClientBuilder builder = Api.controlPlaneBuilder(app); + try (CloseableHttpClient client = builder.build()) { + int status = client.execute(new HttpHead("http://example.com/"), + response -> response.getCode()); + + // Without credentials this is a 407, which checkConnection counts as unreachable -- + // so --doctor failed on a network where the tunnel works. + assertThat(status) + .as("--doctor must authenticate to the proxy the way the tunnel does") + .isEqualTo(200); + } + } + + @Test + void aSocks5ProxyIsActuallyUsedRatherThanIgnored() throws Exception { + // The old code tested `spec != null && !spec.isSocks5()`, so a SOCKS5 --proxy produced a + // plain direct client. Proven here by pointing SOCKS at a dead port while the target is + // perfectly reachable directly: if the request succeeds, the proxy was ignored and the + // check bypassed the only egress the tunnel has. + proxy.stubFor(any(urlMatching(".*")).willReturn(aResponse().withStatus(200))); + int deadPort; + try (java.net.ServerSocket socket = new java.net.ServerSocket(0)) { + deadPort = socket.getLocalPort(); + } + App app = appWithProxy("socks5://127.0.0.1:" + deadPort, null); + + try (CloseableHttpClient client = Api.controlPlaneBuilder(app).build()) { + assertThatThrownBy(() -> client.execute( + new HttpHead("http://127.0.0.1:" + proxy.port() + "/"), + response -> response.getCode())) + .as("a SOCKS5 --proxy must be dialled; going direct would have succeeded") + .isInstanceOf(java.io.IOException.class); + } + + assertThat(proxy.getAllServeEvents()) + .as("nothing should have reached the target directly") + .isEmpty(); + } +} diff --git a/src/test/java/com/testingbot/tunnel/DoctorScopeTest.java b/src/test/java/com/testingbot/tunnel/DoctorScopeTest.java index cd2a465..bbafa60 100644 --- a/src/test/java/com/testingbot/tunnel/DoctorScopeTest.java +++ b/src/test/java/com/testingbot/tunnel/DoctorScopeTest.java @@ -17,6 +17,18 @@ */ class DoctorScopeTest { + /** + * No endpoints, so these tests do not leave the machine. + * + *

Doctor's constructor runs the checks, so every test here used to make four real + * internet requests to assert something about port selection or Kerberos scope -- slow, and + * failing in a sandbox for reasons unrelated to what is being tested. The connectivity path + * itself is covered by DoctorEgressTest against a local server. + */ + private static java.util.ArrayList noEndpoints() { + return new java.util.ArrayList<>(); + } + private static App configured() { App app = new App(); app.setClientKey("test_key"); @@ -30,7 +42,7 @@ void aConfiguredLocalproxyPortIsTheOneChecked() { app.setJettyPort(9999); // Doctor's constructor runs the checks; only the port selection matters here. - new Doctor(app); + new Doctor(app, noEndpoints()); assertThat(app.getJettyPort()) .as("overwriting it meant --doctor reported on a port the user never chose") @@ -41,7 +53,7 @@ void aConfiguredLocalproxyPortIsTheOneChecked() { void withoutOneAFreePortIsStillChosen() { App app = configured(); - new Doctor(app); + new Doctor(app, noEndpoints()); assertThat(app.getJettyPort()).isGreaterThan(0); } diff --git a/src/test/java/com/testingbot/tunnel/GrafanaDashboardsTest.java b/src/test/java/com/testingbot/tunnel/GrafanaDashboardsTest.java new file mode 100644 index 0000000..da908c0 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/GrafanaDashboardsTest.java @@ -0,0 +1,77 @@ +package com.testingbot.tunnel; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; + +import org.junit.jupiter.api.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The two shipped Grafana dashboards have to stay the same dashboard. + * + *

They are the same document in two places -- one for a user importing it by hand, one + * provisioned by the docker-compose example -- and they had silently drifted: the provisioned + * copy was five panels behind, missing everything added for connection, dial and proxy-error + * observability. Someone running the compose example to look at those metrics would have found + * a dashboard that simply did not show them, with nothing to indicate it was stale. + * + *

Panels are compared by title rather than byte-for-byte on the whole file, so the failure + * message names what is missing instead of saying the files differ. + */ +class GrafanaDashboardsTest { + + private static final Path CANONICAL = + Path.of("examples/grafana-dashboard/testingbot-tunnel.json"); + private static final Path PROVISIONED = Path.of( + "examples/docker-compose-prometheus-grafana/grafana/provisioning/dashboards", + "testingbot_tunnel.json"); + + private static JsonNode read(Path path) throws Exception { + assertThat(Files.exists(path)).as("%s should exist", path).isTrue(); + return new ObjectMapper().readTree(Files.readString(path)); + } + + private static List panelTitles(JsonNode dashboard) { + List titles = new ArrayList<>(); + for (JsonNode panel : dashboard.path("panels")) { + titles.add(panel.path("title").asText()); + } + return titles; + } + + @Test + void bothDashboardsShowTheSamePanels() throws Exception { + List canonical = panelTitles(read(CANONICAL)); + List provisioned = panelTitles(read(PROVISIONED)); + + assertThat(canonical).as("the canonical dashboard should have panels").isNotEmpty(); + assertThat(provisioned) + .as("the docker-compose dashboard must not fall behind the canonical one") + .containsExactlyElementsOf(canonical); + } + + @Test + void bothDashboardsAreOtherwiseIdentical() throws Exception { + // Beyond the panels: the datasource, uid, refresh interval and templating all decide + // whether the thing works when provisioned. + assertThat(read(PROVISIONED)) + .as("the two files are one document; keep them in step") + .isEqualTo(read(CANONICAL)); + } + + @Test + void everyPanelQueriesAMetricThisProcessExports() throws Exception { + // A panel naming a metric that no longer exists renders empty, which reads as "the + // tunnel is idle" rather than "this panel is wrong". + String json = Files.readString(CANONICAL); + assertThat(json) + .as("the dashboards are built around the testingbot_ metric namespace") + .contains("testingbot_"); + } +} diff --git a/src/test/java/com/testingbot/tunnel/HealthEndpointsTest.java b/src/test/java/com/testingbot/tunnel/HealthEndpointsTest.java index 59a06a0..b5de147 100644 --- a/src/test/java/com/testingbot/tunnel/HealthEndpointsTest.java +++ b/src/test/java/com/testingbot/tunnel/HealthEndpointsTest.java @@ -30,6 +30,8 @@ private static int findFreePort() throws IOException { return TestPorts.free(); } + private InsightServer insightServer; + @BeforeEach void setUp() throws Exception { TunnelMetrics.setTunnelUp(false); @@ -38,13 +40,20 @@ void setUp() throws Exception { app.setClientKey("test_key"); app.setClientSecret("test_secret"); app.setMetricsPort(metricsPort); - new InsightServer(app); + insightServer = new InsightServer(app); waitForPort(metricsPort); } @AfterEach void tearDown() { TunnelMetrics.setTunnelUp(false); + // The server was constructed and dropped, so every test in this class left a Jetty + // server and a bound port behind for the rest of the JVM's life. Surefire forks per + // class here, which is the only reason it did not accumulate across the whole suite. + if (insightServer != null) { + insightServer.stop(); + insightServer = null; + } } private static void waitForPort(int port) throws Exception { diff --git a/src/test/java/com/testingbot/tunnel/JsonLogFormatWiringTest.java b/src/test/java/com/testingbot/tunnel/JsonLogFormatWiringTest.java new file mode 100644 index 0000000..e28829c --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/JsonLogFormatWiringTest.java @@ -0,0 +1,138 @@ +package com.testingbot.tunnel; + +import ch.qos.logback.classic.LoggerContext; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.Appender; +import ch.qos.logback.core.ConsoleAppender; +import ch.qos.logback.core.encoder.Encoder; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; + +import java.util.ArrayList; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * {@code --log-format json} has to make the whole console stream JSON. + * + *

This process logs through two stacks: JUL for its own classes, and SLF4J/logback for Jetty, + * Apache HC and the proxy handlers. Only the JUL side was reformatted, and {@code logback.xml} + * pins its console appender to a text pattern -- so the option produced a stream that was JSON + * for some records and text for others. That is not a format at all, and it is worse for the + * collector this option exists to serve than plain text would have been, because it parses most + * of the way and then fails. + */ +class JsonLogFormatWiringTest { + + private final List restore = new ArrayList<>(); + + @AfterEach + void tearDown() { + restore.forEach(Runnable::run); + } + + private static LoggerContext context() { + return (LoggerContext) LoggerFactory.getILoggerFactory(); + } + + private static List> rootAppenders(LoggerContext context) { + List> appenders = new ArrayList<>(); + context.getLogger(ch.qos.logback.classic.Logger.ROOT_LOGGER_NAME) + .iteratorForAppenders().forEachRemaining(appenders::add); + return appenders; + } + + @Test + void theLogbackConsoleAppenderGetsTheJsonEncoder() { + LoggerContext context = context(); + List> before = rootAppenders(context); + ConsoleAppender console = before.stream() + .filter(a -> a instanceof ConsoleAppender) + .map(a -> { + @SuppressWarnings("unchecked") + ConsoleAppender typed = (ConsoleAppender) a; + return typed; + }) + .findFirst() + .orElseThrow(() -> new AssertionError( + "logback.xml is expected to configure a console appender")); + + Encoder original = console.getEncoder(); + restore.add(() -> { + console.stop(); + console.setEncoder(original); + console.start(); + }); + + assertThat(original) + .as("the default is the text pattern, which is the whole problem") + .isNotInstanceOf(JsonLogbackEncoder.class); + + App.jsonifyLogbackConsole(context); + + assertThat(console.getEncoder()) + .as("Jetty, Apache HC and the proxy handlers write through this appender") + .isInstanceOf(JsonLogbackEncoder.class); + assertThat(console.isStarted()) + .as("an appender left stopped would drop every record it carries") + .isTrue(); + } + + @Test + void theAppenderIsReplacedInPlaceRatherThanAdded() { + LoggerContext context = context(); + int before = rootAppenders(context).size(); + ConsoleAppender console = rootAppenders(context).stream() + .filter(a -> a instanceof ConsoleAppender) + .map(a -> { + @SuppressWarnings("unchecked") + ConsoleAppender typed = (ConsoleAppender) a; + return typed; + }) + .findFirst() + .orElseThrow(); + Encoder original = console.getEncoder(); + restore.add(() -> { + console.stop(); + console.setEncoder(original); + console.start(); + }); + + App.jsonifyLogbackConsole(context); + + // Adding a second appender would have been the easy implementation and would emit every + // record twice -- once as JSON and once as text, which is the original defect plus + // duplication. + assertThat(rootAppenders(context)).hasSize(before); + } + + @Test + void theEncoderProducesOneParseableObjectPerRecord() throws Exception { + // The encoder is what the whole option rests on, so its output is checked as JSON + // rather than by eye. + JsonLogbackEncoder encoder = new JsonLogbackEncoder(); + encoder.setContext(context()); + encoder.start(); + + ch.qos.logback.classic.Logger logger = context().getLogger("test.logger"); + ch.qos.logback.classic.spi.LoggingEvent event = new ch.qos.logback.classic.spi.LoggingEvent(); + event.setLoggerName("test.logger"); + event.setLevel(ch.qos.logback.classic.Level.WARN); + // Quotes, backslashes and a newline: the characters a hand-rolled escape loses on. + event.setMessage("a \"quoted\" \\ message\nwith a newline"); + event.setTimeStamp(System.currentTimeMillis()); + + String line = new String(encoder.encode(event), java.nio.charset.StandardCharsets.UTF_8); + + assertThat(line).endsWith("\n"); + com.fasterxml.jackson.databind.JsonNode parsed = + new com.fasterxml.jackson.databind.ObjectMapper().readTree(line); + assertThat(parsed.get("message").asText()) + .isEqualTo("a \"quoted\" \\ message\nwith a newline"); + assertThat(parsed.get("level").asText()).isEqualTo("WARN"); + assertThat(parsed.get("logger").asText()).isEqualTo("test.logger"); + } +} diff --git a/src/test/java/com/testingbot/tunnel/LocalWebServerTest.java b/src/test/java/com/testingbot/tunnel/LocalWebServerTest.java new file mode 100644 index 0000000..8ecd66c --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/LocalWebServerTest.java @@ -0,0 +1,83 @@ +package com.testingbot.tunnel; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.net.HttpURLConnection; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The {@code --web} directory server. + * + *

Had no tests at all, and could not have had useful ones: the {@code Server} lived in a + * constructor-local, so nothing could stop it or ask what it was doing. It served an + * operator-chosen directory, with listing enabled, for the life of the JVM -- outliving the + * tunnel it accompanied, leaking a Jetty server per App for an embedder, and holding port 8080 + * so the next run could not bind one. + */ +class LocalWebServerTest { + + @TempDir + Path tempDir; + + private static int freePort() throws IOException { + try (java.net.ServerSocket socket = new java.net.ServerSocket(0)) { + return socket.getLocalPort(); + } + } + + private static String get(String url) throws Exception { + HttpURLConnection connection = (HttpURLConnection) URI.create(url).toURL().openConnection(); + connection.setConnectTimeout(5000); + connection.setReadTimeout(5000); + try (java.io.InputStream in = connection.getInputStream()) { + return new String(in.readAllBytes(), StandardCharsets.UTF_8); + } finally { + connection.disconnect(); + } + } + + @Test + void itServesTheDirectoryItWasGiven() throws Exception { + Files.writeString(tempDir.resolve("hello.txt"), "from the local web server"); + LocalWebServer server = new LocalWebServer(tempDir.toString(), "127.0.0.1", freePort()); + try { + assertThat(server.isRunning()).isTrue(); + assertThat(get("http://127.0.0.1:" + server.getPort() + "/hello.txt")) + .contains("from the local web server"); + } finally { + server.stop(); + } + } + + @Test + void itCanBeStoppedAndReleasesItsPort() throws Exception { + int port = freePort(); + LocalWebServer server = new LocalWebServer(tempDir.toString(), "127.0.0.1", port); + assertThat(server.isRunning()).isTrue(); + + server.stop(); + + assertThat(server.isRunning()).isFalse(); + // The port has to come back, or the next run cannot start one -- the reason this + // mattered beyond tidiness. + try (java.net.ServerSocket rebind = new java.net.ServerSocket(port)) { + assertThat(rebind.isBound()).isTrue(); + } + } + + @Test + void stoppingTwiceIsHarmless() throws Exception { + LocalWebServer server = new LocalWebServer(tempDir.toString(), "127.0.0.1", freePort()); + server.stop(); + server.stop(); + assertThat(server.isRunning()).isFalse(); + } + +} diff --git a/src/test/java/com/testingbot/tunnel/PacFetchRoutingTest.java b/src/test/java/com/testingbot/tunnel/PacFetchRoutingTest.java new file mode 100644 index 0000000..ce3fb87 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/PacFetchRoutingTest.java @@ -0,0 +1,137 @@ +package com.testingbot.tunnel; + +import com.sun.net.httpserver.HttpServer; +import com.testingbot.tunnel.pac.PacPolicy; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.net.InetSocketAddress; +import java.net.Proxy; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Fetching a remote {@code --pac-local} document through the network's own egress rules. + * + *

The fetch was a bare {@code HttpURLConnection}, so it honoured neither {@code --proxy} nor + * {@code --cacert-file}. On a proxy-only network the PAC URL was simply unreachable; on a + * TLS-intercepting network -- the entire reason {@code --cacert-file} exists -- an {@code https} + * PAC URL failed the handshake against a CA the JVM has never seen. In both cases the tunnel + * refused to start over a document it had been told how to reach. + */ +class PacFetchRoutingTest { + + private static final String SCRIPT = + "function FindProxyForURL(url, host) { return \"DIRECT\"; }"; + + @Test + void thePacDocumentIsFetchedThroughTheConfiguredProxy() throws Exception { + List proxiedRequests = new CopyOnWriteArrayList<>(); + + // Stands in for the upstream proxy: an absolute-form request arrives here rather than + // at the origin, which is exactly how we can tell the fetch was routed. + HttpServer proxy = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + proxy.createContext("/", exchange -> { + proxiedRequests.add(exchange.getRequestURI().toString()); + byte[] body = SCRIPT.getBytes(StandardCharsets.UTF_8); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + proxy.start(); + + try { + // A port with nothing on it: if the fetch went direct rather than through the + // proxy, it could only fail, so this cannot pass by accident. + String url = "http://127.0.0.1:" + unusedPort() + "/proxy.pac"; + + PacPolicy policy = PacPolicy.load(url, + sha256Of(SCRIPT), + new PacPolicy.FetchOptions( + new Proxy(Proxy.Type.HTTP, + new InetSocketAddress("127.0.0.1", + proxy.getAddress().getPort())), + null)); + + assertThat(policy.resolve("http://a.corp/", "a.corp").isDirect()).isTrue(); + assertThat(proxiedRequests) + .as("the PAC fetch must go through --proxy, not around it") + .isNotEmpty(); + } finally { + proxy.stop(0); + } + } + + @Test + void aDirectFetchStillWorksWhenNothingIsConfigured() throws Exception { + HttpServer server = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + server.createContext("/proxy.pac", exchange -> { + byte[] body = SCRIPT.getBytes(StandardCharsets.UTF_8); + exchange.sendResponseHeaders(200, body.length); + exchange.getResponseBody().write(body); + exchange.close(); + }); + server.start(); + try { + PacPolicy policy = PacPolicy.load( + "http://127.0.0.1:" + server.getAddress().getPort() + "/proxy.pac", + sha256Of(SCRIPT), + null); + + assertThat(policy.resolve("http://a.corp/", "a.corp").isDirect()).isTrue(); + } finally { + server.stop(0); + } + } + + @Test + void appDerivesFetchOptionsFromTheProxyOption() { + // The wiring, not the mechanism: getPacPolicy() has to pass these on, or the fix is + // present and unreachable. + App app = new App(); + assertThat(app.pacFetchOptions()) + .as("nothing configured means nothing to pass") + .isNull(); + + app.setProxy("proxy.corp:3128"); + PacPolicy.FetchOptions options = app.pacFetchOptions(); + + assertThat(options).isNotNull(); + assertThat(options.proxy()).isNotNull(); + assertThat(options.proxy().type()).isEqualTo(Proxy.Type.HTTP); + assertThat(options.proxy().address().toString()).contains("proxy.corp", "3128"); + } + + @Test + void aSocks5ProxyBecomesASocksProxyForTheFetch() { + App app = new App(); + app.setProxy("socks5://socks.corp:1080"); + + PacPolicy.FetchOptions options = app.pacFetchOptions(); + + assertThat(options).isNotNull(); + assertThat(options.proxy().type()) + .as("a SOCKS proxy is not an HTTP proxy; using the wrong type fails the fetch") + .isEqualTo(Proxy.Type.SOCKS); + } + + /** + * Computed here rather than through PacPolicy.sha256Hex: an independent implementation, so + * the pin actually checks the fetched bytes instead of agreeing with itself. + */ + private static String sha256Of(String text) throws Exception { + byte[] digest = java.security.MessageDigest.getInstance("SHA-256") + .digest(text.getBytes(StandardCharsets.UTF_8)); + return java.util.HexFormat.of().formatHex(digest); + } + + private static int unusedPort() throws IOException { + try (java.net.ServerSocket socket = new java.net.ServerSocket(0)) { + return socket.getLocalPort(); + } + } +} diff --git a/src/test/java/com/testingbot/tunnel/ReadinessGatingTest.java b/src/test/java/com/testingbot/tunnel/ReadinessGatingTest.java new file mode 100644 index 0000000..4a14bd3 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/ReadinessGatingTest.java @@ -0,0 +1,157 @@ +package com.testingbot.tunnel; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.net.ServerSocket; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Readiness has to mean the tunnel is actually forwarding. + * + *

{@code startProxies()} runs three checks -- the Selenium relay reaching the hub, the reverse + * forward reaching the local proxy, and the proxy reaching the internet -- and returned whether + * they passed. The caller then set the gauge to {@code true} regardless and the ready file was + * written from inside {@code startProxies()} on every path, so a tunnel that had just failed all + * three still answered 200 on {@code /readyz} and still touched {@code --readyfile}. Container + * probes and supervisors were being told to send work to a tunnel that could carry none. + * + *

{@link HealthEndpointsTest} covers the endpoints themselves, but sets the gauge by hand -- + * which is why it could not see this. These drive the real startup path instead. + */ +class ReadinessGatingTest { + + @TempDir + Path tempDir; + + @BeforeEach + void setUp() { + TunnelMetrics.setTunnelUp(false); + } + + @AfterEach + void tearDown() { + TunnelMetrics.setTunnelUp(false); + } + + private static int freePort() throws IOException { + try (ServerSocket socket = new ServerSocket(0)) { + return socket.getLocalPort(); + } + } + + /** + * An App whose local listeners come up but whose forwarding cannot work: there is no SSH + * tunnel behind the Selenium relay, so its self-test fails exactly as it would for a + * customer whose tunnel did not establish. + */ + private App appWithNoWorkingTunnel() throws IOException { + App app = new App(); + app.setClientKey("test_key"); + app.setClientSecret("test_secret"); + app.setSeleniumPort(freePort()); + // --no-proxy so the test does not need to bind a proxy port or reach the internet; the + // forwarding check alone is enough to make the startup unhealthy. + app.setNoProxy(true); + return app; + } + + @Test + void aFailedStartupIsNotReported() throws Exception { + App app = appWithNoWorkingTunnel(); + try { + assertThat(app.startProxies()) + .as("the forwarding self-test cannot pass without a tunnel behind it") + .isFalse(); + } finally { + app.stop(); + } + } + + @Test + void aFailedStartupWritesNoReadyFile() throws Exception { + Path readyFile = tempDir.resolve("tunnel.ready"); + App app = appWithNoWorkingTunnel(); + app.setReadyFile(readyFile.toString()); + + try { + assertThat(app.startProxies()).isFalse(); + + // The regression: this file was written at the end of startProxies() whatever the + // checks above it had found, so anything waiting on it started sending work. + assertThat(Files.exists(readyFile)) + .as("--readyfile means ready, not merely 'startup was attempted'") + .isFalse(); + } finally { + app.stop(); + } + } + + @Test + void aFailedStartupLeavesTheTunnelNotReady() throws Exception { + App app = appWithNoWorkingTunnel(); + try { + app.startProxies(); + + assertThat(TunnelMetrics.isTunnelUp()) + .as("/readyz must not answer 200 for a tunnel that failed its self-test") + .isFalse(); + } finally { + app.stop(); + } + } + + @Test + void theReadyFileIsStillWrittenOnTheHealthyPath() throws Exception { + // The other half: gating must not turn into never writing it at all. + Path readyFile = tempDir.resolve("tunnel.ready"); + App app = new App(); + app.setReadyFile(readyFile.toString()); + + app.writeReadyFile(); + + assertThat(Files.exists(readyFile)).isTrue(); + assertThat(Files.readString(readyFile)).contains("Ready"); + } + + @Test + void stoppingRemovesTheReadyFile() throws Exception { + // The file means "this tunnel is forwarding", so it must not outlive the tunnel. The + // shutdown hook covered JVM exit; an explicit stop() -- an embedder between jobs, or the + // reconnect monitor's stop()/boot() rebuild -- left it behind claiming a tunnel that no + // longer existed was ready. + Path readyFile = tempDir.resolve("tunnel.ready"); + App app = new App(); + app.setReadyFile(readyFile.toString()); + app.writeReadyFile(); + assertThat(Files.exists(readyFile)).isTrue(); + + app.stop(); + + assertThat(Files.exists(readyFile)).isFalse(); + } + + @Test + void stoppingWithNoReadyFileConfiguredIsHarmless() { + App app = new App(); + app.stop(); + } + + @Test + void writingTheReadyFileTwiceTouchesRatherThanFails() throws Exception { + Path readyFile = tempDir.resolve("tunnel.ready"); + App app = new App(); + app.setReadyFile(readyFile.toString()); + + app.writeReadyFile(); + app.writeReadyFile(); + + assertThat(Files.exists(readyFile)).isTrue(); + } +} diff --git a/src/test/java/com/testingbot/tunnel/TunnelInfoMetricTest.java b/src/test/java/com/testingbot/tunnel/TunnelInfoMetricTest.java index 05e6bb5..8353aa0 100644 --- a/src/test/java/com/testingbot/tunnel/TunnelInfoMetricTest.java +++ b/src/test/java/com/testingbot/tunnel/TunnelInfoMetricTest.java @@ -29,9 +29,9 @@ private static List samples() { @Test void rebuildingReplacesTheSeriesRatherThanAddingOne() { - TunnelMetrics.setTunnelInfo(5.0f, 1001, "ci-run"); - TunnelMetrics.setTunnelInfo(5.0f, 1002, "ci-run"); - TunnelMetrics.setTunnelInfo(5.0f, 1003, "ci-run"); + TunnelMetrics.setTunnelInfo("5.0", 1001, "ci-run"); + TunnelMetrics.setTunnelInfo("5.0", 1002, "ci-run"); + TunnelMetrics.setTunnelInfo("5.0", 1003, "ci-run"); assertThat(samples()) .as("one active tunnel means one series") @@ -41,8 +41,8 @@ void rebuildingReplacesTheSeriesRatherThanAddingOne() { @Test void theSurvivingSeriesCarriesTheCurrentIdentity() { - TunnelMetrics.setTunnelInfo(5.0f, 2001, "first"); - TunnelMetrics.setTunnelInfo(5.0f, 2002, "second"); + TunnelMetrics.setTunnelInfo("5.0", 2001, "first"); + TunnelMetrics.setTunnelInfo("5.0", 2002, "second"); Collector.MetricFamilySamples.Sample sample = samples().get(0); assertThat(sample.labelValues).containsExactly("5.0", "2002", "second"); @@ -52,7 +52,7 @@ void theSurvivingSeriesCarriesTheCurrentIdentity() { @Test void aMissingIdentifierIsRecordedAsEmptyRatherThanNull() { // Prometheus label values cannot be null; an unnamed tunnel is the common case. - TunnelMetrics.setTunnelInfo(5.0f, 3001, null); + TunnelMetrics.setTunnelInfo("5.0", 3001, null); assertThat(samples()).hasSize(1); assertThat(samples().get(0).labelValues).containsExactly("5.0", "3001", ""); diff --git a/src/test/java/com/testingbot/tunnel/VersionTest.java b/src/test/java/com/testingbot/tunnel/VersionTest.java new file mode 100644 index 0000000..1faf8a3 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/VersionTest.java @@ -0,0 +1,107 @@ +package com.testingbot.tunnel; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Version comparison, which used to be {@code Float} arithmetic. + * + *

The float was not merely imprecise, it was wrong in ways that would have appeared on this + * project's own next few releases, and each failure mode is asserted here directly. + */ +class VersionTest { + + private static Version v(String text) { + return Version.parse(text); + } + + @Test + void aTenthMinorReleaseIsNewerThanANinth() { + // The float bug that mattered most: 5.10 parses to 5.1f, which is less than 5.9f. The + // upgrade notice would have stopped appearing at exactly the release it was needed for. + assertThat(v("5.9").isOlderThan(v("5.10"))).isTrue(); + assertThat(v("5.10").isOlderThan(v("5.9"))).isFalse(); + assertThat(v("5.10")).isGreaterThan(v("5.9")); + } + + @Test + void threeComponentVersionsAreUnderstood() { + // Float.parseFloat("5.0.1") throws. The old code caught that and fell back to 0.0, so a + // patch release made the client believe it was older than everything. + assertThat(v("5.0.1")).isNotNull(); + assertThat(v("5.0").isOlderThan(v("5.0.1"))).isTrue(); + assertThat(v("5.0.1").isOlderThan(v("5.0.2"))).isTrue(); + assertThat(v("5.0.2").isOlderThan(v("5.0.1"))).isFalse(); + } + + @Test + void trailingZerosDoNotChangeTheVersion() { + assertThat(v("5.1")).isEqualTo(v("5.1.0")); + assertThat(v("5")).isEqualTo(v("5.0.0")); + assertThat(v("5.1").isOlderThan(v("5.1.0"))).isFalse(); + // Equal versions must agree on their hash, or a Set of them would hold both. + assertThat(v("5.1")).hasSameHashCodeAs(v("5.1.0")); + } + + @Test + void aSnapshotPrecedesTheReleaseItLeadsTo() { + // So a developer running 5.1.0-SNAPSHOT is correctly told that 5.1.0 is out. + assertThat(v("5.1.0-SNAPSHOT").isOlderThan(v("5.1.0"))).isTrue(); + assertThat(v("5.1.0").isOlderThan(v("5.1.0-SNAPSHOT"))).isFalse(); + // But a snapshot of the next version is still newer than the current release. + assertThat(v("5.1.0").isOlderThan(v("5.2.0-SNAPSHOT"))).isTrue(); + } + + @Test + void preReleasesOfTheSameVersionAreOrdered() { + assertThat(v("5.1.0-rc.1").isOlderThan(v("5.1.0-rc.2"))).isTrue(); + assertThat(v("5.1.0-rc.1")).isEqualTo(v("5.1.0-rc.1")); + } + + @Test + void majorVersionsDominate() { + assertThat(v("5.99.99").isOlderThan(v("6.0"))).isTrue(); + assertThat(v("10.0").isOlderThan(v("9.0"))).isFalse(); + assertThat(v("9.0").isOlderThan(v("10.0"))).isTrue(); + } + + @Test + void somethingUnreadableIsNullRatherThanZero() { + // The distinction the old code could not make. Falling back to 0.0 does not mean + // "unknown", it means "older than every release", so an unreadable version turned the + // upgrade notice on permanently. Null lets the caller decline to guess. + assertThat(Version.parse(null)).isNull(); + assertThat(Version.parse("")).isNull(); + assertThat(Version.parse(" ")).isNull(); + assertThat(Version.parse("unknown")).isNull(); + assertThat(Version.parse("-SNAPSHOT")).isNull(); + } + + @Test + void parsingStopsAtTheFirstNonNumericComponent() { + // "5.1.x.3": the 3 does not mean what its position would suggest once x is dropped, so + // it is not read at all. + assertThat(v("5.1.x.3")).isEqualTo(v("5.1")); + } + + @Test + void theVersionPrintsAsItWasWritten() { + // This is what reaches users and the tunnel_info metric label, so it must not be + // normalised into something they did not ship. + assertThat(v("5.0").toString()).isEqualTo("5.0"); + assertThat(v("5.10.2").toString()).isEqualTo("5.10.2"); + assertThat(v("5.1.0-SNAPSHOT").toString()).isEqualTo("5.1.0-SNAPSHOT"); + } + + @Test + void theProjectsOwnVersionParses() { + // Guards the wiring, not the parser: if version.properties ever takes a shape Version + // cannot read, App.RELEASE is null and the upgrade check silently stops working. + assertThat(App.VERSION).isNotBlank(); + assertThat(App.RELEASE) + .as("App.VERSION (%s) must be comparable, or the upgrade notice never fires", + App.VERSION) + .isNotNull(); + } +} diff --git a/src/test/java/com/testingbot/tunnel/integration/MalformedQueryStringTest.java b/src/test/java/com/testingbot/tunnel/integration/MalformedQueryStringTest.java index 7cca51e..ad8348e 100644 --- a/src/test/java/com/testingbot/tunnel/integration/MalformedQueryStringTest.java +++ b/src/test/java/com/testingbot/tunnel/integration/MalformedQueryStringTest.java @@ -174,7 +174,13 @@ void bareTrailingPercent_isRejectedAsBadRequestNotBadGateway() throws Exception String response = proxyGet("/?q=100%"); assertThat(response).contains("400 Bad Request"); - assertThat(response).doesNotContain("502"); + // The status line, not the whole response. Jetty's error page echoes the request URI, + // which carries the proxy's randomly chosen port -- so doesNotContain("502") failed + // whenever that port happened to contain those digits (seen on port 50281). The same + // substring-on-a-variable-body mistake HttpStatusLine was introduced to remove. + assertThat(response.lines().findFirst().orElse("")) + .as("a 502 would blame the customer's website for a request we never sent") + .doesNotContain("502"); } @Test diff --git a/src/test/java/com/testingbot/tunnel/pac/PacFailureFallbackTest.java b/src/test/java/com/testingbot/tunnel/pac/PacFailureFallbackTest.java new file mode 100644 index 0000000..3cbd59b --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/pac/PacFailureFallbackTest.java @@ -0,0 +1,113 @@ +package com.testingbot.tunnel.pac; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * What a PAC file that fails at runtime means, and what a local file is allowed to be. + * + *

Evaluation failure used to become {@link PacResult#direct()}. That is not a neutral + * default: on a network whose only sanctioned egress is a proxy it takes traffic the operator + * routed deliberately and sends it straight out, past a configured {@code --proxy} as well. The + * component whose entire job is deciding where traffic goes was failing open, and the one word + * in the log was "direct". + */ +class PacFailureFallbackTest { + + @TempDir + Path tempDir; + + /** A file that parses but throws when evaluated -- an undefined function is the easy way. */ + private static PacPolicy throwingPolicy() { + return PacPolicy.of( + "function FindProxyForURL(url, host) { return noSuchFunction(host); }", + "throwing.pac"); + } + + @Test + void aFailedEvaluationIsReportedAsUnknownRatherThanDirect() { + PacPolicy policy = throwingPolicy(); + + assertThat(policy.resolveOrNull("http://example.com/", "example.com")) + .as("null means 'could not decide', which the caller answers with --proxy; " + + "DIRECT would mean 'decided: send it straight out'") + .isNull(); + } + + @Test + void theLegacyResolveStillAnswersDirectSoNothingElseChangesShape() { + // resolve() keeps its old signature and its old answer for callers that have no + // fallback to offer; resolveOrNull is what lets the proxy handlers do better. + assertThat(throwingPolicy().resolve("http://example.com/", "example.com").isDirect()) + .isTrue(); + } + + @Test + void aFailureIsNotCached() { + // A failure is usually about this evaluation -- a dnsResolve that timed out, say -- and + // caching it would hold the fallback route in place for the full cache TTL after the + // condition had passed. + PacPolicy policy = throwingPolicy(); + + assertThat(policy.resolveOrNull("http://example.com/", "example.com")).isNull(); + assertThat(policy.resolveOrNull("http://example.com/", "example.com")).isNull(); + } + + @Test + void aWorkingFileIsUnaffected() { + PacPolicy policy = PacPolicy.of( + "function FindProxyForURL(url, host) { return \"PROXY p.example:3128\"; }", + "ok.pac"); + + PacResult result = policy.resolveOrNull("http://example.com/", "example.com"); + + assertThat(result).isNotNull(); + assertThat(result.first().toProxySpec()).isEqualTo("p.example:3128"); + } + + @Test + void aDeliberateDirectIsStillDirect() { + // The distinction that matters: a file that says DIRECT decided that, and must not be + // rerouted through --proxy just because failures now fall back there. + PacPolicy policy = PacPolicy.of( + "function FindProxyForURL(url, host) { return \"DIRECT\"; }", "direct.pac"); + + PacResult result = policy.resolveOrNull("http://example.com/", "example.com"); + + assertThat(result).isNotNull(); + assertThat(result.isDirect()).isTrue(); + } + + @Test + void anOversizedLocalFileIsRefusedLikeAnOversizedRemoteOne() throws Exception { + // readAllBytes() had no limit, so the cap depended on where the bytes came from. A path + // that is not the small script it was meant to be -- a log, the wrong file entirely -- + // was read into memory in full before anything looked at it. + Path oversized = tempDir.resolve("huge.pac"); + byte[] filler = new byte[PacPolicy.MAX_PAC_BYTES + 1024]; + java.util.Arrays.fill(filler, (byte) ' '); + Files.write(oversized, filler); + + assertThatThrownBy(() -> PacPolicy.load(oversized.toString())) + .isInstanceOf(PacException.class) + .hasMessageContaining("larger than"); + } + + @Test + void aNormalLocalFileStillLoads() throws Exception { + Path file = tempDir.resolve("ok.pac"); + Files.write(file, "function FindProxyForURL(url, host) { return \"DIRECT\"; }" + .getBytes(StandardCharsets.UTF_8)); + + PacPolicy policy = PacPolicy.load(file.toString()); + + assertThat(policy.resolveOrNull("http://example.com/", "example.com").isDirect()).isTrue(); + } +} diff --git a/src/test/java/com/testingbot/tunnel/proxy/ConnectToWithProxyTest.java b/src/test/java/com/testingbot/tunnel/proxy/ConnectToWithProxyTest.java new file mode 100644 index 0000000..f2014c8 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/proxy/ConnectToWithProxyTest.java @@ -0,0 +1,140 @@ +package com.testingbot.tunnel.proxy; + +import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.core.WireMockConfiguration; + +import org.apache.hc.client5.http.classic.methods.HttpGet; +import org.apache.hc.client5.http.config.RequestConfig; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.client5.http.impl.classic.HttpClients; +import org.apache.hc.core5.http.HttpHost; + +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static org.assertj.core.api.Assertions.assertThat; + +/** + * {@code --connect-to} when an upstream proxy is also configured. + * + *

The rule describes where a named *destination* lives. With a proxy in play the socket goes + * to the proxy and the destination travels in the request line, so there is nothing at dial time + * for the rule to apply to -- but it was applied anyway, to the only endpoint being dialled: the + * proxy. A wildcard entry therefore redirected the proxy connection itself and left the + * destination exactly as it was, which is close to the opposite of what was asked for. + * + *

{@code ConnectToMapTest} covers the mapping itself and the existing dial tests cover direct + * connections, so neither could see this: it only appears where the two features meet. + */ +class ConnectToWithProxyTest { + + private WireMockServer upstreamProxy; + private WireMockServer origin; + private WireMockServer decoy; + private Server localProxyServer; + private int localProxyPort; + + @BeforeEach + void setUp() { + upstreamProxy = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + upstreamProxy.start(); + origin = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + origin.start(); + decoy = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + decoy.start(); + } + + @AfterEach + void tearDown() throws Exception { + if (localProxyServer != null && localProxyServer.isStarted()) { + localProxyServer.stop(); + } + for (WireMockServer server : new WireMockServer[]{upstreamProxy, origin, decoy}) { + if (server != null && server.isRunning()) { + server.stop(); + } + } + } + + private void startLocalProxy(String upstream, String... connectTo) throws Exception { + localProxyServer = new Server(0); + TunnelProxyHandler handler = new TunnelProxyHandler(); + if (upstream != null) { + handler.setUpstreamProxy(upstream, null); + } + handler.setConnectTo(ConnectToMap.parse(connectTo)); + localProxyServer.setHandler(handler); + localProxyServer.start(); + localProxyPort = ((ServerConnector) localProxyServer.getConnectors()[0]).getLocalPort(); + } + + private int getThrough(String url) throws Exception { + RequestConfig config = RequestConfig.custom() + .setProxy(new HttpHost("http", "127.0.0.1", localProxyPort)) + .build(); + try (CloseableHttpClient client = HttpClients.custom() + .setDefaultRequestConfig(config) + .build()) { + return client.execute(new HttpGet(url), response -> response.getCode()); + } + } + + @Test + void aWildcardRuleDoesNotDivertTheUpstreamProxyItself() throws Exception { + origin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200))); + upstreamProxy.stubFor(any(urlMatching(".*")) + .willReturn(aResponse().proxiedFrom("http://127.0.0.1:" + origin.port()))); + decoy.stubFor(any(urlMatching(".*")).willReturn(aResponse().withStatus(418))); + + // "::host:port" -- empty HOST1 and PORT1, so it matches anything, as curl allows. + // Applied to the proxy endpoint this sent the proxy connection to the decoy. + startLocalProxy("127.0.0.1:" + upstreamProxy.port(), + "::127.0.0.1:" + decoy.port()); + + assertThat(getThrough("http://127.0.0.1:" + origin.port() + "/thing")).isEqualTo(200); + + upstreamProxy.verify(getRequestedFor(urlPathEqualTo("/thing"))); + assertThat(decoy.getAllServeEvents()) + .as("the proxy connection must not be remapped") + .isEmpty(); + } + + @Test + void aRuleNamingTheProxysAddressStillDoesNotMoveIt() throws Exception { + // The proxy happens to share an address with a rule's HOST1:PORT1. It is still a proxy + // endpoint, not a destination, so the rule does not apply to this socket. + origin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200))); + upstreamProxy.stubFor(any(urlMatching(".*")) + .willReturn(aResponse().proxiedFrom("http://127.0.0.1:" + origin.port()))); + decoy.stubFor(any(urlMatching(".*")).willReturn(aResponse().withStatus(418))); + + startLocalProxy("127.0.0.1:" + upstreamProxy.port(), + "127.0.0.1:" + upstreamProxy.port() + ":127.0.0.1:" + decoy.port()); + + assertThat(getThrough("http://127.0.0.1:" + origin.port() + "/thing")).isEqualTo(200); + + upstreamProxy.verify(getRequestedFor(urlPathEqualTo("/thing"))); + assertThat(decoy.getAllServeEvents()).isEmpty(); + } + + @Test + void withoutAProxyTheDestinationIsStillRemapped() throws Exception { + // The feature itself, unchanged: this is the case the existing tests cover, kept here so + // a fix that simply stopped honouring --connect-to would not pass. + decoy.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(418))); + + startLocalProxy(null, "127.0.0.1:" + origin.port() + ":127.0.0.1:" + decoy.port()); + + assertThat(getThrough("http://127.0.0.1:" + origin.port() + "/thing")).isEqualTo(418); + + decoy.verify(getRequestedFor(urlPathEqualTo("/thing"))); + assertThat(origin.getAllServeEvents()).isEmpty(); + } +} diff --git a/src/test/java/com/testingbot/tunnel/proxy/HttpStatusLineTest.java b/src/test/java/com/testingbot/tunnel/proxy/HttpStatusLineTest.java new file mode 100644 index 0000000..a7a7b81 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/proxy/HttpStatusLineTest.java @@ -0,0 +1,88 @@ +package com.testingbot.tunnel.proxy; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The status-line parser shared by the CONNECT and WebSocket relays. + * + *

The WebSocket relay used to ask whether the first line {@code contains("101")}. The cases + * that gets wrong are not hypothetical -- a target that refuses an upgrade answers with a status + * and a reason phrase, and the phrase is written by whoever wrote the target. + */ +class HttpStatusLineTest { + + @Test + void aRealSwitchingProtocolsIsAccepted() { + assertThat(HttpStatusLine.isSwitchingProtocols("HTTP/1.1 101 Switching Protocols")).isTrue(); + assertThat(HttpStatusLine.isSwitchingProtocols("HTTP/1.0 101 Switching Protocols")).isTrue(); + // No reason phrase is legal, and some servers send none. + assertThat(HttpStatusLine.isSwitchingProtocols("HTTP/1.1 101")).isTrue(); + } + + @Test + void aLongerCodeContaining101IsNot() { + // contains("101") accepted this, and the relay then spliced the client to a target that + // had not agreed to a WebSocket at all. + assertThat(HttpStatusLine.isSwitchingProtocols("HTTP/1.1 2101 Nonsense")).isFalse(); + assertThat(HttpStatusLine.isSwitchingProtocols("HTTP/1.1 1010 Nonsense")).isFalse(); + } + + @Test + void aReasonPhraseMentioning101IsNot() { + // The realistic version of the bug: the failure message itself carries the digits. + assertThat(HttpStatusLine.isSwitchingProtocols( + "HTTP/1.1 500 Error 101 in upstream handler")).isFalse(); + assertThat(HttpStatusLine.isSwitchingProtocols( + "HTTP/1.1 404 Not Found: /socket101")).isFalse(); + assertThat(HttpStatusLine.isSwitchingProtocols( + "HTTP/1.1 403 Forbidden by rule 101")).isFalse(); + } + + @Test + void otherSuccessCodesAreNotAnUpgrade() { + // A 200 to an upgrade request means the server ignored the Upgrade header and answered + // the GET normally. Relaying that as a WebSocket produces a connection that hangs. + assertThat(HttpStatusLine.isSwitchingProtocols("HTTP/1.1 200 OK")).isFalse(); + assertThat(HttpStatusLine.isSwitchingProtocols("HTTP/1.1 204 No Content")).isFalse(); + } + + @Test + void rubbishIsRejectedRatherThanGuessedAt() { + assertThat(HttpStatusLine.parse(null)).isEqualTo(HttpStatusLine.INVALID); + assertThat(HttpStatusLine.parse("")).isEqualTo(HttpStatusLine.INVALID); + assertThat(HttpStatusLine.parse("101")).isEqualTo(HttpStatusLine.INVALID); + assertThat(HttpStatusLine.parse("HTTP/1.1")).isEqualTo(HttpStatusLine.INVALID); + assertThat(HttpStatusLine.parse("HTTP/1.1 abc")).isEqualTo(HttpStatusLine.INVALID); + assertThat(HttpStatusLine.parse("GARBAGE 101 x")).isEqualTo(HttpStatusLine.INVALID); + // Integer.parseInt would take these; a status is exactly three digits. + assertThat(HttpStatusLine.parse("HTTP/1.1 +101")).isEqualTo(HttpStatusLine.INVALID); + assertThat(HttpStatusLine.parse("HTTP/1.1 -101")).isEqualTo(HttpStatusLine.INVALID); + } + + @Test + void theConnectSideKeepsItsExistingBehaviour() { + // CustomConnectHandler delegates here now, so its contract is asserted against the + // shared parser: 2xx and nothing else. + assertThat(HttpStatusLine.isSuccessfulConnect("HTTP/1.1 200 Connection established")).isTrue(); + assertThat(HttpStatusLine.isSuccessfulConnect("HTTP/1.1 299 Odd but successful")).isTrue(); + assertThat(HttpStatusLine.isSuccessfulConnect("HTTP/1.1 407 Proxy Authentication Required")).isFalse(); + assertThat(HttpStatusLine.isSuccessfulConnect("HTTP/1.1 502 Bad Gateway")).isFalse(); + assertThat(HttpStatusLine.isSuccessfulConnect("HTTP/1.1 101 Switching Protocols")).isFalse(); + assertThat(HttpStatusLine.isSuccessfulConnect(null)).isFalse(); + } + + @Test + void theTwoEntryPointsAgreeWithTheParser() { + // Guards the delegation itself: CustomConnectHandler.isSuccessfulConnect is the name the + // framing tests use, and it must stay the same predicate. + for (String line : new String[]{ + "HTTP/1.1 200 OK", "HTTP/1.1 101 Switching Protocols", + "HTTP/1.1 2101 Nonsense", "HTTP/1.1 500 Error 101", null}) { + assertThat(CustomConnectHandler.isSuccessfulConnect(line)) + .as("status line: %s", line) + .isEqualTo(HttpStatusLine.isSuccessfulConnect(line)); + } + } +} diff --git a/src/test/java/com/testingbot/tunnel/proxy/PacHttpRoutingTest.java b/src/test/java/com/testingbot/tunnel/proxy/PacHttpRoutingTest.java new file mode 100644 index 0000000..3433a4b --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/proxy/PacHttpRoutingTest.java @@ -0,0 +1,261 @@ +package com.testingbot.tunnel.proxy; + +import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.core.WireMockConfiguration; +import com.testingbot.tunnel.pac.PacPolicy; + +import org.apache.hc.client5.http.classic.methods.HttpGet; +import org.apache.hc.client5.http.config.RequestConfig; +import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; +import org.apache.hc.client5.http.impl.classic.HttpClients; +import org.apache.hc.core5.http.HttpHost; + +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static org.assertj.core.api.Assertions.assertThat; + +/** + * {@code --pac-local} applied to ordinary HTTP requests. + * + *

This path was wired but not implemented: {@code TunnelProxyHandler} had a + * {@code setPacPolicy} that {@code HttpProxy} dutifully called and nothing ever read, so its + * client only ever knew the static {@code --proxy}. CONNECT and WebSocket upgrades did consult + * the file, which made routing depend on the scheme -- the same tunnel sent {@code https://} + * where the PAC file said and {@code http://} somewhere else entirely. + * + *

It survived because the PAC coverage was all unit tests of the interpreter plus an e2e + * scenario that only ran {@code --pac-test}, which evaluates the file and prints the answer + * without proxying anything. So every test of "what does this PAC file say" passed while + * "where does traffic actually go" was never asked on this path. + */ +class PacHttpRoutingTest { + + private WireMockServer viaProxy; + private WireMockServer directOrigin; + private WireMockServer proxiedOrigin; + private Server localProxyServer; + private int localProxyPort; + + @BeforeEach + void setUp() { + viaProxy = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + viaProxy.start(); + directOrigin = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + directOrigin.start(); + proxiedOrigin = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + proxiedOrigin.start(); + } + + @AfterEach + void tearDown() throws Exception { + if (localProxyServer != null && localProxyServer.isStarted()) { + localProxyServer.stop(); + } + for (WireMockServer server : new WireMockServer[]{viaProxy, directOrigin, proxiedOrigin}) { + if (server != null && server.isRunning()) { + server.stop(); + } + } + } + + private void startLocalProxy(String pacScript, String staticProxy) throws Exception { + localProxyServer = new Server(0); + TunnelProxyHandler handler = new TunnelProxyHandler(); + if (staticProxy != null) { + handler.setUpstreamProxy(staticProxy, null); + } + handler.setPacPolicy(PacPolicy.of(pacScript, "test.pac")); + localProxyServer.setHandler(handler); + localProxyServer.start(); + localProxyPort = ((ServerConnector) localProxyServer.getConnectors()[0]).getLocalPort(); + } + + private int getThrough(String url) throws Exception { + RequestConfig config = RequestConfig.custom() + .setProxy(new HttpHost("http", "127.0.0.1", localProxyPort)) + .build(); + try (CloseableHttpClient client = HttpClients.custom() + .setDefaultRequestConfig(config) + .build()) { + return client.execute(new HttpGet(url), response -> response.getCode()); + } + } + + @Test + void aPacFileRoutesPlainHttpThroughTheProxyItNames() throws Exception { + proxiedOrigin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200).withBody("origin"))); + viaProxy.stubFor(any(urlMatching(".*")) + .willReturn(aResponse().proxiedFrom("http://127.0.0.1:" + proxiedOrigin.port()))); + + // Everything goes via the proxy. Before the fix this file was read, cached, and ignored. + startLocalProxy("function FindProxyForURL(url, host) {" + + " return \"PROXY 127.0.0.1:" + viaProxy.port() + "\"; }", null); + + assertThat(getThrough("http://127.0.0.1:" + proxiedOrigin.port() + "/thing")).isEqualTo(200); + + viaProxy.verify(getRequestedFor(urlPathEqualTo("/thing"))); + } + + @Test + void aPacFileSendsDirectHostsDirect() throws Exception { + directOrigin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200).withBody("origin"))); + viaProxy.stubFor(any(urlMatching(".*")) + .willReturn(aResponse().withStatus(500))); + + startLocalProxy("function FindProxyForURL(url, host) { return \"DIRECT\"; }", null); + + assertThat(getThrough("http://127.0.0.1:" + directOrigin.port() + "/thing")).isEqualTo(200); + + directOrigin.verify(getRequestedFor(urlPathEqualTo("/thing"))); + assertThat(viaProxy.getAllServeEvents()).isEmpty(); + } + + @Test + void theDecisionIsPerDestinationRatherThanOncePerProcess() throws Exception { + // The point of a PAC file, and the thing a single static ProxyConfiguration entry cannot + // express: two origins in one run, one proxied and one direct. + directOrigin.stubFor(get(urlPathEqualTo("/direct")) + .willReturn(aResponse().withStatus(200))); + proxiedOrigin.stubFor(get(urlPathEqualTo("/proxied")) + .willReturn(aResponse().withStatus(200))); + viaProxy.stubFor(any(urlMatching(".*")) + .willReturn(aResponse().proxiedFrom("http://127.0.0.1:" + proxiedOrigin.port()))); + + startLocalProxy("function FindProxyForURL(url, host) {" + + " if (url.indexOf(\"" + proxiedOrigin.port() + "\") >= 0)" + + " return \"PROXY 127.0.0.1:" + viaProxy.port() + "\";" + + " return \"DIRECT\"; }", null); + + assertThat(getThrough("http://127.0.0.1:" + proxiedOrigin.port() + "/proxied")).isEqualTo(200); + assertThat(getThrough("http://127.0.0.1:" + directOrigin.port() + "/direct")).isEqualTo(200); + + viaProxy.verify(getRequestedFor(urlPathEqualTo("/proxied"))); + viaProxy.verify(0, getRequestedFor(urlPathEqualTo("/direct"))); + directOrigin.verify(getRequestedFor(urlPathEqualTo("/direct"))); + } + + @Test + void aPacFileThatThrowsFallsBackToTheStaticProxyRatherThanGoingDirect() throws Exception { + // The fail-open case. A broken PAC file used to resolve to DIRECT, which on a + // proxy-only network is not a fallback but a bypass: traffic the operator deliberately + // routed through --proxy went straight out instead, and the log said "going direct". + proxiedOrigin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200).withBody("origin"))); + viaProxy.stubFor(any(urlMatching(".*")) + .willReturn(aResponse().proxiedFrom("http://127.0.0.1:" + proxiedOrigin.port()))); + + // Parses, but throws on every evaluation. + startLocalProxy("function FindProxyForURL(url, host) { return noSuchFunction(host); }", + "127.0.0.1:" + viaProxy.port()); + + assertThat(getThrough("http://127.0.0.1:" + proxiedOrigin.port() + "/thing")).isEqualTo(200); + + viaProxy.verify(getRequestedFor(urlPathEqualTo("/thing"))); + } + + @Test + void aBrokenPacWithNoProxyConfiguredStillGoesDirect() throws Exception { + // With nothing else configured there is genuinely nowhere else to send it, so the + // fallback must not turn a working direct setup into a failure. + directOrigin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200))); + + startLocalProxy("function FindProxyForURL(url, host) { return noSuchFunction(host); }", + null); + + assertThat(getThrough("http://127.0.0.1:" + directOrigin.port() + "/thing")).isEqualTo(200); + directOrigin.verify(getRequestedFor(urlPathEqualTo("/thing"))); + } + + @Test + void theCredentialIsSentWhenPacRoutesToTheConfiguredProxy() throws Exception { + // The other side of the withholding rule. --proxy-userpwd names a specific proxy, and + // when the PAC file routes there -- or when a failed evaluation falls back to it -- that + // proxy is the rightful recipient and must still get its credential. + proxiedOrigin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200))); + String expected = "Basic " + java.util.Base64.getEncoder() + .encodeToString("user:sesame".getBytes(java.nio.charset.StandardCharsets.UTF_8)); + viaProxy.stubFor(any(urlMatching(".*")) + .withHeader("Proxy-Authorization", equalTo(expected)) + .willReturn(aResponse().proxiedFrom("http://127.0.0.1:" + proxiedOrigin.port()))); + viaProxy.stubFor(any(urlMatching(".*")) + .withHeader("Proxy-Authorization", absent()) + .willReturn(aResponse().withStatus(407))); + + TunnelProxyHandler handler = new TunnelProxyHandler(); + handler.setUpstreamProxy("127.0.0.1:" + viaProxy.port(), "user:sesame"); + handler.setPacPolicy(PacPolicy.of("function FindProxyForURL(url, host) {" + + " return \"PROXY 127.0.0.1:" + viaProxy.port() + "\"; }", "test.pac")); + localProxyServer = new Server(0); + localProxyServer.setHandler(handler); + localProxyServer.start(); + localProxyPort = ((ServerConnector) localProxyServer.getConnectors()[0]).getLocalPort(); + + assertThat(getThrough("http://127.0.0.1:" + proxiedOrigin.port() + "/thing")) + .as("the proxy --proxy-userpwd names must still receive its credential") + .isEqualTo(200); + } + + @Test + void theStaticProxyCredentialIsNotSentToAPacChosenDestination() throws Exception { + // --proxy-userpwd names one specific proxy. Once a PAC file decides routing, --proxy is + // no longer the recipient, so its password must not be stamped on requests -- which for + // a DIRECT answer means sending it straight to the origin, where it lands in an + // arbitrary internet host's access log. + directOrigin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200))); + + WireMockServer staticProxy = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + staticProxy.start(); + try { + TunnelProxyHandler handler = new TunnelProxyHandler(); + handler.setUpstreamProxy("127.0.0.1:" + staticProxy.port(), "user:sesame"); + handler.setPacPolicy(PacPolicy.of( + "function FindProxyForURL(url, host) { return \"DIRECT\"; }", "test.pac")); + localProxyServer = new Server(0); + localProxyServer.setHandler(handler); + localProxyServer.start(); + localProxyPort = ((ServerConnector) localProxyServer.getConnectors()[0]).getLocalPort(); + + assertThat(getThrough("http://127.0.0.1:" + directOrigin.port() + "/thing")) + .isEqualTo(200); + + directOrigin.verify(getRequestedFor(urlPathEqualTo("/thing")) + .withoutHeader("Proxy-Authorization")); + } finally { + staticProxy.stop(); + } + } + + @Test + void pacBeatsAStaticProxyForPlainHttpAsItDoesForConnect() throws Exception { + // A --proxy registered alongside the PAC entries would match every origin and be chosen + // ahead of them, which is the same "file is ignored" outcome by a different route. + directOrigin.stubFor(get(urlPathEqualTo("/thing")) + .willReturn(aResponse().withStatus(200))); + + WireMockServer staticProxy = new WireMockServer(WireMockConfiguration.options().dynamicPort()); + staticProxy.start(); + try { + staticProxy.stubFor(any(urlMatching(".*")).willReturn(aResponse().withStatus(500))); + + startLocalProxy("function FindProxyForURL(url, host) { return \"DIRECT\"; }", + "127.0.0.1:" + staticProxy.port()); + + assertThat(getThrough("http://127.0.0.1:" + directOrigin.port() + "/thing")).isEqualTo(200); + assertThat(staticProxy.getAllServeEvents()) + .as("--pac-local decides; --proxy is not consulted for a host it covers") + .isEmpty(); + } finally { + staticProxy.stop(); + } + } +} diff --git a/src/test/java/com/testingbot/tunnel/proxy/WebsocketUpgradeStatusTest.java b/src/test/java/com/testingbot/tunnel/proxy/WebsocketUpgradeStatusTest.java new file mode 100644 index 0000000..d10325c --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/proxy/WebsocketUpgradeStatusTest.java @@ -0,0 +1,171 @@ +package com.testingbot.tunnel.proxy; + +import com.testingbot.tunnel.App; +import com.testingbot.tunnel.HttpProxy; +import com.testingbot.tunnel.TestPorts; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * What the WebSocket relay accepts as a successful upgrade, driven through the real handler. + * + *

{@link HttpStatusLineTest} covers the parser; this covers the wiring, because the bug was + * never in a helper -- {@code WebsocketHandler} asked whether the target's first line + * {@code contains("101")} and, when it did, answered the client {@code 101 Switching Protocols} + * and spliced the two sockets together. A client then held what it believed was a WebSocket to a + * server that had refused it. + * + *

{@code ConnectFramingTest.aStatusLineMentioning200InItsReasonIsStillARejection} is the same + * test on the CONNECT path, which has had it all along. This is the one the WebSocket path never + * got. + */ +class WebsocketUpgradeStatusTest { + + private ServerSocket target; + private ExecutorService pool; + private HttpProxy httpProxy; + private int proxyPort; + + @AfterEach + void tearDown() throws Exception { + if (httpProxy != null) { + httpProxy.stop(); + } + if (target != null && !target.isClosed()) { + target.close(); + } + if (pool != null) { + pool.shutdownNow(); + // A stale interrupt left by another component's inline shutdown would otherwise + // fail this teardown for reasons unrelated to the test; the same guard + // ConnectFramingTest carries. + Thread.interrupted(); + pool.awaitTermination(5, TimeUnit.SECONDS); + } + } + + /** A stand-in target that answers every upgrade with {@code statusLine}. */ + private void startTarget(String statusLine) throws IOException { + target = new ServerSocket(0, 50, InetAddress.getLoopbackAddress()); + pool = Executors.newCachedThreadPool(); + pool.submit(() -> { + while (!target.isClosed()) { + try { + Socket socket = target.accept(); + pool.submit(() -> { + try { + BufferedReader in = new BufferedReader(new InputStreamReader( + socket.getInputStream(), StandardCharsets.UTF_8)); + String line; + while ((line = in.readLine()) != null && !line.isEmpty()) { + // drain the upgrade request + } + OutputStream out = socket.getOutputStream(); + out.write((statusLine + "\r\nConnection: close\r\n\r\n") + .getBytes(StandardCharsets.US_ASCII)); + out.flush(); + Thread.sleep(30_000); + } catch (Exception ignored) { + // test finished + } + }); + } catch (IOException closed) { + return; + } + } + }); + } + + private void startTunnel() throws Exception { + proxyPort = TestPorts.free(); + App app = new App(); + app.setJettyPort(proxyPort); + app.setClientKey("test_key"); + app.setClientSecret("test_secret"); + httpProxy = new HttpProxy(app); + for (int i = 0; i < 100; i++) { + try (Socket s = new Socket("127.0.0.1", proxyPort)) { + return; + } catch (IOException retry) { + Thread.sleep(50); + } + } + throw new IllegalStateException("proxy did not start"); + } + + /** Sends a ws:// upgrade through the tunnel and returns the client's first response line. */ + private String upgradeThroughTunnel() throws Exception { + try (Socket client = new Socket("127.0.0.1", proxyPort)) { + client.setSoTimeout(15_000); + String host = "127.0.0.1:" + target.getLocalPort(); + String request = "GET http://" + host + "/ws HTTP/1.1\r\n" + + "Host: " + host + "\r\n" + + "Upgrade: websocket\r\n" + + "Connection: Upgrade\r\n" + + "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n" + + "Sec-WebSocket-Version: 13\r\n\r\n"; + client.getOutputStream().write(request.getBytes(StandardCharsets.US_ASCII)); + client.getOutputStream().flush(); + + BufferedReader in = new BufferedReader(new InputStreamReader( + client.getInputStream(), StandardCharsets.UTF_8)); + String first = in.readLine(); + return first == null ? "" : first; + } + } + + @Test + void aGenuineUpgradeIsRelayed() throws Exception { + startTarget("HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket"); + startTunnel(); + + assertThat(upgradeThroughTunnel()) + .as("a real 101 must still be relayed; the fix must not break the feature") + .contains("101"); + } + + @Test + void aFailureWhoseReasonPhraseMentions101IsNotAnUpgrade() throws Exception { + // The realistic shape of the bug: the target refuses, and its own error text carries the + // digits. contains("101") read that as success and handed the client a 101. + startTarget("HTTP/1.1 500 Internal Error 101 in upstream handler"); + startTunnel(); + + assertThat(upgradeThroughTunnel()) + .as("a 500 is a refusal however its reason phrase reads") + .doesNotContain("101 Switching Protocols"); + } + + @Test + void aStatusCodeMerelyContaining101IsNotAnUpgrade() throws Exception { + startTarget("HTTP/1.1 2101 Nonsense"); + startTunnel(); + + assertThat(upgradeThroughTunnel()).doesNotContain("101 Switching Protocols"); + } + + @Test + void aPlainOkIsNotAnUpgrade() throws Exception { + // A server that ignored the Upgrade header and answered the GET normally. Relaying that + // as a WebSocket produces a connection that simply hangs. + startTarget("HTTP/1.1 200 OK"); + startTunnel(); + + assertThat(upgradeThroughTunnel()).doesNotContain("101 Switching Protocols"); + } +} diff --git a/src/test/java/ssh/CustomConnectionMonitorTest.java b/src/test/java/ssh/CustomConnectionMonitorTest.java index 9247518..cfe781a 100644 --- a/src/test/java/ssh/CustomConnectionMonitorTest.java +++ b/src/test/java/ssh/CustomConnectionMonitorTest.java @@ -249,6 +249,47 @@ void aProxyRestartFailureDoesNotTearDownAHealthySshSession() { assertThat(scheduler.hasPending()).as("the proxy start should be retried").isTrue(); } + @Test + void aRepeatedProxyFailureStillNeverRedialsTheSshSession() { + // The gap the previous test left: it fired once and stopped. The retry it scheduled ran + // attemptReconnect(), whose first act is stop() then connect() -- so the branch written + // to avoid re-dialling a healthy session did exactly that on every subsequent tick. + tunnel.authenticated = true; + host.startFailure = new IllegalStateException("Address already in use"); + monitor.connectionLost(new RuntimeException("drop")); + + for (int i = 0; i < 5; i++) { + scheduler.fire(); + } + + assertThat(tunnel.calls) + .as("one dial for the reconnect, and none for the proxy retries") + .containsExactly("stop", "connect"); + assertThat(host.calls) + .filteredOn("startLocalProxy"::equals) + .as("only the proxy is retried") + .hasSize(5); + } + + @Test + void aProxyThatNeverBindsFallsThroughToARebuild() { + // Reaching onReconnected() returned before attemptReconnect()'s limit check, so this + // path had no bound at all: a port that never frees up retried every five seconds + // forever, and the rebuild that would have released it was never reached. + tunnel.authenticated = true; + host.startFailure = new IllegalStateException("Address already in use"); + monitor.connectionLost(new RuntimeException("drop")); + + for (int i = 0; i < CustomConnectionMonitor.MAX_RETRIES + 1; i++) { + if (!scheduler.hasPending()) { + break; + } + scheduler.fire(); + } + + assertThat(host.calls).as("the retry is bounded").contains("rebuildTunnel"); + } + @Test void aProxyRestartThatRecoversCompletesTheReconnect() { tunnel.authenticated = true; diff --git a/src/test/java/ssh/PortForwardingMonitorTest.java b/src/test/java/ssh/PortForwardingMonitorTest.java index 96f3224..8272ba8 100644 --- a/src/test/java/ssh/PortForwardingMonitorTest.java +++ b/src/test/java/ssh/PortForwardingMonitorTest.java @@ -37,12 +37,44 @@ void anyEntryInTheListCounts() { } @Test - void theMatchIsASubstringSoADigitPrefixAlsoMatches() { - // Documenting the real behaviour rather than an assumed one: "445" is a substring of - // "4456", so a port that merely shares a prefix reads as active. Harmless in practice -- - // the alternative is a false "forwarding lost" and a needless restart -- but it is not - // what the code appears to say at a glance. - assertThat(SSHTunnel.localForwardingActive(new String[]{"4456:h:80"}, 445)).isTrue(); + void aPortThatMerelySharesAPrefixIsNotAMatch() { + // Was documented as harmless: "445" is a substring of "4456", so the old contains() + // check reported a forward that does not exist. It is not harmless -- this answers "is + // my forward still there", and the monitor repairs it when the answer is no. A false + // positive means the repair never runs and every request through the tunnel keeps + // failing, with the log insisting forwarding is fine. + assertThat(SSHTunnel.localForwardingActive(new String[]{"4456:h:80"}, 445)).isFalse(); + assertThat(SSHTunnel.localForwardingActive(new String[]{"4456:h:80"}, 4456)).isTrue(); + } + + @Test + void onlyTheLocalPortIsCompared() { + // The digits also appear in the destination host and the remote port, and neither + // identifies this forward. + assertThat(SSHTunnel.localForwardingActive(new String[]{"9999:host80.example:80"}, 80)) + .as("the remote port is not the local port") + .isFalse(); + assertThat(SSHTunnel.localForwardingActive(new String[]{"9999:h4446.example:80"}, 4446)) + .as("digits in the destination host are not a port") + .isFalse(); + } + + @Test + void aBindAddressBeforeThePortIsUnderstood() { + // JSch renders a bound forward as "127.0.0.1:4446:host:80". + assertThat(SSHTunnel.localForwardingActive(new String[]{"127.0.0.1:4446:h:80"}, 4446)) + .isTrue(); + assertThat(SSHTunnel.localForwardingActive(new String[]{"127.0.0.1:4446:h:80"}, 127)) + .as("the bind address is not the port") + .isFalse(); + } + + @Test + void malformedEntriesDoNotMatch() { + assertThat(SSHTunnel.localForwardingActive(new String[]{"nonsense"}, 4446)).isFalse(); + assertThat(SSHTunnel.localForwardingActive(new String[]{""}, 4446)).isFalse(); + assertThat(SSHTunnel.localForwardingActive(new String[]{null}, 4446)).isFalse(); + assertThat(SSHTunnel.localForwardingActive(null, 4446)).isFalse(); } @Test diff --git a/src/test/java/ssh/TunnelPollerTest.java b/src/test/java/ssh/TunnelPollerTest.java index 5d97fbe..af2aa2a 100644 --- a/src/test/java/ssh/TunnelPollerTest.java +++ b/src/test/java/ssh/TunnelPollerTest.java @@ -107,17 +107,62 @@ void aTunnelStillBootingIsPolledAgain() throws Exception { } @Test - void aFailureWhilePollingStopsTheSchedule() throws Exception { - when(api.pollTunnel(anyString())).thenThrow(new RuntimeException("boom")); + void oneFailedPollIsRetriedRatherThanEndingTheTunnel() throws Exception { + when(api.pollTunnel(anyString())) + .thenThrow(new RuntimeException("boom")) + .thenReturn(state("READY")); poller(); scheduler.poll(); - // The cancel is the point: without it the poller keeps throwing every five seconds for - // the life of the process. The old test asserted only that tunnelReady was not called. - verify(api, times(1)).pollTunnel("tunnel123"); - verify(app, never()).tunnelReady(any()); + // The previous behaviour, and what the previous test asserted: a single exception + // cancelled the schedule for good. Nothing then terminated or reported, so the process + // stayed alive and permanently unready on one transient API error. + assertThat(scheduler.wasCancelled()) + .as("a single failed poll must not stop the schedule") + .isFalse(); + verify(app, never()).setupFailed(anyString(), org.mockito.ArgumentMatchers.anyInt()); + + scheduler.poll(); + + verify(app).tunnelReady(any()); + } + + @Test + void aSustainedRunOfFailuresGivesUpAndReportsIt() throws Exception { + when(api.pollTunnel(anyString())).thenThrow(new RuntimeException("boom")); + poller(); + + for (int i = 0; i < TunnelPoller.MAX_CONSECUTIVE_ERRORS; i++) { + scheduler.poll(); + } + + // Retrying forever would be the opposite mistake. The schedule stops, and -- the part + // that was missing -- the app is told, so it stops reporting ready and the command line + // client exits instead of lingering. assertThat(scheduler.wasCancelled()).isTrue(); + verify(api, times(TunnelPoller.MAX_CONSECUTIVE_ERRORS)).pollTunnel("tunnel123"); + verify(app).setupFailed(anyString(), org.mockito.ArgumentMatchers.anyInt()); + } + + @Test + void aSuccessfulPollResetsTheFailureCount() throws Exception { + when(api.pollTunnel(anyString())) + .thenThrow(new RuntimeException("boom")) + .thenThrow(new RuntimeException("boom")) + .thenReturn(state("BOOTING")) + .thenThrow(new RuntimeException("boom")) + .thenThrow(new RuntimeException("boom")); + poller(); + + for (int i = 0; i < 5; i++) { + scheduler.poll(); + } + + // Four failures in total but never MAX_CONSECUTIVE_ERRORS in a row, which is the + // distinction the counter exists to make: an unreliable network is not a dead tunnel. + assertThat(scheduler.wasCancelled()).isFalse(); + verify(app, never()).setupFailed(anyString(), org.mockito.ArgumentMatchers.anyInt()); } @Test @@ -130,8 +175,10 @@ void aTunnelThatCameUpButCouldNotBeSetUpStopsTheSchedule() throws Exception { scheduler.poll(); // Runs on a timer thread with nobody to propagate to, so it must stop itself rather - // than retry a setup that already failed. + // than retry a setup that already failed -- and say so, rather than leaving a live + // process that will never be ready. assertThat(scheduler.wasCancelled()).isTrue(); + verify(app).setupFailed(anyString(), org.mockito.ArgumentMatchers.anyInt()); } @Test