Skip to content

Commit

Permalink
fix: Check tracking annotation for being self-referencing (#9791)
Browse files Browse the repository at this point in the history 
* fix: Check tracking annotation for being self-referencing

Signed-off-by: jannfis <jann@mistrust.net>

* Tweak isManagedLiveObj() logic

Signed-off-by: jannfis <jann@mistrust.net>

* Rename isManagedLiveResource to isSelfReferencedObj

Signed-off-by: jannfis <jann@mistrust.net>

* Add e2e test

Signed-off-by: jannfis <jann@mistrust.net>
  • Loading branch information
@jannfis
jannfis authored Jul 6, 2022
1 parent 2a3c692 commit bcfdef8
Show file tree
Hide file tree
Showing 4 changed files with 243 additions and 8 deletions.
41 changes: 38 additions & 3 deletions controller/state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,14 +494,16 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
}
gvk := obj.GroupVersionKind()

isSelfReferencedObj := m.isSelfReferencedObj(liveObj, appLabelKey, trackingMethod)

resState := v1alpha1.ResourceStatus{
Namespace: obj.GetNamespace(),
Name: obj.GetName(),
Kind: gvk.Kind,
Version: gvk.Version,
Group: gvk.Group,
Hook: hookutil.IsHook(obj),
RequiresPruning: targetObj == nil && liveObj != nil,
RequiresPruning: targetObj == nil && liveObj != nil && isSelfReferencedObj,
}

var diffResult diff.DiffResult
Expand All @@ -510,8 +512,11 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
} else {
diffResult = diff.DiffResult{Modified: false, NormalizedLive: []byte("{}"), PredictedLive: []byte("{}")}
}
if resState.Hook || ignore.Ignore(obj) || (targetObj != nil && hookutil.Skip(targetObj)) {
// For resource hooks or skipped resources, don't store sync status, and do not affect overall sync status
if resState.Hook || ignore.Ignore(obj) || (targetObj != nil && hookutil.Skip(targetObj)) || !isSelfReferencedObj {
// For resource hooks, skipped resources or objects that may have
// been created by another controller with annotations copied from
// the source object, don't store sync status, and do not affect
// overall sync status
} else if diffResult.Modified || targetObj == nil || liveObj == nil {
// Set resource state to OutOfSync since one of the following is true:
// * target and live resource are different
Expand Down Expand Up @@ -667,3 +672,33 @@ func NewAppStateManager(
resourceTracking: resourceTracking,
}
}

// 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 {
return true
}

// If tracking method doesn't contain required metadata for this check,
// we are not able to determine and just assume the object to be managed.
if trackingMethod == argo.TrackingMethodLabel {
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.
appInstance := m.resourceTracking.GetAppInstance(obj, appLabelKey, trackingMethod)
if appInstance != nil {
return obj.GetNamespace() == appInstance.Namespace &&
obj.GetName() == appInstance.Name &&
obj.GetObjectKind().GroupVersionKind().Group == appInstance.Group &&
obj.GetObjectKind().GroupVersionKind().Kind == appInstance.Kind
}

return true
}
127 changes: 127 additions & 0 deletions controller/state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
. "github.com/argoproj/gitops-engine/pkg/utils/testing"
"github.com/stretchr/testify/assert"
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"
Expand All @@ -21,6 +22,7 @@ import (
argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/argoproj/argo-cd/v2/reposerver/apiclient"
"github.com/argoproj/argo-cd/v2/test"
"github.com/argoproj/argo-cd/v2/util/argo"
)

// TestCompareAppStateEmpty tests comparison when both git and live have no objects
Expand Down Expand Up @@ -770,3 +772,128 @@ func TestComparisonResult_GetSyncStatus(t *testing.T) {

assert.Equal(t, status, res.GetSyncStatus())
}

func TestIsLiveResourceManaged(t *testing.T) {
managedObj := kube.MustToUnstructured(&corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "ConfigMap",
},
ObjectMeta: metav1.ObjectMeta{
Name: "configmap1",
Namespace: "default",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: "guestbook:/ConfigMap:default/configmap1",
},
},
})
managedObjWithLabel := kube.MustToUnstructured(&corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "ConfigMap",
},
ObjectMeta: metav1.ObjectMeta{
Name: "configmap1",
Namespace: "default",
Labels: map[string]string{
common.LabelKeyAppInstance: "guestbook",
},
},
})
unmanagedObjWrongName := kube.MustToUnstructured(&corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "ConfigMap",
},
ObjectMeta: metav1.ObjectMeta{
Name: "configmap2",
Namespace: "default",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: "guestbook:/ConfigMap:default/configmap1",
},
},
})
unmanagedObjWrongKind := kube.MustToUnstructured(&corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "ConfigMap",
},
ObjectMeta: metav1.ObjectMeta{
Name: "configmap2",
Namespace: "default",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: "guestbook:/Service:default/configmap2",
},
},
})
unmanagedObjWrongGroup := kube.MustToUnstructured(&corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "ConfigMap",
},
ObjectMeta: metav1.ObjectMeta{
Name: "configmap2",
Namespace: "default",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: "guestbook:apps/ConfigMap:default/configmap2",
},
},
})
unmanagedObjWrongNamespace := kube.MustToUnstructured(&corev1.ConfigMap{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1",
Kind: "ConfigMap",
},
ObjectMeta: metav1.ObjectMeta{
Name: "configmap2",
Namespace: "default",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: "guestbook:/ConfigMap:fakens/configmap2",
},
},
})
ctrl := newFakeController(&fakeData{
apps: []runtime.Object{app, &defaultProj},
manifestResponse: &apiclient.ManifestResponse{
Manifests: []string{},
Namespace: test.FakeDestNamespace,
Server: test.FakeClusterURL,
Revision: "abc123",
},
managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{
kube.GetResourceKey(managedObj): managedObj,
kube.GetResourceKey(unmanagedObjWrongName): unmanagedObjWrongName,
kube.GetResourceKey(unmanagedObjWrongKind): unmanagedObjWrongKind,
kube.GetResourceKey(unmanagedObjWrongGroup): unmanagedObjWrongGroup,
kube.GetResourceKey(unmanagedObjWrongNamespace): unmanagedObjWrongNamespace,
},
})

