Refactor handling of references (#240)

Author: cbarbian-sap

go.mod

@@ -82,7 +82,6 @@ require (
 	github.com/x448/float16 v0.8.4 // indirect
 	github.com/xlab/treeprint v1.2.0 // indirect
 	golang.org/x/crypto v0.31.0 // indirect
-	golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect
 	golang.org/x/net v0.33.0 // indirect
 	golang.org/x/oauth2 v0.24.0 // indirect
 	golang.org/x/sync v0.10.0 // indirect

go.sum

@@ -6,12 +6,10 @@ SPDX-License-Identifier: Apache-2.0
 package component
 
 import (
-	"encoding/json"
 	"fmt"
 	"reflect"
 	"time"
 
-	"github.com/sap/component-operator-runtime/internal/walk"
 	"github.com/sap/component-operator-runtime/pkg/reconciler"
 )
 
@@ -120,41 +118,6 @@ func assertTypeConfiguration[T Component](component T) (TypeConfiguration, bool)
 	return nil, false
 }
 
-// Calculate digest of given component, honoring annotations, spec, and references.
-func calculateComponentDigest[T Component](component T) string {
-	digestData := make(map[string]any)
-	spec := getSpec(component)
-	digestData["annotations"] = component.GetAnnotations()
-	digestData["spec"] = spec
-	if err := walk.Walk(getSpec(component), func(x any, path []string, _ reflect.StructTag) error {
-		// note: this must() is ok because marshalling []string should always work
-		rawPath := must(json.Marshal(path))
-		switch r := x.(type) {
-		case *ConfigMapReference:
-			if r != nil {
-				digestData["refs:"+string(rawPath)] = r.digest()
-			}
-		case *ConfigMapKeyReference:
-			if r != nil {
-				digestData["refs:"+string(rawPath)] = r.digest()
-			}
-		case *SecretReference:
-			if r != nil {
-				digestData["refs:"+string(rawPath)] = r.digest()
-			}
-		case *SecretKeyReference:
-			if r != nil {
-				digestData["refs:"+string(rawPath)] = r.digest()
-			}
-		}
-		return nil
-	}); err != nil {
-		// note: this panic is ok because walk.Walk() only produces errors if the given walker function raises any (which ours here does not do)
-		panic("this cannot happen")
-	}
-	return calculateDigest(digestData)
-}
-
 // Implement the PlacementConfiguration interface.
 func (s *PlacementSpec) GetDeploymentNamespace() string {
 	return s.Namespace

pkg/component/component.go

@@ -84,6 +84,8 @@ const (
 // manager), and the current (potentially unsaved) state of the component.
 // Post-hooks will only be called if the according operation (read, reconcile, delete)
 // has been successful.
+// Note: hooks may change the status of the component, but must not alter the metadata or spec,
+// since changes might be persisted by the framework (e.g. when updating finalizers).
 type HookFunc[T Component] func(ctx context.Context, clnt client.Client, component T) error
 
 // NewClientFunc is the function signature that can be used to modify or replace the default
@@ -181,9 +183,8 @@ func NewReconciler[T Component](name string, resourceGenerator manifests.Generat
 		statusAnalyzer: status.NewStatusAnalyzer(name),
 		options:        options,
 		// TODO: make backoff configurable via options?
-		backoff:       backoff.NewBackoff(10 * time.Second),
-		postReadHooks: []HookFunc[T]{resolveReferences[T]},
-		triggerCh:     make(chan event.TypedGenericEvent[apitypes.NamespacedName], triggerBufferSize),
+		backoff:   backoff.NewBackoff(10 * time.Second),
+		triggerCh: make(chan event.TypedGenericEvent[apitypes.NamespacedName], triggerBufferSize),
 	}
 }
 
@@ -203,7 +204,7 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
 
 	now := metav1.Now()
 
-	// fetch reconciled object
+	// fetch reconciled component
 	component := newComponent[T]()
 	if err := r.client.Get(ctx, req.NamespacedName, component); err != nil {
 		if apierrors.IsNotFound(err) {
@@ -213,6 +214,8 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
 		return ctrl.Result{}, errors.Wrap(err, "unexpected get error")
 	}
 	component.GetObjectKind().SetGroupVersionKind(r.groupVersionKind)
+	// componentDigest is populated after post-read hook phase
+	componentDigest := ""
 
 	// fetch requeue interval, retry interval and timeout
 	requeueInterval := time.Duration(0)
@@ -306,14 +309,24 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
 			}
 		}
 
-		// TODO: should we move this behind the DeepEqual check below to avoid noise?
+		// TODO: should we move this behind the DeepEqual check below to reduce noise?
 		// also note: it seems that no events will be written if the component's namespace is in deletion
 		state, reason, message := status.GetState()
 		var eventAnnotations map[string]string
 		// TODO: formalize this into a real published interface
-		if eventAnnotationProvider, ok := Component(component).(interface{ GetEventAnnotations() map[string]string }); ok {
-			eventAnnotations = eventAnnotationProvider.GetEventAnnotations()
-		}
+		// TODO: pass previousState, and especially componentDigest in a better way;
+		// maybe we could even make the component aware of its own digest ...
+		// another option could be to model this as a hook-like function (instead of a component method) ...
+		// note: the passed component digest might be empty (that is, if we return before the post-read phase)
+		// note: this interface is not released for usage; it may change without announcement
+		if eventAnnotationProvider, ok := Component(component).(interface {
+			GetEventAnnotations(previousState State, componentDigest string) map[string]string
+		}); ok {
+			eventAnnotations = eventAnnotationProvider.GetEventAnnotations(savedStatus.State, componentDigest)
+		}
+		// TODO: sending events may block a little while (some seconds), in particular if enhanced recorders are installed through options.NewClient(),
+		// such as the flux notfication recorder; should we therefore send the events asynchronously, or start synchronously and continue asynchronous
+		// after a little while?
 		if state == StateError {
 			r.client.EventRecorder().AnnotatedEventf(component, eventAnnotations, corev1.EventTypeWarning, reason, message)
 		} else {
@@ -350,6 +363,12 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
 		return ctrl.Result{Requeue: true}, nil
 	}
 
+	// resolve references
+	componentDigest, err = resolveReferences(ctx, r.client, component)
+	if err != nil {
+		return ctrl.Result{}, errors.Wrap(err, "error resolving references")
+	}
+
 	// run post-read hooks
 	// note: it's important that this happens after deferring the status handler
 	// TODO: enhance ctx with tailored logger and event recorder
@@ -400,7 +419,7 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
 				return ctrl.Result{}, errors.Wrapf(err, "error running pre-reconcile hook (%d)", hookOrder)
 			}
 		}
