From 05dd483baf7e0625cddb5c556580b283d49512ea Mon Sep 17 00:00:00 2001 From: nevercase <1024769485@qq.com> Date: Tue, 25 Jan 2022 18:27:30 +0800 Subject: [PATCH 1/7] fix: aviod create pluginconfig in the tranlsation of route (#836) --- pkg/kube/translation/apisix_route.go | 27 ++--------------------- pkg/kube/translation/apisix_route_test.go | 3 --- 2 files changed, 2 insertions(+), 28 deletions(-) diff --git a/pkg/kube/translation/apisix_route.go b/pkg/kube/translation/apisix_route.go index 8e92485ffd..772fa09513 100644 --- a/pkg/kube/translation/apisix_route.go +++ b/pkg/kube/translation/apisix_route.go @@ -530,17 +530,8 @@ func (t *translator) translateHTTPRouteV2beta3(ctx *TranslateContext, ar *config route.EnableWebsocket = part.Websocket route.Plugins = pluginMap route.Timeout = timeout - pluginConfig := apisixv1.NewDefaultPluginConfig() - if len(pluginMap) > 0 { - pluginConfigName := apisixv1.ComposePluginConfigName(ar.Namespace, backend.ServiceName) - route.PluginConfigId = id.GenID(pluginConfigName) - pluginConfig.ID = route.PluginConfigId - pluginConfig.Name = pluginConfigName - pluginConfig.Plugins = pluginMap - } else { - if part.PluginConfigName != "" { - route.PluginConfigId = part.PluginConfigName - } + if part.PluginConfigName != "" { + route.PluginConfigId = part.PluginConfigName } if len(backends) > 0 { @@ -566,9 +557,6 @@ func (t *translator) translateHTTPRouteV2beta3(ctx *TranslateContext, ar *config } ctx.addUpstream(ups) } - if len(pluginMap) > 0 { - ctx.addPluginConfig(pluginConfig) - } } return nil } @@ -851,7 +839,6 @@ func (t *translator) translateStreamRoute(ctx *TranslateContext, ar *configv2bet if !ctx.checkUpstreamExist(ups.Name) { ctx.addUpstream(ups) } - } return nil } @@ -977,13 +964,6 @@ func (t *translator) translateHTTPRouteV2beta3NotStrictly(ctx *TranslateContext, route := apisixv1.NewDefaultRoute() route.Name = apisixv1.ComposeRouteName(ar.Namespace, ar.Name, part.Name) route.ID = id.GenID(route.Name) - pluginConfig := apisixv1.NewDefaultPluginConfig() - if len(pluginMap) > 0 { - pluginConfigName := apisixv1.ComposePluginConfigName(ar.Namespace, backend.ServiceName) - route.PluginConfigId = id.GenID(pluginConfigName) - pluginConfig.ID = route.PluginConfigId - pluginConfig.Name = pluginConfigName - } ctx.addRoute(route) if !ctx.checkUpstreamExist(upstreamName) { @@ -993,9 +973,6 @@ func (t *translator) translateHTTPRouteV2beta3NotStrictly(ctx *TranslateContext, } ctx.addUpstream(ups) } - if len(pluginMap) > 0 { - ctx.addPluginConfig(pluginConfig) - } } return nil } diff --git a/pkg/kube/translation/apisix_route_test.go b/pkg/kube/translation/apisix_route_test.go index ed15ea8b1b..3844139530 100644 --- a/pkg/kube/translation/apisix_route_test.go +++ b/pkg/kube/translation/apisix_route_test.go @@ -391,16 +391,13 @@ func TestTranslateApisixRouteV2alpha1NotStrictly(t *testing.T) { assert.NoError(t, err, "translateRoute not strictly should be no error") assert.Equal(t, 2, len(tx.Routes), "There should be 2 routes") assert.Equal(t, 2, len(tx.Upstreams), "There should be 2 upstreams") - assert.Equal(t, 1, len(tx.PluginConfigs), "There should be 1 pluginConfigs") assert.Equal(t, "test_ar_rule1", tx.Routes[0].Name, "route1 name error") assert.Equal(t, "test_ar_rule2", tx.Routes[1].Name, "route2 name error") assert.Equal(t, "test_svc1_81", tx.Upstreams[0].Name, "upstream1 name error") assert.Equal(t, "test_svc2_82", tx.Upstreams[1].Name, "upstream2 name error") - assert.Equal(t, "test_svc1", tx.PluginConfigs[0].Name, "pluginConfig1 name error") assert.Equal(t, id.GenID("test_ar_rule1"), tx.Routes[0].ID, "route1 id error") assert.Equal(t, id.GenID("test_ar_rule2"), tx.Routes[1].ID, "route2 id error") assert.Equal(t, id.GenID("test_svc1_81"), tx.Upstreams[0].ID, "upstream1 id error") assert.Equal(t, id.GenID("test_svc2_82"), tx.Upstreams[1].ID, "upstream2 id error") - assert.Equal(t, id.GenID("test_svc1"), tx.PluginConfigs[0].ID, "pluginConfig1 id error") } From 5aada25a7e24bedf0b9e9f8a41b7e84b0c781df7 Mon Sep 17 00:00:00 2001 From: nevercase <1024769485@qq.com> Date: Wed, 26 Jan 2022 11:53:09 +0800 Subject: [PATCH 2/7] =?UTF-8?q?fix:=20the=20Route.PluginConfigId=20will=20?= =?UTF-8?q?not=20be=20written=20if=20the=20http=20PluginConfigName=20had?= =?UTF-8?q?=E2=80=99n=20been=20specified?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/kube/translation/apisix_route_test.go | 82 ++++++++++++++++++++++- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/pkg/kube/translation/apisix_route_test.go b/pkg/kube/translation/apisix_route_test.go index 3844139530..9534ffcbba 100644 --- a/pkg/kube/translation/apisix_route_test.go +++ b/pkg/kube/translation/apisix_route_test.go @@ -185,7 +185,7 @@ func TestRouteMatchExpr(t *testing.T) { assert.Equal(t, []string{"foo.com"}, results[9][2].SliceVal) } -func TestTranslateApisixRouteV2alpha1WithDuplicatedName(t *testing.T) { +func mockTranslator(t *testing.T) (*translator, <-chan struct{}) { svc := &corev1.Service{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{ @@ -278,6 +278,11 @@ func TestTranslateApisixRouteV2alpha1WithDuplicatedName(t *testing.T) { go epInformer.Run(stopCh) cache.WaitForCacheSync(stopCh, svcInformer.HasSynced) + return tr, processCh +} + +func TestTranslateApisixRouteV2alpha1WithDuplicatedName(t *testing.T) { + tr, processCh := mockTranslator(t) <-processCh <-processCh @@ -324,11 +329,84 @@ func TestTranslateApisixRouteV2alpha1WithDuplicatedName(t *testing.T) { }, } - _, err = tr.TranslateRouteV2beta3(ar) + _, err := tr.TranslateRouteV2beta3(ar) assert.NotNil(t, err) assert.Equal(t, "duplicated route rule name", err.Error()) } +func TestTranslateApisixRouteV2alpha1WithEmptyPluginConfigName(t *testing.T) { + tr, processCh := mockTranslator(t) + <-processCh + <-processCh + + ar := &configv2beta3.ApisixRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ar", + Namespace: "test", + }, + Spec: configv2beta3.ApisixRouteSpec{ + HTTP: []configv2beta3.ApisixRouteHTTP{ + { + Name: "rule1", + Match: configv2beta3.ApisixRouteHTTPMatch{ + Paths: []string{ + "/*", + }, + }, + Backends: []configv2beta3.ApisixRouteHTTPBackend{ + { + ServiceName: "svc", + ServicePort: intstr.IntOrString{ + IntVal: 80, + }, + }, + }, + }, + { + Name: "rule2", + Match: configv2beta3.ApisixRouteHTTPMatch{ + Paths: []string{ + "/*", + }, + }, + Backends: []configv2beta3.ApisixRouteHTTPBackend{ + { + ServiceName: "svc", + ServicePort: intstr.IntOrString{ + IntVal: 80, + }, + }, + }, + PluginConfigName: "test-PluginConfigName-1", + }, + { + Name: "rule3", + Match: configv2beta3.ApisixRouteHTTPMatch{ + Paths: []string{ + "/*", + }, + }, + Backends: []configv2beta3.ApisixRouteHTTPBackend{ + { + ServiceName: "svc", + ServicePort: intstr.IntOrString{ + IntVal: 80, + }, + }, + }, + }, + }, + }, + } + res, err := tr.TranslateRouteV2beta3(ar) + assert.NoError(t, err) + assert.Len(t, res.PluginConfigs, 0) + assert.Len(t, res.Routes, 3) + assert.Equal(t, "", res.Routes[0].PluginConfigId) + assert.Equal(t, ar.Spec.HTTP[1].PluginConfigName, res.Routes[1].PluginConfigId) + assert.Equal(t, "", res.Routes[2].PluginConfigId) +} + func TestTranslateApisixRouteV2alpha1NotStrictly(t *testing.T) { tr := &translator{ &TranslatorOptions{}, From c090ab76736f601e3a368780e04f8a13f0703293 Mon Sep 17 00:00:00 2001 From: nevercase <1024769485@qq.com> Date: Wed, 26 Jan 2022 13:05:44 +0800 Subject: [PATCH 3/7] fix: translate route e2e test cases bug --- test/e2e/ingress/resourcepushing.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/ingress/resourcepushing.go b/test/e2e/ingress/resourcepushing.go index 66f8069432..9a1fd6baf4 100644 --- a/test/e2e/ingress/resourcepushing.go +++ b/test/e2e/ingress/resourcepushing.go @@ -423,7 +423,7 @@ spec: assert.Nil(ginkgo.GinkgoT(), s.CreateResourceFromString(apisixRoute), "creating ApisixRoute") assert.Nil(ginkgo.GinkgoT(), s.EnsureNumApisixRoutesCreated(2)) assert.Nil(ginkgo.GinkgoT(), s.EnsureNumApisixUpstreamsCreated(1)) - assert.Nil(ginkgo.GinkgoT(), s.EnsureNumApisixPluginConfigCreated(1)) + assert.Nil(ginkgo.GinkgoT(), s.EnsureNumApisixPluginConfigCreated(0)) // Hit rule1 resp := s.NewAPISIXClient().GET("/ip").WithHeader("Host", "httpbin.com").Expect() From 3905cd7fe4ba07f324376f40280b1633a5813d34 Mon Sep 17 00:00:00 2001 From: Jintao Zhang Date: Wed, 9 Feb 2022 17:03:06 +0800 Subject: [PATCH 4/7] chore: rename function name Signed-off-by: Jintao Zhang --- pkg/kube/translation/apisix_route_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/kube/translation/apisix_route_test.go b/pkg/kube/translation/apisix_route_test.go index 9534ffcbba..662db988c5 100644 --- a/pkg/kube/translation/apisix_route_test.go +++ b/pkg/kube/translation/apisix_route_test.go @@ -281,7 +281,7 @@ func mockTranslator(t *testing.T) (*translator, <-chan struct{}) { return tr, processCh } -func TestTranslateApisixRouteV2alpha1WithDuplicatedName(t *testing.T) { +func TestTranslateApisixRouteV2beta3WithDuplicatedName(t *testing.T) { tr, processCh := mockTranslator(t) <-processCh <-processCh @@ -334,7 +334,7 @@ func TestTranslateApisixRouteV2alpha1WithDuplicatedName(t *testing.T) { assert.Equal(t, "duplicated route rule name", err.Error()) } -func TestTranslateApisixRouteV2alpha1WithEmptyPluginConfigName(t *testing.T) { +func TestTranslateApisixRouteV2beta3WithEmptyPluginConfigName(t *testing.T) { tr, processCh := mockTranslator(t) <-processCh <-processCh @@ -407,7 +407,7 @@ func TestTranslateApisixRouteV2alpha1WithEmptyPluginConfigName(t *testing.T) { assert.Equal(t, "", res.Routes[2].PluginConfigId) } -func TestTranslateApisixRouteV2alpha1NotStrictly(t *testing.T) { +func TestTranslateApisixRouteV2beta3NotStrictly(t *testing.T) { tr := &translator{ &TranslatorOptions{}, } From 6a1497994d86677ae381016faa79ee0207d84742 Mon Sep 17 00:00:00 2001 From: nevercase <1024769485@qq.com> Date: Fri, 18 Feb 2022 11:40:34 +0800 Subject: [PATCH 5/7] =?UTF-8?q?bug:=20apisix=20shouldn=E2=80=99t=20use=20A?= =?UTF-8?q?pisixPluginConfig.Name=20as=20the=20router.PluginConfigId?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/kube/translation/apisix_route.go | 2 +- pkg/kube/translation/apisix_route_test.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/kube/translation/apisix_route.go b/pkg/kube/translation/apisix_route.go index 772fa09513..7ec65b4147 100644 --- a/pkg/kube/translation/apisix_route.go +++ b/pkg/kube/translation/apisix_route.go @@ -531,7 +531,7 @@ func (t *translator) translateHTTPRouteV2beta3(ctx *TranslateContext, ar *config route.Plugins = pluginMap route.Timeout = timeout if part.PluginConfigName != "" { - route.PluginConfigId = part.PluginConfigName + route.PluginConfigId = id.GenID(apisixv1.ComposePluginConfigName(ar.Namespace, part.PluginConfigName)) } if len(backends) > 0 { diff --git a/pkg/kube/translation/apisix_route_test.go b/pkg/kube/translation/apisix_route_test.go index 662db988c5..7a63c7c554 100644 --- a/pkg/kube/translation/apisix_route_test.go +++ b/pkg/kube/translation/apisix_route_test.go @@ -33,6 +33,7 @@ import ( fakeapisix "github.com/apache/apisix-ingress-controller/pkg/kube/apisix/client/clientset/versioned/fake" apisixinformers "github.com/apache/apisix-ingress-controller/pkg/kube/apisix/client/informers/externalversions" _const "github.com/apache/apisix-ingress-controller/pkg/kube/apisix/const" + apisixv1 "github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1" ) func TestRouteMatchExpr(t *testing.T) { @@ -403,7 +404,8 @@ func TestTranslateApisixRouteV2beta3WithEmptyPluginConfigName(t *testing.T) { assert.Len(t, res.PluginConfigs, 0) assert.Len(t, res.Routes, 3) assert.Equal(t, "", res.Routes[0].PluginConfigId) - assert.Equal(t, ar.Spec.HTTP[1].PluginConfigName, res.Routes[1].PluginConfigId) + expectedPluginId := id.GenID(apisixv1.ComposePluginConfigName(ar.Namespace, ar.Spec.HTTP[1].PluginConfigName)) + assert.Equal(t, expectedPluginId, res.Routes[1].PluginConfigId) assert.Equal(t, "", res.Routes[2].PluginConfigId) } From 19748ad4dd5a7d78ab991ca93192eedc4795df7e Mon Sep 17 00:00:00 2001 From: nevercase <1024769485@qq.com> Date: Wed, 23 Feb 2022 16:39:05 +0800 Subject: [PATCH 6/7] bug: only check pluginName when creating the ApisixRoute and remove replacement --- pkg/ingress/apisix_route.go | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/pkg/ingress/apisix_route.go b/pkg/ingress/apisix_route.go index 8d601a4efd..b787e89efe 100644 --- a/pkg/ingress/apisix_route.go +++ b/pkg/ingress/apisix_route.go @@ -92,9 +92,8 @@ func (c *apisixRouteController) sync(ctx context.Context, ev *types.Event) error return err } var ( - ar kube.ApisixRoute - replaced *v2beta3.ApisixRoute - tctx *translation.TranslateContext + ar kube.ApisixRoute + tctx *translation.TranslateContext ) switch obj.GroupVersion { case kube.ApisixRouteV2beta1: @@ -164,8 +163,8 @@ func (c *apisixRouteController) sync(ctx context.Context, ev *types.Event) error } case kube.ApisixRouteV2beta3: if ev.Type != types.EventDelete { - if replaced, err = c.replacePluginNameWithIdIfNotEmptyV2beta3(ctx, ar.V2beta3()); err == nil { - tctx, err = c.controller.translator.TranslateRouteV2beta3(replaced) + if err = c.checkPluginNameIfNotEmptyV2beta3(ctx, ar.V2beta3()); err == nil { + tctx, err = c.controller.translator.TranslateRouteV2beta3(ar.V2beta3()) } } else { tctx, err = c.controller.translator.TranslateRouteV2beta3NotStrictly(ar.V2beta3()) @@ -211,9 +210,7 @@ func (c *apisixRouteController) sync(ctx context.Context, ev *types.Event) error case kube.ApisixRouteV2beta2: oldCtx, err = c.controller.translator.TranslateRouteV2beta2(obj.OldObject.V2beta2()) case kube.ApisixRouteV2beta3: - if replaced, err = c.replacePluginNameWithIdIfNotEmptyV2beta3(ctx, obj.OldObject.V2beta3()); err == nil { - oldCtx, err = c.controller.translator.TranslateRouteV2beta3(replaced) - } + oldCtx, err = c.controller.translator.TranslateRouteV2beta3(obj.OldObject.V2beta3()) } if err != nil { log.Errorw("failed to translate old ApisixRoute", @@ -237,27 +234,20 @@ func (c *apisixRouteController) sync(ctx context.Context, ev *types.Event) error return c.controller.syncManifests(ctx, added, updated, deleted) } -func (c *apisixRouteController) replacePluginNameWithIdIfNotEmptyV2beta3(ctx context.Context, in *v2beta3.ApisixRoute) (*v2beta3.ApisixRoute, error) { - clusterName := c.controller.cfg.APISIX.DefaultClusterName - news := make([]v2beta3.ApisixRouteHTTP, 0) +func (c *apisixRouteController) checkPluginNameIfNotEmptyV2beta3(ctx context.Context, in *v2beta3.ApisixRoute) error { for _, v := range in.Spec.HTTP { - pluginConfigId := "" if v.PluginConfigName != "" { - pc, err := c.controller.apisix.Cluster(clusterName).PluginConfig().Get(ctx, apisixv1.ComposePluginConfigName(in.Namespace, v.PluginConfigName)) + _, err := c.controller.apisix.Cluster(c.controller.cfg.APISIX.DefaultClusterName).PluginConfig().Get(ctx, apisixv1.ComposePluginConfigName(in.Namespace, v.PluginConfigName)) if err != nil { - log.Errorw("replacePluginNameWithIdIfNotEmptyV2beta3 error: plugin_config not found", + log.Errorw("checkPluginNameIfNotEmptyV2beta3 error: plugin_config not found", zap.String("name", apisixv1.ComposePluginConfigName(in.Namespace, v.PluginConfigName)), zap.Any("obj", in), zap.Error(err)) - } else { - pluginConfigId = pc.ID + return err } } - v.PluginConfigName = pluginConfigId - news = append(news, v) } - in.Spec.HTTP = news - return in, nil + return nil } func (c *apisixRouteController) handleSyncErr(obj interface{}, errOrigin error) { From 4b6cce04474623a0cb0653d5f16babb50b9766a7 Mon Sep 17 00:00:00 2001 From: nevercase <1024769485@qq.com> Date: Wed, 23 Feb 2022 19:41:37 +0800 Subject: [PATCH 7/7] bug: translateHTTPRouteV2beta3NotStrictly also set the field `PluginConfigId` --- pkg/ingress/apisix_route.go | 16 ++++++++++++---- pkg/kube/translation/apisix_route.go | 3 +++ pkg/kube/translation/apisix_route_test.go | 4 ++++ 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/pkg/ingress/apisix_route.go b/pkg/ingress/apisix_route.go index b787e89efe..a0248f9fa3 100644 --- a/pkg/ingress/apisix_route.go +++ b/pkg/ingress/apisix_route.go @@ -25,6 +25,7 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" + apisixcache "github.com/apache/apisix-ingress-controller/pkg/apisix/cache" "github.com/apache/apisix-ingress-controller/pkg/kube" "github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v2beta3" "github.com/apache/apisix-ingress-controller/pkg/kube/translation" @@ -239,10 +240,17 @@ func (c *apisixRouteController) checkPluginNameIfNotEmptyV2beta3(ctx context.Con if v.PluginConfigName != "" { _, err := c.controller.apisix.Cluster(c.controller.cfg.APISIX.DefaultClusterName).PluginConfig().Get(ctx, apisixv1.ComposePluginConfigName(in.Namespace, v.PluginConfigName)) if err != nil { - log.Errorw("checkPluginNameIfNotEmptyV2beta3 error: plugin_config not found", - zap.String("name", apisixv1.ComposePluginConfigName(in.Namespace, v.PluginConfigName)), - zap.Any("obj", in), - zap.Error(err)) + if err == apisixcache.ErrNotFound { + log.Errorw("checkPluginNameIfNotEmptyV2beta3 error: plugin_config not found", + zap.String("name", apisixv1.ComposePluginConfigName(in.Namespace, v.PluginConfigName)), + zap.Any("obj", in), + zap.Error(err)) + } else { + log.Errorw("checkPluginNameIfNotEmptyV2beta3 PluginConfig get failed", + zap.String("name", apisixv1.ComposePluginConfigName(in.Namespace, v.PluginConfigName)), + zap.Any("obj", in), + zap.Error(err)) + } return err } } diff --git a/pkg/kube/translation/apisix_route.go b/pkg/kube/translation/apisix_route.go index 7ec65b4147..cb0df7d62b 100644 --- a/pkg/kube/translation/apisix_route.go +++ b/pkg/kube/translation/apisix_route.go @@ -964,6 +964,9 @@ func (t *translator) translateHTTPRouteV2beta3NotStrictly(ctx *TranslateContext, route := apisixv1.NewDefaultRoute() route.Name = apisixv1.ComposeRouteName(ar.Namespace, ar.Name, part.Name) route.ID = id.GenID(route.Name) + if part.PluginConfigName != "" { + route.PluginConfigId = id.GenID(apisixv1.ComposePluginConfigName(ar.Namespace, part.PluginConfigName)) + } ctx.addRoute(route) if !ctx.checkUpstreamExist(upstreamName) { diff --git a/pkg/kube/translation/apisix_route_test.go b/pkg/kube/translation/apisix_route_test.go index 7a63c7c554..56517df7cd 100644 --- a/pkg/kube/translation/apisix_route_test.go +++ b/pkg/kube/translation/apisix_route_test.go @@ -445,6 +445,7 @@ func TestTranslateApisixRouteV2beta3NotStrictly(t *testing.T) { }, }, }, + PluginConfigName: "echo-and-cors-apc", }, { Name: "rule2", @@ -478,6 +479,9 @@ func TestTranslateApisixRouteV2beta3NotStrictly(t *testing.T) { assert.Equal(t, id.GenID("test_ar_rule1"), tx.Routes[0].ID, "route1 id error") assert.Equal(t, id.GenID("test_ar_rule2"), tx.Routes[1].ID, "route2 id error") + assert.Equal(t, id.GenID(apisixv1.ComposePluginConfigName(ar.Namespace, ar.Spec.HTTP[0].PluginConfigName)), tx.Routes[0].PluginConfigId, "route1 PluginConfigId error") + assert.Equal(t, "", tx.Routes[1].PluginConfigId, "route2 PluginConfigId error ") + assert.Equal(t, id.GenID("test_svc1_81"), tx.Upstreams[0].ID, "upstream1 id error") assert.Equal(t, id.GenID("test_svc2_82"), tx.Upstreams[1].ID, "upstream2 id error") }