Skip to content

Commit

Permalink
fix: trigger refresh on changed ignoreDifferences (argoproj#12607)
Browse files Browse the repository at this point in the history 
* fix: trigger refresh on changed ignoreDifferences

Signed-off-by: Michael Crenshaw <[email protected]>

* make the tests mean things

Signed-off-by: Michael Crenshaw <[email protected]>

---------

Signed-off-by: Michael Crenshaw <[email protected]>
  • Loading branch information
@crenshaw-dev
crenshaw-dev authored and xiaowu.zhu committed Aug 9, 2023
1 parent 95d23ca commit 96d0ac5
Show file tree
Hide file tree
Showing 13 changed files with 1,005 additions and 687 deletions.
7 changes: 7 additions & 0 deletions assets/swagger.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6767,6 +6767,13 @@
"destination": {
"$ref": "#/definitions/v1alpha1ApplicationDestination"
},
"ignoreDifferences": {
"type": "array",
"title": "IgnoreDifferences is a reference to the application's ignored differences used for comparison",
"items": {
"$ref": "#/definitions/v1alpha1ResourceIgnoreDifferences"
}
},
"source": {
"$ref": "#/definitions/v1alpha1ApplicationSource"
},
Expand Down
2 changes: 2 additions & 0 deletions controller/appcontroller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1533,6 +1533,8 @@ func (ctrl *ApplicationController) needRefreshAppStatus(app *appv1.Application,
reason = "spec.destination differs"
} else if app.HasChangedManagedNamespaceMetadata() {
reason = "spec.syncPolicy.managedNamespaceMetadata differs"
} else if !app.Spec.IgnoreDifferences.Equals(app.Status.Sync.ComparedTo.IgnoreDifferences) {
reason = "spec.ignoreDifferences differs"
} else if requested, level := ctrl.isRefreshRequested(app.QualifiedName()); requested {
compareWith = level
reason = "controller refresh requested"
Expand Down
97 changes: 74 additions & 23 deletions controller/appcontroller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,6 @@ func TestNeedRefreshAppStatus(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})
app := tc.app
now := metav1.Now()
app.Status.ReconciledAt = &now
Expand All @@ -961,36 +960,58 @@ func TestNeedRefreshAppStatus(t *testing.T) {
app.Status.Sync.ComparedTo.Source = app.Spec.GetSource()
}

// no need to refresh just reconciled application
needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)
ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})

t.Run("no need to refresh just reconciled application", func(t *testing.T) {
needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)
})

t.Run("requested refresh is respected", func(t *testing.T) {
needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)

// refresh app using the 'deepest' requested comparison level
ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
ctrl.requestAppRefresh(app.Name, ComparisonWithNothing.Pointer(), nil)
// use a one-off controller so other tests don't have a manual refresh request
ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})

needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeNormal, refreshType)
assert.Equal(t, CompareWithRecent, compareWith)
// refresh app using the 'deepest' requested comparison level
ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
ctrl.requestAppRefresh(app.Name, ComparisonWithNothing.Pointer(), nil)

needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeNormal, refreshType)
assert.Equal(t, CompareWithRecent, compareWith)
})

// refresh application which status is not reconciled using latest commit
app.Status.Sync = v1alpha1.SyncStatus{Status: v1alpha1.SyncStatusCodeUnknown}
t.Run("refresh application which status is not reconciled using latest commit", func(t *testing.T) {
app := app.DeepCopy()
needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)
app.Status.Sync = v1alpha1.SyncStatus{Status: v1alpha1.SyncStatusCodeUnknown}

needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeNormal, refreshType)
assert.Equal(t, CompareWithLatestForceResolve, compareWith)
needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeNormal, refreshType)
assert.Equal(t, CompareWithLatestForceResolve, compareWith)
})

t.Run("refresh app using the 'latest' level if comparison expired", func(t *testing.T) {
app := app.DeepCopy()

// use a one-off controller so other tests don't have a manual refresh request
ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})

needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)

ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
app.Status.ReconciledAt = &reconciledAt
needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Minute, 2*time.Hour)
needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Minute, 2*time.Hour)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeNormal, refreshType)
assert.Equal(t, CompareWithLatestForceResolve, compareWith)
assert.Equal(t, CompareWithLatest, compareWith)
})

t.Run("refresh app using the 'latest' level if comparison expired for hard refresh", func(t *testing.T) {
Expand All @@ -1006,31 +1027,40 @@ func TestNeedRefreshAppStatus(t *testing.T) {
} else {
app.Status.Sync.ComparedTo.Source = app.Spec.GetSource()
}

// use a one-off controller so other tests don't have a manual refresh request
ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})

needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)
ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
app.Status.ReconciledAt = &reconciledAt
needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 2*time.Hour, 1*time.Minute)
needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 2*time.Hour, 1*time.Minute)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeHard, refreshType)
assert.Equal(t, CompareWithLatest, compareWith)
})

t.Run("execute hard refresh if app has refresh annotation", func(t *testing.T) {
app := app.DeepCopy()
needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)
reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
app.Status.ReconciledAt = &reconciledAt
app.Annotations = map[string]string{
v1alpha1.AnnotationKeyRefresh: string(v1alpha1.RefreshTypeHard),
}
needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeHard, refreshType)
assert.Equal(t, CompareWithLatestForceResolve, compareWith)
})

