Handle otlp output - #7821
Draft
MichelLosier wants to merge 9 commits into
Draft
Handle otlp output#7821MichelLosier wants to merge 9 commits into
MichelLosier wants to merge 9 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain in OTLP cleanup, credential isolation, API-key lifecycle handling, and test expectations.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds managed and external OTLP output support, including API-key lifecycle handling and exporter authentication.
Changes:
- Adds OTLP output preparation, secret resolution, and API-key management.
- Injects credentials into OTLP/OTLPHTTP exporters and filters OTLP outputs from delivered policies.
- Adds unit, API, and integration test coverage.
File summaries
| File | Summary | Final review findings |
|---|---|---|
internal/pkg/policy/policy_output.go |
OTLP preparation and API-key management | Moderate (3 votes): Cleanup is skipped when changing managed OTLP to external. Critical (3 votes): OTLP permission changes do not clean stale roles, keys, or secrets. Critical (1 vote): OTLP retirement records are not consumed by Elasticsearch-only cleanup. Critical (1 vote): OTLP keys may be routed through the wrong bulker during inactive-agent cleanup. |
internal/pkg/policy/policy_output_test.go |
Unit tests | Critical (1 vote): Mock expectations use incorrect MockBulk call arguments. |
internal/pkg/policy/policy_output_integration_test.go |
Integration coverage | No final comments. |
internal/pkg/api/handleCheckin.go |
Exporter authorization and output filtering | Moderate (2 votes): Dropping OTLP outputs leaves stale secret paths. Critical (1 vote): Shallow-cloned exporter configs can cause concurrent credential leakage and map access. Critical (1 vote): Case-variant authorization headers can leave conflicting credentials. |
internal/pkg/api/handleCheckin_test.go |
Policy delivery tests | No final comments. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+506
to
+510
| if toRetireAPIKeys != nil { | ||
| fields := map[string]any{ | ||
| dl.FieldPolicyOutputToRetireAPIKeyIDs: *toRetireAPIKeys, | ||
| } | ||
| body, err := renderUpdatePainlessScript(p.Name, fields) |
Comment on lines
+554
to
+560
| newRoles, err := mergeRoles(zlog, currentRoles, p.Role) | ||
| if err != nil { | ||
| zlog.Error().Str("apiKeyID", output.APIKeyID).Err(err).Msg("fail merging roles for OTLP key") | ||
| return err | ||
| } | ||
|
|
||
| if err = bulker.APIKeyUpdate(ctx, output.APIKeyID, newRoles.Sha2, newRoles.Raw); err != nil { |
Comment on lines
+645
to
+648
| output.Type = OutputTypeOTLP | ||
| output.APIKey = apiKeyRef | ||
| output.APIKeyID = outputAPIKey.ID | ||
| output.PermissionsHash = p.Role.Sha2 |
Comment on lines
+486
to
+488
| b.On("APIKeyRead", mock.Anything, oldKeyID, mock.Anything). | ||
| Return(&bulk.APIKeyMetadata{ID: oldKeyID, RoleDescriptors: TestPayload}, nil).Once() | ||
| b.On("APIKeyUpdate", mock.Anything, oldKeyID, mock.Anything, mock.Anything).Return(nil).Once() |
Comment on lines
+1129
to
+1134
| for name, out := range pp.Outputs { | ||
| if out.Type != policy.OutputTypeOTLP { | ||
| continue | ||
| } | ||
| delete(data.Outputs, name) | ||
| } |
This comment has been minimized.
This comment has been minimized.
MichelLosier
force-pushed
the
handle-otlp-output
branch
from
September 14, 2026 22:07
e632106 to
fa9b8b9
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings affect OTLP key cleanup, output typing, invalidation routing, secret paths, and test correctness.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
internal/pkg/api/handleCheckin.go:1203
- The OTLP output is removed after
pp.SecretKeyshas already been populated, so any secret in the output leaves anoutputs.<name>.*entry insecret_pathseven though that path is absent from the delivered policy. Filter the deleted output's secret paths here, as the code already does for stripped remote service tokens.
for name, out := range pp.Outputs {
if out.Type != policy.OutputTypeOTLP {
continue
}
delete(pp.Policy.Data.Outputs, name)
internal/pkg/policy/policy_output.go:461
- Returning before touching
agent.Outputsmeans changing a managed OTLP output to an external OTLP output (or switching to an external OTLP policy after removing another managed output) leaves the old output record, secret reference, and API key active. The external path should skip minting/resolution but still retire and remove stale managed-output entries.
if p.Role == nil {
zlog.Debug().Msg("no output permissions for OTLP output; skipping API key management")
return nil
internal/pkg/policy/policy_output.go:611
- Because
typeis persisted only when!foundOutput, reusing an existing agent output name for a managed OTLP output never writesoutputs.<name>.type = "otlp"to Elasticsearch. The agent document then continues to advertise the old output type, so subsequent check-in/ack processing classifies this OTLP key incorrectly. Persist the OTLP type for existing outputs too.
if !foundOutput {
fields[dl.FiledType] = OutputTypeOTLP
}
internal/pkg/policy/policy_output.go:648
- Marking the record as
otlpmakes it invisible to the existing API-key finalization paths:handleAck.goandhandleCheckin.goonly callupdateAPIKeyforOutputTypeElasticsearch. Permission changes therefore leave-rdstaleroles installed, and retired OTLP keys/secrets are not finalized. Include local OTLP outputs in the same cleanup flow.
output.Type = OutputTypeOTLP
output.APIKey = apiKeyRef
output.APIKeyID = outputAPIKey.ID
output.PermissionsHash = p.Role.Sha2
internal/pkg/policy/policy_output_test.go:488
MockBulk.APIKeyReadandMockBulk.APIKeyUpdaterecord only(ctx, id)(internal/pkg/testing/bulk.go:148-168), but these expectations pass three and four arguments. The hash-changed subtest will therefore fail with an unexpected mock call before it reaches the assertions.
b.On("APIKeyRead", mock.Anything, oldKeyID, mock.Anything).
Return(&bulk.APIKeyMetadata{ID: oldKeyID, RoleDescriptors: TestPayload}, nil).Once()
b.On("APIKeyUpdate", mock.Anything, oldKeyID, mock.Anything, mock.Anything).Return(nil).Once()
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
|
This pull request does not have a backport label. Could you fix it @MichelLosier? 🙏
|
Contributor
TL;DRThe failing Remediation
Investigation detailsRoot CauseFrom the available artifact ( Given Evidence
Verification
Follow-upOnce the pre-teardown failure output is available, I can classify precisely (code/test/dependency/config/infrastructure) and provide a concrete patch-level remediation. What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical OTLP API-key routing and cleanup issues remain, along with a failing mock expectation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
internal/pkg/api/handleAck.go:423
- Adding OTLP here passes
outputNameintoupdateAPIKey, which callsbulk.GetBulker(outputName)when a bulker exists. If this name previously belonged to a remote ES output, the cached remote bulker is used for the primary-cluster OTLP key; the role read/cleanup fails and merged-rdstalepermissions can remain indefinitely. Force the primary bulker for OTLP or makeupdateAPIKeytype-aware; the check-in path needs the same fix.
if output.Type != policy.OutputTypeElasticsearch && output.Type != policy.OutputTypeOTLP {
continue
}
internal/pkg/policy/policy_output.go:487
- On an mOTLP→external transition, the preceding update appends the retirement item to
outputs[p.Name], but this update immediately removes that same entry. After both requests succeed, no agent output retainsto_retire_api_key_ids, so the next ack cannot invalidate the old primary-cluster key and it remains active. Preserve the retirement item on a surviving output or retire the key explicitly before removing the entry.
body, err = renderRemoveOutputPainlessScript(p.Name)
if err != nil {
return fmt.Errorf("could not render remove-output script for mOTLP→external transition: %w", err)
}
if err = bulker.Update(ctx, dl.FleetAgents, agent.Id, body, bulk.WithRefresh(), bulk.WithRetryOnConflict(3)); err != nil {
internal/pkg/policy/policy_output.go:527
- When a removed output is OTLP, this sets
Outputto its name.invalidateAPIKeystreats a non-emptyOutputas a remote-ES key and routes it throughGetBulker(agentOutputName), so the primary-cluster OTLP key is not invalidated; the subsequent secret deletion can leave an active key without its stored secret. KeepOutputempty for retired OTLP keys.
retiring := model.ToRetireAPIKeyIdsItems{
ID: agentOutput.APIKeyID,
RetiredAt: time.Now().UTC().Format(time.RFC3339),
Output: agentOutputName,
}
internal/pkg/policy/policy_output.go:564
- This reuses any existing key, even when the same output name previously referred to Elasticsearch or remote Elasticsearch. A remote output's key is not valid on the primary cluster: if the hashes match, its credential is sent as the OTLP Authorization header; if they differ,
fetchAPIKeyRolesreads the ID from the wrong cluster and preparation fails. Treat an output-type transition as requiring a new primary-cluster key.
case output.APIKey == "":
zlog.Debug().Msg("must generate OTLP API key as it is not present")
needNewKey = true
internal/pkg/policy/policy_output.go:642
- When an existing output is converted to managed OTLP,
foundOutputis true, so this condition skips persistingtype: otlpin Elasticsearch. The in-memory pointer is changed later, but the next checkin reloads the old type; after rotating the key for the conversion it can mint another key on every policy delivery and the ACK/checkin paths keep classifying it as Elasticsearch. Persist the OTLP type whenever this conversion path writes the new key.
if !foundOutput {
fields[dl.FiledType] = OutputTypeOTLP
}
internal/pkg/policy/policy_output.go:650
- The retirement path correctly leaves
Outputempty because OTLP keys are created on the primary cluster, but the active-key path still treats every non-default output as remote inmodel.Agent.APIKeyIDs(internal/pkg/model/ext.go:62-71). A named OTLP key will therefore be sent through remote-bulker invalidation during unenrollment or inactive-agent cleanup, where no OTLP remote bulker can exist, and remain valid. Update the active-key classification so OTLP keys also use an emptyOutput.
retiring := model.ToRetireAPIKeyIdsItems{
ID: output.APIKeyID,
RetiredAt: time.Now().UTC().Format(time.RFC3339),
}
internal/pkg/policy/policy_output_test.go:488
MockBulk.APIKeyReadandAPIKeyUpdateforward only(ctx, id)to testify (internal/pkg/testing/bulk.go:148-167), but these expectations register three and four arguments. The hash-changed subtest will therefore fail on an unexpected mock call before exercising the OTLP update path. Register the two arguments passed by the mock, or update the mock and all expectations consistently.
b.On("APIKeyRead", mock.Anything, oldKeyID, mock.Anything).
Return(&bulk.APIKeyMetadata{ID: oldKeyID, RoleDescriptors: TestPayload}, nil).Once()
b.On("APIKeyUpdate", mock.Anything, oldKeyID, mock.Anything, mock.Anything).Return(nil).Once()
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+1604
to
1606
| if output.Type != policy.OutputTypeElasticsearch && output.Type != policy.OutputTypeOTLP { | ||
| continue | ||
| } |
Comment on lines
+457
to
+464
| // External OTLP output — Kibana embeds auth credentials directly in the exporter config. | ||
| // Only mOTLP outputs carry an output_permissions block and need API key management. | ||
| if p.Role == nil { | ||
| zlog.Debug().Msg("no output permissions for OTLP output; skipping API key management") | ||
| // If this output previously had a managed API key (mOTLP → external transition), | ||
| // retire it now. Without this the old key stays valid indefinitely and its | ||
| // .fleet-secrets doc is pinned alive by outputSecretIsReferenced. | ||
| if prev, ok := agent.Outputs[p.Name]; ok && prev.APIKeyID != "" { |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Address stale-output retirement and ensure OTLP API-key maintenance uses the primary bulker.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
internal/pkg/api/handleAck.go:421
- Adding OTLP to this loop routes it through
updateAPIKey, which prefersbulk.GetBulker(outputName)(seehandleAck.go:462). Remote bulkers are retained when an output changes type, so an OTLP key under a reused remote-output name can be read or cleaned on the remote cluster instead of the primary cluster. Make API-key maintenance choose the primary bulker for OTLP outputs.
for outputName, output := range agent.Outputs {
if output.Type != policy.OutputTypeElasticsearch && output.Type != policy.OutputTypeOTLP {
internal/pkg/api/handleCheckin.go:1604
- This new OTLP branch has the same routing problem in the policy-detail/check-in path:
updateAPIKeycan select a stale remote bulker solely from the output name, even though managed OTLP keys are created on the primary cluster. A remote-to-OTLP name reuse can therefore leave the primary key's roles stale or make this check-in fail. Route OTLP maintenance through the primary bulker.
for outputName, output := range agent.Outputs {
if output.Type != policy.OutputTypeElasticsearch && output.Type != policy.OutputTypeOTLP {
internal/pkg/policy/policy_output.go:300
- This assumes the previous key is an mOTLP key on the primary cluster, but a reused output name can previously refer to
remote_elasticsearch(those entries are persisted with the Elasticsearch type as well). In that transitionbulker.APIKeyInvalidatetargets the primary cluster, leaving the remote key active after the output entry is removed. Resolve the owning output bulker before invalidating the previous key.
if prev, ok := agent.Outputs[p.Name]; ok && prev.APIKeyID != "" {
internal/pkg/policy/policy_output.go:304
- When invalidation fails, this path continues to delete the secret and remove the output entry below. The old API key can therefore remain active while its only agent-document/retirement tracking is discarded, leaving an orphaned credential that later acknowledgement cleanup cannot revoke. Keep the tracking state and retry or fail the transition until invalidation succeeds.
if err := bulker.APIKeyInvalidate(ctx, prev.APIKeyID); err != nil {
zlog.Warn().Err(err).Str(ecs.APIKeyID, prev.APIKeyID).Str(ecs.PolicyOutputName, p.Name).
Msg("failed to invalidate mOTLP API key during transition to external OTLP")
}
internal/pkg/policy/policy_output.go:315
- The key and secret are destroyed before the replacement policy is delivered or acknowledged. If the update at lines 315-317, exporter preparation, or action delivery fails, the agent can remain on the old managed OTLP policy while its credential is already invalid; even on success there is a pre-ack window with the same failure. Defer invalidation and secret deletion through the existing ACK/retirement flow until the transition is confirmed.
body, err := renderRemoveOutputPainlessScript(p.Name)
if err != nil {
return fmt.Errorf("could not render remove-output script for mOTLP→external transition: %w", err)
}
if err = bulker.Update(ctx, dl.FleetAgents, agent.Id, body, bulk.WithRefresh(), bulk.WithRetryOnConflict(3)); err != nil {
internal/pkg/policy/policy_output.go:309
DeleteSecretfailures are only logged, but the output entry is then removed. After a transient deletion failure there is no agent reference, retirement record, orOutputSecretCandidateleft to trigger a retry, so the Fleet secret can be leaked indefinitely. Preserve or enqueue the cleanup before dropping the entry.
if secretID, ok := secret.ParseSecretReference(prev.APIKey); ok {
if err := bulker.DeleteSecret(ctx, secretID); err != nil {
zlog.Warn().Err(err).Str("secret.id", secretID).Str(ecs.PolicyOutputName, p.Name).
Msg("failed to delete mOTLP API key secret during transition to external OTLP")
}
internal/pkg/policy/policy_output.go:350
- Key reuse is based only on an empty
APIKeyor a matching permissions hash; it does not ensure that the stored key belongs to OTLP on the primary cluster. Reusing an entry from a priorremote_elasticsearchoutput with the same name can update a remote key through the primary bulker, or inject that remote key into OTLP when the hash happens to match. Force a new primary key when the existing output is not already an OTLP output.
switch {
case output.APIKey == "":
zlog.Debug().Msg("must generate OTLP API key as it is not present")
needNewKey = true
case p.Role.Sha2 != output.PermissionsHash:
zlog.Debug().Msg("must update OTLP API key as policy output permissions changed")
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+294
to
+295
| if p.Role == nil { | ||
| zlog.Debug().Msg("no output permissions for OTLP output; skipping API key management") |
10 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved API-key lifecycle issues and integration-test failures require fixes before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
internal/pkg/api/handleAck.go:422
- Adding OTLP outputs to this loop routes their keys through
updateAPIKey, which unconditionally prefersbulk.GetBulker(outputName). Remote bulkers are retained by name and are not removed when an output changes type, so an OTLP key whose name previously belonged to a remote Elasticsearch output can be read, updated, or retired against the stale remote cluster instead of the primary cluster. Select the bulker using the current output type or clear stale remote bulkers on type changes.
for outputName, output := range agent.Outputs {
if output.Type != policy.OutputTypeElasticsearch && output.Type != policy.OutputTypeOTLP {
continue
internal/pkg/policy/policy_output.go:298
- When the policy contains only an external OTLP output, this early return never calls
retireRemovedOutput. If the previous agent document had a different managed output (for example, a default Elasticsearch output), there is no otherPreparecall that scans the removed entries, so that API key and secret remain active indefinitely; the same-name cleanup below does not cover them. Please retire/invalidate all removed managed entries, or provide a managed surviving holder, before returning.
if p.Role == nil {
zlog.Debug().Msg("no output permissions for OTLP output; skipping API key management")
// If this output previously had a managed API key (mOTLP → external transition),
// invalidate it directly. A retirement record cannot be used here: there is no surviving
// output entry to park it on, so any record written would be deleted by
internal/pkg/policy/policy_output.go:34
- This PR introduces a new OTLP output type, API-key lifecycle, and policy-delivery behavior, but no changelog fragment was added under
changelog/fragments. The repository requires fragments for notable changes (AGENTS.md:203and.github/PULL_REQUEST_TEMPLATE.md:46); please add one or apply the documentedskip-changelogexception if this change is intentionally excluded.
OutputTypeOTLP = "otlp"
internal/pkg/policy/policy_output.go:640
- After the second update succeeds,
agent.Outputsstill containsremovedOutputName. With multiple surviving Elasticsearch/OTLP outputs, each laterPreparecall scans that stale in-memory entry and appends the same retirement record to another survivor, causing duplicate invalidation and secret-deletion attempts on ack. Remove the entry from the in-memory map after the successful remove update.
if err = bulker.Update(ctx, dl.FleetAgents, agent.Id, body, bulk.WithRefresh(), bulk.WithRetryOnConflict(3)); err != nil {
zlog.Error().Err(err).Msg("fail update agent record")
return fmt.Errorf("fail update agent record: %w", err)
}
return nil
internal/pkg/policy/policy_output.go:613
- The scan stops after the first output missing from
outputMap, so a single policy revision that removes two managed OTLP outputs retires only one key. The other agent-document entry and its API key/secret remain active until some later policy change happens to trigger another scan. Process all removed managed outputs (or otherwise persist a retryable record for each) in the same revision.
if !found {
zlog.Info().Str(ecs.APIKeyID, agentOutput.APIKeyID).Str(ecs.PolicyOutputName, agentOutputName).Msg("Output removed, will retire API key")
retiring := model.ToRetireAPIKeyIdsItems{
ID: agentOutput.APIKeyID,
RetiredAt: time.Now().UTC().Format(time.RFC3339),
Output: agentOutputName,
}
if secretID, ok := secret.ParseSecretReference(agentOutput.APIKey); ok {
retiring.SecretID = secretID
}
toRetire = &retiring
removedOutputName = agentOutputName
break
internal/pkg/policy/policy_output_integration_test.go:39
ELASTICSEARCH_HOSTSis used by the test configuration as a host URL and may already include a scheme (for examplehttps://elasticsearch:9200). Prefixing it withhttp://elastic:changeme@then produces an invalid URL such ashttp://elastic:changeme@https://elasticsearch:9200, so this invalidation check fails outside the host:port-only setup. Parse the first host and add credentials while preserving its scheme.
host := strings.SplitN(hosts, ",", 2)[0]
return "http://elastic:changeme@" + host
internal/pkg/server/otlp_output_integration_test.go:41
ELASTICSEARCH_HOSTSis used by the test configuration as a host URL and may already include a scheme (for examplehttps://elasticsearch:9200). Prefixing it withhttp://elastic:changeme@then produces an invalid URL such ashttp://elastic:changeme@https://elasticsearch:9200, so these integration checks fail outside the host:port-only setup. Parse the first host and add credentials while preserving its scheme.
host := strings.SplitN(hosts, ",", 2)[0]
return "http://elastic:changeme@" + host
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Lite
Comment on lines
+300
to
+304
| if prev, ok := agent.Outputs[p.Name]; ok && prev.APIKeyID != "" { | ||
| if err := bulker.APIKeyInvalidate(ctx, prev.APIKeyID); err != nil { | ||
| zlog.Warn().Err(err).Str(ecs.APIKeyID, prev.APIKeyID).Str(ecs.PolicyOutputName, p.Name). | ||
| Msg("failed to invalidate mOTLP API key during transition to external OTLP") | ||
| } |
Comment on lines
+301
to
+305
| if err := bulker.APIKeyInvalidate(ctx, prev.APIKeyID); err != nil { | ||
| zlog.Warn().Err(err).Str(ecs.APIKeyID, prev.APIKeyID).Str(ecs.PolicyOutputName, p.Name). | ||
| Msg("failed to invalidate mOTLP API key during transition to external OTLP") | ||
| } | ||
| if secretID, ok := secret.ParseSecretReference(prev.APIKey); ok { |
Comment on lines
+346
to
+351
| case output.APIKey == "": | ||
| zlog.Debug().Msg("must generate OTLP API key as it is not present") | ||
| needNewKey = true | ||
| case p.Role.Sha2 != output.PermissionsHash: | ||
| zlog.Debug().Msg("must update OTLP API key as policy output permissions changed") | ||
| needUpdateKey = true |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect API-key routing, retirement, cleanup, and integration-test reliability.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (8)
Previously missed (3) — in code that hasn't changed since the last review.
internal/pkg/policy/policy_output.go:640
retireRemovedOutputis called once for every surviving Elasticsearch/OTLP output fromprocessPolicy, but this function never removesremovedOutputNamefromagent.Outputs. With two surviving managed outputs, each call sees the same stale entry and appends duplicate retirement records to different outputs (and repeats the remove update). Mark the entry as processed in the in-memory agent after the second update succeeds.
internal/pkg/policy/policy_output_integration_test.go:39- This helper has the same scheme-handling bug:
ELASTICSEARCH_HOSTSmay already behttps://host:port, but the code unconditionally prefixeshttp://elastic:changeme@, producing an invalid URL and making the invalidation test fail in that supported configuration. Parse the host URL and set its userinfo instead of concatenating a second scheme.
internal/pkg/server/otlp_output_integration_test.go:41 ELASTICSEARCH_HOSTSis documented/configured elsewhere as ascheme://host:portvalue, but this helper always prependshttp://elastic:changeme@. With a valid value such ashttps://localhost:9200, it constructshttp://elastic:changeme@https://localhost:9200, so the API-key assertions cannot connect. Preserve an existing scheme (and only add credentials to the parsed URL) instead of unconditionally prefixing it.
internal/pkg/api/handleAck.go:423
- Adding OTLP to this loop routes its key through
updateAPIKey, which prefersbulk.GetBulker(outputName). If an output changes fromremote_elasticsearchto OTLP with the same name, the old remote bulker remains registered, so the primary-cluster OTLP key is read/updated against the remote cluster and the ack can fail or leave retirements unprocessed. Select the bulker from the current output type or clear stale remote bulkers on type changes.
if output.Type != policy.OutputTypeElasticsearch && output.Type != policy.OutputTypeOTLP {
continue
}
internal/pkg/api/handleCheckin.go:1607
- This new check-in path has the same output-name routing problem as the ack path:
updateAPIKeyprefers a cached remote bulker even when the current OTLP output's key was created on the primary cluster. After a remote-ES→OTLP rename using the same output name, policy processing can read/update the OTLP key on the wrong cluster and repeatedly fail to complete retirement. Make this routing type-aware or remove the stale remote bulker during the transition.
if output.Type != policy.OutputTypeElasticsearch && output.Type != policy.OutputTypeOTLP {
continue
}
if err := updateAPIKey(ctx, zlog, ct.bulker, agent.Id, output.APIKeyID, output.PermissionsHash, output.ToRetireAPIKeyIds, outputName); err != nil {
internal/pkg/policy/policy_output.go:298
- When a policy consists only of external OTLP outputs, this early return is the only preparation path for the current outputs. It never retires entries in
agent.Outputsthat belong to removed managed outputs (for example, replacing a manageddefaultoutput with an external OTLP output under a new name), so those API keys remain active and their secrets are not cleaned up. Cleanup must also cover outputs absent from the new policy when there is no managed output available to carry a retirement record.
if p.Role == nil {
zlog.Debug().Msg("no output permissions for OTLP output; skipping API key management")
// If this output previously had a managed API key (mOTLP → external transition),
// invalidate it directly. A retirement record cannot be used here: there is no surviving
// output entry to park it on, so any record written would be deleted by
internal/pkg/policy/policy_output.go:305
- If
APIKeyInvalidatefails transiently here, the code still deletes the secret and later removes the agent entry while returning nil. That loses the key ID and secret reference needed for a retry, leaving the managed API key potentially active indefinitely; retain a retirement record or fail the preparation until invalidation succeeds.
if err := bulker.APIKeyInvalidate(ctx, prev.APIKeyID); err != nil {
zlog.Warn().Err(err).Str(ecs.APIKeyID, prev.APIKeyID).Str(ecs.PolicyOutputName, p.Name).
Msg("failed to invalidate mOTLP API key during transition to external OTLP")
}
if secretID, ok := secret.ParseSecretReference(prev.APIKey); ok {
internal/pkg/policy/policy_output.go:309
- If secret deletion fails, the code still removes the output entry below, losing the only reference needed to retry cleanup and potentially orphaning the API-key secret. Do not discard the entry until this cleanup succeeds, or preserve a retirement record for retry.
if secretID, ok := secret.ParseSecretReference(prev.APIKey); ok {
if err := bulker.DeleteSecret(ctx, secretID); err != nil {
zlog.Warn().Err(err).Str("secret.id", secretID).Str(ecs.PolicyOutputName, p.Name).
Msg("failed to delete mOTLP API key secret during transition to external OTLP")
}
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+769
to
+773
| if outputBulk == nil { | ||
| outputBulk = bulk | ||
| } | ||
| if err := outputBulk.APIKeyInvalidate(ctx, outputIds...); err != nil { | ||
| zlog.Info().Err(err).Strs("ids", outputIds).Str(ecs.PolicyOutputName, outputName).Msg("Failed to invalidate API keys") |
Comment on lines
+300
to
+304
| if prev, ok := agent.Outputs[p.Name]; ok && prev.APIKeyID != "" { | ||
| if err := bulker.APIKeyInvalidate(ctx, prev.APIKeyID); err != nil { | ||
| zlog.Warn().Err(err).Str(ecs.APIKeyID, prev.APIKeyID).Str(ecs.PolicyOutputName, p.Name). | ||
| Msg("failed to invalidate mOTLP API key during transition to external OTLP") | ||
| } |
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
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.
What is the problem this PR solves?
// Please do not just reference an issue. Explain WHAT the problem this PR solves here.
How does this PR solve the problem?
// Explain HOW you solved the problem in your code. It is possible that during PR reviews this changes and then this section should be updated.
How to test this PR locally
Design Checklist
Checklist
./changelog/fragmentsusing the changelog toolRelated issues
Closes: https://github.com/elastic/ingest-dev/issues/8739