membphis commented on PR #13707: URL: https://github.com/apache/apisix/pull/13707#issuecomment-6407680905
I found three issues that should be addressed before merging. Reviewed head: [`b6ef18c7d85f`](https://github.com/apache/apisix/commit/b6ef18c7d85f704be7c4ab7599eb7ecbd6f5f83c). Comparison baseline: [`5dccf0e04977`](https://github.com/apache/apisix/commit/5dccf0e04977a1a31d46eade07d4a5ca2fca4beb). Validation scope: targeted LuaJIT probes loaded the unmodified `consumer.lua` from each commit. Configuration callbacks, cache storage, and secret responses were stubbed; a coroutine suspension simulated a yielding remote-secret read. These results are module-level reproductions, not full gateway/Vault integration tests. ### [P1] An older secret lookup can restore a superseded authentication key **Trigger:** two consumer updates overlap while the first request is resolving a remote secret. **Reproduction:** 1. Warm the consumer key map with an initial key. 2. Apply version 2, whose key resolves from a remote secret to `new1`. 3. Start request A and suspend it inside secret resolution. 4. Apply version 3 with `new2`; let request B finish updating the index and authenticate with `new2`. 5. Resume request A, then try `new1` again. 6. Invoke the background consistency callback and retry `new1`. **Result:** | Lookup after version 3 is applied | Baseline | PR | | --- | --- | --- | | Superseded key `new1` | Rejected | Accepted | | Superseded key after the consistency callback | Rejected | Still accepted | | Current key `new2` | Accepted | Accepted | The shared nodes/index have already been modified when [`kv_upsert()` calls `secret.fetch_secrets(..., false)`](https://github.com/apache/apisix/blob/b6ef18c7d85f704be7c4ab7599eb7ecbd6f5f83c/apisix/consumer.lua#L238-L254). A newer update can finish while that call yields. The older coroutine then writes its result into the same `pd.kv` without checking the entity revision. The final version bookkeeping can mark this mixed state as current. The consistency callback checks IDs, so it does not detect the orphaned key; it can remain accepted until the key map is rebuilt on TTL expiry. **Suggested fix:** resolve secrets before publishing shared state, validate the captured entity/config revision before committing the result, and prevent concurrent incremental applications from publishing stale results or consuming each other's pending changes. Add a regression with this coroutine interleaving. ### [P1] Snapshot replacement leaves deleted consumers authenticating until the background check **Trigger:** the index is already initialized, then a YAML/JSON replacement or an etcd full resynchronization removes a consumer without delivering an individual value-less deletion callback. **Reproduction:** 1. Warm the map and verify that Alice's key authenticates. 2. Replace the configuration snapshot with one that omits Alice and advance `conf_version`. 3. Deliver filters only for surviving records, as the snapshot-loading paths do. 4. Authenticate with Alice's deleted key before running the consistency callback. 5. Run the callback and retry. **Result:** | Deleted-key lookup | Baseline | PR | | --- | --- | --- | | First lookup after the snapshot/version change | Rejected | Accepted | | After the consistency callback | Rejected | Rejected | [Deletion reconciliation runs only when `pending_delete` is set](https://github.com/apache/apisix/blob/b6ef18c7d85f704be7c4ab7599eb7ecbd6f5f83c/apisix/consumer.lua#L405-L424). That flag depends on a value-less filter event. [YAML/JSON replacement removes old records without that event](https://github.com/apache/apisix/blob/b6ef18c7d85f704be7c4ab7599eb7ecbd6f5f83c/apisix/core/config_yaml.lua#L247-L262), and [etcd full loading filters the records present in the new snapshot](https://github.com/apache/apisix/blob/b6ef18c7d85f704be7c4ab7599eb7ecbd6f5f83c/apisix/core/config_etcd.lua#L598-L639). Advancing the version therefore does not necessarily remove stale auth entries. The 30-second timer eventually repairs this, but immediate revocation has regressed. **Suggested fix:** supply deletion notifications or an explicit reconciliation signal for snapshot replacement, and complete the removal before serving authentication against the new version. Add snapshot-deletion coverage, including a replacement where other consumers survive. ### [P2] A request now performs remote-secret reads for unrelated authentication plugins **Trigger:** both key-auth and basic-auth maps have been warmed; a basic-auth consumer's Vault-backed password changes; the next request uses key-auth. **Reproduction:** 1. Create a plaintext key-auth consumer and one or two basic-auth consumers, each with one remote-secret password. 2. Warm both plugin maps, then reset the secret-resolution counter. 3. Update only the basic-auth passwords and deliver their configuration callbacks. 4. Make one lookup for the unchanged key-auth consumer. 5. Count secret resolutions performed by that key-auth lookup. **Result:** | Pending basic-auth updates | Baseline secret resolutions | PR secret resolutions | | --- | --- | --- | | 1 | 0 | 1 | | 2 | 0 | 2 | These are counted resolver invocations in the probe. For Vault, each uncached reference follows [`vault.get() -> make_request_to_vault() -> httpc:request_uri()`](https://github.com/apache/apisix/blob/b6ef18c7d85f704be7c4ab7599eb7ecbd6f5f83c/apisix/secret/vault.lua#L51-L106), so the additional resolutions introduce outbound HTTP calls on an otherwise unaffected request. [`apply_incremental()`](https://github.com/apache/apisix/blob/b6ef18c7d85f704be7c4ab7599eb7ecbd6f5f83c/apisix/consumer.lua#L378-L403) processes all pending entities, and [`add_consumer_entry()` eagerly calls `kv_upsert()`](https://github.com/apache/apisix/blob/b6ef18c7d85f704be7c4ab7599eb7ecbd6f5f83c/apisix/consumer.lua#L331-L338) for any plugin whose map is already populated. This couples key-auth traffic to unrelated basic-auth updates and remote-secret latency/availability. **Suggested fix:** resolve and cache remote secrets off the request path. At minimum, avoid refreshing another plugin's secret-backed map from the current plugin's request, and retain suitable caching for unchanged entries. Add a regression that asserts the unrelated key-auth request performs no additional secret reads. The ordinary watch-based key update and deletion control probes passed for both versions; the failures above exercise the snapshot, concurrency, and cross-plugin paths specifically. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
