fix: use not strictly translation when remote ApisixRoute (#544)

* fix: use not strictly translation when remote ApisixRoute

* fix: lint & comments

* add comments

* fix: add upstreams to ctx when translation

* use upstraemId instead of upstream in tcproute

* fix: check upstream exist

* test: add e2e test

* CI: [Temporary changes]use APISIX:2.6 instead of APISIX:dev

* use Apisix:dev image & use intstring type for counter

* fix: counter

* fix: use json.Unmarshal instead

* fix: the count in cache should be modify too

* recover test case

* fix: style

* fix: count compare

* fix: style

* fix: remove ginkgo focus test case

Author: gxthrj

pkg/apisix/resource.go

@@ -18,6 +18,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"strconv"
 	"strings"
 
 	"github.com/apache/apisix-ingress-controller/pkg/log"
@@ -30,8 +31,23 @@ type getResponse struct {
 
 // listResponse is the unified LIST response mapping of APISIX.
 type listResponse struct {
-	Count string `json:"count"`
-	Node  node   `json:"node"`
+	Count IntOrString `json:"count"`
+	Node  node        `json:"node"`
+}
+
+// IntOrString processing number and string types, after json deserialization will output int
+type IntOrString struct {
+	IntValue int `json:"int_value"`
+}
+
+func (ios *IntOrString) UnmarshalJSON(p []byte) error {
+	result := strings.Trim(string(p), "\"")
+	count, err := strconv.Atoi(result)
+	if err != nil {
+		return err
+	}
+	ios.IntValue = count
+	return nil
 }
 
 type createResponse struct {

pkg/ingress/apisix_route.go

@@ -139,7 +139,14 @@ func (c *apisixRouteController) sync(ctx context.Context, ev *types.Event) error
 			return err
 		}
 	} else {
-		tctx, err = c.controller.translator.TranslateRouteV2alpha1(ar.V2alpha1())
+		if ev.Type != types.EventDelete {
+			tctx, err = c.controller.translator.TranslateRouteV2alpha1(ar.V2alpha1())
+		} else {
+			// Use TranslateRouteV2alpha1NotStrictly in EventDelete.
+			// if K8S service has been removed before ApisixRoute resource, the translation about nodes
+			// of upstream will be failed.
+			tctx, err = c.controller.translator.TranslateRouteV2alpha1NotStrictly(ar.V2alpha1())
+		}
 		if err != nil {
 			log.Errorw("failed to translate ApisixRoute v2alpha1",
 				zap.Error(err),

pkg/kube/translation/apisix_route.go

@@ -78,6 +78,21 @@ func (t *translator) TranslateRouteV1(ar *configv1.ApisixRoute) (*TranslateConte
 	return ctx, nil
 }
 
+// TranslateRouteV2alpha1NotStrictly translates route v2alpha1 with a loose way, only generate ID and Name for delete Event.
+func (t *translator) TranslateRouteV2alpha1NotStrictly(ar *configv2alpha1.ApisixRoute) (*TranslateContext, error) {
+	ctx := &TranslateContext{
+		upstreamMap: make(map[string]struct{}),
+	}
+
+	if err := t.translateHTTPRouteNotStrictly(ctx, ar); err != nil {
+		return nil, err
+	}
+	if err := t.translateTCPRouteNotStrictly(ctx, ar); err != nil {
+		return nil, err
+	}
+	return ctx, nil
+}
+
 func (t *translator) TranslateRouteV2alpha1(ar *configv2alpha1.ApisixRoute) (*TranslateContext, error) {
 	ctx := &TranslateContext{
 		upstreamMap: make(map[string]struct{}),
@@ -92,6 +107,32 @@ func (t *translator) TranslateRouteV2alpha1(ar *configv2alpha1.ApisixRoute) (*Tr
 	return ctx, nil
 }
 
+// translateHTTPRouteNotStrictly translates http route with a loose way, only generate ID and Name for delete Event.
+func (t *translator) translateHTTPRouteNotStrictly(ctx *TranslateContext, ar *configv2alpha1.ApisixRoute) error {
+	for _, part := range ar.Spec.HTTP {
+		backends := part.Backends
+		backend := part.Backend
+		if len(backends) > 0 {
+			// Use the first backend as the default backend in Route,
+			// others will be configured in traffic-split plugin.
+			backend = backends[0]
+		} // else use the deprecated Backend.
+		upstreamName := apisixv1.ComposeUpstreamName(ar.Namespace, backend.ServiceName, backend.Subset, backend.ServicePort.IntVal)
+		route := apisixv1.NewDefaultRoute()
+		route.Name = apisixv1.ComposeRouteName(ar.Namespace, ar.Name, part.Name)
+		route.ID = id.GenID(route.Name)
+		ctx.addRoute(route)
+		if !ctx.checkUpstreamExist(upstreamName) {
+			ups, err := t.translateUpstreamNotStrictly(ar.Namespace, backend.ServiceName, backend.Subset, backend.ServicePort.IntVal)
+			if err != nil {
+				return err
+			}
+			ctx.addUpstream(ups)
+		}
+	}
+	return nil
+}
+
 func (t *translator) translateHTTPRoute(ctx *TranslateContext, ar *configv2alpha1.ApisixRoute) error {
 	ruleNameMap := make(map[string]struct{})
 	for _, part := range ar.Spec.HTTP {
@@ -304,6 +345,27 @@ func (t *translator) translateRouteMatchExprs(nginxVars []configv2alpha1.ApisixR
 	return vars, nil
 }
 
+// translateTCPRouteNotStrictly translates tcp route with a loose way, only generate ID and Name for delete Event.
+func (t *translator) translateTCPRouteNotStrictly(ctx *TranslateContext, ar *configv2alpha1.ApisixRoute) error {
+	for _, part := range ar.Spec.TCP {
+		backend := &part.Backend
+		sr := apisixv1.NewDefaultStreamRoute()
+		name := apisixv1.ComposeStreamRouteName(ar.Namespace, ar.Name, part.Name)
+		sr.ID = id.GenID(name)
+		sr.ServerPort = part.Match.IngressPort
+		ups, err := t.translateUpstreamNotStrictly(ar.Namespace, backend.ServiceName, backend.Subset, backend.ServicePort.IntVal)
+		if err != nil {
+			return err
+		}
+		sr.UpstreamId = ups.ID
+		ctx.addStreamRoute(sr)
+		if !ctx.checkUpstreamExist(ups.Name) {
+			ctx.addUpstream(ups)
+		}
+	}
+	return nil
+}
+
 func (t *translator) translateTCPRoute(ctx *TranslateContext, ar *configv2alpha1.ApisixRoute) error {
 	ruleNameMap := make(map[string]struct{})
 	for _, part := range ar.Spec.TCP {
@@ -329,8 +391,12 @@ func (t *translator) translateTCPRoute(ctx *TranslateContext, ar *configv2alpha1
 		if err != nil {
 			return err
 		}
-		sr.Upstream = ups
+		sr.UpstreamId = ups.ID
 		ctx.addStreamRoute(sr)
+		if !ctx.checkUpstreamExist(ups.Name) {
+			ctx.addUpstream(ups)
+		}
+
 	}
 	return nil
 }

pkg/kube/translation/apisix_route_test.go

@@ -16,6 +16,7 @@ package translation
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -26,6 +27,7 @@ import (
 	"k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/tools/cache"
 
+	"github.com/apache/apisix-ingress-controller/pkg/id"
 	configv2alpha1 "github.com/apache/apisix-ingress-controller/pkg/kube/apisix/apis/config/v2alpha1"
 	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"
@@ -320,3 +322,63 @@ func TestTranslateApisixRouteV2alpha1WithDuplicatedName(t *testing.T) {
 	_, err = tr.TranslateRouteV2alpha1(ar)
 	assert.Equal(t, err.Error(), "duplicated route rule name")
 }
+
+func TestTranslateApisixRouteV2alpha1NotStrictly(t *testing.T) {
+	tr := &translator{
+		&TranslatorOptions{},
+	}
+	ar := &configv2alpha1.ApisixRoute{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "ar",
+			Namespace: "test",
+		},
+		Spec: &configv2alpha1.ApisixRouteSpec{
+			HTTP: []*configv2alpha1.ApisixRouteHTTP{
+				{
+					Name: "rule1",
+					Match: &configv2alpha1.ApisixRouteHTTPMatch{
+						Paths: []string{
+							"/*",
+						},
+					},
+					Backend: &configv2alpha1.ApisixRouteHTTPBackend{
+						ServiceName: "svc1",
+						ServicePort: intstr.IntOrString{
+							IntVal: 81,
+						},
+					},
+				},
+				{
+					Name: "rule2",
+					Match: &configv2alpha1.ApisixRouteHTTPMatch{
+						Paths: []string{
+							"/*",
+						},
+					},
+					Backend: &configv2alpha1.ApisixRouteHTTPBackend{
+						ServiceName: "svc2",
+						ServicePort: intstr.IntOrString{
+							IntVal: 82,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	tx, err := tr.TranslateRouteV2alpha1NotStrictly(ar)
+	fmt.Println(tx)
+	assert.NoError(t, err, "translateRoute not strictly should be no error")
+	assert.Equal(t, len(tx.Routes), 2, "There should be 2 routes")
+	assert.Equal(t, len(tx.Upstreams), 2, "There should be 2 upstreams")
+	assert.Equal(t, tx.Routes[0].Name, "test_ar_rule1", "route1 name error")
+	assert.Equal(t, tx.Routes[1].Name, "test_ar_rule2", "route2 name error")
+	assert.Equal(t, tx.Upstreams[0].Name, "test_svc1_81", "upstream1 name error")
+	assert.Equal(t, tx.Upstreams[1].Name, "test_svc2_82", "upstream2 name error")
+
+	assert.Equal(t, tx.Routes[0].ID, id.GenID("test_ar_rule1"), "route1 id error")
+	assert.Equal(t, tx.Routes[1].ID, id.GenID("test_ar_rule2"), "route2 id error")
+	assert.Equal(t, tx.Upstreams[0].ID, id.GenID("test_svc1_81"), "upstream1 id error")
+	assert.Equal(t, tx.Upstreams[1].ID, id.GenID("test_svc2_82"), "upstream2 id error")
+
+}

pkg/kube/translation/translator.go

@@ -70,6 +70,9 @@ type Translator interface {
 	// TranslateRouteV2alpha1 translates the configv2alpha1.ApisixRoute object into several Route
 	// and Upstream resources.
 	TranslateRouteV2alpha1(*configv2alpha1.ApisixRoute) (*TranslateContext, error)
+	// TranslateRouteV2alpha1NotStrictly translates the configv2alpha1.ApisixRoute object into several Route
+	// and Upstream resources not strictly, only used for delete event.
+	TranslateRouteV2alpha1NotStrictly(*configv2alpha1.ApisixRoute) (*TranslateContext, error)
 	// TranslateSSL translates the configv2alpha1.ApisixTls object into the APISIX SSL resource.
 	TranslateSSL(*configv1.ApisixTls) (*apisixv1.Ssl, error)
 	// TranslateClusterConfig translates the configv2alpha1.ApisixClusterConfig object into the APISIX

pkg/kube/translation/util.go

@@ -110,6 +110,14 @@ loop:
 	return svc.Spec.ClusterIP, svcPort, nil
 }
 
+// translateUpstreamNotStrictly translates Upstream nodes with a loose way, only generate ID and Name for delete Event.
+func (t *translator) translateUpstreamNotStrictly(namespace, svcName, subset string, svcPort int32) (*apisixv1.Upstream, error) {
+	ups := &apisixv1.Upstream{}
+	ups.Name = apisixv1.ComposeUpstreamName(namespace, svcName, subset, svcPort)
+	ups.ID = id.GenID(ups.Name)
+	return ups, nil
+}
+
 func (t *translator) translateUpstream(namespace, svcName, subset, svcResolveGranularity, svcClusterIP string, svcPort int32) (*apisixv1.Upstream, error) {
 	ups, err := t.TranslateUpstream(namespace, svcName, subset, svcPort)
 	if err != nil {

test/e2e/ingress/resourcepushing.go

@@ -150,6 +150,86 @@ spec:
 		assert.Contains(ginkgo.GinkgoT(), body, "404 Route Not Found")
 	})
 
+	ginkgo.It("create, update, remove k8s service, remove ApisixRoute", func() {
+		backendSvc, backendSvcPort := s.DefaultHTTPBackend()
+		apisixRoute := fmt.Sprintf(`
+apiVersion: apisix.apache.org/v2alpha1
+kind: ApisixRoute
+metadata:
+  name: httpbin-route
+spec:
+  http:
+  - name: rule1
+    match:
+      hosts:
+      - httpbin.com
+      paths:
+      - /ip
+    backend:
+      serviceName: %s
+      servicePort: %d
+`, backendSvc, backendSvcPort[0])
+
+		assert.Nil(ginkgo.GinkgoT(), s.CreateResourceFromString(apisixRoute), "creating ApisixRoute")
+		err := s.EnsureNumApisixRoutesCreated(1)
+		assert.Nil(ginkgo.GinkgoT(), err, "Checking number of routes")
+		err = s.EnsureNumApisixUpstreamsCreated(1)
+		assert.Nil(ginkgo.GinkgoT(), err, "Checking number of upstreams")
+
+		s.NewAPISIXClient().GET("/ip").WithHeader("Host", "httpbin.com").Expect().Status(http.StatusOK)
+
+		// update
+		apisixRoute = fmt.Sprintf(`
+apiVersion: apisix.apache.org/v2alpha1
+kind: ApisixRoute
+metadata:
+  name: httpbin-route
+spec:
+  http:
+  - name: rule1
+    match:
+      hosts:
+      - httpbin.com
+      paths:
+      - /ip
+      exprs:
+      - subject:
+          scope: Header
+          name: X-Foo
+        op: Equal
+        value: "barbaz"
+    backend:
+      serviceName: %s
+      servicePort: %d
+`, backendSvc, backendSvcPort[0])
+
+		assert.Nil(ginkgo.GinkgoT(), s.CreateResourceFromString(apisixRoute))
+		// TODO When ingress controller can feedback the lifecycle of CRDs to the
+		// status field, we can poll it rather than sleeping.
+		time.Sleep(10 * time.Second)
+
+		err = s.EnsureNumApisixRoutesCreated(1)
+		assert.Nil(ginkgo.GinkgoT(), err, "Checking number of routes")
+		err = s.EnsureNumApisixUpstreamsCreated(1)
+		assert.Nil(ginkgo.GinkgoT(), err, "Checking number of upstreams")
+
+		s.NewAPISIXClient().GET("/ip").WithHeader("Host", "httpbin.com").Expect().Status(http.StatusNotFound)
+		s.NewAPISIXClient().GET("/ip").WithHeader("Host", "httpbin.com").WithHeader("X-Foo", "barbaz").Expect().Status(http.StatusOK)
+		// remove k8s service first
+		s.DeleteHTTPBINService()
+		// remove
+		assert.Nil(ginkgo.GinkgoT(), s.RemoveResourceByString(apisixRoute))
+		// TODO When ingress controller can feedback the lifecycle of CRDs to the
+		// status field, we can poll it rather than sleeping.
+		time.Sleep(10 * time.Second)
+		ups, err := s.ListApisixUpstreams()
+		assert.Nil(ginkgo.GinkgoT(), err, "list upstreams error")
+		assert.Len(ginkgo.GinkgoT(), ups, 0, "upstreams nodes not expect")
+
+		body := s.NewAPISIXClient().GET("/ip").WithHeader("Host", "httpbin.com").Expect().Status(http.StatusNotFound).Body().Raw()
+		assert.Contains(ginkgo.GinkgoT(), body, "404 Route Not Found")
+	})
+
 	ginkgo.It("change route rule name", func() {
 		backendSvc, backendSvcPort := s.DefaultHTTPBackend()
 		apisixRoute := fmt.Sprintf(`

test/e2e/scaffold/k8s.go

@@ -17,9 +17,9 @@ package scaffold
 import (
 	"context"
 	"encoding/json"
+	"io/ioutil"
 	"net/http"
 	"net/url"
-	"strconv"
 	"time"
 
 	"github.com/apache/apisix-ingress-controller/pkg/apisix"
@@ -33,7 +33,7 @@ import (
 )
 
 type counter struct {
-	Count string `json:"count"`
+	Count apisix.IntOrString `json:"count"`
 }
 
 // ApisixRoute is the ApisixRoute CRD definition.
@@ -150,19 +150,18 @@ func (s *Scaffold) ensureNumApisixCRDsCreated(url string, desired int) error {
 			ginkgo.GinkgoT().Logf("got status code %d from APISIX", resp.StatusCode)
 			return false, nil
 		}
-		var c counter
-		dec := json.NewDecoder(resp.Body)
-		if err := dec.Decode(&c); err != nil {
+		c := &counter{}
+		b, err := ioutil.ReadAll(resp.Body)
+		if err != nil {
 			return false, err
 		}
-		// NOTE count field is a string.
-		count, err := strconv.Atoi(c.Count)
+		err = json.Unmarshal(b, c)
 		if err != nil {
 			return false, err
 		}
-		// 1 for dir.
-		if count != desired+1 {
-			ginkgo.GinkgoT().Logf("mismatched number of items, expected %d but found %d", desired, count-1)
+		count := c.Count.IntValue
+		if count != desired {
+			ginkgo.GinkgoT().Logf("mismatched number of items, expected %d but found %d", desired, count)
 			return false, nil
 		}
 		return true, nil