AlinsRan commented on code in PR #2883:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2883#discussion_r4042993582
##########
internal/adc/cache/store.go:
##########
@@ -26,24 +27,80 @@ import (
adctypes "github.com/apache/apisix-ingress-controller/api/adc"
"github.com/apache/apisix-ingress-controller/internal/controller/label"
+ "github.com/apache/apisix-ingress-controller/internal/types"
)
type Store struct {
cacheMap map[string]Cache
pluginMetadataMap map[string]adctypes.PluginMetadata
+ // owners maps each service, ssl and consumer a cacheKey holds to the
Kubernetes
+ // resource that produced it, recorded from Insert's labels as it is
written.
+ // global_rule and plugin_metadata don't go through this yet: see
Lookup.
+ owners map[string]map[entityKey]types.NamespacedNameKind
Review Comment:
memdb already maintains this exact index, so this map is a second copy of
state the DB keeps for us.
`KindLabelIndexer` (`internal/adc/cache/indexer.go:41`) is keyed on
`(LabelKind, LabelNamespace, LabelName)` — the owner — and `list()`
(`internal/adc/cache/memdb.go:244`) switches to it whenever a
`KindLabelSelector` is passed. So:
- `OwnedEntities(name, owner)` is
`ListServices`/`ListSSL`/`ListConsumers(&KindLabelSelector{Kind, Namespace,
Name})` — an index lookup, instead of the current full scan over
`s.owners[name]` under the store mutex.
- `Lookup(name, resourceType, id)` is `Get*(id)` +
`ownerFromLabels(obj.GetLabels())`, which is what `GetResourceLabel`
(`store.go:263`) already does and what `internal/provider/apisix/status.go:129`
already consumes.
Beyond the 8 `setOwner`/`removeOwner` call sites this would remove, the
bigger cost is having two sources of truth that can already disagree. `Insert`
records the owner from the **caller-supplied `Labels` argument**, while
`removeOwner` only ever fires for what `ListXxx(selector)` returns — i.e. the
labels **stored on the object itself**.
`TestLookupFindsTheOwnerOfEveryTopLevelType` constructs exactly that split: the
SSL carries no labels of its own, yet an owner is recorded for it, so `Lookup`
answers `ApisixTls/ns/tls` and `GetResourceLabel` answers empty for the same
entity. Production happens to agree today because `provider.go:284` passes the
same `label.GenLabel(obj)` the translator stamps on the objects, but nothing
enforces it. When they do diverge, the entry is also unreachable for removal
and leaks for the lifetime of the process — deriving the owner from the
object's own labels removes that class of bug entirely.
Keeping `Entity` and both method signatures but backing them with the
existing label index would leave 2b/2c's interface untouched and leave `Store`
with no extra state to maintain.
--
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]
