From f31e4bbe2f414daa8318d4b2bf05eada689254c8 Mon Sep 17 00:00:00 2001 From: Parth Date: Fri, 28 Aug 2026 14:29:32 +0100 Subject: [PATCH] rate_limit: fix swapped metric tag and prefix in YAML configs initializeMetrics() takes the tag before the prefix, and the remap path passes them in that order, but the YAML path had them reversed. Every SNI limiter with a metrics node was named tag.sni.prefix instead of prefix.sni.tag, including when only one of the keys was set and the other came from its default. Fixes: #13599 --- plugins/experimental/rate_limit/limiter.h | 4 +- .../rate_limit_metric_names.test.py | 97 +++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 tests/gold_tests/pluginTest/rate_limit/rate_limit_metric_names.test.py diff --git a/plugins/experimental/rate_limit/limiter.h b/plugins/experimental/rate_limit/limiter.h index 038c295faf1..715cb6316e6 100644 --- a/plugins/experimental/rate_limit/limiter.h +++ b/plugins/experimental/rate_limit/limiter.h @@ -224,8 +224,8 @@ template class RateLimiter std::string prefix = metrics["prefix"] ? metrics["prefix"].as() : RATE_LIMITER_METRIC_PREFIX; std::string tag = metrics["tag"] ? metrics["tag"].as() : name(); - Dbg(dbg_ctl, "Metrics for selector rule: %s(%s, %s)", name().c_str(), prefix.c_str(), tag.c_str()); - initializeMetrics(RATE_LIMITER_TYPE_SNI, prefix, tag); + Dbg(dbg_ctl, "Metrics for selector rule: %s (prefix=%s, tag=%s)", name().c_str(), prefix.c_str(), tag.c_str()); + initializeMetrics(RATE_LIMITER_TYPE_SNI, tag, prefix); } return true; diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_metric_names.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_metric_names.test.py new file mode 100644 index 00000000000..85a8fca820e --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_metric_names.test.py @@ -0,0 +1,97 @@ +''' +Test that the rate_limit YAML metrics node names metrics as prefix.type.tag. +''' +# 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. + +import os + +Test.Summary = ''' +rate_limit: the YAML metrics node builds names as prefix.type.tag. +''' + +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) +Test.ContinueOnFail = True + +server = Test.MakeOriginServer("server") +server.addResponse( + "sessionlog.json", { + "headers": "GET /health HTTP/1.1\r\nHost: metrics.example.com\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" + }, { + "headers": "HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\n", + "timestamp": "1469733493.993", + "body": "OK" + }) + +ts = Test.MakeATSProcess("ts") + +# Three selectors: both keys set, only the tag set, and only the prefix set. +# The defaults are the metric prefix and the SNI respectively, so a swap is +# visible in every one of them. +rate_limit_yaml = os.path.join(ts.Variables.CONFIGDIR, 'rate_limit.yaml') +ts.Disk.File( + rate_limit_yaml, typename="ats:config").AddLines( + [ + 'selector:', + ' - sni: both.example.com', + ' limit: 100', + ' metrics:', + ' prefix: myprefix', + ' tag: mytag', + ' - sni: tagonly.example.com', + ' limit: 100', + ' metrics:', + ' tag: onlytag', + ' - sni: prefixonly.example.com', + ' limit: 100', + ' metrics:', + ' prefix: onlyprefix', + '', + ]) + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', +}) + +ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{server.Variables.Port}/') +ts.Disk.plugin_config.AddLine(f'rate_limit.so {rate_limit_yaml}') + +# The metrics are registered by TSStatCreate while the config is parsed, so they +# are queryable as soon as ATS is up, with no traffic needed. +tr = Test.AddTestRun("Metric names use the configured prefix and tag") +tr.Processes.Default.StartBefore(server, ready=When.PortOpen(server.Variables.Port)) +tr.Processes.Default.StartBefore(ts) +tr.Processes.Default.Command = 'traffic_ctl metric match sni' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.StillRunningAfter = ts + +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'myprefix.sni.mytag.queued', 'prefix and tag should keep their configured positions') +tr.Processes.Default.Streams.All += Testers.ExcludesExpression('mytag.sni.myprefix', 'the prefix and tag must not be swapped') + +tr.Processes.Default.Streams.All += Testers.ContainsExpression( + 'plugin.rate_limiter.sni.onlytag.queued', 'an unset prefix should fall back to the default prefix') +tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + 'onlytag.sni.plugin.rate_limiter', 'the default prefix must not be used as the tag') + +tr.Processes.Default.Streams.All += Testers.ContainsExpression( + 'onlyprefix.sni.prefixonly.example.com.queued', 'an unset tag should fall back to the SNI') +tr.Processes.Default.Streams.All += Testers.ExcludesExpression( + 'prefixonly.example.com.sni.onlyprefix', 'the SNI must not be used as the prefix')