AlinsRan commented on code in PR #2884:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2884#discussion_r4045642930
##########
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 the per-owner rows
on master 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 ever matches whoever currently holds the row — so deleting that
owner removes the plugin outright, including the declaration the other owner
still has.
Repro (through `Insert`/`Delete`, the path `provider.go` actually uses):
1. Insert `global_rule` `{"prometheus": ...}` with owner A's labels.
2. Insert `global_rule` `{"prometheus": ...}` with owner B's labels.
3. `Delete(configName, ["global_rule"], labelsOf(B))` — A is untouched and
still declares it.
4. `GetResources(configName).GlobalRules`
master gives `{prometheus: <A's config>}`; this PR gives `{}`.
Two variants, both reachable:
- **Two `ApisixGlobalRule` CRs enabling the same plugin.** A user conflict,
but on master it degraded to "the survivor's config", now it degrades to "no
plugin".
- **A `GatewayProxy`'s own `spec.plugins` vs an `ApisixGlobalRule`.** Not a
conflict at all: `translateGateway` writes the GatewayProxy's plugins under the
*Gateway's* labels (`gateway.go:60-66`), so they share the cacheKey. Deleting
the `ApisixGlobalRule` drops the GatewayProxy's plugin, and nothing brings it
back — neither `gateway_controller.go` nor `ingressclass_controller.go` watches
`ApisixGlobalRule`, and the periodic sync pushes the store rather than
re-translating. It stays missing until the Gateway object itself changes or the
controller restarts. 2c should make this one moot by attributing those plugins
to the GatewayProxy, but 2b alone leaves the window open.
There is also an attribution flap: because the write upserts `Owner`,
`Lookup(global_rule, "prometheus")` 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 only writes each owner once.
Suggested fix: key the row by `(owner, plugin)` and resolve the collision on
read instead of on write. Concretely: `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, which 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 this locally: ~130 lines across the six files in
`internal/adc/cache/`, the two repro cases above pass, and the rest of the
package plus `internal/provider/...` and `internal/adc/...` stay green. The one
test that has to change is
`TestSetGlobalRulesOfTheSameNameOverwritesAndAttributesToTheLastWriter`, since
"last writer wins" is exactly the rule being replaced; rewriting it to run both
write orders and assert the same outcome covers the flap too. 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]
