Repository navigation
Conversation
|
Thanks so much for the pull request! |
listenVsock split the CID out of the listen address and discarded it, then called vsock.Listen, which binds the context ID of the machine Telegraf runs on. An address like vsock://2:9273 came up on a different CID, nothing warned, and a scraper addressing the configured one never reached it. Bind the given CID via vsock.ListenContextID so the address decides what gets bound, and fail on startup when the CID is not ours instead of silently binding something else. An address with no CID (vsock://:9273), the documented form, keeps binding the local context ID. This completes influxdata#19774, which fixed the same defect for inputs.socket_listener through plugins/common/socket and left this plugin's own listener as a follow-up. resolves influxdata#19775
6f63a3c to
31309fe
Compare
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
|
Two jobs are red and I do not think either is from this change, so here is the evidence rather than an assertion — I cannot re-run them from my side.
A PowerShell invocation timing out on the runner. The same log shows this PR's own tests taking the skip path on Windows, which is what should happen:
I checked the one thing that could have connected them to this PR: The change itself is confined to |
Summary
outputs.prometheus_clientsplit the CID off avsock://2:9273listen address and threw it away, then calledvsock.Listen(port, nil), which binds the context ID of the machine Telegraf runs on. The listener came up on a different CID than the configuration asked for, nothing warned, and a scraper addressing the configured CID never reached it.This is the same defect #19774 fixed for
inputs.socket_listener, and that PR named this plugin as the follow-up: "outputs.prometheus_clienthas the samevsock.Listencall with its own parsing and is left for a follow-up." That plugin goes throughplugins/common/socket;prometheus_clienthas its own listener, so the fix did not reach it. This completes it.The change
Error wording matches #19774 so the two plugins fail the same way, and the README gets the same note yours did.
One deliberate difference from #19774
prometheus_clientdocuments the address aslisten = "vsock://:9273"— no CID — and for that form binding the local context ID is correct.inputs.socket_listeneralways carries a CID, so #19774 had no such case and routes everything throughListenContextID.Doing that here would have broken the documented default, so the empty host keeps using
vsock.Listenand only an explicit CID goes toListenContextID. There is a test pinning that, because it is the part of this change most likely to be undone by someone later making the two plugins "consistent".Behaviour change
As in #19774, a configuration naming a CID that does not belong to this machine now fails at startup with
EADDRNOTAVAILinstead of quietly binding a different one. That is the point — the previous behaviour was unreachable-by-design — but it is a breaking change for anyone who wrote a CID that was never honoured. CHANGELOG entry added under the existing Important Changes section, next to #19774's.Testing
Four cases in
plugins/outputs/prometheus_client/vsock_test.go, in the shape #19774 used:TestListenVsockBindsTheConfiguredContextIDlistener.Addr().(*vsock.Addr).ContextIDTestListenVsockWithoutContextIDBindsLocal:9274still binds the local CID — the documented form, and the difference from #19774TestListenVsockRejectsForeignContextID4294967294is refused withlistening on CID 4294967294 failedrather than silently rebindingTestListenVsockRejectsNonNumericContextIDfailed to parse CID hostgofmtis clean,go vet ./plugins/outputs/prometheus_client/passes, andgo test ./plugins/outputs/prometheus_client/... -count=1is green: every pre-existing test passes, the non-numeric case passes, and the three that need a vsock-capable host skip.The skip guard is tighter than #19774's, for a reason I hit
I tried to actually run the binding assertions rather than assume them, in a privileged Linux container with
/dev/vsockpresent.vsock.ContextID()succeeds there and returns4294967295—VMADDR_CID_ANY, the wildcard — because Docker Desktop's VM has no guest transport. A guard on the error alone does not fire, and the bind then fails:That is not specific to my tests. Your merged test fails the same way in the same container:
So these tests skip through
requireVsock, which also skips when the reported context ID is the wildcard. I have observed the skip path firing; the non-skip path is auint32comparison, and a real guest or host CID is never0xffffffff.plugins/common/socket/socket_test.gohas the same gap. I did not touch it — it is not what this PR is about and I would rather you decide — but say the word and I will send the same two-line guard there.What that leaves unverified
The two positive binding assertions did not execute anywhere I have access to, so the claim that
ListenContextIDbinds the CID you give it rests on the same upstream contract #19774 already relies on, not on a run I can show you.go test ./plugins/outputs/prometheus_client/ -run Vsock -von a vsock-capable guest would turn those two skips into real coverage.Checklist
Related issues
resolves #19775