AlinsRan commented on PR #2794: URL: https://github.com/apache/apisix-ingress-controller/pull/2794#issuecomment-5486856682
Expanding on my earlier comment — the first point is the one that matters and I don't think I made it legible. **1. The write path this PR assumes doesn't exist.** The design assumes two independent paths: (a) a CRD changes and the controller pushes that resource, and (b) a periodic sweep that deletes whatever has no CRD behind it. `exclude_resource_type` is meant to touch only (b). Path (a) isn't there. `apisixProvider.Update()` calls `client.UpdateConfig()`, which writes the local Store and nothing else — it sends no request to APISIX. It then relies on `defer d.syncNotify()` to wake the loop in `Start()`, which calls `client.Sync()`. `Client.Update()`, the only method that actually pushes, has no callers anywhere in the repo. Every CRD change is delivered by (b); there is no separate sweep to open a hole in. And ADC implements the exclusion by dropping the type from both the local and the remote configuration before diffing (`filterResourceType`, called twice in `apps/cli/src/server/sync.ts`). With neither side carrying consumers, the diff has no consumer events at all — nothing gets deleted, but nothing gets created or updated either. Put together, here is what someone running `exclude_resource_type: [consumer]` would see: they create a Consumer CRD, it reconciles, the status is healthy, the controller logs no error — and the consumer never reaches APISIX. Deleting a Gateway leaves the CRD-managed consumers stranded there with nothing left to clean them up. That's a worse failure than the one being fixed, and a silent one. **2. The deletions come from the sweep not knowing what it owns.** `Client.Sync()` builds its `Task` with `Labels` nil, which tells ADC to diff the local config against *everything* in APISIX and delete whatever the local side doesn't have. That's why externally created consumers disappear — not because the controller lacks a per-type opt-out. The translators already stamp `manager-by: apisix-ingress-controller` on services, SSLs and consumers; the sweep just doesn't use it. Passing it as the label selector changes the question from "what isn't in my config?" to "what did I create that isn't in my config?", which is the one that should have been asked. That protects externally created resources of every type, needs no new configuration surface, and doesn't switch off reconciliation for anything. Global rules and plugin metadata have no labels to select on, but ADC's `filterConfiguration` already skips both, so they keep diffing exactly as they do today. The cost is that resources created by a controller predating the label would suddenly look unowned — no longer updated, no longer collected. That needs checking against real deployments before committing to it. **3. Under standalone the problem being solved doesn't exist.** `apisix-standalone` exposes only GET/PUT on `/apisix/admin/configs`, with no per-resource Admin API, so nothing outside the controller can create a consumer there in the first place. The scenario this PR targets can't arise in that mode — while the silent failure from point 1 still can, and `validateProvider()` doesn't distinguish the two providers. -- 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]
