Merge pull request #293 from vmware-tanzu/23-informers-supplychains

Replace SupplyChain retries with Informers for Templates

pkg/apis/v1alpha1/cluster_supply_chain.go

@@ -162,8 +162,15 @@ type SupplyChainResource struct {
 	Configs     []ResourceReference      `json:"configs,omitempty"`
 }
 
+var ValidSupplyChainTemplates = []client.Object{
+	&ClusterSourceTemplate{},
+	&ClusterImageTemplate{},
+	&ClusterConfigTemplate{},
+	&ClusterTemplate{},
+}
+
 type ClusterTemplateReference struct {
-	// +kubebuilder:validation:Enum=ClusterSourceTemplate;ClusterImageTemplate;ClusterTemplate;ClusterConfigTemplate
+	//+kubebuilder:validation:Enum=ClusterSourceTemplate;ClusterImageTemplate;ClusterTemplate;ClusterConfigTemplate
 	Kind string `json:"kind"`
 	// +kubebuilder:validation:MinLength=1
 	Name string `json:"name"`

pkg/apis/v1alpha1/cluster_supply_chain_test.go

@@ -23,6 +23,9 @@ import (
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	crdmarkers "sigs.k8s.io/controller-tools/pkg/crd/markers"
+	"sigs.k8s.io/controller-tools/pkg/loader"
+	"sigs.k8s.io/controller-tools/pkg/markers"
 
 	"github.com/vmware-tanzu/cartographer/pkg/apis/v1alpha1"
 )
@@ -492,4 +495,57 @@ var _ = Describe("ClusterSupplyChain", func() {
 			})
 		})
 	})
+
+	Describe("ClusterTemplateReference", func() {
+		It("has four valid references", func() {
+			Expect(v1alpha1.ValidSupplyChainTemplates).To(HaveLen(4))
+
+			Expect(v1alpha1.ValidSupplyChainTemplates).To(ContainElements(
+				&v1alpha1.ClusterSourceTemplate{},
+				&v1alpha1.ClusterConfigTemplate{},
+				&v1alpha1.ClusterImageTemplate{},
+				&v1alpha1.ClusterTemplate{},
+			))
+		})
+
+		It("has a matching valid enum for Kind", func() {
+			// an extension of go parser for loading markers.
+			// Loads the package but only with the contents of the cluster_supply_chain file
+			packages, err := loader.LoadRoots("./cluster_supply_chain.go")
+			Expect(err).NotTo(HaveOccurred())
+			Expect(packages).To(HaveLen(1))
+			Expect(packages[0].GoFiles).To(HaveLen(1))
+
+			// create a registry of CRD markers
+			reg := &markers.Registry{}
+			err = crdmarkers.Register(reg)
+			Expect(err).NotTo(HaveOccurred())
+
+			// and a collector which `EachType` requires
+			coll := &markers.Collector{Registry: reg}
+
+			// find the "kubebuilder:validation:Enum" marker for the ClusterTemplateReference.Kind field
+			var enumMarkers crdmarkers.Enum
+			err = markers.EachType(coll, packages[0], func(info *markers.TypeInfo) {
+				if info.Name == "ClusterTemplateReference" {
+					for _, fieldInfo := range info.Fields {
+						if fieldInfo.Name == "Kind" {
+							var ok bool
+							enumMarkers, ok = fieldInfo.Markers.Get("kubebuilder:validation:Enum").(crdmarkers.Enum)
+							Expect(ok).To(BeTrue())
+						}
+					}
+				}
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			// There should be 4, all found in the ValidSupplyChainTemplates List
+			Expect(enumMarkers).To(HaveLen(len(v1alpha1.ValidSupplyChainTemplates)))
+			for _, validTemplate := range v1alpha1.ValidSupplyChainTemplates {
+				typ := reflect.TypeOf(validTemplate)
+				templateName := typ.Elem().Name()
+				Expect(enumMarkers).To(ContainElement(templateName))
+			}
+		})
+	})
 })

pkg/controller/delivery/reconciler.go