manager := ctrl.appStateManager.(*appStateManager)

// Managed resource w/ annotations
assert.True(t, manager.isSelfReferencedObj(managedObj, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
assert.True(t, manager.isSelfReferencedObj(managedObj, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))

// Managed resource w/ label
assert.True(t, manager.isSelfReferencedObj(managedObjWithLabel, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))

// Wrong resource name
assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongName, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongName, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))

// Wrong resource group
assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongGroup, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongGroup, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))

// Wrong resource kind
assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongKind, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongKind, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))

// Wrong resource namespace
assert.True(t, manager.isSelfReferencedObj(unmanagedObjWrongNamespace, common.AnnotationKeyAppInstance, argo.TrackingMethodLabel))
assert.False(t, manager.isSelfReferencedObj(unmanagedObjWrongNamespace, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotationAndLabel))

// Nil resource
assert.True(t, manager.isSelfReferencedObj(nil, common.AnnotationKeyAppInstance, argo.TrackingMethodAnnotation))
}
52 changes: 52 additions & 0 deletions test/e2e/app_management_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2250,3 +2250,55 @@ func TestSwitchTrackingLabel(t *testing.T) {
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy))
}

func TestAnnotationTrackingExtraResources(t *testing.T) {
ctx := Given(t)

SetTrackingMethod(string(argo.TrackingMethodAnnotation))
ctx.
Path("deployment").
When().
CreateApp().
Sync().
Refresh(RefreshTypeNormal).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Add a resource with an annotation that is not referencing the
// resource.
FailOnErr(KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Create(context.Background(), &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "extra-configmap",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: fmt.Sprintf("%s:apps/Deployment:%s/guestbook-cm", Name(), DeploymentNamespace()),
},
},
}, metav1.CreateOptions{}))
}).
Refresh(RefreshTypeNormal).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Add a resource with an annotation that is self-referencing the
// resource.
FailOnErr(KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Create(context.Background(), &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "other-configmap",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: fmt.Sprintf("%s:/ConfigMap:%s/other-configmap", Name(), DeploymentNamespace()),
},
},
}, metav1.CreateOptions{}))
}).
Refresh(RefreshTypeNormal).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeOutOfSync)).
Expect(HealthIs(health.HealthStatusHealthy))
}
31 changes: 26 additions & 5 deletions util/argo/resource_tracking.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ var LabelMaxLength = 63
// ResourceTracking defines methods which allow setup and retrieve tracking information to resource
type ResourceTracking interface {
GetAppName(un *unstructured.Unstructured, key string, trackingMethod v1alpha1.TrackingMethod) string
GetAppInstance(un *unstructured.Unstructured, key string, trackingMethod v1alpha1.TrackingMethod) *AppInstanceValue
SetAppInstance(un *unstructured.Unstructured, key, val, namespace string, trackingMethod v1alpha1.TrackingMethod) error
BuildAppInstanceValue(value AppInstanceValue) string
ParseAppInstanceValue(value string) (*AppInstanceValue, error)
Expand Down Expand Up @@ -64,15 +65,23 @@ func IsOldTrackingMethod(trackingMethod string) bool {
return trackingMethod == "" || trackingMethod == string(TrackingMethodLabel)
}

func (rt *resourceTracking) getAppInstanceValue(un *unstructured.Unstructured, key string, trackingMethod v1alpha1.TrackingMethod) *AppInstanceValue {
appInstanceAnnotation := argokube.GetAppInstanceAnnotation(un, common.AnnotationKeyAppInstance)
value, err := rt.ParseAppInstanceValue(appInstanceAnnotation)
if err != nil {
return nil
}
return value
}

// GetAppName retrieve application name base on tracking method
func (rt *resourceTracking) GetAppName(un *unstructured.Unstructured, key string, trackingMethod v1alpha1.TrackingMethod) string {
retrieveAppInstanceValue := func() string {
appInstanceAnnotation := argokube.GetAppInstanceAnnotation(un, common.AnnotationKeyAppInstance)
value, err := rt.ParseAppInstanceValue(appInstanceAnnotation)
if err != nil {
return ""
value := rt.getAppInstanceValue(un, key, trackingMethod)
if value != nil {
return value.ApplicationName
}
return value.ApplicationName
return ""
}
switch trackingMethod {
case TrackingMethodLabel:
Expand All @@ -86,6 +95,18 @@ func (rt *resourceTracking) GetAppName(un *unstructured.Unstructured, key string
}
}

// GetAppInstance returns the representation of the app instance annotation.
// If the tracking method does not support metadata, or the annotation could
// not be parsed, it returns nil.
func (rt *resourceTracking) GetAppInstance(un *unstructured.Unstructured, key string, trackingMethod v1alpha1.TrackingMethod) *AppInstanceValue {
switch trackingMethod {
case TrackingMethodAnnotation, TrackingMethodAnnotationAndLabel:
return rt.getAppInstanceValue(un, key, trackingMethod)
default:
return nil
}
}

// 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 {
Expand Down

0 comments on commit bcfdef8

Please sign in to comment.