t.Run("ensure that CompareWithLatest level is used if application source has changed", func(t *testing.T) {
app := app.DeepCopy()
ctrl.requestAppRefresh(app.Name, ComparisonWithNothing.Pointer(), nil)
needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)
// sample app source change
if app.Spec.HasMultipleSources() {
app.Spec.Sources[0].Helm = &v1alpha1.ApplicationSourceHelm{
Expand All @@ -1048,11 +1078,32 @@ func TestNeedRefreshAppStatus(t *testing.T) {
}
}

needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeNormal, refreshType)
assert.Equal(t, CompareWithLatestForceResolve, compareWith)
})

t.Run("ensure that CompareWithLatest level is used if ignored differences change", func(t *testing.T) {
app := app.DeepCopy()
needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.False(t, needRefresh)

app.Spec.IgnoreDifferences = []v1alpha1.ResourceIgnoreDifferences{
{
Group: "apps",
Kind: "Deployment",
JSONPointers: []string{
"/spec/template/spec/containers/0/image",
},
},
}

needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
assert.True(t, needRefresh)
assert.Equal(t, v1alpha1.RefreshTypeNormal, refreshType)
assert.Equal(t, CompareWithLatest, compareWith)
})
})
}
}
Expand Down
36 changes: 36 additions & 0 deletions manifests/core-install.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3807,6 +3807,42 @@ spec:
and must be set to the Kubernetes control plane API
type: string
type: object
ignoreDifferences:
description: IgnoreDifferences is a reference to the application's
ignored differences used for comparison
items:
description: ResourceIgnoreDifferences contains resource
filter and list of json paths which should be ignored
during comparison with live state.
properties:
group:
type: string
jqPathExpressions:
items:
type: string
type: array
jsonPointers:
items:
type: string
type: array
kind:
type: string
managedFieldsManagers:
description: ManagedFieldsManagers is a list of trusted
managers. Fields mutated by those managers will take
precedence over the desired state defined in the SCM
and won't be displayed in diffs
items:
type: string
type: array
name:
type: string
namespace:
type: string
required:
- kind
type: object
type: array
source:
description: Source is a reference to the application's source
used for comparison
Expand Down
36 changes: 36 additions & 0 deletions manifests/crds/application-crd.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3806,6 +3806,42 @@ spec:
and must be set to the Kubernetes control plane API
type: string
type: object
ignoreDifferences:
description: IgnoreDifferences is a reference to the application's
ignored differences used for comparison
items:
description: ResourceIgnoreDifferences contains resource
filter and list of json paths which should be ignored
during comparison with live state.
properties:
group:
type: string
jqPathExpressions:
items:
type: string
type: array
jsonPointers:
items:
type: string
type: array
kind:
type: string
managedFieldsManagers:
description: ManagedFieldsManagers is a list of trusted
managers. Fields mutated by those managers will take
precedence over the desired state defined in the SCM
and won't be displayed in diffs
items:
type: string
type: array
name:
type: string
namespace:
type: string
required:
- kind
type: object
type: array
source:
description: Source is a reference to the application's source
used for comparison
Expand Down
36 changes: 36 additions & 0 deletions manifests/ha/install.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3807,6 +3807,42 @@ spec:
and must be set to the Kubernetes control plane API
type: string
type: object
ignoreDifferences:
description: IgnoreDifferences is a reference to the application's
ignored differences used for comparison
items:
description: ResourceIgnoreDifferences contains resource
filter and list of json paths which should be ignored
during comparison with live state.
properties:
group:
type: string
jqPathExpressions:
items:
type: string
type: array
jsonPointers:
items:
type: string
type: array
kind:
type: string
managedFieldsManagers:
description: ManagedFieldsManagers is a list of trusted
managers. Fields mutated by those managers will take
precedence over the desired state defined in the SCM
and won't be displayed in diffs
items:
type: string
type: array
name:
type: string
namespace:
type: string
required:
- kind
type: object
type: array
source:
description: Source is a reference to the application's source
used for comparison
Expand Down
36 changes: 36 additions & 0 deletions manifests/install.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3807,6 +3807,42 @@ spec:
and must be set to the Kubernetes control plane API
type: string
type: object
ignoreDifferences:
description: IgnoreDifferences is a reference to the application's
ignored differences used for comparison
items:
description: ResourceIgnoreDifferences contains resource
filter and list of json paths which should be ignored
during comparison with live state.
properties:
group:
type: string
jqPathExpressions:
items:
type: string
type: array
jsonPointers:
items:
type: string
type: array
kind:
type: string
managedFieldsManagers:
description: ManagedFieldsManagers is a list of trusted
managers. Fields mutated by those managers will take
precedence over the desired state defined in the SCM
and won't be displayed in diffs
items:
type: string
type: array
name:
type: string
namespace:
type: string
required:
- kind
type: object
type: array
source:
description: Source is a reference to the application's source
used for comparison
Expand Down
1 change: 0 additions & 1 deletion pkg/apis/api-rules/violation_exceptions.list
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ API rule violation: list_type_missing,github.com/argoproj/argo-cd/v2/pkg/apis/ap
API rule violation: list_type_missing,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ApplicationSourceJsonnet,ExtVars
API rule violation: list_type_missing,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ApplicationSourceJsonnet,Libs
API rule violation: list_type_missing,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ApplicationSourceJsonnet,TLAs
API rule violation: list_type_missing,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ApplicationSpec,IgnoreDifferences
API rule violation: list_type_missing,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ApplicationSpec,Info
API rule violation: list_type_missing,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ApplicationStatus,Conditions
API rule violation: list_type_missing,github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1,ApplicationStatus,Resources
Expand Down
Loading

0 comments on commit 96d0ac5

Please sign in to comment.