Fix create/update bug (#493)

* Workload/deliverable use Get instead of List (by labels) to find existing objects * Runnable uses List by `carto.run/runnable-name` label only * Developer service account now requires Get permission Co-authored-by: Emily Johnson <[email protected]>
vmware-tanzu · Dec 17, 2021 · ec82275 · ec82275
1 parent 68b3056
commit ec82275
Show file tree

Hide file tree

Showing 13 changed files with 856 additions and 288 deletions.
diff --git a/examples/source-to-knative-service/developer/serviceaccount.yaml b/examples/source-to-knative-service/developer/serviceaccount.yaml
@@ -62,3 +62,4 @@ rules:
       - delete
       - patch
       - watch
+      - get
diff --git a/pkg/realizer/deliverable/component.go b/pkg/realizer/deliverable/component.go
@@ -115,7 +115,7 @@ func (r *resourceRealizer) Do(ctx context.Context, resource *v1alpha1.DeliveryRe
 		}
 	}
 
-	err = r.deliverableRepo.EnsureObjectExistsOnCluster(ctx, stampedObject, true)
+	err = r.deliverableRepo.EnsureMutableObjectExistsOnCluster(ctx, stampedObject)
 	if err != nil {
 		log.Error(err, "failed to ensure object exists on cluster", "object", stampedObject)
 		return nil, nil, ApplyStampedObjectError{

diff --git a/pkg/realizer/deliverable/component_test.go b/pkg/realizer/deliverable/component_test.go
@@ -159,17 +159,16 @@ var _ = Describe("Resource", func() {
 				}
 
 				fakeSystemRepo.GetDeliveryTemplateReturns(templateAPI, nil)
-				fakeDeliverableRepo.EnsureObjectExistsOnClusterReturns(nil)
+				fakeDeliverableRepo.EnsureMutableObjectExistsOnClusterReturns(nil)
 			})
 
 			It("creates a stamped object and returns the outputs and stampedObjects", func() {
 				returnedStampedObject, out, err := r.Do(ctx, &resource, deliveryName, outputs)
 				Expect(err).ToNot(HaveOccurred())
 
-				_, stampedObject, allowUpdate := fakeDeliverableRepo.EnsureObjectExistsOnClusterArgsForCall(0)
+				_, stampedObject := fakeDeliverableRepo.EnsureMutableObjectExistsOnClusterArgsForCall(0)
 
 				Expect(returnedStampedObject).To(Equal(stampedObject))
-				Expect(allowUpdate).To(BeTrue())
 
 				metadata := stampedObject.Object["metadata"]
 				metadataValues, ok := metadata.(map[string]interface{})
@@ -186,6 +185,7 @@ var _ = Describe("Resource", func() {
 						"blockOwnerDeletion": true,
 					},
 				}))
+				Expect(stampedObject.Object["data"]).To(Equal(map[string]interface{}{"player_current_lives": "some-url", "some_other_info": "some-revision"}))
 				Expect(metadataValues["labels"]).To(Equal(map[string]interface{}{
 					"carto.run/cluster-delivery-name": "delivery-name",
 					"carto.run/resource-name":         "resource-1",
@@ -194,7 +194,6 @@ var _ = Describe("Resource", func() {
 					"carto.run/deliverable-namespace": "",
 					"carto.run/template-kind":         "ClusterSourceTemplate",
 				}))
-				Expect(stampedObject.Object["data"]).To(Equal(map[string]interface{}{"player_current_lives": "some-url", "some_other_info": "some-revision"}))
 
 				Expect(out.Source.Revision).To(Equal("some-revision"))
 				Expect(out.Source.URL).To(Equal("some-url"))
@@ -305,7 +304,7 @@ var _ = Describe("Resource", func() {
 				}
 
 				fakeSystemRepo.GetDeliveryTemplateReturns(templateAPI, nil)
-				fakeDeliverableRepo.EnsureObjectExistsOnClusterReturns(nil)
+				fakeDeliverableRepo.EnsureMutableObjectExistsOnClusterReturns(nil)
 			})
 
 			It("returns RetrieveOutputError", func() {
@@ -316,7 +315,7 @@ var _ = Describe("Resource", func() {
 			})
 		})
 
-		When("unable to EnsureObjectExistsOnCluster the stamped object", func() {
+		When("unable to EnsureImmutableObjectExistsOnCluster the stamped object", func() {
 			BeforeEach(func() {
 				resource.Sources = []v1alpha1.ResourceReference{
 					{
@@ -366,7 +365,7 @@ var _ = Describe("Resource", func() {
 				}
 
 				fakeSystemRepo.GetDeliveryTemplateReturns(templateAPI, nil)
-				fakeDeliverableRepo.EnsureObjectExistsOnClusterReturns(errors.New("bad object"))
+				fakeDeliverableRepo.EnsureMutableObjectExistsOnClusterReturns(errors.New("bad object"))
 			})
 
 			It("returns ApplyStampedObjectError", func() {

diff --git a/pkg/realizer/runnable/realizer.go b/pkg/realizer/runnable/realizer.go
@@ -96,7 +96,7 @@ func (p *runnableRealizer) Realize(ctx context.Context, runnable *v1alpha1.Runna
 	}
 
 	// FIXME: why are we taking a DeepCopy?
-	err = runnableRepo.EnsureObjectExistsOnCluster(ctx, stampedObject.DeepCopy(), false)
+	err = runnableRepo.EnsureImmutableObjectExistsOnCluster(ctx, stampedObject.DeepCopy(), map[string]string{"carto.run/runnable-name": runnable.Name})
 	if err != nil {
 		log.Error(err, "failed to ensure object exists on cluster", "object", stampedObject)
 		return nil, nil, ApplyStampedObjectError{
@@ -105,16 +105,13 @@ func (p *runnableRealizer) Realize(ctx context.Context, runnable *v1alpha1.Runna
 		}
 	}
 
-	objectForListCall := stampedObject.DeepCopy()
-	objectForListCall.SetLabels(labels)
-
-	allRunnableStampedObjects, err := runnableRepo.ListUnstructured(ctx, objectForListCall)
+	allRunnableStampedObjects, err := runnableRepo.ListUnstructured(ctx, stampedObject.GroupVersionKind(), stampedObject.GetNamespace(), labels)
 	if err != nil {
 		log.Error(err, "failed to list objects")
 		return stampedObject, nil, ListCreatedObjectsError{
 			Err:       err,
-			Namespace: objectForListCall.GetNamespace(),
-			Labels:    objectForListCall.GetLabels(),
+			Namespace: stampedObject.GetNamespace(),
+			Labels:    labels,
 		}
 	}
 
@@ -146,12 +143,8 @@ func resolveSelector(ctx context.Context, selector *v1alpha1.ResourceSelector, r
 	if selector == nil {
 		return nil, nil
 	}
-	queryObj := &unstructured.Unstructured{}
-	queryObj.SetGroupVersionKind(schema.FromAPIVersionAndKind(selector.Resource.APIVersion, selector.Resource.Kind))
-	queryObj.SetLabels(selector.MatchingLabels)
-	queryObj.SetNamespace(namespace)
 
-	results, err := repository.ListUnstructured(ctx, queryObj)
+	results, err := repository.ListUnstructured(ctx, schema.FromAPIVersionAndKind(selector.Resource.APIVersion, selector.Resource.Kind), namespace, selector.MatchingLabels)
 	if err != nil {
 		log.Error(err, "failed to list objects matching selector", "selector", selector.MatchingLabels)
 		return nil, fmt.Errorf("failed to list objects matching selector [%+v]: %w", selector.MatchingLabels, err)
@@ -174,7 +167,7 @@ func resolveClusterScopedSelector(ctx context.Context, selector *v1alpha1.Resour
 	queryObj.SetGroupVersionKind(schema.FromAPIVersionAndKind(selector.Resource.APIVersion, selector.Resource.Kind))
 	queryObj.SetLabels(selector.MatchingLabels)
 
-	results, err := repository.ListUnstructured(ctx, queryObj)
+	results, err := repository.ListUnstructured(ctx, schema.FromAPIVersionAndKind(selector.Resource.APIVersion, selector.Resource.Kind), "", selector.MatchingLabels)
 	if err != nil {
 		log.Error(err, "failed to list objects matching selector", "selector", selector.MatchingLabels)
 		return nil, fmt.Errorf("failed to list objects matching selector [%+v]: %w", selector.MatchingLabels, err)

diff --git a/pkg/realizer/runnable/realizer_test.go b/pkg/realizer/runnable/realizer_test.go
@@ -108,7 +108,7 @@ var _ = Describe("Realizer", func() {
 
 			createdUnstructured = &unstructured.Unstructured{}
 
-			runnableRepo.EnsureObjectExistsOnClusterStub = func(ctx context.Context, obj *unstructured.Unstructured, allowUpdate bool) error {
+			runnableRepo.EnsureImmutableObjectExistsOnClusterStub = func(ctx context.Context, obj *unstructured.Unstructured, labels map[string]string) error {
 				createdUnstructured.Object = obj.Object
 				return nil
 			}
@@ -128,9 +128,8 @@ var _ = Describe("Realizer", func() {
 				},
 			))
 
-			Expect(runnableRepo.EnsureObjectExistsOnClusterCallCount()).To(Equal(1))
-			_, stamped, allowUpdate := runnableRepo.EnsureObjectExistsOnClusterArgsForCall(0)
-			Expect(allowUpdate).To(BeFalse())
+			Expect(runnableRepo.EnsureImmutableObjectExistsOnClusterCallCount()).To(Equal(1))
+			_, stamped, labels := runnableRepo.EnsureImmutableObjectExistsOnClusterArgsForCall(0)
 			Expect(stamped.Object).To(
 				MatchKeys(IgnoreExtras, Keys{
 					"metadata": MatchKeys(IgnoreExtras, Keys{
@@ -143,6 +142,9 @@ var _ = Describe("Realizer", func() {
 					}),
 				}),
 			)
+			Expect(labels).To(Equal(map[string]string{
+				"carto.run/runnable-name": "my-runnable",
+			}))
 		})
 
 		It("does not return an error", func() {
@@ -165,9 +167,9 @@ var _ = Describe("Realizer", func() {
 			Expect(stampedObject.Object["kind"]).To(Equal("TestObj"))
 		})
 
-		Context("error on EnsureObjectExistsOnCluster", func() {
+		Context("error on EnsureImmutableObjectExistsOnCluster", func() {
 			BeforeEach(func() {
-				runnableRepo.EnsureObjectExistsOnClusterReturns(errors.New("some bad error"))
+				runnableRepo.EnsureImmutableObjectExistsOnClusterReturns(errors.New("some bad error"))
 			})
 
 			It("returns ApplyStampedObjectError", func() {
@@ -207,15 +209,15 @@ var _ = Describe("Realizer", func() {
 				_, _, _ = rlzr.Realize(ctx, runnable, systemRepo, runnableRepo)
 
 				Expect(runnableRepo.ListUnstructuredCallCount()).To(Equal(2))
-				_, clientQueryObjectForSelector := runnableRepo.ListUnstructuredArgsForCall(0)
+				_, gvk, namespace, labels := runnableRepo.ListUnstructuredArgsForCall(0)
 
-				Expect(clientQueryObjectForSelector.GetAPIVersion()).To(Equal("apiversion-to-be-selected"))
-				Expect(clientQueryObjectForSelector.GetKind()).To(Equal("kind-to-be-selected"))
-				Expect(clientQueryObjectForSelector.GetLabels()).To(Equal(map[string]string{"expected-label": "expected-value"}))
+				Expect(gvk.Version).To(Equal("apiversion-to-be-selected"))
+				Expect(gvk.Kind).To(Equal("kind-to-be-selected"))
+				Expect(labels).To(Equal(map[string]string{"expected-label": "expected-value"}))
+				Expect(namespace).To(Equal("my-important-ns"))
 
-				Expect(runnableRepo.EnsureObjectExistsOnClusterCallCount()).To(Equal(1))
-				_, stamped, allowUpdate := runnableRepo.EnsureObjectExistsOnClusterArgsForCall(0)
-				Expect(allowUpdate).To(BeFalse())
+				Expect(runnableRepo.EnsureImmutableObjectExistsOnClusterCallCount()).To(Equal(1))
+				_, stamped, labels := runnableRepo.EnsureImmutableObjectExistsOnClusterArgsForCall(0)
 				Expect(stamped.Object).To(
 					MatchKeys(IgnoreExtras, Keys{
 						"metadata": MatchKeys(IgnoreExtras, Keys{
@@ -230,6 +232,9 @@ var _ = Describe("Realizer", func() {
 						}),
 					}),
 				)
+				Expect(labels).To(Equal(map[string]string{
+					"carto.run/runnable-name": "my-runnable",
+				}))
 			})
 		})
 
@@ -317,7 +322,7 @@ var _ = Describe("Realizer", func() {
 
 			createdUnstructured = &unstructured.Unstructured{}
 
-			runnableRepo.EnsureObjectExistsOnClusterStub = func(ctx context.Context, obj *unstructured.Unstructured, allowUpdate bool) error {
+			runnableRepo.EnsureImmutableObjectExistsOnClusterStub = func(ctx context.Context, obj *unstructured.Unstructured, labels map[string]string) error {
 				createdUnstructured.Object = obj.Object
 				return nil
 			}

diff --git a/pkg/realizer/workload/component.go b/pkg/realizer/workload/component.go
@@ -122,7 +122,7 @@ func (r *resourceRealizer) Do(ctx context.Context, resource *v1alpha1.SupplyChai
 		}
 	}
 
-	err = r.workloadRepo.EnsureObjectExistsOnCluster(ctx, stampedObject, true)
+	err = r.workloadRepo.EnsureMutableObjectExistsOnCluster(ctx, stampedObject)
 	if err != nil {
 		log.Error(err, "failed to ensure object exists on cluster", "object", stampedObject)
 		return nil, nil, ApplyStampedObjectError{

diff --git a/pkg/realizer/workload/component_test.go b/pkg/realizer/workload/component_test.go
@@ -160,16 +160,15 @@ var _ = Describe("Resource", func() {
 				}
 
 				fakeSystemRepo.GetSupplyChainTemplateReturns(templateAPI, nil)
-				fakeWorkloadRepo.EnsureObjectExistsOnClusterReturns(nil)
+				fakeWorkloadRepo.EnsureMutableObjectExistsOnClusterReturns(nil)
 			})
 
 			It("creates a stamped object using the workload repository and returns the outputs and stampedObjects", func() {
 				returnedStampedObject, out, err := r.Do(ctx, &resource, supplyChainName, outputs)
 				Expect(err).ToNot(HaveOccurred())
 
-				_, stampedObject, allowUpdate := fakeWorkloadRepo.EnsureObjectExistsOnClusterArgsForCall(0)
+				_, stampedObject := fakeWorkloadRepo.EnsureMutableObjectExistsOnClusterArgsForCall(0)
 				Expect(returnedStampedObject).To(Equal(stampedObject))
-				Expect(allowUpdate).To(BeTrue())
 
 				metadata := stampedObject.Object["metadata"]
 				metadataValues, ok := metadata.(map[string]interface{})
@@ -186,6 +185,7 @@ var _ = Describe("Resource", func() {
 						"blockOwnerDeletion": true,
 					},
 				}))
+				Expect(stampedObject.Object["data"]).To(Equal(map[string]interface{}{"player_current_lives": "some-url", "some_other_info": "some-revision"}))
 				Expect(metadataValues["labels"]).To(Equal(map[string]interface{}{
 					"carto.run/cluster-supply-chain-name": "supply-chain-name",
 					"carto.run/resource-name":             "resource-1",
@@ -194,7 +194,6 @@ var _ = Describe("Resource", func() {
 					"carto.run/workload-namespace":        "",
 					"carto.run/template-kind":             "ClusterImageTemplate",
 				}))
-				Expect(stampedObject.Object["data"]).To(Equal(map[string]interface{}{"player_current_lives": "some-url", "some_other_info": "some-revision"}))
 
 				Expect(out.Image).To(Equal("some-revision"))
 			})
@@ -304,7 +303,7 @@ var _ = Describe("Resource", func() {
 				}
 
 				fakeSystemRepo.GetSupplyChainTemplateReturns(templateAPI, nil)
-				fakeWorkloadRepo.EnsureObjectExistsOnClusterReturns(nil)
+				fakeWorkloadRepo.EnsureMutableObjectExistsOnClusterReturns(nil)
 			})
 
 			It("returns RetrieveOutputError", func() {
@@ -315,7 +314,7 @@ var _ = Describe("Resource", func() {
 			})
 		})
 
-		When("unable to EnsureObjectExistsOnCluster the stamped object", func() {
+		When("unable to EnsureImmutableObjectExistsOnCluster the stamped object", func() {
 			BeforeEach(func() {
 				resource.Sources = []v1alpha1.ResourceReference{
 					{
@@ -365,7 +364,7 @@ var _ = Describe("Resource", func() {
 				}
 
 				fakeSystemRepo.GetSupplyChainTemplateReturns(templateAPI, nil)
-				fakeWorkloadRepo.EnsureObjectExistsOnClusterReturns(errors.New("bad object"))
+				fakeWorkloadRepo.EnsureMutableObjectExistsOnClusterReturns(errors.New("bad object"))
 			})
 			It("returns ApplyStampedObjectError", func() {
 				_, _, err := r.Do(ctx, &resource, supplyChainName, outputs)
-Original file line number
+Diff line change
@@ Expand Up / @@ -62,3 +62,4 @@ rules: @@
           - delete
           - patch
           - watch
+          - get