Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle apiGroup updates in resource-tracking #11012

Merged
merged 7 commits into from
Nov 3, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 38 additions & 17 deletions controller/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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
}

Expand All @@ -714,17 +715,37 @@ 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)
// Ideally the Tracking Id should be built from the config object.
// This is necessary because in case of an ApiGroup upgrade in the desired
// state, the comparison must be made with the new tracking id.
leoluz marked this conversation as resolved.
Show resolved Hide resolved
// 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.
leoluz marked this conversation as resolved.
Show resolved Hide resolved
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
}

func isSelfReferencedObj(obj *unstructured.Unstructured, aiv argo.AppInstanceValue) bool {
leoluz marked this conversation as resolved.
Show resolved Hide resolved
return (obj.GetNamespace() == aiv.Namespace || obj.GetNamespace() == "") &&
obj.GetName() == aiv.Name &&
obj.GetObjectKind().GroupVersionKind().Group == aiv.Group &&
obj.GetObjectKind().GroupVersionKind().Kind == aiv.Kind
}
91 changes: 72 additions & 19 deletions controller/state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand All @@ -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))
})
}
2 changes: 1 addition & 1 deletion controller/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion util/argo/diff/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
41 changes: 21 additions & 20 deletions util/argo/resource_tracking.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -107,21 +102,27 @@ func (rt *resourceTracking) GetAppInstance(un *unstructured.Unstructured, key st
}
}

// UnstructuredToAppInstanceValue will build the AppInstanceValue based
// on the provided unstructured
leoluz marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand Down