Skip to content

Commit 5c0b75b

Browse files
zvkemparielvalentin
zvkemp
and
arielvalentin
authored
fix: do not expose auth params with Redis 5 (#1370)
Additionally: - Add a redis 5 appraisal - use RedisClient in RedisClient tests Co-authored-by: Ariel Valentin <[email protected]>
1 parent 1e9853a commit 5c0b75b

File tree

4 files changed

+228
-43
lines changed

4 files changed

+228
-43
lines changed

‎instrumentation/redis/Appraisals‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,7 @@ appraise 'redis-4.x' do
44
gem 'redis-client', '~> 0.22'
55
gem 'redis', '~> 4.8'
66
end
7+
8+
appraise 'redis-5.x' do
9+
gem 'redis', '~> 5.0'
10+
end

‎instrumentation/redis/lib/opentelemetry/instrumentation/redis/middlewares/redis_client.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ def serialize_commands(commands)
5858

5959
serialized_commands = commands.map do |command|
6060
# If we receive an authentication request command we want to obfuscate it
61-
if obfuscate || command[0].match?(/\A(AUTH|HELLO)\z/)
61+
if obfuscate || command[0].match?(/\A(AUTH|HELLO)\z/i)
6262
command[0].to_s.upcase + (' ?' * (command.size - 1))
6363
else
6464
command_copy = command.dup

‎instrumentation/redis/test/opentelemetry/instrumentation/redis/patches/client_test.rb‎

Lines changed: 180 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010
require_relative '../../../../../lib/opentelemetry/instrumentation/redis/patches/redis_v4_client'
1111

1212
describe OpenTelemetry::Instrumentation::Redis::Patches::RedisV4Client do
13+
# NOTE: These tests should be run for redis v4 and redis v5, even though the patches won't be installed on v5.
14+
# Perhaps these tests should live in a different file?
1315
let(:instrumentation) { OpenTelemetry::Instrumentation::Redis::Instrumentation.instance }
1416
let(:exporter) { EXPORTER }
1517
let(:password) { 'passw0rd' }
@@ -21,12 +23,24 @@
2123
# will generate one extra span on connect because the Redis client will
2224
# send an AUTH command before doing anything else.
2325
def redis_with_auth(redis_options = {})
24-
redis_options[:password] = password
25-
redis_options[:host] = redis_host
26-
redis_options[:port] = redis_port
26+
redis_options[:password] ||= password
27+
redis_options[:host] ||= redis_host
28+
redis_options[:port] ||= redis_port
2729
Redis.new(redis_options)
2830
end
2931

32+
def redis_version
33+
Gem.loaded_specs['redis']&.version
34+
end
35+
36+
def redis_version_major
37+
redis_version&.segments&.first
38+
end
39+
40+
def redis_gte_5?
41+
redis_version_major&.>=(5)
42+
end
43+
3044
before do
3145
# ensure obfuscation is off if it was previously set in a different test
3246
config = { db_statement: :include }
@@ -95,19 +109,49 @@ def redis_with_auth(redis_options = {})
95109
end
96110

97111
it 'reflects db index' do
112+
skip if redis_gte_5?
113+
98114
redis = redis_with_auth(db: 1)
99115
redis.get('K')
100116

101117
_(exporter.finished_spans.size).must_equal 3
102118

103119
select_span = exporter.finished_spans[1]
104120
_(select_span.name).must_equal 'SELECT'
105-
_(select_span.attributes['db.system']).must_equal 'redis'
106121
_(select_span.attributes['db.statement']).must_equal('SELECT 1')
122+
123+
get_span = exporter.finished_spans.last
124+
125+
_(select_span.attributes['db.system']).must_equal 'redis'
107126
_(select_span.attributes['net.peer.name']).must_equal redis_host
108127
_(select_span.attributes['net.peer.port']).must_equal redis_port
128+
_(select_span.attributes['db.redis.database_index']).must_equal 1
109129

130+
_(get_span.name).must_equal 'GET'
131+
_(get_span.attributes['db.system']).must_equal 'redis'
132+
_(get_span.attributes['db.statement']).must_equal('GET K')
133+
_(get_span.attributes['db.redis.database_index']).must_equal 1
134+
_(get_span.attributes['net.peer.name']).must_equal redis_host
135+
_(get_span.attributes['net.peer.port']).must_equal redis_port
136+
end
137+
138+
it 'reflects db index v5' do
139+
skip unless redis_gte_5?
140+
141+
redis = redis_with_auth(db: 1)
142+
redis.get('K')
143+
144+
_(exporter.finished_spans.size).must_equal 2
145+
select_span = exporter.finished_spans.first
110146
get_span = exporter.finished_spans.last
147+
_(select_span.name).must_equal 'PIPELINED'
148+
_(select_span.attributes['db.statement']).must_equal("AUTH ?\nSELECT 1")
149+
150+
_(select_span.attributes['db.system']).must_equal 'redis'
151+
_(select_span.attributes['net.peer.name']).must_equal redis_host
152+
_(select_span.attributes['net.peer.port']).must_equal redis_port
153+
_(select_span.attributes['db.redis.database_index']).must_equal 1
154+
111155
_(get_span.name).must_equal 'GET'
112156
_(get_span.attributes['db.system']).must_equal 'redis'
113157
_(get_span.attributes['db.statement']).must_equal('GET K')
@@ -134,6 +178,8 @@ def redis_with_auth(redis_options = {})
134178
end
135179

136180
it 'records exceptions' do
181+
skip if redis_gte_5?
182+
137183
expect do
138184
redis = redis_with_auth
139185
redis.call 'THIS_IS_NOT_A_REDIS_FUNC', 'THIS_IS_NOT_A_VALID_ARG'
@@ -150,21 +196,68 @@ def redis_with_auth(redis_options = {})
150196
_(last_span.status.code).must_equal(
151197
OpenTelemetry::Trace::Status::ERROR
152198
)
199+
153200
_(last_span.status.description.tr('`', "'")).must_include(
154201
"ERR unknown command 'THIS_IS_NOT_A_REDIS_FUNC', with args beginning with: 'THIS_IS_NOT_A_VALID_ARG"
155202
)
156203
end
157204

158-
it 'records net.peer.name and net.peer.port attributes' do
205+
it 'records exceptions v5' do
206+
skip unless redis_gte_5?
207+
159208
expect do
160-
Redis.new(host: 'example.com', port: 8321, timeout: 0.01).auth(password)
161-
end.must_raise Redis::CannotConnectError
209+
redis = redis_with_auth
210+
redis.call 'THIS_IS_NOT_A_REDIS_FUNC', 'THIS_IS_NOT_A_VALID_ARG'
211+
end.must_raise Redis::CommandError
162212

163-
_(last_span.name).must_equal 'AUTH'
213+
_(exporter.finished_spans.size).must_equal 2
214+
_(last_span.name).must_equal 'THIS_IS_NOT_A_REDIS_FUNC'
164215
_(last_span.attributes['db.system']).must_equal 'redis'
165-
_(last_span.attributes['db.statement']).must_equal 'AUTH ?'
166-
_(last_span.attributes['net.peer.name']).must_equal 'example.com'
167-
_(last_span.attributes['net.peer.port']).must_equal 8321
216+
_(last_span.attributes['db.statement']).must_equal(
217+
'THIS_IS_NOT_A_REDIS_FUNC THIS_IS_NOT_A_VALID_ARG'
218+
)
219+
_(last_span.attributes['net.peer.name']).must_equal redis_host
220+
_(last_span.attributes['net.peer.port']).must_equal redis_port
221+
_(last_span.status.code).must_equal(
222+
OpenTelemetry::Trace::Status::ERROR
223+
)
224+
225+
_(last_span.status.description.tr('`', "'")).must_include(
226+
'Unhandled exception of type: RedisClient::CommandError'
227+
)
228+
end
229+
230+
it 'records net.peer.name and net.peer.port attributes' do
231+
skip if redis_gte_5?
232+
233+
client = Redis.new(host: 'example.com', port: 8321, timeout: 0.01)
234+
expect { client.auth(password) }.must_raise Redis::CannotConnectError
235+
236+
if redis_gte_5?
237+
skip(
238+
'Redis 5 is a wrapper around RedisClient, which calls' \
239+
'`ensure_connected` before any of the middlewares are invoked.' \
240+
'This is more appropriately instrumented via a `#connect` hook in the middleware.'
241+
)
242+
else
243+
_(last_span.name).must_equal 'AUTH'
244+
_(last_span.attributes['db.system']).must_equal 'redis'
245+
_(last_span.attributes['db.statement']).must_equal 'AUTH ?'
246+
_(last_span.attributes['net.peer.name']).must_equal 'example.com'
247+
_(last_span.attributes['net.peer.port']).must_equal 8321
248+
end
249+
end
250+
251+
it 'records net.peer.name and net.peer.port attributes v5' do
252+
skip unless redis_gte_5?
253+
254+
client = Redis.new(host: 'example.com', port: 8321, timeout: 0.01)
255+
expect { client.auth(password) }.must_raise Redis::CannotConnectError
256+
257+
# NOTE: Redis 5 is a wrapper around RedisClient, which calls
258+
# ensure_connected` before any of the middlewares are invoked, so we don't
259+
# have a span here. This is more appropriately instrumented via a `#connect`
260+
# hook in the middleware.
168261
end
169262

170263
it 'traces pipelined commands' do
@@ -185,10 +278,11 @@ def redis_with_auth(redis_options = {})
185278

186279
it 'traces pipelined commands on commit' do
187280
redis = redis_with_auth
188-
redis.queue([:set, 'v1', '0'])
189-
redis.queue([:incr, 'v1'])
190-
redis.queue([:get, 'v1'])
191-
redis.commit
281+
redis.pipelined do |pipeline|
282+
pipeline.set('v1', '0')
283+
pipeline.incr('v1')
284+
pipeline.get('v1')
285+
end
192286

193287
_(exporter.finished_spans.size).must_equal 2
194288
_(last_span.name).must_equal 'PIPELINED'
@@ -225,16 +319,17 @@ def redis_with_auth(redis_options = {})
225319
it 'truncates long db.statements' do
226320
redis = redis_with_auth
227321
the_long_value = 'y' * 100
228-
redis.queue([:set, 'v1', the_long_value])
229-
redis.queue([:set, 'v1', the_long_value])
230-
redis.queue([:set, 'v1', the_long_value])
231-
redis.queue([:set, 'v1', the_long_value])
232-
redis.queue([:set, 'v1', the_long_value])
233-
redis.queue([:set, 'v1', the_long_value])
234-
redis.queue([:set, 'v1', the_long_value])
235-
redis.queue([:set, 'v1', the_long_value])
236-
redis.queue([:set, 'v1', the_long_value])
237-
redis.commit
322+
redis.pipelined do |pipeline|
323+
pipeline.set('v1', the_long_value)
324+
pipeline.set('v1', the_long_value)
325+
pipeline.set('v1', the_long_value)
326+
pipeline.set('v1', the_long_value)
327+
pipeline.set('v1', the_long_value)
328+
pipeline.set('v1', the_long_value)
329+
pipeline.set('v1', the_long_value)
330+
pipeline.set('v1', the_long_value)
331+
pipeline.set('v1', the_long_value)
332+
end
238333

239334
expected_db_statement = <<~HEREDOC.chomp
240335
SET v1 yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
@@ -291,13 +386,39 @@ def redis_with_auth(redis_options = {})
291386
end
292387

293388
it 'omits db.statement attribute' do
389+
skip if redis_gte_5?
390+
391+
redis = redis_with_auth
392+
_(redis.set('K', 'xyz')).must_equal 'OK'
393+
_(redis.get('K')).must_equal 'xyz'
394+
_(exporter.finished_spans.size).must_equal 3
395+
396+
set_span = exporter.finished_spans[0]
397+
_(set_span.name).must_equal('AUTH')
398+
_(set_span.attributes['db.system']).must_equal 'redis'
399+
_(set_span.attributes).wont_include('db.statement')
400+
401+
set_span = exporter.finished_spans[1]
402+
_(set_span.name).must_equal 'SET'
403+
_(set_span.attributes['db.system']).must_equal 'redis'
404+
_(set_span.attributes).wont_include('db.statement')
405+
406+
set_span = exporter.finished_spans[2]
407+
_(set_span.name).must_equal 'GET'
408+
_(set_span.attributes['db.system']).must_equal 'redis'
409+
_(set_span.attributes).wont_include('db.statement')
410+
end
411+
412+
it 'omits db.statement attribute v5' do
413+
skip unless redis_gte_5?
414+
294415
redis = redis_with_auth
295416
_(redis.set('K', 'xyz')).must_equal 'OK'
296417
_(redis.get('K')).must_equal 'xyz'
297418
_(exporter.finished_spans.size).must_equal 3
298419

299420
set_span = exporter.finished_spans[0]
300-
_(set_span.name).must_equal 'AUTH'
421+
_(set_span.name).must_equal('PIPELINED')
301422
_(set_span.attributes['db.system']).must_equal 'redis'
302423
_(set_span.attributes).wont_include('db.statement')
303424

@@ -320,13 +441,45 @@ def redis_with_auth(redis_options = {})
320441
end
321442

322443
it 'obfuscates arguments in db.statement' do
444+
skip if redis_gte_5?
445+
446+
redis = redis_with_auth
447+
_(redis.set('K', 'xyz')).must_equal 'OK'
448+
_(redis.get('K')).must_equal 'xyz'
449+
_(exporter.finished_spans.size).must_equal 3
450+
451+
set_span = exporter.finished_spans[0]
452+
_(set_span.name).must_equal('AUTH')
453+
_(set_span.attributes['db.system']).must_equal 'redis'
454+
_(set_span.attributes['db.statement']).must_equal(
455+
'AUTH ?'
456+
)
457+
458+
set_span = exporter.finished_spans[1]
459+
_(set_span.name).must_equal 'SET'
460+
_(set_span.attributes['db.system']).must_equal 'redis'
461+
_(set_span.attributes['db.statement']).must_equal(
462+
'SET ? ?'
463+
)
464+
465+
set_span = exporter.finished_spans[2]
466+
_(set_span.name).must_equal 'GET'
467+
_(set_span.attributes['db.system']).must_equal 'redis'
468+
_(set_span.attributes['db.statement']).must_equal(
469+
'GET ?'
470+
)
471+
end
472+
473+
it 'obfuscates arguments in db.statement v5' do
474+
skip unless redis_gte_5?
475+
323476
redis = redis_with_auth
324477
_(redis.set('K', 'xyz')).must_equal 'OK'
325478
_(redis.get('K')).must_equal 'xyz'
326479
_(exporter.finished_spans.size).must_equal 3
327480

328481
set_span = exporter.finished_spans[0]
329-
_(set_span.name).must_equal 'AUTH'
482+
_(set_span.name).must_equal('PIPELINED')
330483
_(set_span.attributes['db.system']).must_equal 'redis'
331484
_(set_span.attributes['db.statement']).must_equal(
332485
'AUTH ?'

0 commit comments

Comments
 (0)