Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: avoid create pluginconfig in the tranlsation of route (#836) #845

Merged
merged 12 commits into from
Mar 2, 2022
Merged

fix: avoid create pluginconfig in the tranlsation of route (#836) #845

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
05dd483
fix: aviod create pluginconfig in the tranlsation of route (#836)
neverCase Jan 25, 2022
5aada25
fix: the Route.PluginConfigId will not be written if the http PluginC…
neverCase Jan 26, 2022
c090ab7
fix: translate route e2e test cases bug
neverCase Jan 26, 2022
e50920b
Merge remote-tracking branch 'origin/master' into bug-836
neverCase Feb 8, 2022
3905cd7
chore: rename function name
tao12345666333 Feb 9, 2022
fa0443e
Merge branch 'master' into bug-836
tao12345666333 Feb 14, 2022
6a14979
bug: apisix shouldn’t use ApisixPluginConfig.Name as the router.Plugi…
neverCase Feb 18, 2022
8d723a7
Merge branch 'master' into bug-836
tao12345666333 Feb 20, 2022
19748ad
bug: only check pluginName when creating the ApisixRoute and remove r…
neverCase Feb 23, 2022
1360c6b
Merge branch 'master' into bug-836
neverCase Feb 23, 2022
4b6cce0
bug: translateHTTPRouteV2beta3NotStrictly also set the field `PluginC…
neverCase Feb 23, 2022
4547a1a
Merge remote-tracking branch 'origin/master' into bug-836
neverCase Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 10 additions & 20 deletions pkg/ingress/apisix_route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should distinguish the error type, not all errors are "not found".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

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) {
Expand Down
27 changes: 2 additions & 25 deletions pkg/kube/translation/apisix_route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = id.GenID(apisixv1.ComposePluginConfigName(ar.Namespace, part.PluginConfigName))
}

if len(backends) > 0 {
Expand All @@ -566,9 +557,6 @@ func (t *translator) translateHTTPRouteV2beta3(ctx *TranslateContext, ar *config
}
ctx.addUpstream(ups)
}
if len(pluginMap) > 0 {
ctx.addPluginConfig(pluginConfig)
}
}
return nil
}
Expand Down Expand Up @@ -851,7 +839,6 @@ func (t *translator) translateStreamRoute(ctx *TranslateContext, ar *configv2bet
if !ctx.checkUpstreamExist(ups.Name) {
ctx.addUpstream(ups)
}

}
return nil
}
Expand Down Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we don't handle the PluginConfigName in this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because when it creates the ApisixRoute and the field PluginConfigName has been specified, I want to use apisix.Cluster api to check the plugin was whether exist.
And at the same time replace it with the plugin id.
...
Since the method to generate the plugin id is fixed.
I will just remove the replacement and remain checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I mean translateHTTPRouteV2beta3NotStrictly is basically same to translateHTTPRouteV2beta3, so here you should also set the plugin config id.

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) {
Expand All @@ -993,9 +973,6 @@ func (t *translator) translateHTTPRouteV2beta3NotStrictly(ctx *TranslateContext,
}
ctx.addUpstream(ups)
}
if len(pluginMap) > 0 {
ctx.addPluginConfig(pluginConfig)
}
}
return nil
}
Expand Down
89 changes: 83 additions & 6 deletions pkg/kube/translation/apisix_route_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -185,7 +186,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{
Expand Down Expand Up @@ -278,6 +279,11 @@ func TestTranslateApisixRouteV2alpha1WithDuplicatedName(t *testing.T) {
go epInformer.Run(stopCh)
cache.WaitForCacheSync(stopCh, svcInformer.HasSynced)

return tr, processCh
}

func TestTranslateApisixRouteV2beta3WithDuplicatedName(t *testing.T) {
tr, processCh := mockTranslator(t)
<-processCh
<-processCh

Expand Down Expand Up @@ -324,12 +330,86 @@ 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 TestTranslateApisixRouteV2alpha1NotStrictly(t *testing.T) {
func TestTranslateApisixRouteV2beta3WithEmptyPluginConfigName(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)
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)
}

func TestTranslateApisixRouteV2beta3NotStrictly(t *testing.T) {
tr := &translator{
&TranslatorOptions{},
}
Expand Down Expand Up @@ -391,16 +471,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")
}
2 changes: 1 addition & 1 deletion test/e2e/ingress/resourcepushing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down