Update Repository Resource Status (#3020)

When repository interactions encounter errors, we update status
of the repository with a failure condition. Likewise, on successful
resync of the repository we set a success condition.

Author: martinmaly

porch/api/porchconfig/v1alpha1/config.porch.kpt.dev_repositories.yaml

@@ -29,7 +29,11 @@ spec:
     singular: repository
   scope: Namespaced
   versions:
-  - name: v1alpha1
+  - additionalPrinterColumns:
+    - jsonPath: .status.conditions[?(@.type=='Ready')].status
+      name: Ready
+      type: string
+    name: v1alpha1
     schema:
       openAPIV3Schema:
         description: Repository
@@ -338,6 +342,8 @@ spec:
         type: object
     served: true
     storage: true
+    subresources:
+      status: {}
 status:
   acceptedNames:
     kind: ""

porch/api/porchconfig/v1alpha1/types.go

@@ -19,7 +19,9 @@ import (
 )
 
 //+kubebuilder:object:root=true
+//+kubebuilder:subresource:status
 //+kubebuilder:resource:path=repositories,singular=repository
+//+kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=='Ready')].status`
 
 // Repository
 type Repository struct {
@@ -142,6 +144,16 @@ type FunctionRef struct {
 	Name string `json:"name"`
 }
 
+const (
+	// Type of the Repository condition.
+	RepositoryReady = "Ready"
+
+	// Reason for the condition is error.
+	ReasonError = "Error"
+	// Reason for the condition is the repository is ready.
+	ReasonReady = "Ready"
+)
+
 // RepositoryStatus defines the observed state of Repository
 type RepositoryStatus struct {
 	// Conditions describes the reconciliation state of the object.

porch/apiserver/pkg/e2e/e2e_test.go

@@ -22,6 +22,7 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+	"time"
 
 	kptfilev1 "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1"
 	porchapi "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
@@ -30,6 +31,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	coreapi "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -1034,6 +1036,71 @@ func (t *PorchSuite) TestPodEvaluator(ctx context.Context) {
 	}
 }
 
+func (t *PorchSuite) TestRepositoryError(ctx context.Context) {
+	const (
+		repositoryName = "repo-with-error"
+	)
+	t.CreateF(ctx, &configapi.Repository{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       configapi.RepositoryGVK.Kind,
+			APIVersion: configapi.GroupVersion.Identifier(),
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      repositoryName,
+			Namespace: t.namespace,
+		},
+		Spec: configapi.RepositorySpec{
+			Description: "Repository With Error",
+			Type:        configapi.RepositoryTypeGit,
+			Content:     configapi.RepositoryContentPackage,
+			Git: &configapi.GitRepository{
+				// Use `incalid` domain: https://www.rfc-editor.org/rfc/rfc6761#section-6.4
+				Repo: "https://repo.invalid/repository.git",
+			},
+		},
+	})
+	t.Cleanup(func() {
+		t.DeleteL(ctx, &configapi.Repository{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      repositoryName,
+				Namespace: t.namespace,
+			},
+		})
+	})
+
+	giveUp := time.Now().Add(60 * time.Second)
+
+	for {
+		if time.Now().After(giveUp) {
+			t.Errorf("Timed out waiting for Repository Condition")
+			break
+		}
+
+		time.Sleep(5 * time.Second)
+
+		var repository configapi.Repository
+		t.GetF(ctx, client.ObjectKey{
+			Namespace: t.namespace,
+			Name:      repositoryName,
+		}, &repository)
+
+		available := meta.FindStatusCondition(repository.Status.Conditions, configapi.RepositoryReady)
+		if available == nil {
+			// Condition not yet set
+			t.Logf("Repository condition not yet available")
+			continue
+		}
+
+		if got, want := available.Status, metav1.ConditionFalse; got != want {
+			t.Errorf("Repository Available Condition Status; got %q, want %q", got, want)
+		}
+		if got, want := available.Reason, configapi.ReasonError; got != want {
+			t.Errorf("Repository Available Condition Reason: got %q, want %q", got, want)
+		}
+		break
+	}
+}
+
 func (t *PorchSuite) registerGitRepositoryF(ctx context.Context, repo, name string) {
 	t.CreateF(ctx, &configapi.Repository{
 		TypeMeta: metav1.TypeMeta{

porch/apiserver/pkg/registry/porch/background.go

@@ -19,13 +19,13 @@ import (
 	"fmt"
 	"time"
 
+	configapi "github.com/GoogleContainerTools/kpt/porch/api/porchconfig/v1alpha1"
+	"github.com/GoogleContainerTools/kpt/porch/repository/pkg/cache"
+	"k8s.io/apimachinery/pkg/api/meta"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-
-	configapi "github.com/GoogleContainerTools/kpt/porch/api/porchconfig/v1alpha1"
-	"github.com/GoogleContainerTools/kpt/porch/repository/pkg/cache"
 )
 
 func RunBackground(ctx context.Context, coreClient client.WithWatch, cache *cache.Cache) {
@@ -161,15 +161,38 @@ func (b *background) runOnce(ctx context.Context) error {
 			}
 		}
 
-		b.cacheRepository(ctx, repo)
+		if err := b.cacheRepository(ctx, repo); err != nil {
+			klog.Errorf("Failed to cache repository: %v", err)
+		}
 	}
 
 	return nil
 }
 
 func (b *background) cacheRepository(ctx context.Context, repo *configapi.Repository) error {
-	if _, err := b.cache.OpenRepository(ctx, repo); err != nil {
-		return fmt.Errorf("error opening repository: %w", err)
+	var condition v1.Condition
+	if _, err := b.cache.OpenRepository(ctx, repo); err == nil {
+		condition = v1.Condition{
+			Type:               configapi.RepositoryReady,
+			Status:             v1.ConditionTrue,
+			ObservedGeneration: repo.Generation,
+			LastTransitionTime: v1.Now(),
+			Reason:             configapi.ReasonReady,
+		}
+	} else {
+		condition = v1.Condition{
+			Type:               configapi.RepositoryReady,
+			Status:             v1.ConditionFalse,
+			ObservedGeneration: repo.Generation,
+			LastTransitionTime: v1.Now(),
+			Reason:             configapi.ReasonError,
+			Message:            err.Error(),
+		}
+	}
+
+	meta.SetStatusCondition(&repo.Status.Conditions, condition)
+	if err := b.coreClient.Status().Update(ctx, repo); err != nil {
+		return fmt.Errorf("error updating repository status: %w", err)
 	}
 	return nil
 }

porch/config/deploy/5-rbac.yaml

@@ -25,7 +25,7 @@ rules:
       ["mutatingwebhookconfigurations", "validatingwebhookconfigurations"]
     verbs: ["get", "watch", "list"]
   - apiGroups: ["config.porch.kpt.dev"]
-    resources: ["repositories"]
+    resources: ["repositories", "repositories/status"]
     verbs: ["get", "list", "watch", "create", "update", "patch"]
   # Needed for priority and fairness
   - apiGroups: ["flowcontrol.apiserver.k8s.io"]

porch/repository/pkg/cache/cache.go

@@ -109,6 +109,11 @@ func (c *Cache) OpenRepository(ctx context.Context, repositorySpec *configapi.Re
 				cr = newRepository(key, r)
 				c.repositories[key] = cr
 			}
+		} else {
+			// If there is an error from the background refresh goroutine, return it.
+			if err := cr.getRefreshError(); err != nil {
+				return nil, err
+			}
 		}
 		return cr, nil
 

porch/repository/pkg/cache/repository.go

@@ -38,6 +38,9 @@ type cachedRepository struct {
 	cachedPackages []*cachedPackageRevision
 	// TODO: Currently we support repositories with homogenous content (only packages xor functions). Model this more optimally?
 	cachedFunctions []repository.Function
+	// Error encountered on repository refresh by the refresh goroutine.
+	// This is returned back by the cache to the background goroutine when it calls periodicall to resync repositories.
+	refreshError error
 }
 
 // We take advantage of the cache having a global view of all the packages
@@ -98,9 +101,16 @@ func (r *cachedRepository) ListFunctions(ctx context.Context) ([]repository.Func
 	return functions, nil
 }
 
+func (r *cachedRepository) getRefreshError() error {
+	r.mutex.Lock()
+	defer r.mutex.Unlock()
+	return r.refreshError
+}
+
 func (r *cachedRepository) getPackages(ctx context.Context, forceRefresh bool) ([]repository.PackageRevision, error) {
 	r.mutex.Lock()
 	packages := r.cachedPackages
+	err := r.refreshError
 	r.mutex.Unlock()
 
 	if forceRefresh {
@@ -109,17 +119,22 @@ func (r *cachedRepository) getPackages(ctx context.Context, forceRefresh bool) (
 
 	if packages == nil {
 		// TODO: Avoid simultaneous fetches?
-		p, err := r.repo.ListPackageRevisions(ctx)
-		if err != nil {
-			return nil, err
+		var p []repository.PackageRevision
+		p, err = r.repo.ListPackageRevisions(ctx)
+		if err == nil {
+			packages = toCachedPackageRevisionSlice(p)
 		}
-		packages = toCachedPackageRevisionSlice(p)
 
 		r.mutex.Lock()
 		r.cachedPackages = packages
+		r.refreshError = err
 		r.mutex.Unlock()
 	}
 
+	if err != nil {
+		return nil, err
+	}
+
 	return toPackageRevisionSlice(packages), nil
 }