Fix/improve X-Forwarded-For/throttling testing (#410)

* Previously it was not actually sending the header * Now runs in serial, regardless of parallel specification * Now runs only once, rather than for each map layer _Also_: * Removed no-longer used `storage_mutex` * Was only used for Apache HTTP Server < v2.4
openstreetmap · Mar 15, 2024 · 21713d0 · 21713d0
1 parent d67a672
commit 21713d0
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 64 deletions.
diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml
@@ -208,7 +208,8 @@ jobs:
     continue-on-error: true
     env:
       CFLAGS: --coverage
-      CTEST_HOST: localhost
+      CTEST_CLIENT_HOST: ::1
+      CTEST_SERVER_HOST: localhost
       CXXFLAGS: --coverage
       LIBRARY_PATH: /usr/local/lib
       TMPDIR: /tmp

diff --git a/docs/build/building_on_freebsd.md b/docs/build/building_on_freebsd.md
@@ -34,6 +34,8 @@ sudo pkg install --yes \
 
 # Download, Build, Test & Install `mod_tile`
 export CMAKE_BUILD_PARALLEL_LEVEL=$(sysctl -n hw.ncpu)
+export CTEST_CLIENT_HOST="::1"
+export CTEST_SERVER_HOST="localhost"
 export LIBRARY_PATH="/usr/local/lib"
 rm -rf /tmp/mod_tile_src /tmp/mod_tile_build
 mkdir /tmp/mod_tile_src /tmp/mod_tile_build

diff --git a/src/mod_tile.c b/src/mod_tile.c
@@ -90,7 +90,6 @@ char *shmfilename;
 char *shmfilename_delaypool;
 apr_global_mutex_t *stats_mutex;
 apr_global_mutex_t *delay_mutex;
-apr_global_mutex_t *storage_mutex;
 
 char *mutexfilename;
 int layerCount = 0;
@@ -1868,36 +1867,6 @@ static int mod_tile_post_config(apr_pool_t *pconf, apr_pool_t *plog,
 		return HTTP_INTERNAL_SERVER_ERROR;
 	}
 
-#endif /* MOD_TILE_SET_MUTEX_PERMS */
-
-	/*
-	     * Create another unique filename to lock upon. Note that
-	     * depending on OS and locking mechanism of choice, the file
-	     * may or may not be actually created.
-	     */
-	mutexfilename = apr_psprintf(pconf, "/tmp/httpd_mutex_storage.%ld",
-				     (long int) getpid());
-
-	rs = apr_global_mutex_create(&storage_mutex, (const char *) mutexfilename,
-				     APR_LOCK_DEFAULT, pconf);
-
-	if (rs != APR_SUCCESS) {
-		ap_log_error(APLOG_MARK, APLOG_ERR, rs, s,
-			     "Failed to create mutex on file %s",
-			     mutexfilename);
-		return HTTP_INTERNAL_SERVER_ERROR;
-	}
-
-#ifdef MOD_TILE_SET_MUTEX_PERMS
-	rs = ap_unixd_set_global_mutex_perms(storage_mutex);
-
-	if (rs != APR_SUCCESS) {
-		ap_log_error(APLOG_MARK, APLOG_CRIT, rs, s,
-			     "Parent could not set permissions on mod_tile "
-			     "mutex: check User and Group directives");
-		return HTTP_INTERNAL_SERVER_ERROR;
-	}
-
 #endif /* MOD_TILE_SET_MUTEX_PERMS */
 
 	return OK;

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -36,12 +36,21 @@ find_program(SLEEP_EXECUTABLE NAMES sleep REQUIRED)
 find_program(TOUCH_EXECUTABLE NAMES gtouch touch REQUIRED)
 
 # Sets the host to be used for CTest test services
-if(DEFINED ENV{CTEST_HOST})
-  # To the value of environment variable "CTEST_HOST"
-  set(CTEST_HOST "$ENV{CTEST_HOST}")
+if(DEFINED ENV{CTEST_SERVER_HOST})
+  # To the value of environment variable "CTEST_SERVER_HOST"
+  set(CTEST_SERVER_HOST "$ENV{CTEST_SERVER_HOST}")
 else()
   # Or to 0.0.0.0 by default
-  set(CTEST_HOST "0.0.0.0")
+  set(CTEST_SERVER_HOST "0.0.0.0")
+endif()
+
+# Sets the host to be used for CTest client
+if(DEFINED ENV{CTEST_CLIENT_HOST})
+  # To the value of environment variable "CTEST_CLIENT_HOST"
+  set(CTEST_CLIENT_HOST "$ENV{CTEST_CLIENT_HOST}")
+else()
+  # Or to 127.0.0.1 by default
+  set(CTEST_CLIENT_HOST "127.0.0.1")
 endif()
 
 #-----------------------------------------------------------------------------
@@ -51,15 +60,15 @@ endif()
 #-----------------------------------------------------------------------------
 
 set(DEFAULT_MAP_NAME "default")
-set(HTTPD0_HOST "${CTEST_HOST}")
+set(HTTPD0_HOST "${CTEST_SERVER_HOST}")
 set(HTTPD0_PORT_BASE "59000")
-set(HTTPD1_HOST "${CTEST_HOST}")
+set(HTTPD1_HOST "${CTEST_SERVER_HOST}")
 set(HTTPD1_PORT_BASE "59100")
-set(HTTPD2_HOST "${CTEST_HOST}")
+set(HTTPD2_HOST "${CTEST_SERVER_HOST}")
 set(HTTPD2_PORT_BASE "59200")
-set(MEMCACHED_HOST "${CTEST_HOST}")
+set(MEMCACHED_HOST "${CTEST_SERVER_HOST}")
 set(MEMCACHED_PORT_BASE "60000")
-set(RENDERD1_HOST "${CTEST_HOST}")
+set(RENDERD1_HOST "${CTEST_SERVER_HOST}")
 set(RENDERD1_PORT_BASE "59500")
 
 set(CURL_CMD "${CURL_EXECUTABLE} --fail --silent")
@@ -754,26 +763,6 @@ foreach(STORAGE_BACKEND_INDEX RANGE ${STORAGE_BACKENDS_LENGTH})
     set_tests_properties(tile_expired_${MAP_NAME}_${STORAGE_BACKEND} PROPERTIES
       FIXTURES_REQUIRED services_started_${STORAGE_BACKEND}
     )
-    set(TILE_URL_PATH "/tiles/${MAP_NAME}/3/0/3.${EXTENSION}")
-    set(HTTPD1_URL "http://${HTTPD1_HOST}:${HTTPD1_PORT}${TILE_URL_PATH}")
-    set(HTTPD2_URL "http://${HTTPD2_HOST}:${HTTPD2_PORT}${TILE_URL_PATH}")
-    add_test(NAME throttling_xforward_${MAP_NAME}_${STORAGE_BACKEND}
-      COMMAND ${BASH} -c "
-        TEST_CURL_CMD=\"${CURL_CMD} --header 'X-Forwarded-For: ${CTEST_HOST}, 127.0.0.1' --output /dev/null\"
-        for i in {0..10}; do
-          if ! \${TEST_CURL_CMD} ${HTTPD1_URL}; then
-            echo \"${HTTPD1_URL}\";
-          fi
-          if ! \${TEST_CURL_CMD} ${HTTPD2_URL}; then
-            echo \"${HTTPD2_URL}\";
-          fi
-        done
-      "
-      WORKING_DIRECTORY tests
-    )
-    set_tests_properties(throttling_xforward_${MAP_NAME}_${STORAGE_BACKEND} PROPERTIES
-      FIXTURES_REQUIRED services_started_${STORAGE_BACKEND}
-    )
   endforeach()
 
   # Generate file and URL paths for tiles
@@ -977,6 +966,27 @@ foreach(STORAGE_BACKEND_INDEX RANGE ${STORAGE_BACKENDS_LENGTH})
     set_tests_properties(max_load_missing_${STORAGE_BACKEND} PROPERTIES
       FIXTURES_REQUIRED services_started_${STORAGE_BACKEND}
     )
+    add_test(NAME throttling_xforward_${STORAGE_BACKEND}
+      COMMAND ${BASH} -c "
+        RESPONSE_CODE_CMD=\"${CURL_CMD} --write-out %{http_code}\"
+        for i in {0..20}; do
+          RESPONSE_CODE=$(\${RESPONSE_CODE_CMD} --header \"X-Forwarded-For: ${CTEST_SERVER_HOST}, ${CTEST_CLIENT_HOST}\" \
+            http://${HTTPD2_HOST}:${HTTPD2_PORT}/tiles/webp/\$i/0/0.webp)
+          echo \"Response code: '\${RESPONSE_CODE}'\"
+          if [ \"\${RESPONSE_CODE}\" = \"503\" ]; then
+            echo 'Request being rejected';
+            exit 0;
+          fi
+        done
+        echo 'Request was never rejected';
+        exit 1;
+      "
+      WORKING_DIRECTORY tests
+    )
+    set_tests_properties(throttling_xforward_${STORAGE_BACKEND} PROPERTIES
+      FIXTURES_REQUIRED services_started_${STORAGE_BACKEND}
+      RUN_SERIAL TRUE
+    )
   endif()
 
   if(JQ_EXECUTABLE)

diff --git a/tests/httpd.conf.in b/tests/httpd.conf.in
@@ -58,7 +58,7 @@ Redirect /renderd-example-map/leaflet/leaflet.min.js https://unpkg.com/leaflet/d
   ModTileEnableDirtyURL Off
   ModTileEnableStats Off
   ModTileEnableStatusURL Off
-  ModTileEnableTileThrottling On
+  ModTileEnableTileThrottling Off
   ModTileEnableTileThrottlingXForward 1
   ModTileMissingRequestTimeout 3
   ModTileRenderdSocketAddr @RENDERD1_HOST@ @RENDERD1_PORT@
@@ -88,8 +88,8 @@ Redirect /renderd-example-map/leaflet/leaflet.min.js https://unpkg.com/leaflet/d
   ModTileMissingRequestTimeout 3
   ModTileRenderdSocketName @RENDERD2_SOCKET@
   ModTileRequestTimeout 3
-  ModTileThrottlingRenders 1 0.2
-  ModTileThrottlingTiles 1 1
+  ModTileThrottlingRenders 10 0.2
+  ModTileThrottlingTiles 10 0.2
   ModTileTileDir @TILE_DIR@
   ModTileVeryOldThreshold -10000000
 </VirtualHost>