Skip to content

Commit 30a3a1c

Browse files
BridgeARrochdev
authored andcommitted
fix(grpc): require a colon and a strictly numeric tail before tagging… (#8378)
`finish` parsed the peer string with `peer.split(':') + parts.at(-1)` and tagged `network.destination.port` whenever the last segment matched the unanchored `/^\d+/`. Two malformed-peer shapes slipped through: 1. Partially-numeric tails — e.g. `'1.2.3.4:80abc'` matched the leading `80` and tagged a mangled `port='80abc'`. 2. Pure-digit peers without a colon — e.g. `'8080'` tagged `ip='', port='8080'`, leaking an empty ip into peer-service resolution. Use `peer.lastIndexOf(':')` plus an anchored `/^\d+$/` on the tail; both shapes now fall through to the catch-all branch that tags the raw peer as `network.destination.ip`. The hot path also stops allocating an intermediate `parts` array per call. Bench (Node 24.13 / V8 13.6, n=200 k+ x 7 trials, drop best+worst): split + slice + at(-1) 360.06 ns/op lastIndexOf + slice 166.78 ns/op speedup: 2.16x
1 parent d7be74e commit 30a3a1c

2 files changed

Lines changed: 91 additions & 6 deletions

File tree

packages/datadog-plugin-grpc/src/client.js

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,12 +86,11 @@ class GrpcClientPlugin extends ClientPlugin {
8686
// The only scheme we want to support here is ipv[46]:port, although
8787
// more are supported by the library
8888
// https://github.com/grpc/grpc/blob/v1.60.0/doc/naming.md
89-
const parts = peer.split(':')
90-
if (/^\d+/.test(parts.at(-1))) {
91-
const port = parts.at(-1)
92-
const ip = parts.slice(0, -1).join(':')
93-
span.setTag('network.destination.ip', ip)
94-
span.setTag('network.destination.port', port)
89+
const colonIndex = peer.lastIndexOf(':')
90+
const tail = colonIndex === -1 ? '' : peer.slice(colonIndex + 1)
91+
if (tail && /^\d+$/.test(tail)) {
92+
span.setTag('network.destination.ip', peer.slice(0, colonIndex))
93+
span.setTag('network.destination.port', tail)
9594
} else {
9695
span.setTag('network.destination.ip', peer)
9796
}
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
'use strict'
2+
3+
const assert = require('node:assert/strict')
4+
5+
const { describe, it } = require('mocha')
6+
7+
require('../../dd-trace/test/setup/core')
8+
9+
const GrpcClientPlugin = require('../src/client')
10+
11+
// Exercise the peer-string parser inside `GrpcClientPlugin.prototype.finish`
12+
// directly via `.call(fakeThis, ...)`. The tightened parser only emits
13+
// `network.destination.port` when the last segment is strictly numeric
14+
// *and* a colon is present; the previous `/^\d+/` and `split(':')` shape
15+
// let two malformed-peer cases through.
16+
function tagsFor (peer) {
17+
const tags = {}
18+
const fakeSpan = {
19+
setTag (key, value) { tags[key] = value },
20+
finish () {},
21+
}
22+
const fakePlugin = {
23+
config: {},
24+
addCode () {},
25+
tagPeerService () {},
26+
}
27+
GrpcClientPlugin.prototype.finish.call(fakePlugin, { span: fakeSpan, result: {}, peer })
28+
return tags
29+
}
30+
31+
describe('grpc client finish peer-string tags', () => {
32+
it('splits a standard `ipv4:port` peer', () => {
33+
assert.deepStrictEqual(tagsFor('127.0.0.1:50051'), {
34+
'network.destination.ip': '127.0.0.1',
35+
'network.destination.port': '50051',
36+
})
37+
})
38+
39+
it('splits an IPv6-style peer on the last colon only', () => {
40+
assert.deepStrictEqual(tagsFor('[::1]:50051'), {
41+
'network.destination.ip': '[::1]',
42+
'network.destination.port': '50051',
43+
})
44+
assert.deepStrictEqual(tagsFor('::1:50051'), {
45+
'network.destination.ip': '::1',
46+
'network.destination.port': '50051',
47+
})
48+
})
49+
50+
it('drops the port tag when the trailing segment is only partially numeric', () => {
51+
// Regression: `/^\d+/` was unanchored, so the previous parser tagged
52+
// `port='80abc'` and `ip='1.2.3.4'`. The anchored `/^\d+$/` rejects
53+
// the entire tail and falls back to tagging the raw peer.
54+
assert.deepStrictEqual(tagsFor('1.2.3.4:80abc'), {
55+
'network.destination.ip': '1.2.3.4:80abc',
56+
})
57+
})
58+
59+
it('drops the port tag for pure-digit peers without a colon', () => {
60+
// Regression: `'8080'.split(':') === ['8080']`, the unanchored regex
61+
// matched, and `parts.slice(0, -1).join(':')` produced an empty `ip`,
62+
// so a peer with no host info leaked `ip=''` plus a numeric `port`.
63+
assert.deepStrictEqual(tagsFor('8080'), { 'network.destination.ip': '8080' })
64+
assert.deepStrictEqual(tagsFor('12abc'), { 'network.destination.ip': '12abc' })
65+
})
66+
67+
it('tags the raw peer for unix-socket peers', () => {
68+
assert.deepStrictEqual(tagsFor('unix:'), { 'network.destination.ip': 'unix:' })
69+
assert.deepStrictEqual(tagsFor('unix:/tmp/socket'), { 'network.destination.ip': 'unix:/tmp/socket' })
70+
})
71+
72+
it('tags the raw peer when there is no colon and no digits', () => {
73+
assert.deepStrictEqual(tagsFor('localhost'), { 'network.destination.ip': 'localhost' })
74+
})
75+
76+
it('still tags an empty ip when the host half is empty (existing behaviour)', () => {
77+
// Boundary: the parser does not require a non-empty *host* — a malformed
78+
// peer with an empty host but a strictly numeric port still yields the
79+
// (empty, numeric) split. This matches both the previous and the new
80+
// parser; pinning it so a future tightening does not drop it silently.
81+
assert.deepStrictEqual(tagsFor(':50051'), {
82+
'network.destination.ip': '',
83+
'network.destination.port': '50051',
84+
})
85+
})
86+
})

0 commit comments

Comments
 (0)