Add support for composition functions to request OpenAPI schemas#7022
Add support for composition functions to request OpenAPI schemas#7022negz wants to merge 9 commits intocrossplane:mainfrom
Conversation
cmd/crank/render/render.go
Outdated
| }() | ||
|
|
||
| runner := xfn.NewFetchingFunctionRunner(runtimes, NewFilteringFetcher(append(in.ExtraResources, in.RequiredResources...)...)) | ||
| runner := xfn.NewFetchingFunctionRunner(runtimes, NewFilteringFetcher(append(in.ExtraResources, in.RequiredResources...)...), xfn.NopRequiredSchemasFetcher{}) |
There was a problem hiding this comment.
Not entirely sure how we'd pass schemas in crossplane render. We could add --required-schemas as a flag, but what would the file format be?
There was a problem hiding this comment.
A flag makes sense to me. I think I'd make the flag value either <name>=<file> or <g/k>=<file> so the user explicitly specifies which file is for which requirement. Probably makes sense to accept both JSON and YAML OpenAPI specs.
There was a problem hiding this comment.
How're users going to get those OpenAPI schemas conveniently though? Will they need to manually extract them from CRDs? What about built in types?
I'm wondering if we should support some combo of:
- Plain OpenAPI v3 file
- CRDs
- Point it at a Kubernetes API server?
Maybe the last one is overkill considering you can presumably curl the OpenAPI endpoint and then use that.
There was a problem hiding this comment.
Yes, seems like a good idea to support CRDs as well as plain OpenAPI. Maybe also XRDs (which we can easily convert to a CRD internally) so users don't have to jump through hoops to get specs for their own APIs.
Adding support for fetching from the k8s API in render feels a little weird, since render doesn't otherwise depend on a real cluster at all. But, it would probably be a simple change, and might make life easier for users.
There was a problem hiding this comment.
just a side note for the render package to make RequiredSchemasFetcher configurable via the Inputs struct: c350330
as we know we using the render package by external CLIs (up-cli, future crossplane-cli split), they cannot import from internal/xfn, so the hardcoded xfn.NopRequiredSchemasFetcher{} blocked any schema fetching functionality - I know this is just a draft for discussion, just wanted to flag it, was we need to make sure its still working with the cli🙂
in my commit is a public render.RequiredSchemasFetcher interface that external tools can implement. For example, in up-cli i have a draft PR with a fetcher that uses the apiDependencies from our internal up-cli cache along with XRDs and CRDs from dependencies, we just needed the ability to inject it as an input parameter.
Note on OpenAPI schema handling: The hard part with OpenAPI schemas are $refs - they need to be flattened before they can be used as input ...
This should make it straightforward to wire up whatever flag format we settle on (OpenAPI files, CRDs, XRDs, or API server)...
There was a problem hiding this comment.
Yes, seems like a good idea to support CRDs as well as plain OpenAPI.
@adamwg For now I've just added support for plain OpenAPI files, in the format you get if you curl the API server, so you could do that to grab them and save them to disk. I agree CRDs, XRDs, etc would be nice, but if we take that path we'd still need to support built-in types some other way, and also inject the ObjectMeta somehow (hardcode it?). With OpenAPI JSON files read from the API server you can include the component the ObjectMeta $ref points to, so clients can resolve the ref.
make RequiredSchemasFetcher configurable via the Inputs struct
@haarchri We don't currently do this for required resources, right? With that in mind let's handle this (making these configurable) separately from his PR, since we need to do something for both.
5ada8c9 to
9b3addf
Compare
Crossplane PR #7022 adds support for composition functions to request OpenAPI schemas for resource kinds. When a function needs to validate or introspect a resource's schema, it can request the schema from Crossplane and receive it in subsequent invocations. This commit updates the protobuf definitions from PR #7022 and adds helper functions to work with schema requirements: - response.require_schema() declares a schema requirement - request.get_required_schema() retrieves the resolved schema The schema is returned as a plain dict, allowing callers to use it directly with validation libraries like openapi-schema-validator or jsonschema. Both get_required_resources() and get_required_schema() docstrings now document how to distinguish between "not yet resolved" and "resolved but not found" using the `name in req.required_*` pattern. Ref: crossplane/crossplane#7022 Signed-off-by: Nic Cope <[email protected]>
📝 WalkthroughWalkthroughAdds per-step required OpenAPI schema selection and fetching: new selector types and protobuf fields, OpenAPI discovery cache and schema fetchers (online/offline), wiring through controllers and the function-runner, CRD/OpenAPI schema updates, unit and e2e tests, and capability advertisement for function requests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,230,241,0.5)
participant Controller as Controller
participant Reconciler as Reconciler
participant SchemasFetcher as SchemasFetcher
participant Discovery as Discovery
participant FunctionRunner as FunctionRunner
participant Function as Function
end
Controller->>Reconciler: Build pipeline / bootstrap
Reconciler->>SchemasFetcher: Fetch(ctx, SchemaSelector) for each requiredSchemas
SchemasFetcher->>Discovery: Paths() / get OpenAPI v3 doc
Discovery-->>SchemasFetcher: OpenAPI document / paths
SchemasFetcher->>SchemasFetcher: Parse & collect $ref schemas
SchemasFetcher-->>Reconciler: Return Schema (per requirementName)
Reconciler->>FunctionRunner: RunFunction(req with RequiredSchemas, RequiredResources, Meta.Capabilities)
FunctionRunner->>Function: Deliver RunFunctionRequest
Function-->>FunctionRunner: RunFunctionResponse
FunctionRunner-->>Reconciler: Results / status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Would you like me to highlight the highest-risk files for focused review (schema collection, cached discovery invalidation, and fetching-runner tests)? Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (3 passed)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/crank/alpha/render/op/render.go (1)
81-146: Render doesn’t populate bootstrap required schemas yet. Thanks for wiring the new fetcher, butcrank renderstill only fillsreq.RequiredResourcesfromfn.Requirements. Should Render also mirror controller behavior by populatingreq.RequiredSchemas(even if it’s empty via the no-op fetcher) so function behavior matches between render and controllers?🛠️ Proposed fix (mirror bootstrap required schemas)
- runner := xfn.NewFetchingFunctionRunner(runtimes, render.NewFilteringFetcher(in.RequiredResources...), xfn.NopRequiredSchemasFetcher{}) + schemaFetcher := xfn.NopRequiredSchemasFetcher{} + runner := xfn.NewFetchingFunctionRunner(runtimes, render.NewFilteringFetcher(in.RequiredResources...), schemaFetcher) @@ if fn.Requirements != nil { req.RequiredResources = map[string]*fnv1.Resources{} for _, sel := range fn.Requirements.RequiredResources { resources, err := render.NewFilteringFetcher(in.RequiredResources...).Fetch(ctx, xfn.ToProtobufResourceSelector(&sel)) if err != nil { return Outputs{}, errors.Wrapf(err, "cannot fetch bootstrap required resources for requirement %q", sel.RequirementName) } req.RequiredResources[sel.RequirementName] = resources } + + req.RequiredSchemas = map[string]*fnv1.Schema{} + for _, sel := range fn.Requirements.RequiredSchemas { + schema, err := schemaFetcher.Fetch(ctx, xfn.ToProtobufSchemaSelector(&sel)) + if err != nil { + return Outputs{}, errors.Wrapf(err, "cannot fetch bootstrap required schema for requirement %q", sel.RequirementName) + } + req.RequiredSchemas[sel.RequirementName] = schema + } }cluster/crds/ops.crossplane.io_operations.yaml (1)
126-133: Minor typo: "unction" should be "function".I noticed a small typo in line 129-130: "will be passed to the unction" should be "will be passed to the function". Since this is a generated CRD, the fix would need to be in the source file (
apis/ops/v1alpha1/operation_types.go, line 71-72 in thePipelineStep.Inputfield comment).This appears to be pre-existing rather than introduced by this PR, so feel free to address it separately if you prefer!
🤖 Fix all issues with AI agents
In `@internal/xfn/cached_discovery.go`:
- Around line 51-64: The error messages returned from
CachedOpenAPIClient.InvalidateOnCRDChanges when ca.GetInformer or
i.AddEventHandler fail are too generic; update the wrapped errors to state that
schema cache invalidation for CRDs couldn't be set up and suggest checking CRD
RBAC/permissions (e.g., change the errors.Wrap calls around ca.GetInformer and
i.AddEventHandler to include text like "failed to set up CRD informer; schema
cache invalidation not configured — check RBAC/permissions for
CustomResourceDefinitions"). Reference CachedOpenAPIClient and
InvalidateOnCRDChanges, and keep the original wrapped error appended for
diagnostics.
In `@internal/xfn/required_resources.go`:
- Around line 134-141: Improve the operator-facing error message when
c.schemas.Fetch(ctx, selector) fails in the loop over
newRequirements.GetSchemas(): update the errors.Wrapf call to include actionable
guidance (e.g., suggest verifying the CRD is installed and that OpenAPI
discovery is reachable) while keeping context (include the schema name from the
loop), so operators know next steps; locate this inside the function iterating
newRequirements.GetSchemas() where c.schemas.Fetch is called and
req.RequiredSchemas is populated and replace the terse "fetching schema for %s"
wrap with a message that combines the schema name and a brief remediation hint.
🧹 Nitpick comments (3)
apis/ops/v1alpha1/operation_types.go (1)
205-232: RequiredSchemaSelector is duplicated across API groups.This type is defined identically in both
apis/ops/v1alpha1/operation_types.goandapis/apiextensions/v1/composition_common.go. I see that a conversion function exists inzz_generated.conversion.goto handle the separation. Understanding the reasoning here would be helpful: is this duplication intentional to keep the alpha ops API self-contained, or is there an opportunity to consolidate? If keeping them separate is a deliberate design choice (e.g., allowing independent evolution of the alpha and stable APIs), that makes sense!The implementation itself is solid—the getters correctly satisfy the
RequiredSchemaSelectorinterface frominternal/xfn/selector.go.test/e2e/manifests/apiextensions/composition/required-schemas/setup/functions.yaml (1)
6-8: TODO tracking for temporary function build.Thanks for documenting this clearly! Could you confirm whether there's an issue or tracking item to remind the team to update this to the official function-python release once schema support is merged upstream? This would help ensure it doesn't get forgotten.
Would you like me to help create a tracking issue for this follow-up task?
cmd/crossplane/core/core.go (1)
34-35: OpenAPI discovery wiring looks good—please confirm RBAC/failure mode.Thanks for adding the cached OpenAPI client and invalidation hook. Could you confirm the Crossplane ClusterRole includes GET on
/openapi/v3(non‑resource URL) so schema fetches won’t be RBAC‑blocked, and that failing to set up invalidation should be fatal? If it’s optional, a log‑and‑continue path might be safer.Also applies to: 447-460
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
apis/apiextensions/v1/zz_generated.conversion.gois excluded by!**/zz_generated*.goand included by**/*.goapis/apiextensions/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.goapis/ops/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.goproto/fn/v1/run_function.pb.gois excluded by!**/*.pb.go,!**/*.pb.goand included by**/*.goproto/fn/v1beta1/zz_generated_run_function.pb.gois excluded by!**/*.pb.go,!**/zz_generated*.go,!**/*.pb.goand included by**/*.go
📒 Files selected for processing (34)
apis/apiextensions/v1/composition_common.goapis/ops/v1alpha1/operation_types.gocluster/crds/apiextensions.crossplane.io_compositionrevisions.yamlcluster/crds/apiextensions.crossplane.io_compositions.yamlcluster/crds/ops.crossplane.io_cronoperations.yamlcluster/crds/ops.crossplane.io_operations.yamlcluster/crds/ops.crossplane.io_watchoperations.yamlcmd/crank/alpha/render/op/render.gocmd/crank/render/render.gocmd/crossplane/core/core.gointernal/controller/apiextensions/composite/composition_functions.gointernal/controller/apiextensions/controller/options.gointernal/controller/apiextensions/definition/reconciler.gointernal/controller/ops/controller/options.gointernal/controller/ops/operation/constructor.gointernal/controller/ops/operation/reconciler.gointernal/xfn/cached_discovery.gointernal/xfn/required_resources.gointernal/xfn/required_resources_test.gointernal/xfn/required_schemas.gointernal/xfn/required_schemas_test.gointernal/xfn/selector.goproto/fn/v1/run_function.protoproto/fn/v1beta1/zz_generated_run_function.prototest/e2e/apiextensions_compositions_test.gotest/e2e/manifests/apiextensions/composition/required-resources/setup/composition.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/configmap.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/definition.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/functions.yamltest/e2e/manifests/apiextensions/composition/required-resources/xr.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/composition.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/definition.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/functions.yamltest/e2e/manifests/apiextensions/composition/required-schemas/xr.yaml
🧰 Additional context used
📓 Path-based instructions (10)
{apis,cmd}/**
📄 CodeRabbit inference engine (Custom checks)
{apis,cmd}/**: Public fields/flags in apis/** or cmd/** must not be removed or renamed without a 'breaking-change' label
New required public fields/flags in apis/** or cmd/** must not be added without a 'breaking-change' label
Behavior removals in apis/** or cmd/** must be labeled with 'breaking-change'
Files:
apis/ops/v1alpha1/operation_types.gocmd/crank/alpha/render/op/render.goapis/apiextensions/v1/composition_common.gocmd/crank/render/render.gocmd/crossplane/core/core.go
apis/**
📄 CodeRabbit inference engine (Custom checks)
New experimental features affecting apis/** must have feature flag implementations
Files:
apis/ops/v1alpha1/operation_types.goapis/apiextensions/v1/composition_common.go
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
apis/ops/v1alpha1/operation_types.gointernal/controller/ops/controller/options.gointernal/controller/ops/operation/reconciler.gointernal/xfn/selector.gocmd/crank/alpha/render/op/render.gointernal/xfn/required_resources_test.gointernal/xfn/required_schemas_test.gointernal/controller/apiextensions/composite/composition_functions.gointernal/xfn/required_resources.goapis/apiextensions/v1/composition_common.gointernal/controller/apiextensions/definition/reconciler.gointernal/xfn/cached_discovery.gotest/e2e/apiextensions_compositions_test.gocmd/crank/render/render.gocmd/crossplane/core/core.gointernal/controller/apiextensions/controller/options.gointernal/xfn/required_schemas.gointernal/controller/ops/operation/constructor.go
**/apis/**
⚙️ CodeRabbit configuration file
**/apis/**: Focus on API design following Kubernetes API conventions from
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md.
Check for proper field naming (camelCase), appropriate types, validation
tags, and documentation. Ask about backward compatibility and the impact
on existing users and upgrade paths. Consider if changes need feature
gates or alpha/beta graduation. Ensure error messages in validation are
user-friendly. Pay attention to API consistency, proper use of optional
vs required fields, and following established Kubernetes patterns.
Files:
apis/ops/v1alpha1/operation_types.goapis/apiextensions/v1/composition_common.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/ops/controller/options.gointernal/controller/ops/operation/reconciler.gointernal/controller/apiextensions/composite/composition_functions.gointernal/controller/apiextensions/definition/reconciler.gointernal/controller/apiextensions/controller/options.gointernal/controller/ops/operation/constructor.go
**/test/**
⚙️ CodeRabbit configuration file
**/test/**: Focus on test coverage, test clarity, and proper use of testing
utilities. Ask about testing scenarios and edge cases. Ensure tests are
maintainable and cover the happy path and error conditions. Verify
error testing uses proper patterns (cmpopts.EquateErrors, sentinel
errors for complex cases).
Files:
test/e2e/manifests/apiextensions/composition/required-resources/xr.yamltest/e2e/manifests/apiextensions/composition/required-schemas/xr.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/composition.yamltest/e2e/apiextensions_compositions_test.gotest/e2e/manifests/apiextensions/composition/required-resources/setup/configmap.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/composition.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/definition.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/functions.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/functions.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/definition.yaml
**/test/e2e/**
⚙️ CodeRabbit configuration file
**/test/e2e/**: Review E2E tests for proper structure following Crossplane patterns:
Use features.NewWithDescription() with CamelCase names, not sentences.
Ensure proper Setup/Assess/Teardown phases with descriptive names.
Check that tests have appropriate labels (area, size, test suite).
Verify tests use YAML manifests in test/e2e/manifests//
structure. Ensure tests avoid complex logic in e2e package and use
funcs package for reusable logic. Check that tests don't assume
ordering and can run independently. Verify proper cleanup in teardown
phases.
Files:
test/e2e/manifests/apiextensions/composition/required-resources/xr.yamltest/e2e/manifests/apiextensions/composition/required-schemas/xr.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/composition.yamltest/e2e/apiextensions_compositions_test.gotest/e2e/manifests/apiextensions/composition/required-resources/setup/configmap.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/composition.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/definition.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/functions.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/functions.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/definition.yaml
**/cluster/**
⚙️ CodeRabbit configuration file
**/cluster/**: Review Kubernetes manifests for security, resource limits, and proper
RBAC. Ensure Helm chart follows best practices. Consider upgrade and
rollback scenarios.
Files:
cluster/crds/apiextensions.crossplane.io_compositions.yamlcluster/crds/ops.crossplane.io_operations.yamlcluster/crds/ops.crossplane.io_cronoperations.yamlcluster/crds/ops.crossplane.io_watchoperations.yamlcluster/crds/apiextensions.crossplane.io_compositionrevisions.yaml
**/cmd/**
⚙️ CodeRabbit configuration file
**/cmd/**: Review CLI commands for proper flag handling, help text, and error
messages. Ensure commands follow Crossplane CLI conventions. Ask about
backward compatibility and user experience. CLI error messages must be
especially user-friendly - avoid internal error details, provide
actionable guidance.
Files:
cmd/crank/alpha/render/op/render.gocmd/crank/render/render.gocmd/crossplane/core/core.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/xfn/required_resources_test.gointernal/xfn/required_schemas_test.gotest/e2e/apiextensions_compositions_test.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Review code for functional correctness, proper documentation and intent explanation, adherence to Coding Style guidelines, idiomatic alignment with existing Crossplane APIs, sufficient test coverage, DCO sign-off, and passing CI jobs
Learnt from: negz
Repo: crossplane/crossplane PR: 6777
File: test/e2e/manifests/apiextensions/composition/circuit-breaker/xr.yaml:5-9
Timestamp: 2025-09-09T21:35:41.478Z
Learning: XRDs (CompositeResourceDefinitions) don't need to explicitly define spec.crossplane.compositionRef in their openAPIV3Schema because this field is automatically available on all composite resources by Crossplane itself. The compositionRef field is implicit and doesn't need to be defined in the XRD schema.
📚 Learning: 2025-09-09T21:35:41.478Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6777
File: test/e2e/manifests/apiextensions/composition/circuit-breaker/xr.yaml:5-9
Timestamp: 2025-09-09T21:35:41.478Z
Learning: XRDs (CompositeResourceDefinitions) don't need to explicitly define spec.crossplane.compositionRef in their openAPIV3Schema because this field is automatically available on all composite resources by Crossplane itself. The compositionRef field is implicit and doesn't need to be defined in the XRD schema.
Applied to files:
test/e2e/manifests/apiextensions/composition/required-resources/xr.yamltest/e2e/manifests/apiextensions/composition/required-schemas/xr.yamlcluster/crds/apiextensions.crossplane.io_compositions.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/composition.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/composition.yamltest/e2e/manifests/apiextensions/composition/required-resources/setup/definition.yamlcluster/crds/apiextensions.crossplane.io_compositionrevisions.yamltest/e2e/manifests/apiextensions/composition/required-schemas/setup/definition.yaml
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use https://github.com/google/go-cmp/ for comparisons in tests to keep test output consistent
Applied to files:
internal/xfn/required_resources_test.gointernal/xfn/required_schemas_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use table-driven tests instead of test frameworks like Ginkgo; structure tests with 'cases' map containing test cases with 'reason', 'args', and 'want' fields
Applied to files:
internal/xfn/required_resources_test.gointernal/xfn/required_schemas_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : All Crossplane features must be covered by unit tests and end-to-end (E2E) tests
Applied to files:
internal/xfn/required_schemas_test.gotest/e2e/apiextensions_compositions_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/test/e2e/**/*.go : Add E2E tests only for important use cases that cannot be verified by unit tests; E2E tests are considered expensive and should be used sparingly
Applied to files:
internal/xfn/required_schemas_test.gotest/e2e/apiextensions_compositions_test.go
📚 Learning: 2025-12-02T14:42:02.085Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T14:42:02.085Z
Learning: Applies to {apis,cmd}/** : New required public fields/flags in apis/** or cmd/** must not be added without a 'breaking-change' label
Applied to files:
apis/apiextensions/v1/composition_common.gocluster/crds/apiextensions.crossplane.io_compositionrevisions.yaml
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Review code for functional correctness, proper documentation and intent explanation, adherence to Coding Style guidelines, idiomatic alignment with existing Crossplane APIs, sufficient test coverage, DCO sign-off, and passing CI jobs
Applied to files:
test/e2e/manifests/apiextensions/composition/required-resources/setup/composition.yaml
📚 Learning: 2025-10-10T19:15:32.438Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6844
File: cluster/crds/pkg.crossplane.io_deploymentruntimeconfigs.yaml:843-845
Timestamp: 2025-10-10T19:15:32.438Z
Learning: CRDs under cluster/crds/*.yaml are generated from upstream Kubernetes API types via controller-gen (see controller-gen.kubebuilder.io/version annotation). Descriptive text in these files mirrors upstream and should not be edited locally; fixes must be made upstream and pulled in via k8s.io module updates.
Applied to files:
cluster/crds/apiextensions.crossplane.io_compositionrevisions.yaml
🧬 Code graph analysis (15)
apis/ops/v1alpha1/operation_types.go (2)
apis/apiextensions/v1/composition_common.go (1)
RequiredSchemaSelector(205-215)internal/xfn/selector.go (1)
RequiredSchemaSelector(62-66)
internal/controller/ops/controller/options.go (1)
internal/xfn/required_schemas.go (1)
OpenAPIV3Client(56-58)
internal/controller/ops/operation/reconciler.go (2)
internal/xfn/required_schemas.go (1)
RequiredSchemasFetcher(33-35)internal/xfn/selector.go (1)
ToProtobufSchemaSelector(70-75)
internal/xfn/selector.go (2)
apis/apiextensions/v1/composition_common.go (1)
RequiredSchemaSelector(205-215)apis/ops/v1alpha1/operation_types.go (1)
RequiredSchemaSelector(207-217)
cmd/crank/alpha/render/op/render.go (3)
internal/xfn/required_resources.go (1)
NewFetchingFunctionRunner(62-64)cmd/crank/render/render.go (1)
NewFilteringFetcher(502-504)internal/xfn/required_schemas.go (2)
NopRequiredSchemasFetcher(47-47)NopRequiredSchemasFetcher(50-52)
internal/xfn/required_resources_test.go (4)
internal/xfn/required_schemas.go (4)
RequiredSchemasFetcher(33-35)NopRequiredSchemasFetcher(47-47)NopRequiredSchemasFetcher(50-52)RequiredSchemasFetcherFn(38-38)internal/controller/apiextensions/composite/composition_functions.go (1)
FunctionRunnerFn(130-130)internal/xfn/utils_test.go (1)
MustStruct(218-225)internal/xfn/required_resources.go (1)
NewFetchingFunctionRunner(62-64)
internal/xfn/required_schemas_test.go (1)
internal/xfn/required_schemas.go (6)
RequiredSchemasFetcher(33-35)OpenAPISchemasFetcher(62-64)OpenAPIV3Client(56-58)NewOpenAPISchemasFetcher(77-79)NopRequiredSchemasFetcher(47-47)NopRequiredSchemasFetcher(50-52)
internal/controller/apiextensions/composite/composition_functions.go (3)
internal/xfn/required_schemas.go (1)
RequiredSchemasFetcher(33-35)internal/controller/ops/operation/constructor.go (1)
WithRequiredSchemasFetcher(94-98)internal/xfn/selector.go (1)
ToProtobufSchemaSelector(70-75)
internal/xfn/required_resources.go (2)
internal/xfn/required_schemas.go (1)
RequiredSchemasFetcher(33-35)internal/controller/apiextensions/composite/composition_functions.go (1)
FunctionRunner(124-127)
apis/apiextensions/v1/composition_common.go (2)
apis/ops/v1alpha1/operation_types.go (1)
RequiredSchemaSelector(207-217)internal/xfn/selector.go (1)
RequiredSchemaSelector(62-66)
internal/controller/apiextensions/definition/reconciler.go (3)
internal/controller/apiextensions/composite/composition_functions.go (1)
WithRequiredSchemasFetcher(230-234)internal/controller/ops/operation/constructor.go (1)
WithRequiredSchemasFetcher(94-98)internal/xfn/required_schemas.go (1)
NewOpenAPISchemasFetcher(77-79)
cmd/crank/render/render.go (2)
internal/xfn/required_resources.go (1)
NewFetchingFunctionRunner(62-64)internal/xfn/required_schemas.go (2)
NopRequiredSchemasFetcher(47-47)NopRequiredSchemasFetcher(50-52)
cmd/crossplane/core/core.go (3)
internal/xfn/cached_discovery.go (1)
NewCachedOpenAPIClient(38-40)internal/xfn/required_resources.go (2)
NewFetchingFunctionRunner(62-64)NewExistingRequiredResourcesFetcher(157-159)internal/xfn/required_schemas.go (1)
NewOpenAPISchemasFetcher(77-79)
internal/controller/apiextensions/controller/options.go (1)
internal/xfn/required_schemas.go (1)
OpenAPIV3Client(56-58)
internal/controller/ops/operation/constructor.go (2)
internal/xfn/required_schemas.go (2)
NewOpenAPISchemasFetcher(77-79)RequiredSchemasFetcher(33-35)internal/controller/ops/operation/reconciler.go (1)
Reconciler(70-81)
🔍 Remote MCP
Let me search for more specific information about the required resources pattern that this PR mirrors, to better understand the implementation context.
Additional Context Summary
Based on my search for relevant context about this PR, I've identified important background information for reviewing the changes:
Composition Functions and Required Resources Pattern
Functions can request required resources by specifying them in the returned RunFunctionResponse, and Crossplane then calls the function again including the requested required resources alongside the same input, observed and desired state of the previous RunFunctionRequest. Functions can iteratively request required resources if needed, but Crossplane limits the number of iterations to 5, and considers the function satisfied as soon as the required resources requests become stable (same request two iterations in a row).
The PR extends this existing required resources pattern to support required schemas, implementing parallel functionality within the same iteration/bootstrap framework.
Function-Python SDK Context
The RunFunctionRequest and RunFunctionResponse types provided by the Python SDK are generated from a proto3 protocol buffer schema. The PR notes that the e2e test uses a temporary function-python build with schema support (marked as TODO to switch to an official release). The Python SDK automatically generates the RunFunctionRequest and RunFunctionResponse Python objects from a Protocol Buffers schema, which can be seen in the Buf Schema Registry.
Discovery Cache Invalidation Importance
The PR's implementation of CachedOpenAPIClient.InvalidateOnCRDChanges() addresses a known issue in Kubernetes tooling. The discovery cache invalidates itself every 10 minutes, meaning clients have to make a request to every group version on the cluster to populate the cache again, which results in numerous GET requests and can trigger client-side rate limiting. The discovery cache was configured to expire after 6 hours rather than 10 minutes in recent Kubernetes versions. By invalidating on CRD changes rather than waiting for timeout, the PR ensures schema changes are immediately reflected without stale cache issues—a critical concern for systems managing many CRDs.
Key Implementation Pattern Consistency
The PR mirrors the required resources bootstrap pattern:
- Extra Resources were introduced in Crossplane 1.15 as a powerful feature allowing a function to search for any other Crossplane resource on a Cluster and store data in the Function's context.
The required schemas feature follows the same bootstrap/fetch pattern but for OpenAPI schema metadata rather than actual resources, enabling functions to access schema information to make decisions about resource composition.
This context confirms the PR extends an established and well-tested pattern (required resources) with parallel functionality for schemas, uses standard protobuf-based function interfaces, and implements proactive cache invalidation to avoid known discovery cache staleness issues in systems with many CRDs.
🔇 Additional comments (52)
test/e2e/manifests/apiextensions/composition/required-resources/setup/definition.yaml (1)
1-32: LGTM!This XRD manifest is well-structured for the required-resources e2e test. The status fields (
resourceReceived,resourceHasExpectedData) clearly support the test assertions for validating resource fetch behavior. Based on learnings, the implicitcompositionReffield doesn't need to be explicitly defined.cluster/crds/ops.crossplane.io_cronoperations.yaml (1)
225-255: LGTM!The
requiredSchemasfield structure mirrors the existingrequiredResourcespattern nicely, maintaining API consistency. The list semantics withrequirementNameas the map key ensure proper merging behavior. Thank you for keeping the descriptions clear about what each field represents!test/e2e/manifests/apiextensions/composition/required-resources/setup/configmap.yaml (1)
1-8: LGTM!Simple and clear test fixture for the required-resources e2e test. The explicit namespace and descriptive key-value pairs make the test intent easy to follow.
test/e2e/manifests/apiextensions/composition/required-schemas/xr.yaml (1)
1-7: LGTM!This XR manifest appropriately triggers the required-schemas test flow. Quick clarifying question: is the composition reference set via a default composition in the test setup, or is there another mechanism that wires this XR to the composition that requests schemas? (Based on learnings, the
compositionReffield is implicit on all XRs, so I assume the test setup handles the binding.)internal/controller/apiextensions/definition/reconciler.go (1)
515-520: Consider adding nil-safety for the OpenAPIClientThe production initialization path in
core.gois solid—the discovery client is created with proper error handling before being passed to Options. However, the wiring code lacks defensive nil checks. Ifr.options.OpenAPIClientis nil (which could happen in test scenarios or if Options initialization fails elsewhere), theFetch()method will panic when callingf.client.Paths()on line 105 ofrequired_schemas.go.Given the coding guidelines emphasizing proper error handling in controllers, consider adding a nil check either:
- In the wiring at line 519, or
- In
NewOpenAPISchemasFetcher()itself with a clear error messageThis would gracefully handle cases where the client isn't initialized rather than allowing a runtime panic. What's your preference on where this safety check should live?
test/e2e/manifests/apiextensions/composition/required-resources/setup/functions.yaml (1)
1-6: Looks good for the required-resources E2E setup. Thanks for adding this manifest; it’s minimal and clear.internal/controller/apiextensions/controller/options.go (1)
40-42: Nice addition of OpenAPI client plumbing. This aligns well with the new schema bootstrap flow. Thanks!test/e2e/manifests/apiextensions/composition/required-resources/xr.yaml (1)
1-7: LGTM for the test resource instance. Clear and appropriately scoped.internal/controller/ops/controller/options.go (1)
34-36: Looks good. This mirrors the apiextensions controller wiring cleanly.test/e2e/manifests/apiextensions/composition/required-schemas/setup/definition.yaml (1)
1-32: LGTM! Well-structured XRD for e2e testing.This CompositeResourceDefinition is appropriately designed for testing the required schemas feature. The status fields
schemaReceivedandschemaHasPropertiesprovide clear signals for validating that the function correctly received and processed the OpenAPI schema data. Thanks for including these validation points in the test fixture!apis/ops/v1alpha1/operation_types.go (1)
137-143: LGTM! Clean extension of FunctionRequirements.The new
RequiredSchemasfield follows the same pattern asRequiredResources, with appropriate kubebuilder markers (+listType=map,+listMapKey=requirementName) ensuring consistent list semantics. Thanks for maintaining the parallel structure!cluster/crds/apiextensions.crossplane.io_compositions.yaml (1)
217-246: LGTM! CRD schema correctly reflects the new RequiredSchemas field.The generated schema for
requiredSchemasfollows the same structural pattern asrequiredResources, with:
- Proper list-map semantics (
x-kubernetes-list-map-keys: [requirementName])- Required fields:
apiVersion,kind,requirementName- Clear descriptions with helpful examples
This ensures consistent strategic merge patch behavior and a good user experience. Thanks for keeping the API surface consistent!
cluster/crds/ops.crossplane.io_operations.yaml (1)
191-221: LGTM! RequiredSchemas field correctly added to Operation CRD.The schema structure is consistent with the Compositions CRD, maintaining the same list-map semantics and required field definitions. This ensures a uniform API experience across both Composition and Operation resources.
cluster/crds/apiextensions.crossplane.io_compositionrevisions.yaml (1)
221-250: LGTM! CompositionRevision CRD correctly mirrors Composition schema.The
requiredSchemasfield is appropriately added to the CompositionRevision CRD, ensuring that revision snapshots correctly capture schema requirements. This is important for maintaining consistency when users reference specific composition revisions.test/e2e/manifests/apiextensions/composition/required-resources/setup/composition.yaml (1)
1-60: Well-structured e2e test composition for required resources.Thank you for adding this comprehensive test! The Python script nicely covers the full lifecycle: requesting the resource, checking receipt, validating expected data, and providing observability through results. The status field updates align well with the assertions in
TestRequiredResources.internal/controller/ops/operation/reconciler.go (2)
80-80: LGTM - New schemas fetcher dependency.The new
schemasfield follows the established pattern alongside the existingresourcesfetcher.
249-266: LGTM - Schema bootstrap logic mirrors resources pattern.This implementation nicely parallels the existing required resources bootstrap (lines 231-248). The error handling is consistent, including proper failure counting, logging with context, event emission, and status updates. The error messages are user-friendly and include the requirement name for troubleshooting.
test/e2e/apiextensions_compositions_test.go (2)
416-452: Solid e2e test for required schemas feature.Thanks for adding this test coverage! The structure follows the established Crossplane e2e patterns nicely:
- Proper Setup/Assess/Teardown phases with descriptive step names
- Appropriate labels for categorization
- Assertions on both schema receipt and schema properties validation
This is exactly the kind of integration that needs e2e coverage since it involves runtime function-to-Crossplane interactions.
454-490: Good parallel test for required resources.This test complements TestRequiredSchemas well and validates the existing required resources mechanism with the same thoroughness. The status field assertions (
resourceReceived,resourceHasExpectedData) align perfectly with what the composition script inrequired-resources/setup/composition.yamlsets.internal/xfn/selector.go (1)
59-75: Interface implementation verified and design is sound.Both
apiextensions/v1.RequiredSchemaSelectorandops/v1alpha1.RequiredSchemaSelectorproperly implement all three required getter methods. The interface design is appropriately minimal for schema identification (only GVK needed), and theToProtobufSchemaSelectorfunction correctly omitsRequirementNamesince it serves as the map key in the request, not part of the selector itself.cluster/crds/ops.crossplane.io_watchoperations.yaml (1)
228-258: LGTM! The requiredSchemas field follows the established pattern nicely.The schema structure mirrors
requiredResourcesappropriately, with the list-map keyed byrequirementNameto ensure uniqueness. I appreciate that you've kept the three required fields (apiVersion,kind,requirementName) simple and focused—schemas don't need the selection complexity that resources do.test/e2e/manifests/apiextensions/composition/required-schemas/setup/composition.yaml (1)
1-54: Well-structured e2e test for the required schemas feature!Thanks for choosing ConfigMap as the test subject—it's a stable core type that should reliably have the
data/binaryDataproperties. The three-state handling (properties found, empty schema, waiting) provides good observability into what's happening during the test.A quick question: the PR summary mentions this uses a temporary
function-python-schemasbuild with SDK support for schemas. Is there a tracking issue or TODO comment elsewhere to remind us to switch to the official release once available?internal/controller/apiextensions/composite/composition_functions.go (5)
85-86: LGTM!The error constant follows the established naming pattern and provides a clear, user-friendly message format.
112-113: LGTM!The new
schemasfield follows the same pattern asresources, keeping the struct design consistent.
228-235: LGTM!The option function mirrors
WithRequiredResourcesFetchernicely—consistent API surface makes it easy for consumers to understand how to configure both fetchers.
254-255: LGTM!Good default—
NopRequiredSchemasFetcher{}returns an empty schema, which is safe and matches the documented behavior for missing schemas.
355-364: LGTM! Clean parallel implementation to resource fetching.The schema bootstrap loop mirrors the resource bootstrap loop above it, which makes the code predictable and easy to follow.
One small observation: the error wrapping correctly uses
errFmtFetchBootstrapSchemaswith the requirement name, giving users actionable context if something goes wrong. Nice!cmd/crank/render/render.go (1)
206-206: Reasonable interim solution using the no-op fetcher.I see from the past discussion that there's uncertainty about how to pass schemas in
crossplane render. UsingNopRequiredSchemasFetcher{}means functions requesting schemas will receive emptySchema{}responses, which matches the "not found" behavior in the controller.Could you share your thoughts on the follow-up plan? Would it be helpful to add a brief TODO comment here noting that schema support for
crossplane renderis intentionally limited, so future contributors have context?internal/xfn/required_resources_test.go (5)
340-343: LGTM!Adding the
schemasfield to the params struct keeps the test infrastructure aligned with the updated constructor signature.
641-667: Good error path coverage!The
FetchSchemasErrortest case nicely mirrors the existingFetchResourcesErrorpattern, ensuring consistent error handling behavior between resources and schemas.
668-720: Solid happy-path test for schema fetching!I appreciate the verification on the second call that checks
pod-schemais properly populated. The mock schema includes thex-kubernetes-group-version-kindextension, which reflects realistic OpenAPI schema structure.
721-799: Excellent coverage for the bootstrap preservation scenario!This test mirrors
PreserveBootstrapResourcesWithDynamicRequirementsand validates that bootstrap schemas aren't lost when functions add dynamic schema requirements. The two-iteration verification (first call checks bootstrap exists, second call verifies both bootstrap and dynamic) provides good confidence in the merging behavior.
802-815: LGTM!The test runner correctly passes the schemas fetcher from test params to the constructor.
internal/controller/ops/operation/constructor.go (3)
42-46: Guard schema fetcher setup if the OpenAPI client can be nil.Thanks for wiring the schemas fetcher. Quick question: is
o.OpenAPIClientguaranteed to be non‑nil for ops controllers (including tests)? If not,NewOpenAPISchemasFetcherwill later dereference a nil client. Consider conditionally applying the option or defaulting toxfn.NopRequiredSchemasFetcher{}.
93-98: Option pattern stays consistent.Nice addition—this matches the existing option style and keeps wiring flexible. Thanks!
102-109: Safe default for schemas fetcher.Defaulting to
NopRequiredSchemasFetcherpreserves current behavior when schemas aren’t configured. Thanks!apis/apiextensions/v1/composition_common.go (2)
128-143: Is RequiredSchemas intended to be always‑on or gated?Great addition. If required schemas are still experimental, should this be behind the appropriate feature flag to avoid early API commitments? If it’s ready for general use, all good. Thanks! As per coding guidelines, ...
204-230: Selector accessors align with shared interface.Accessor methods mirror the RequiredResourceSelector pattern—nice consistency. Thanks!
internal/xfn/required_schemas_test.go (4)
35-59: Mocks are clear and flexible.Function‑field mocks make per‑case behavior easy to set up. Thanks!
61-73: Helper keeps fixtures tidy.
openAPIDockeeps the test data compact and readable—nice touch.
75-413: Solid table‑driven coverage.Great spread of cases (nil selector, discovery errors, invalid JSON, multi‑GVK filtering, and success paths). Nice work.
415-427: Good sanity test for the NOP fetcher.Simple and effective coverage for the no‑op behavior.
cmd/crossplane/core/core.go (1)
465-485: OpenAPI client threading into controller options looks consistent.Nice to see a shared client passed into both API extension and ops controllers—keeps schema fetching centralized. Thanks!
proto/fn/v1beta1/zz_generated_run_function.proto (1)
96-101: Schema requirements are documented clearly.The new
required_schemas/schemasfields and SchemaSelector/Schema messages read consistent with required_resources semantics, including the empty‑schema‑on‑miss behavior. Thanks for keeping v1beta1 in sync.Also applies to: 182-201
internal/xfn/required_resources.go (2)
72-110: Bootstrap retention for required resources/schemas looks solid.Keeping the initial maps and re-seeding each iteration preserves prior behavior while adding schemas—nice consistency. Thank you!
52-64: The codebase already handles this correctly. All call sites pass valid implementations—eitherxfn.NewOpenAPISchemasFetcher(oac)orxfn.NopRequiredSchemasFetcher{}—which aligns with Crossplane's pattern of expecting non-nil interfaces and placing responsibility on the caller. No defensive nil guard is needed.internal/xfn/cached_discovery.go (1)
30-45: LGTM: clean wrapper for OpenAPI v3 paths.Simple, focused API surface—nice addition. Thanks!
proto/fn/v1/run_function.proto (2)
94-99: Docs for required_schemas are clear and consistent.The “empty Schema indicates attempted lookup” behavior mirrors required_resources nicely. Thanks!
180-199: SchemaSelector/Schema additions look consistent with existing selectors.This aligns well with ResourceSelector conventions and keeps the API intuitive. Nice work!
internal/xfn/required_schemas.go (3)
32-52: Clean RequiredSchemasFetcher abstraction + no‑op implementation.This makes wiring optional schema support straightforward. Thanks!
54-79: OpenAPIV3Client and fetcher setup look tidy.Constructor + minimal interface keeps testing and wiring easy. Appreciate the clarity!
100-115: The code is correct as-is.application/jsonis the standard and recommended media type for fetching OpenAPI v3 schemas via client-go'sSchema()method (confirmed in v0.34.1). The library accepts a string parameter, and there's no constant to reference—the literal string is the normal pattern across the Kubernetes ecosystem. Thanks for double-checking!
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@design/one-pager-function-capabilities.md`:
- Around line 100-116: Several paragraphs in the "Self-describing messages" and
"Implicit detection via iteration" sections exceed the 100‑column guideline;
reflow those paragraphs to wrap at 100 columns for consistency. Edit the text
blocks mentioning FileDescriptorSet, RunFunctionRequest, and the sentences about
descriptors, message size, gRPC reflection, and "extra round-trip"/"unknown
field preservation" so each line is <=100 characters while preserving original
wording and punctuation. Ensure blank lines and markdown headers remain intact
and that links like [self-description] are not broken across lines.
In `@internal/controller/ops/operation/reconciler.go`:
- Around line 250-266: The schema-fetch branch should surface which selector
failed and an actionable next step: when r.schemas.Fetch (called with
xfn.ToProtobufSchemaSelector(&sel)) returns an error, include sel details (e.g.,
sel.APIVersion, sel.Kind, sel.RequirementName or other selector fields) in the
wrapped error and log/event messages and append guidance like “ensure the CRD is
installed and exposes an OpenAPI schema (or the schema is available)”. Update
the process in the error block: enhance the log.Debug call, replace errors.Wrapf
with a wrapped message that contains the selector fields and the remediation
hint, and ensure r.record.Event(op, event.Warning(reasonBootstrapRequirements,
err)) and status.MarkConditions(xpv1.ReconcileError(err)) use that enriched
error before calling r.client.Status().Update(ctx, op).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/ops/operation/constructor.go`:
- Around line 42-46: Guard the call to xfn.NewOpenAPISchemasFetcher so it isn't
invoked with a nil o.OpenAPIClient: in the NewReconciler wiring (the expression
that passes WithRequiredSchemasFetcher) check o.OpenAPIClient for nil and pass a
no-op schema fetcher when nil (e.g., use xfn.NopOpenAPISchemasFetcher or the
package's equivalent) otherwise call
xfn.NewOpenAPISchemasFetcher(o.OpenAPIClient); update the
WithRequiredSchemasFetcher argument accordingly so NewReconciler never receives
a fetcher that will panic when OpenAPIClient is nil.
♻️ Duplicate comments (3)
internal/controller/ops/operation/reconciler.go (1)
250-266: Make schema-fetch errors more actionable. Could we include the requested apiVersion/kind and a brief next step (e.g., ensure the CRD is installed and exposes OpenAPI) so operators know what to fix? Thanks! As per coding guidelines, ...💡 Possible improvement
- schema, err := r.schemas.Fetch(ctx, xfn.ToProtobufSchemaSelector(&sel)) + ss := xfn.ToProtobufSchemaSelector(&sel) + schema, err := r.schemas.Fetch(ctx, ss) if err != nil { op.Status.Failures++ - log.Debug("Cannot fetch bootstrap required schema", "error", err, "failures", op.Status.Failures, "requirement", sel.RequirementName) - err = errors.Wrapf(err, "cannot fetch bootstrap required schema for requirement %q", sel.RequirementName) + log.Debug("Cannot fetch bootstrap required schema", + "error", err, + "failures", op.Status.Failures, + "requirement", sel.RequirementName, + "apiVersion", ss.GetApiVersion(), + "kind", ss.GetKind(), + ) + err = errors.Wrapf(err, + "cannot fetch bootstrap required schema for requirement %q (%s/%s); ensure the CRD is installed and exposes an OpenAPI schema", + sel.RequirementName, ss.GetApiVersion(), ss.GetKind(), + ) r.record.Event(op, event.Warning(reasonBootstrapRequirements, err)) status.MarkConditions(xpv1.ReconcileError(err)) _ = r.client.Status().Update(ctx, op)internal/xfn/required_resources.go (1)
134-141: Make schema-fetch errors more operator-friendly. Could we add a quick remediation hint so users know what to check when schema discovery fails? Thanks! As per coding guidelines, ...💡 Suggested wording
- if err != nil { - return nil, errors.Wrapf(err, "fetching schema for %s", name) - } + if err != nil { + return nil, errors.Wrapf(err, "cannot fetch required schema %q; ensure the kind exists and OpenAPI discovery is accessible", name) + }internal/xfn/cached_discovery.go (1)
51-64: Consider improving error messages for operators (echoing prior feedback).The previous review raised a good point about making these errors more actionable for operators. When CRD informer setup fails, it can be confusing without context about what functionality is affected and what to check. Have you had a chance to consider the suggested wording from the earlier review?
🧹 Nitpick comments (3)
design/one-pager-required-schemas.md (1)
68-75: Consider a brief note about capability detection. Should we mention that functions can checkRequestMeta.capabilitiesforCAPABILITY_REQUIRED_SCHEMAS(and link to the capabilities one‑pager) to avoid confusion on older Crossplane versions? Thanks!internal/xfn/cached_discovery.go (1)
66-69: Question: Should we log the ignored error from RemoveEventHandler?I see we're discarding the error from
i.RemoveEventHandler(h). While this is in a cleanup path (context cancelled), I'm curious if there's a reason we wouldn't want to log this at debug level? It might help with troubleshooting edge cases. That said, I understand cleanup errors are often not actionable, so this is just a thought - happy to hear your reasoning!💭 Optional: Log cleanup error at debug level
go func() { <-ctx.Done() - _ = i.RemoveEventHandler(h) + if err := i.RemoveEventHandler(h); err != nil { + // Log at debug level - cleanup errors are usually not actionable + // but may help with troubleshooting. + } }()Note: This would require adding a logger to the struct if you decide it's worthwhile.
internal/controller/apiextensions/composite/composition_functions.go (1)
85-86: Error message could be more user-actionable.Per the coding guidelines, error messages should be meaningful to end users. The current message tells developers what failed but could help users understand what to check. Would something like this be helpful?
💭 Optional: More actionable error message
- errFmtFetchBootstrapSchemas = "cannot fetch bootstrap required schema for requirement %q" + errFmtFetchBootstrapSchemas = "cannot fetch OpenAPI schema for requirement %q; check that the resource kind exists in the cluster"That said, I understand keeping consistency with the
errFmtFetchBootstrapRequirementsmessage format might be preferred. What do you think?
|
Note E2Es are failing because they use a temporary image that I only built for arm64. I've tried to build a multi-platform image locally but I'm running into all kinds of obscure issues. I pinky promise the E2E test passes. 😄 Once we merge this, I can merge crossplane/function-sdk-python#193, cut a real function-python release with the new schema functionality, and update the E2E tests to use that. |
adamwg
left a comment
There was a problem hiding this comment.
LGTM. I think we should implement schema support in crank render before releasing this in v2.2, but fine with saving that for a separate PR.
internal/xfn/required_schemas.go
Outdated
| // x-kubernetes-group-version-kind annotation identifying what GVK it | ||
| // represents. Schemas shared across GVKs (e.g. DeleteOptions) are skipped. | ||
| for _, s := range doc.Components.Schemas { | ||
| gvks, ok := s["x-kubernetes-group-version-kind"].([]any) |
There was a problem hiding this comment.
I don't love all the type assertions we have to do here, but not sure there's a great alternative. We could use kube-openapi's Schema type (x-kubernetes-group-version-kind ends up in Extensions), but given that we need to end up with a map anyway to create the structpb.Struct it probably ends up being more complex.
| Embedded schemas are useful when function logic is baked in at build time - the | ||
| function author knows what types they're working with. Required schemas are | ||
| useful when function logic is provided dynamically at runtime, for example via | ||
| Composition input like function-kcl or function-python. These functions are | ||
| generic runtimes that can't know at build time what types users will reference. |
There was a problem hiding this comment.
Additionally, the presence of build-time schemas from the DevEx doesn't say anything about whether the types are present in the cluster where Crossplane is running or whether those types match the build-time schemas used to generate resources. A function could theoretically use build-time schemas to construct resources, and also use required schemas to validate that the constructed resources will apply correctly.
There was a problem hiding this comment.
Thanks for driving this @negz, it will be really helpful for functions to do more discovery about the objects they are using.
I took it for a test spin and one issue I see is that metadata is not fully included. What is passed over the wire is more of an unresolved reference $ref, e.g.:
"metadata":{
"description":"Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata",
"allOf":[{"$ref":"#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"}]
}
Would it be possible to include the object metadata schema too? I searched the full schema and that's the only place where I see a $ref, so hopefully this is the only "incomplete" schema ref that we'd need to handle.
Full schema for securitygroup.ec2.aws.m.upbound.io/v1beta1 from my function logs for reference
{"gvk": "ec2.aws.m.upbound.io/v1beta1, Kind=SecurityGroup", "OpenapiV3": {"description":"SecurityGroup is the Schema for the SecurityGroups API. Provides a security group resource.","type":"object","required":["spec"],"properties":{"apiVersion":{"description":"APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources","type":"string"},"kind":{"description":"Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds","type":"string"},"metadata":{"description":"Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata","allOf":[{"$ref":"#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"}]},"spec":{"description":"SecurityGroupSpec defines the desired state of SecurityGroup","type":"object","required":["forProvider"],"properties":{"forProvider":{"type":"object","required":["region"],"properties":{"description":{"description":"Security group description. Cannot be \"\". NOTE: This field maps to the AWS GroupDescription attribute, for which there is no Update API. If you'd like to classify your security groups in a way that can be updated, use tags.","type":"string"},"name":{"description":"Name of the security group.","type":"string"},"region":{"description":"Region where this resource will be managed. Defaults to the Region set in the provider configuration.\nRegion is the region you'd like your resource to be created in.","type":"string"},"revokeRulesOnDelete":{"description":"This is normally not needed, however certain AWS services such as Elastic Map Reduce may automatically add required rules to security groups used with the service, and those rules may contain a cyclic dependency that prevent the security groups from being destroyed without removing the dependency first. Default false.","type":"boolean"},"tags":{"description":"Key-value map of resource tags.","type":"object","additionalProperties":{"type":"string"},"x-kubernetes-map-type":"granular"},"vpcId":{"description":"VPC ID. Defaults to the region's default VPC.","type":"string"},"vpcIdRef":{"description":"Reference to a VPC in ec2 to populate vpcId.","type":"object","required":["name"],"properties":{"name":{"description":"Name of the referenced object.","type":"string"},"namespace":{"description":"Namespace of the referenced object","type":"string"},"policy":{"description":"Policies for referencing.","type":"object","properties":{"resolution":{"description":"Resolution specifies whether resolution of this reference is required.\nThe default is 'Required', which means the reconcile will fail if the\nreference cannot be resolved. 'Optional' means this reference will be\na no-op if it cannot be resolved.","type":"string","default":"Required","enum":["Required","Optional"]},"resolve":{"description":"Resolve specifies when this reference should be resolved. The default\nis 'IfNotPresent', which will attempt to resolve the reference only when\nthe corresponding field is not present. Use 'Always' to resolve the\nreference on every reconcile.","type":"string","enum":["Always","IfNotPresent"]}}}}},"vpcIdSelector":{"description":"Selector for a VPC in ec2 to populate vpcId.","type":"object","properties":{"matchControllerRef":{"description":"MatchControllerRef ensures an object with the same controller reference\nas the selecting object is selected.","type":"boolean"},"matchLabels":{"description":"MatchLabels ensures an object with matching labels is selected.","type":"object","additionalProperties":{"type":"string"}},"namespace":{"description":"Namespace for the selector","type":"string"},"policy":{"description":"Policies for selection.","type":"object","properties":{"resolution":{"description":"Resolution specifies whether resolution of this reference is required.\nThe default is 'Required', which means the reconcile will fail if the\nreference cannot be resolved. 'Optional' means this reference will be\na no-op if it cannot be resolved.","type":"string","default":"Required","enum":["Required","Optional"]},"resolve":{"description":"Resolve specifies when this reference should be resolved. The default\nis 'IfNotPresent', which will attempt to resolve the reference only when\nthe corresponding field is not present. Use 'Always' to resolve the\nreference on every reconcile.","type":"string","enum":["Always","IfNotPresent"]}}}}}}},"initProvider":{"description":"THIS IS A BETA FIELD. It will be honored\nunless the Management Policies feature flag is disabled.\nInitProvider holds the same fields as ForProvider, with the exception\nof Identifier and other resource reference fields. The fields that are\nin InitProvider are merged into ForProvider when the resource is created.\nThe same fields are also added to the terraform ignore_changes hook, to\navoid updating them after creation. This is useful for fields that are\nrequired on creation, but we do not desire to update them after creation,\nfor example because of an external controller is managing them, like an\nautoscaler.","type":"object","properties":{"description":{"description":"Security group description. Cannot be \"\". NOTE: This field maps to the AWS GroupDescription attribute, for which there is no Update API. If you'd like to classify your security groups in a way that can be updated, use tags.","type":"string"},"name":{"description":"Name of the security group.","type":"string"},"revokeRulesOnDelete":{"description":"This is normally not needed, however certain AWS services such as Elastic Map Reduce may automatically add required rules to security groups used with the service, and those rules may contain a cyclic dependency that prevent the security groups from being destroyed without removing the dependency first. Default false.","type":"boolean"},"tags":{"description":"Key-value map of resource tags.","type":"object","additionalProperties":{"type":"string"},"x-kubernetes-map-type":"granular"},"vpcId":{"description":"VPC ID. Defaults to the region's default VPC.","type":"string"},"vpcIdRef":{"description":"Reference to a VPC in ec2 to populate vpcId.","type":"object","required":["name"],"properties":{"name":{"description":"Name of the referenced object.","type":"string"},"namespace":{"description":"Namespace of the referenced object","type":"string"},"policy":{"description":"Policies for referencing.","type":"object","properties":{"resolution":{"description":"Resolution specifies whether resolution of this reference is required.\nThe default is 'Required', which means the reconcile will fail if the\nreference cannot be resolved. 'Optional' means this reference will be\na no-op if it cannot be resolved.","type":"string","default":"Required","enum":["Required","Optional"]},"resolve":{"description":"Resolve specifies when this reference should be resolved. The default\nis 'IfNotPresent', which will attempt to resolve the reference only when\nthe corresponding field is not present. Use 'Always' to resolve the\nreference on every reconcile.","type":"string","enum":["Always","IfNotPresent"]}}}}},"vpcIdSelector":{"description":"Selector for a VPC in ec2 to populate vpcId.","type":"object","properties":{"matchControllerRef":{"description":"MatchControllerRef ensures an object with the same controller reference\nas the selecting object is selected.","type":"boolean"},"matchLabels":{"description":"MatchLabels ensures an object with matching labels is selected.","type":"object","additionalProperties":{"type":"string"}},"namespace":{"description":"Namespace for the selector","type":"string"},"policy":{"description":"Policies for selection.","type":"object","properties":{"resolution":{"description":"Resolution specifies whether resolution of this reference is required.\nThe default is 'Required', which means the reconcile will fail if the\nreference cannot be resolved. 'Optional' means this reference will be\na no-op if it cannot be resolved.","type":"string","default":"Required","enum":["Required","Optional"]},"resolve":{"description":"Resolve specifies when this reference should be resolved. The default\nis 'IfNotPresent', which will attempt to resolve the reference only when\nthe corresponding field is not present. Use 'Always' to resolve the\nreference on every reconcile.","type":"string","enum":["Always","IfNotPresent"]}}}}}}},"managementPolicies":{"description":"THIS IS A BETA FIELD. It is on by default but can be opted out\nthrough a Crossplane feature flag.\nManagementPolicies specify the array of actions Crossplane is allowed to\ntake on the managed and external resources.\nSee the design doc for more information: https://github.com/crossplane/crossplane/blob/499895a25d1a1a0ba1604944ef98ac7a1a71f197/design/design-doc-observe-only-resources.md?plain=1#L223\nand this one: https://github.com/crossplane/crossplane/blob/444267e84783136daa93568b364a5f01228cacbe/design/one-pager-ignore-changes.md","type":"array","default":["*"],"items":{"description":"A ManagementAction represents an action that the Crossplane controllers\ncan take on an external resource.","type":"string","enum":["Observe","Create","Update","Delete","LateInitialize","*"]}},"providerConfigRef":{"description":"ProviderConfigReference specifies how the provider that will be used to\ncreate, observe, update, and delete this managed resource should be\nconfigured.","type":"object","default":{"kind":"ClusterProviderConfig","name":"default"},"required":["kind","name"],"properties":{"kind":{"description":"Kind of the referenced object.","type":"string"},"name":{"description":"Name of the referenced object.","type":"string"}}},"writeConnectionSecretToRef":{"description":"WriteConnectionSecretToReference specifies the namespace and name of a\nSecret to which any connection details for this managed resource should\nbe written. Connection details frequently include the endpoint, username,\nand password required to connect to the managed resource.","type":"object","required":["name"],"properties":{"name":{"description":"Name of the secret.","type":"string"}}}}},"status":{"description":"SecurityGroupStatus defines the observed state of SecurityGroup.","type":"object","properties":{"atProvider":{"type":"object","properties":{"arn":{"description":"ARN of the security group.","type":"string"},"description":{"description":"Security group description. Cannot be \"\". NOTE: This field maps to the AWS GroupDescription attribute, for which there is no Update API. If you'd like to classify your security groups in a way that can be updated, use tags.","type":"string"},"egress":{"description":"Configuration block for egress rules. Can be specified multiple times for each egress rule. Each egress block supports fields documented below. This argument is processed in attribute-as-blocks mode.","type":"array","items":{"type":"object","properties":{"cidrBlocks":{"description":"List of CIDR blocks.","type":"array","items":{"type":"string"}},"description":{"description":"Security group description. Cannot be \"\". NOTE: This field maps to the AWS GroupDescription attribute, for which there is no Update API. If you'd like to classify your security groups in a way that can be updated, use tags.","type":"string"},"fromPort":{"description":"Start port (or ICMP type number if protocol is icmp or icmpv6).","type":"number"},"ipv6CidrBlocks":{"description":"List of IPv6 CIDR blocks.","type":"array","items":{"type":"string"}},"prefixListIds":{"description":"List of Prefix List IDs.","type":"array","items":{"type":"string"}},"protocol":{"description":"Protocol. If you select a protocol of -1 (semantically equivalent to all, which is not a valid value here), you must specify a from_port and to_port equal to 0. The supported values are defined in the IpProtocol argument on the IpPermission API reference.12.","type":"string"},"securityGroups":{"description":"List of security groups. A group name can be used relative to the default VPC. Otherwise, group ID.","type":"array","items":{"type":"string"},"x-kubernetes-list-type":"set"},"self":{"description":"Whether the security group itself will be added as a source to this ingress rule.","type":"boolean"},"toPort":{"description":"End range port (or ICMP code if protocol is icmp).","type":"number"}}}},"id":{"description":"ID of the security group.","type":"string"},"ingress":{"description":"Configuration block for ingress rules. Can be specified multiple times for each ingress rule. Each ingress block supports fields documented below. This argument is processed in attribute-as-blocks mode.","type":"array","items":{"type":"object","properties":{"cidrBlocks":{"description":"List of CIDR blocks.","type":"array","items":{"type":"string"}},"description":{"description":"Security group description. Cannot be \"\". NOTE: This field maps to the AWS GroupDescription attribute, for which there is no Update API. If you'd like to classify your security groups in a way that can be updated, use tags.","type":"string"},"fromPort":{"description":"Start port (or ICMP type number if protocol is icmp or icmpv6).","type":"number"},"ipv6CidrBlocks":{"description":"List of IPv6 CIDR blocks.","type":"array","items":{"type":"string"}},"prefixListIds":{"description":"List of Prefix List IDs.","type":"array","items":{"type":"string"}},"protocol":{"description":"Protocol. If you select a protocol of -1 (semantically equivalent to all, which is not a valid value here), you must specify a from_port and to_port equal to 0. The supported values are defined in the IpProtocol argument on the IpPermission API reference.12.","type":"string"},"securityGroups":{"description":"List of security groups. A group name can be used relative to the default VPC. Otherwise, group ID.","type":"array","items":{"type":"string"},"x-kubernetes-list-type":"set"},"self":{"description":"Whether the security group itself will be added as a source to this ingress rule.","type":"boolean"},"toPort":{"description":"End range port (or ICMP code if protocol is icmp).","type":"number"}}}},"name":{"description":"Name of the security group.","type":"string"},"ownerId":{"description":"Owner ID.","type":"string"},"region":{"description":"Region where this resource will be managed. Defaults to the Region set in the provider configuration.\nRegion is the region you'd like your resource to be created in.","type":"string"},"revokeRulesOnDelete":{"description":"This is normally not needed, however certain AWS services such as Elastic Map Reduce may automatically add required rules to security groups used with the service, and those rules may contain a cyclic dependency that prevent the security groups from being destroyed without removing the dependency first. Default false.","type":"boolean"},"tags":{"description":"Key-value map of resource tags.","type":"object","additionalProperties":{"type":"string"},"x-kubernetes-map-type":"granular"},"tagsAll":{"description":"A map of tags assigned to the resource, including those inherited from the provider default_tags configuration block.","type":"object","additionalProperties":{"type":"string"},"x-kubernetes-map-type":"granular"},"vpcId":{"description":"VPC ID. Defaults to the region's default VPC.","type":"string"}}},"conditions":{"description":"Conditions of the resource.","type":"array","items":{"description":"A Condition that may apply to a resource.","type":"object","required":["lastTransitionTime","reason","status","type"],"properties":{"lastTransitionTime":{"description":"LastTransitionTime is the last time this condition transitioned from one\nstatus to another.","type":"string","format":"date-time"},"message":{"description":"A Message containing details about this condition's last transition from\none status to another, if any.","type":"string"},"observedGeneration":{"description":"ObservedGeneration represents the .metadata.generation that the condition was set based upon.\nFor instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date\nwith respect to the current state of the instance.","type":"integer","format":"int64"},"reason":{"description":"A Reason for this condition's last transition from one status to another.","type":"string"},"status":{"description":"Status of this condition; is it currently True, False, or Unknown?","type":"string"},"type":{"description":"Type of this condition. At most one of each condition type may apply to\na resource at any point in time.","type":"string"}}},"x-kubernetes-list-map-keys":["type"],"x-kubernetes-list-type":"map"},"observedGeneration":{"description":"ObservedGeneration is the latest metadata.generation\nwhich resulted in either a ready state, or stalled due to error\nit can not recover from without human intervention.","type":"integer","format":"int64"}}}},"x-kubernetes-group-version-kind":[{"group":"ec2.aws.m.upbound.io","kind":"SecurityGroup","version":"v1beta1"}]}}
edit: I was able to use compile time schemas for ObjectMeta successfully in my test function, but Crossplane returning complete schemas is the ideal solution here 😇
I've updated it to include all referenced components (e.g. ObjectMeta) in the requested schema. They're not inlined - the |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/xfn/required_schemas_test.go`:
- Around line 648-661: TestNopRequiredSchemasFetcher is not table‑driven;
refactor it into a table-driven test that defines input args (use a struct with
ctx and *fnv1.SchemaSelector) and expected want (expected schema and expected
error), iterate cases and call NopRequiredSchemasFetcher{}.Fetch, and compare
results using cmp.Diff with cmpopts.EquateErrors() to assert error equality and
compare the returned schema (e.g., schema.GetOpenapiV3()) against the expected
value; reference the NopRequiredSchemasFetcher type and its Fetch method and use
cmp.Diff in assertions instead of direct t.Errorf checks.
In `@internal/xfn/required_schemas.go`:
- Around line 86-169: The error messages returned by OpenAPISchemasFetcher.Fetch
(e.g., the errors.Wrapf calls around schema.ParseGroupVersion, f.client.Paths(),
client.Schema, json.Unmarshal, and structpb.NewStruct) are developer‑centric;
update each wrapped error string to be user‑facing, include brief context and a
suggested next step (for example: explain what input failed like "invalid
apiVersion", advise checking the resource apiVersion or cluster OpenAPI
availability, or suggest retrying/validating the manifest), and preserve the
underlying error when wrapping. Locate these messages in the Fetch method and
revise strings such as "cannot parse apiVersion %q", "cannot get OpenAPI paths",
"cannot get OpenAPI schema for %s", "cannot parse OpenAPI schema for %s", and
"cannot convert schema to protobuf Struct" to concise user-friendly forms that
include actionable guidance and keep the original error wrapped.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/crank/render/schemas.go`:
- Around line 55-77: Update the three error messages that surface to CLI users
so they include the path and a brief next-step hint: when wrapping fs.Stat
errors (the errors.Wrap call after fs.Stat(fileOrDir)) include fileOrDir and a
suggestion like "check the path exists and is readable"; when wrapping
afero.ReadDir errors (the errors.Wrap after afero.ReadDir) include fileOrDir and
a hint to check directory permissions; and when returning errors.Errorf("no JSON
files found in %q", fileOrDir) include the directory path plus a suggestion such
as "ensure the directory contains .json OpenAPI files or point to a file". Use
the existing error helpers (errors.Wrap / errors.Errorf) but expand the messages
to be user-facing and include fileOrDir for context.
🧹 Nitpick comments (1)
cmd/crank/render/schemas_test.go (1)
232-335: Usecmpopts.AnyErrorwithcmp.Difffor cleaner error assertions.Your test structure is solid! To align with established patterns in the codebase, consider using the standard
cmpopts.AnyErrorsentinel for error-checking cases:🧪 Refined pattern
import ( "context" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/spf13/afero" "google.golang.org/protobuf/testing/protocmp" ) type args struct { fs afero.Fs file string } type want struct { count int + err error } cases := map[string]struct { reason string args args want want }{ "SingleFile": { reason: "Should load a single JSON file", args: args{ fs: func() afero.Fs { fs := afero.NewMemMapFs() _ = afero.WriteFile(fs, "/schemas/apps-v1.json", []byte(deploymentJSON), 0o644) return fs }(), file: "/schemas/apps-v1.json", }, want: want{ count: 1, + err: nil, }, }, // ... "FileNotFound": { reason: "Should return error for non-existent file", args: args{ fs: afero.NewMemMapFs(), file: "/does-not-exist.json", }, want: want{ + err: cmpopts.AnyError, }, }, "InvalidJSON": { reason: "Should return error for invalid JSON", args: args{ fs: func() afero.Fs { fs := afero.NewMemMapFs() _ = afero.WriteFile(fs, "/bad.json", []byte("not valid json"), 0o644) return fs }(), file: "/bad.json", }, want: want{ + err: cmpopts.AnyError, }, }, "EmptyDirectory": { reason: "Should return error for empty directory", args: args{ fs: func() afero.Fs { fs := afero.NewMemMapFs() _ = fs.MkdirAll("/empty", 0o755) return fs }(), file: "/empty", }, want: want{ + err: cmpopts.AnyError, }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { schemas, err := LoadRequiredSchemas(tc.args.fs, tc.args.file) + + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("\n%s\nLoadRequiredSchemas(...): unexpected error diff (-want,+got):\n%s", tc.reason, diff) + return + } - if tc.want.err { - if err == nil { - t.Errorf("\n%s\nLoadRequiredSchemas(...): expected error, got nil", tc.reason) - } - return - } - - if err != nil { - t.Errorf("\n%s\nLoadRequiredSchemas(...): unexpected error: %v", tc.reason, err) - return - } if len(schemas) != tc.want.count { t.Errorf("\n%s\nLoadRequiredSchemas(...): expected %d schemas, got %d", tc.reason, tc.want.count, len(schemas)) }This uses
cmpopts.AnyErroras a sentinel—a standard pattern across Crossplane tests. The singlecmp.Diffcall withcmpopts.EquateErrors()handles all error cases cleanly and consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/crank/alpha/render/op/cmd.go`:
- Around line 53-54: The predictor "json_file_or_directory" used on the
RequiredSchemas field is not registered, causing completions to fail; either
register a new predictor named "json_file_or_directory" in the predictors map
inside cmd/crank/completion/completion.go (implement it the same way as
yaml_file_or_directory but matching JSON semantics), or change the predictor on
RequiredSchemas in cmd/crank/alpha/render/op/cmd.go to an existing registered
predictor such as "yaml_file_or_directory", "file", or "file_or_directory" to
match the stable cmd/crank/render/cmd.go pattern so completions work.
In `@cmd/crank/render/schemas.go`:
- Around line 126-141: The loop over doc.Components.Schemas currently rejects
any schema whose "x-kubernetes-group-version-kind" array length is not exactly
1; update the logic in that loop (the variables gvks, gvk, g, v, k and the
comparisons against gv.Group, gv.Version and ss.GetKind()) to iterate over all
entries in gvks instead of checking len(gvks) == 1, attempt to cast each entry
to map[string]any, extract group/version/kind for each, and accept the schema
when any entry matches (use strings.EqualFold for group/version and k ==
ss.GetKind() for kind) so schemas annotated with multiple GVKs are matched
correctly.
🧹 Nitpick comments (1)
cmd/crank/render/render.go (1)
305-305: Consider using RenderCapabilities() for clarity.Would it be clearer to call
xfn.RenderCapabilities()here so render-specific capabilities can diverge later without touching call sites?♻️ Suggested tweak
- req.Meta = &fnv1.RequestMeta{Capabilities: xfn.SupportedCapabilities()} + req.Meta = &fnv1.RequestMeta{Capabilities: xfn.RenderCapabilities()}
jbw976
left a comment
There was a problem hiding this comment.
I took the latest in this PR for a test spin tonight and the kube-openapi package I'm using for schemas doesn't appear to have or understand the Components field, so it doesn't resolve the $refs to their actual schemas from the Components section:
https://pkg.go.dev/k8s.io/[email protected]/pkg/validation/spec#Schema
Is it unreasonable for Crossplane to do this resolution and pass along fully resolved schemas to the functions, so the each function doesn't have to do any additional processing on their own? What do you think @negz?
I can look into it. I assumed there wasn't a need - it seems strange that an OAPI SDK wouldn't support this. We're sending through basically the same format the API server would send clients. |
|
@negz checkout https://github.com/kubernetes/apiserver/blob/master/pkg/cel/openapi/resolver/refs.go#L30 |
Composition functions sometimes need OpenAPI schemas for resource kinds, for example to validate resources or understand field types. Functions can't fetch schemas directly since they cannot assume network access. This commit extends the existing required resources pattern to support schemas. Functions return requirements.schemas specifying GVKs they need, and Crossplane populates required_schemas on the next request. The implementation adds: - Proto messages SchemaSelector and Schema, plus new fields on Requirements and RunFunctionRequest - OpenAPISchemasFetcher that fetches schemas via the Kubernetes discovery API, looking up schemas by x-kubernetes-group-version-kind annotation - CachedOpenAPIClient wrapper that invalidates the discovery cache when CRDs are created, updated, or deleted - Extension of FetchingFunctionRunner to fetch schemas alongside resources in the requirements iteration loop Schemas shared across multiple GVKs (like DeleteOptions) are skipped since functions won't realistically request these meta types. If a requested schema isn't found, Crossplane returns an empty Schema to indicate it tried, matching the pattern used for required resources. Signed-off-by: Nic Cope <[email protected]>
The previous commit added support for composition functions to request OpenAPI schemas dynamically via requirements.schemas. However, platform operators may want to pre-populate schemas before a function runs, avoiding an extra function call roundtrip. This commit adds RequiredSchemas to the FunctionRequirements type in both Composition and Operation APIs. These "bootstrap" schemas are fetched before the first function call and included in the initial RunFunctionRequest, alongside any bootstrap required resources. To enable this, the commit plumbs OpenAPIV3Client through controller options. Both the apiextensions and ops controllers receive the shared CachedOpenAPIClient created in core.go, ensuring all schema fetching shares a single memory-cached discovery client. Each controller creates its own OpenAPISchemasFetcher from this client. The ToProtobufSchemaSelector helper converts API types to the protobuf SchemaSelector used by the fetcher. Signed-off-by: Nic Cope <[email protected]>
Composition functions can request OpenAPI schemas for resource kinds via the requirements.schemas field in their response. Crossplane then provides the requested schemas in the required_schemas field of subsequent RunFunctionRequest messages. This commit adds an E2E test that verifies the end-to-end flow works correctly. The test uses a Python function that requests the ConfigMap schema, verifies the schema is received, and checks that it contains expected properties (data, binaryData). The XR status fields reflect whether the schema was successfully received and validated. The test currently uses a temporary function-python build that includes SDK support for required schemas. A TODO marks this for replacement once an official release is available. Signed-off-by: Nic Cope <[email protected]>
Composition functions can request existing cluster resources via the requirements.resources field in their response. Crossplane fetches the requested resources and provides them in the required_resources field of subsequent RunFunctionRequest messages. This commit adds an E2E test that verifies the end-to-end flow. The test creates a ConfigMap, then uses a Python function to request it by name. The function verifies the ConfigMap is received and contains the expected data, setting XR status fields to reflect the results. Signed-off-by: Nic Cope <[email protected]>
Functions have no way to know which protocol features Crossplane supports. When a function uses a newer feature like requirements.schemas with an older Crossplane, Crossplane silently ignores the unknown fields. The function can't distinguish "Crossplane fetched the schema but found nothing" from "Crossplane doesn't understand schema requests". This commit adds a Capability enum and capabilities field to RequestMeta. Crossplane populates the field with all features it supports when calling functions. Functions check for a capability before relying on the corresponding feature, falling back gracefully when absent. CAPABILITY_CAPABILITIES serves as a bootstrap: its presence tells functions that Crossplane advertises capabilities, so absent capabilities indicate lack of support rather than an old Crossplane that predates advertisement. See design/one-pager-function-capabilities.md for alternatives considered. Signed-off-by: Nic Cope <[email protected]>
When composition functions request OpenAPI schemas via the required schemas mechanism, the returned schema contains $ref pointers like "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta". However, the referenced schemas aren't included, so functions can't resolve these references. Kubernetes OpenAPI v3 endpoints return self-contained documents with all referenced schemas bundled in the components.schemas section. This commit updates the schema fetcher to recursively collect all $ref dependencies from the requested schema and include them in a components.schemas section of the response. Signed-off-by: Nic Cope <[email protected]>
No need to test a nop implementation. Signed-off-by: Nic Cope <[email protected]>
The render CLI used NopRequiredSchemasFetcher, which returns empty schemas for all requests. Functions that need OpenAPI schemas to validate or default resources couldn't work with render. This commit adds a --required-schemas flag to both `crossplane render` and `crossplane alpha render op`. Users can provide JSON files obtained from `kubectl get --raw /openapi/v3/<group-version>` either as a single file or a directory of files. The new FilteringSchemaFetcher searches the provided documents for matching GVKs and includes referenced schemas (via $ref) in the result. RenderCapabilities() now returns all supported capabilities including CAPABILITY_REQUIRED_SCHEMAS. Signed-off-by: Nic Cope <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/crank/render/render.go (1)
291-305:⚠️ Potential issue | 🟠 MajorBootstrap required schemas aren’t populated in render requests
Inside the bootstrap requirements block we only populate
req.RequiredResources. If Composition steps can now declare required schemas,crank renderwill send the first request without them. Should we mirror controller behavior by fetching and settingreq.RequiredSchemashere as well?💡 Possible fix (adjust selector helper/type names if they differ)
if fn.Requirements != nil { // Bootstrap requirements were introduced alongside the new field names, // so we only need to support the new required_resources field. req.RequiredResources = map[string]*fnv1.Resources{} for _, sel := range fn.Requirements.RequiredResources { resources, err := NewFilteringFetcher(in.RequiredResources...).Fetch(ctx, xfn.ToProtobufResourceSelector(&sel)) if err != nil { return Outputs{}, errors.Wrapf(err, "cannot fetch bootstrap required resources for requirement %q", sel.RequirementName) } req.RequiredResources[sel.RequirementName] = resources } + + req.RequiredSchemas = map[string]*fnv1.Schema{} + for _, sel := range fn.Requirements.RequiredSchemas { + schema, err := NewFilteringSchemaFetcher(in.RequiredSchemas).Fetch(ctx, xfn.ToProtobufSchemaSelector(&sel)) + if err != nil { + return Outputs{}, errors.Wrapf(err, "cannot fetch bootstrap required schemas for requirement %q", sel.RequirementName) + } + req.RequiredSchemas[sel.RequirementName] = schema + } }
🤖 Fix all issues with AI agents
In `@cmd/crank/alpha/render/op/render.go`:
- Line 82: The bootstrap path currently preloads only RequiredResources so steps
with requirements.requiredSchemas miss schemas; mirror the resources preload for
schemas by adding the same bootstrap pre-population logic for in.RequiredSchemas
into the schema fetcher used by NewFetchingFunctionRunner. Locate the bootstrap
block that seeds RequiredResources and add equivalent logic that injects or
preloads schemas into render.NewFilteringSchemaFetcher (referencing
in.RequiredSchemas and render.NewFilteringSchemaFetcher) so the runner
(NewFetchingFunctionRunner) receives preloaded schemas the same way it does
resources.
🧹 Nitpick comments (2)
design/one-pager-function-capabilities.md (1)
113-116: Minor grammar nit (optional).Static analysis suggests "unknown field" could be hyphenated as "unknown-field" when used as a compound adjective before "preservation." This is a very minor style point—feel free to skip if you prefer the current phrasing.
Functions could request a feature (e.g. schemas), then check whether Crossplane populated the response on the next iteration. This works but requires an extra -round-trip, complicates function logic, and relies on subtle proto unknown field +round-trip, complicates function logic, and relies on subtle proto unknown-field preservation semantics.internal/xfn/required_schemas.go (1)
83-148: Validate schema selectorkindto avoid silent misconfig.If
ss.GetKind()is empty, the fetcher currently returns an empty schema (same as “not found”), which can mask a bad selector. Should we treat missingkindas invalid input and return a clear error instead? Happy to align with intended behavior.🧭 Suggested validation
func (f *OpenAPISchemasFetcher) Fetch(_ context.Context, ss *fnv1.SchemaSelector) (*fnv1.Schema, error) { if ss == nil { return nil, errors.New("you must specify a schema selector") } + if ss.GetKind() == "" { + return nil, errors.New("schema selector kind is required to fetch an OpenAPI schema") + } gv, err := schema.ParseGroupVersion(ss.GetApiVersion())
| }() | ||
|
|
||
| runner := xfn.NewFetchingFunctionRunner(runtimes, render.NewFilteringFetcher(in.RequiredResources...)) | ||
| runner := xfn.NewFetchingFunctionRunner(runtimes, render.NewFilteringFetcher(in.RequiredResources...), render.NewFilteringSchemaFetcher(in.RequiredSchemas)) |
There was a problem hiding this comment.
Bootstrap required schemas aren’t preloaded for render.
Thanks for wiring the schema fetcher into the runner. One gap: the bootstrap requirements block only pre-populates RequiredResources, so a pipeline step with requirements.requiredSchemas won’t receive schemas during crank alpha render, even if Inputs.RequiredSchemas is provided. Was this intentional? If not, consider mirroring the resource path and preloading schemas too.
💡 Possible fix (reuse a shared schema fetcher)
- runner := xfn.NewFetchingFunctionRunner(runtimes, render.NewFilteringFetcher(in.RequiredResources...), render.NewFilteringSchemaFetcher(in.RequiredSchemas))
+ schemaFetcher := render.NewFilteringSchemaFetcher(in.RequiredSchemas)
+ runner := xfn.NewFetchingFunctionRunner(runtimes, render.NewFilteringFetcher(in.RequiredResources...), schemaFetcher)
@@
if fn.Requirements != nil {
req.RequiredResources = map[string]*fnv1.Resources{}
for _, sel := range fn.Requirements.RequiredResources {
resources, err := render.NewFilteringFetcher(in.RequiredResources...).Fetch(ctx, xfn.ToProtobufResourceSelector(&sel))
if err != nil {
return Outputs{}, errors.Wrapf(err, "cannot fetch bootstrap required resources for requirement %q", sel.RequirementName)
}
req.RequiredResources[sel.RequirementName] = resources
}
+
+ req.RequiredSchemas = map[string]*fnv1.Schema{}
+ for _, sel := range fn.Requirements.RequiredSchemas {
+ schema, err := schemaFetcher.Fetch(ctx, xfn.ToProtobufSchemaSelector(&sel))
+ if err != nil {
+ return Outputs{}, errors.Wrapf(err, "cannot fetch bootstrap required schema for requirement %q", sel.RequirementName)
+ }
+ req.RequiredSchemas[sel.RequirementName] = schema
+ }
}🤖 Prompt for AI Agents
In `@cmd/crank/alpha/render/op/render.go` at line 82, The bootstrap path currently
preloads only RequiredResources so steps with requirements.requiredSchemas miss
schemas; mirror the resources preload for schemas by adding the same bootstrap
pre-population logic for in.RequiredSchemas into the schema fetcher used by
NewFetchingFunctionRunner. Locate the bootstrap block that seeds
RequiredResources and add equivalent logic that injects or preloads schemas into
render.NewFilteringSchemaFetcher (referencing in.RequiredSchemas and
render.NewFilteringSchemaFetcher) so the runner (NewFetchingFunctionRunner)
receives preloaded schemas the same way it does resources.
| return nil, errors.Wrap(err, "cannot stat file") | ||
| } | ||
|
|
||
| if info.IsDir() { |
There was a problem hiding this comment.
Should probably walk this dir recursively.
When functions request OpenAPI schemas via the required schemas mechanism, referenced schemas like ObjectMeta were included in a separate components.schemas section. Functions had to resolve $ref pointers themselves to access the full schema structure. This commit uses kube-openapi's PopulateRefs to flatten all $ref references inline before returning schemas to functions. Referenced schemas are now embedded directly at their use sites rather than collected in a separate section. Circular references are replaced with empty object schemas per PopulateRefs behavior. This simplifies function implementations since they no longer need to resolve references. The tradeoff is increased schema size when the same type is referenced multiple times (e.g., ObjectMeta appearing in both Deployment and Pod template metadata). Signed-off-by: Nic Cope <[email protected]>
Description of your changes
Composition functions sometimes need OpenAPI schemas for resource kinds - for example to validate resources, generate resources with correct field types, or build schema-aware tooling. Since functions can't assume network access per the function specification, they have no way to fetch schemas directly.
This PR extends the existing required resources pattern to support schemas. Functions return
requirements.schemasspecifying GVKs they need, and Crossplane populatesrequired_schemason the next request. If a schema isn't found, Crossplane returns an emptySchema{}to indicate it tried, matching the pattern used for required resources.This PR also adds capability advertisement. Functions have no way to know which protocol features Crossplane supports - when a function uses a newer feature with an older Crossplane, the request is silently ignored. Crossplane now populates
meta.capabilitieswith all supported features. Functions can check for a capability before relying on a feature, and fall back gracefully when absent. Seedesign/one-pager-function-capabilities.mdfor details.I have:
earthly +reviewableto ensure this PR is ready for review.Addedbackport release-x.ylabels to auto-backport this PR.Followed the API promotion workflow if this PR introduces, removes, or promotes an API.