Report not ready when no namespace matches (#2582)

* Report not ready when no namespace matches

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

* Fix flaky a test

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

* Simplify ClusterExternalSecret status

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

---------

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>

apis/externalsecrets/v1beta1/clusterexternalsecret_types.go

@@ -50,11 +50,7 @@ type ExternalSecretMetadata struct {
 
 type ClusterExternalSecretConditionType string
 
-const (
-	ClusterExternalSecretReady          ClusterExternalSecretConditionType = "Ready"
-	ClusterExternalSecretPartiallyReady ClusterExternalSecretConditionType = "PartiallyReady"
-	ClusterExternalSecretNotReady       ClusterExternalSecretConditionType = "NotReady"
-)
+const ClusterExternalSecretReady ClusterExternalSecretConditionType = "Ready"
 
 type ClusterExternalSecretStatusCondition struct {
 	Type   ClusterExternalSecretConditionType `json:"type"`

docs/api/spec.md

@@ -1230,11 +1230,7 @@ ClusterExternalSecretStatus
 <th>Description</th>
 </tr>
 </thead>
-<tbody><tr><td><p>&#34;NotReady&#34;</p></td>
-<td></td>
-</tr><tr><td><p>&#34;PartiallyReady&#34;</p></td>
-<td></td>
-</tr><tr><td><p>&#34;Ready&#34;</p></td>
+<tbody><tr><td><p>&#34;Ready&#34;</p></td>
 <td></td>
 </tr></tbody>
 </table>

pkg/controllers/clusterexternalsecret/cesmetrics/cesmetrics.go

@@ -72,43 +72,21 @@ func UpdateClusterExternalSecretCondition(ces *esv1beta1.ClusterExternalSecret,
 	conditionLabels := ctrlmetrics.RefineConditionMetricLabels(cesInfo)
 	clusterExternalSecretCondition := GetGaugeVec(ClusterExternalSecretStatusConditionKey)
 
-	targetConditionType := condition.Type
-	var theOtherConditionTypes []esv1beta1.ClusterExternalSecretConditionType
-	for _, ct := range []esv1beta1.ClusterExternalSecretConditionType{
-		esv1beta1.ClusterExternalSecretReady,
-		esv1beta1.ClusterExternalSecretPartiallyReady,
-		esv1beta1.ClusterExternalSecretNotReady,
-	} {
-		if ct != targetConditionType {
-			theOtherConditionTypes = append(theOtherConditionTypes, ct)
-		}
+	theOtherStatus := v1.ConditionFalse
+	if condition.Status == v1.ConditionFalse {
+		theOtherStatus = v1.ConditionTrue
 	}
 
-	// Set the target condition metric
 	clusterExternalSecretCondition.With(ctrlmetrics.RefineLabels(conditionLabels,
 		map[string]string{
-			"condition": string(targetConditionType),
-			"status":    string(v1.ConditionTrue),
+			"condition": string(condition.Type),
+			"status":    string(condition.Status),
 		})).Set(1)
 	clusterExternalSecretCondition.With(ctrlmetrics.RefineLabels(conditionLabels,
 		map[string]string{
-			"condition": string(targetConditionType),
-			"status":    string(v1.ConditionFalse),
+			"condition": string(condition.Type),
+			"status":    string(theOtherStatus),
 		})).Set(0)
-
-	// Remove the other condition metrics
-	for _, ct := range theOtherConditionTypes {
-		clusterExternalSecretCondition.Delete(ctrlmetrics.RefineLabels(conditionLabels,
-			map[string]string{
-				"condition": string(ct),
-				"status":    string(v1.ConditionFalse),
-			}))
-		clusterExternalSecretCondition.Delete(ctrlmetrics.RefineLabels(conditionLabels,
-			map[string]string{
-				"condition": string(ct),
-				"status":    string(v1.ConditionTrue),
-			}))
-	}
 }
 
 // RemoveMetrics deletes all metrics published by the resource.

pkg/controllers/clusterexternalsecret/clusterexternalsecret_controller.go

@@ -58,6 +58,7 @@ const (
 	errNamespaces           = "could not get namespaces from selector"
 	errGetExistingES        = "could not get existing ExternalSecret"
 	errNamespacesFailed     = "one or more namespaces failed"
+	errNamespaceNotFound    = "no namespace matches"
 )
 
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
@@ -92,14 +93,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	labelSelector, err := metav1.LabelSelectorAsSelector(&clusterExternalSecret.Spec.NamespaceSelector)
 	if err != nil {
 		log.Error(err, errConvertLabelSelector)
-		return ctrl.Result{RequeueAfter: refreshInt}, err
+		return ctrl.Result{}, err
 	}
 
 	namespaceList := v1.NamespaceList{}
 	err = r.List(ctx, &namespaceList, &client.ListOptions{LabelSelector: labelSelector})
 	if err != nil {
 		log.Error(err, errNamespaces)
-		return ctrl.Result{RequeueAfter: refreshInt}, err
+		return ctrl.Result{}, err
 	}
 
 	esName := clusterExternalSecret.Spec.ExternalSecretName

pkg/controllers/clusterexternalsecret/clusterexternalsecret_controller_test.go

@@ -394,8 +394,8 @@ var _ = Describe("ClusterExternalSecret controller", func() {
 						},
 						Conditions: []esv1beta1.ClusterExternalSecretStatusCondition{
 							{
-								Type:    esv1beta1.ClusterExternalSecretNotReady,
-								Status:  v1.ConditionTrue,
+								Type:    esv1beta1.ClusterExternalSecretReady,
+								Status:  v1.ConditionFalse,
 								Message: "one or more namespaces failed",
 							},
 						},
@@ -420,7 +420,7 @@ var _ = Describe("ClusterExternalSecret controller", func() {
 				}
 			},
 		}),
-		Entry("Should crate an external secret and if one with the same name has been deleted", testCase{
+		Entry("Should crate an external secret if one with the same name has been deleted", testCase{
 			namespaces: []v1.Namespace{
 				{ObjectMeta: metav1.ObjectMeta{Name: randomNamespaceName()}},
 			},
@@ -464,11 +464,6 @@ var _ = Describe("ClusterExternalSecret controller", func() {
 						ProvisionedNamespaces: []string{namespaces[0].Name},
 						Conditions: []esv1beta1.ClusterExternalSecretStatusCondition{
 							{
-								Type:    esv1beta1.ClusterExternalSecretNotReady,
-								Status:  v1.ConditionTrue,
-								Message: "one or more namespaces failed",
-							},
-							{
 								Type:   esv1beta1.ClusterExternalSecretReady,
 								Status: v1.ConditionTrue,
 							},
@@ -518,10 +513,8 @@ var _ = Describe("ClusterExternalSecret controller", func() {
 					}
 				}).WithTimeout(timeout).WithPolling(interval).Should(Succeed())
 
-				for _, ns := range namespaces {
-					ns.Labels = map[string]string{}
-					Expect(k8sClient.Update(ctx, &ns)).ShouldNot(HaveOccurred())
-				}
+				namespaces[0].Labels = map[string]string{}
+				Expect(k8sClient.Update(ctx, &namespaces[0])).ShouldNot(HaveOccurred())
 			},
 			expectedClusterExternalSecret: func(namespaces []v1.Namespace, created esv1beta1.ClusterExternalSecret) esv1beta1.ClusterExternalSecret {
 				return esv1beta1.ClusterExternalSecret{
@@ -530,7 +523,8 @@ var _ = Describe("ClusterExternalSecret controller", func() {
 					},
 					Spec: created.Spec,
 					Status: esv1beta1.ClusterExternalSecretStatus{
-						ExternalSecretName: created.Name,
+						ExternalSecretName:    created.Name,
+						ProvisionedNamespaces: []string{namespaces[1].Name},
 						Conditions: []esv1beta1.ClusterExternalSecretStatusCondition{
 							{
 								Type:   esv1beta1.ClusterExternalSecretReady,
@@ -541,7 +535,15 @@ var _ = Describe("ClusterExternalSecret controller", func() {
 				}
 			},
 			expectedExternalSecrets: func(namespaces []v1.Namespace, created esv1beta1.ClusterExternalSecret) []esv1beta1.ExternalSecret {
-				return []esv1beta1.ExternalSecret{}
+				return []esv1beta1.ExternalSecret{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: namespaces[1].Name,
+							Name:      created.Name,
+						},
+						Spec: created.Spec.ExternalSecretSpec,
+					},
+				}
 			},
 		}),
 		Entry("Should sync with match expression", testCase{
@@ -615,6 +617,41 @@ var _ = Describe("ClusterExternalSecret controller", func() {
 					},
 				}
 			},
+		}),
+		Entry("Should not be ready if no namespace matches", testCase{
+			namespaces: []v1.Namespace{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: randomNamespaceName(),
+					},
+				},
+			},
+			clusterExternalSecret: func(namespaces []v1.Namespace) esv1beta1.ClusterExternalSecret {
+				ces := defaultClusterExternalSecret()
+				ces.Spec.NamespaceSelector.MatchLabels = map[string]string{"kubernetes.io/metadata.name": "no-namespace-matches"}
+				return *ces
+			},
+			expectedClusterExternalSecret: func(namespaces []v1.Namespace, created esv1beta1.ClusterExternalSecret) esv1beta1.ClusterExternalSecret {
+				return esv1beta1.ClusterExternalSecret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: created.Name,
+					},
+					Spec: created.Spec,
+					Status: esv1beta1.ClusterExternalSecretStatus{
+						ExternalSecretName: created.Name,
+						Conditions: []esv1beta1.ClusterExternalSecretStatusCondition{
+							{
+								Type:    esv1beta1.ClusterExternalSecretReady,
+								Status:  v1.ConditionFalse,
+								Message: errNamespaceNotFound,
+							},
+						},
+					},
+				}
+			},
+			expectedExternalSecrets: func(namespaces []v1.Namespace, created esv1beta1.ClusterExternalSecret) []esv1beta1.ExternalSecret {
+				return []esv1beta1.ExternalSecret{}
+			},
 		}))
 })
 

pkg/controllers/clusterexternalsecret/util.go

@@ -22,14 +22,20 @@ import (
 )
 
 func NewClusterExternalSecretCondition(failedNamespaces map[string]error, namespaceList *v1.NamespaceList) *esv1beta1.ClusterExternalSecretStatusCondition {
-	conditionType := getConditionType(failedNamespaces, namespaceList)
-	condition := &esv1beta1.ClusterExternalSecretStatusCondition{
-		Type:   conditionType,
-		Status: v1.ConditionTrue,
+	if len(namespaceList.Items) > 0 && len(failedNamespaces) == 0 {
+		return &esv1beta1.ClusterExternalSecretStatusCondition{
+			Type:   esv1beta1.ClusterExternalSecretReady,
+			Status: v1.ConditionTrue,
+		}
 	}
 
-	if conditionType != esv1beta1.ClusterExternalSecretReady {
-		condition.Message = errNamespacesFailed
+	condition := &esv1beta1.ClusterExternalSecretStatusCondition{
+		Type:    esv1beta1.ClusterExternalSecretReady,
+		Status:  v1.ConditionFalse,
+		Message: errNamespacesFailed,
+	}
+	if len(failedNamespaces) == 0 {
+		condition.Message = errNamespaceNotFound
 	}
 
 	return condition
@@ -51,15 +57,3 @@ func filterOutCondition(conditions []esv1beta1.ClusterExternalSecretStatusCondit
 	}
 	return newConditions
 }
-
-func getConditionType(failedNamespaces map[string]error, namespaceList *v1.NamespaceList) esv1beta1.ClusterExternalSecretConditionType {
-	if len(failedNamespaces) == 0 {
-		return esv1beta1.ClusterExternalSecretReady
-	}
-
-	if len(failedNamespaces) < len(namespaceList.Items) {
-		return esv1beta1.ClusterExternalSecretPartiallyReady
-	}
-
-	return esv1beta1.ClusterExternalSecretNotReady
-}