feat(egress): auto-allow OTLP endpoint egress traffic - #1504
Conversation
…oup#1491) Parse OTEL_EXPORTER_OTLP_METRICS_ENDPOINT / OTEL_EXPORTER_OTLP_ENDPOINT at egress startup and inject an always-allow rule for the endpoint host so telemetry export works under the default deny-all policy without manual allowlist rules. The rule survives user policy updates and always-rule file reloads; deny.always still takes precedence.
github-actions
Bot
added
component/egress
component/ingress
size/L
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Use a Kubernetes-resolvable OTLP service name
In Kubernetes pods with the usual search domains, this single-label endpoint causes the resolver to query names such as otel-collector.<ns>.svc.cluster.local, while the auto-generated allow rule is only the exact host otel-collector; the DNS proxy returns NXDOMAIN for the search-expanded service name under default deny, so the documented example still cannot reach the collector. Use the full service DNS name in the example or teach the auto-allow logic to account for search-expanded service names.
ℹ️ 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".
Single-label names are subject to search-domain expansion; the deny-all DNS proxy answers expanded names with NXDOMAIN without fallback, so the example would not reach the collector.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2da97a8ca
ℹ️ 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".
…nsandbox-group#1491) - Auto-allow the exporter fallback node IP (HOST_IP / /etc/hostinfo) when no OTEL endpoint env var is set, so egress's own metric export is not blocked by its own deny-all chain in that configuration. - Trim the trailing root dot from FQDN endpoint hosts before building the rule so it matches DNS policy normalization.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e63b77cf0
ℹ️ 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".
Refactor the nested switch/if message handling into small helpers so the function stays under the gocognit threshold (37 > 30). The execd CI lint (installing golangci-lint@latest) fails every PR on this pre-existing issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e83886e3ea
ℹ️ 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".
…opensandbox-group#1491) The otlpmetrichttp exporter parses endpoint env vars with url.Parse and reads u.Host, so bare host:port values become opaque URLs with an empty host and are never dialed; only scheme://host URLs are valid. Restrict parseOTLPEndpoint to URL form to avoid injecting allow rules for hosts the exporter never connects to. Also skip the node-IP fallback when an endpoint env var is set but unparseable: metricsClientOptions never falls back once the env var is non-empty, so the rule would open unrelated node-IP egress.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64c15604d1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8c51c2592
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !ok { | ||
| return nil | ||
| } | ||
| rule, err := policy.ParseValidatedEgressRule(policy.ActionAllow, host) |
There was a problem hiding this comment.
When an endpoint such as http://*.example.com:4318 is configured, net/url accepts *.example.com as the hostname, but ParseValidatedEgressRule interprets that target using the policy engine's wildcard semantics. The resulting always-allow rule authorizes every subdomain of example.com, rather than only the destination the exporter attempts to dial, unexpectedly weakening the default-deny boundary; reject policy metacharacters in parsed OTLP hosts.
Useful? React with 👍 / 👎.
| if !changed { | ||
| return false, nil | ||
| } | ||
| allow = withTelemetryAllow(allow) |
There was a problem hiding this comment.
When only deny.always changes while allow.always is unchanged or absent, RefreshIfDue returns the allow slice previously stored by setAlwaysRules, which already contains the synthetic telemetry rule. This line appends the same rule again and saves the combined slice back into the loader, so every subsequent deny-only reload adds another duplicate to the proxy and effective policy; keep file-backed loader state separate from the generated overlay or deduplicate before storing it.
Useful? React with 👍 / 👎.
| When an OTLP destination is configured — the endpoint env vars below, or the | ||
| exporter fallback node IP (`HOST_IP` / `/etc/hostinfo`) when both are unset — | ||
| egress automatically injects an always-allow egress rule for that host |
There was a problem hiding this comment.
This newly documents that HOST_IP or /etc/hostinfo enables the exporter fallback when both OTEL endpoint variables are unset, but the immediately preceding configuration section still states that export is disabled in exactly that situation. Since metricsEnabled(false) does use the node-IP fallback for egress, operators may incorrectly assume metrics remain local; update the earlier description to match the behavior documented here.
AGENTS.md reference: AGENTS.md:L44-L44
Useful? React with 👍 / 👎.
| // ptyViewerHandleBinaryMessage reports stdin payloads on a read-only viewer; | ||
| // returns false when the read loop should exit. | ||
| func ptyViewerHandleBinaryMessage(data []byte, readOnlyError func() bool) bool { |
There was a problem hiding this comment.
This extraction changes execd PTY viewer code in a commit whose stated scope is egress OTLP policy handling, without contributing to that feature or its verification. Keeping this unrelated component refactor in the same change expands the regression and review surface and directly violates the repository rule against mixing unrelated component work; revert it here and submit it independently if still needed.
AGENTS.md reference: AGENTS.md:L60-L63
Useful? React with 👍 / 👎.
| if err != nil { | ||
| log.Fatalf("failed to load always allow/deny rule files: %v", err) | ||
| } | ||
| alwaysAllow = withTelemetryAllow(alwaysAllow) |
There was a problem hiding this comment.
This changes externally visible default-deny behavior by adding an implicit always-allow rule, but the published source-of-truth page docs/components/egress.md remains unchanged and still describes the always-rule layer solely in terms of operator-managed files. Operators relying on the docs site therefore cannot discover that configuring telemetry also authorizes sandbox traffic to that host on every port; document the behavior in docs/ rather than only in the component-local reference.
AGENTS.md reference: AGENTS.md:L69-L79
Useful? React with 👍 / 👎.
| return "", "", false | ||
| } | ||
| host = strings.TrimRight(strings.TrimSpace(u.Hostname()), ".") | ||
| if host == "" { |
There was a problem hiding this comment.
When a valid endpoint contains a Unicode hostname, such as https://métrics.example:4318, url.Hostname() leaves the Unicode form in the generated exact-host rule while Go's HTTP transport converts the destination to IDNA ASCII before resolving it. The DNS proxy therefore receives an xn--... query that does not match this allow rule and returns NXDOMAIN under default deny, so telemetry remains blocked. Fresh evidence beyond the addressed trailing-dot case is this HTTP transport IDNA conversion; normalize the rule host to the same ASCII form used for dialing.
Useful? React with 👍 / 👎.
| if !ok { | ||
| return nil | ||
| } | ||
| rule, err := policy.ParseValidatedEgressRule(policy.ActionAllow, host) |
There was a problem hiding this comment.
For an endpoint such as http://[::ffff:10.0.0.5]:4318, the policy parser classifies the mapped literal as IPv6 and places it in allow_v6, but Go's TCP dial path recognizes it as IPv4 and emits an IPv4 connection to 10.0.0.5. In dns+nft default-deny mode that packet misses the IPv6 allowance and is dropped. Fresh evidence beyond the addressed hostname normalization cases is this address-family mismatch; unmap IPv4-mapped literals before generating the policy target.
Useful? React with 👍 / 👎.
2 participants
Closes #1491
Why
The egress component's default policy denies all outbound traffic, so users
had to manually inject allowlist rules for the OTLP endpoint to export
metrics. Since the egress sidecar shares the sandbox network namespace, its
own metric export is blocked by its own egress chain too.
What
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT(preferred) thenOTEL_EXPORTER_OTLP_ENDPOINTat egress startup(
components/internal/telemetry/endpoint.go), supporting URL,host:port, and bare-host forms with scheme-default ports.resolved via the DNS proxy, IP rules land in the static nft allow sets)
(
components/egress/telemetry_allow.go).POST/PATCH/DELETE policy updates and always-rule file reloads; operators
can still override via
deny.always, which takes precedence.per-rule port enforcement is not added.
components/egress/docs/opentelemetry.md.Tests
components/internal/telemetry/endpoint_test.go: endpoint parsing(URL/host:port/bare host, scheme defaults, precedence, invalid input).
components/egress/telemetry_allow_test.go: rule generation, static IPsets, merge/evaluate behavior, and pass-through when unconfigured.
go test -mod=mod ./...in components/egress and components/internal allgreen (note: egress vendor/ is stale vs go.mod — pre-existing, unrelated).