fix: handle apiGroup updates in resource-tracking (argoproj#11012)

* fix: handle apiGroup updates in resource-tracking Signed-off-by: Leonardo Luz Almeida <[email protected]> * Fix test Signed-off-by: Leonardo Luz Almeida <[email protected]> * change the fix approach by inspecting tracking id from the config Signed-off-by: Leonardo Luz Almeida <[email protected]> * add unit-test to validate the scenario Signed-off-by: Leonardo Luz Almeida <[email protected]> * fix test lint Signed-off-by: Leonardo Luz Almeida <[email protected]> * review fixes Signed-off-by: Leonardo Luz Almeida <[email protected]> * Reword godocs for clarity Signed-off-by: Leonardo Luz Almeida <[email protected]> Signed-off-by: Leonardo Luz Almeida <[email protected]>
aspring · Nov 3, 2022 · 327936d · 327936d
1 parent 9fa3c4f
commit 327936d
Show file tree

Hide file tree

Showing 5 changed files with 140 additions and 58 deletions.
diff --git a/controller/state.go b/controller/state.go
@@ -514,7 +514,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
 		}
 		gvk := obj.GroupVersionKind()
 
-		isSelfReferencedObj := m.isSelfReferencedObj(liveObj, appLabelKey, trackingMethod)
+		isSelfReferencedObj := m.isSelfReferencedObj(liveObj, targetObj, app.GetName(), appLabelKey, trackingMethod)
 
 		resState := v1alpha1.ResourceStatus{
 			Namespace:       obj.GetNamespace(),
@@ -699,12 +699,13 @@ func NewAppStateManager(
 }
 
 // isSelfReferencedObj returns whether the given obj is managed by the application
-// according to the values in the tracking annotation. It returns true when all
-// of the properties in the annotation (name, namespace, group and kind) match
-// the properties of the inspected object, or if the tracking method used does
-// not provide the required properties for matching.
-func (m *appStateManager) isSelfReferencedObj(obj *unstructured.Unstructured, appLabelKey string, trackingMethod v1alpha1.TrackingMethod) bool {
-	if obj == nil {
+// according to the values of the tracking id (aka app instance value) annotation.
+// It returns true when all of the properties of the tracking id (app name, namespace,
+// group and kind) match the properties of the live object, or if the tracking method
+// used does not provide the required properties for matching.
+// Reference: https://github.com/argoproj/argo-cd/issues/8683
+func (m *appStateManager) isSelfReferencedObj(live, config *unstructured.Unstructured, appName, appLabelKey string, trackingMethod v1alpha1.TrackingMethod) bool {
+	if live == nil {
 		return true
 	}
 
@@ -714,17 +715,42 @@ func (m *appStateManager) isSelfReferencedObj(obj *unstructured.Unstructured, ap
 		return true
 	}
 
-	// In order for us to assume obj to be managed by this application, the
-	// values from the annotation have to match the properties from the live
-	// object. Cluster scoped objects carry the app's destination namespace
-	// in the tracking annotation, but are unique in GVK + name combination.
-	appInstance := m.resourceTracking.GetAppInstance(obj, appLabelKey, trackingMethod)
+	// config != nil is the best-case scenario for constructing an accurate
+	// Tracking ID. `config` is the "desired state" (from git/helm/etc.).
+	// Using the desired state is important when there is an ApiGroup upgrade.
+	// When upgrading, the comparison must be made with the new tracking ID.
+	// Example:
+	//     live resource annotation will be:
+	//        ingress-app:extensions/Ingress:default/some-ingress
+	//     when it should be:
+	//        ingress-app:networking.k8s.io/Ingress:default/some-ingress
+	// More details in: https://github.com/argoproj/argo-cd/pull/11012
+	var aiv argo.AppInstanceValue
+	if config != nil {
+		aiv = argo.UnstructuredToAppInstanceValue(config, appName, "")
+		return isSelfReferencedObj(live, aiv)
+	}
+
+	// If config is nil then compare the live resource with the value
+	// of the annotation. In this case, in order to validate if obj is
+	// managed by this application, the values from the annotation have
+	// to match the properties from the live object. Cluster scoped objects
+	// carry the app's destination namespace in the tracking annotation,
+	// but are unique in GVK + name combination.
+	appInstance := m.resourceTracking.GetAppInstance(live, appLabelKey, trackingMethod)
 	if appInstance != nil {
-		return (obj.GetNamespace() == appInstance.Namespace || obj.GetNamespace() == "") &&
-			obj.GetName() == appInstance.Name &&
-			obj.GetObjectKind().GroupVersionKind().Group == appInstance.Group &&
-			obj.GetObjectKind().GroupVersionKind().Kind == appInstance.Kind
+		return isSelfReferencedObj(live, *appInstance)
 	}
-
 	return true
 }
+
+// isSelfReferencedObj returns true if the given Tracking ID (`aiv`) matches
+// the given object. It returns false when the ID doesn't match. This sometimes
+// happens when a tracking label or annotation gets accidentally copied to a
+// different resource.
+func isSelfReferencedObj(obj *unstructured.Unstructured, aiv argo.AppInstanceValue) bool {
+	return (obj.GetNamespace() == aiv.Namespace || obj.GetNamespace() == "") &&
+		obj.GetName() == aiv.Name &&
+		obj.GetObjectKind().GroupVersionKind().Group == aiv.Group &&
+		obj.GetObjectKind().GroupVersionKind().Kind == aiv.Kind
+}
diff --git a/controller/state_test.go b/controller/state_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	networkingv1 "k8s.io/api/networking/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -852,6 +853,19 @@ func TestIsLiveResourceManaged(t *testing.T) {
 			},
 		},
 	})
