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.

Sign up for GitHub

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: trigger refresh on changed ignoreDifferences #12607

Merged
merged 6 commits into from
Jul 7, 2023
Merged

fix: trigger refresh on changed ignoreDifferences #12607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f04099
fix: trigger refresh on changed ignoreDifferences
crenshaw-dev Feb 24, 2023
62587b7
Merge remote-tracking branch 'origin/master' into refresh-on-changing…
crenshaw-dev Mar 8, 2023
3077672
make the tests mean things
crenshaw-dev Mar 8, 2023
a37da09
Merge remote-tracking branch 'origin/master' into refresh-on-changing…
crenshaw-dev Mar 14, 2023
db897d3
Merge remote-tracking branch 'origin/master' into refresh-on-changing…
crenshaw-dev May 30, 2023
9f6aa95
Merge remote-tracking branch 'origin/master' into refresh-on-changing…
crenshaw-dev Jul 7, 2023
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
7 changes: 7 additions & 0 deletions assets/swagger.json
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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