feat(etcd): periodic auth token refresh to prevent expiry - #763
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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 | ✅ 6✅ Passed checks (6 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. |
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