Skip to content

Commit 9fe498d

Browse files
jcogilviejcogilvie
committed
Lint and fix tests
Signed-off-by: Jonathan Ogilvie <[email protected]>
1 parent c911906 commit 9fe498d

File tree

7 files changed

+80
-73
lines changed

7 files changed

+80
-73
lines changed

‎controller/appcontroller.go

Lines changed: 44 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ import (
6767
appstatecache "github.com/argoproj/argo-cd/v3/util/cache/appstate"
6868
"github.com/argoproj/argo-cd/v3/util/db"
6969
"github.com/argoproj/argo-cd/v3/util/errors"
70-
errorutils "github.com/argoproj/argo-cd/v3/util/errors"
7170
"github.com/argoproj/argo-cd/v3/util/glob"
7271
"github.com/argoproj/argo-cd/v3/util/helm"
7372
logutils "github.com/argoproj/argo-cd/v3/util/log"
@@ -627,7 +626,7 @@ func (ctrl *ApplicationController) getResourceTree(destCluster *appv1.Cluster, a
627626
return true
628627
})
629628
if err != nil {
630-
if !errorutils.IsConversionWebhookError(err) {
629+
if !errors.IsConversionWebhookError(err) {
631630
return nil, fmt.Errorf("failed to iterate resource hierarchy v2: %w", err)
632631
}
633632

@@ -667,7 +666,7 @@ func (ctrl *ApplicationController) getResourceTree(destCluster *appv1.Cluster, a
667666
return true
668667
})
669668
if err != nil {
670-
if !errorutils.IsConversionWebhookError(err) {
669+
if !errors.IsConversionWebhookError(err) {
671670
return nil, fmt.Errorf("failed to iterate resource hierarchy v2 for orphaned resources: %w", err)
672671
}
673672

@@ -1827,46 +1826,45 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
18271826
// Check for cluster cache issues that affect this application
18281827
hasCacheIssues := false
18291828
usesTaintedResources := false
1830-
1829+
18311830
// Take a snapshot of cluster taint state to ensure consistency throughout the function
18321831
clusterURL := app.Spec.Destination.Server
18331832
failedGVKs := ctrl.stateCache.GetTaintedGVKs(clusterURL)
1834-
1833+
18351834
// 1. First check if the app itself reports conversion webhook errors
18361835
hasCacheIssues = hasClusterCacheIssues(app)
1837-
1836+
18381837
// 2. Check app conditions for cache-related errors during refresh
18391838
for _, condition := range app.Status.Conditions {
1840-
if condition.Type == appv1.ApplicationConditionComparisonError &&
1841-
(strings.Contains(condition.Message, "conversion webhook") ||
1842-
strings.Contains(condition.Message, "known conversion webhook failures") ||
1843-
strings.Contains(condition.Message, "unavailable resource types") ||
1844-
strings.Contains(condition.Message, "failed to list resources") ||
1845-
strings.Contains(condition.Message, "Expired: too old resource version")) {
1839+
if condition.Type == appv1.ApplicationConditionComparisonError &&
1840+
(strings.Contains(condition.Message, "conversion webhook") ||
1841+
strings.Contains(condition.Message, "known conversion webhook failures") ||
1842+
strings.Contains(condition.Message, "unavailable resource types") ||
1843+
strings.Contains(condition.Message, "failed to list resources") ||
1844+
strings.Contains(condition.Message, "Expired: too old resource version")) {
18461845
hasCacheIssues = true
18471846
break
18481847
}
18491848
}
1850-
1849+
18511850
// 3. Check resources for errors
18521851
for _, res := range app.Status.Resources {
1853-
if res.Health != nil && res.Health.Message != "" &&
1854-
(strings.Contains(res.Health.Message, "conversion webhook") ||
1855-
strings.Contains(res.Health.Message, "unavailable resource types") ||
1856-
strings.Contains(res.Health.Message, "failed to get resource")) {
1852+
if res.Health != nil && res.Health.Message != "" &&
1853+
(strings.Contains(res.Health.Message, "conversion webhook") ||
1854+
strings.Contains(res.Health.Message, "unavailable resource types") ||
1855+
strings.Contains(res.Health.Message, "failed to get resource")) {
18571856
hasCacheIssues = true
18581857
break
18591858
}
18601859
}
1861-
1860+
18621861
// 4. Check if the app directly uses any tainted resources (using consistent snapshot)
18631862
if len(failedGVKs) > 0 {
1864-
1865-
resLoop:
1863+
resLoop:
18661864
for _, res := range app.Status.Resources {
18671865
gvkStr := fmt.Sprintf("%s/%s, Kind=%s", res.Group, res.Version, res.Kind)
18681866
gvkWildcard := fmt.Sprintf("%s/*, Kind=%s", res.Group, res.Kind)
1869-
1867+
18701868
for _, failedGVK := range failedGVKs {
18711869
if failedGVK == gvkStr || failedGVK == gvkWildcard {
18721870
usesTaintedResources = true
@@ -1875,28 +1873,28 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
18751873
}
18761874
}
18771875
}
1878-
1879-
// If the application has conversion webhook errors, set health status to degraded
1876+
18801877
// Set health status based on the severity of detected issues
1881-
if usesTaintedResources {
1878+
switch {
1879+
case usesTaintedResources:
18821880
// Application directly uses tainted resources - mark as degraded
18831881
app.Status.Health.Status = health.HealthStatusDegraded
18841882
// Add a condition explaining why it's degraded
18851883
now := metav1.Now()
18861884
app.Status.SetConditions(
18871885
[]appv1.ApplicationCondition{
18881886
{
1889-
Type: appv1.ApplicationConditionComparisonError,
1890-
Message: "Application directly uses resources with known issues in the cluster cache",
1887+
Type: appv1.ApplicationConditionComparisonError,
1888+
Message: "Application directly uses resources with known issues in the cluster cache",
18911889
LastTransitionTime: &now,
18921890
},
18931891
},
18941892
map[appv1.ApplicationConditionType]bool{appv1.ApplicationConditionComparisonError: true},
18951893
)
1896-
} else if hasCacheIssues {
1894+
case hasCacheIssues:
18971895
// Application has cache issues but doesn't directly use tainted resources
18981896
app.Status.Health.Status = health.HealthStatusDegraded
1899-
} else {
1897+
default:
19001898
app.Status.Health.Status = compareResult.healthStatus
19011899
}
19021900
app.Status.Resources = compareResult.resources
@@ -2064,21 +2062,21 @@ func (ctrl *ApplicationController) needRefreshAppStatus(app *appv1.Application,
20642062
func hasClusterCacheIssues(app *appv1.Application) bool {
20652063
// Check operation state for conversion webhook errors
20662064
if app.Status.OperationState != nil {
2067-
if strings.Contains(app.Status.OperationState.Message, "conversion webhook") ||
2068-
strings.Contains(app.Status.OperationState.Message, "known conversion webhook failures") ||
2069-
strings.Contains(app.Status.OperationState.Message, "unavailable resource types") ||
2070-
strings.Contains(app.Status.OperationState.Message, "failed to list resources") ||
2071-
strings.Contains(app.Status.OperationState.Message, "Expired: too old resource version") {
2065+
if strings.Contains(app.Status.OperationState.Message, "conversion webhook") ||
2066+
strings.Contains(app.Status.OperationState.Message, "known conversion webhook failures") ||
2067+
strings.Contains(app.Status.OperationState.Message, "unavailable resource types") ||
2068+
strings.Contains(app.Status.OperationState.Message, "failed to list resources") ||
2069+
strings.Contains(app.Status.OperationState.Message, "Expired: too old resource version") {
20722070
return true
20732071
}
20742072
// Also check sync result resources
20752073
if app.Status.OperationState.SyncResult != nil {
20762074
for _, res := range app.Status.OperationState.SyncResult.Resources {
2077-
if strings.Contains(res.Message, "conversion webhook") ||
2078-
strings.Contains(res.Message, "known conversion webhook failures") ||
2079-
strings.Contains(res.Message, "unavailable resource types") ||
2080-
strings.Contains(res.Message, "failed to list resources") ||
2081-
strings.Contains(res.Message, "Expired: too old resource version") {
2075+
if strings.Contains(res.Message, "conversion webhook") ||
2076+
strings.Contains(res.Message, "known conversion webhook failures") ||
2077+
strings.Contains(res.Message, "unavailable resource types") ||
2078+
strings.Contains(res.Message, "failed to list resources") ||
2079+
strings.Contains(res.Message, "Expired: too old resource version") {
20822080
return true
20832081
}
20842082
}
@@ -2087,17 +2085,17 @@ func hasClusterCacheIssues(app *appv1.Application) bool {
20872085

20882086
// Check application conditions
20892087
for _, condition := range app.Status.Conditions {
2090-
if (condition.Type == appv1.ApplicationConditionComparisonError ||
2091-
condition.Type == appv1.ApplicationConditionSyncError) &&
2092-
(strings.Contains(condition.Message, "conversion webhook") ||
2093-
strings.Contains(condition.Message, "known conversion webhook failures") ||
2094-
strings.Contains(condition.Message, "unavailable resource types") ||
2095-
strings.Contains(condition.Message, "failed to list resources") ||
2096-
strings.Contains(condition.Message, "Expired: too old resource version")) {
2088+
if (condition.Type == appv1.ApplicationConditionComparisonError ||
2089+
condition.Type == appv1.ApplicationConditionSyncError) &&
2090+
(strings.Contains(condition.Message, "conversion webhook") ||
2091+
strings.Contains(condition.Message, "known conversion webhook failures") ||
2092+
strings.Contains(condition.Message, "unavailable resource types") ||
2093+
strings.Contains(condition.Message, "failed to list resources") ||
2094+
strings.Contains(condition.Message, "Expired: too old resource version")) {
20972095
return true
20982096
}
20992097
}
2100-
2098+
21012099
return false
21022100
}
21032101

‎controller/appcontroller_test.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,7 @@ func newFakeControllerWithResync(data *fakeData, appResyncPeriod time.Duration,
218218
mockStateCache.On("IsNamespaced", mock.Anything, mock.Anything).Return(true, nil)
219219
mockStateCache.On("GetManagedLiveObjs", mock.Anything, mock.Anything, mock.Anything).Return(data.managedLiveObjs, nil)
220220
mockStateCache.On("GetVersionsInfo", mock.Anything).Return("v1.2.3", nil, nil)
221+
mockStateCache.On("GetTaintedGVKs", mock.Anything).Return([]string{}, nil)
221222
response := make(map[kube.ResourceKey]v1alpha1.ResourceNode)
222223
for k, v := range data.namespacedResources {
223224
response[k] = v.ResourceNode
@@ -2977,7 +2978,7 @@ func TestConversionWebhookFailureIsolation(t *testing.T) {
29772978

29782979
// Create a conversion webhook error
29792980
conversionError := errors.New("failed to sync cluster: failed to load initial state of resource BucketServerSideEncryptionConfiguration.s3.aws.upbound.io: conversion webhook for s3.aws.upbound.io/v1beta1, Kind=BucketServerSideEncryptionConfiguration failed: Post \"https://provider-aws-s3.crossplane-system.svc:9443/convert?timeout=30s\": no endpoints available for service \"provider-aws-s3\"")
2980-
2981+
29812982
// Create a specific list operation conversion webhook error
29822983
listConversionError := errors.New("failed to list resources: conversion webhook for conversion.example.com/v1, Kind=Example failed: Post \"https://conversion-webhook-service.webhook-system.svc:443/convert?timeout=30s\": service \"conversion-webhook-service\" not found")
29832984

@@ -3016,6 +3017,9 @@ func TestConversionWebhookFailureIsolation(t *testing.T) {
30163017
customMockStateCache.On("GetNamespaceTopLevelResources", mock.Anything, mock.Anything).Return(nil, conversionError)
30173018
customMockStateCache.On("IterateHierarchyV2", mock.Anything, mock.Anything, mock.Anything).Return(conversionError)
30183019
customMockStateCache.On("IterateResources", mock.Anything, mock.Anything).Return(nil)
3020+
3021+
// Set up taint manager methods
3022+
customMockStateCache.On("GetTaintedGVKs", mock.Anything).Return([]string{}, nil)
30193023

30203024
// Set up cluster cache mock
30213025
clusterCacheMock := &mocks.ClusterCache{}
@@ -3051,7 +3055,7 @@ func TestConversionWebhookFailureIsolation(t *testing.T) {
30513055
// Verify that conversion webhook errors are properly handled and reported
30523056
hasConversionError := false
30533057
hasListConversionError := false
3054-
3058+
30553059
for _, app := range []*v1alpha1.Application{updatedApp1, updatedApp2} {
30563060
for _, condition := range app.Status.Conditions {
30573061
if condition.Type == v1alpha1.ApplicationConditionComparisonError {

‎controller/clusterinfoupdater.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,6 @@ func (c *clusterInfoUpdater) updateClusters() {
114114
log.Debugf("Successfully saved info of %d clusters", len(clustersFiltered))
115115
}
116116

117-
118117
func (c *clusterInfoUpdater) updateClusterInfo(ctx context.Context, cluster appv1.Cluster, info *cache.ClusterInfo) error {
119118
apps, err := c.appLister.List(labels.Everything())
120119
if err != nil {

‎controller/clusterinfoupdater_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func TestClusterSecretUpdater(t *testing.T) {
9999
require.NoError(t, err)
100100
assert.Equal(t, updatedK8sVersion, clusterInfo.ServerVersion)
101101
assert.Equal(t, test.ExpectedStatus, clusterInfo.ConnectionState.Status)
102-
102+
103103
// For degraded state, verify that the error message indicates conversion webhook errors
104104
if test.ExpectedStatus == v1alpha1.ConnectionStatusDegraded {
105105
assert.Contains(t, clusterInfo.ConnectionState.Message, "unavailable resource types")

‎controller/health.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,20 +26,20 @@ func setApplicationHealth(resources []managedResource, statuses []appv1.Resource
2626

2727
// Check if application has conversion webhook error conditions
2828
hasConversionWebhookIssues := false
29-
29+
3030
// Check sync operation state for conversion webhook errors
3131
if app.Status.OperationState != nil && app.Status.OperationState.Phase == synccommon.OperationFailed {
3232
if strings.Contains(app.Status.OperationState.Message, "conversion webhook") {
3333
hasConversionWebhookIssues = true
3434
}
3535
}
36-
36+
3737
// Also check application conditions
3838
for _, condition := range app.Status.Conditions {
39-
if (condition.Type == appv1.ApplicationConditionComparisonError ||
40-
condition.Type == appv1.ApplicationConditionSyncError) &&
41-
(strings.Contains(condition.Message, "conversion webhook") ||
42-
strings.Contains(condition.Message, "unavailable resource types")) {
39+
if (condition.Type == appv1.ApplicationConditionComparisonError ||
40+
condition.Type == appv1.ApplicationConditionSyncError) &&
41+
(strings.Contains(condition.Message, "conversion webhook") ||
42+
strings.Contains(condition.Message, "unavailable resource types")) {
4343
hasConversionWebhookIssues = true
4444
break
4545
}

‎controller/state.go

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,9 @@ import (
4141
"github.com/argoproj/argo-cd/v3/util/argo/normalizers"
4242
appstatecache "github.com/argoproj/argo-cd/v3/util/cache/appstate"
4343
"github.com/argoproj/argo-cd/v3/util/db"
44+
errorutils "github.com/argoproj/argo-cd/v3/util/errors"
4445
"github.com/argoproj/argo-cd/v3/util/gpg"
4546
utilio "github.com/argoproj/argo-cd/v3/util/io"
46-
errorutils "github.com/argoproj/argo-cd/v3/util/errors"
4747
"github.com/argoproj/argo-cd/v3/util/settings"
4848
"github.com/argoproj/argo-cd/v3/util/stats"
4949
)
@@ -687,23 +687,31 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
687687
if errorutils.IsConversionWebhookError(err) {
688688
logCtx.Warnf("Conversion webhook error while getting managed live objects, continuing with empty live state: %v", err)
689689
liveObjByKey = make(map[kubeutil.ResourceKey]*unstructured.Unstructured)
690-
690+
691691
// Extract more detailed information from the error message for better diagnostics
692692
errorMsg := err.Error()
693-
if strings.Contains(errorMsg, "cannot sync application because all target resources have known conversion webhook failures") {
693+
switch {
694+
case strings.Contains(errorMsg, "cannot sync application because all target resources have known conversion webhook failures"):
694695
// More detailed error message
695696
conditions = append(conditions, v1alpha1.ApplicationCondition{
696697
Type: v1alpha1.ApplicationConditionComparisonError,
697698
Message: fmt.Sprintf("This application cannot be synced because it contains resources with conversion webhook failures: %v. Check cluster status for more details about the affected resource types.", errorMsg),
698699
LastTransitionTime: &now,
699700
})
700-
} else if strings.Contains(errorMsg, "conversion webhook") || strings.Contains(errorMsg, "unavailable resource types") {
701+
case strings.Contains(errorMsg, "failed to list resources") && strings.Contains(errorMsg, "conversion webhook"):
702+
// Specific handling for list operation failures due to conversion webhook issues
701703
conditions = append(conditions, v1alpha1.ApplicationCondition{
702704
Type: v1alpha1.ApplicationConditionComparisonError,
703-
Message: fmt.Sprintf("Application contains resources affected by conversion webhook failures. Some resources may not be properly synchronized. Check cluster status for more details about the affected resource types."),
705+
Message: fmt.Sprintf("Failed to list resources due to conversion webhook error: %v. Check cluster status for more details about the affected resource types.", errorMsg),
704706
LastTransitionTime: &now,
705707
})
706-
} else {
708+
case strings.Contains(errorMsg, "conversion webhook") || strings.Contains(errorMsg, "unavailable resource types"):
709+
conditions = append(conditions, v1alpha1.ApplicationCondition{
710+
Type: v1alpha1.ApplicationConditionComparisonError,
711+
Message: "Application contains resources affected by conversion webhook failures. Some resources may not be properly synchronized. Check cluster status for more details about the affected resource types.",
712+
LastTransitionTime: &now,
713+
})
714+
default:
707715
conditions = append(conditions, v1alpha1.ApplicationCondition{
708716
Type: v1alpha1.ApplicationConditionComparisonError,
709717
Message: fmt.Sprintf("Error retrieving live state: %v", err),
@@ -793,12 +801,12 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
793801
}
794802

795803
reconciliation := sync.Reconcile(targetObjs, liveObjByKey, app.Spec.Destination.Namespace, infoProvider)
796-
804+
797805
// Check if the app has conversion webhook error conditions
798806
hasConversionWebhookIssues := false
799807
for _, condition := range app.Status.Conditions {
800808
if condition.Type == v1alpha1.ApplicationConditionComparisonError &&
801-
(strings.Contains(condition.Message, "conversion webhook") || strings.Contains(condition.Message, "unavailable resource types")) {
809+
(strings.Contains(condition.Message, "conversion webhook") || strings.Contains(condition.Message, "unavailable resource types")) {
802810
hasConversionWebhookIssues = true
803811
break
804812
}
@@ -980,17 +988,18 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
980988
resourceSummaries[i] = resState
981989
}
982990

983-
if failedToLoadObjs {
991+
switch {
992+
case failedToLoadObjs:
984993
syncCode = v1alpha1.SyncStatusCodeUnknown
985-
} else if hasConversionWebhookIssues && len(targetObjs) == 0 {
994+
case hasConversionWebhookIssues && len(targetObjs) == 0:
986995
// If we have conversion webhook issues and no target objects successfully loaded,
987996
// the sync status should be Unknown since we can't determine real state
988997
syncCode = v1alpha1.SyncStatusCodeUnknown
989-
} else if hasConversionWebhookIssues {
998+
case hasConversionWebhookIssues:
990999
// If we have conversion webhook issues but some targets loaded, mark as OutOfSync
9911000
// This ensures the user knows something needs attention
9921001
syncCode = v1alpha1.SyncStatusCodeOutOfSync
993-
} else if app.HasChangedManagedNamespaceMetadata() {
1002+
case app.HasChangedManagedNamespaceMetadata():
9941003
syncCode = v1alpha1.SyncStatusCodeOutOfSync
9951004
}
9961005

@@ -1248,4 +1257,3 @@ func isSelfReferencedObj(obj *unstructured.Unstructured, aiv argo.AppInstanceVal
12481257
obj.GetObjectKind().GroupVersionKind().Group == aiv.Group &&
12491258
obj.GetObjectKind().GroupVersionKind().Kind == aiv.Kind
12501259
}
1251-

‎controller/sync.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,6 @@ func newSyncOperationResult(app *v1alpha1.Application, op v1alpha1.SyncOperation
118118
return syncRes
119119
}
120120

121-
122-
123121
func (m *appStateManager) SyncAppState(app *v1alpha1.Application, project *v1alpha1.AppProject, state *v1alpha1.OperationState) {
124122
syncId, err := syncid.Generate()
125123
if err != nil {
@@ -164,10 +162,10 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, project *v1alp
164162
compareResult, err := m.CompareAppState(app, project, revisions, sources, false, true, syncOp.Manifests, isMultiSourceSync)
165163
if err != nil && !stderrors.Is(err, ErrCompareStateRepo) {
166164
state.Phase = common.OperationError
167-
165+
168166
// Check if this is a conversion webhook error and provide a more helpful message
169167
if errorutils.IsConversionWebhookError(err) {
170-
state.Message = fmt.Sprintf("Application contains resources with conversion webhook failures. Some resources cannot be properly synchronized. Check cluster status for more details about the affected resource types.")
168+
state.Message = "Application contains resources with conversion webhook failures. Some resources cannot be properly synchronized. Check cluster status for more details about the affected resource types."
171169
} else {
172170
state.Message = err.Error()
173171
}

0 commit comments

Comments
 (0)