AlinsRan opened a new pull request, #13717: URL: https://github.com/apache/apisix/pull/13717
### Description Fixes #12834 The full reload path and the incremental watch path in `core/config_etcd.lua` disagree on what to do with an item whose data fails validation: - **watch path** (`sync_data`): `goto CONTINUE` — only the update of that item is cancelled, the value already in memory keeps serving. This is deliberate, see #11268. - **full reload path** (`load_full_data`): before calling it, `sync_data` fired the clean handlers of *every* old item and dropped the whole table; the invalid item is then simply not inserted. The item disappears from the data plane. So the root cause is not "validation is too strict" — an item can sit there rejected by validation for days with zero impact, because the watch path protects it. What breaks is that a full reload is triggered asynchronously by something completely unrelated: etcd cancels the watch with `compacted` whenever an ordinary auto compaction happened after the global revision moved on outside the watched prefix. At that random moment the item is discarded. Since global plugins normally all live on the same global rule, one invalid plugin conf takes down *every* global plugin, and the operator sees no cause-effect relation with anything they did. The reporter's case: a `proxy-cache` conf written through the dashboard with the default `cache_zone: disk_cache_one`, on a node whose `config.yaml` declares different zones. `proxy-cache`'s `check_schema` validates `cache_zone` against the *local* node config, so the item is permanently invalid on that node. **Commit 1** makes the full reload behave like the watch path. `sync_data` no longer fires the old clean handlers up front; it hands the previous values to `load_full_data`, which: - carries the previous item over **as is** when the new data of the same key fails validation — no clean handlers, no `filter` re-run, no `modifiedIndex` change, no `conf_version` bump. "As is" is exactly what `goto CONTINUE` means on the watch path, and re-running `filter` is not safe in general (e.g. the `/plugins` filter triggers `plugin.load`); - still skips an invalid item that has no previous value (first load — nothing to keep); - still drops keys absent from the `readdir` result, so a deleted item is not resurrected; - fires the clean handlers of the old items that were replaced or deleted *after* the new table is built. Handlers only act on resources owned by their own item table (healthcheckers, parent references), old and new items are distinct tables, so build-then-clean has no race. Behaviour change: a full reload hitting invalid data now logs a WARN with the key and the error and keeps serving the last valid config, instead of silently dropping it. That is the semantics the watch path has had since 3.10.0, so no new semantics are introduced — the fix only removes "whether a compaction happens to occur" as a factor. **Commit 2** removes the amplifier. `_meta.disable: true` currently does not exempt a plugin from `check_schema` (`skip_disabled_plugin` only covers plugins not enabled in `config.yaml`, which is a different thing). A disabled plugin is never executed — `_M.filter` skips it via `check_disable` — so an environment dependent validation failure has no reason to invalidate the item that carries it. `check_schema` still runs (its side effects, such as default injection, are wanted), but the failure is only fatal when the plugin is enabled; otherwise it is logged as a warning and accepted. Re-enabling the conf goes through a normal write and is fully validated, so an invalid conf cannot silently become active. `stream_check_schema` gets the same treatment. Note this also relaxes the Admin API, which shares `check_single_plugin_schema`: a disabled plugin with a locally invalid conf is now accepted there too. That is intentional — with heterogeneous `config.yaml` across data planes it is the expected behaviour, and the dashboard/direct etcd writes bypassed this check anyway. The two commits are independent: commit 1 alone already removes the outage. ### Tests `t/core/config_etcd.t`, three new cases, all driving the real sync loop against etcd and asserting on the config object of the worker that serves the request. The full reload is entered by setting `need_reload = true` — the exact state `sync_data` sets when etcd answers `compacted` — followed by a write that wakes the loop blocked on the watch semaphore. - **TEST 16** (fails without commit 1): store a valid global rule, then overwrite it with a conf containing a `proxy-cache` whose `cache_zone` does not exist on this node. Assert the rule survives the watch path (existing behaviour), then force a full reload and assert it is *still* there with its `response-rewrite` plugin intact, plus the `keep the previous configuration` WARN. Before the fix the rule is gone at that point and the WARN never appears. - **TEST 17**: inject an item that exists in memory but not in etcd, force a full reload, assert it is dropped — guards against a carry implementation that resurrects deleted items. - **TEST 18**: write an invalid item under a *new* key, force a full reload, assert it is not loaded and that no `keep the previous configuration` is logged — the no-previous-value case must still skip. `t/plugin/proxy-cache/disk.t`: Admin `PUT` of a disabled `proxy-cache` with a non-existent zone now returns 200 and logs the warning; the same conf with `disable: false` still returns 400. The tests have not been executed locally; relying on CI. ### Checklist - [x] I have explained the need for this PR and the problem it solves - [x] I have explained the changes or the new features added to this PR - [x] I have added tests corresponding to this change - [ ] I have updated the documentation to reflect this change - [x] I have verified that this change is backward compatible -- 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]
