feat(etcd): periodic auth token refresh to prevent expiry - #763
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:
📝 WalkthroughWalkthroughThe change adds configurable etcd authentication-token refresh, updates provider and application wiring, upgrades Changesetcd authentication refresh
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant EtcdConfigProvider
participant EtcdClient
participant AdminStore
Server->>EtcdConfigProvider: connect with refresh interval
EtcdConfigProvider->>EtcdClient: authenticate and start refresh task
EtcdConfigProvider->>AdminStore: provide connected client
AdminStore->>EtcdClient: write admin configuration
EtcdConfigProvider->>EtcdClient: refresh token periodically
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for working on this — the underlying issue is real, and periodically refreshing credential-based etcd auth is valuable. The overall direction looks reasonable. Before this is ready to merge, please address the following blocking items:
After updating the branch, please ensure the new fork workflow run is approved and actually executes all jobs. The current workflow runs are cargo fmt --all -- --check
cargo clippy --workspace --all-targets -- -D warnings
cargo test --workspace
cargo check --workspaceOnce these items are addressed, this PR should be worth proceeding with. |
|
@moonming Thanks for the detailed review! All blocking items have been addressed — compilation fix, retry logic, watch path coverage, and sync with main. Please take another look. |
|
Thanks for this, and sorry it sat — the diagnosis here is right and the reproduction with a short We're going to fix it a different way though, and I'd rather explain than quietly supersede you. The case that pushed us elsewhere is an etcd restart rather than a TTL expiry. A restart invalidates the token immediately, so a scheduled refresh only recovers at its next tick — up to 240s of failing calls with the default here. Reacting to the Two smaller things pointed the same way. The 240s default is really a client-side copy of etcd's own The 0.14 → 0.19 bump is also more awkward now than it was in July: #1134 and #1135 both changed the watch path, and the merged What we are keeping is your test setup — the auth-enabled etcd service container with the short TTL, and the integration tests built on it. That infrastructure didn't exist in this repo before your PR and it's what any fix needs to prove itself against, so it's going in with attribution to this PR. I'll link the replacement here once it's up so you can see whether it actually handles your case; if it misses something you hit in production, please say so on it. |
|
Opened #1136 with the reactive version of this — on Not carried over: One thing that came out of building it, in case it's useful: restarting etcd 3.5 does not by itself invalidate a simple token — |
…ken (#1136) `etcd-client` authenticates once, inside `Client::connect`, and never again, so the token that connection carries is the only one it will ever have. etcd stops accepting it in two ordinary situations — the token's lifetime elapses, or the auth store's revision changes, which under `--auth-token jwt` happens on any `etcdctl user add` or permission edit — and every later call was then refused until the gateway was restarted. An etcd restart is deliberately not on that list: `Authenticate` is a raft entry, so replaying the WAL re-registers the tokens it minted. Recovery is reactive. `LazyEtcdClient::call` runs one etcd call and, if etcd's answer says the token is stale, discards the connection, dials a new one — which re-runs `Authenticate` — and retries the call once. Both etcd consumers go through it: the configuration provider's range read and watch create, and the admin GET surface's reads. Nothing runs on a timer and no configuration key is added. Credentials etcd has refused cannot reach that path. `authentication failed, invalid user ID or password`, `PermissionDenied` and `FailedPrecondition` stay `Rejected` and fail on the first answer, and the retry is bounded at one attempt. The two stale-token answers that share `InvalidArgument` with a wrong password are told apart by etcd's message, which is how etcd's own reference client recovers them; the constant says so, so it is not later "cleaned up" into a code check. A token refused on a freshly authenticated connection reports as `TokenRefused`, with a line that points at the auth store or the clock rather than at a password that is fine. Separately, a boot dial that has not finished now says so every 10s. With `dial_timeout_ms` unset an endpoint that accepts TCP and answers nothing left the gateway stuck before any listener bound, writing nothing at all. No timeout semantics change: unset and `0` still mean unbounded. The authenticated short-lifetime etcd in CI and the approach of asserting recovery against a real expiry come from community PR #763 by @okaybase. Its scheduled-refresh fix, config field and dependency bump are not carried over.
5 participants
Summary
The etcd auth token acquired at connect time was never refreshed. Once it expired (default 300s
--auth-token-ttl), every subsequent Admin API write and etcd watch operation failed withetcdserver: invalid auth token.Changes
aisix-etcd: addstart_token_refresh_task— a background Tokio task that callsClient::refresh_tokenon a configurable interval (default 240s, leaving a 60s margin below etcd's default 300s TTL). No-ops when no credentials are configured, or when called outside a Tokio runtime.aisix-core: addEtcdConfig.auth_token_refresh_secsfield (default 240). Documented inconfig.example.yamlandconfig.managed.yaml.aisix-server: threadauth_token_refresh_secsfrom config throughEtcdConfigProvider::connectandEtcdConfigStore::new.aisix-admin:EtcdConfigStore::newnow spawns the refresh task for the admin write-path client, preventing Admin API writes from failing after token expiry.etcd-clientfrom 0.14 to 0.19 to pick upClient::refresh_tokenand the mergedWatchStream(no longer requires a separateWatcherhandle).admin_write_survives_token_expiryandtoken_refresh_prevents_auth_expiryregression tests against an auth-enabled etcd with a 5s TTL, exercising the refresh loop.etcd-authservice container (short TTL) and "Enable auth on etcd-auth" step to run the token-refresh tests.Verification
cargo test --workspacepassescargo clippy --workspace -- -D warningscleanrust-unitjob runs the new token-refresh tests against the auth-enabled etcd containeretcd.user+password_env, verify Admin API writes still succeed after 5+ minutesSummary by CodeRabbit
New Features
Bug Fixes
Tests