From 9b2cdc2ccfbe8680e18080ec58773565a3276ae5 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Thu, 3 Nov 2022 15:02:13 -0400 Subject: [PATCH] fix: handle apiGroup updates in resource-tracking (#11012) * fix: handle apiGroup updates in resource-tracking Signed-off-by: Leonardo Luz Almeida * Fix test Signed-off-by: Leonardo Luz Almeida * change the fix approach by inspecting tracking id from the config Signed-off-by: Leonardo Luz Almeida * add unit-test to validate the scenario Signed-off-by: Leonardo Luz Almeida * fix test lint Signed-off-by: Leonardo Luz Almeida * review fixes Signed-off-by: Leonardo Luz Almeida * Reword godocs for clarity Signed-off-by: Leonardo Luz Almeida Signed-off-by: Leonardo Luz Almeida --- controller/state.go | 60 +++++++++++++++------- controller/state_test.go | 91 +++++++++++++++++++++++++++------- controller/sync.go | 2 +- util/argo/diff/diff.go | 2 +- util/argo/resource_tracking.go | 43 ++++++++-------- 5 files changed, 140 insertions(+), 58 deletions(-) diff --git a/controller/state.go b/controller/state.go index 8897735be3eeb..8ee6012630458 100644 --- 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 index 055dc2f72e007..7d577ea1cb82d 100644 --- 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 index 7c7cd1108c4a6..5f597495cee0a 100644 --- 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 index ad339cde744ff..59f878036c361 100644 --- 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 index 1741bf413d05d..53659115e8b10 100644 --- 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 {