Skip to content

fix(operator): resolve_string mishandled ECS-native :jsonKey:: suffix#1288

Merged
thepagent merged 2 commits into
mainfrom
fix/telegram-secret-ecs-native-json-key
Jul 4, 2026
Merged

fix(operator): resolve_string mishandled ECS-native :jsonKey:: suffix#1288
thepagent merged 2 commits into
mainfrom
fix/telegram-secret-ecs-native-json-key

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes a real bug found within minutes of merging #1285 by re-testing oabctl apply live against a real manifest.

resolve_string (used to fetch TELEGRAM_BOT_TOKEN's plaintext value for
the new auto-setWebhook call) claimed in its doc comment that
GetSecretValue "resolves natively" an ARN carrying the ECS-only
:<jsonKey>:: suffix — that claim was simply wrong. AWS's own docs confirm
GetSecretValue's SecretId parameter only ever accepts a plain ARN or
secret name; the :<jsonKey>:: JSON-key-extraction suffix is purely an ECS
valueFrom convention, resolved by ECS itself at container launch via
register_task_definition, not something the Secrets Manager API
understands at all.

Result: any manifest using the ECS-native valueFrom ARN format for
TELEGRAM_BOT_TOKEN — which is Format 1 in the README, i.e. the more common
of the two documented formats — failed webhook registration with failed to resolve TELEGRAM_BOT_TOKEN, even though the exact same value works fine as
an ECS task-definition secret (that path goes through resolve_value_from,
which never had this bug).

TELEGRAM_BOT_TOKEN: "arn:...:secret:oab/telegram/mybot-AC80TP:TELEGRAM_BOT_TOKEN::"
                                                              └──────┬──────┘
                                                         ECS-only suffix —
                                                    GetSecretValue rejects this
                                                    as part of the SecretId

Before: resolve_string(value) → GetSecretValue(secret_id=value) → error
After:  resolve_string(value) → split_ecs_json_key_suffix(value)
                                   → GetSecretValue(secret_id=<base-arn>)
                                   → extract "TELEGRAM_BOT_TOKEN" from the
                                     returned JSON secret string manually

Fix

Added split_ecs_json_key_suffix, which detects and strips the ECS-only
suffix from an ARN-shaped value (only ARNs — a bare secret name is never
split), then resolve_string extracts the JSON key manually from the
fetched secret string — the same way it already did for the aws-sm://
form. resolve_value_from (used to build the actual ECS task definition,
where ECS does understand the suffix) is untouched and unaffected.

Testing done

cargo build --release                        # clean
cargo clippy --all-targets -- -D warnings    # clean
cargo test --release                         # 29 passed; 0 failed

3 new unit tests, including the exact real-world ARN value that surfaced
the bug (arn:aws:secretsmanager:us-east-1:903779448426:secret:oab/telegram/pahudxbot-AC80TP:TELEGRAM_BOT_TOKEN::)
as a regression test.

Live E2E: re-ran oabctl apply against the real manifest that hit this
bug (TELEGRAM_BOT_TOKEN as an ECS-native ARN). Before the fix:
⚠ Telegram webhook registration failed (apply still succeeded): failed to resolve TELEGRAM_BOT_TOKEN. After: ✓ Telegram webhook registered: Webhook was set.

Update: addressed 2 code review findings

A review flagged two real gaps in the initial split_ecs_json_key_suffix:

# Finding Verdict
1 arn:...:secret:mysecret:: gets misparsed — mysecret is treated as a json-key, but it's actually part of the secret name (a full-secret-value reference with all suffix fields empty) Real bug, cheap fix — confirmed by simulating the exact value
2 The ECS suffix isn't just :<jsonKey>:: — it's 3 positional fields (<json-key>:<version-stage>:<version-id>), unhandled here Real gap, correctly identified as a feature-boundary — verified against AWS's own ECS docs worked examples

Both fixed:

  1. Locate the unambiguous :secret: marker in the ARN and treat everything
    up to the next colon as the secret-name segment (which per AWS's docs
    never itself contains a colon) — rather than blindly rsplit-ing on the
    last colon, which can't distinguish "secret name" from "json-key" when
    the suffix fields are all empty.
  2. Parse all three positional fields; fail closed with a clear, specific
    error when version-stage/version-id is non-empty, instead of silently
    falling through to "no suffix" (which would then fail later at
    GetSecretValue with a confusing, unrelated error). oabctl's own use
    case — fetching a secret's plaintext value in-process to call a
    third-party API — never needs to pin a rotation version, so this is an
    intentional, documented limitation, not a missing feature to build out
    further right now.

7 new tests added covering both findings, using AWS's own documented
example ARN shapes (key-only, version-stage-only, version-id-only,
key+version-stage). Re-verified live against the real manifest from the
original bug report — still resolves correctly, no regression.

Found via live E2E testing right after #1285 merged: applying a real
manifest using the ECS-native valueFrom ARN format (the one the README
itself documents as Format 1) failed with 'failed to resolve
TELEGRAM_BOT_TOKEN', because resolve_string passed the full value
- including the trailing :<jsonKey>:: suffix - straight to
GetSecretValue's secret_id parameter. That suffix is purely an ECS
valueFrom convention (resolved by ECS itself at container launch, via
register_task_definition), not something the Secrets Manager API
understands; AWS docs confirm SecretId only ever accepts a plain ARN
or name. The previous doc comment's claim that 'GetSecretValue
resolves natively' was simply wrong.

