This is an automated email from the ASF dual-hosted git repository.
cmcfarlen pushed a commit to branch 10.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/10.0.x by this push:
new ae23a7d085 Add unit tests for remap rules. Fix release-assert crasher
(#11951) (#11957)
ae23a7d085 is described below
commit ae23a7d085d0d48b2563f96ff22ec176dac33f32
Author: Chris McFarlen <[email protected]>
AuthorDate: Wed Jan 15 11:31:06 2025 -0600
Add unit tests for remap rules. Fix release-assert crasher (#11951)
(#11957)
* Add unit tests for remap rules. Fix release-assert crasher (#11951)
* Add unit tests for remap rules. Fix release-assert when adding default
src ip rule
* move RegressionSM from proxy to cache
* Fix link for linux
* fix memory leaks
* Clear rules_list in destructor
* Move rules cleanup to destructor from reset
---------
Co-authored-by: Chris McFarlen <[email protected]>
(cherry picked from commit 302a31fd8d2580e3dcfaeab8838681e8e8995b59)
* Adjust test for ats 10.0.x for warning
---------
Co-authored-by: Chris McFarlen <[email protected]>
---
include/proxy/http/remap/RemapConfig.h | 1 +
include/proxy/http/remap/UrlRewrite.h | 4 +-
src/iocore/cache/CMakeLists.txt | 3 +-
src/iocore/cache/CacheTest.cc | 1 +
src/iocore/cache/P_CacheTest.h | 2 +-
src/{proxy => iocore/cache}/RegressionSM.cc | 4 +-
{include/proxy => src/iocore/cache}/RegressionSM.h | 0
src/proxy/CMakeLists.txt | 4 -
src/proxy/http/remap/RemapConfig.cc | 38 +++--
src/proxy/http/remap/unit-tests/CMakeLists.txt | 18 +++
src/proxy/http/remap/unit-tests/test_RemapRules.cc | 166 +++++++++++++++++++++
11 files changed, 209 insertions(+), 32 deletions(-)
diff --git a/include/proxy/http/remap/RemapConfig.h
b/include/proxy/http/remap/RemapConfig.h
index 024a86142b..4ef2470bd1 100644
--- a/include/proxy/http/remap/RemapConfig.h
+++ b/include/proxy/http/remap/RemapConfig.h
@@ -72,6 +72,7 @@ struct BUILD_TABLE_INFO {
};
const char *remap_parse_directive(BUILD_TABLE_INFO *bti, char *errbuf, size_t
errbufsize);
+bool remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti);
const char *remap_validate_filter_args(acl_filter_rule **rule_pp, const char
**argv, int argc, char *errStrBuf,
size_t errStrBufSize, ACLBehaviorPolicy
behavior_policy);
diff --git a/include/proxy/http/remap/UrlRewrite.h
b/include/proxy/http/remap/UrlRewrite.h
index adb6ab71b2..38ca57ca0c 100644
--- a/include/proxy/http/remap/UrlRewrite.h
+++ b/include/proxy/http/remap/UrlRewrite.h
@@ -24,7 +24,6 @@
#pragma once
-#include "tscore/ink_config.h"
#include "proxy/http/remap/UrlMapping.h"
#include "proxy/http/remap/UrlMappingPathIndex.h"
#include "proxy/http/HttpTransact.h"
@@ -38,9 +37,8 @@
#define URL_REMAP_FILTER_REFERER 0x00000001 /* enable "referer" header
validation */
#define URL_REMAP_FILTER_REDIRECT_FMT 0x00010000 /* enable redirect URL
formatting */
-struct BUILD_TABLE_INFO;
-
/**
+
* used for redirection, mapping, and reverse mapping
**/
enum mapping_type {
diff --git a/src/iocore/cache/CMakeLists.txt b/src/iocore/cache/CMakeLists.txt
index 6e67dc1866..3e47baa943 100644
--- a/src/iocore/cache/CMakeLists.txt
+++ b/src/iocore/cache/CMakeLists.txt
@@ -37,8 +37,7 @@ add_library(
add_library(ts::inkcache ALIAS inkcache)
if(BUILD_REGRESSION_TESTING)
- target_sources(inkcache PRIVATE CacheTest.cc)
- target_link_libraries(inkcache PRIVATE ts::proxy)
+ target_sources(inkcache PRIVATE CacheTest.cc RegressionSM.cc)
endif()
target_include_directories(inkcache PRIVATE ${CMAKE_SOURCE_DIR}/lib)
diff --git a/src/iocore/cache/CacheTest.cc b/src/iocore/cache/CacheTest.cc
index 072a27834b..9afe798cf6 100644
--- a/src/iocore/cache/CacheTest.cc
+++ b/src/iocore/cache/CacheTest.cc
@@ -24,6 +24,7 @@
#include "P_Cache.h"
#include "P_CacheTest.h"
+#include "RegressionSM.h"
#include "tscore/Random.h"
#include <vector>
#include <cmath>
diff --git a/src/iocore/cache/P_CacheTest.h b/src/iocore/cache/P_CacheTest.h
index 33d45d2ab8..68a3bcc334 100644
--- a/src/iocore/cache/P_CacheTest.h
+++ b/src/iocore/cache/P_CacheTest.h
@@ -24,7 +24,7 @@
#pragma once
#include "P_Cache.h"
-#include "proxy/RegressionSM.h"
+#include "RegressionSM.h"
#define MAX_HOSTS_POSSIBLE 256
#define PINNED_DOC_TABLE_SIZE 16
diff --git a/src/proxy/RegressionSM.cc b/src/iocore/cache/RegressionSM.cc
similarity index 98%
rename from src/proxy/RegressionSM.cc
rename to src/iocore/cache/RegressionSM.cc
index 033c151a5c..4955f33489 100644
--- a/src/proxy/RegressionSM.cc
+++ b/src/iocore/cache/RegressionSM.cc
@@ -21,8 +21,8 @@
limitations under the License.
*/
-#include "../iocore/eventsystem/P_EventSystem.h"
-#include "proxy/RegressionSM.h"
+#include "../eventsystem/P_EventSystem.h"
+#include "RegressionSM.h"
#define REGRESSION_SM_RETRY (100 * HRTIME_MSECOND)
diff --git a/include/proxy/RegressionSM.h b/src/iocore/cache/RegressionSM.h
similarity index 100%
rename from include/proxy/RegressionSM.h
rename to src/iocore/cache/RegressionSM.h
diff --git a/src/proxy/CMakeLists.txt b/src/proxy/CMakeLists.txt
index 6b25c6683d..b4a426ad25 100644
--- a/src/proxy/CMakeLists.txt
+++ b/src/proxy/CMakeLists.txt
@@ -40,10 +40,6 @@ add_library(
)
add_library(ts::proxy ALIAS proxy)
-if(BUILD_REGRESSION_TESTING)
- target_sources(proxy PRIVATE RegressionSM.cc)
-endif()
-
target_link_libraries(
proxy
PUBLIC ts::inkcache ts::inkevent ts::tsutil ts::tscore
diff --git a/src/proxy/http/remap/RemapConfig.cc
b/src/proxy/http/remap/RemapConfig.cc
index aa49a4b931..8ff273c3f5 100644
--- a/src/proxy/http/remap/RemapConfig.cc
+++ b/src/proxy/http/remap/RemapConfig.cc
@@ -46,9 +46,6 @@ namespace
DbgCtl dbg_ctl_url_rewrite{"url_rewrite"};
DbgCtl dbg_ctl_remap_plugin{"remap_plugin"};
DbgCtl dbg_ctl_url_rewrite_regex{"url_rewrite_regex"};
-
-bool remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti);
-
} // end anonymous namespace
/**
@@ -100,6 +97,14 @@ BUILD_TABLE_INFO::BUILD_TABLE_INFO()
BUILD_TABLE_INFO::~BUILD_TABLE_INFO()
{
this->reset();
+
+ // clean up any leftover named filter rules
+ auto *rp = rules_list;
+ while (rp != nullptr) {
+ auto *tmp = rp->next;
+ delete rp;
+ rp = tmp;
+ }
}
void
@@ -146,6 +151,15 @@ process_filter_opt(url_mapping *mp, const BUILD_TABLE_INFO
*bti, char *errStrBuf
bti->behavior_policy)) !=
nullptr) {
break;
}
+ if (auto rule = *rpp; rule) {
+ // If no IP addresses are listed, treat that like `@src_ip=all`.
+ if (rule->src_ip_valid == 0 && rule->src_ip_cnt == 0) {
+ src_ip_info_t *ipi = &rule->src_ip_array[rule->src_ip_cnt];
+ ipi->match_all_addresses = true;
+ rule->src_ip_cnt++;
+ rule->src_ip_valid = 1;
+ }
+ }
}
}
@@ -471,8 +485,7 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const
char **argv, int arg
Dbg(dbg_ctl_url_rewrite, "[validate_filter_args] new acl_filter_rule class
was created during remap rule processing");
}
- bool action_flag = false;
- bool ip_is_listed = false;
+ bool action_flag = false;
for (i = 0; i < argc; i++) {
unsigned long ul;
bool hasarg;
@@ -555,7 +568,6 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const
char **argv, int arg
if (ipi) {
rule->src_ip_cnt++;
rule->src_ip_valid = 1;
- ip_is_listed = true;
}
}
@@ -585,7 +597,6 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const
char **argv, int arg
if (ipi) {
rule->src_ip_category_cnt++;
rule->src_ip_category_valid = 1;
- ip_is_listed = true;
}
}
@@ -628,7 +639,6 @@ remap_validate_filter_args(acl_filter_rule **rule_pp, const
char **argv, int arg
if (ipi) {
rule->in_ip_cnt++;
rule->in_ip_valid = 1;
- ip_is_listed = true;
}
}
@@ -687,15 +697,6 @@ remap_validate_filter_args(acl_filter_rule **rule_pp,
const char **argv, int arg
}
}
- if (!ip_is_listed) {
- // If no IP addresses are listed, treat that like `@src_ip=all`.
- ink_release_assert(rule->src_ip_valid == 0 && rule->src_ip_cnt == 0);
- src_ip_info_t *ipi = &rule->src_ip_array[rule->src_ip_cnt];
- ipi->match_all_addresses = true;
- rule->src_ip_cnt++;
- rule->src_ip_valid = 1;
- }
-
if (dbg_ctl_url_rewrite.on()) {
rule->print();
}
@@ -1025,8 +1026,6 @@ lFail:
return false;
}
-namespace
-{
bool
remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti)
{
@@ -1472,7 +1471,6 @@ remap_parse_config_bti(const char *path, BUILD_TABLE_INFO
*bti)
IpAllow::enableAcceptCheck(bti->accept_check_p);
return true;
}
-} // end anonymous namespace
bool
remap_parse_config(const char *path, UrlRewrite *rewrite)
diff --git a/src/proxy/http/remap/unit-tests/CMakeLists.txt
b/src/proxy/http/remap/unit-tests/CMakeLists.txt
index 04ed65782a..ed651e29ec 100644
--- a/src/proxy/http/remap/unit-tests/CMakeLists.txt
+++ b/src/proxy/http/remap/unit-tests/CMakeLists.txt
@@ -269,3 +269,21 @@ target_link_libraries(
)
add_test(NAME test_NextHopConsistentHash COMMAND
$<TARGET_FILE:test_NextHopConsistentHash>)
+
+### test_RemapRules
########################################################################
+add_executable(test_RemapRules
"${PROJECT_SOURCE_DIR}/src/iocore/cache/unit_tests/stub.cc" test_RemapRules.cc)
+
+target_link_libraries(
+ test_RemapRules
+ PRIVATE catch2::catch2
+ ts::http
+ ts::hdrs # transitive
+ logging # transitive
+ ts::http_remap # transitive
+ ts::proxy
+ inkdns # transitive
+ ts::inknet
+ ts::jsonrpc_protocol
+)
+
+add_test(NAME test_RemapRules COMMAND $<TARGET_FILE:test_RemapRules>)
diff --git a/src/proxy/http/remap/unit-tests/test_RemapRules.cc
b/src/proxy/http/remap/unit-tests/test_RemapRules.cc
new file mode 100644
index 0000000000..f226f4dc45
--- /dev/null
+++ b/src/proxy/http/remap/unit-tests/test_RemapRules.cc
@@ -0,0 +1,166 @@
+/** @file
+
+ Unit tests for a class that deals with remap rules
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ @section details Details
+
+ Implements code necessary for Reverse Proxy which mostly consists of
+ general purpose hostname substitution in URLs.
+
+ */
+
+#include "proxy/hdrs/HdrHeap.h"
+#include "proxy/http/remap/RemapConfig.h"
+#include "proxy/http/remap/UrlMapping.h"
+#include "proxy/http/remap/UrlRewrite.h"
+#include "records/RecordsConfig.h"
+#include "swoc/swoc_file.h"
+#include "ts/apidefs.h"
+#include "tscore/BaseLogFile.h"
+#include <memory>
+
+#define CATCH_CONFIG_MAIN /* include main function */
+#include <catch.hpp> /* catch unit-test framework */
+
+struct TestListener : Catch::TestEventListenerBase {
+ using TestEventListenerBase::TestEventListenerBase;
+
+ void
+ testRunStarting(Catch::TestRunInfo const & /* testRunInfo ATS_UNUSED */)
override
+ {
+ Thread *main_thread = new EThread();
+ main_thread->set_specific();
+
+ DiagsPtr::set(new Diags("test_RemapRules", "*", "", new
BaseLogFile("stderr")));
+ // diags()->activate_taglist(".*", DiagsTagType_Debug);
+ // diags()->config.enabled(DiagsTagType_Debug, 1);
+ diags()->show_location = SHOW_LOCATION_DEBUG;
+
+ url_init();
+ mime_init();
+ http_init();
+ Layout::create();
+ RecProcessInit(diags());
+ LibRecordsConfigInit();
+ }
+};
+
+CATCH_REGISTER_LISTENER(TestListener);
+
+swoc::file::path
+write_test_remap(const std::string &config, const std::string &tag)
+{
+ auto tmpdir = swoc::file::temp_directory_path();
+ auto path = tmpdir / swoc::file::path(tag + ".config");
+
+ std::ofstream f(path.c_str(), std::ios::trunc);
+ f.write(config.data(), config.size());
+ f.close();
+
+ return path;
+}
+
+SCENARIO("Parsing ACL named filters", "[proxy][remap]")
+{
+ GIVEN("Named filter definitions with multiple actions")
+ {
+ BUILD_TABLE_INFO bti{};
+ UrlRewrite rewrite{};
+
+ bti.rewrite = &rewrite;
+
+ WHEN("filter rule definition has multiple @action")
+ {
+ std::string config = R"RMCFG(
+ .definefilter deny_methods @action=deny @method=CONNECT @action=allow
@method=PUT @method=DELETE
+ )RMCFG";
+ auto cpath = write_test_remap(config, "test2");
+ THEN("For ATS 10.0.x, this doesnt fail")
+ {
+ REQUIRE(remap_parse_config_bti(cpath.c_str(), &bti) == true);
+ }
+ }
+
+ WHEN("filter rule redefine has multiple @action")
+ {
+ std::string config = R"RMCFG(
+ .definefilter deny_methods @action=deny @method=CONNECT
+ .definefilter deny_methods @action=allow @method=PUT @method=DELETE
+ )RMCFG";
+ auto cpath = write_test_remap(config, "test2");
+ THEN("The rule uses the last action specified")
+ {
+ REQUIRE(remap_parse_config_bti(cpath.c_str(), &bti) == true);
+ REQUIRE(bti.rules_list != nullptr);
+ REQUIRE(bti.rules_list->next == nullptr);
+ REQUIRE(bti.rules_list->allow_flag == true);
+ }
+ }
+ }
+}
+
+struct EasyURL {
+ URL url;
+ HdrHeap *heap;
+
+ EasyURL(std::string_view s)
+ {
+ heap = new_HdrHeap();
+ url.create(heap);
+ url.parse(s);
+ }
+ ~EasyURL() { heap->destroy(); }
+};
+
+SCENARIO("Parsing UrlRewrite", "[proxy][remap]")
+{
+ GIVEN("A named remap rule without ips")
+ {
+ std::unique_ptr<UrlRewrite> urlrw = std::make_unique<UrlRewrite>();
+
+ std::string config = R"RMCFG(
+.definefilter deny_methods @action=deny @method=CONNECT @method=PUT
@method=DELETE
+.activatefilter deny_methods
+map https://h1.example.com \
+ https://h2.example.com
+.deactivatefilter deny_methods
+ )RMCFG";
+
+ auto cpath = write_test_remap(config, "test1");
+ printf("wrote config to path: %s\n", cpath.c_str());
+ int rc = urlrw->BuildTable(cpath.c_str());
+ EasyURL url("https://h1.example.com";);
+ const char *host = "h1.example.com";
+
+ THEN("the remap rules has an ip=all")
+ {
+ REQUIRE(rc == TS_SUCCESS);
+ REQUIRE(urlrw->rule_count() == 1);
+ UrlMappingContainer urlmap;
+
+ REQUIRE(urlrw->forwardMappingLookup(&url.url, 443, host, strlen(host),
urlmap));
+ REQUIRE(urlmap.getMapping()->filter);
+ REQUIRE(urlmap.getMapping()->filter->src_ip_cnt == 1);
+ REQUIRE(urlmap.getMapping()->filter->src_ip_valid);
+
REQUIRE(urlmap.getMapping()->filter->src_ip_array[0].match_all_addresses);
+ }
+ }
+}
