Add status.Conditions - ready with count of reflected clusters (#17)

* Add status.Conditions - ready with count of reflected clusters
* Add default value for ObservedGeneration
---------
Co-authored-by: Simon <simon@weave.works>

api/v1alpha1/automatedclusterdiscovery_types.go

@@ -64,6 +64,15 @@ type AutomatedClusterDiscoverySpec struct {
 type AutomatedClusterDiscoveryStatus struct {
 	meta.ReconcileRequestStatus `json:",inline"`
 
+	// ObservedGeneration is the last observed generation of the
+	// object.
+	// +optional
+	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
+
+	// Conditions holds the conditions for the AutomatedClusterDiscovery
+	// +optional
+	Conditions []metav1.Condition `json:"conditions,omitempty"`
+
 	// Inventory contains the list of Kubernetes resource object references that
 	// have been successfully applied
 	// +optional
@@ -72,13 +81,16 @@ type AutomatedClusterDiscoveryStatus struct {
 
 //+kubebuilder:object:root=true
 //+kubebuilder:subresource:status
+//+kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type==\"Ready\")].status",description=""
+//+kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.conditions[?(@.type==\"Ready\")].message",description=""
 
 // AutomatedClusterDiscovery is the Schema for the automatedclusterdiscoveries API
 type AutomatedClusterDiscovery struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
 
-	Spec   AutomatedClusterDiscoverySpec   `json:"spec,omitempty"`
+	Spec AutomatedClusterDiscoverySpec `json:"spec,omitempty"`
+	// +kubebuilder:default={"observedGeneration":-1}
 	Status AutomatedClusterDiscoveryStatus `json:"status,omitempty"`
 }
 

api/v1alpha1/conditions.go

@@ -0,0 +1,42 @@
+package v1alpha1
+
+import (
+	"github.com/fluxcd/pkg/apis/meta"
+	apimeta "k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+const (
+	// ReconciliationFailedReason represents the fact that
+	// the reconciliation failed.
+	ReconciliationFailedReason string = "ReconciliationFailed"
+
+	// ReconciliationSucceededReason represents the fact that
+	// the reconciliation succeeded.
+	ReconciliationSucceededReason string = "ReconciliationSucceeded"
+)
+
+// SetAutomatedClusterDiscoveryReadiness sets the ready condition with the given status, reason and message.
+func SetAutomatedClusterDiscoveryReadiness(acd *AutomatedClusterDiscovery, inventory *ResourceInventory, status metav1.ConditionStatus, reason, message string) {
+	if inventory != nil {
+		acd.Status.Inventory = inventory
+
+		if len(inventory.Entries) == 0 {
+			acd.Status.Inventory = nil
+		}
+	}
+
+	acd.Status.ObservedGeneration = acd.ObjectMeta.Generation
+	newCondition := metav1.Condition{
+		Type:    meta.ReadyCondition,
+		Status:  status,
+		Reason:  reason,
+		Message: message,
+	}
+	apimeta.SetStatusCondition(&acd.Status.Conditions, newCondition)
+}
+
+// GetAutomatedClusterDiscoveryReadiness returns the readiness condition of the AutomatedClusterDiscovery.
+func GetAutomatedClusterDiscoveryReadiness(acd *AutomatedClusterDiscovery) metav1.ConditionStatus {
+	return apimeta.FindStatusCondition(acd.Status.Conditions, meta.ReadyCondition).Status
+}

config/crd/bases/clusters.weave.works_automatedclusterdiscoveries.yaml

@@ -14,7 +14,14 @@ spec:
     singular: automatedclusterdiscovery
   scope: Namespaced
   versions:
-  - name: v1alpha1
+  - additionalPrinterColumns:
+    - jsonPath: .status.conditions[?(@.type=="Ready")].status
+      name: Ready
+      type: string
+    - jsonPath: .status.conditions[?(@.type=="Ready")].message
+      name: Status
+      type: string
+    name: v1alpha1
     schema:
       openAPIV3Schema:
         description: AutomatedClusterDiscovery is the Schema for the automatedclusterdiscoveries
@@ -80,6 +87,75 @@ spec:
             description: AutomatedClusterDiscoveryStatus defines the observed state
               of AutomatedClusterDiscovery
             properties:
+              conditions:
+                description: Conditions holds the conditions for the AutomatedClusterDiscovery
+                items:
+                  description: "Condition contains details for one aspect of the current
+                    state of this API Resource. --- This struct is intended for direct
+                    use as an array at the field path .status.conditions.  For example,
+                    \n type FooStatus struct{ // Represents the observations of a
+                    foo's current state. // Known .status.conditions.type are: \"Available\",
+                    \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge
+                    // +listType=map // +listMapKey=type Conditions []metav1.Condition
+                    `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"
+                    protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }"
+                  properties:
+                    lastTransitionTime:
+                      description: lastTransitionTime is the last time the condition
+                        transitioned from one status to another. This should be when
+                        the underlying condition changed.  If that is not known, then
+                        using the time when the API field changed is acceptable.
+                      format: date-time
+                      type: string
+                    message:
+                      description: message is a human readable message indicating
+                        details about the transition. This may be an empty string.
+                      maxLength: 32768
+                      type: string
+                    observedGeneration:
+                      description: observedGeneration represents the .metadata.generation
+                        that the condition was set based upon. For instance, if .metadata.generation
+                        is currently 12, but the .status.conditions[x].observedGeneration
+                        is 9, the condition is out of date with respect to the current
+                        state of the instance.
+                      format: int64
+                      minimum: 0
+                      type: integer
+                    reason:
+                      description: reason contains a programmatic identifier indicating
+                        the reason for the condition's last transition. Producers
+                        of specific condition types may define expected values and
+                        meanings for this field, and whether the values are considered
+                        a guaranteed API. The value should be a CamelCase string.
+                        This field may not be empty.
+                      maxLength: 1024
+                      minLength: 1
+                      pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$
+                      type: string
+                    status:
+                      description: status of the condition, one of True, False, Unknown.
+                      enum:
+                      - "True"
+                      - "False"
+                      - Unknown
+                      type: string
+                    type:
+                      description: type of condition in CamelCase or in foo.example.com/CamelCase.
+                        --- Many .condition.type values are consistent across resources
+                        like Available, but because arbitrary conditions can be useful
+                        (see .node.status.conditions), the ability to deconflict is
+                        important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
+                      maxLength: 316
+                      pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$
+                      type: string
+                  required:
+                  - lastTransitionTime
+                  - message
+                  - reason
+                  - status
+                  - type
+                  type: object
+                type: array
               inventory:
                 description: Inventory contains the list of Kubernetes resource object
                   references that have been successfully applied
@@ -109,6 +185,11 @@ spec:
                   reconcile request value, so a change of the annotation value can
                   be detected.
                 type: string
+              observedGeneration:
+                description: ObservedGeneration is the last observed generation of
+                  the object.
+                format: int64
+                type: integer
             type: object
         type: object
     served: true

internal/controller/automatedclusterdiscovery_controller.go

@@ -88,43 +88,42 @@ func (r *AutomatedClusterDiscoveryReconciler) Reconcile(ctx context.Context, req
 		}
 	}
 
-	if clusterDiscovery.Spec.Type == "aks" {
-		logger.Info("reconciling AKS cluster reflector",
-			"name", clusterDiscovery.Spec.Name,
-		)
-
-		azureProvider := r.AKSProvider(clusterDiscovery.Spec.AKS.SubscriptionID)
+	inventoryRefs, err := r.reconcileResources(ctx, clusterDiscovery)
+	if err != nil {
+		clustersv1alpha1.SetAutomatedClusterDiscoveryReadiness(clusterDiscovery, clusterDiscovery.Status.Inventory, metav1.ConditionFalse, clustersv1alpha1.ReconciliationFailedReason, err.Error())
 
-		// We get the clusters and cluster ID separately so that we can return
-		// the error from the Reconciler without touching the inventory.
-		clusters, err := azureProvider.ListClusters(ctx)
-		if err != nil {
-			logger.Error(err, "failed to list AKS clusters")
+		if err := r.patchStatus(ctx, req, clusterDiscovery.Status); err != nil {
 			return ctrl.Result{}, err
 		}
 
-		clusterID, err := azureProvider.ClusterID(ctx, r.Client)
-		if err != nil {
-			logger.Error(err, "failed to list get Cluster ID from AKS cluster")
-			return ctrl.Result{}, err
-		}
+		return ctrl.Result{}, err
+	}
+
+	clusterDiscovery.Status.Inventory = &clustersv1alpha1.ResourceInventory{Entries: inventoryRefs}
 
-		// TODO: Fix this so that we record the inventoryRefs even if we get an
-		// error.
-		inventoryRefs, err := r.reconcileClusters(ctx, clusters, clusterID, clusterDiscovery)
+	// Get number of clusters in inventory
+	clusters := 0
+	for _, item := range inventoryRefs {
+		objMeta, err := object.ParseObjMetadata(item.ID)
 		if err != nil {
-			return ctrl.Result{}, err
+			return ctrl.Result{}, fmt.Errorf("failed to parse object ID %s: %w", item.ID, err)
 		}
 
-		sort.Slice(inventoryRefs, func(i, j int) bool {
-			return inventoryRefs[i].ID < inventoryRefs[j].ID
-		})
+		if objMeta.GroupKind.Kind == "GitopsCluster" {
+			clusters++
+		}
+	}
 
-		clusterDiscovery.Status.Inventory = &clustersv1alpha1.ResourceInventory{Entries: inventoryRefs}
+	if inventoryRefs != nil {
+		logger.Info("reconciled clusters", "count", len(inventoryRefs))
+		clustersv1alpha1.SetAutomatedClusterDiscoveryReadiness(clusterDiscovery, clusterDiscovery.Status.Inventory, metav1.ConditionTrue, clustersv1alpha1.ReconciliationSucceededReason,
+			fmt.Sprintf("%d clusters discovered", clusters))
 
 		if err = r.patchStatus(ctx, req, clusterDiscovery.Status); err != nil {
 			return ctrl.Result{}, err
 		}
+	} else {
+		logger.Info("no clusters to reconcile")
 	}
 
 	interval := clusterDiscovery.Spec.Interval
@@ -135,6 +134,52 @@ func (r *AutomatedClusterDiscoveryReconciler) Reconcile(ctx context.Context, req
 	return ctrl.Result{RequeueAfter: interval.Duration}, nil
 }
 
+func (r *AutomatedClusterDiscoveryReconciler) reconcileResources(ctx context.Context, cd *clustersv1alpha1.AutomatedClusterDiscovery) ([]clustersv1alpha1.ResourceRef, error) {
+	logger := log.FromContext(ctx)
+
+	var err error
+	clusters, clusterID := []*providers.ProviderCluster{}, ""
+
+	if cd.Spec.Type == "aks" {
+		logger.Info("reconciling AKS cluster reflector",
+			"name", cd.Spec.Name,
+		)
+
+		azureProvider := r.AKSProvider(cd.Spec.AKS.SubscriptionID)
+
+		// We get the clusters and cluster ID separately so that we can return
+		// the error from the Reconciler without touching the inventory.
+		clusters, err = azureProvider.ListClusters(ctx)
+		if err != nil {
+			logger.Error(err, "failed to list AKS clusters")
+
+			return nil, err
+		}
+
+		clusterID, err = azureProvider.ClusterID(ctx, r.Client)
+		if err != nil {
+			logger.Error(err, "failed to list get Cluster ID from AKS cluster")
+
+			return nil, err
+		}
+	}
+
+	// TODO: Fix this so that we record the inventoryRefs even if we get an
+	// error.
+	inventoryRefs, err := r.reconcileClusters(ctx, clusters, clusterID, cd)
+	if err != nil {
+		logger.Error(err, "failed to reconcile clusters")
+
+		return nil, err
+	}
+
+	sort.Slice(inventoryRefs, func(i, j int) bool {
+		return inventoryRefs[i].ID < inventoryRefs[j].ID
+	})
+
+	return inventoryRefs, nil
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *AutomatedClusterDiscoveryReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).

internal/controller/automatedclusterdiscovery_controller_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -140,6 +141,7 @@ func TestAutomatedClusterDiscoveryReconciler(t *testing.T) {
 		assertInventoryHasItems(t, aksCluster,
 			newSecret(client.ObjectKeyFromObject(secret)),
 			newGitopsCluster(secret.GetName(), client.ObjectKeyFromObject(gitopsCluster)))
+		assertAutomatedClusterDiscoveryCondition(t, aksCluster, meta.ReadyCondition, "1 clusters discovered")
 
 		clusterRef := metav1.OwnerReference{
 			Kind:       "AutomatedClusterDiscovery",
@@ -274,6 +276,7 @@ func TestAutomatedClusterDiscoveryReconciler(t *testing.T) {
 
 		secret := newSecret(types.NamespacedName{Name: "cluster-1-kubeconfig", Namespace: "default"})
 		assertInventoryHasItems(t, aksCluster, secret, gitopsCluster)
+		assertAutomatedClusterDiscoveryCondition(t, aksCluster, meta.ReadyCondition, "1 clusters discovered")
 
 		testProvider.response = []*providers.ProviderCluster{}
 
@@ -345,6 +348,7 @@ func TestAutomatedClusterDiscoveryReconciler(t *testing.T) {
 
 		secret := newSecret(types.NamespacedName{Name: "cluster-1-kubeconfig", Namespace: "default"})
 		assertInventoryHasItems(t, aksCluster, secret, gitopsCluster)
+		assertAutomatedClusterDiscoveryCondition(t, aksCluster, meta.ReadyCondition, "1 clusters discovered")
 
 		cluster.KubeConfig.Clusters["cluster-1"].Server = "https://cluster-test.example.com/"
 
@@ -474,6 +478,7 @@ func TestAutomatedClusterDiscoveryReconciler(t *testing.T) {
 		secret := newSecret(types.NamespacedName{Name: "cluster-1-kubeconfig", Namespace: aksCluster.GetNamespace()})
 		gitopsCluster := newGitopsCluster(secret.GetName(), types.NamespacedName{Name: "cluster-1", Namespace: aksCluster.GetNamespace()})
 		assertInventoryHasItems(t, aksCluster, secret, gitopsCluster)
+		assertAutomatedClusterDiscoveryCondition(t, aksCluster, meta.ReadyCondition, "1 clusters discovered")
 
 		assert.NoError(t, k8sClient.Delete(ctx, secret))
 		assert.NoError(t, k8sClient.Delete(ctx, gitopsCluster))
@@ -678,6 +683,17 @@ func deleteClusterDiscoveryAndInventory(t *testing.T, cl client.Client, cd *clus
 	}
 }
 
+func assertAutomatedClusterDiscoveryCondition(t *testing.T, acd *clustersv1alpha1.AutomatedClusterDiscovery, condType, msg string) {
+	t.Helper()
+	cond := apimeta.FindStatusCondition(acd.Status.Conditions, condType)
+	if cond == nil {
+		t.Fatalf("failed to find matching status condition for type %s in %#v", condType, acd.Status.Conditions)
+	}
+	if cond.Message != msg {
+		t.Fatalf("got %s, want %s", cond.Message, msg)
+	}
+}
+
 func assertInventoryHasItems(t *testing.T, acd *clustersv1alpha1.AutomatedClusterDiscovery, objs ...runtime.Object) {
 	t.Helper()