internal/olm: make CLI output text more consistent; check resource errors

internal/olm/client.go

@@ -72,8 +72,11 @@ func (c Client) InstallVersion(ctx context.Context, version string) (*olmresourc
 	objs := toObjects(resources...)
 
 	status := c.GetObjectsStatus(ctx, objs...)
-	if status.HasExistingResources() {
+	installed, err := status.HasInstalledResources()
+	if installed {
 		return nil, errors.New("detected existing OLM resources: OLM must be completely uninstalled before installation")
+	} else if err != nil {
+		return nil, errors.New("detected errored OLM resources, see resource statuses for more details")
 	}
 
 	log.Print("Creating CRDs and resources")
@@ -129,7 +132,8 @@ func (c Client) UninstallVersion(ctx context.Context, version string) error {
 	objs := toObjects(resources...)
 
 	status := c.GetObjectsStatus(ctx, objs...)
-	if !status.HasExistingResources() {
+	installed, err := status.HasInstalledResources()
+	if !installed && err == nil {
 		return olmresourceclient.ErrOLMNotInstalled
 	}
 
@@ -148,7 +152,8 @@ func (c Client) GetStatus(ctx context.Context, version string) (*olmresourceclie
 	objs := toObjects(resources...)
 
 	status := c.GetObjectsStatus(ctx, objs...)
-	if !status.HasExistingResources() {
+	installed, err := status.HasInstalledResources()
+	if !installed && err == nil {
 		return nil, olmresourceclient.ErrOLMNotInstalled
 	}
 	return &status, nil

internal/olm/client/client.go

@@ -89,11 +89,10 @@ func (c Client) DoCreate(ctx context.Context, objs ...runtime.Object) error {
 		log.Infof("  Creating %s %q", kind, getName(a.GetNamespace(), a.GetName()))
 		err = c.KubeClient.Create(ctx, obj)
 		if err != nil {
-			if apierrors.IsAlreadyExists(err) {
-				log.Infof("    %s %q already exists", kind, getName(a.GetNamespace(), a.GetName()))
-				return nil
+			if !apierrors.IsAlreadyExists(err) {
+				return err
 			}
-			return err
+			log.Infof("    %s %q already exists", kind, getName(a.GetNamespace(), a.GetName()))
 		}
 	}
 	return nil
@@ -109,19 +108,19 @@ func (c Client) DoDelete(ctx context.Context, objs ...runtime.Object) error {
 		log.Infof("  Deleting %s %q", kind, getName(a.GetNamespace(), a.GetName()))
 		err = c.KubeClient.Delete(ctx, obj, client.PropagationPolicy(metav1.DeletePropagationForeground))
 		if err != nil {
-			if apierrors.IsNotFound(err) {
-				log.Infof("    %s %q does not exist", kind, getName(a.GetNamespace(), a.GetName()))
-				continue
+			if !apierrors.IsNotFound(err) {
+				return err
 			}
-			return err
+			log.Infof("    %s %q does not exist", kind, getName(a.GetNamespace(), a.GetName()))
 		}
 	}
 
 	log.Infof("  Waiting for deleted resources to disappear")
 
 	return wait.PollImmediateUntil(time.Second, func() (bool, error) {
 		s := c.GetObjectsStatus(ctx, objs...)
-		return !s.HasExistingResources(), nil
+		installed, err := s.HasInstalledResources()
+		return !installed, err
 	}, ctx.Done())
 }
 
@@ -151,27 +150,27 @@ func (c Client) DoRolloutWait(ctx context.Context, key types.NamespacedName) err
 			}
 			if deployment.Spec.Replicas != nil && deployment.Status.UpdatedReplicas < *deployment.Spec.Replicas {
 				onceReplicasUpdated.Do(func() {
-					log.Printf("  Waiting for deployment %q to rollout: %d out of %d new replicas have been updated", deployment.Name, deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas)
+					log.Printf("  Waiting for Deployment %q to rollout: %d out of %d new replicas have been updated", key, deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas)
 				})
 				return false, nil
 			}
 			if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
 				oncePendingTermination.Do(func() {
-					log.Printf("  Waiting for deployment %q to rollout: %d old replicas are pending termination", deployment.Name, deployment.Status.Replicas-deployment.Status.UpdatedReplicas)
+					log.Printf("  Waiting for Deployment %q to rollout: %d old replicas are pending termination", key, deployment.Status.Replicas-deployment.Status.UpdatedReplicas)
 				})
 				return false, nil
 			}
 			if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
 				onceNotAvailable.Do(func() {
-					log.Printf("  Waiting for deployment %q to rollout: %d of %d updated replicas are available", deployment.Name, deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas)
+					log.Printf("  Waiting for Deployment %q to rollout: %d of %d updated replicas are available", key, deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas)
 				})
 				return false, nil
 			}
-			log.Printf("  Deployment %q successfully rolled out", deployment.Name)
+			log.Printf("  Deployment %q successfully rolled out", key)
 			return true, nil
 		}
 		onceSpecUpdate.Do(func() {
-			log.Printf("  Waiting for deployment %q to rollout: waiting for deployment spec update to be observed", deployment.Name)
+			log.Printf("  Waiting for Deployment %q to rollout: waiting for deployment spec update to be observed", key)
 		})
 		return false, nil
 	}
@@ -191,7 +190,7 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error {
 		if err != nil {
 			if apierrors.IsNotFound(err) {
 				once.Do(func() {
-					log.Printf("  Waiting for clusterserviceversion %q to appear", key.Name)
+					log.Printf("  Waiting for ClusterServiceVersion %q to appear", key)
 				})
 				return false, nil
 			}
@@ -200,7 +199,7 @@ func (c Client) DoCSVWait(ctx context.Context, key types.NamespacedName) error {
 		newPhase = csv.Status.Phase
 		if newPhase != curPhase {
 			curPhase = newPhase
-			log.Printf("  Found clusterserviceversion %q phase: %s", key.Name, curPhase)
+			log.Printf("  Found ClusterServiceVersion %q phase: %s", key, curPhase)
 		}
 		return curPhase == olmapiv1alpha1.CSVPhaseSucceeded, nil
 	}

internal/olm/client/status.go

@@ -20,15 +20,20 @@ package olm
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
+	"sort"
 	"text/tabwriter"
 
 	log "github.com/sirupsen/logrus"
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 type Status struct {
@@ -40,29 +45,31 @@ type ResourceStatus struct {
 	Resource       *unstructured.Unstructured
 	GVK            schema.GroupVersionKind
 	Error          error
+
+	requestObject runtime.Object // Needed for context on errors from requests on an object.
 }
 
 func (c Client) GetObjectsStatus(ctx context.Context, objs ...runtime.Object) Status {
 	var rss []ResourceStatus
 	for _, obj := range objs {
+		gvk := obj.GetObjectKind().GroupVersionKind()
 		a, aerr := meta.Accessor(obj)
 		if aerr != nil {
-			log.Fatalf("Object %s: %v", obj.GetObjectKind().GroupVersionKind(), aerr)
+			log.Fatalf("Object %s: %v", gvk, aerr)
 		}
 		nn := types.NamespacedName{
 			Namespace: a.GetNamespace(),
 			Name:      a.GetName(),
 		}
-		u := unstructured.Unstructured{}
-		u.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind())
-		err := c.KubeClient.Get(ctx, nn, &u)
 		rs := ResourceStatus{
 			NamespacedName: nn,
-			GVK:            obj.GetObjectKind().GroupVersionKind(),
+			GVK:            gvk,
+			requestObject:  obj,
 		}
-		if err != nil {
-			rs.Error = err
-		} else {
+		u := unstructured.Unstructured{}
+		u.SetGroupVersionKind(gvk)
+		rs.Error = c.KubeClient.Get(ctx, nn, &u)
+		if rs.Error == nil {
 			rs.Resource = &u
 		}
 		rss = append(rss, rs)
@@ -71,13 +78,68 @@ func (c Client) GetObjectsStatus(ctx context.Context, objs ...runtime.Object) St
 	return Status{Resources: rss}
 }
 
-func (s Status) HasExistingResources() bool {
+// HasInstalledResources only returns true if at least one resource in s
+// was returned successfully by the API server. A resource error status
+// containing any error except "not found", or "no kind match" errors
+// for Custom Resources, will result in HasInstalledResources returning
+// false and the error.
+func (s Status) HasInstalledResources() (bool, error) {
+	crdKindSet, err := s.getCRDKindSet()
+	if err != nil {
+		return false, fmt.Errorf("error getting set of CRD kinds in resources: %w", err)
+	}
+	// Sort resources by whether they're installed or not to get consistent
+	// return values.
+	sort.Slice(s.Resources, func(i int, j int) bool {
+		return s.Resources[i].Resource != nil
+	})
 	for _, r := range s.Resources {
 		if r.Resource != nil {
-			return true
+			return true, nil
+		} else if r.Error != nil && !apierrors.IsNotFound(r.Error) {
+			// We know the error is not a "resource not found" error at this point.
+			// It still may be the equivalent for a CR, "no kind match", if its
+			// corresponding CRD has been deleted already. We want to make sure
+			// we're only allowing "no kind match" errors to be skipped for CR's
+			// since we do not know if a kind is a CR kind, hence checking
+			// crdKindSet for existence of a resource's kind.
+			nkmerr := &meta.NoKindMatchError{}
+			if !errors.As(r.Error, &nkmerr) || !crdKindSet.Has(r.GVK.Kind) {
+				return false, r.Error
+			}
 		}
 	}
-	return false
+	return false, nil
+}
+
+// getCRDKindSet returns the set of all kinds specified by all CRDs in s.
+func (s Status) getCRDKindSet() (sets.String, error) {
+	crdKindSet := sets.NewString()
+	for _, r := range s.Resources {
+		if r.GVK.Kind == "CustomResourceDefinition" {
+			u := &unstructured.Unstructured{}
+			switch v := r.requestObject.(type) {
+			case *unstructured.Unstructured:
+				u = v
+			default:
+				uObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&v)
+				if err != nil {
+					return nil, err
+				}
+				// Other fields are not important, just the CRD kind.
+				u.Object = uObj
+			}
+			// Use unversioned CustomResourceDefinition to avoid implementing cast
+			// for all versions.
+			crd := &apiextensions.CustomResourceDefinition{}
+			err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &crd)
+			if err != nil {
+				return nil, err
+			}
+			crdKindSet.Insert(crd.Spec.Names.Kind)
+		}
+	}
+	return crdKindSet, nil
 }
 
 func (s Status) String() string {
@@ -88,10 +150,10 @@ func (s Status) String() string {
 		nn := r.NamespacedName
 		kind := r.GVK.Kind
 		var status string
-		if r.Resource != nil {
-			status = "Installed"
-		} else if r.Error != nil {
+		if r.Error != nil {
 			status = r.Error.Error()
+		} else if r.Resource != nil {
+			status = "Installed"
 		} else {
 			status = "Unknown"
 		}