Fix: add split_ecs_json_key_suffix to detect and strip the ECS-only
suffix from an ARN-shaped value, then extract the JSON key manually -
the same way the aws-sm://...#... path already did. resolve_value_from
(used for the actual ECS task definition, where ECS does understand
the suffix) is unaffected.

Verified live: re-ran oabctl apply against the real manifest
(TELEGRAM_BOT_TOKEN as an ECS-native ARN with the JSON-key suffix) -
now prints '✓ Telegram webhook registered: Webhook was set' instead of
the previous resolution failure.

Testing: cargo build/clippy --all-targets -D warnings/test clean on
macmini; 29/29 tests pass (3 new, including the exact real-world ARN
value that surfaced the bug as a regression test).
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 4, 2026 04:08
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

Two real gaps found in code review of the initial fix:

1. arn:...:secret:mysecret:: was misparsed as json-key='mysecret' when
   it's actually a full-secret-value reference where 'mysecret' is
   part of the secret name/base ARN, with all three optional ECS
   suffix fields empty. Fixed by locating the unambiguous ':secret:'
   marker and treating everything up to the next colon as the
   secret-name segment (which per AWS's own docs never contains a
   colon), rather than blindly rsplit'ing on the last colon.

2. The ECS valueFrom suffix actually has three positional fields, not
   one: <json-key>:<version-stage>:<version-id> (confirmed against
   AWS's ECS docs' worked examples for referencing a specific
   key/version). The previous implementation only handled the
   json-key-only case and silently fell through to 'no suffix' for
   any value with a non-empty version-stage or version-id, which
   would then fail at GetSecretValue with a confusing error. Now
   parses all three fields and fails closed with a clear, specific
   error when a version is pinned - oabctl's own use case (fetching a
   secret's plaintext value in-process) never needs to pin a
   rotation version, so this is an intentional, documented
   limitation rather than a silent misparse.

Testing: cargo build/clippy --all-targets -D warnings/test clean on
macmini; 33/33 tests pass (7 new, covering both findings using AWS's
own documented example ARN shapes). Re-verified live against the real
manifest from the original bug report - still resolves correctly
('Webhook is already set'), confirming no regression.
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Real bug fix with solid, well-tested ECS suffix parsing that handles all documented ARN shapes correctly.

What This PR Does

Fixes resolve_string which incorrectly passed ECS-native :<jsonKey>:: suffixed ARNs directly to GetSecretValue — an API that has no knowledge of that suffix. This broke webhook registration for manifests using the more common ECS-native valueFrom format for TELEGRAM_BOT_TOKEN.

How It Works

Adds split_ecs_json_key_suffix which:

  1. Locates the :secret: marker in the ARN to unambiguously identify the secret-name boundary
  2. Parses all three ECS positional fields (json-key, version-stage, version-id)
  3. Returns the base ARN + optional JSON key for manual extraction
  4. Fails closed with a clear error for unsupported version-stage/version-id pinning

resolve_string is refactored to a unified flow: both aws-sm:// and ECS-native formats now go through the same fetch → optional JSON-key extraction path, eliminating the duplicated branches.

Findings

# Severity Finding Location
1 🟢 Correct use of :secret: marker for unambiguous parsing — avoids the original misparsing trap where mysecret:: (empty suffix fields) was confused with a json-key secrets.rs:76-82
2 🟢 Fail-closed design for unsupported version pinning — prevents silent misresolution and gives actionable error message secrets.rs:92-97
3 🟢 Excellent test coverage — 7 tests covering the exact real-world ARN, plain ARN, bare name, empty-suffix misparsing edge case, and all three reject scenarios from AWS docs secrets.rs:119-181
4 🟢 Unified resolve_string flow eliminates code duplication between aws-sm:// and ECS-native paths secrets.rs:107-117
Baseline Check
  • PR opened: follow-up fix to feat(operator): auto-register Telegram webhook, aws-sm:// secret refs #1285 (merged)
  • Main already has: resolve_string that handles aws-sm:// and plain ARN, but incorrectly passes ECS-suffixed ARNs verbatim to GetSecretValue
  • Net-new value: correct ECS suffix stripping + JSON-key extraction, full three-field parsing with fail-closed boundary, 7 regression tests
What's Good (🟢)
  • The fix correctly identifies that the :<jsonKey>:: suffix is an ECS-only convention, not a Secrets Manager API feature — a subtle but important distinction that was wrong in the original doc comment
  • Parsing anchors on :secret: rather than naive rsplit, which prevents the mysecret:: misparsing edge case
  • The version-stage/version-id rejection is an intentional, documented limitation with a clear error path — not a missing feature
  • The PR description is exceptionally thorough, documenting the before/after behavior, the root cause, and both review findings with their resolutions
  • Live E2E verification against the exact manifest that surfaced the bug

@thepagent thepagent merged commit cca01f7 into main Jul 4, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants