fix: OTLP export silently skips when endpoints are configured but no auth token is present.#199
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughexportToHTTP now skips only when both endpoints are unset; if endpoints exist but AuthToken is empty it logs a warning and still calls httpWriter.Send, handling any returned jwt_assertion by updating in-memory config and persisting it to metadata; tests validate refresh persistence and header behavior. ChangesHTTP export authentication guard removal
Sequence DiagramsequenceDiagram
participant Exporter
participant HTTPWriter
participant MetadataDB
Exporter->>HTTPWriter: Send(payload, maybe-empty AuthToken)
HTTPWriter-->>Exporter: response + jwt_assertion (newToken)
Exporter->>MetadataDB: updateTokenInMetadata(newToken)
MetadataDB-->>Exporter: success / failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
miconeilaws
force-pushed
the
feat/custom-otlp-export
branch
from
5682feb to
33f9ba0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exporter/exporter_test.go (1)
563-590:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove obsolete test that conflicts with the new behavior.
This test expects the old behavior where
exportToHTTPwould skip whenAuthTokenis empty. However, the PR removes that early-return guard, allowing exports to proceed without an auth token when endpoints are configured. This test now conflicts with the new test at lines 647-689, which validates the same scenario (endpoints configured, no auth token) but expects the HTTP request to actually be made.The test name "skips when no auth token configured" and the comment on line 585 ("Should not error, just skip gracefully") confirm this is testing obsolete behavior that the PR explicitly changes.
🗑️ Suggested action
Remove this entire test case since it validates behavior that no longer exists after this PR's changes.
- t.Run("skips when no auth token configured", func(t *testing.T) { - cfg := &config.HealthExporterConfig{ - Interval: metav1.Duration{Duration: 1 * time.Minute}, - Timeout: metav1.Duration{Duration: 30 * time.Second}, - MetricsEndpoint: "http://example.com", - LogsEndpoint: "http://example.com", - AuthToken: "", - } - - ctx := context.Background() - exporter, err := New(ctx, WithConfig(cfg), WithMachineID("test-machine-id")) - require.NoError(t, err) - require.NotNil(t, exporter) - - he := exporter.(*healthExporter) - - healthData := &collector.HealthData{ - MachineID: "test-machine", - Timestamp: time.Now(), - } - - err = he.exportToHTTP(ctx, healthData) - require.NoError(t, err) // Should not error, just skip gracefully - - // Cleanup - err = exporter.Stop() - require.NoError(t, err) - }) -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/exporter/exporter_test.go` around lines 563 - 590, The test "skips when no auth token configured" is obsolete because exportToHTTP no longer early-returns when AuthToken is empty; remove the entire t.Run block that defines that test (including the cfg setup, New(...), the exportToHTTP call and cleanup) to avoid conflicting with the updated behavior validated later; search for the test by its name string and by references to exportToHTTP and healthExporter in this file to locate and delete it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/exporter/exporter_test.go`:
- Around line 563-590: The test "skips when no auth token configured" is
obsolete because exportToHTTP no longer early-returns when AuthToken is empty;
remove the entire t.Run block that defines that test (including the cfg setup,
New(...), the exportToHTTP call and cleanup) to avoid conflicting with the
updated behavior validated later; search for the test by its name string and by
references to exportToHTTP and healthExporter in this file to locate and delete
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7f272271-3a48-4b3c-b8e3-e8e867678b78
📒 Files selected for processing (2)
internal/exporter/exporter.gointernal/exporter/exporter_test.go
💤 Files with no reviewable changes (1)
- internal/exporter/exporter.go
miconeilaws
force-pushed
the
feat/custom-otlp-export
branch
from
33f9ba0 to
016cd2f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/exporter/exporter_test.go (2)
618-659:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify at least one request was sent in the no-auth path.
At Line 620/654, header assertions run only if the handler is hit. If export is skipped, this test still passes. Add an explicit “request observed” assertion.
Suggested request-observed guard
t.Run("exports without auth token when endpoints configured", func(t *testing.T) { + requestObserved := make(chan struct{}, 1) // Create mock server that accepts requests without Authorization header server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case requestObserved <- struct{}{}: + default: + } assert.Equal(t, http.MethodPost, r.Method) assert.Empty(t, r.Header.Get("Authorization"), "should not send Authorization header when token is empty") w.WriteHeader(http.StatusOK) })) defer server.Close() @@ err = he.exportToHTTP(ctx, healthData) require.NoError(t, err) + select { + case <-requestObserved: + default: + t.Fatal("expected at least one HTTP export request") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/exporter/exporter_test.go` around lines 618 - 659, The test currently only asserts headers inside the httptest handler which will silently pass if no request is made; modify the test for the "exports without auth token when endpoints configured" case to record that the handler was hit (e.g., increment an atomic counter or send a value on a channel from the httptest handler) and after calling he.exportToHTTP(ctx, healthData) assert that at least one request was observed; locate the handler closure created for server := httptest.NewServer(...) and the export call to healthExporter.exportToHTTP to add the observation mechanism and final assertion, then run exporter.Stop() as before.
563-616:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert JWT refresh outcomes, not just success.
At Line 610 the test only verifies
exportToHTTPreturns no error; it never checks that the new JWT was persisted or reflected in config, so a broken refresh path can still pass.Suggested test assertions
err = he.exportToHTTP(ctx, healthData) require.NoError(t, err) + + storedToken, err := pkgmetadata.ReadMetadata(ctx, tmpDB, pkgmetadata.MetadataKeyToken) + require.NoError(t, err) + assert.Equal(t, newToken, storedToken) + assert.Equal(t, newToken, he.options.config.AuthToken)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/exporter/exporter_test.go` around lines 563 - 616, The test currently only checks exportToHTTP returned no error; update it to also assert the JWT was refreshed: after calling he.exportToHTTP(ctx, healthData) assert that the active token was replaced with newToken (e.g. check cfg.AuthToken or he.config.AuthToken) and, if the implementation persists tokens to the DB, query the temporary DB (tmpDB) to verify the stored token equals newToken; include these assertions before exporter.Stop() so the test fails if the refresh path did not persist the new JWT.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/exporter/exporter_test.go`:
- Around line 618-659: The test currently only asserts headers inside the
httptest handler which will silently pass if no request is made; modify the test
for the "exports without auth token when endpoints configured" case to record
that the handler was hit (e.g., increment an atomic counter or send a value on a
channel from the httptest handler) and after calling he.exportToHTTP(ctx,
healthData) assert that at least one request was observed; locate the handler
closure created for server := httptest.NewServer(...) and the export call to
healthExporter.exportToHTTP to add the observation mechanism and final
assertion, then run exporter.Stop() as before.
- Around line 563-616: The test currently only checks exportToHTTP returned no
error; update it to also assert the JWT was refreshed: after calling
he.exportToHTTP(ctx, healthData) assert that the active token was replaced with
newToken (e.g. check cfg.AuthToken or he.config.AuthToken) and, if the
implementation persists tokens to the DB, query the temporary DB (tmpDB) to
verify the stored token equals newToken; include these assertions before
exporter.Stop() so the test fails if the refresh path did not persist the new
JWT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9037926c-15d8-48af-b0de-d8494706b0ae
📒 Files selected for processing (2)
internal/exporter/exporter.gointernal/exporter/exporter_test.go
💤 Files with no reviewable changes (1)
- internal/exporter/exporter.go
miconeilaws
force-pushed
the
feat/custom-otlp-export
branch
from
016cd2f to
fdad7ae
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exporter/exporter_test.go (1)
568-620:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the actual JWT refresh response header key expected by the HTTP writer.
At Line 569, the test sets
X-New-JWT-Token, but the writer readsjwt_assertion(seeinternal/exporter/writer/http.go:273-283). This prevents the refresh path from being exercised as intended and can break the assertions at Lines 615 and 620.Suggested fix
- server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("X-New-JWT-Token", newToken) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("jwt_assertion", newToken) w.WriteHeader(http.StatusOK) _, _ = w.Write([]byte("SUCCESS")) }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/exporter/exporter_test.go` around lines 568 - 620, The test fails to exercise the JWT refresh path because the fake server sets header "X-New-JWT-Token" while the HTTP writer expects "jwt_assertion"; update the test server response in exporter_test.go (the handler used by New/WithConfig and exportToHTTP via healthExporter) to set the header key "jwt_assertion" (or the shared constant if one exists) with newToken so exportToHTTP and the underlying HTTP writer will pick up and persist the refreshed token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/exporter/exporter_test.go`:
- Around line 568-620: The test fails to exercise the JWT refresh path because
the fake server sets header "X-New-JWT-Token" while the HTTP writer expects
"jwt_assertion"; update the test server response in exporter_test.go (the
handler used by New/WithConfig and exportToHTTP via healthExporter) to set the
header key "jwt_assertion" (or the shared constant if one exists) with newToken
so exportToHTTP and the underlying HTTP writer will pick up and persist the
refreshed token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 300bdae6-9d2b-42ad-816e-88afff46a09c
📒 Files selected for processing (2)
internal/exporter/exporter.gointernal/exporter/exporter_test.go
💤 Files with no reviewable changes (1)
- internal/exporter/exporter.go
miconeilaws
force-pushed
the
feat/custom-otlp-export
branch
from
fdad7ae to
62856ed
Compare
|
@miconeilaws The change looks good to me. Could you please rebase this branch on the latest main? |
miconeilaws
force-pushed
the
feat/custom-otlp-export
branch
from
62856ed to
aa014dc
Compare
|
Thanks guys, rebased and kept the warning log. I'm trying to forward the telemetry to my existing OTel node-local collector so I can ingest it in my existing telemetry pipeline. Ideally I would also want a TLS config for auth as thats what I do for other things like scraping prometheus endpoints. I did evaluate the prometheus option here but it doesn't give me the health classifications as those seem to get emitted as logs. No auth is fine for some prototyping for now. |
|
@jingxiang-z @rsampaio let me know of any other blockers |
|
@miconeilaws Thanks. Could you rebase the latest main to include the security fix? Also could you change the PR title to be fix: ... |
miconeilaws
changed the title
… auth token Replace the hard AuthToken == "" early return with a warning log. The HTTP writer already handles empty tokens correctly by omitting the Authorization header, so non-enrolled users can now push metrics and logs to any OTLP-compatible endpoint when endpoints are configured. Signed-off-by: Michael O'Neill <miconeil@amazon.com>
miconeilaws
force-pushed
the
feat/custom-otlp-export
branch
from
aa014dc to
d5fd340
Compare
|
@jingxiang-z done, thanks a lot! |
|
@miconeilaws Thanks for your contribution! Approved and merged. |
3 participants
Description
Bug: OTLP export silently skips when endpoints are configured but no auth token is present.
The README lists OTLP remote export as a supported format and the metadata store allows configuring metrics_endpoint/logs_endpoint independently. However, exportToHTTP has an early return when AuthToken == "" that silently drops all data even though the HTTP writer already handles missing tokens correctly (just omits the header).
This removes the redundant gate so the export path works as documented when endpoints are configured.
Checklist
Summary by CodeRabbit
New Features
Tests