Merge pull request #812 from bschaatsbergen/fix/dont-apply-externalrefs

fix: exclude externalRef nodes from applySet to prevent empty CRs

Author: k8s-ci-robot

pkg/applyset/applyset.go

@@ -554,24 +554,31 @@ func (a *applySet) apply(ctx context.Context, dryRun bool) (*ApplyResult, error)
 		if err != nil {
 			return results, err
 		}
+
+		// Apply resources using server-side apply
 		eg.Go(func() error {
-			lastApplied, err := dynResource.Apply(egctx, obj.GetName(), obj.Unstructured, options)
+			lastApplied, err := a.applyResource(egctx, dynResource, obj, options)
 			mu.Lock()
 			defer mu.Unlock()
 			results.recordApplied(obj, lastApplied, err)
-			var appliedRevision string
-			if lastApplied != nil {
-				appliedRevision = lastApplied.GetResourceVersion()
-			}
-			a.log.V(2).Info("applied object", "object", obj.String(), "applied-revision", appliedRevision,
-				"error", err)
 			return nil
 		})
 	}
 
 	return results, eg.Wait()
 }
 
+// applyResource applies a resource to the cluster using server-side apply.
+func (a *applySet) applyResource(ctx context.Context, dynResource dynamic.ResourceInterface, obj ApplyableObject, options metav1.ApplyOptions) (*unstructured.Unstructured, error) {
+	resource, err := dynResource.Apply(ctx, obj.GetName(), obj.Unstructured, options)
+	var appliedRevision string
+	if resource != nil {
+		appliedRevision = resource.GetResourceVersion()
+	}
+	a.log.V(2).Info("applied resource", "object", obj.String(), "applied-revision", appliedRevision, "error", err)
+	return resource, err
+}
+
 func (a *applySet) prune(ctx context.Context, results *ApplyResult, dryRun bool) (*ApplyResult, error) {
 	pruneObjects, err := a.findAllObjectsToPrune(ctx, a.dynamicClient, results.AppliedUIDs())
 	if err != nil {

pkg/applyset/tracker.go

@@ -50,10 +50,6 @@ type ApplyableObject struct {
 	// It is opaque and is passed in the callbacks as is
 	ID string
 
-	// Lifecycle hints
-	// TODO(barney-s): need to exapnd on these: https://github.com/kubernetes-sigs/kro/issues/542
-	ExternalRef bool
-
 	// lastReadRevision is the revision of the object that was last read from the cluster.
 	lastReadRevision string
 }

pkg/controller/instance/controller_reconcile.go

@@ -98,6 +98,8 @@ type instanceGraphReconciler struct {
 // It manages the full lifecycle of the instance including creation, updates,
 // and deletion.
 func (igr *instanceGraphReconciler) reconcile(ctx context.Context) error {
+	igr.log.V(2).Info("reconciling instance")
+
 	igr.state = newInstanceState()
 
 	// Create runtime - if this fails, the defer in Controller.Reconcile handles status
@@ -225,10 +227,28 @@ func (igr *instanceGraphReconciler) reconcileInstance(ctx context.Context) error
 			break
 		}
 
+		// ExternalRefs are read-only - fetch them directly instead of adding to applyset
+		if igr.runtime.ResourceDescriptor(resourceID).IsExternalRef() {
+			clusterObj, err := igr.readExternalRef(ctx, resourceID, resource)
+			if err != nil {
+				resourceState.State = ResourceStateError
+				resourceState.Err = fmt.Errorf("failed to read external ref: %w", err)
+				break
+			}
+			igr.runtime.SetResource(resourceID, clusterObj)
+			igr.updateResourceReadiness(resourceID)
+			// Synchronize runtime state after each resource to re-evaluate CEL expressions
+			if _, err := igr.runtime.Synchronize(); err != nil {
+				return fmt.Errorf("failed to synchronize after reading external ref: %w", err)
+			}
+			resourceState.State = ResourceStateSynced
+			continue
+		}
+
+		// Regular resources go through the applyset
 		applyable := applyset.ApplyableObject{
 			Unstructured: resource,
 			ID:           resourceID,
-			ExternalRef:  igr.runtime.ResourceDescriptor(resourceID).IsExternalRef(),
 		}
 		clusterObj, err := aset.Add(ctx, applyable)
 		if err != nil {
@@ -273,7 +293,7 @@ func (igr *instanceGraphReconciler) reconcileInstance(ctx context.Context) error
 
 	if err := result.Errors(); err != nil {
 		mark.ResourcesNotReady("there was an error while reconciling resources in the apply set: %v", err)
-		return fmt.Errorf("failed to apply/prune resources: %w", err)
+		return igr.delayedRequeue(fmt.Errorf("failed to apply/prune resources: %w", err))
 	}
 
 	if unresolvedResourceID != "" {
@@ -548,6 +568,32 @@ func (igr *instanceGraphReconciler) delayedRequeue(err error) error {
 	return requeue.NeededAfter(err, igr.reconcileConfig.DefaultRequeueDuration)
 }
 
+// readExternalRef fetches an external reference from the cluster.
+// External references are resources that exist outside of this instance's control.
+func (igr *instanceGraphReconciler) readExternalRef(ctx context.Context, resourceID string, resource *unstructured.Unstructured) (*unstructured.Unstructured, error) {
+	gvk := resource.GroupVersionKind()
+	restMapping, err := igr.restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get REST mapping for %v: %w", gvk, err)
+	}
+
+	var dynResource dynamic.ResourceInterface
+	if restMapping.Scope.Name() == meta.RESTScopeNameNamespace {
+		namespace := igr.getResourceNamespace(resourceID)
+		dynResource = igr.client.Resource(restMapping.Resource).Namespace(namespace)
+	} else {
+		dynResource = igr.client.Resource(restMapping.Resource)
+	}
+
+	clusterObj, err := dynResource.Get(ctx, resource.GetName(), metav1.GetOptions{})
+	if err != nil {
+		return nil, fmt.Errorf("failed to get external ref %s/%s: %w", resource.GetNamespace(), resource.GetName(), err)
+	}
+
+	igr.log.V(2).Info("read external ref", "gvk", gvk, "namespace", resource.GetNamespace(), "name", resource.GetName())
+	return clusterObj, nil
+}
+
 // getResourceNamespace determines the appropriate namespace for a resource.
 // It follows this precedence order:
 // 1. Resource's explicitly specified namespace

pkg/dynamiccontroller/dynamic_controller.go

@@ -270,7 +270,7 @@ func (dc *DynamicController) processNextWorkItem(ctx context.Context) bool {
 	}
 
 	err := dc.syncFunc(ctx, item, handler.(Handler))
-	if err == nil || apierrors.IsNotFound(err) {
+	if err == nil {
 		dc.queue.Forget(item)
 		return true
 	}
@@ -291,6 +291,12 @@ func (dc *DynamicController) processNextWorkItem(ctx context.Context) bool {
 		requeueTotal.WithLabelValues(gvrKey, "requeue_after").Inc()
 		dc.queue.AddAfter(item, typedErr.Duration())
 	default:
+		// we only check here for this not found error here because we want explicit requeue signals to have priority
+		if apierrors.IsNotFound(err) {
+			dc.log.V(1).Info("item no longer exists, dropping from queue", "item", item)
+			dc.queue.Forget(item)
+			return true
+		}
 		requeueTotal.WithLabelValues(gvrKey, "rate_limited").Inc()
 		if dc.queue.NumRequeues(item) < dc.config.QueueMaxRetries {
 			dc.log.Error(err, "Error syncing item, requeuing with rate limit", "item", item)

test/e2e/chainsaw/check-external-references/chainsaw-test.yaml

@@ -0,0 +1,62 @@
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: Test
+metadata:
+  name: check-external-references
+spec:
+  description: | 
+    Tests if a ResourceGraphDefinition creates an Instance CRD
+  steps:
+  - name: setup
+    try:
+    - apply:
+        file: rgd.yaml
+      description: Apply RGD
+    - assert:
+        file: rgd-assert.yaml
+      description: Verify RGD state
+    - apply:
+        file: instance.yaml
+      description: Apply ExternalReferenceTest Instance
+    finally:
+    - description: sleep operation to let instance settle
+      sleep:
+        duration: 3s
+    catch:
+    - description: kro controller pod logs collector
+      podLogs:
+        selector: app.kubernetes.io/instance=kro
+        namespace: kro-system
+
+  - name: check-instance-state
+    try:
+    - assert:
+        file: instance-assert-in-progress.yaml
+      description: Verify Instance State is in progress (waiting for external resource)
+    - error:
+        file: configmap.yaml
+      description: Verify ConfigMap does not exist
+    - error:
+        file: owned.yaml
+      description: Verify Owned Resource does not exist
+    catch:
+    - description: kro controller pod logs collector
+      podLogs:
+        selector: app.kubernetes.io/instance=kro
+        namespace: kro-system
+
+  - name: ensure-external-ref
+    try:
+    - apply:
+        file: configmap.yaml
+      description: Apply External Reference ConfigMap
+    - assert:
+        file: instance-assert.yaml
+      description: Verify Instance becomes Ready after Reference is fulfilled
+    - assert:
+        file: owned.yaml
+      description: Verify Owned Resource based on External Reference is created
+    catch:
+    - description: kro controller pod logs collector
+      podLogs:
+        selector: app.kubernetes.io/instance=kro
+        namespace: kro-system

test/e2e/chainsaw/check-external-references/configmap.yaml

@@ -0,0 +1,7 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: check-external-references
+  namespace: default
+data:
+  ECHO_VALUE: "Hello, World!"

test/e2e/chainsaw/check-external-references/instance-assert-in-progress.yaml

@@ -0,0 +1,14 @@
+apiVersion: kro.run/v1alpha1
+kind: ExternalReferenceTest
+metadata:
+  finalizers:
+    - kro.run/finalizer
+  generation: 1
+  labels:
+    kro.run/owned: "true"
+    kro.run/resource-graph-definition-name: check-external-references
+  name: test-external-reference
+spec:
+  name: foobar
+status:
+  state: IN_PROGRESS

test/e2e/chainsaw/check-external-references/instance-assert.yaml

@@ -0,0 +1,18 @@
+apiVersion: kro.run/v1alpha1
+kind: ExternalReferenceTest
+metadata:
+  finalizers:
+    - kro.run/finalizer
+  labels:
+    kro.run/owned: "true"
+    kro.run/resource-graph-definition-name: check-external-references
+  name: test-external-reference
+spec:
+  name: foobar
+status:
+  # filter conditions array to keep elements where `type == 'Ready'`
+  # and assert there's a single element matching the filter
+  # and that this element status is `True`
+  (conditions[?type == 'Ready']):
+    - status: 'True'
+  state: ACTIVE

test/e2e/chainsaw/check-external-references/instance-propagate.yaml

@@ -0,0 +1,6 @@
+kind: ExternalReferenceTest
+apiVersion: kro.run/v1alpha1
+metadata:
+  name: test-external-reference
+spec:
+  name: propagate

test/e2e/chainsaw/check-external-references/instance.yaml

@@ -0,0 +1,4 @@
+kind: ExternalReferenceTest
+apiVersion: kro.run/v1alpha1
+metadata:
+  name: test-external-reference