AlinsRan commented on code in PR #2884:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2884#discussion_r4045679295
##########
internal/adc/cache/cache.go:
##########
@@ -19,8 +19,40 @@ package cache
import (
types "github.com/apache/apisix-ingress-controller/api/adc"
+ internaltypes
"github.com/apache/apisix-ingress-controller/internal/types"
)
+// GlobalRuleRow is one global_rules plugin, keyed by the plugin name. Owner
is the
+// Kubernetes resource that declared it: a GatewayProxy or an ApisixGlobalRule.
+type GlobalRuleRow struct {
+ ID string
+ Owner internaltypes.NamespacedNameKind
+ Config any
+}
Review Comment:
Keying the row by the plugin name alone costs something master's per-owner
rows gave us: `ID` is the memdb primary key (`schema.go`, `Unique: true`), so a
second owner declaring the same plugin upserts over the first owner's row,
`Owner` included. `setGlobalRules` then deletes by `OwnerSelector`, which only
matches whoever currently holds the row — so deleting that owner removes the
plugin outright, including the declaration the other owner still has.
Repro, on top of the usual GatewayProxy + IngressClass setup:
1. Create two `ApisixGlobalRule`s enabling the same plugin:
```yaml
apiVersion: apisix.apache.org/v2
kind: ApisixGlobalRule
metadata: { name: gr-a }
spec:
ingressClassName: apisix
plugins:
- name: response-rewrite
enable: true
config: { headers: { set: { X-From: a } } }
---
apiVersion: apisix.apache.org/v2
kind: ApisixGlobalRule
metadata: { name: gr-b }
spec:
ingressClassName: apisix
plugins:
- name: response-rewrite
enable: true
config: { headers: { set: { X-From: b } } }
```
2. `curl -i` any route — the response carries `X-From: b`.
3. `kubectl delete apisixglobalrule gr-b`. `gr-a` is untouched and still
reports `Accepted`.
4. `curl -i` again.
Expected, and what master does: `X-From: a`. On this PR the header is gone —
`response-rewrite` is no longer configured at all.
Driving the same thing through `apisixProvider.Update`/`Delete` with real
`ApisixGlobalRule` objects, so the resourceTypes and labels are the ones
`provider.go` computes:
```
both present ->
map[response-rewrite:map[headers:map[set:map[X-From:b]]]]
after deleting gr-b -> map[] # master: map[...X-From:a]
```
A variant worth trying, since it needs no user conflict at all: drop `gr-a`
and put the same `response-rewrite` in the `GatewayProxy`'s own `spec.plugins`.
Deleting `gr-b` then removes the GatewayProxy's plugin, and nothing restores it
— the IngressClass that carried those plugins into the store is not
re-reconciled by an `ApisixGlobalRule` deletion, and the periodic sync pushes
the store rather than re-translating. 2c should make this variant moot by
attributing those plugins to the GatewayProxy, but 2b alone leaves the window
open.
There is also an attribution flap: since the write upserts `Owner`,
`Lookup(global_rule, ...)` returns whichever owner reconciled last, so the
`SyncFailed` condition for one plugin failure ping-pongs between the two
objects across rounds.
`TestSetGlobalRulesOfTheSameNameOverwritesAndAttributesToTheLastWriter` holds
within a round but not across them — it writes each owner only once.
Suggested fix: key the row by `(owner, plugin)` and resolve the collision on
read rather than on write. `ID = owner.String() + "/" + plugin`, keep the bare
name in a `Plugin` field, add a non-unique `plugin` index for `Lookup`, and
have `GetResources` sort by row id before merging so the winner is a property
of the owners rather than of who reconciled last; `Lookup` picks the winner by
the same rule. That keeps the invariant the existing test is really after —
what gets pushed and who it is attributed to always agree — while making it
independent of write order. I tried it locally: ~130 lines across the six files
in `internal/adc/cache/`, both repro cases pass and the rest of the package
plus `internal/provider/...` stay green. The one test that has to change is the
last-writer one above, since that is the rule being replaced. Happy to push the
patch if useful.
--
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]
