feat(egress): count DNS resolution and nftables update failures

[ferponse]

Fixes #1406. Fixes #1407.

The egress had no metric for either of its two ways of failing. Both were visible only in logs.

DNS — egress.dns.query.failed_total{reason}

serveDNS recorded the same latency sample whether forward succeeded or returned an error, and nothing else. So a sandbox that cannot resolve anything because its resolver chain is down looked identical to a healthy one.

Worse, the nearest-looking metric means the opposite. egress.policy.denied_total counts the policy working as designed; there was nothing counting the sidecar failing to do its job. The two situations invite opposite responses and were indistinguishable.

forward already tracked why each upstream failed and discarded it on the way out. It now returns a bounded reason:

reason	Set when
no_upstreams	no resolvers configured or discovered
upstream_error	every Exchange failed (network, timeout)
empty_response	a resolver returned a nil message
rcode	the last resolver answered with a failover-worthy rcode, e.g. SERVFAIL

nftables — egress.nftables.updates.failed_total{operation}

RecordNftablesUpdate was only ever reached on success (if err == nil in AddResolvedIPs; after the run in ApplyStatic; nowhere in RemoveEnforcement).

dynamic_add is the case worth alerting on: it adds the IPs behind an allowed domain to the dynamic allow set, so when it fails the chain drops traffic the policy permits. Inside the sandbox that is indistinguishable from a denial, and outside it nothing moves at all — not policy.denied_total (it was not a policy decision) and not nftables.updates.count (there was no success). A silent fail-closed outage.

Design notes

• New counters, not an outcome attribute on the existing ones: both their descriptions already promise successes, so adding a failure dimension would silently change what existing series mean.
• Closed attribute sets. Four reasons, three operations. No queried names, no error strings — cardinality is fixed regardless of what the workload does, which matters because these attributes ride alongside a per-sandbox sandbox_id.
• egressMetricOptWith copies the shared attribute slice rather than appending to it. That slice comes from a sync.OnceValue and can have spare capacity, so append would write into the shared backing array and leak one call's reason into another's. There is a test for exactly this.

Testing

go test ./... green across the egress module, go vet and gofmt clean, builds for linux/amd64.

Two things worth flagging about the tests, because the first version of them was wrong:

• The failure-classification subtests dial loopback, and the proxy's dialer sets SO_MARK, which returns EPERM without CAP_NET_ADMIN. My "every upstream unreachable" case therefore passed for the wrong reason — upstream_error from the mark, not from the unreachable port. Both subtests now exempt loopback (EnvNameserverExempt), the same way the existing TestForwardAddsEDNS0BufferSize does, so they exercise the path they claim to.
• The rcode subtest waits on NotifyStartedFunc instead of trusting the goroutine scheduler, so it cannot flake into upstream_error.

Note on overlap

Touches pkg/telemetry/metrics.go, as do my #1405 and the follow-up for #1409. They are independent in substance; whichever merges first leaves the others needing a trivial rebase in the instrument-registration block.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7b1a8c9694

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[ferponse]

Correct — fixed in afd932f.

docs/components/egress.md names no metrics at all today, and nothing in docs/ links to components/egress/docs/opentelemetry.md, so on the published site these counters and their alerting guidance would simply not exist.

What I put in docs/ is the denied vs failed distinction, since that is the part that changes what an operator does: the two counters look similar and mean opposite things, and reading one for the other inverts the diagnosis. Plus the note that dynamic_add is the failure worth alerting on. The full inventory stays in the component page, which docs/ now points at — duplicating the table would guarantee one of the two goes stale.

Same fix applied to my other two open metric PRs (#1405, #1411).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: afd932f743

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[ferponse]

Sharp catch, and correct — fixed in 1f04758.

You're right that this made the counter worse than useless: setupNft calls log.Fatalf, Fatalf calls os.Exit(1) (pkg/log/logger.go:51-55), and that skips main's deferred otelShutdown. With metrics leaving through a PeriodicReader, the sample was recorded into the SDK and then discarded on exit — so static_apply failures would have looked covered while never reaching the backend.

Added ForceFlush to the shared telemetry package — a no-op when metrics are disabled, so dns-only deployments and tests are unaffected — and the static-apply path now flushes with a 3s timeout before exiting. Runtime policy updates are untouched; they keep exporting on the normal interval, as you noted.

I chose flushing over returning the error and letting main unwind. Returning it would be tidier in the abstract, but a sidecar that cannot install its policy must not keep running: it shares a netns with an untrusted container, and continuing would mean serving DNS with no enforcement behind it. Dying is the right behaviour; the fix is making sure the reason gets out first.

Documented in the component page, since "this counter is only useful because that path flushes" is exactly the kind of thing that gets silently broken later.

[Pangjiping]

Please resolve conflicts

[ferponse]

Done — conflicts resolved in 4b70b47.

They came from #1405 landing, which I had flagged in the description: both PRs touch the instrument registration block and the same docs sections. Nothing semantic was in dispute — each side had added a different section at the same anchor — so everything from both is kept:

• pkg/telemetry/metrics_test.go — the two tests had been spliced into each other because they share the ManualReader boilerplate. Separated back into TestDNSQueryDurationBucketsSpanRealisticLatencies (yours, from fix(egress): give the DNS latency histogram buckets that match its unit #1405) and TestFailureCountersCarryBoundedAttributeWithoutSharingState (this PR), each with its own helper.
• components/egress/docs/opentelemetry.md and docs/components/egress.md — bucket section first, failure signals after it.

One thing the merge surfaced that I fixed rather than just resolved: counterByAttr asserted a hardcoded sandbox_id, which only worked while this was the only test recording. egressSharedAttrs is a sync.OnceValue, so whichever test records first fixes the attributes for the whole process — with both tests in the same package, that assertion depended on test order. It now compares against egressSharedAttrs() itself, so it holds regardless of order and still catches the aliasing bug it was written for.

Verified on the merged tree: go build for linux/amd64, go vet and gofmt clean, and the full egress module green with both tests passing in the same process.

I also merged main into #1411, which had the same conflict for the same reason. Both are MERGEABLE again now.

And thanks for the #1405 review and merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1569f3f253

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".