Conversation
- Added a new test case for posting a configuration with a timeout duration string in `apiserver_extra_http_test.go`. - Updated `ConfigApplyRequest` struct in `apply.go` to include `mapstructure` tags for better decoding. - Improved the decoding process in `handleConfigTunnelTargetMatchesPost` to use a custom decoder with a duration hook. - Refactored the `getTunnelTargetMatch` function in `tunnel_server.go` to simplify the matching logic. - Enhanced the `ToStore` method in `config.go` to mirror tunnel server targets into the tunnel-target-matches store. - Added a regression test in `file_output_test.go` to ensure closing stdout does not affect the process's standard output.
hellt
approved these changes
Jul 5, 2026
drewelliott
added a commit
to drewelliott/gnmic
that referenced
this pull request
Jul 6, 2026
Adds a second OTLP transport that complements the existing gRPC path, plus a series of lifecycle and correctness fixes that apply to both transports. OTLP/HTTP transport ------------------- * New protocol value http (in addition to grpc). The endpoint accepts either a bare host:port (port required) or a full URL; bare endpoints are normalized to http://host:port/v1/metrics or https://host:port/v1/metrics based on whether tls is configured. * Full URLs preserve scheme and path; empty or "/" path defaults to /v1/metrics. Only http and https schemes are accepted; userinfo in URLs is rejected (auth belongs in headers). * mTLS support via the existing tls config block (ca-file, cert-file, key-file, skip-verify). http:// + a configured tls block is rejected at Init to prevent silent mTLS bypass. * Configurable per-request body compression (compression: gzip|none) for HTTP; defaults to gzip. The compression field is ignored by the gRPC transport. Content-Type and Content-Encoding headers are protocol-owned and cannot be overridden by user-supplied headers. * HTTP error classification per OTLP spec: 429/502/503/504 are retryable, all other 4xx/5xx are permanent. Transport-level errors (no HTTP status) are also retryable. * Retry-After honored on 429/503 up to a 30s cap; otherwise exponential backoff with full jitter via math/rand/v2 (no global mutex). * OTLP PartialSuccess (HTTP and gRPC) is treated as a permanent failure — per spec, retrying would duplicate the data points the server accepted. Accounting is request-level: a partially-successful batch counts toward sent events (full and partial success share one recordBatchSent path); the data-loss signal is gnmic_otlp_output_rejected_data_points_total. * HTTP redirects are never followed (CheckRedirect returns ErrUseLastResponse); a redirect response classifies as a permanent error. Go forwards user-defined headers cross-host and 307/308 replay the POST body, so following redirects would let a redirecting (or compromised) collector pull the payload and headers to another origin. * With compression: none, a user-supplied Content-Encoding header is stripped rather than passed through, so the uncompressed body is never mislabelled. * A retry sleep interrupted by cancellation (worker restart or Close) counts the batch as failed rather than silently dropping it from the accounting. Endpoint validation ------------------- * Bare endpoints (both transports) must include an explicit port via net.SplitHostPort. Missing-port or trailing-colon forms are rejected at Init rather than failing on first dial against the wrong port. * gRPC schemed forms (dns:///, unix:///, xds:///) pass through unchanged to grpc.NewClient. Config validation ----------------- * Numeric options are validated after defaulting: non-positive batch-size, interval, buffer-size, num-workers or timeout, and a negative max-retries are rejected at Init and on reload. Previously a negative value panicked at runtime (make(chan)/wg.Add at Init; make([])/time.NewTicker inside a worker goroutine, killing the whole process), silently disabled deadlines, or silently dropped every batch. Live config reload ------------------ * outputState atomically bundles (cfg, *transportState) under atomic.Pointer. sendBatch loads once and uses the captured pointer for both conversion and dispatch, so a reload cannot tear the cfg/transport pair mid-batch. * transportState holds the per-protocol client plus an inFlight WaitGroup shared across config-only reloads layered on top of the same transport. Update's rebuild path waits on the transport's inFlight before tearing down conn/Transport, eliminating wall-clock cleanup heuristics and the config-only-then-rebuild race where a fresh empty WaitGroup would have let the rebuild close a transport still in use. * Update defers dynCfg.Store and state.Swap until after all fallible work (buildEventProcessors, buildOutputState) succeeds, so a transport-build failure no longer leaves processors partially applied. * Header-only reloads take effect on the next batch without a transport rebuild — headers are read from state.cfg.Headers per request, not cached at Init. * Init reorder: register metrics, then build resource state, then start workers. A failure after metrics register triggers a deferred unregister so a subsequent Init does not collide with the registry. Per-instance metric ownership ----------------------------- * otlpOutput tracks the prometheus collectors it actually registered. A failed Init or Close that calls unregisterMetrics() removes only this instance's registrations, not collectors another OTLP output owns in a shared registry. Worker lifecycle ---------------- * worker takes its *sync.WaitGroup as a parameter rather than reading o.wg at goroutine start, eliminating a race against Update's o.wg = newWG when a not-yet-scheduled worker would otherwise call Done() on the new WaitGroup and deadlock oldWG.Wait(). * Close waits on transport.inFlight before cleanup, making it symmetric with Update's rebuild path and robust against future non-worker sendBatch callers. * Close no longer closes the event channel: the outputs manager dispatches Write/WriteEvent goroutines without synchronizing against Close, so a concurrent producer could panic the process with a send on a closed channel. Workers exit via context cancellation instead, and Write/WriteEvent drop events once state is torn down rather than dereferencing nil. Headers ------- * The headers config field attaches key/value pairs to every export request, sent as gRPC metadata when protocol is grpc and as HTTP headers when protocol is http. Useful for multi-tenant backends (Grafana Mimir/Loki/Tempo) that route via X-Scope-OrgID. * String() redacts header values (key names stay visible), so config-reload logging cannot leak credentials such as Authorization bearer tokens placed in headers. Documentation ------------- * Rewrites the OTLP output user guide with an example-first layout (gRPC, HTTP, HTTP+mTLS examples up front), a full configuration reference table, and dedicated sections for transport behavior, TLS/compression, metric naming, type detection, resource attributes, headers, and retries. * Documents the redirect policy, header-value redaction, the partial-success accounting definition (alert on the rejected data-points counter to detect data loss), and the buffer-size reload caveat. Tests ----- * mTLS test server harness shared by HTTP tests. * TLS coherence, scheme rejection, userinfo rejection, port-required, Retry-After parsing, PartialSuccess handling (HTTP and gRPC), gzip body compression with content-encoding header, race regression for worker-restart, two-step reload race regression for transportState sharing, real Update() reload-path coverage (channel swap, worker restart, header-only reload, endpoint change), Init-failure metrics unregistration, and end-to-end OTLP/HTTP delivery against a real mTLS server. * Redirect refusal (origin hit exactly once, redirect target never contacted, error permanent), case-insensitive Content-Encoding strip with compression: none, partial-success accounting (an all-rejected batch still counts as sent and is not retried), header-value redaction with copy semantics, numeric config validation table, and a concurrent WriteEvent-vs-Close race regression; the cancelled-retry test also asserts the dropped batch is counted as failed. * Rebase note: renames one of the two identically-named TestOTLP_InitCompilesCounterPatterns functions that main currently carries from the openconfig#886/openconfig#887 merge (the duplicate prevents the package's tests from compiling) and adapts the counter-patterns tests to the outputState accessor introduced here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.