@@ -70,6 +70,7 @@ func (r *Reconciler) reconcileDelivery(delivery *v1alpha1.ClusterDelivery) error
 	var missing []string
 	for _, resource := range delivery.Spec.Resources {
 		_, err := r.repo.GetDeliveryClusterTemplate(resource.TemplateRef)
+		// FIXME: should be checking if err IsNotFound
 		if err != nil {
 			missing = append(missing, resource.Name)
 			r.logger.Error(err, "retrieving cluster template")

pkg/controller/supplychain/reconciler.go

@@ -17,7 +17,6 @@ package supplychain
 import (
 	"context"
 	"fmt"
-	"time"
 
 	"github.com/go-logr/logr"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
@@ -29,8 +28,6 @@ import (
 	"github.com/vmware-tanzu/cartographer/pkg/repository"
 )
 
-const reconcileInterval = 5 * time.Second
-
 type Timer interface {
 	Now() metav1.Time
 }
@@ -98,30 +95,31 @@ func (r *Reconciler) completeReconciliation(ctx context.Context, supplyChain *v1
 		return ctrl.Result{}, err
 	}
 
-	return ctrl.Result{RequeueAfter: reconcileInterval}, nil
+	return ctrl.Result{}, nil
 }
 
 func (r *Reconciler) reconcileSupplyChain(chain *v1alpha1.ClusterSupplyChain) error {
 	var (
-		resourceHandlingError, err error
-		resourcesNotFound          []string
+		err               error
+		resourcesNotFound []string
 	)
 
 	for _, resource := range chain.Spec.Resources {
 		_, err = r.repo.GetClusterTemplate(resource.TemplateRef)
 		if err != nil {
-			resourcesNotFound = append(resourcesNotFound, resource.Name)
-			if resourceHandlingError == nil {
-				resourceHandlingError = fmt.Errorf("handle resource: %w", err)
+			if !kerrors.IsNotFound(err) {
+				return err
 			}
+
+			resourcesNotFound = append(resourcesNotFound, resource.Name)
 		}
 	}
 
-	if resourceHandlingError != nil {
+	if len(resourcesNotFound) > 0 {
 		r.conditionManager.AddPositive(TemplatesNotFoundCondition(resourcesNotFound))
 	} else {
 		r.conditionManager.AddPositive(TemplatesFoundCondition())
 	}
 
-	return resourceHandlingError
+	return nil
 }

pkg/controller/supplychain/reconciler_test.go

@@ -17,7 +17,6 @@ package supplychain_test
 import (
 	"context"
 	"errors"
-	"time"
 
 	"github.com/go-logr/logr"
 	. "github.com/onsi/ginkgo"
@@ -169,32 +168,19 @@ var _ = Describe("Reconciler", func() {
 			Expect(conditionManager.AddPositiveArgsForCall(0)).To(Equal(supplychain.TemplatesFoundCondition()))
 		})
 
-		It("reschedules for 5 seconds", func() {
-			result, _ := reconciler.Reconcile(ctx, req)
-
-			Expect(result).To(Equal(ctrl.Result{RequeueAfter: 5 * time.Second}))
-		})
-
 		It("does not return an error", func() {
 			_, err := reconciler.Reconcile(ctx, req)
 
 			Expect(err).NotTo(HaveOccurred())
 		})
 
-		Context("when retrieving a resource template fails", func() {
+		Context("get cluster template fails", func() {
 			BeforeEach(func() {
-				repo.GetClusterTemplateReturnsOnCall(0, nil, nil)
-				repo.GetClusterTemplateReturnsOnCall(1, nil, errors.New("getting templates is hard"))
-			})
-
-			It("adds a positive templates NOT found condition listing the failed resource", func() {
-				_, _ = reconciler.Reconcile(ctx, req)
-				Expect(conditionManager.AddPositiveArgsForCall(0)).To(Equal(supplychain.TemplatesNotFoundCondition([]string{"second name"})))
+				repo.GetClusterTemplateReturnsOnCall(0, nil, errors.New("getting templates is hard"))
 			})
 
 			It("returns an error", func() {
 				_, err := reconciler.Reconcile(ctx, req)
-				Expect(err).To(MatchError(ContainSubstring("handle resource")))
 				Expect(err).To(MatchError(ContainSubstring("getting templates is hard")))
 			})
 
@@ -203,29 +189,17 @@ var _ = Describe("Reconciler", func() {
 
 				Expect(result).To(Equal(ctrl.Result{Requeue: false}))
 			})
+		})
 
-			Context("when retrieving multiple resource templates fails", func() {
-				BeforeEach(func() {
-					repo.GetClusterTemplateReturnsOnCall(0, nil, errors.New("first error is all that matters"))
-				})
-
-				It("adds a positive templates NOT found condition listing the failed resources", func() {
-					_, _ = reconciler.Reconcile(ctx, req)
-					Expect(conditionManager.AddPositiveArgsForCall(0)).To(Equal(supplychain.TemplatesNotFoundCondition([]string{"first name", "second name"})))
-				})
-
-				It("returns an error", func() {
-					_, err := reconciler.Reconcile(ctx, req)
-					Expect(err).To(MatchError(ContainSubstring("handle resource")))
-					Expect(err).To(MatchError(ContainSubstring("first error is all that matters")))
-					Expect(err).NotTo(MatchError(ContainSubstring("getting templates is hard")))
-				})
-
-				It("does not requeue", func() {
-					result, _ := reconciler.Reconcile(ctx, req)
+		Context("cannot find cluster template", func() {
+			BeforeEach(func() {
+				repo.GetClusterTemplateReturnsOnCall(0, nil, nil)
+				repo.GetClusterTemplateReturnsOnCall(1, nil, kerrors.NewNotFound(schema.GroupResource{}, ""))
+			})
 
-					Expect(result).To(Equal(ctrl.Result{Requeue: false}))
-				})
+			It("adds a positive templates NOT found condition", func() {
+				_, _ = reconciler.Reconcile(ctx, req)
+				Expect(conditionManager.AddPositiveArgsForCall(0)).To(Equal(supplychain.TemplatesNotFoundCondition([]string{"second name"})))
 			})
 		})
 

pkg/registrar/map_functions.go

@@ -14,6 +14,8 @@
 
 package registrar
 
+//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate
+
 import (
 	"context"
 	"fmt"
@@ -25,13 +27,16 @@ import (
 	"github.com/vmware-tanzu/cartographer/pkg/apis/v1alpha1"
 )
 
+//counterfeiter:generate sigs.k8s.io/controller-runtime/pkg/client.Client
+
 //counterfeiter:generate . Logger
 type Logger interface {
 	Error(err error, msg string, keysAndValues ...interface{})
 }
 
 type Mapper struct {
 	Client client.Client
+	// fixme We should accept the context, not the logger - then we get the right logger and so does the client
 	Logger Logger
 }
 
@@ -133,6 +138,66 @@ func (mapper *Mapper) RunTemplateToRunnableRequests(object client.Object) []reco
 	return requests
 }
 
+// addGVK fulfills the 'GVK of an object returned from the APIServer
+// https://github.com/kubernetes-sigs/controller-runtime/issues/1517#issuecomment-844703142
+func (mapper *Mapper) addGVK(obj client.Object) error {
+	gvks, unversioned, err := mapper.Client.Scheme().ObjectKinds(obj)
+	if err != nil {
+		return fmt.Errorf("missing apiVersion or kind: %s err: %w", obj.GetName(), err)
+	}
+
+	if unversioned {
+		return fmt.Errorf("unversioned object: %s", obj.GetName())
+	}
+
+	if len(gvks) != 1 {
+		return fmt.Errorf("unexpected GVK count: %s", obj.GetName())
+	}
+
+	obj.GetObjectKind().SetGroupVersionKind(gvks[0])
+	return nil
+}
+
+func (mapper *Mapper) TemplateToSupplyChainRequests(template client.Object) []reconcile.Request {
+
+	templateName := template.GetName()
+
+	err := mapper.addGVK(template)
+	if err != nil {
+		mapper.Logger.Error(err, fmt.Sprintf("could not get GVK for template: %s", templateName))
+		return nil
+	}
+
+	list := &v1alpha1.ClusterSupplyChainList{}
+
+	err = mapper.Client.List(
+		context.TODO(),
+		list,
+	)
+
+	if err != nil {
+		mapper.Logger.Error(err, "list ClusterSupplyChains")
+		return nil
+	}
+
+	templateKind := template.GetObjectKind().GroupVersionKind().Kind
+
+	var requests []reconcile.Request
+	for _, sc := range list.Items {
+		for _, res := range sc.Spec.Resources {
+			if res.TemplateRef.Kind == templateKind && res.TemplateRef.Name == templateName {
+				requests = append(requests, reconcile.Request{
+					NamespacedName: types.NamespacedName{
+						Name: sc.Name,
+					},
+				})
+			}
+		}
+	}
+
+	return requests
+}
+
 func runTemplateRefMatch(ref v1alpha1.TemplateReference, runTemplate *v1alpha1.ClusterRunTemplate) bool {
 	if ref.Name != runTemplate.Name {
 		return false