+	managedWrongAPIGroup := kube.MustToUnstructured(&networkingv1.Ingress{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "networking.k8s.io/v1",
+			Kind:       "Ingress",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "some-ingress",
+			Namespace: "default",
+			Annotations: map[string]string{
+				common.AnnotationKeyAppInstance: "guestbook:extensions/Ingress:default/some-ingress",
+			},
+		},
+	})
 	ctrl := newFakeController(&fakeData{
 		apps: []runtime.Object{app, &defaultProj},
 		manifestResponse: &apiclient.ManifestResponse{
@@ -870,30 +884,69 @@ func TestIsLiveResourceManaged(t *testing.T) {
 	})
 
 	manager := ctrl.appStateManager.(*appStateManager)
+	appName := "guestbook"
+
+	t.Run("will return true if trackingid matches the resource", func(t *testing.T) {
+		// given
+		t.Parallel()
+		configObj := managedObj.DeepCopy()
 
-	// Managed resource w/ annotations
-	assert.True(t, manager.isSelfReferencedObj(managedObj, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
-	assert.True(t, manager.isSelfReferencedObj(managedObj, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+		// then
+		assert.True(t, manager.isSelfReferencedObj(managedObj, configObj, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
+		assert.True(t, manager.isSelfReferencedObj(managedObj, configObj, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+	})
+	t.Run("will return true if tracked with label", func(t *testing.T) {
+		// given
+		t.Parallel()
+		configObj := managedObjWithLabel.DeepCopy()
 
-	// Managed resource w/ label
-	assert.True(t, manager.isSelfReferencedObj(managedObjWithLabel, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
+		// then
+		assert.True(t, manager.isSelfReferencedObj(managedObjWithLabel, configObj, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
+	})
+	t.Run("will handle if trackingId has wrong resource name and config is nil", func(t *testing.T) {
+		// given
+		t.Parallel()
 
-	// Wrong resource name
-	assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongName, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
-	assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+		// then
+		assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongName, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
+		assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongName, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+	})
+	t.Run("will handle if trackingId has wrong resource group and config is nil", func(t *testing.T) {
+		// given
+		t.Parallel()
 
-	// Wrong resource group
-	assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongGroup, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
-	assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongGroup, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+		// then
+		assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongGroup, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
+		assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongGroup, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+	})
+	t.Run("will handle if trackingId has wrong kind and config is nil", func(t *testing.T) {
+		// given
+		t.Parallel()
 
-	// Wrong resource kind
-	assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongKind, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
-	assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongKind, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+		// then
+		assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongKind, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
+		assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongKind, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+	})
+	t.Run("will handle if trackingId has wrong namespace and config is nil", func(t *testing.T) {
+		// given
+		t.Parallel()
 
-	// Wrong resource namespace
-	assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongNamespace, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
-	assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongNamespace, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotationAndLabel))
+		// then
+		assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongNamespace, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
+		assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongNamespace, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotationAndLabel))
+	})
+	t.Run("will return true if live is nil", func(t *testing.T) {
+		t.Parallel()
+		assert.True(t, manager.isSelfReferencedObj(nil, nil, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+	})
 
-	// Nil resource
-	assert.True(t, manager.isSelfReferencedObj(nil, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+	t.Run("will handle upgrade in desired state APIGroup", func(t *testing.T) {
+		// given
+		t.Parallel()
+		config := managedWrongAPIGroup.DeepCopy()
+		delete(config.GetAnnotations(), common.AnnotationKeyAppInstance)
+
+		// then
+		assert.True(t, manager.isSelfReferencedObj(managedWrongAPIGroup, config, appName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
+	})
 }
diff --git a/controller/sync.go b/controller/sync.go
@@ -246,7 +246,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
 		sync.WithResourcesFilter(func(key kube.ResourceKey, target *unstructured.Unstructured, live *unstructured.Unstructured) bool {
 			return (len(syncOp.Resources) == 0 ||
 				argo.ContainsSyncResource(key.Name, key.Namespace, schema.GroupVersionKind{Kind: key.Kind, Group: key.Group}, syncOp.Resources)) &&
-				m.isSelfReferencedObj(live, appLabelKey, trackingMethod)
+				m.isSelfReferencedObj(live, target, app.GetName(), appLabelKey, trackingMethod)
 		}),
 		sync.WithManifestValidation(!syncOp.SyncOptions.HasOption(common.SyncOptionsDisableValidation)),
 		sync.WithNamespaceCreation(syncOp.SyncOptions.HasOption("CreateNamespace=true"), func(un *unstructured.Unstructured) bool {

diff --git a/util/argo/diff/diff.go b/util/argo/diff/diff.go
@@ -334,7 +334,7 @@ func (c *diffConfig) DiffFromCache(appName string) (bool, []*appv1.ResourceDiff)
 }
 
 // preDiffNormalize applies the normalization of live and target resources before invoking
-// the diff. None of the attributes in the preDiffNormalizeParams will be modified.
+// the diff. None of the attributes in the lives and targets params will be modified.
 func preDiffNormalize(lives, targets []*unstructured.Unstructured, diffConfig DiffConfig) (*NormalizationResult, error) {
 	if diffConfig == nil {
 		return nil, fmt.Errorf("preDiffNormalize error: diffConfig can not be nil")

diff --git a/util/argo/resource_tracking.go b/util/argo/resource_tracking.go
@@ -4,17 +4,12 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/argoproj/gitops-engine/pkg/utils/kube"
-
 	"github.com/argoproj/argo-cd/v2/common"
-
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-
-	"github.com/argoproj/argo-cd/v2/util/settings"
-
 	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
-
+	"github.com/argoproj/argo-cd/v2/util/kube"
 	argokube "github.com/argoproj/argo-cd/v2/util/kube"
+	"github.com/argoproj/argo-cd/v2/util/settings"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 const (
@@ -107,21 +102,29 @@ func (rt *resourceTracking) GetAppInstance(un *unstructured.Unstructured, key st
 	}
 }
 
+// UnstructuredToAppInstanceValue will build the AppInstanceValue based
+// on the provided unstructured. The given namespace works as a default
+// value if the resource's namespace is not defined. It should be the
+// Application's target destination namespace.
+func UnstructuredToAppInstanceValue(un *unstructured.Unstructured, appName, namespace string) AppInstanceValue {
+	ns := un.GetNamespace()
+	if ns == "" {
+		ns = namespace
+	}
+	gvk := un.GetObjectKind().GroupVersionKind()
+	return AppInstanceValue{
+		ApplicationName: appName,
+		Group:           gvk.Group,
+		Kind:            gvk.Kind,
+		Namespace:       ns,
+		Name:            un.GetName(),
+	}
+}
+
 // SetAppInstance set label/annotation base on tracking method
 func (rt *resourceTracking) SetAppInstance(un *unstructured.Unstructured, key, val, namespace string, trackingMethod v1alpha1.TrackingMethod) error {
 	setAppInstanceAnnotation := func() error {
-		ns := un.GetNamespace()
-		if ns == "" {
-			ns = namespace
-		}
-		gvk := un.GetObjectKind().GroupVersionKind()
-		appInstanceValue := AppInstanceValue{
-			ApplicationName: val,
-			Group:           gvk.Group,
-			Kind:            gvk.Kind,
-			Namespace:       ns,
-			Name:            un.GetName(),
-		}
+		appInstanceValue := UnstructuredToAppInstanceValue(un, val, namespace)
 		return argokube.SetAppInstanceAnnotation(un, common.AnnotationKeyAppInstance, rt.BuildAppInstanceValue(appInstanceValue))
 	}
 	switch trackingMethod {