cherry-pick(#1101) fix component custom revison loop infinitely create revision (#1104)

* fix component custom revison loop infinitely create revision

* use accurate compare function for revision to componentName

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/component.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"sort"
 
+	"github.com/crossplane/crossplane-runtime/pkg/logging"
 	appsv1 "k8s.io/api/apps/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -17,8 +18,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
-	"github.com/crossplane/crossplane-runtime/pkg/logging"
-
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2"
 	"github.com/oam-dev/kubevela/pkg/controller/utils"
 )
@@ -80,7 +79,7 @@ func (c *ComponentHandler) Generic(_ event.GenericEvent, _ workqueue.RateLimitin
 func isMatch(appConfigs *v1alpha2.ApplicationConfigurationList, compName string) (bool, types.NamespacedName) {
 	for _, app := range appConfigs.Items {
 		for _, comp := range app.Spec.Components {
-			if comp.ComponentName == compName {
+			if comp.ComponentName == compName || utils.ExtractComponentName(comp.RevisionName) == compName {
 				return true, types.NamespacedName{Namespace: app.Namespace, Name: app.Name}
 			}
 		}
@@ -125,6 +124,10 @@ func (c *ComponentHandler) IsRevisionDiff(mt klog.KMetadata, curComp *v1alpha2.C
 func (c *ComponentHandler) createControllerRevision(mt metav1.Object, obj runtime.Object) ([]reconcile.Request, bool) {
 	curComp := obj.(*v1alpha2.Component)
 	comp := curComp.DeepCopy()
+	// No generation changed, will not create revision
+	if comp.Generation == comp.Status.ObservedGeneration {
+		return nil, false
+	}
 	diff, curRevision := c.IsRevisionDiff(mt, comp)
 	if !diff {
 		// No difference, no need to create new revision.

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/component_custom_revision_test.go

@@ -18,50 +18,68 @@ package applicationconfiguration
 
 import (
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2"
 )
 
-func TestCustomRevisionHook(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		var req RevisionHookRequest
-		data, err := ioutil.ReadAll(r.Body)
-		if err != nil {
-			w.WriteHeader(400)
-			return
-		}
-		err = json.Unmarshal(data, &req)
-		if err != nil {
-			w.WriteHeader(401)
-			return
-		}
-		if len(req.RelatedApps) != 1 {
-			w.WriteHeader(400)
-			w.Write([]byte("we should have only one relatedApps"))
-			return
-		}
-		if req.Comp.Annotations == nil {
-			req.Comp.Annotations = make(map[string]string)
+var RevisionHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	var req RevisionHookRequest
+	data, err := ioutil.ReadAll(r.Body)
+	if err != nil {
+		w.WriteHeader(400)
+		return
+	}
+	err = json.Unmarshal(data, &req)
+	if err != nil {
+		w.WriteHeader(401)
+		return
+	}
+	fmt.Println("got request from", req.Comp.Name)
+
+	if len(req.RelatedApps) != 1 {
+		var abc []string
+		for _, v := range req.RelatedApps {
+			abc = append(abc, v.Name)
 		}
+		// we can add a check here for real world handler
+		fmt.Printf("we should have only one relatedApps, but now %d: %s\n", len(req.RelatedApps), strings.Join(abc, ", "))
+	}
+	if req.Comp.Annotations == nil {
+		req.Comp.Annotations = make(map[string]string)
+	}
+	if len(req.RelatedApps) > 0 {
 		req.Comp.Annotations["app-name"] = req.RelatedApps[0].Name
 		req.Comp.Annotations["app-namespace"] = req.RelatedApps[0].Namespace
+	}
+	a := &unstructured.Unstructured{}
+	err = json.Unmarshal(req.Comp.Spec.Workload.Raw, a)
+	fmt.Println("XX:", err)
+	a.SetAnnotations(map[string]string{"time": time.Now().Format(time.RFC3339Nano)})
+	data, _ = json.Marshal(a)
+	req.Comp.Spec.Workload.Raw = data
+	newdata, err := json.Marshal(req.Comp)
+	if err != nil {
+		w.WriteHeader(500)
+		return
+	}
+	w.WriteHeader(200)
+	w.Write(newdata)
+})
 
-		newdata, err := json.Marshal(req.Comp)
-		if err != nil {
-			w.WriteHeader(500)
-			return
-		}
-		w.WriteHeader(200)
-		w.Write(newdata)
-	}))
+func TestCustomRevisionHook(t *testing.T) {
+	srv := httptest.NewServer(RevisionHandler)
 	defer srv.Close()
 	compHandler := ComponentHandler{
 		CustomRevisionHookURL: srv.URL,
@@ -71,7 +89,4 @@ func TestCustomRevisionHook(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "app1", comp.Annotations["app-name"])
 	assert.Equal(t, "default1", comp.Annotations["app-namespace"])
-
-	err = compHandler.customComponentRevisionHook([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "app1", Namespace: "default1"}}, {NamespacedName: types.NamespacedName{Name: "app2", Namespace: "default2"}}}, comp)
-	assert.Equal(t, err.Error(), "httpcode(400) err: we should have only one relatedApps")
 }

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go

@@ -19,18 +19,20 @@ package applicationconfiguration
 import (
 	"context"
 	"fmt"
+	"net/http/httptest"
 	"strconv"
 	"time"
 
-	v1 "k8s.io/api/apps/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
+	"github.com/crossplane/crossplane-runtime/pkg/logging"
+	v1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -340,3 +342,165 @@ var _ = Describe("Test ApplicationConfiguration Component Revision Enabled trait
 	})
 
 })
+
+var _ = Describe("Test Component Revision Enabled with custom component revision hook", func() {
+	const (
+		namespace = "revision-enable-test2"
+		compName  = "revision-test-comp2"
+	)
+	var (
+		ctx       = context.Background()
+		component v1alpha2.Component
+		ns        = corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: namespace,
+			},
+		}
+	)
+
+	BeforeEach(func() {})
+
+	AfterEach(func() {
+		// delete the namespace with all its resources
+		Expect(k8sClient.Delete(ctx, &ns, client.PropagationPolicy(metav1.DeletePropagationForeground))).
+			Should(SatisfyAny(BeNil(), &util.NotFoundMatcher{}))
+	})
+
+	It("custom component change revision lead to revision difference, it should not loop infinitely create", func() {
+		srv := httptest.NewServer(RevisionHandler)
+		defer srv.Close()
+		customComponentHandler := &ComponentHandler{Client: k8sClient, RevisionLimit: 100, Logger: logging.NewLogrLogger(ctrl.Log.WithName("component-handler")), CustomRevisionHookURL: srv.URL}
+		getDeploy := func(image string) *v1.Deployment {
+			return &v1.Deployment{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Deployment",
+					APIVersion: "apps/v1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: namespace,
+				},
+				Spec: v1.DeploymentSpec{
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{
+						"app": compName,
+					}},
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{
+							"app": compName,
+						}},
+						Spec: corev1.PodSpec{Containers: []corev1.Container{{
+							Name:  "wordpress",
+							Image: image,
+							Ports: []corev1.ContainerPort{
+								{
+									Name:          "wordpress",
+									ContainerPort: 80,
+								},
+							},
+						},
+						}}},
+				},
+			}
+		}
+		component = v1alpha2.Component{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: "core.oam.dev/v1alpha2",
+				Kind:       "Component",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      compName,
+				Namespace: namespace,
+			},
+			Spec: v1alpha2.ComponentSpec{
+				Workload: runtime.RawExtension{
+					Object: getDeploy("wordpress:4.6.1-apache"),
+				},
+			},
+		}
+
+		By("Create namespace")
+		Eventually(
+			func() error {
+				return k8sClient.Create(ctx, &ns)
+			},
+			time.Second*3, time.Millisecond*300).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{}))
+
+		By("Create Component")
+		Expect(k8sClient.Create(ctx, &component)).Should(Succeed())
+
+		By("component handler will automatically create controller revision")
+		Expect(func() bool {
+			_, ok := customComponentHandler.createControllerRevision(component.DeepCopy(), component.DeepCopy())
+			return ok
+		}()).Should(BeTrue())
+
+		By("it should not create again for the same generation component")
+		cmpV1 := &v1alpha2.Component{}
+		Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV1)).Should(Succeed())
+		Expect(func() bool {
+			_, ok := customComponentHandler.createControllerRevision(cmpV1, cmpV1)
+			return ok
+		}()).Should(BeFalse())
+
+		var crList v1.ControllerRevisionList
+		By("Check controller revision created successfully")
+		Eventually(func() error {
+			labels := &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					ControllerRevisionComponentLabel: compName,
+				},
+			}
+			selector, err := metav1.LabelSelectorAsSelector(labels)
+			if err != nil {
+				return err
+			}
+			err = k8sClient.List(ctx, &crList, &client.ListOptions{
+				LabelSelector: selector,
+			})
+			if err != nil {
+				return err
+			}
+			if len(crList.Items) != 1 {
+				return fmt.Errorf("want only 1 revision created but got %d", len(crList.Items))
+			}
+			return nil
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+
+		By("===================================== Start to Update =========================================")
+		cmpV2 := &v1alpha2.Component{}
+		Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV2)).Should(Succeed())
+		cmpV2.Spec.Workload = runtime.RawExtension{
+			Object: getDeploy("wordpress:v2"),
+		}
+		By("Update Component")
+		Expect(k8sClient.Update(ctx, cmpV2)).Should(Succeed())
+		By("component handler will automatically create a ne controller revision")
+		Expect(func() bool { _, ok := componentHandler.createControllerRevision(cmpV2, cmpV2); return ok }()).Should(BeTrue())
+
+		cmpV3 := &v1alpha2.Component{}
+		Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV3)).Should(Succeed())
+		Expect(func() bool { _, ok := componentHandler.createControllerRevision(cmpV3, cmpV3); return ok }()).Should(BeFalse())
+
+		By("Check controller revision created successfully")
+		Eventually(func() error {
+			labels := &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					ControllerRevisionComponentLabel: compName,
+				},
+			}
+			selector, err := metav1.LabelSelectorAsSelector(labels)
+			if err != nil {
+				return err
+			}
+			err = k8sClient.List(ctx, &crList, &client.ListOptions{
+				LabelSelector: selector,
+			})
+			if err != nil {
+				return err
+			}
+			if len(crList.Items) != 2 {
+				return fmt.Errorf("there should be exactly 2 revision created but got %d", len(crList.Items))
+			}
+			return nil
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+	})
+})