AlinsRan commented on PR #2794:
URL: 
https://github.com/apache/apisix-ingress-controller/pull/2794#issuecomment-5486437820

   As written, `exclude_resource_type` doesn't skip garbage collection — it 
turns off syncing for that type entirely. I don't think a per-type opt-out is 
the right place to solve this.
   
   **1. Excluding a type disables it, not just its GC.**
   
   With `[consumer]` set, `Consumer`/`ApisixConsumer` CRDs would still 
reconcile and report a healthy status, but would never reach APISIX. Deleting a 
Gateway would strand the CRD-managed consumers there permanently.
   
   Two things combine to cause that:
   
   - `Sync()` isn't a periodic sweep sitting beside a per-CRD write path — it 
*is* the write path. `apisixProvider.Update()` calls `client.UpdateConfig()`, 
which only writes the local Store, then leans on `defer d.syncNotify()` to wake 
the loop in `Start()`, which calls `client.Sync()`. `Client.Update()`, the one 
that actually pushes, has no callers.
   - ADC's `filterResourceType` strips the excluded type from both local and 
remote before diffing (`apps/cli/src/server/sync.ts`), so the type drops out of 
the diff altogether rather than just being spared deletion.
   
   **2. The root cause looks like the missing label selector on the full sync.**
   
   The `Task` built in `Client.Sync()` leaves `Labels` nil, so ADC diffs 
against the entire remote config and ignores the `manager-by: 
apisix-ingress-controller` label the translators already put on services, SSLs 
and consumers.
   
   Scoping the sweep to that selector would protect externally created 
resources of any type — no new config knob, and no turning off reconciliation 
for the types you exclude. Global rules and plugin metadata have no labels to 
select on, but ADC's `filterConfiguration` already skips both, so they'd keep 
diffing as they do today.
   
   It isn't free: anything created by a controller old enough to predate the 
label would suddenly look unmanaged. That needs verifying before anyone commits 
to it. But it goes at the real problem, and I'd rather we try that first.
   
   **3. The scenario doesn't arise under standalone.**
   
   `apisix-standalone` has no per-resource Admin API, just GET/PUT on 
`/apisix/admin/configs`, so nothing external can create a consumer out of band 
there — while the side effect in point 1 still applies.
   
   ---
   
   One small thing for whenever this settles: ADC resource types are lowercase 
snake_case (`route`, `service`, `ssl`, `global_rule`, `consumer`, 
`consumer_group`, ...), so `Consumer` and `ConsumerGroup` in the sample config 
and docs would be rejected by the server's schema validation.
   


-- 
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]

Reply via email to