Commit f78cd68d authored by Andrew Newdigate's avatar Andrew Newdigate

Switch back to using regexps in `tracing_url_template`

This approach is able to cope with `%` characters in the URL template,
which is important since `%` is a valid URL character. Additionally
this approach is less likely to fail on an invalid string. This is
important since the distributed tracing infrastructure is designed to
degrade gracefully when not properly configured, and a small mistake
in the configuration of the URL template could have led to a production
outage.
parent 24fb8cda
...@@ -27,10 +27,11 @@ module Gitlab ...@@ -27,10 +27,11 @@ module Gitlab
def self.tracing_url def self.tracing_url
return unless tracing_url_enabled? return unless tracing_url_enabled?
tracing_url_template % { # Avoid using `format` since it can throw TypeErrors
correlation_id: Gitlab::CorrelationId.current_id.to_s, # which we want to avoid on unsanitised env var input
service: Gitlab.process_name tracing_url_template.to_s
} .gsub(/\{\{\s*correlation_id\s*\}\}/, Gitlab::CorrelationId.current_id.to_s)
.gsub(/\{\{\s*service\s*\}\}/, Gitlab.process_name)
end end
end end
end end
...@@ -44,13 +44,15 @@ describe Gitlab::Tracing do ...@@ -44,13 +44,15 @@ describe Gitlab::Tracing do
describe '.tracing_url' do describe '.tracing_url' do
where(:tracing_url_enabled?, :tracing_url_template, :correlation_id, :process_name, :tracing_url) do where(:tracing_url_enabled?, :tracing_url_template, :correlation_id, :process_name, :tracing_url) do
false | "https://localhost" | "123" | "web" | nil false | "https://localhost" | "123" | "web" | nil
true | "https://localhost" | "123" | "web" | "https://localhost" true | "https://localhost" | "123" | "web" | "https://localhost"
true | "https://localhost?service=%{service}" | "123" | "web" | "https://localhost?service=web" true | "https://localhost?service={{ service }}" | "123" | "web" | "https://localhost?service=web"
true | "https://localhost?c=%{correlation_id}" | "123" | "web" | "https://localhost?c=123" true | "https://localhost?c={{ correlation_id }}" | "123" | "web" | "https://localhost?c=123"
true | "https://localhost?c=%{correlation_id}&s=%{service}" | "123" | "web" | "https://localhost?c=123&s=web" true | "https://localhost?c={{ correlation_id }}&s={{ service }}" | "123" | "web" | "https://localhost?c=123&s=web"
true | "https://localhost?c=%{correlation_id}" | nil | "web" | "https://localhost?c=" true | "https://localhost?c={{ correlation_id }}" | nil | "web" | "https://localhost?c="
true | "https://localhost?c=%{correlation_id}&s=%22%{service}%22" | "123" | "web" | "https://localhost?c=123&s=%22web%22" true | "https://localhost?c={{ correlation_id }}&s=%22{{ service }}%22" | "123" | "web" | "https://localhost?c=123&s=%22web%22"
true | "https://localhost?c={{correlation_id}}&s={{service}}" | "123" | "web" | "https://localhost?c=123&s=web"
true | "https://localhost?c={{correlation_id }}&s={{ service}}" | "123" | "web" | "https://localhost?c=123&s=web"
end end
with_them do with_them do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment