crossplane icon indicating copy to clipboard operation
crossplane copied to clipboard
verified publisher icon crossplane

fix(fn-cache): prevent caching responses with unfulfilled requirements

Open haarchri opened this issue 3 weeks ago • 2 comments

Description of your changes

This change prevents caching of function responses that have unfulfilled requirements. When a composition function requests extra resources through the Requirements API, the function needs to be called again once those resources are available in the request.

Previously, responses with unfulfilled requirements would be cached, causing subsequent calls to return the cached response without the function ever processing the newly available resources.

output from the cache:

cat crossplane-contrib-function-environment-configs/3e100bfe4c0884050c4914697b2edb0ad38559a22ea3d19e2e9a4054f86da1c5 
?????䔕?
F
@3e100bfe4c0884050c4914697b2edb0ad38559a22ea3d19e2e9a4054f86da1c<"*X
V
environment-config-0>

using the context of a environmentconfig - check its Undefined because of the cache:

kubectl get cm environment-example -o yaml
apiVersion: v1
data:
  environment: Undefined
kind: ConfigMap
metadata:
  annotations:
    crossplane.io/composition-resource-name: example
  creationTimestamp: "2025-12-03T10:01:20Z"
  labels:
    crossplane.io/composite: example
  name: environment-example
  namespace: default
  ownerReferences:
  - apiVersion: example.upbound.io/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: Example
    name: example
    uid: 19c241f6-17d9-4627-b953-22bc779c60fd
  resourceVersion: "2056"
  uid: b62551a0-7ab3-4de8-94be-55808eb85c9d

The implementation adds a check before caching to verify all requested resources are present in the incoming request. Tests verify that responses with unfulfilled requirements are not cached and the wrapped function is called on every invocation, while responses with fulfilled requirements continue to be cached normally.

output from the cache:

cat crossplane-contrib-function-environment-configs/d8294d208f4463e1234eac1a946e249de3518618571ee780c800b6bf6cbf488b
????פ?
F
@d8294d208f4463e1234eac1a946e249de3518618571ee780c800b6bf6cbf488<"?
?
'apiextensions.crossplane.io/environments*q
/
apiVersion!internal.crossplane.io/v1alpha1
kind
Environment
foobar
vpcIdvpc-1234567890*X
V
environment-config-0>
#apiextensions.crossplane.io/v1beta1EnvironmentConfigtest

again using the context of a environmentconfig - check its now available - as its also part of the cache:

kubectl get cm environment-example -o yaml
apiVersion: v1
data:
  environment: '{''apiVersion'': ''internal.crossplane.io/v1alpha1'', ''foo'': ''bar'',
    ''kind'': ''Environment'', ''vpcId'': ''vpc-1234567890''}'
kind: ConfigMap
metadata:
  annotations:
    crossplane.io/composition-resource-name: example
  creationTimestamp: "2025-12-03T10:01:20Z"
  labels:
    crossplane.io/composite: example
  name: environment-example
  namespace: default
  ownerReferences:
  - apiVersion: example.upbound.io/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: Example
    name: example
    uid: 19c241f6-17d9-4627-b953-22bc779c60fd
  resourceVersion: "13334"
  uid: b62551a0-7ab3-4de8-94be-55808eb85c9d

thanks for the reproducer @fhochleitner https://github.com/fhochleitner/up-35-minimal-example

Fixes #

I have:

  • [x] Read and followed Crossplane's contribution process.
  • [x] Run earthly +reviewable to ensure this PR is ready for review.
  • [x] Added or updated unit tests.
  • [x] Added or updated e2e tests.
  • [x] Linked a PR or a docs tracking issue to document this change.
  • [x] Added backport release-x.y labels to auto-backport this PR.
  • [x] Followed the API promotion workflow if this PR introduces, removes, or promotes an API.

Need help with this checklist? See the cheat sheet.

haarchri avatar Dec 03 '25 11:12 haarchri

📝 Walkthrough

Walkthrough

Adds an unexported helper hasUnfulfilledRequirements and a pre-check in CacheFunction to skip caching when a RunFunctionResponse declares resource requirements that the RunFunctionRequest does not satisfy. Tests were added to cover both unfulfilled and fulfilled requirement scenarios.

Changes

Cohort / File(s) Summary
Caching logic change
internal/xfn/cached/cached_runner.go		 Adds unexported hasUnfulfilledRequirements(req *fnv1.RunFunctionRequest, rsp *fnv1.RunFunctionResponse) bool and invokes it in CacheFunction to detect missing required resources; when true, caching is skipped and a debug message is logged.
Unit tests (requirement-based caching)
internal/xfn/cached/cached_runner_test.go		 Adds tests for requirement-aware caching: one ensuring responses with unfulfilled requirements are not cached and the wrapper is invoked each call, and one ensuring responses with fulfilled requirements are cached. The file contains duplicated test definitions for the same scenarios (appears repeated).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review hasUnfulfilledRequirements for correctness around requirement/resource matching and edge cases (nil checks, empty lists).
  • Verify the CacheFunction integration ensures no side-effects when skipping caching (e.g., file creation, partial writes).
  • Inspect cached_runner_test.go duplication to confirm tests are intentional or should be deduplicated.
  • Confirm logging and error-path behavior when requirements are unmet.

Thanks for the change — could you confirm whether the duplicated tests in cached_runner_test.go are intentional or should be consolidated?

Pre-merge checks

✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely describes the main change: preventing caching of responses with unfulfilled requirements, which is exactly what the changeset implements.
Description check ✅ Passed The PR description is comprehensive and directly related to the changeset, providing context about the bug, its impact, the fix, and concrete examples demonstrating the corrected behavior.
Breaking Changes ✅ Passed Changes are limited to internal/xfn/cached/ directory with only unexported helper functions and tests; no public APIs or command-line flags affected.
Feature Gate Requirement ✅ Passed This PR fixes a bug in Requirements API caching behavior by preventing incorrect caching of responses with unfulfilled requirements. It is a correctness fix to existing internal caching logic, not a new experimental feature requiring a feature flag.
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2fd20f277330cd0018302a61db9fcf0249a7263d and 428d16077e2ef62bd35df5a321821d7b60723d12.

📒 Files selected for processing (1)
  • internal/xfn/cached/cached_runner_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/xfn/cached/cached_runner_test.go

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Dec 03 '25 11:12 coderabbitai[bot]

caching a result that has unfulfilled requirements is also a bit odd, that's kind of like caching a partial result, it's not complete yet.

that was my thinking here

haarchri avatar Dec 03 '25 23:12 haarchri