fix(config_etcd): keep the previous value when a full reload gets invalid data - #13717
Open
AlinsRan wants to merge 5 commits into
Open
fix(config_etcd): keep the previous value when a full reload gets invalid data#13717AlinsRan wants to merge 5 commits into
AlinsRan wants to merge 5 commits into
Conversation
…alid data The full reload path (load_full_data) and the incremental watch path treat an item that fails validation differently: - the watch path only cancels the update of that item and keeps the value already in memory (`goto CONTINUE`, see apache#11268); - the full reload path fires the clean handlers of every old item before reading from etcd and then skips the invalid item, so the item silently disappears from the data plane. A full reload is triggered whenever the watch is cancelled by etcd with `compacted`, which happens on ordinary etcd auto compaction when the global revision moved on outside the watched prefix. An item that has been rejected by the validation for a long time without any impact therefore vanishes at a random point in time. Since global plugins usually all live on the same global rule, one invalid plugin conf takes down every global plugin. Make the full reload behave like the watch path: pass the previous values to load_full_data and, when the new data of a key fails the validation and a previous value for the same key exists, carry the old item over as is - no clean handlers, no filter re-run, no version bump. Keys that are absent from the readdir result are still dropped, and an invalid item without a previous value (first load) is still skipped. The clean handlers of the old items that were replaced or deleted are now fired after the new table has been built. Fixes apache#12834
A plugin conf carrying `_meta.disable: true` is never executed, but its check_schema is still run unconditionally, and a failure invalidates the whole item (route, service, global rule, ...). Some plugins validate the local environment inside check_schema - proxy-cache requires cache_zone to be declared in the config.yaml of this very node - so a conf that is legal elsewhere permanently invalidates the item on this node, even though the plugin is turned off. Keep running check_schema (its side effects, e.g. default injection, are still wanted) but only reject the conf when the plugin is enabled; a disabled plugin logs a warning and is accepted. Re-enabling it goes through a full write and is validated again, so an invalid conf cannot silently become active. Same treatment for the stream subsystem.
dosubot
Bot
added
size:L
The blocks drive a real etcd sync round and sleep past the 5s default block timeout, so they died on a client socket timeout rather than on an assertion.
…y disabled check_disable returns _meta.disable verbatim, so a truthy non-boolean value such as the string "false" would have taken the downgrade path and silently accepted a conf that the schema should have rejected.
The downgrade for _meta.disable plugins piped the check_schema error straight into a warn. That string can echo config values (e.g. a pattern mismatch), so a disabled plugin's bad config could leak into the shared error log. Keep the warn as the signal that the config did not validate, but drop the error detail — the caller can still see it by re-submitting with the plugin enabled. Also assert the admin PUT actually returns passed in the disk.t case, not just the log line.
nic-6443
reviewed
Jul 28, 2026
Comment on lines
+689
to
+697
| -- fire the clean handlers of the previous items that were not carried | ||
| -- over: they were either replaced by a new value or deleted from etcd | ||
| if prev_values then | ||
| for _, item in ipairs(prev_values) do | ||
| if item and not carried[item] then | ||
| config_util.fire_all_clean_handlers(item) | ||
| end | ||
| end | ||
| end |
Member
There was a problem hiding this comment.
The entire clean handler should be removable after #12426
Contributor
Author
There was a problem hiding this comment.
You're right that clean handlers have no registrant after #12426 (health checks moved to healthcheck_manager), so this is effectively a no-op.
I've kept it here for consistency: config_yaml/config_xds full reloads fire on replaced/deleted items the same way, and dropping it only in the etcd full-reload path would leave a confusing asymmetry. Removing the whole clean_handlers mechanism uniformly (the config_util API plus all fire sites across the config backends) is broader than this bugfix, so I'll do it in a dedicated follow-up PR rather than entangle it with a backportable fix.
nic-6443
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #12834
The incremental watch path and the full reload path in
core/config_etcd.luadisagree on invalid data:sync_data):goto CONTINUE— cancels only that item's update, the value in memory keeps serving (deliberate, fix: validation fails causing etcd events not to be handled correctly #11268).load_full_data): the old table is dropped up front and the invalid item is simply not re-inserted, so it disappears.A full reload is triggered asynchronously when etcd cancels the watch with
compacted. So an item that validation rejects can serve for days, then vanish the moment an unrelated compaction happens. Because global plugins usually share one global rule, one invalid plugin conf takes down every global plugin — with no visible cause.Reporter's case: a
proxy-cacheconf with the defaultcache_zone: disk_cache_one, on a node whoseconfig.yamldeclares different zones.check_schemavalidatescache_zoneagainst the local node config, so the item is permanently invalid on that node.Changes
Commit 1 — make full reload behave like the watch path. On validation failure
load_full_datanow carries the previous value over as is (no clean handlers, nofilterre-run, noconf_versionbump), instead of dropping it. It still skips an invalid item with no previous value (first load), still drops keys absent from etcd (no resurrection), and fires the clean handlers of replaced/deleted items after the new table is built. This is the same semantics the watch path has had since 3.10.0 — no new behavior, only removing "did a compaction happen to occur" as a factor.Commit 2 — remove the amplifier. A plugin disabled via
_meta.disable: trueis never executed, so an environment-dependent validation failure should not invalidate the item that carries it.check_schemastill runs (its default injection is wanted) but the failure is fatal only when the plugin is enabled; otherwise it is logged and accepted. Re-enabling goes through a normal fully-validated write, so an invalid conf can't silently become active.stream_check_schemagets the same treatment. This also relaxes the Admin API (sharedcheck_single_plugin_schema), which is intended given heterogeneousconfig.yamlacross data planes.Tests
t/core/config_etcd.t: full reload keeps the previous value on invalid data (TEST 16), does not resurrect a deleted item (TEST 17), still skips an invalid item with no previous value (TEST 18). All drive the real sync loop against etcd.t/plugin/proxy-cache/disk.t: AdminPUTof a disabledproxy-cachewith a non-existent zone returns 200; the same conf enabled still returns 400.Checklist