Better status functions

We pass in a function to query the current state of objects, this
allows us to cache the results (if using SSA) and allows for cluster
targeting.

Author: justinsb

pkg/patterns/addon/pkg/status/aggregate.go

@@ -25,66 +25,70 @@ import (
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative"
-	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
 const successfulDeployment = appsv1.DeploymentAvailable
 
 // NewAggregator provides an implementation of declarative.Reconciled that
 // aggregates the status of deployed objects to configure the 'Healthy'
 // field on an addon that derives from CommonStatus
-func NewAggregator(client client.Client) *aggregator {
-	return &aggregator{client}
+//
+// TODO: Create a version that doesn't require the unused client arg
+func NewAggregator(_ client.Client) *aggregator {
+	return &aggregator{}
 }
 
 type aggregator struct {
-	client client.Client
 }
 
-func (a *aggregator) Reconciled(ctx context.Context, src declarative.DeclarativeObject, objs *manifest.Objects, reconcileErr error) error {
+func (a *aggregator) BuildStatus(ctx context.Context, info *declarative.StatusInfo) error {
 	log := log.FromContext(ctx)
 
 	statusHealthy := true
 	statusErrors := []string{}
 
-	if reconcileErr != nil {
+	shouldComputeHealthFromObjects := info.Manifest != nil && info.LiveObjects != nil
+	if info.Err != nil {
 		statusHealthy = false
+		shouldComputeHealthFromObjects = false
 	}
 
-	for _, o := range objs.GetItems() {
-		gk := o.Group + "/" + o.Kind
-		healthy := true
-		objKey := client.ObjectKey{
-			Name:      o.GetName(),
-			Namespace: o.GetNamespace(),
-		}
-		// If the namespace isn't set on the object, we would want to use the namespace of src
-		if objKey.Namespace == "" {
-			objKey.Namespace = src.GetNamespace()
-		}
-		var err error
-		switch gk {
-		case "/Service":
-			healthy, err = a.service(ctx, objKey)
-		case "extensions/Deployment", "apps/Deployment":
-			healthy, err = a.deployment(ctx, objKey)
-		default:
-			log.WithValues("type", gk).V(2).Info("type not implemented for status aggregation, skipping")
-		}
-
-		statusHealthy = statusHealthy && healthy
-		if err != nil {
-			statusErrors = append(statusErrors, fmt.Sprintf("%v", err))
+	if shouldComputeHealthFromObjects {
+		for _, o := range info.Manifest.GetItems() {
+			gvk := o.GroupVersionKind()
+			nn := o.NamespacedName()
+
+			log := log.WithValues("kind", gvk.Kind).WithValues("name", nn.Name).WithValues("namespace", nn.Namespace)
+
+			healthy := true
+
+			var err error
+			switch gvk.Group + "/" + gvk.Kind {
+			case "/Service":
+				healthy, err = a.serviceIsHealthy(ctx, info.LiveObjects, gvk, nn)
+			case "extensions/Deployment", "apps/Deployment":
+				healthy, err = a.deploymentIsHealthy(ctx, info.LiveObjects, gvk, nn)
+			default:
+				log.V(4).Info("type not implemented for status aggregation, skipping")
+			}
+
+			statusHealthy = statusHealthy && healthy
+			if err != nil {
+				statusErrors = append(statusErrors, fmt.Sprintf("%v", err))
+			}
 		}
 	}
 
-	log.WithValues("object", src).WithValues("status", statusHealthy).V(2).Info("built status")
+	log.WithValues("status", statusHealthy).V(2).Info("built status")
 
-	currentStatus, err := utils.GetCommonStatus(src)
+	currentStatus, err := utils.GetCommonStatus(info.Subject)
 	if err != nil {
 		return err
 	}
@@ -94,27 +98,24 @@ func (a *aggregator) Reconciled(ctx context.Context, src declarative.Declarative
 	status.Errors = statusErrors
 
 	if !reflect.DeepEqual(status, currentStatus) {
-		err := utils.SetCommonStatus(src, status)
+		err := utils.SetCommonStatus(info.Subject, status)
 		if err != nil {
 			return err
 		}
-
-		log.WithValues("name", src.GetName()).WithValues("status", status).Info("updating status")
-		err = a.client.Status().Update(ctx, src)
-		if err != nil {
-			log.Error(err, "updating status")
-			return err
-		}
 	}
 
 	return nil
 }
 
-func (a *aggregator) deployment(ctx context.Context, key client.ObjectKey) (bool, error) {
-	dep := &appsv1.Deployment{}
+func (a *aggregator) deploymentIsHealthy(ctx context.Context, liveObjects declarative.LiveObjectReader, gvk schema.GroupVersionKind, nn types.NamespacedName) (bool, error) {
+	u, err := liveObjects(ctx, gvk, nn)
+	if err != nil {
+		return false, fmt.Errorf("error reading deployment: %w", err)
+	}
 
-	if err := a.client.Get(ctx, key, dep); err != nil {
-		return false, fmt.Errorf("error reading deployment (%s): %v", key, err)
+	dep := &appsv1.Deployment{}
+	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, dep); err != nil {
+		return false, fmt.Errorf("error converting deployment from unstructured: %w", err)
 	}
 
 	for _, cond := range dep.Status.Conditions {
@@ -123,14 +124,13 @@ func (a *aggregator) deployment(ctx context.Context, key client.ObjectKey) (bool
 		}
 	}
 
-	return false, fmt.Errorf("deployment (%s) does not meet condition: %s", key, successfulDeployment)
+	return false, fmt.Errorf("deployment does not meet condition: %s", successfulDeployment)
 }
 
-func (a *aggregator) service(ctx context.Context, key client.ObjectKey) (bool, error) {
-	svc := &corev1.Service{}
-	err := a.client.Get(ctx, key, svc)
+func (a *aggregator) serviceIsHealthy(ctx context.Context, liveObjects declarative.LiveObjectReader, gvk schema.GroupVersionKind, nn types.NamespacedName) (bool, error) {
+	_, err := liveObjects(ctx, gvk, nn)
 	if err != nil {
-		return false, fmt.Errorf("error reading service (%s): %v", key, err)
+		return false, fmt.Errorf("error reading service: %w", err)
 	}
 
 	return true, nil

pkg/patterns/addon/pkg/status/basic.go

@@ -21,13 +21,13 @@ import (
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative"
 )
 
-// Deprecated: This function exists for backward compatibility, please use NewKstatusCheck
-
 // NewBasic provides an implementation of declarative.Status that
 // performs no preflight checks.
+//
+// Deprecated: This function exists for backward compatibility, please use NewKstatusCheck
 func NewBasic(client client.Client) declarative.Status {
 	return &declarative.StatusBuilder{
-		ReconciledImpl: NewAggregator(client),
+		BuildStatusImpl: NewAggregator(client),
 		// no preflight checks
 	}
 }
@@ -41,14 +41,15 @@ func NewBasicVersionChecks(client client.Client, version string) (declarative.St
 	}
 
 	return &declarative.StatusBuilder{
-		ReconciledImpl:   NewAggregator(client),
+		BuildStatusImpl:  NewAggregator(client),
 		VersionCheckImpl: v,
 		// no preflight checks
 	}, nil
 }
 
+// TODO: Create a version that doesn't take (unusued) client & reconciler args
 func NewKstatusCheck(client client.Client, d *declarative.Reconciler) declarative.Status {
 	return &declarative.StatusBuilder{
-		ReconciledImpl: NewKstatusAgregator(client, d),
+		BuildStatusImpl: NewKstatusAgregator(client, d),
 	}
 }

pkg/patterns/addon/pkg/status/kstatus.go

@@ -2,71 +2,86 @@ package status
 
 import (
 	"context"
-	"fmt"
 
 	"sigs.k8s.io/cli-utils/pkg/kstatus/status"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/addon/pkg/utils"
 	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative"
-	"sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative/pkg/manifest"
 )
 
 type kstatusAggregator struct {
-	client     client.Client
-	reconciler *declarative.Reconciler
 }
 
-func NewKstatusAgregator(c client.Client, reconciler *declarative.Reconciler) *kstatusAggregator {
-	return &kstatusAggregator{client: c, reconciler: reconciler}
+// TODO: Create a version that doesn't need reconciler or client?
+func NewKstatusAgregator(_ client.Client, _ *declarative.Reconciler) *kstatusAggregator {
+	return &kstatusAggregator{}
 }
 
-func (k *kstatusAggregator) Reconciled(ctx context.Context, src declarative.DeclarativeObject,
-	objs *manifest.Objects, _ error) error {
+func (k *kstatusAggregator) BuildStatus(ctx context.Context, info *declarative.StatusInfo) error {
 	log := log.FromContext(ctx)
 
-	statusMap := make(map[status.Status]bool)
-	for _, object := range objs.Items {
+	currentStatus, err := utils.GetCommonStatus(info.Subject)
+	if err != nil {
+		log.Error(err, "error retrieving status")
+		return err
+	}
 
-		unstruct, err := declarative.GetObjectFromCluster(object, k.reconciler)
-		if err != nil {
-			log.WithValues("object", object.Kind+"/"+object.GetName()).Error(err, "Unable to get status of object")
-			return err
+	shouldComputeHealthFromObjects := info.Manifest != nil && info.LiveObjects != nil
+	if info.Err != nil {
+		currentStatus.Healthy = false
+		switch info.KnownError {
+		case declarative.KnownErrorApplyFailed:
+			currentStatus.Phase = "Applying"
+			// computeHealthFromObjects if we can (leave unchanged)
+		case declarative.KnownErrorVersionCheckFailed:
+			currentStatus.Phase = "VersionMismatch"
+			shouldComputeHealthFromObjects = false
+		default:
+			currentStatus.Phase = "InternalError"
+			shouldComputeHealthFromObjects = false
 		}
+	}
 
-		res, err := status.Compute(unstruct)
-		if err != nil {
-			log.WithValues("kind", object.Kind).WithValues("name", object.GetName()).WithValues("status", res.Status).WithValues(
-				"message", res.Message).Info("Got status of resource:")
-			statusMap[status.NotFoundStatus] = true
+	if shouldComputeHealthFromObjects {
+		statusMap := make(map[status.Status]bool)
+		for _, object := range info.Manifest.Items {
+			gvk := object.GroupVersionKind()
+			nn := object.NamespacedName()
+
+			log := log.WithValues("kind", gvk.Kind).WithValues("name", nn.Name).WithValues("namespace", nn.Namespace)
+
+			unstruct, err := info.LiveObjects(ctx, gvk, nn)
+			if err != nil {
+				log.Error(err, "unable to get object to determine status")
+				statusMap[status.UnknownStatus] = true
+				continue
+			}
+
+			res, err := status.Compute(unstruct)
+			if err != nil {
+				log.Error(err, "error getting status of resource")
+				statusMap[status.UnknownStatus] = true
+			} else if res != nil {
+				log.WithValues("status", res.Status).WithValues("message", res.Message).Info("Got status of resource:")
+				statusMap[res.Status] = true
+			} else {
+				log.Info("resource status was nil")
+				statusMap[status.UnknownStatus] = true
+			}
 		}
-		if res != nil {
-			log.WithValues("kind", object.Kind).WithValues("name", object.GetName()).WithValues("status", res.Status).WithValues("message", res.Message).Info("Got status of resource:")
-			statusMap[res.Status] = true
+
+		aggregatedPhase := string(aggregateStatus(statusMap))
+
+		if currentStatus.Phase != aggregatedPhase {
+			currentStatus.Phase = aggregatedPhase
 		}
 	}
 
-	aggregatedPhase := string(aggregateStatus(statusMap))
-
-	currentStatus, err := utils.GetCommonStatus(src)
-	if err != nil {
-		log.Error(err, "error retrieving status")
+	if err := utils.SetCommonStatus(info.Subject, currentStatus); err != nil {
 		return err
 	}
-	if currentStatus.Phase != aggregatedPhase {
-		currentStatus.Phase = aggregatedPhase
-		err := utils.SetCommonStatus(src, currentStatus)
-		if err != nil {
-			return err
-		}
-		log.WithValues("name", src.GetName()).WithValues("phase", aggregatedPhase).Info("updating status")
-		err = k.client.Status().Update(ctx, src)
-		if err != nil {
-			log.Error(err, "error updating status")
-			return fmt.Errorf("error error status: %v", err)
-		}
-	}
 
 	return nil
 }

pkg/patterns/declarative/pkg/manifest/objects.go

@@ -27,6 +27,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	k8syaml "k8s.io/apimachinery/pkg/util/yaml"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
@@ -320,6 +321,16 @@ func (o *Object) GroupVersionKind() schema.GroupVersionKind {
 	return o.object.GroupVersionKind()
 }
 
+// NamespacedName returns the name and namespace of the object in a types.NamespacedName.
+// Note that this reflects the state of the object; if the namespace is not yet set,
+// it will returned as "" here, even though it would likely be defaulted before apply.
+func (o *Object) NamespacedName() types.NamespacedName {
+	return types.NamespacedName{
+		Namespace: o.GetNamespace(),
+		Name:      o.GetName(),
+	}
+}
+
 func (o *Objects) JSONManifest() (string, error) {
 	var b bytes.Buffer