chore: Duplicate upgrade-related information into resource bindings from RR (#774)

Co-authored-by: Stella Englezou <9325243+senglezou@users.noreply.github.com>
Co-authored-by: Shane Dowling <shane@shanedowling.com>

Author: 3 people

api/v1alpha1/resourcebinding_types.go

@@ -64,6 +64,9 @@ type ResourceBindingStatus struct {
 	// +listMapKey=type
 	// +optional
 	Conditions []metav1.Condition `json:"conditions,omitempty"`
+	// Reference to the Resource Request version
+	// +optional
+	LastAppliedVersion string `json:"lastAppliedVersion,omitempty"`
 }
 
 // +kubebuilder:object:root=true

config/crd/bases/platform.kratix.io_resourcebindings.yaml

@@ -151,6 +151,9 @@ spec:
                 x-kubernetes-list-map-keys:
                 - type
                 x-kubernetes-list-type: map
+              lastAppliedVersion:
+                description: Reference to the Resource Request version
+                type: string
             type: object
         required:
         - spec

internal/controller/dynamic_resource_request_controller.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"maps"
 	"strings"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -61,6 +62,8 @@ const (
 	resourcePromiseVersionStatus      = "promiseVersion"
 	resourceBindingVersionStatus      = "resourceBindingVersion"
 	promiseRevisionLookupFailedReason = "FailedPromiseRevisionLookup"
+	UnversionedPromiseVersion         = "not-set"
+	LatestVersion                     = "latest"
 )
 
 type DynamicResourceRequestController struct {
@@ -79,7 +82,7 @@ type DynamicResourceRequestController struct {
 	NumberOfJobsToKeep          int
 	ReconciliationInterval      time.Duration
 	EventRecorder               record.EventRecorder
-	PromiseUpgrade              bool
+	PromiseUpgradeFeatFlag      bool
 }
 
 //+kubebuilder:rbac:groups="batch",resources=jobs,verbs=get;list;watch;create;update;patch;delete
@@ -169,7 +172,7 @@ func (r *DynamicResourceRequestController) Reconcile(ctx context.Context, req ct
 		promiseRevisionUsed *v1alpha1.PromiseRevision
 		bindingVersion      string
 	)
-	if r.PromiseUpgrade {
+	if r.PromiseUpgradeFeatFlag {
 		logging.Trace(baseLogger,
 			"PromiseUpgrade feature flag set to true; will reconcile with a PromiseRevision.")
 
@@ -205,18 +208,17 @@ func (r *DynamicResourceRequestController) Reconcile(ctx context.Context, req ct
 	if resourceutil.FinalizersAreMissing(
 		rr, []string{workFinalizer, removeAllWorkflowJobsFinalizer, runDeleteWorkflowsFinalizer},
 	) {
-		if err := addFinalizers(opts, rr, r.getRRFinalizers()); err != nil {
-			if apierrors.IsConflict(err) {
-				return fastRequeue, nil
-			}
-			return ctrl.Result{}, err
+		err := addFinalizers(opts, rr, r.getRRFinalizers())
+		if apierrors.IsConflict(err) {
+			return fastRequeue, nil
 		}
-		return ctrl.Result{}, nil
+		return ctrl.Result{}, err
 	}
 
-	if r.PromiseUpgrade {
+	if r.PromiseUpgradeFeatFlag {
 		err := r.updateResourceBinding(ctx, logger, rr, promise)
 		if err != nil {
+			logging.Error(logger, err, "failed to update resource binding for resource request")
 			return ctrl.Result{}, err
 		}
 	}
@@ -290,19 +292,19 @@ func (r *DynamicResourceRequestController) Reconcile(ctx context.Context, req ct
 		return r.nextReconciliation(logger), r.cleanupWorkflowCountersAndExecution(ctx, logger, rr)
 	}
 
-	rrNamespace := ""
-	if promise.WorkflowPipelineNamespaceSet() {
-		rrNamespace = rr.GetNamespace()
-	}
-	workLabels := resourceutil.GetWorkLabels(r.PromiseIdentifier, rr.GetName(), rrNamespace, "", v1alpha1.WorkTypeResource)
-
-	statusUpdate, err := r.generateResourceStatus(ctx, logger, rr, int64(len(pipelineResources)), workLabels, bindingVersion, promiseRevisionUsed)
+	statusUpdated, err := r.ensureResourceStatus(ctx, logger, rr, promise, pipelineResources, bindingVersion, promiseRevisionUsed)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
+	if statusUpdated {
+		return ctrl.Result{}, nil
+	}
 
-	if statusUpdate {
-		return ctrl.Result{}, r.Client.Status().Update(ctx, rr)
+	if r.PromiseUpgradeFeatFlag {
+		if err := r.updateResourceBindingVersionStatus(ctx, logger, promise.GetName(), rr); err != nil {
+			logging.Error(logger, err, "failed to update resource binding version status")
+			return ctrl.Result{}, err
+		}
 	}
 
 	workflowCompletedCondition := resourceutil.GetCondition(rr, resourceutil.ConfigureWorkflowCompletedCondition)
@@ -316,7 +318,7 @@ func (r *DynamicResourceRequestController) Reconcile(ctx context.Context, req ct
 		return r.nextReconciliation(logger), nil
 	}
 
-	if r.PromiseUpgrade {
+	if r.PromiseUpgradeFeatFlag {
 		if r.updatePromiseVersionStatus(logger, rr, bindingVersion, promiseRevisionUsed) {
 			return ctrl.Result{}, r.Client.Status().Update(ctx, rr)
 		}
@@ -325,6 +327,53 @@ func (r *DynamicResourceRequestController) Reconcile(ctx context.Context, req ct
 	return ctrl.Result{}, nil
 }
 
+func (r *DynamicResourceRequestController) ensureResourceStatus(
+	ctx context.Context,
+	logger logr.Logger,
+	rr *unstructured.Unstructured,
+	promise *v1alpha1.Promise,
+	pipelineResources []v1alpha1.PipelineJobResources,
+	bindingVersion string,
+	promiseRevisionUsed *v1alpha1.PromiseRevision,
+) (bool, error) {
+	rrNamespace := ""
+	if promise.WorkflowPipelineNamespaceSet() {
+		rrNamespace = rr.GetNamespace()
+	}
+	workLabels := resourceutil.GetWorkLabels(r.PromiseIdentifier, rr.GetName(), rrNamespace, "", v1alpha1.WorkTypeResource)
+
+	statusUpdate, err := r.generateResourceStatus(ctx, logger, rr, int64(len(pipelineResources)), workLabels, bindingVersion, promiseRevisionUsed)
+	if err != nil {
+		return false, err
+	}
+
+	if statusUpdate {
+		return true, r.Client.Status().Update(ctx, rr)
+	}
+	return false, nil
+}
+
+func (r *DynamicResourceRequestController) updateResourceBindingVersionStatus(ctx context.Context, logger logr.Logger, promiseName string, rr *unstructured.Unstructured) error {
+	resourceBindingName := objectutil.GenerateDeterministicObjectName(fmt.Sprintf("%s-%s", rr.GetName(), promiseName))
+	resourceBinding := &v1alpha1.ResourceBinding{}
+	err := r.Client.Get(ctx, types.NamespacedName{Name: resourceBindingName, Namespace: rr.GetNamespace()}, resourceBinding)
+	if err != nil {
+		return fmt.Errorf("failed to get resourceBinding: %w", err)
+	}
+
+	desiredVersion := resourceutil.GetStatus(rr, resourcePromiseVersionStatus)
+	if resourceBinding.Status.LastAppliedVersion == desiredVersion {
+		return nil
+	}
+
+	logging.Info(logger, "updating resource binding Status.LastAppliedVersion", "oldVersion", resourceBinding.Status.LastAppliedVersion, "newVersion", desiredVersion)
+	resourceBinding.Status.LastAppliedVersion = desiredVersion
+	if err := r.Client.Status().Update(ctx, resourceBinding); err != nil {
+		return fmt.Errorf("failed to update resource binding status: %w", err)
+	}
+	return nil
+}
+
 func (r *DynamicResourceRequestController) updateResourceBinding(ctx context.Context, logger logr.Logger, rr *unstructured.Unstructured, promise *v1alpha1.Promise) error {
 	resourceBinding := &v1alpha1.ResourceBinding{
 		ObjectMeta: metav1.ObjectMeta{
@@ -333,9 +382,20 @@ func (r *DynamicResourceRequestController) updateResourceBinding(ctx context.Con
 		},
 	}
 	op, err := controllerutil.CreateOrUpdate(ctx, r.Client, resourceBinding, func() error {
-		resourceBinding.SetLabels(resourceBindingLabels(rr, promise))
+		// Exclude one-shot trigger labels — propagating them would cause the
+		// ResourceBinding controller to re-trigger reconciliation in an infinite loop.
+		mergedLabels := rr.GetLabels()
+		delete(mergedLabels, resourceutil.ManualReconciliationLabel)
+		delete(mergedLabels, resourceutil.WorkflowRunFromStartLabel)
+		if mergedLabels == nil {
+			mergedLabels = make(map[string]string)
+		}
+		maps.Copy(mergedLabels, resourceBindingLabels(rr, promise))
+		resourceBinding.SetLabels(mergedLabels)
 		if resourceBinding.Spec.Version == "" {
-			resourceBinding.Spec.Version = "latest"
+			resourceBinding.Spec.Version = LatestVersion
+			// if the resource binding got deleted, when we recreate the resource binding we infer what the resource binding
+			// version used to be from the resource request `status.resourceBindingVersion`
 			existingPromiseVersion := resourceutil.GetStatus(rr, resourceBindingVersionStatus)
 			if existingPromiseVersion != "" {
 				resourceBinding.Spec.Version = existingPromiseVersion
@@ -355,6 +415,7 @@ func (r *DynamicResourceRequestController) updateResourceBinding(ctx context.Con
 	}
 
 	logging.Debug(logger, "ResourceBinding reconciled for Resource",
+		"resourceBindingName", resourceBinding.GetName(),
 		"operation", op,
 		"promiseName", resourceBinding.Spec.PromiseRef.Name,
 		"promiseVersion", resourceBinding.Spec.Version,
@@ -559,7 +620,7 @@ func (r *DynamicResourceRequestController) updateReconciledCondition(rr *unstruc
 
 func (r *DynamicResourceRequestController) updatePromiseVersionStatus(logger logr.Logger, rr *unstructured.Unstructured, bindingVersion string, promiseRevision *v1alpha1.PromiseRevision) bool {
 	logging.Trace(logger, "Checking if we need to update the promise version in the status")
-	if !r.PromiseUpgrade || promiseRevision == nil {
+	if !r.PromiseUpgradeFeatFlag || promiseRevision == nil {
 		logging.Trace(logger, "Feature flag disabled or no PromiseRevision: no update promise version required")
 		return false
 	}
@@ -761,7 +822,7 @@ func (r *DynamicResourceRequestController) deleteResources(o opts, promise *v1al
 		return fastRequeue, nil
 	}
 
-	if r.PromiseUpgrade {
+	if r.PromiseUpgradeFeatFlag {
 		if controllerutil.ContainsFinalizer(resourceRequest, resourceBindingFinalizer) {
 			err := r.deleteResourceBinding(o, resourceRequest, promise, resourceBindingFinalizer)
 			if err != nil {
@@ -1041,6 +1102,11 @@ func latestRevision(ctx context.Context, c client.Client, promise *v1alpha1.Prom
 func fetchRevision(ctx context.Context, c client.Client, promise *v1alpha1.Promise,
 	binding *v1alpha1.ResourceBinding, promiseVersionFromRRStatus string) (*v1alpha1.PromiseRevision, error) {
 
+	// there is a scenario where the PromiseVersion from resource request status
+	//  is set, but no resource binding exists, which means the resource binding was
+	// deleted at some point.
+	//
+	// We infer what the resource binding version used to be from the resource request `status.resourceBindingVersion`
 	desiredVersion := promiseVersionFromRRStatus
 
 	if binding != nil {
@@ -1098,7 +1164,7 @@ func (r *DynamicResourceRequestController) setPromiseLabels(ctx context.Context,
 
 func (r *DynamicResourceRequestController) getRRFinalizers() []string {
 	rrFinalizers := []string{workFinalizer, removeAllWorkflowJobsFinalizer, runDeleteWorkflowsFinalizer}
-	if r.PromiseUpgrade {
+	if r.PromiseUpgradeFeatFlag {
 		rrFinalizers = append(rrFinalizers, resourceBindingFinalizer)
 	}
 	return rrFinalizers

internal/controller/dynamic_resource_request_controller_test.go

@@ -1203,7 +1203,7 @@ var _ = Describe("DynamicResourceRequestController", func() {
 	When("promise upgrade feature is on", func() {
 		BeforeEach(func() {
 			Expect(fakeK8sClient.Delete(ctx, resReq)).To(Succeed())
-			reconciler.PromiseUpgrade = true
+			reconciler.PromiseUpgradeFeatFlag = true
 			resReq = createResourceRequest(resourceRequestPath)
 			resReqNameNamespace = client.ObjectKeyFromObject(resReq)
 		})
@@ -1476,6 +1476,124 @@ var _ = Describe("DynamicResourceRequestController", func() {
 			})
 		})
 
+		When("updating the resource binding version status", func() {
+			const promiseVersion = "v1.1.0"
+
+			BeforeEach(func() {
+				createPromiseRevision(fakeK8sClient, promise, promiseVersion)
+			})
+
+			When("the resource binding Status.LastAppliedVersion does not yet reflect the current promise version", func() {
+				It("updates Status.LastAppliedVersion to track the current promise version", func() {
+					setReconcileConfigureWorkflowToReturnFinished()
+					result, err := t.reconcileUntilCompletion(reconciler, resReq)
+					Expect(err).NotTo(HaveOccurred())
+					Expect(result).To(Equal(ctrl.Result{}))
+
+					binding := getResourceBinding(promise.GetName(), resReqNameNamespace)
+					Expect(binding.Spec.Version).To(Equal("latest"))
+					Expect(binding.Status.LastAppliedVersion).To(Equal(promiseVersion))
+				})
+			})
+
+			When("the resource binding is pinned to a specific promise version", func() {
+				BeforeEach(func() {
+					createResourceBinding(fakeK8sClient, promise, resReq, promiseVersion)
+				})
+
+				It("updates Status.LastAppliedVersion to reflect the current promise version", func() {
+					setReconcileConfigureWorkflowToReturnFinished()
+					result, err := t.reconcileUntilCompletion(reconciler, resReq)
+					Expect(err).NotTo(HaveOccurred())
+					Expect(result).To(Equal(ctrl.Result{}))
+
+					binding := getResourceBinding(promise.GetName(), resReqNameNamespace)
+					Expect(binding.Spec.Version).To(Equal(promiseVersion))
+					Expect(binding.Status.LastAppliedVersion).To(Equal(promiseVersion))
+				})
+			})
+		})
+
+		When("creating the resource binding", func() {
+			BeforeEach(func() {
+				createPromiseRevision(fakeK8sClient, promise, "v1.1.0")
+			})
+
+			It("merges the resource request's labels with the binding-specific labels", func() {
+				setReconcileConfigureWorkflowToReturnFinished()
+				result, err := t.reconcileUntilCompletion(reconciler, resReq)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(result).To(Equal(ctrl.Result{}))
+
+				binding := getResourceBinding(promise.GetName(), resReqNameNamespace)
+				// Labels originating from the resource request itself
+				Expect(binding.GetLabels()).To(HaveKeyWithValue("non-kratix-label", "true"))
+				// Binding-specific labels added by resourceBindingLabels
+				Expect(binding.GetLabels()).To(HaveKeyWithValue("kratix.io/promise-name", promise.GetName()))
+				Expect(binding.GetLabels()).To(HaveKeyWithValue("kratix.io/resource-name", resReq.GetName()))
+			})
+
+			It("does not propagate ephemeral trigger labels from the resource request to the binding", func() {
+				Expect(fakeK8sClient.Get(ctx, resReqNameNamespace, resReq)).To(Succeed())
+				labels := resReq.GetLabels()
+				labels[resourceutil.ManualReconciliationLabel] = "true"
+				labels[resourceutil.WorkflowRunFromStartLabel] = "true"
+				resReq.SetLabels(labels)
+				Expect(fakeK8sClient.Update(ctx, resReq)).To(Succeed())
+
+				setReconcileConfigureWorkflowToReturnFinished()
+				result, err := t.reconcileUntilCompletion(reconciler, resReq)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(result).To(Equal(ctrl.Result{}))
+
+				binding := getResourceBinding(promise.GetName(), resReqNameNamespace)
+				Expect(binding.GetLabels()).NotTo(HaveKey(resourceutil.ManualReconciliationLabel))
+				Expect(binding.GetLabels()).NotTo(HaveKey(resourceutil.WorkflowRunFromStartLabel))
+			})
+		})
+
+		When("updating the labels on an existing resource binding", func() {
+			BeforeEach(func() {
+				createPromiseRevision(fakeK8sClient, promise, "v1.1.0")
+				setReconcileConfigureWorkflowToReturnFinished()
+				_, err := t.reconcileUntilCompletion(reconciler, resReq)
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("reflects a changed label from the resource request on the binding", func() {
+				Expect(fakeK8sClient.Get(ctx, resReqNameNamespace, resReq)).To(Succeed())
+				updatedLabels := resReq.GetLabels()
+				updatedLabels["non-kratix-label"] = "updated-value"
+				resReq.SetLabels(updatedLabels)
+				Expect(fakeK8sClient.Update(ctx, resReq)).To(Succeed())
+
+				result, err := t.reconcileUntilCompletion(reconciler, resReq)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(result).To(Equal(ctrl.Result{}))
+
+				binding := getResourceBinding(promise.GetName(), resReqNameNamespace)
+				Expect(binding.GetLabels()).To(HaveKeyWithValue("non-kratix-label", "updated-value"))
+			})
+
+			It("removes a label from the binding when it is removed from the resource request", func() {
+				Expect(fakeK8sClient.Get(ctx, resReqNameNamespace, resReq)).To(Succeed())
+				updatedLabels := resReq.GetLabels()
+				delete(updatedLabels, "non-kratix-label")
+				resReq.SetLabels(updatedLabels)
+				Expect(fakeK8sClient.Update(ctx, resReq)).To(Succeed())
+
+				result, err := t.reconcileUntilCompletion(reconciler, resReq)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(result).To(Equal(ctrl.Result{}))
+
+				binding := getResourceBinding(promise.GetName(), resReqNameNamespace)
+				Expect(binding.GetLabels()).NotTo(HaveKey("non-kratix-label"))
+				// Binding-specific labels should remain untouched
+				Expect(binding.GetLabels()).To(HaveKeyWithValue("kratix.io/promise-name", promise.GetName()))
+				Expect(binding.GetLabels()).To(HaveKeyWithValue("kratix.io/resource-name", resReq.GetName()))
+			})
+		})
+
 	})
 })
 
@@ -1523,7 +1641,7 @@ func createPromiseRevision(fakeK8sClient client.Client, promise *v1alpha1.Promis
 	Expect(fakeK8sClient.Status().Update(ctx, promiseRevision)).To(Succeed())
 }
 
-func createResourceBinding(client client.Client, promise *v1alpha1.Promise, rr *unstructured.Unstructured, version string) *v1alpha1.ResourceBinding {
+func createResourceBinding(client client.Client, promise *v1alpha1.Promise, rr *unstructured.Unstructured, version string) {
 	resourceBinding := &v1alpha1.ResourceBinding{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      objectutil.GenerateDeterministicObjectName(fmt.Sprintf("%s-%s", rr.GetName(), promise.GetName())),
@@ -1545,7 +1663,6 @@ func createResourceBinding(client client.Client, promise *v1alpha1.Promise, rr *
 		},
 	}
 	ExpectWithOffset(1, client.Create(ctx, resourceBinding)).To(Succeed())
-	return resourceBinding
 }
 
 func setConfigureWorkflowStatus(resReq *unstructured.Unstructured, status v1.ConditionStatus, lastTransitionTime ...time.Time) {