This is an automated email from the ASF dual-hosted git repository. bzp2010 pushed a commit to branch bzp/feat-isolation-pr2c-provider-wiring in repository https://gitbox.apache.org/repos/asf/apisix-ingress-controller.git
commit a9a14190bf18053398e8fa1f0374813d14570ddd Author: bzp2010 <[email protected]> AuthorDate: Thu Sep 17 18:05:55 2026 +0800 feat(apisix): attribute a GatewayProxy's global rules and plugin metadata to itself Update and Delete route global_rules and plugin_metadata through the new SetGlobalRules/SetPluginMetadata instead of Insert/Delete, and attribute Gateway- and IngressClass-sourced plugins to the target GatewayProxy rather than to the Gateway or IngressClass that was reconciled. Several Gateways sharing one GatewayProxy now write its plugins once instead of racing each other's labels into the store. Also fixes a bug this exposed in Store.Delete: its old empty- resourceTypes-wipes-everything special case, still needed by Gateway and IngressClass's full-config deletion, would also fire for the now resourceTypes-less ApisixGlobalRule deletion path and wipe every owner's global rules in that config, not just the one being deleted. Delete now only ever removes what it's asked to; DeleteAll is the explicit whole-cacheKey wipe, and Gateway/IngressClass deletion calls it directly. Part 2/6 of #2877 (formerly part 2, now split into 2a/2b/2c: this is 2c, the last). --- internal/adc/cache/store.go | 15 +++-- internal/adc/cache/store_test.go | 6 +- internal/provider/apisix/provider.go | 105 +++++++++++++++++++++--------- internal/provider/apisix/provider_test.go | 70 ++++++++++++++++++++ 4 files changed, 162 insertions(+), 34 deletions(-) diff --git a/internal/adc/cache/store.go b/internal/adc/cache/store.go index 42dc53c5..58ae02ac 100644 --- a/internal/adc/cache/store.go +++ b/internal/adc/cache/store.go @@ -262,6 +262,9 @@ func (s *Store) Insert(name string, resourceTypes []string, resources *adctypes. return nil } +// Delete removes the services, consumers, ssls and global_rules the resource identified +// by Labels contributes to the cacheKey name. An empty resourceTypes deletes nothing; +// see DeleteAll for wiping a whole cacheKey. func (s *Store) Delete(name string, resourceTypes []string, Labels map[string]string) error { s.Lock() defer s.Unlock() @@ -320,13 +323,17 @@ func (s *Store) Delete(name string, resourceTypes []string, Labels map[string]st } } } - if len(resourceTypes) == 0 { - delete(s.cacheMap, name) - delete(s.owners, name) - } return nil } +// DeleteAll wipes everything the cacheKey name holds. +func (s *Store) DeleteAll(name string) { + s.Lock() + defer s.Unlock() + delete(s.cacheMap, name) + delete(s.owners, name) +} + func (s *Store) GetResources(name string) (*adctypes.Resources, error) { s.Lock() defer s.Unlock() diff --git a/internal/adc/cache/store_test.go b/internal/adc/cache/store_test.go index 8aaa9200..eaa49248 100644 --- a/internal/adc/cache/store_test.go +++ b/internal/adc/cache/store_test.go @@ -161,13 +161,17 @@ func TestInsertForgetsTheOwnerOfReplacedResources(t *testing.T) { assert.False(t, ok) } -func TestDeleteWithoutResourceTypesForgetsEveryOwnerToo(t *testing.T) { +func TestDeleteWithoutResourceTypesDeletesNothing(t *testing.T) { route := ownerNamed(types.KindApisixRoute, "route") s := NewStore(logr.Discard()) require.NoError(t, s.Insert(configName, []string{adctypes.TypeService}, &adctypes.Resources{Services: []*adctypes.Service{service("svc", route)}}, labelsOf(route))) require.NoError(t, s.Delete(configName, nil, nil)) _, ok := s.Lookup(configName, adctypes.TypeService, "svc") + assert.True(t, ok, "an empty resourceTypes deletes nothing; see DeleteAll for wiping a whole cacheKey") + + s.DeleteAll(configName) + _, ok = s.Lookup(configName, adctypes.TypeService, "svc") assert.False(t, ok) } diff --git a/internal/provider/apisix/provider.go b/internal/provider/apisix/provider.go index f83df0c0..cf6115b8 100644 --- a/internal/provider/apisix/provider.go +++ b/internal/provider/apisix/provider.go @@ -56,6 +56,19 @@ const ( MinSyncPeriod = 1 * time.Second ) +// pluginSource says whose global_rules and plugin_metadata a translate result carries. +type pluginSource int + +const ( + pluginsNone pluginSource = iota + // pluginsFromGatewayProxy is GatewayProxy.Spec.Plugins and .PluginMetadata, owned by + // each target config's GatewayProxy rather than the Gateway or IngressClass that was + // reconciled. + pluginsFromGatewayProxy + // pluginsFromResource is the reconciled resource's own global rules. + pluginsFromResource +) + // apisixProvider owns AIC's own view of what should be live: which Kubernetes resource // targets which GatewayProxy config (configManager) and the merged, translated resource // snapshot per config (store). It builds the input the adc client package needs and hands @@ -135,6 +148,7 @@ func (d *apisixProvider) Update(ctx context.Context, tctx *provider.TranslateCon var ( result *translator.TranslateResult resourceTypes []string + plugins pluginSource err error ) @@ -158,7 +172,8 @@ func (d *apisixProvider) Update(ctx context.Context, tctx *provider.TranslateCon resourceTypes = append(resourceTypes, adctypes.TypeService) case *gatewayv1.Gateway: result, err = d.translator.TranslateGateway(tctx, t.DeepCopy()) - resourceTypes = append(resourceTypes, adctypes.TypeGlobalRule, adctypes.TypeSSL, adctypes.TypePluginMetadata) + resourceTypes = append(resourceTypes, adctypes.TypeSSL) + plugins = pluginsFromGatewayProxy case *networkingv1.Ingress: result, err = d.translator.TranslateIngress(tctx, t.DeepCopy()) resourceTypes = append(resourceTypes, adctypes.TypeService, adctypes.TypeSSL) @@ -167,13 +182,13 @@ func (d *apisixProvider) Update(ctx context.Context, tctx *provider.TranslateCon resourceTypes = append(resourceTypes, adctypes.TypeConsumer) case *networkingv1.IngressClass: result, err = d.translator.TranslateIngressClass(tctx, t.DeepCopy()) - resourceTypes = append(resourceTypes, adctypes.TypeGlobalRule, adctypes.TypePluginMetadata) + plugins = pluginsFromGatewayProxy case *apiv2.ApisixRoute: result, err = d.translator.TranslateApisixRoute(tctx, t.DeepCopy()) resourceTypes = append(resourceTypes, adctypes.TypeService) case *apiv2.ApisixGlobalRule: result, err = d.translator.TranslateApisixGlobalRule(tctx, t.DeepCopy()) - resourceTypes = append(resourceTypes, adctypes.TypeGlobalRule) + plugins = pluginsFromResource case *apiv2.ApisixTls: result, err = d.translator.TranslateApisixTls(tctx, t.DeepCopy()) resourceTypes = append(resourceTypes, adctypes.TypeSSL) @@ -211,30 +226,34 @@ func (d *apisixProvider) Update(ctx context.Context, tctx *provider.TranslateCon labels := label.GenLabel(obj) d.log.V(1).Info("updating config", "resourceKey", rk, "configs", configs, "resourceTypes", resourceTypes) - return d.applyResourceState(rk, configs, resourceTypes, resources, labels) + return d.applyResourceState(rk, configs, resourceTypes, resources, labels, plugins) } func (d *apisixProvider) Delete(ctx context.Context, obj client.Object) error { d.log.V(1).Info("deleting object", "object", obj) - var resourceTypes []string - var labels map[string]string + var ( + resourceTypes []string + labels map[string]string + plugins pluginSource + // wholeConfig is set for a Gateway or IngressClass, whose deletion wipes every + // config it referenced. + wholeConfig bool + ) switch obj.(type) { case *gatewayv1.HTTPRoute, *apiv2.ApisixRoute, *gatewayv1.GRPCRoute, *gatewayv1.TCPRoute, *gatewayv1.UDPRoute, *gatewayv1.TLSRoute: resourceTypes = append(resourceTypes, adctypes.TypeService) labels = label.GenLabel(obj) - case *gatewayv1.Gateway: - // delete all resources + case *gatewayv1.Gateway, *networkingv1.IngressClass: + wholeConfig = true case *networkingv1.Ingress: resourceTypes = append(resourceTypes, adctypes.TypeService, adctypes.TypeSSL) labels = label.GenLabel(obj) case *v1alpha1.Consumer: resourceTypes = append(resourceTypes, adctypes.TypeConsumer) labels = label.GenLabel(obj) - case *networkingv1.IngressClass: - // delete all resources case *apiv2.ApisixGlobalRule: - resourceTypes = append(resourceTypes, adctypes.TypeGlobalRule) + plugins = pluginsFromResource labels = label.GenLabel(obj) case *apiv2.ApisixTls: resourceTypes = append(resourceTypes, adctypes.TypeSSL) @@ -245,22 +264,17 @@ func (d *apisixProvider) Delete(ctx context.Context, obj client.Object) error { } nnk := utils.NamespacedNameKind(obj) + removed, err := d.removeResourceState(nnk, resourceTypes, labels, plugins, wholeConfig) + if err != nil { + return err + } // Full synchronization is performed on a gateway by gateway basis // and it is not possible to perform scheduled synchronization // on deleted gateway level resources - if len(resourceTypes) == 0 { - removed, err := d.removeResourceState(nnk, resourceTypes, labels) - if err != nil { - return err - } + if wholeConfig { d.syncEvictedConfigsNow(ctx, removed, resourceTypes, labels) return nil } - - removed, err := d.removeResourceState(nnk, resourceTypes, labels) - if err != nil { - return err - } // Syncing pushes the whole store to every data plane. Objects this controller never // configured delete nothing, and reconciles for them are frequent, so notify only // when the store actually changed. @@ -279,17 +293,33 @@ func (d *apisixProvider) applyResourceState( resourceTypes []string, resources *adctypes.Resources, labels map[string]string, + plugins pluginSource, ) error { d.Lock() defer d.Unlock() evicted := d.configManager.Update(rk, configs) - if err := d.evictFromStore(evicted, resourceTypes, labels); err != nil { + if err := d.evictFromStore(rk, evicted, resourceTypes, labels, plugins); err != nil { return err } - for _, cfg := range configs { - if err := d.store.Insert(cfg.Name, resourceTypes, resources, labels); err != nil { - return fmt.Errorf("store insert failed for config %s: %w", cfg.Name, err) + for gatewayProxy, cfg := range configs { + if len(resourceTypes) > 0 { + if err := d.store.Insert(cfg.Name, resourceTypes, resources, labels); err != nil { + return fmt.Errorf("store insert failed for config %s: %w", cfg.Name, err) + } + } + switch plugins { + case pluginsFromGatewayProxy: + if err := d.store.SetGlobalRules(cfg.Name, gatewayProxy, resources.GlobalRules); err != nil { + return fmt.Errorf("store global rules failed for config %s: %w", cfg.Name, err) + } + if err := d.store.SetPluginMetadata(cfg.Name, resources.PluginMetadata); err != nil { + return fmt.Errorf("store plugin metadata failed for config %s: %w", cfg.Name, err) + } + case pluginsFromResource: + if err := d.store.SetGlobalRules(cfg.Name, rk, resources.GlobalRules); err != nil { + return fmt.Errorf("store global rules failed for config %s: %w", cfg.Name, err) + } } } return nil @@ -297,34 +327,51 @@ func (d *apisixProvider) applyResourceState( // removeResourceState forgets a resource's config associations and evicts its contribution // from each config it used to reference, returning those configs so an immediate-push -// caller (see syncEvictedConfigsNow) knows what to push right away. +// caller (see syncEvictedConfigsNow) knows what to push right away. wholeConfig wipes each +// of those configs entirely instead. func (d *apisixProvider) removeResourceState( rk types.NamespacedNameKind, resourceTypes []string, labels map[string]string, + plugins pluginSource, + wholeConfig bool, ) (map[types.NamespacedNameKind]adctypes.Config, error) { d.Lock() defer d.Unlock() evicted := d.configManager.Get(rk) d.configManager.Delete(rk) - if err := d.evictFromStore(evicted, resourceTypes, labels); err != nil { + if wholeConfig { + for _, cfg := range evicted { + d.store.DeleteAll(cfg.Name) + } + return evicted, nil + } + if err := d.evictFromStore(rk, evicted, resourceTypes, labels, plugins); err != nil { return nil, err } return evicted, nil } -// evictFromStore deletes a resource's contribution from each of the given configs' cached -// snapshots. Callers must already hold d.Lock. +// evictFromStore deletes rk's contribution from each of the given configs' cached +// snapshots. global_rules and plugin_metadata sourced from a GatewayProxy belong to that +// GatewayProxy's own config and are left in place. Callers must already hold d.Lock. func (d *apisixProvider) evictFromStore( + rk types.NamespacedNameKind, configs map[types.NamespacedNameKind]adctypes.Config, resourceTypes []string, labels map[string]string, + plugins pluginSource, ) error { for _, cfg := range configs { if err := d.store.Delete(cfg.Name, resourceTypes, labels); err != nil { return fmt.Errorf("store delete failed for config %s: %w", cfg.Name, err) } + if plugins == pluginsFromResource { + if err := d.store.SetGlobalRules(cfg.Name, rk, nil); err != nil { + return fmt.Errorf("store global rules failed for config %s: %w", cfg.Name, err) + } + } } return nil } diff --git a/internal/provider/apisix/provider_test.go b/internal/provider/apisix/provider_test.go index 66a91b7f..eac54aba 100644 --- a/internal/provider/apisix/provider_test.go +++ b/internal/provider/apisix/provider_test.go @@ -35,6 +35,7 @@ import ( adctypes "github.com/apache/apisix-ingress-controller/api/adc" "github.com/apache/apisix-ingress-controller/internal/adc/cache" adcclient "github.com/apache/apisix-ingress-controller/internal/adc/client" + "github.com/apache/apisix-ingress-controller/internal/controller/label" "github.com/apache/apisix-ingress-controller/internal/provider/common" "github.com/apache/apisix-ingress-controller/internal/types" "github.com/apache/apisix-ingress-controller/internal/utils" @@ -66,6 +67,14 @@ func newTestProvider(t *testing.T) *apisixProvider { } } +func labelsOf(owner types.NamespacedNameKind) map[string]string { + return map[string]string{ + label.LabelKind: owner.Kind, + label.LabelNamespace: owner.Namespace, + label.LabelName: owner.Name, + } +} + // TestDeleteNotifiesSyncOnlyWhenConfigWasRemoved covers the cost side of route // ownership: a sync pushes the whole store to every data plane, and reconciles // for routes this controller never configured are frequent (any EndpointSlice @@ -176,3 +185,64 @@ func TestSyncStillPushesHealthyConfigsWhenAnotherFails(t *testing.T) { assert.True(t, seen["bad"], "the failing config must still have been attempted") assert.True(t, seen["good"], "a config failing must not stop the others from being pushed") } + +func TestApplyResourceStateAttributesGatewayProxyPluginsToTheGatewayProxy(t *testing.T) { + d := newTestProvider(t) + gw1 := types.NamespacedNameKind{Kind: types.KindGateway, Namespace: "ns", Name: "gw1"} + gw2 := types.NamespacedNameKind{Kind: types.KindGateway, Namespace: "ns", Name: "gw2"} + oldProxy := types.NamespacedNameKind{Kind: types.KindGatewayProxy, Namespace: "ns", Name: "old"} + newProxy := types.NamespacedNameKind{Kind: types.KindGatewayProxy, Namespace: "ns", Name: "new"} + configFor := func(proxy types.NamespacedNameKind) map[types.NamespacedNameKind]adctypes.Config { + return map[types.NamespacedNameKind]adctypes.Config{proxy: {Name: proxy.String()}} + } + resourcesOf := func(sslID string) *adctypes.Resources { + return &adctypes.Resources{ + SSLs: []*adctypes.SSL{{Metadata: adctypes.Metadata{ID: sslID, Labels: labelsOf(gw1)}}}, + GlobalRules: adctypes.GlobalRule{"cors": map[string]any{}}, + PluginMetadata: adctypes.PluginMetadata{"http-logger": map[string]any{}}, + } + } + + require.NoError(t, d.applyResourceState(gw1, configFor(oldProxy), []string{adctypes.TypeSSL}, resourcesOf("ssl1"), labelsOf(gw1), pluginsFromGatewayProxy)) + gw2Resources := resourcesOf("ssl2") + gw2Resources.SSLs[0].Labels = labelsOf(gw2) + require.NoError(t, d.applyResourceState(gw2, configFor(oldProxy), []string{adctypes.TypeSSL}, gw2Resources, labelsOf(gw2), pluginsFromGatewayProxy)) + + ssl, ok := d.store.Lookup(oldProxy.String(), adctypes.TypeSSL, "ssl1") + require.True(t, ok) + assert.Equal(t, gw1, ssl.Owner) + for _, resourceType := range []string{adctypes.TypeGlobalRule, adctypes.TypePluginMetadata} { + id := map[string]string{adctypes.TypeGlobalRule: "cors", adctypes.TypePluginMetadata: "http-logger"}[resourceType] + entity, ok := d.store.Lookup(oldProxy.String(), resourceType, id) + require.True(t, ok, resourceType) + assert.Equal(t, oldProxy, entity.Owner, "%s comes from the GatewayProxy, not the Gateway that was reconciled", resourceType) + } + assert.Len(t, d.store.OwnedEntities(oldProxy.String(), oldProxy), 2, "Gateways sharing a GatewayProxy write its plugins once") + + require.NoError(t, d.applyResourceState(gw1, configFor(newProxy), []string{adctypes.TypeSSL}, resourcesOf("ssl1"), labelsOf(gw1), pluginsFromGatewayProxy)) + _, ok = d.store.Lookup(oldProxy.String(), adctypes.TypeSSL, "ssl1") + assert.False(t, ok, "the Gateway's own certificate leaves the config it no longer references") + _, ok = d.store.Lookup(oldProxy.String(), adctypes.TypeGlobalRule, "cors") + assert.True(t, ok, "the old GatewayProxy's own plugins stay in its own config") +} + +func TestRemoveResourceStateRemovesAnApisixGlobalRulesPlugins(t *testing.T) { + d := newTestProvider(t) + globalRule := types.NamespacedNameKind{Kind: types.KindApisixGlobalRule, Namespace: "ns", Name: "global"} + proxy := types.NamespacedNameKind{Kind: types.KindGatewayProxy, Namespace: "ns", Name: "gp"} + configs := map[types.NamespacedNameKind]adctypes.Config{proxy: {Name: proxy.String()}} + require.NoError(t, d.store.SetGlobalRules(proxy.String(), proxy, adctypes.GlobalRule{"cors": map[string]any{}})) + require.NoError(t, d.applyResourceState(globalRule, configs, nil, &adctypes.Resources{ + GlobalRules: adctypes.GlobalRule{"prometheus": map[string]any{}}, + }, labelsOf(globalRule), pluginsFromResource)) + + resources, err := d.store.GetResources(proxy.String()) + require.NoError(t, err) + assert.Len(t, resources.GlobalRules, 2) + + _, err = d.removeResourceState(globalRule, nil, labelsOf(globalRule), pluginsFromResource, false) + require.NoError(t, err) + resources, err = d.store.GetResources(proxy.String()) + require.NoError(t, err) + assert.Equal(t, adctypes.GlobalRule{"cors": map[string]any{}}, resources.GlobalRules, "only the deleted resource's own plugins go away") +}
