Skip to content

Commit 46105dc

Browse files
jgwestjgwest
authored
IsObjectFound function should not bury unexpected errors (argoproj-labs#1728)
Signed-off-by: Jonathan West <jonwest@redhat.com>
1 parent 50eb221 commit 46105dc

25 files changed

+604
-157
lines changed

‎controllers/argocd/applicationset.go‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -161,13 +161,15 @@ func (r *ReconcileArgoCD) reconcileApplicationSetController(cr *argoproj.ArgoCD)
161161
// reconcileApplicationControllerDeployment will ensure the Deployment resource is present for the ArgoCD Application Controller component.
162162
func (r *ReconcileArgoCD) reconcileApplicationSetDeployment(cr *argoproj.ArgoCD, sa *corev1.ServiceAccount) error {
163163

164-
exists := false
165164
existing := newDeploymentWithSuffix("applicationset-controller", "controller", cr)
166-
if argoutil.IsObjectFound(r.Client, cr.Namespace, existing.Name, existing) {
167-
exists = true
165+
166+
deplExists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, existing.Name, existing)
167+
if err != nil {
168+
return err
168169
}
170+
169171
if cr.Spec.ApplicationSet == nil || !cr.Spec.ApplicationSet.IsEnabled() {
170-
if exists {
172+
if deplExists {
171173
argoutil.LogResourceDeletion(log, existing, "application set not enabled")
172174
return r.Client.Delete(context.TODO(), existing)
173175
}
@@ -236,7 +238,13 @@ func (r *ReconcileArgoCD) reconcileApplicationSetDeployment(cr *argoproj.ArgoCD,
236238
addSCMGitlabVolumeMount := false
237239
if scmRootCAConfigMapName := getSCMRootCAConfigMapName(cr); scmRootCAConfigMapName != "" {
238240
cm := newConfigMapWithName(scmRootCAConfigMapName, cr)
239-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cr.Spec.ApplicationSet.SCMRootCAConfigMap, cm) {
241+
242+
cmExists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cr.Spec.ApplicationSet.SCMRootCAConfigMap, cm)
243+
if err != nil {
244+
return err
245+
}
246+
247+
if cmExists {
240248
addSCMGitlabVolumeMount = true
241249
podSpec.Volumes = append(podSpec.Volumes, corev1.Volume{
242250
Name: "appset-gitlab-scm-tls-cert",
@@ -268,7 +276,7 @@ func (r *ReconcileArgoCD) reconcileApplicationSetDeployment(cr *argoproj.ArgoCD,
268276
}
269277
AddSeccompProfileForOpenShift(r.Client, podSpec)
270278

271-
if exists {
279+
if deplExists {
272280
// Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata.
273281
addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels)
274282
addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations)
@@ -903,22 +911,27 @@ func (r *ReconcileArgoCD) reconcileApplicationSetService(cr *argoproj.ArgoCD) er
903911
log.Info("reconciling applicationset service")
904912

905913
svc := newServiceWithSuffix(common.ApplicationSetServiceNameSuffix, common.ApplicationSetServiceNameSuffix, cr)
914+
serviceExists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc)
915+
if err != nil {
916+
return err
917+
}
918+
906919
if cr.Spec.ApplicationSet == nil || !cr.Spec.ApplicationSet.IsEnabled() {
907920

908-
if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
921+
if serviceExists {
909922
err := argoutil.FetchObject(r.Client, cr.Namespace, svc.Name, svc)
910923
if err != nil {
911924
return err
912925
}
913926
argoutil.LogResourceDeletion(log, svc, "application set not enabled")
914-
err = r.Delete(context.TODO(), svc)
915-
if err != nil {
927+
if err := r.Delete(context.TODO(), svc); err != nil {
916928
return err
917929
}
918930
}
919931
return nil
932+
920933
} else {
921-
if argoutil.IsObjectFound(r.Client, cr.Namespace, svc.Name, svc) {
934+
if serviceExists {
922935
return nil // Service found, do nothing
923936
}
924937
}

‎controllers/argocd/applicationset_test.go‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -159,17 +159,22 @@ func checkExpectedDeploymentValues(t *testing.T, r *ReconcileArgoCD, deployment
159159
},
160160
}
161161

162-
if a.Spec.ApplicationSet.SCMRootCAConfigMap != "" && argoutil.IsObjectFound(r.Client, a.Namespace, common.ArgoCDAppSetGitlabSCMTLSCertsConfigMapName, a) {
163-
volumes = append(volumes, v1.Volume{
164-
Name: "appset-gitlab-scm-tls-cert",
165-
VolumeSource: v1.VolumeSource{
166-
ConfigMap: &v1.ConfigMapVolumeSource{
167-
LocalObjectReference: v1.LocalObjectReference{
168-
Name: common.ArgoCDAppSetGitlabSCMTLSCertsConfigMapName,
162+
if a.Spec.ApplicationSet.SCMRootCAConfigMap != "" {
163+
164+
exists, err := argoutil.IsObjectFound(r.Client, a.Namespace, common.ArgoCDAppSetGitlabSCMTLSCertsConfigMapName, a)
165+
assert.Nil(t, err)
166+
if exists {
167+
volumes = append(volumes, v1.Volume{
168+
Name: "appset-gitlab-scm-tls-cert",
169+
VolumeSource: v1.VolumeSource{
170+
ConfigMap: &v1.ConfigMapVolumeSource{
171+
LocalObjectReference: v1.LocalObjectReference{
172+
Name: common.ArgoCDAppSetGitlabSCMTLSCertsConfigMapName,
173+
},
169174
},
170175
},
171-
},
172-
})
176+
})
177+
}
173178
}
174179

175180
if extraVolumes != nil {

‎controllers/argocd/argocd_controller_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,9 @@ func TestReconcileArgoCD_CleanUp(t *testing.T) {
356356

357357
for _, test := range tt {
358358
t.Run(test.name, func(t *testing.T) {
359-
if argoutil.IsObjectFound(r.Client, "", test.name, test.resource) {
359+
found, err := argoutil.IsObjectFound(r.Client, "", test.name, test.resource)
360+
assert.Nil(t, err)
361+
if found {
360362
t.Errorf("Expected %s to be deleted", test.name)
361363
}
362364
})

‎controllers/argocd/configmap.go‎

Lines changed: 81 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -334,12 +334,21 @@ func (r *ReconcileArgoCD) reconcileConfigMaps(cr *argoproj.ArgoCD, useTLSForRedi
334334
// This ConfigMap holds the CA Certificate data for client use.
335335
func (r *ReconcileArgoCD) reconcileCAConfigMap(cr *argoproj.ArgoCD) error {
336336
cm := newConfigMapWithName(getCAConfigMapName(cr), cr)
337-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
337+
338+
configMapExists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm)
339+
if err != nil {
340+
return err
341+
}
342+
if configMapExists {
338343
return nil // ConfigMap found, do nothing
339344
}
340345

341346
caSecret := argoutil.NewSecretWithSuffix(cr, common.ArgoCDCASuffix)
342-
if !argoutil.IsObjectFound(r.Client, cr.Namespace, caSecret.Name, caSecret) {
347+
caSecretExists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, caSecret.Name, caSecret)
348+
if err != nil {
349+
return err
350+
}
351+
if !caSecretExists {
343352
log.Info(fmt.Sprintf("ca secret [%s] not found, waiting to reconcile ca configmap [%s]", caSecret.Name, cm.Name))
344353
return nil
345354
}
@@ -405,7 +414,12 @@ func (r *ReconcileArgoCD) reconcileArgoConfigMap(cr *argoproj.ArgoCD) error {
405414
cm.Data[common.ArgoCDKeyResourceInclusions] = getResourceInclusions(cr)
406415
cm.Data[common.ArgoCDKeyResourceTrackingMethod] = getResourceTrackingMethod(cr)
407416
cm.Data[common.ArgoCDKeyStatusBadgeEnabled] = fmt.Sprint(cr.Spec.StatusBadgeEnabled)
408-
cm.Data[common.ArgoCDKeyServerURL] = r.getArgoServerURI(cr)
417+
418+
serverURI, err := r.getArgoServerURI(cr)
419+
if err != nil {
420+
return err
421+
}
422+
cm.Data[common.ArgoCDKeyServerURL] = serverURI
409423
cm.Data[common.ArgoCDKeyUsersAnonymousEnabled] = fmt.Sprint(cr.Spec.UsersAnonymousEnabled)
410424

411425
// deprecated: log warning for deprecated field InitialRepositories
@@ -459,7 +473,11 @@ func (r *ReconcileArgoCD) reconcileArgoConfigMap(cr *argoproj.ArgoCD) error {
459473
}
460474

461475
existingCM := &corev1.ConfigMap{}
462-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
476+
found, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM)
477+
if err != nil {
478+
return err
479+
}
480+
if found {
463481

464482
// reconcile dex configuration if dex is enabled `.spec.sso.dex.provider` or there is
465483
// existing dex configuration
@@ -533,7 +551,12 @@ func (r *ReconcileArgoCD) reconcileGrafanaDashboards(cr *argoproj.ArgoCD) error
533551
// reconcileRBAC will ensure that the ArgoCD RBAC ConfigMap is present.
534552
func (r *ReconcileArgoCD) reconcileRBAC(cr *argoproj.ArgoCD) error {
535553
cm := newConfigMapWithName(common.ArgoCDRBACConfigMapName, cr)
536-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
554+
555+
found, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm)
556+
if err != nil {
557+
return err
558+
}
559+
if found {
537560
return r.reconcileRBACConfigMap(cm, cr)
538561
}
539562
return r.createRBACConfigMap(cm, cr)
@@ -659,7 +682,11 @@ func (r *ReconcileArgoCD) reconcileRedisHAHealthConfigMap(cr *argoproj.ArgoCD, u
659682
}
660683
if !cr.Spec.HA.Enabled {
661684
existingCM := &corev1.ConfigMap{}
662-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
685+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM)
686+
if err != nil {
687+
return err
688+
}
689+
if exists {
663690
// ConfigMap exists but HA enabled flag has been set to false, delete the ConfigMap
664691
argoutil.LogResourceDeletion(log, cm, "redis ha is disabled")
665692
return r.Client.Delete(context.TODO(), existingCM)
@@ -672,7 +699,11 @@ func (r *ReconcileArgoCD) reconcileRedisHAHealthConfigMap(cr *argoproj.ArgoCD, u
672699
}
673700

674701
existingCM := &corev1.ConfigMap{}
675-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
702+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM)
703+
if err != nil {
704+
return err
705+
}
706+
if exists {
676707
// Check if the data has changed
677708
if !reflect.DeepEqual(cm.Data, existingCM.Data) {
678709
existingCM.Data = cm.Data
@@ -700,7 +731,11 @@ func (r *ReconcileArgoCD) reconcileRedisHAConfigMap(cr *argoproj.ArgoCD, useTLSF
700731
if !cr.Spec.HA.Enabled {
701732

702733
existingCM := &corev1.ConfigMap{}
703-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
734+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM)
735+
if err != nil {
736+
return err
737+
}
738+
if exists {
704739
// ConfigMap exists but HA enabled flag has been set to false, delete the ConfigMap
705740
argoutil.LogResourceDeletion(log, cm, "redis ha is disabled")
706741
return r.Client.Delete(context.TODO(), existingCM)
@@ -714,7 +749,11 @@ func (r *ReconcileArgoCD) reconcileRedisHAConfigMap(cr *argoproj.ArgoCD, useTLSF
714749
}
715750

716751
existingCM := &corev1.ConfigMap{}
717-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
752+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM)
753+
if err != nil {
754+
return err
755+
}
756+
if exists {
718757
// Check if the data has changed
719758
if !reflect.DeepEqual(cm.Data, existingCM.Data) {
720759
existingCM.Data = cm.Data
@@ -730,7 +769,12 @@ func (r *ReconcileArgoCD) reconcileRedisHAConfigMap(cr *argoproj.ArgoCD, useTLSF
730769

731770
func (r *ReconcileArgoCD) recreateRedisHAConfigMap(cr *argoproj.ArgoCD, useTLSForRedis bool) error {
732771
cm := newConfigMapWithName(common.ArgoCDRedisHAConfigMapName, cr)
733-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
772+
773+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm)
774+
if err != nil {
775+
return err
776+
}
777+
if exists {
734778
argoutil.LogResourceDeletion(log, cm, "deleting config map in order to recreate it")
735779
if err := r.Client.Delete(context.TODO(), cm); err != nil {
736780
return err
@@ -741,7 +785,12 @@ func (r *ReconcileArgoCD) recreateRedisHAConfigMap(cr *argoproj.ArgoCD, useTLSFo
741785

742786
func (r *ReconcileArgoCD) recreateRedisHAHealthConfigMap(cr *argoproj.ArgoCD, useTLSForRedis bool) error {
743787
cm := newConfigMapWithName(common.ArgoCDRedisHAHealthConfigMapName, cr)
744-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
788+
789+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm)
790+
if err != nil {
791+
return err
792+
}
793+
if exists {
745794
argoutil.LogResourceDeletion(log, cm, "deleting config map in order to recreate it")
746795
if err := r.Client.Delete(context.TODO(), cm); err != nil {
747796
return err
@@ -753,7 +802,11 @@ func (r *ReconcileArgoCD) recreateRedisHAHealthConfigMap(cr *argoproj.ArgoCD, us
753802
// reconcileSSHKnownHosts will ensure that the ArgoCD SSH Known Hosts ConfigMap is present.
754803
func (r *ReconcileArgoCD) reconcileSSHKnownHosts(cr *argoproj.ArgoCD) error {
755804
cm := newConfigMapWithName(common.ArgoCDKnownHostsConfigMapName, cr)
756-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
805+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm)
806+
if err != nil {
807+
return err
808+
}
809+
if exists {
757810
return nil // ConfigMap found, move along...
758811
}
759812

@@ -771,7 +824,11 @@ func (r *ReconcileArgoCD) reconcileSSHKnownHosts(cr *argoproj.ArgoCD) error {
771824
// reconcileTLSCerts will ensure that the ArgoCD TLS Certs ConfigMap is present.
772825
func (r *ReconcileArgoCD) reconcileTLSCerts(cr *argoproj.ArgoCD) error {
773826
cm := newConfigMapWithName(common.ArgoCDTLSCertsConfigMapName, cr)
774-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
827+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm)
828+
if err != nil {
829+
return err
830+
}
831+
if exists {
775832
return nil // ConfigMap found, move along...
776833
}
777834

@@ -787,7 +844,11 @@ func (r *ReconcileArgoCD) reconcileTLSCerts(cr *argoproj.ArgoCD) error {
787844
// reconcileGPGKeysConfigMap creates a gpg-keys config map
788845
func (r *ReconcileArgoCD) reconcileGPGKeysConfigMap(cr *argoproj.ArgoCD) error {
789846
cm := newConfigMapWithName(common.ArgoCDGPGKeysConfigMapName, cr)
790-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
847+
exists, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm)
848+
if err != nil {
849+
return err
850+
}
851+
if exists {
791852
return nil
792853
}
793854
if err := controllerutil.SetControllerReference(cr, cm, r.Scheme); err != nil {
@@ -818,7 +879,12 @@ func (r *ReconcileArgoCD) reconcileArgoCmdParamsConfigMap(cr *argoproj.ArgoCD) e
818879
}
819880

820881
existingCM := &corev1.ConfigMap{}
821-
if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
882+
isFound, err := argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM)
883+
if err != nil {
884+
return err
885+
}
886+
if isFound {
887+
822888
changed := false
823889

824890
// Compare only if data is being managed

‎controllers/argocd/configmap_test.go‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -132,20 +132,27 @@ func TestReconcileArgoCD_reconcileRedisHAHealthConfigMap(t *testing.T) {
132132

133133
// Modify ConfigMap data to simulate external changes
134134
existingCM := &corev1.ConfigMap{}
135-
assert.True(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCM))
135+
136+
exists, err := argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCM)
137+
assert.Nil(t, err)
138+
assert.True(t, exists)
136139
existingCM.Data["redis_liveness.sh"] = "modified_script_content"
137140
assert.NoError(t, cl.Update(context.TODO(), existingCM))
138141

139142
// Reconcile again and verify changes are reverted
140143
assert.NoError(t, r.reconcileRedisHAHealthConfigMap(cr, false))
141144
existingCMAfter := &corev1.ConfigMap{}
142-
assert.True(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCMAfter))
145+
exists, err = argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCMAfter)
146+
assert.True(t, exists)
147+
assert.Nil(t, err)
143148
assert.Equal(t, getRedisLivenessScript(false), existingCMAfter.Data["redis_liveness.sh"])
144149

145150
// Disable HA and ensure ConfigMap is deleted
146151
cr.Spec.HA.Enabled = false
147152
assert.NoError(t, r.reconcileRedisHAHealthConfigMap(cr, false))
148-
assert.False(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCM))
153+
exists, err = argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCM)
154+
assert.Nil(t, err)
155+
assert.False(t, exists)
149156
}
150157

151158
// TestReconcileArgoCD_reconcileRedisHAConfigMap tests the reconcileRedisHAConfigMap function.
@@ -169,20 +176,26 @@ func TestReconcileArgoCD_reconcileRedisHAConfigMap(t *testing.T) {
169176

170177
// Modify ConfigMap data to simulate external changes
171178
existingCM := &corev1.ConfigMap{}
172-
assert.True(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCM))
179+
exists, err := argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCM)
180+
assert.Nil(t, err)
181+
assert.True(t, exists)
173182
existingCM.Data["haproxy.cfg"] = "modified_config_content"
174183
assert.NoError(t, cl.Update(context.TODO(), existingCM))
175184

176185
// Reconcile again and verify changes are reverted
177186
assert.NoError(t, r.reconcileRedisHAConfigMap(cr, false))
178187
existingCMAfter := &corev1.ConfigMap{}
179-
assert.True(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCMAfter))
188+
exists, err = argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCMAfter)
189+
assert.Nil(t, err)
190+
assert.True(t, exists)
180191
assert.Equal(t, getRedisHAProxyConfig(cr, false), existingCMAfter.Data["haproxy.cfg"])
181192

182193
// Disable HA and ensure ConfigMap is deleted
183194
cr.Spec.HA.Enabled = false
184195
assert.NoError(t, r.reconcileRedisHAConfigMap(cr, false))
185-
assert.False(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCM))
196+
exists, err = argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCM)
197+
assert.Nil(t, err)
198+
assert.False(t, exists)
186199
}
187200

188201
func TestReconcileArgoCD_reconcileTLSCerts_withInitialCertsUpdate(t *testing.T) {

0 commit comments

Comments
 (0)