-		ok, digest, err := target.Apply(ctx, component)
+		ok, processingDigest, err := target.Apply(ctx, component, componentDigest)
 		if err != nil {
 			log.V(1).Info("error while reconciling dependent resources")
 			return ctrl.Result{}, errors.Wrap(err, "error reconciling dependent resources")
@@ -418,8 +437,8 @@ func (r *Reconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) (result
 			return ctrl.Result{RequeueAfter: requeueInterval}, nil
 		} else {
 			log.V(1).Info("not all dependent resources successfully reconciled")
-			if digest != status.ProcessingDigest {
-				status.ProcessingDigest = digest
+			if processingDigest != status.ProcessingDigest {
+				status.ProcessingDigest = processingDigest
 				status.ProcessingSince = &now
 				r.backoff.Forget(req)
 			}

pkg/component/reconciler.go

@@ -7,6 +7,7 @@ package component
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"reflect"
 	"strings"
@@ -66,6 +67,7 @@ func (r *ConfigMapReference) load(ctx context.Context, clnt client.Client, names
 
 func (r *ConfigMapReference) digest() string {
 	if !r.loaded {
+		// TODO: shouldn't we panic here?
 		return ""
 	}
 	return calculateDigest(r.data)
@@ -128,6 +130,7 @@ func (r *ConfigMapKeyReference) load(ctx context.Context, clnt client.Client, na
 
 func (r *ConfigMapKeyReference) digest() string {
 	if !r.loaded {
+		// TODO: shouldn't we panic here?
 		return ""
 	}
 	return sha256hex([]byte(r.value))
@@ -172,6 +175,7 @@ func (r *SecretReference) load(ctx context.Context, clnt client.Client, namespac
 
 func (r *SecretReference) digest() string {
 	if !r.loaded {
+		// TODO: shouldn't we panic here?
 		return ""
 	}
 	return calculateDigest(r.data)
@@ -234,6 +238,7 @@ func (r *SecretKeyReference) load(ctx context.Context, clnt client.Client, names
 
 func (r *SecretKeyReference) digest() string {
 	if !r.loaded {
+		// TODO: shouldn't we panic here?
 		return ""
 	}
 	return sha256hex(r.value)
@@ -248,15 +253,26 @@ func (r *SecretKeyReference) Value() []byte {
 	return r.value
 }
 
-func resolveReferences[T Component](ctx context.Context, clnt client.Client, component T) error {
-	return walk.Walk(getSpec(component), func(x any, path []string, tag reflect.StructTag) error {
+func resolveReferences[T Component](ctx context.Context, clnt client.Client, component T) (string, error) {
+	digestData := make(map[string]any)
+	spec := getSpec(component)
+	digestData["generation"] = component.GetGeneration()
+	digestData["annotations"] = component.GetAnnotations()
+	digestData["spec"] = spec
+	if err := walk.Walk(spec, func(x any, path []string, tag reflect.StructTag) error {
+		// note: this must() is ok because marshalling []string should always work
+		rawPath := must(json.Marshal(path))
+		// TODO: allow arbitrary loadable types (with an interface LoadableReference or similar)
 		switch r := x.(type) {
 		case *ConfigMapReference:
 			if r == nil {
 				return nil
 			}
 			ignoreNotFound := !component.GetDeletionTimestamp().IsZero() && tag.Get(tagNotFoundPolicy) == notFoundPolicyIgnoreOnDeletion
-			return r.load(ctx, clnt, component.GetNamespace(), ignoreNotFound)
+			if err := r.load(ctx, clnt, component.GetNamespace(), ignoreNotFound); err != nil {
+				return err
+			}
+			digestData["refs:"+string(rawPath)] = r.digest()
 		case *ConfigMapKeyReference:
 			if r == nil {
 				return nil
@@ -266,13 +282,19 @@ func resolveReferences[T Component](ctx context.Context, clnt client.Client, com
 			if s := tag.Get(tagFallbackKeys); s != "" {
 				fallbackKeys = strings.Split(s, ",")
 			}
-			return r.load(ctx, clnt, component.GetNamespace(), ignoreNotFound, fallbackKeys...)
+			if err := r.load(ctx, clnt, component.GetNamespace(), ignoreNotFound, fallbackKeys...); err != nil {
+				return err
+			}
+			digestData["refs:"+string(rawPath)] = r.digest()
 		case *SecretReference:
 			if r == nil {
 				return nil
 			}
 			ignoreNotFound := !component.GetDeletionTimestamp().IsZero() && tag.Get(tagNotFoundPolicy) == notFoundPolicyIgnoreOnDeletion
-			return r.load(ctx, clnt, component.GetNamespace(), ignoreNotFound)
+			if err := r.load(ctx, clnt, component.GetNamespace(), ignoreNotFound); err != nil {
+				return err
+			}
+			digestData["refs:"+string(rawPath)] = r.digest()
 		case *SecretKeyReference:
 			if r == nil {
 				return nil
@@ -282,8 +304,14 @@ func resolveReferences[T Component](ctx context.Context, clnt client.Client, com
 			if s := tag.Get(tagFallbackKeys); s != "" {
 				fallbackKeys = strings.Split(s, ",")
 			}
-			return r.load(ctx, clnt, component.GetNamespace(), ignoreNotFound, fallbackKeys...)
+			if err := r.load(ctx, clnt, component.GetNamespace(), ignoreNotFound, fallbackKeys...); err != nil {
+				return err
+			}
+			digestData["refs:"+string(rawPath)] = r.digest()
 		}
 		return nil
-	})
+	}); err != nil {
+		return "", err
+	}
+	return calculateDigest(digestData), nil
 }

pkg/component/reference.go

@@ -35,7 +35,7 @@ func newReconcileTarget[T Component](reconcilerName string, reconcilerId string,
 	}
 }
 
-func (t *reconcileTarget[T]) Apply(ctx context.Context, component T) (bool, string, error) {
+func (t *reconcileTarget[T]) Apply(ctx context.Context, component T, componentDigest string) (bool, string, error) {
 	//log := log.FromContext(ctx)
 	namespace := ""
 	name := ""
@@ -51,7 +51,6 @@ func (t *reconcileTarget[T]) Apply(ctx context.Context, component T) (bool, stri
 	}
 	ownerId := t.reconcilerId + "/" + component.GetNamespace() + "/" + component.GetName()
 	status := component.GetStatus()
-	componentDigest := calculateComponentDigest(component)
 
 	// TODO: enhance ctx with local client
 	generateCtx := NewContext(ctx).

pkg/component/target.go

@@ -243,6 +243,8 @@ func NewReconciler(name string, clnt cluster.Client, options ReconcilerOptions)
 // otherwise, if it returns false, the caller should re-call it periodically, until it returns true. In any case, the passed inventory should match the state of the
 // inventory after the previous invocation of Apply(); usually, the caller saves the inventory after calling Apply(), and loads it before calling Apply().
 // The namespace and ownerId arguments should not be changed across subsequent invocations of Apply(); the componentRevision should be incremented only.
+//
+// Also note: it is absolutely crucial that this method returns (true, nil) immediately (on the first call) if everything is already in the right state.
 func (r *Reconciler) Apply(ctx context.Context, inventory *[]*InventoryItem, objects []client.Object, namespace string, ownerId string, componentRevision int64) (bool, error) {
 	var err error
 	log := log.FromContext(ctx)