diff --git a/.claude/skills/audit-patches/SKILL.md b/.claude/skills/audit-patches/SKILL.md new file mode 100644 index 0000000..d172f1a --- /dev/null +++ b/.claude/skills/audit-patches/SKILL.md @@ -0,0 +1,202 @@ +--- +name: audit-patches +description: Audit function-kro's patches documentation against the actual upstream KRO diff. Validates that v*_PATCHES.md accurately documents all modifications, additions, and exclusions. +arguments: [upstream-tag] +!command: ./scripts/diff-upstream-kro.sh -s -r $ARGUMENTS +--- + +# Audit Patches Skill + +**STOP CHECK:** If "$ARGUMENTS" is empty or was not provided, do NOT proceed. Tell the user: "This skill requires an upstream KRO tag. Usage: `/audit-patches v0.8.3`" and stop immediately. + +You are auditing function-kro's patches documentation against the actual upstream KRO code. The diff summary output from `./scripts/diff-upstream-kro.sh -s -r $ARGUMENTS` has been pre-injected above this prompt. + +**Your job:** Verify that the patches documentation (`patches/v*_PATCHES.md`) accurately and completely describes every difference between our code and upstream KRO at the specified tag. + +## ARGUMENTS + +The user provides an upstream KRO git tag (e.g., `v0.8.3`). This was already passed to the diff script. Use this same tag throughout. + +--- + +## Phase 1: Parse Diff Results + +From the pre-injected diff summary output, extract three lists: + +1. **MODIFIED files** — files that differ from upstream (after import path normalization) +2. **LOCAL ONLY files** — files that exist only in function-kro (our additions) +3. **UPSTREAM ONLY files** — files that exist only in upstream (we intentionally exclude them) + +Also extract the summary counts (Identical, Modified, Local only, Upstream only). + +Record these lists — they are the source of truth for the rest of the audit. + +--- + +## Phase 2: Find and Read Patches Doc + +1. Glob for `patches/v*_PATCHES.md` to find the matching patches document. +2. Read the entire patches document. +3. Note the document's structure: which sections exist, what files are listed where, what counts are claimed. + +--- + +## Phase 3: Cross-Reference Coverage + +Check each category systematically: + +### 3a. Modified Files Coverage + +For every `[MODIFIED]` file from Phase 1: +- Is it documented in the patches doc with a dedicated section or table entry? +- If not, flag it as **MISSING: undocumented modification**. + +### 3b. Local-Only Files Coverage + +For every `[LOCAL ONLY]` file from Phase 1: +- Is it listed in the "Files Added" table (or equivalent section)? +- If not, flag it as **MISSING: undocumented local-only file**. + +### 3c. Upstream-Only Files Coverage + +For every `[UPSTREAM ONLY]` file from Phase 1: +- Is it listed in the "Files Excluded" table (or equivalent section)? +- If not, flag it as **MISSING: undocumented upstream-only exclusion**. + +### 3d. Phantom Entries + +For every file the patches doc claims is modified: +- Does it actually appear as `[MODIFIED]` in the diff output? +- If the diff shows it as `[IDENTICAL]`, flag it as **PHANTOM: doc claims modification but file is identical to upstream**. + +### 3e. Summary Counts + +Compare the patches doc's claimed counts (e.g., "9 modified files") against the actual diff summary counts. Flag any mismatches. + +--- + +## Phase 4: Deep-Dive Modified Files + +This is the most important phase. For each `[MODIFIED]` file: + +### 4.0 Clone Upstream Once + +Clone upstream to a reusable location so per-file diffs don't each trigger a fresh clone: + +```bash +AUDIT_DIR="/tmp/kro-audit-$ARGUMENTS" +if [ ! -d "$AUDIT_DIR" ]; then + git clone --depth 1 --branch $ARGUMENTS https://github.com/kubernetes-sigs/kro.git "$AUDIT_DIR" +fi +``` + +### 4.1 Get the Full Diff + +For each modified file, run: + +```bash +./scripts/diff-upstream-kro.sh -f -u "$AUDIT_DIR" -l 0 +``` + +The `-u` flag reuses the existing clone. The `-l 0` flag shows the complete diff without truncation. + +### 4.2 Assess Documentation Quality + +For each modified file, evaluate: + +1. **Accuracy** — Does the patches doc correctly describe what was changed? Are the described modifications actually present in the diff? +2. **Completeness** — Does the doc capture ALL modifications shown in the diff, or are some changes undocumented? +3. **Gaps** — Are there significant diff hunks that the doc doesn't mention at all? +4. **Quality** — Are the descriptions clear enough that an engineer could re-apply these changes to a new upstream version? + +Record your findings per file. + +--- + +## Phase 5: Quality and Effectiveness Assessment + +### 5a. Quick Reference Tables + +Check that the "Quick Reference" tables at the bottom of the patches doc are complete and accurate: +- Do they list all modified files with correct adaptation summaries? +- Do they list all local-only files? +- Do they list all upstream-only exclusions with reasons? + +### 5b. Upgrade Readiness + +Ask: If an agent were following `UPGRADE_PROCESS.md` Phase 3 (Re-Apply Adaptations), using this patches doc as their guide, would they: +- Know every file that needs modification? +- Understand the intent of each adaptation well enough to re-apply it to changed upstream code? +- Know which files to add back that aren't in upstream? +- Know which upstream files to intentionally skip? + +Flag any gaps that would cause an upgrade to fail or produce incorrect results. + +--- + +## Phase 6: Update Patches Doc (if needed) + +If any issues were found in Phases 3-5, fix them: + +1. **Fix summary counts** if they don't match the diff +2. **Add missing file entries** for undocumented modifications, additions, or exclusions +3. **Correct inaccurate descriptions** where the doc doesn't match the actual diff +4. **Add undocumented adaptations** discovered in Phase 4 +5. **Remove phantom entries** for files that are actually identical +6. **Update quick reference tables** to match corrections + +**Important:** Only make changes that are clearly necessary based on diff evidence. Do not rewrite sections that are already accurate. + +--- + +## Phase 7: Validate and Report + +### 7a. Re-validate + +If changes were made in Phase 6: +- Re-read the updated patches doc +- Verify all Phase 3 checks now pass +- Verify quick reference tables are consistent with the detailed sections + +### 7b. Structured Report + +Present a final report with this structure: + +``` +## Audit Report: patches/v{X}_PATCHES.md vs upstream {tag} + +### Summary +- Modified files: {N} (documented: {M}, missing: {K}) +- Local-only files: {N} (documented: {M}, missing: {K}) +- Upstream-only files: {N} (documented: {M}, missing: {K}) +- Phantom entries: {N} +- Overall accuracy: {percentage or qualitative} + +### Issues Found +{List of issues, or "None — documentation is accurate"} + +### Changes Made +{List of changes to patches doc, or "None — no changes needed"} + +### Upgrade Readiness +{Assessment of whether the doc is sufficient for a future upgrade} +``` + +### 7c. Cleanup + +Remove the temporary upstream clone: + +```bash +rm -rf "/tmp/kro-audit-$ARGUMENTS" +``` + +--- + +## Escalation Triggers + +**STOP and ask the user** before continuing if you encounter any of these: + +- A modified file has a large diff (>100 lines changed) with no corresponding documentation — this may indicate an intentional but undocumented change +- You find a local-only file you don't recognize — the user may have context about its purpose +- The adaptation intent is unclear — better to ask than guess wrong +- The patches doc structure is significantly different from what this skill expects — the user may have reorganized it intentionally diff --git a/.claude/skills/review-kro-adaptations/SKILL.md b/.claude/skills/review-kro-adaptations/SKILL.md new file mode 100644 index 0000000..da27603 --- /dev/null +++ b/.claude/skills/review-kro-adaptations/SKILL.md @@ -0,0 +1,197 @@ +--- +name: review-kro-adaptations +description: Principal-level engineering review of all function-kro adaptations to upstream KRO code. Assesses quality, minimality, correctness, and maintainability of every change we've made. +arguments: [upstream-tag] +!command: ./scripts/diff-upstream-kro.sh -s -r $ARGUMENTS +--- + +# Review KRO Adaptations Skill + +**STOP CHECK:** If "$ARGUMENTS" is empty or was not provided, do NOT proceed. Tell the user: "This skill requires an upstream KRO tag. Usage: `/review-kro-adaptations v0.8.3`" and stop immediately. + +You are performing a principal-level engineering review of every adaptation function-kro has made to upstream KRO code. The diff summary from `./scripts/diff-upstream-kro.sh -s -r $ARGUMENTS` has been pre-injected above. + +**Your job:** Review every modification and addition as a principal engineer would — not just "does it work?" but "is this the best way to achieve our goals?" + +**Your mindset:** You are an expert in Go, Kubernetes controllers, Crossplane composition functions, and CEL. You have high standards. You care about: +- Minimizing divergence from upstream (every changed line is upgrade debt) +- Correctness under edge cases +- Code that communicates intent clearly +- Simplicity over cleverness +- Making the next upgrade easier, not harder + +--- + +## Phase 1: Inventory + +From the pre-injected diff summary, extract: + +1. **MODIFIED files** — these are the focus of the review +2. **LOCAL ONLY files** — our additions, also reviewed +3. **Counts** — total files modified, added, excluded + +Read `AGENTS.md` to understand the architecture and the purpose of each component. Read `patches/v*_PATCHES.md` (glob for it) to understand the documented intent behind each adaptation. + +--- + +## Phase 2: Clone and Collect All Diffs + +Clone upstream once for reuse: + +```bash +AUDIT_DIR="/tmp/kro-review-$ARGUMENTS" +if [ ! -d "$AUDIT_DIR" ]; then + git clone --depth 1 --branch $ARGUMENTS https://github.com/kubernetes-sigs/kro.git "$AUDIT_DIR" +fi +``` + +For **every** modified file, collect the full diff: + +```bash +./scripts/diff-upstream-kro.sh -f -u "$AUDIT_DIR" -l 0 +``` + +Also read each local-only file in full. + +--- + +## Phase 3: Per-File Engineering Review + +For each modified file and each local-only file, evaluate against ALL of the following criteria. Be thorough — this is the core of the review. + +### 3a. Minimality + +- **Is every changed line necessary?** Could the same goal be achieved with fewer modifications? +- **Are there changes that could be avoided** by using interfaces, adapters, or wrapper patterns instead of modifying upstream code directly? +- **Are there drive-by changes** (formatting, renaming, reordering) mixed in with functional changes? These add upgrade noise for zero benefit. +- **Could any modification be pushed upstream** instead of maintained as a fork delta? If a change would benefit all KRO users, it shouldn't live only here. + +### 3b. Correctness + +- **Are there edge cases the modification doesn't handle?** Think about nil maps, empty slices, missing fields, unexpected types. +- **Does removing upstream code remove important safety checks?** When we delete validation or normalization, are we sure Crossplane handles it elsewhere, or are we creating a gap? +- **Are error paths correct?** Do modifications properly propagate errors, or do they swallow/ignore failures? +- **Are there concurrency concerns?** Shared state, missing locks, race conditions in the runtime execution path? + +### 3c. Clarity and Intent + +- **Would a new team member understand WHY each change was made** just by reading the code? Or does it require tribal knowledge? +- **Are adapter/wrapper patterns clearly named** to signal "this is a Crossplane-specific bridge"? +- **Are removed features clearly absent** or do they leave confusing dead code, unused parameters, or empty interfaces? + +### 3d. Maintainability and Upgrade Cost + +- **How painful will each modification be during the next upgrade?** Rate each file: trivial (mechanical), moderate (needs thought), painful (likely to break). +- **Are there modifications that could be restructured** to isolate our changes from upstream code? For example: wrapping upstream functions instead of modifying them, using composition over modification, or introducing thin adapter layers. +- **Is there duplicated logic** between our adaptations and upstream code that could diverge silently? + +### 3e. Principal-Level Design Review + +This is the highest-level assessment. Step back from individual changes and ask: + +- **Is the overall adaptation strategy sound?** Is "vendor and modify" the right approach for each package, or would some packages be better served by a different integration pattern? +- **Are there architectural improvements** that would reduce total adaptation surface? For example: could an interface or adapter layer between fn.go and the KRO libraries absorb most modifications, leaving upstream code closer to untouched? +- **Are we fighting upstream's design** in places where we should instead embrace it and adapt our wrapper layer? +- **Are there upstream extension points** (interfaces, hooks, options patterns) that we're ignoring in favor of direct modification? +- **Would a principal engineer redesign any of these adaptations?** If you had unlimited time to refactor (but still needed to vendor KRO), what would you change? +- **Are there opportunities to contribute adapter interfaces upstream** that would make function-kro a thin wrapper instead of a fork? + +### 3f. Local-Only Files + +For files we've added (not in upstream): + +- **Do they follow upstream's coding patterns** (naming, error handling, package organization)? +- **Are they well-scoped** or do they accumulate unrelated responsibilities? +- **Could any of them be contributed upstream** as general-purpose utilities? +- **Are they tested adequately?** + +--- + +## Phase 4: Cross-Cutting Concerns + +After reviewing all files individually, assess these system-level concerns: + +### 4a. Consistency + +- Are similar adaptations done the same way across files? Or does each file use a different approach to solve the same problem? +- Are naming conventions consistent across our additions and modifications? + +### 4b. Test Coverage + +- Are our adaptations tested? Not just "do tests exist" but "do tests verify the adaptation behavior specifically"? +- Are there modifications that silently change behavior but have no corresponding test changes? +- Run `go test -cover ./kro/...` and note coverage for modified packages. + +### 4c. Error Surface + +- Do our adaptations increase or decrease the error surface compared to upstream? +- Are there failure modes that only exist because of our modifications? + +--- + +## Phase 5: Findings Report + +Present findings in this structure. Be direct — praise what's good, be specific about what should improve. + +``` +## Engineering Review: function-kro adaptations vs upstream KRO {tag} + +### Executive Summary +{2-3 sentences: overall quality assessment, biggest concern, biggest strength} + +### Adaptation Surface +- Modified files: {N} +- Local-only files: {N} +- Total lines changed: ~{estimate from diffs} +- Upgrade difficulty estimate: {trivial / moderate / significant} + +### File-by-File Findings + +#### {file path} +- **Purpose of adaptation:** {one line} +- **Minimality:** {assessment} +- **Correctness:** {assessment} +- **Upgrade cost:** {trivial / moderate / painful} +- **Findings:** {specific issues or "Clean — no concerns"} +- **Recommendations:** {specific actionable items, or "None"} + +{repeat for each file} + +### Cross-Cutting Findings +{Consistency, test coverage, error surface observations} + +### Principal-Level Recommendations + +#### Quick Wins +{Changes that are small effort, high impact on quality or maintainability} + +#### Strategic Improvements +{Larger refactors that would significantly reduce adaptation surface or improve quality} + +#### Upstream Contributions +{Changes that could/should be proposed upstream to reduce our fork delta} + +### Summary Table + +| File | Minimality | Correctness | Upgrade Cost | Action Needed | +|------|-----------|-------------|--------------|---------------| +| ... | ... | ... | ... | ... | +``` + +--- + +## Phase 6: Cleanup + +```bash +rm -rf "/tmp/kro-review-$ARGUMENTS" +``` + +--- + +## Important Guidelines + +- **Do NOT make code changes.** This skill produces a review report only. The user decides what to act on. +- **Be specific.** "This could be better" is useless. "Lines 45-52 of builder.go remove the REST mapper check, but the replacement doesn't handle the case where..." is useful. +- **Cite line numbers and diff hunks.** Every finding should reference the specific code. +- **Distinguish severity.** Not every finding needs immediate action. Use: `critical` (correctness risk), `important` (significant quality/maintenance concern), `suggestion` (would be nice), `nitpick` (style/preference). +- **Acknowledge good work.** If an adaptation is clean, minimal, and well-done, say so. This calibrates the review — if everything is flagged, nothing stands out. diff --git a/.gitignore b/.gitignore index 64ec2d3..be0221f 100644 --- a/.gitignore +++ b/.gitignore @@ -20,8 +20,9 @@ # Go workspace file go.work -# ignore AI tools settings/config -/.claude +# Claude Code - ignore user-specific files, commit shared skills/commands +/.claude/settings.local.json +/.claude/memory/ # Demo files aws-credentials.txt \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index af62f8a..4750afa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,7 +48,7 @@ Crossplane RunFunctionRequest │ - Check IsIgnored (includeWhen + contagious dependency skip) │ │ - Evaluate GetDesired (hard resolve for resources/collections│ │ soft resolve for instance) │ -│ - Handle collections (forEach → {id}-0, {id}-1, ...) │ +│ - Handle collections (forEach → {id}-{name}, ...) │ │ - Evaluate readiness (ReadyTrue/False or ReadyUnspecified) │ │ 8. Build XR status from instance node's soft-resolved fields │ └──────────────────────────────────────────────────────────────────┘ @@ -117,7 +117,7 @@ function-kro/ │ └── UPGRADE_PROCESS.md # Process for upgrading from upstream KRO ├── package/ # Crossplane package definition ├── example/ # Usage examples (basic, collections, conditionals, externalref, readiness) -├── scripts/ # Build scripts (build-local.sh) +├── scripts/ # Build scripts (build-local.sh, diff-upstream-kro.sh) └── Dockerfile # Production build ``` @@ -150,7 +150,7 @@ type ForEachDimension map[string]string // iterator variable name → CEL list The graph builder categorizes each resource into a NodeType that determines runtime behavior: - **NodeTypeResource**: Standard managed resource. Hard resolution (fails if any CEL expression can't resolve). -- **NodeTypeCollection**: forEach-expanded resource. Hard resolution per expansion item. Named `{id}-{index}` in composed output. +- **NodeTypeCollection**: forEach-expanded resource. Hard resolution per expansion item. Named `{id}-{metadata.name}` in composed output (e.g., `subnets-my-app-us-east-1`). - **NodeTypeExternal**: ExternalRef read-only resource. Identity-only resolution (name/namespace). Excluded from desired output. - **NodeTypeInstance**: The XR itself. Soft resolution (best-effort partial status — skips unresolvable fields). @@ -277,7 +277,7 @@ The DAG package (`kro/graph/dag/`) handles topological sorting: Resources with `forEach` expand into multiple composed resources at runtime: - Builder sets `NodeTypeCollection` on the graph node (`kro/graph/builder.go`) - Runtime evaluates forEach dimensions as CEL list expressions, then expands via cartesian product for multi-dimensional cases (`kro/runtime/node.go:hardResolveCollection`) -- Each expanded resource gets a `kro.run/collection-index` label and is named `{id}-{index}` in composed resources +- Each expanded resource gets a `kro.run/collection-index` label and is named `{id}-{metadata.name}` in composed resources (e.g., `subnets-my-app-us-east-1`) - Collection `readyWhen` uses the `each` variable to evaluate per-item readiness - Observed composed resources are matched back to collection nodes by checking the `kro.run/collection-index` label and stripping the `-N` suffix @@ -324,6 +324,53 @@ The runtime deduplicates CEL expression evaluation via a shared `expressionsCach - `spec-desired-ssa.md` — Original SSA design spec (references older API names; implementation evolved) - `example/README.md` — Working examples for all major features (basic, collections, conditionals, externalref, readiness) +## Auditing Our Code Against Upstream KRO + +**MANDATORY PROCESS:** When asked to audit, compare, verify, or review function-kro's KRO libraries against upstream KRO, you MUST follow this process exactly. Do NOT guess, speculate, or infer what is or isn't upstream code by reading our files alone. + +### Preferred: Use Skills + +Two skills automate the audit process end-to-end: + +- **`/audit-patches v`** — Validates that `patches/v*_PATCHES.md` accurately documents all modifications, additions, and exclusions. Use when checking documentation accuracy or before an upgrade. +- **`/review-kro-adaptations v`** — Principal-level engineering review of every adaptation. Assesses minimality, correctness, maintainability, and whether a senior engineer would make different choices. Use for code quality assessment. + +Both skills use the diff script under the hood, clone upstream once, and produce structured reports. + +### Manual Fallback + +If the skills are unavailable or you need to debug, use the diff script directly: + +1. **Run the diff script.** This is non-negotiable. The script clones upstream KRO and performs real file-by-file diffs with import path normalization: + ```bash + # Summary of all differences + ./scripts/diff-upstream-kro.sh -s -r + + # Full diff for a specific file + ./scripts/diff-upstream-kro.sh -f graph/builder.go -r + + # Full diff of everything (verbose) + ./scripts/diff-upstream-kro.sh -r + ``` + +2. **Use the script output as the sole source of truth.** The script categorizes every file as `[IDENTICAL]`, `[MODIFIED]`, `[LOCAL ONLY]`, or `[UPSTREAM ONLY]`. Only files marked `[MODIFIED]` are actual adaptations. Only files marked `[LOCAL ONLY]` are our additions. + +3. **For any claim about a specific file being modified or identical, cite the diff.** Run the script with `-f ` to get the actual diff. Do not claim a file is modified without seeing the diff output. + +### What You Must NOT Do + +- Do NOT read our code and guess whether something "looks like" a function-kro addition +- Do NOT claim a function exists only in function-kro without checking upstream +- Do NOT report findings without having run the diff script first +- Do NOT use your training data knowledge of upstream KRO — it may be outdated + +### When to Use This Process + +- User asks to "audit", "compare", "verify", or "check" our code against upstream +- User asks if the patches documentation is accurate +- User asks what we've changed from upstream KRO +- During upgrade process (Phase 1, Step 1.0 of UPGRADE_PROCESS.md) + ## Troubleshooting ### "schema not found" Errors diff --git a/fn.go b/fn.go index 9d70815..ef325bb 100644 --- a/fn.go +++ b/fn.go @@ -99,7 +99,7 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1.RunFunctionRequest) } // Build the KRO graph using the schema resolver. - gb := graph.NewBuilder(resolver, nil) + gb := graph.NewBuilder(resolver) g, err := gb.NewResourceGraphDefinition(rg, xrSchema) if err != nil { response.Fatal(rsp, errors.Wrap(err, "cannot create resource graph")) diff --git a/fn_test.go b/fn_test.go index 959d035..0dd189e 100644 --- a/fn_test.go +++ b/fn_test.go @@ -645,7 +645,7 @@ func TestRunFunction(t *testing.T) { Resource: resource.MustStructJSON(`{ "apiVersion": "s3.aws.upbound.io/v1beta1", "kind": "Bucket", - "metadata": {"namespace": "xr-ns"}, + "metadata": {}, "spec": { "forProvider": { "region": "cool-region-2" @@ -770,7 +770,7 @@ func TestRunFunction(t *testing.T) { Resource: resource.MustStructJSON(`{ "apiVersion": "s3.aws.upbound.io/v1beta1", "kind": "Bucket", - "metadata": {"namespace": "xr-ns"}, + "metadata": {}, "spec": { "forProvider": { "region": "us-west-2" diff --git a/go.mod b/go.mod index ade6903..d6f383b 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,6 @@ require ( github.com/alecthomas/kong v0.9.0 github.com/crossplane/crossplane-runtime/v2 v2.2.0 github.com/crossplane/function-sdk-go v0.6.0 - github.com/gobuffalo/flect v1.0.3 github.com/google/cel-go v0.27.0 github.com/google/go-cmp v0.7.0 github.com/hashicorp/golang-lru/v2 v2.0.7 @@ -56,6 +55,7 @@ require ( github.com/go-openapi/swag/stringutils v0.25.4 // indirect github.com/go-openapi/swag/typeutils v0.25.4 // indirect github.com/go-openapi/swag/yamlutils v0.25.4 // indirect + github.com/gobuffalo/flect v1.0.3 // indirect github.com/google/gnostic-models v0.7.1 // indirect github.com/google/uuid v1.6.0 // indirect github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus v1.1.0 // indirect diff --git a/kro/graph/builder.go b/kro/graph/builder.go index dcf358d..e2a2213 100644 --- a/kro/graph/builder.go +++ b/kro/graph/builder.go @@ -21,7 +21,6 @@ import ( "github.com/google/cel-go/cel" "golang.org/x/exp/maps" - "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/yaml" "k8s.io/apiserver/pkg/cel/openapi" @@ -39,15 +38,13 @@ import ( "github.com/upbound/function-kro/kro/metadata" ) -// NewBuilder creates a new GraphBuilder instance from a SchemaResolver and RESTMapper. +// NewBuilder creates a new GraphBuilder instance from a SchemaResolver. // // This is the constructor for use with Crossplane functions. The resolver provides -// schema resolution for GVKs, and the restMapper (optional) provides GVK to GVR mapping. -// If restMapper is nil, a convention-based fallback is used (pluralize kind, assume namespaced). -func NewBuilder(schemaResolver resolver.SchemaResolver, restMapper meta.RESTMapper) *Builder { +// schema resolution for GVKs. +func NewBuilder(schemaResolver resolver.SchemaResolver) *Builder { return &Builder{ schemaResolver: schemaResolver, - restMapper: restMapper, } } @@ -72,7 +69,6 @@ func NewBuilder(schemaResolver resolver.SchemaResolver, restMapper meta.RESTMapp type Builder struct { // schemaResolver is used to resolve the OpenAPI schema for the resources. schemaResolver resolver.SchemaResolver - restMapper meta.RESTMapper } // NewResourceGraphDefinition creates a new Graph from a ResourceGraph input and XR schema. @@ -149,7 +145,10 @@ func (b *Builder) NewResourceGraphDefinition(rg *v1beta1.ResourceGraph, xrSchema // // not that we only include the spec and metadata fields, instance status references // are not allowed in RGDs (yet) - schemaWithoutStatus := getSchemaWithoutStatus(xrSchema) + schemaWithoutStatus, err := getSchemaWithoutStatus(xrSchema) + if err != nil { + return nil, fmt.Errorf("failed to get schema without status: %w", err) + } celSchemas[SchemaVarName] = schemaWithoutStatus // Create a DeclTypeProvider for introspecting type structures during validation @@ -267,24 +266,7 @@ func (b *Builder) buildRGResource( return nil, nil, fmt.Errorf("failed to get schema for resource %s: %w", rgResource.ID, err) } - // 6. Get REST mapping for GVR and namespace scope. - // Use convention-based fallback if restMapper is nil. - var gvr = gvk.GroupVersion().WithResource(strings.ToLower(gvk.Kind) + "s") - var namespaced = true - if b.restMapper != nil { - mapping, err := b.restMapper.RESTMapping(gvk.GroupKind(), gvk.Version) - if err != nil { - return nil, nil, fmt.Errorf("failed to get REST mapping for resource %s: %w", rgResource.ID, err) - } - gvr = mapping.Resource - namespaced = mapping.Scope.Name() == meta.RESTScopeNameNamespace - } - - if err := validateTemplateConstraints(rgResource, resourceObject, namespaced); err != nil { - return nil, nil, err - } - - // 7. Extract CEL fieldDescriptors from the resource. + // 6. Extract CEL fieldDescriptors from the resource. var fieldDescriptors []variable.FieldDescriptor if gvk.Group == "apiextensions.k8s.io" && gvk.Version == "v1" && gvk.Kind == "CustomResourceDefinition" { fieldDescriptors, err = parser.ParseSchemalessResource(resourceObject) @@ -347,11 +329,9 @@ func (b *Builder) buildRGResource( // Note that dependencies are not set here - they're extracted later in buildDependencyGraph. node := &Node{ Meta: NodeMeta{ - ID: rgResource.ID, - Index: order, - Type: nodeType, - GVR: gvr, - Namespaced: namespaced, + ID: rgResource.ID, + Index: order, + Type: nodeType, // Dependencies will be set by buildDependencyGraph }, Template: &unstructured.Unstructured{Object: resourceObject}, @@ -477,20 +457,12 @@ func extractTemplateDependencies( // Track iterators used in identity fields (name/namespace). switch templateVariable.Path { - case MetadataNamePath: + case MetadataNamePath, MetadataNamespacePath: for _, iter := range iteratorRefs { if !slices.Contains(iteratorsInIdentity, iter) { iteratorsInIdentity = append(iteratorsInIdentity, iter) } } - case MetadataNamespacePath: - if node.Meta.Namespaced { - for _, iter := range iteratorRefs { - if !slices.Contains(iteratorsInIdentity, iter) { - iteratorsInIdentity = append(iteratorsInIdentity, iter) - } - } - } } } } @@ -610,7 +582,6 @@ func (b *Builder) buildInstanceNode( Meta: NodeMeta{ ID: InstanceNodeID, Type: NodeTypeInstance, - Namespaced: true, // Instances are always namespaced Dependencies: uniqueInstanceDeps, }, Template: &unstructured.Unstructured{ @@ -1077,28 +1048,24 @@ func validateForEachExpressions(env *cel.Env, node *Node) (map[string]*cel.Type, return iteratorTypes, nil } -// getSchemaWithoutStatus creates a copy of the schema with status removed and -// metadata added. This is used for the "schema" CEL variable which should only -// include spec fields (not status) but should include metadata for templating. -func getSchemaWithoutStatus(schema *spec.Schema) *spec.Schema { +// getSchemaWithoutStatus creates a copy of the schema with status removed. +// This is used for the "schema" CEL variable which should only include spec +// and metadata fields (not status) for templating. +func getSchemaWithoutStatus(schema *spec.Schema) (*spec.Schema, error) { if schema == nil { - return nil + return nil, nil } // Deep copy the schema to avoid modifying the original - schemaCopy := kroschema.DeepCopySchema(schema) - - if schemaCopy.Properties == nil { - schemaCopy.Properties = make(map[string]spec.Schema) + schemaCopy, err := kroschema.DeepCopySchema(schema) + if err != nil { + return nil, fmt.Errorf("failed to deep copy schema: %w", err) } // Remove status property delete(schemaCopy.Properties, "status") - // Add metadata schema - schemaCopy.Properties["metadata"] = kroschema.ObjectMetaSchema - - return schemaCopy + return schemaCopy, nil } // collectNodeSchemas builds a map of node IDs to their OpenAPI schemas. diff --git a/kro/graph/node.go b/kro/graph/node.go index 5f93993..8fc397a 100644 --- a/kro/graph/node.go +++ b/kro/graph/node.go @@ -18,7 +18,6 @@ import ( "slices" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" "github.com/upbound/function-kro/kro/graph/variable" ) @@ -84,10 +83,6 @@ type NodeMeta struct { Index int // Type identifies the kind of node (Resource, Collection, External, Instance). Type NodeType - // GVR is the GroupVersionResource for this node's resources. - GVR schema.GroupVersionResource - // Namespaced indicates if the resource is namespace-scoped. - Namespaced bool // Dependencies lists the IDs of nodes this node depends on. Dependencies []string } @@ -140,8 +135,6 @@ func (n *Node) DeepCopy() *Node { ID: n.Meta.ID, Index: n.Meta.Index, Type: n.Meta.Type, - GVR: n.Meta.GVR, - Namespaced: n.Meta.Namespaced, Dependencies: slices.Clone(n.Meta.Dependencies), }, IncludeWhen: slices.Clone(n.IncludeWhen), diff --git a/kro/graph/schema/resolver/crd_resolver.go b/kro/graph/schema/resolver/crd_resolver.go index 9f39a17..bc92217 100644 --- a/kro/graph/schema/resolver/crd_resolver.go +++ b/kro/graph/schema/resolver/crd_resolver.go @@ -16,7 +16,6 @@ package resolver import ( "fmt" - "sync" extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -27,10 +26,10 @@ import ( // CRDSchemaResolver is a resolver.SchemaResolver backed by a set of CRDs. // It extracts OpenAPI schemas from CRD validation schemas. +// The map is read-only after construction. type CRDSchemaResolver struct { // schemas maps GVK to the extracted spec.Schema schemas map[schema.GroupVersionKind]*spec.Schema - mx sync.RWMutex } // NewCRDSchemaResolver creates a new CRDSchemaResolver from a set of CRDs. @@ -82,7 +81,5 @@ func NewCRDSchemaResolver(crds []*extv1.CustomResourceDefinition) (*CRDSchemaRes // ResolveSchema returns the OpenAPI schema for the given GVK. func (r *CRDSchemaResolver) ResolveSchema(gvk schema.GroupVersionKind) (*spec.Schema, error) { - r.mx.RLock() - defer r.mx.RUnlock() return r.schemas[gvk], nil } diff --git a/kro/graph/schema/resolver/schema_map_resolver.go b/kro/graph/schema/resolver/schema_map_resolver.go index 1c37592..4817f49 100644 --- a/kro/graph/schema/resolver/schema_map_resolver.go +++ b/kro/graph/schema/resolver/schema_map_resolver.go @@ -15,8 +15,6 @@ package resolver import ( - "sync" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/kube-openapi/pkg/validation/spec" ) @@ -24,9 +22,9 @@ import ( // SchemaMapResolver is a resolver.SchemaResolver backed by a map of GVK to // OpenAPI schemas. This is used when schemas are provided directly (e.g., from // Crossplane's required_schemas) rather than extracted from CRDs. +// The map is read-only after construction. type SchemaMapResolver struct { schemas map[schema.GroupVersionKind]*spec.Schema - mx sync.RWMutex } // NewSchemaMapResolver creates a new SchemaMapResolver from a map of GVK to @@ -37,7 +35,5 @@ func NewSchemaMapResolver(schemas map[schema.GroupVersionKind]*spec.Schema) *Sch // ResolveSchema returns the OpenAPI schema for the given GVK. func (r *SchemaMapResolver) ResolveSchema(gvk schema.GroupVersionKind) (*spec.Schema, error) { - r.mx.RLock() - defer r.mx.RUnlock() return r.schemas[gvk], nil } diff --git a/kro/graph/schema/schema.go b/kro/graph/schema/schema.go index 3fe2733..be165a8 100644 --- a/kro/graph/schema/schema.go +++ b/kro/graph/schema/schema.go @@ -75,22 +75,22 @@ func WrapSchemaAsList(itemSchema *spec.Schema) *spec.Schema { // DeepCopySchema creates a deep copy of a spec.Schema. // This is useful when you need to modify a schema without affecting the original. -func DeepCopySchema(schema *spec.Schema) *spec.Schema { +func DeepCopySchema(schema *spec.Schema) (*spec.Schema, error) { if schema == nil { - return nil + return nil, nil } // Use JSON round-trip for deep copy since spec.Schema is a complex struct // with many nested pointers and maps. data, err := json.Marshal(schema) if err != nil { - return nil + return nil, fmt.Errorf("failed to marshal schema: %w", err) } copied := &spec.Schema{} if err := json.Unmarshal(data, copied); err != nil { - return nil + return nil, fmt.Errorf("failed to unmarshal schema: %w", err) } - return copied + return copied, nil } diff --git a/kro/graph/validation.go b/kro/graph/validation.go index 2987c06..2d812b5 100644 --- a/kro/graph/validation.go +++ b/kro/graph/validation.go @@ -18,7 +18,6 @@ import ( "fmt" "regexp" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/sets" "github.com/upbound/function-kro/input/v1beta1" @@ -247,20 +246,4 @@ func validateCombinableResourceFields(res *v1beta1.Resource) error { return nil } -// validateTemplateConstraints enforces template-level constraints before parsing expressions. -// Keep this small and focused on invariants that must hold regardless of CEL. -// -// TODO: We need more constraints here, e.g, you cannot set kro owned labels/annotations... -func validateTemplateConstraints(rgResource *v1beta1.Resource, resourceObject map[string]interface{}, namespaced bool) error { - if !namespaced { - _, found, err := unstructured.NestedFieldNoCopy(resourceObject, "metadata", "namespace") - if err != nil { - return fmt.Errorf("resource %q has invalid metadata.namespace: %w", rgResource.ID, err) - } - if found { - return fmt.Errorf("resource %q is cluster-scoped and must not set metadata.namespace", rgResource.ID) - } - } - return nil -} diff --git a/kro/metadata/groupversion.go b/kro/metadata/groupversion.go index b0abc3f..cfefa2b 100644 --- a/kro/metadata/groupversion.go +++ b/kro/metadata/groupversion.go @@ -18,7 +18,6 @@ import ( "fmt" "strings" - "github.com/gobuffalo/flect" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation" @@ -65,11 +64,4 @@ func ExtractGVKFromUnstructured(unstructured map[string]interface{}) (schema.Gro }, nil } -func GetResourceGraphDefinitionInstanceGVR(group, apiVersion, kind string) schema.GroupVersionResource { - pluralKind := flect.Pluralize(strings.ToLower(kind)) - return schema.GroupVersionResource{ - Group: group, - Version: apiVersion, - Resource: pluralKind, - } -} + diff --git a/kro/runtime/node.go b/kro/runtime/node.go index b0d1af4..2163646 100644 --- a/kro/runtime/node.go +++ b/kro/runtime/node.go @@ -147,10 +147,6 @@ func (n *Node) GetDesired() ([]*unstructured.Unstructured, error) { } if err == nil { - if n.Spec.Meta.Namespaced && n.Spec.Meta.Type != graph.NodeTypeInstance { - inst := n.deps[graph.InstanceNodeID] - normalizeNamespaces(result, inst.observed[0].GetNamespace()) - } n.desired = result } return result, err @@ -166,25 +162,9 @@ func (n *Node) GetDesiredIdentity() ([]*unstructured.Unstructured, error) { vars := n.templateVarsForPaths(identityPaths) switch n.Spec.Meta.Type { case graph.NodeTypeCollection: - result, err := n.hardResolveCollection(vars, false) - if err != nil { - return nil, err - } - if n.Spec.Meta.Namespaced { - inst := n.deps[graph.InstanceNodeID] - normalizeNamespaces(result, inst.observed[0].GetNamespace()) - } - return result, nil + return n.hardResolveCollection(vars, false) case graph.NodeTypeResource, graph.NodeTypeExternal: - result, err := n.hardResolveSingleResource(vars) - if err != nil { - return nil, err - } - if n.Spec.Meta.Namespaced { - inst := n.deps[graph.InstanceNodeID] - normalizeNamespaces(result, inst.observed[0].GetNamespace()) - } - return result, nil + return n.hardResolveSingleResource(vars) case graph.NodeTypeInstance: panic("GetDesiredIdentity called for instance node") default: @@ -192,16 +172,7 @@ func (n *Node) GetDesiredIdentity() ([]*unstructured.Unstructured, error) { } } -func normalizeNamespaces(objs []*unstructured.Unstructured, namespace string) { - // TODO: When cluster-scoped instances are supported, either default to - // metav1.NamespaceDefault here or enforce namespace presence in graphbuilder. - for _, obj := range objs { - if obj.GetNamespace() != "" { - continue - } - obj.SetNamespace(namespace) - } -} + // DeleteTargets returns the ordered list of objects this node should delete now. // diff --git a/package/input/kro.fn.crossplane.io_resourcegraphs.yaml b/package/input/kro.fn.crossplane.io_resourcegraphs.yaml index e071f2d..517d351 100644 --- a/package/input/kro.fn.crossplane.io_resourcegraphs.yaml +++ b/package/input/kro.fn.crossplane.io_resourcegraphs.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.18.0 + controller-gen.kubebuilder.io/version: v0.20.0 name: resourcegraphs.kro.fn.crossplane.io spec: group: kro.fn.crossplane.io diff --git a/patches/UPGRADE_PROCESS.md b/patches/UPGRADE_PROCESS.md index a515be3..726dc87 100644 --- a/patches/UPGRADE_PROCESS.md +++ b/patches/UPGRADE_PROCESS.md @@ -58,19 +58,37 @@ patches/ Before relying on `v{OLD}_PATCHES.md` as your migration guide, verify it actually matches the current codebase. Stale or inaccurate patch docs will cause you to apply wrong adaptations to the new version. -**Checks:** +**Preferred method — use the `/audit-patches` skill:** -1. **Verify documented signatures match actual code.** Spot-check key adaptations listed in the patches doc against the real source files: - - Builder constructor signature in `kro/graph/builder.go` - - `NewResourceGraphDefinition` signature in `kro/graph/builder.go` - - Runtime node methods in `kro/runtime/node.go` - - Combined resolver factories in `kro/graph/schema/resolver/resolver.go` +``` +/audit-patches v{OLD} +``` + +This skill automates the entire validation process: runs the diff script, cross-references every file against the patches doc, deep-dives each modification, fixes any discrepancies, and produces a structured audit report. It is the recommended way to validate patches documentation. + +**Manual fallback** (if the skill is unavailable or you need to debug): + +Run the diff script — this is the source of truth for what we've actually changed: + +```bash +# Summary: which files are identical, modified, local-only, or upstream-only +./scripts/diff-upstream-kro.sh -s -r v{OLD} + +# Full diff of a specific file +./scripts/diff-upstream-kro.sh -f graph/builder.go -r v{OLD} +``` -2. **Verify "Files Removed" are actually absent.** Confirm that files listed as removed (e.g., `kro/graph/crd/`, `kro/metadata/owner_reference.go`) don't exist in the tree. +Then verify the patches doc against the script output: -3. **Verify "Files Added" actually exist.** Confirm that files listed as our additions (e.g., `schema_map_resolver.go`, `crd_resolver.go`) are present. +1. **Every `[MODIFIED]` file from the script should be documented** in `v{OLD}_PATCHES.md`. If a file shows as modified but isn't in the doc, the doc is incomplete. -4. **Check for undocumented changes.** Look for commits to `kro/` files made after the patches doc was last updated: +2. **Every `[LOCAL ONLY]` file should be in the "Files Added" table.** If a local-only file isn't documented, add it. + +3. **Every `[IDENTICAL]` file should NOT be listed as modified** in the patches doc. If the doc claims a file was modified but the script shows it's identical, the doc is wrong. + +4. **Every `[UPSTREAM ONLY]` file in a vendored package** should be in the "Files Removed" table or intentionally excluded. + +5. **Check for undocumented changes since the doc was last updated:** ```bash # Find the last commit that touched the patches doc git log -1 --format="%H %ci" -- patches/v{OLD}_PATCHES.md @@ -79,7 +97,7 @@ Before relying on `v{OLD}_PATCHES.md` as your migration guide, verify it actuall git log --oneline --after="" -- kro/ ``` -5. **If discrepancies are found:** Stop and report them to the user before proceeding. The patches doc is the source of truth for the adaptation step (Phase 3) — if it's wrong, the entire upgrade will apply wrong adaptations. Present the discrepancies clearly, update `v{OLD}_PATCHES.md` to reflect reality, and get user confirmation that the updated doc is accurate before continuing to Step 1.1. +6. **If discrepancies are found:** Stop and report them to the user before proceeding. The patches doc is the source of truth for the adaptation step (Phase 3) — if it's wrong, the entire upgrade will apply wrong adaptations. Present the discrepancies clearly, update `v{OLD}_PATCHES.md` to reflect reality, and get user confirmation that the updated doc is accurate before continuing to Step 1.1. **Do not skip this confirmation.** Even if the fixes seem obvious, the user may have context about why the code diverged from the doc (e.g., an intentional change that was never documented, or a work-in-progress that shouldn't be carried forward). @@ -207,16 +225,26 @@ commits will show exactly what we modified. ### Step 2.3: Fix Import Paths -Upstream uses `github.com/kubernetes-sigs/kro/pkg/...` but we use `github.com/upbound/function-kro/kro/...`: +Upstream uses `github.com/kubernetes-sigs/kro/pkg/...` (or `sigs.k8s.io/kro/pkg/...` if they changed their module path) but we use `github.com/upbound/function-kro/kro/...`: ```bash -# Update import paths in copied files +# Update import paths in copied files (handle both possible upstream module paths) find kro/ -name "*.go" -exec sed -i '' \ 's|github.com/kubernetes-sigs/kro/pkg/|github.com/upbound/function-kro/kro/|g' {} \; +find kro/ -name "*.go" -exec sed -i '' \ + 's|sigs.k8s.io/kro/pkg/|github.com/upbound/function-kro/kro/|g' {} \; # Also update any api/ imports if present +# IMPORTANT: This also handles the v1alpha1 → v1beta1 version change +find kro/ -name "*.go" -exec sed -i '' \ + 's|github.com/kubernetes-sigs/kro/api/v1alpha1|github.com/upbound/function-kro/input/v1beta1|g' {} \; +find kro/ -name "*.go" -exec sed -i '' \ + 's|sigs.k8s.io/kro/api/v1alpha1|github.com/upbound/function-kro/input/v1beta1|g' {} \; +# Handle any other api/ subpaths that aren't version-specific find kro/ -name "*.go" -exec sed -i '' \ 's|github.com/kubernetes-sigs/kro/api/|github.com/upbound/function-kro/input/|g' {} \; +find kro/ -name "*.go" -exec sed -i '' \ + 's|sigs.k8s.io/kro/api/|github.com/upbound/function-kro/input/|g' {} \; ``` **COMMIT CHECKPOINT 2: Import paths fixed** @@ -273,6 +301,42 @@ during the copy operation. The codebase likely won't compile yet. **Goal:** Make the new upstream code work with Crossplane's function model. +### Mindset: Intelligent Merging, Not Mechanical Pasting + +**CRITICAL:** Start from the new upstream code and ask "what does this need to work in our context?" — NOT "where do I paste our old changes?" The patches doc records what we changed *last time*, but the new upstream may have evolved in ways that change what's needed. + +Every adaptation we maintain exists to bridge a specific gap between upstream's assumptions (Kubernetes controller with API server access) and our runtime environment (Crossplane composition function with no API access). The question for each adaptation during upgrade is: **does this gap still exist in the new code?** + +#### Principles + +1. **Evaluate each adaptation independently against the new code.** For each one, ask: + - Did upstream add extension points (interfaces, options, hooks) that make this adaptation unnecessary? + - Did upstream refactor so the adaptation needs to land in a different place or look different? + - Did upstream add new functionality in the same area that our old code would silently break or overwrite? + - Is the gap smaller now — maybe we only need half the old adaptation? + +2. **Prefer upstream's solution when one exists.** If upstream added a way to inject a `SchemaResolver`, use that — don't replace their constructor with ours. Less divergence is always better. Our old adaptation was the best solution *at the time*; the new upstream may offer a better one. + +3. **Preserve upstream improvements in code we modify.** If upstream added better error handling, validation, or edge case coverage in a function we rewrite, our replacement should incorporate those improvements, not overwrite them with our old version. Read the new code carefully before replacing it. + +4. **Watch for new gaps.** If upstream added a new feature that calls the REST mapper or requires API access, that needs a *new* adaptation — not just re-applying old ones. The patches doc won't mention these because they didn't exist before. + +5. **The adaptation surface should shrink over time, not grow.** Each upgrade is a chance to reduce divergence. If upstream moved closer to what we need, take advantage of it. + +#### Decision Framework + +For each adaptation in the old patches doc, the outcome is one of: + +| Outcome | When | Action | +|---------|------|--------| +| **Same** | Gap still exists, same location | Apply, adapted to surrounding code changes | +| **Moved** | Gap still exists, code restructured | Find new location, apply the *intent* there | +| **Shrunk** | Upstream partially solved it | Only apply what's still necessary | +| **Gone** | Upstream eliminated the gap | Drop the adaptation entirely | +| **Grown** | New upstream code has the same gap | Extend adaptation to cover new code too | + +**The patches doc documents WHAT we changed. You must reason about WHY we changed it and whether that WHY still applies.** + ### Step 3.1: Identify Compilation Errors ```bash @@ -286,7 +350,7 @@ This will show what broke. Common issues: ### Step 3.2: Apply Adaptations -Using the previous `v{OLD}_PATCHES.md` as a guide, re-apply each adaptation: +Using the previous `v{OLD}_PATCHES.md` as a guide and the "Intelligent Merging" principles above, adapt the new upstream code to work in our context. **AI Agent Prompt for Adaptations:** ``` @@ -295,28 +359,35 @@ I'm upgrading function-kro from KRO v{OLD} to v{NEW}. Context files: 1. patches/v{OLD}_PATCHES.md - Our previous adaptations and their intent 2. patches/v{OLD}_to_v{NEW}_CHANGES.md - What changed upstream -3. fn.go - Our Crossplane function entry point -4. Current compilation errors from `go build` - -Tasks: -1. Re-apply the adaptations from v{OLD}_PATCHES.md to the new code -2. For each adaptation, check if the upstream API changed and adapt accordingly -3. If upstream added new features (like forEach), integrate them -4. Do NOT blindly copy old code - understand the intent and apply appropriately - -Key adaptations to re-apply: -- Builder constructor: Accept (resolver, restMapper) instead of (clientConfig, httpClient) -- NewResourceGraphDefinition: Accept (ResourceGraph, *spec.Schema) instead of full RGD CR -- REST mapping fallback when restMapper is nil -- ObjectMeta schema injection in schema resolution paths -- Schema resolution via SchemaMapResolver and CRDSchemaResolver - -After fixing compilation, also do the following - -- run code generation to update generated methods and CRD schemas: -go generate ./... - -- run tests and fix any failures +3. patches/UPGRADE_PROCESS.md - Read the "Intelligent Merging" section carefully +4. fn.go - Our Crossplane function entry point +5. Current compilation errors from `go build` + +IMPORTANT: Do NOT blindly paste old adaptation code into the new upstream. +For EACH adaptation in v{OLD}_PATCHES.md: +1. Read the NEW upstream code in that area first +2. Understand WHY the adaptation existed (what gap it bridges) +3. Check if upstream already solved the problem (new interfaces, options, etc.) +4. Decide the outcome: Same / Moved / Shrunk / Gone / Grown +5. Only then write the adaptation — tailored to the new code, not copied from the old + +The fundamental gaps we bridge (these are the WHYs): +- No API server access: no REST mapper, no dynamic client, no CRD fetching +- Schema from Crossplane: XR schema arrives as *spec.Schema, not via SimpleSchema +- No CRD generation: Crossplane manages the XR CRD, not us +- No namespace defaulting: Crossplane handles namespace assignment +- Input type differences: our ResourceGraph vs upstream's ResourceGraphDefinition CR + +If upstream added extension points or restructured code to make any of these +gaps smaller, prefer upstream's approach over our old adaptation. + +If upstream added NEW code that has the same gaps (e.g., a new function that +calls the REST mapper), that needs a NEW adaptation not in the old patches doc. + +After applying adaptations: +- Run `go generate ./...` to update generated methods and CRD schemas +- Run `go build ./...` to verify compilation +- Run `go test ./...` and fix any failures ``` **COMMIT CHECKPOINT 4: Adaptations applied** @@ -329,7 +400,7 @@ chore(upgrade): apply function-kro adaptations to v{NEW} Re-applied adaptations for Crossplane function compatibility: - Modified Builder constructor to accept resolver instead of clientConfig - Updated NewResourceGraphDefinition to accept schema parameter -- Added REST mapping fallback for nil restMapper +- Namespace scope inferred from template (no RESTMapper) - Injected ObjectMeta schema in resolution paths - [Add any new adaptations specific to this version] @@ -357,6 +428,8 @@ Create `v{NEW}_PATCHES.md` documenting: - All adaptations (carried forward + new) - Any changes to the adaptation approach - New features and how they're integrated +- Adaptations that were **dropped** because upstream eliminated the gap (record why — this is valuable for future upgrades) +- Adaptations that **changed shape** because upstream restructured the code (record what's different and why) **COMMIT CHECKPOINT 5: Documentation updated** @@ -500,11 +573,26 @@ the upgrade process. Squashing is optional and depends on team preference. ### Files We Modify +Based on the v0.8.3 audit, these are all files we modify from upstream: + | File | Adaptation | |------|------------| -| `kro/graph/builder.go` | Constructor accepts resolver; NewResourceGraphDefinition accepts schema | -| `kro/graph/schema/resolver/resolver.go` | Combined resolver constructors | -| `kro/graph/schema/schema.go` | ObjectMetaSchema, DeepCopySchema | +| `kro/graph/builder.go` | Constructor accepts resolver; NewResourceGraphDefinition accepts schema; remove SimpleSchema/CRD gen; remove REST mapper | +| `kro/graph/node.go` | Remove `GVR` and `Namespaced` from NodeMeta | +| `kro/graph/validation.go` | API type adaptation; remove `validateResourceGraphDefinitionNamingConventions` and `validateTemplateConstraints` | +| `kro/graph/schema/resolver/resolver.go` | Add combined resolver factories and `combinedResolver` type | +| `kro/graph/schema/schema.go` | Add `DeepCopySchema` | +| `kro/runtime/node.go` | Remove `normalizeNamespaces` and all namespace auto-defaulting | +| `kro/metadata/finalizers.go` | Import path change | +| `kro/metadata/labels.go` | Import path change | +| `kro/metadata/groupversion.go` | Import path change; remove `GetResourceGraphDefinitionInstanceGVR` | + +### Files We Intentionally Exclude + +| Upstream File | Reason | +|---------------|--------| +| `graph/crd/*` (6 files) | CRD synthesis/compat — function-kro doesn't generate CRDs | +| `metadata/owner_reference.go` | Owner reference helpers — Crossplane manages resource ownership | ### What We Vendor (Allowlist) diff --git a/patches/v0.8.x_PATCHES.md b/patches/v0.8.x_PATCHES.md index 8e168fd..27d7464 100644 --- a/patches/v0.8.x_PATCHES.md +++ b/patches/v0.8.x_PATCHES.md @@ -1,278 +1,842 @@ -# Function-KRO Adaptations for KRO v0.8.x +# Function-KRO Adaptations for KRO v0.8.3 -This document describes all adaptations made to upstream KRO v0.8.3 code to make it work as a Crossplane composition function. It covers both the initial upgrade adaptations and all subsequent refinements. +This document is the **upgrade guide** for re-applying function-kro's adaptations when upgrading to a new upstream KRO version. It is designed to be used by an agent following `UPGRADE_PROCESS.md` Phase 3. -## Summary of Changes +For each adaptation, this document provides: +1. **What to look for** in the new upstream code +2. **What to do** (remove, replace, or modify) +3. **The complete function-kro code** where applicable, so you can reconstruct it even if upstream changed significantly +4. **Conditional guidance** for when upstream may have changed -| File | Change Type | Description | -|------|-------------|-------------| -| `kro/graph/builder.go` | Major Rewrite | Adapted constructor and graph building for Crossplane function model | -| `kro/graph/node.go` | Rewrite | Node types, metadata, and graph node structure for function-kro | -| `kro/graph/schema/resolver/resolver.go` | Addition | Combined resolver factories and `combinedResolver` type | -| `kro/graph/schema/resolver/schema_map_resolver.go` | Addition | Schema resolution from Crossplane's `required_schemas` | -| `kro/graph/schema/resolver/crd_resolver.go` | Addition | Schema resolution from CRDs (fallback path) | -| `kro/graph/schema/schema.go` | Addition | `ObjectMetaSchema`, `DeepCopySchema`, `WrapSchemaAsList` | -| `kro/runtime/node.go` | Rewrite | Runtime node with `GetDesiredIdentity()`, soft/hard resolve, collections | -| `kro/metadata/owner_reference.go` | **Deleted** | Unused KRO owner reference code | -| `kro/metadata/*.go` | Import Fix | Changed `v1alpha1` to `v1beta1` imports | -| `input/v1beta1/input.go` | Addition | `ResourceGraph`, `Resource`, `ExternalRef`, `ForEachDimension` types | -| `fn.go` | Major Rewrite | Capability-based schema resolution, external refs, readiness handling | +Based on a full audit using `./scripts/diff-upstream-kro.sh -r v0.8.3`. -## Detailed Adaptations +--- -### 1. Builder Adaptations (`kro/graph/builder.go`) +## Quick Reference -**Intent:** Make the builder work with Crossplane's schema resolution instead of Kubernetes API server discovery. +### Type Mapping (Upstream → Function-kro) -**Changes:** +Anywhere upstream code references these types, replace with our equivalents: -1. **Constructor signature:** - ```go - // Upstream (v0.8.x): - func NewBuilder(clientConfig *rest.Config, httpClient *http.Client) *Builder +| Upstream Type | Function-kro Type | Notes | +|---------------|-------------------|-------| +| `*v1alpha1.ResourceGraphDefinition` | `*v1beta1.ResourceGraph` | Top-level input | +| `*v1alpha1.Resource` | `*v1beta1.Resource` | Individual resource | +| `*v1alpha1.ExternalRef` | `*v1beta1.ExternalRef` | External reference | +| `*v1alpha1.Schema` | Not used | Replaced by `xrSchema *spec.Schema` parameter | +| `[]v1alpha1.ForEachDimension` | `[]v1beta1.ForEachDimension` | forEach dimensions | +| `v1alpha1.KRODomainName` | `v1beta1.KRODomainName` | Domain constant | - // Function-kro: - func NewBuilder(schemaResolver resolver.SchemaResolver, restMapper meta.RESTMapper) *Builder - ``` - - Accepts pre-configured schema resolver (Crossplane provides schemas via `required_schemas` or CRDs) - - RESTMapper is optional (nil triggers convention-based fallback) +### Field Access Mapping -2. **NewResourceGraphDefinition signature:** - ```go - // Upstream (v0.8.x): - func (b *Builder) NewResourceGraphDefinition(rgd *v1alpha1.ResourceGraphDefinition) (*Graph, error) +Anywhere upstream code accesses these paths, replace: - // Function-kro: - func (b *Builder) NewResourceGraphDefinition(rg *v1beta1.ResourceGraph, xrSchema *spec.Schema) (*Graph, error) - ``` - - Accepts our `ResourceGraph` input type (not the full RGD CR) - - XR schema passed explicitly (Crossplane resolves it, not the builder) - - No SimpleSchema processing (Crossplane provides OpenAPI schemas directly) +| Upstream Access | Function-kro Access | Notes | +|-----------------|---------------------|-------| +| `rgd.Spec.Resources` | `rgd.Resources` | No `Spec` wrapper | +| `rgd.Spec.Schema` | `xrSchema` parameter | Schema is a parameter, not nested in input | +| `rgd.Spec.Schema.Spec.Raw` | Not used | SimpleSchema not applicable | +| `rgd.Spec.Schema.Status.Raw` | `rgd.Status.Raw` | Status is top-level on ResourceGraph | +| `rgd.Spec.Schema.Types.Raw` | Not used | Custom types not applicable | +| `rgd.Spec.Schema.Group` | Not used | GVR from Crossplane | +| `rgd.Spec.Schema.APIVersion` | Not used | GVR from Crossplane | +| `rgd.Spec.Schema.Kind` | Not used | GVR from Crossplane | +| `rgd.Name` | Not used | No CR name | +| `node.Meta.GVR` | Removed | Crossplane manages resource identity | +| `node.Meta.Namespaced` | Removed | Crossplane handles namespace scoping | -3. **REST mapping fallback:** - When `restMapper` is nil, the builder uses convention-based GVR derivation: - ```go - var gvr = gvk.GroupVersion().WithResource(strings.ToLower(gvk.Kind) + "s") - var namespaced = true - ``` +### Import Replacements -4. **Instance node dependencies:** - Fixed bug where instance status expression dependencies weren't being set on the instance node's `Meta.Dependencies` field. +| Upstream Import | Action | +|-----------------|--------| +| `github.com/kubernetes-sigs/kro/api/v1alpha1` or `sigs.k8s.io/kro/api/v1alpha1` | Replace with `github.com/upbound/function-kro/input/v1beta1` | +| `sigs.k8s.io/kro/pkg/simpleschema` | Remove entirely | +| `sigs.k8s.io/kro/pkg/graph/crd` | Remove entirely | +| `k8s.io/client-go/rest` | Remove (no API access) | +| `sigs.k8s.io/controller-runtime/pkg/client/apiutil` | Remove (no REST mapper) | +| `github.com/gobuffalo/flect` | Remove (no pluralization needed) | +| `net/http` | Remove if only used for NewBuilder | +| `k8s.io/apimachinery/pkg/api/meta` | Remove if only used for RESTMapper | -5. **External reference support:** - The builder creates `NodeTypeExternal` nodes for resources with `ExternalRef` instead of `Template`. External nodes get their GVK directly from the `ExternalRef` fields and are treated as read-only in the graph. +### Applying Mappings Globally -### 2. Graph Node Types (`kro/graph/node.go`) +**IMPORTANT:** The Type Mapping and Field Access Mapping tables above must be applied **globally** across ALL functions in every modified file, not just the specific functions called out in the numbered adaptations below. When you encounter any reference to an upstream type or field path in any function, apply the appropriate mapping. The numbered adaptations below highlight the major structural changes, but simple type/field substitutions occur throughout. -**Intent:** Define clean node type taxonomy for the different resource kinds function-kro handles. +### Guidance for New Upstream Fields -The `NodeType` enum and `Node`/`NodeMeta` structures were rewritten to support function-kro's needs: +If a new upstream KRO version adds fields to `v1alpha1.ResourceGraphDefinition` or `v1alpha1.Resource`, you must decide how to handle them in `input/v1beta1/input.go`: + +1. **Fields that map to Crossplane concepts** (e.g., schema, GVR): Skip — Crossplane provides these via the function protocol +2. **Fields that affect resource behavior** (e.g., new options on Resources like readyWhen, includeWhen, forEach): Add the equivalent field to `input/v1beta1/input.go` `Resource` struct +3. **Fields on Schema** (e.g., new schema options): Evaluate — function-kro receives the XR schema as a `*spec.Schema` parameter, so Schema-level fields are typically not needed +4. **Fields on ResourceGraphDefinition.Spec**: Evaluate — our `ResourceGraph` has a flat structure (no `Spec` wrapper), so add directly to `ResourceGraph` if needed + +After adding new fields, update the Field Access Mapping table above and ensure `builder.go` accesses them correctly. + +### Audit Summary (v0.8.3) + +| Category | Count | +|----------|-------| +| Identical files | 29 | +| Modified files | 9 | +| Local-only files | 2 (function-kro specific) | +| Upstream-only files | 7 (intentionally excluded) | + +### Modified Files + +| File | Lines Changed | Change Category | +|------|---------------|-----------------| +| `kro/graph/builder.go` | +122/-293 | Major: API adaptation, SimpleSchema/CRD gen removal | +| `kro/graph/node.go` | +0/-7 | Minor: Remove `GVR` and `Namespaced` from NodeMeta | +| `kro/graph/validation.go` | +8/-37 | Minor: API type adaptation, remove template constraints | +| `kro/graph/schema/resolver/resolver.go` | +55/-0 | Addition: Combined resolver factories | +| `kro/graph/schema/schema.go` | +19/-0 | Addition: `DeepCopySchema` utility | +| `kro/runtime/node.go` | +2/-32 | Minor: Remove namespace normalization | +| `kro/metadata/finalizers.go` | +1/-1 | Trivial: Import path change | +| `kro/metadata/labels.go` | +1/-1 | Trivial: Import path change | +| `kro/metadata/groupversion.go` | +1/-10 | Minor: Import path + remove `GetResourceGraphDefinitionInstanceGVR` | + +--- + +## Adaptation 1: Builder Constructor (`kro/graph/builder.go`) + +### What to look for + +Upstream's `NewBuilder` takes Kubernetes client config and creates resolvers/REST mappers internally: ```go -const ( - NodeTypeResource NodeType = iota // Regular managed resource - NodeTypeCollection // forEach collection → multiple resources - NodeTypeExternal // External reference (read-only) - NodeTypeInstance // The XR/instance node -) +func NewBuilder(clientConfig *rest.Config, httpClient *http.Client) (*Builder, error) { + schemaResolver, err := schemaresolver.NewCombinedResolver(clientConfig, httpClient) + // ... + rm, err := apiutil.NewDynamicRESTMapper(clientConfig, httpClient) + // ... + return &Builder{schemaResolver: schemaResolver, restMapper: rm}, nil +} ``` -Key `NodeMeta` fields: -- `ID`, `Index`, `Type`, `GVR`, `Namespaced`, `Dependencies` +### What to do -### 3. Schema Resolver Factories (`kro/graph/schema/resolver/resolver.go`) +Replace the entire constructor. Remove `restMapper` from `Builder` struct. -**Intent:** Provide factory functions to create combined resolvers for the two schema resolution paths, with correct priority ordering. +**If upstream still has a constructor taking `rest.Config`:** Replace it entirely with our version. +**If upstream changed the constructor signature but still uses Kubernetes clients:** Same — replace entirely. +**If upstream already accepts a `SchemaResolver` parameter:** Great — just remove any `restMapper` and simplify. -**Additions:** +### Function-kro code ```go -// Create resolver from Crossplane's required_schemas -func NewCombinedResolverFromSchemas(schemaMapResolver *SchemaMapResolver) resolver.SchemaResolver +// NewBuilder creates a new GraphBuilder instance from a SchemaResolver. +// +// This is the constructor for use with Crossplane functions. The resolver provides +// schema resolution for GVKs. +func NewBuilder(schemaResolver resolver.SchemaResolver) *Builder { + return &Builder{ + schemaResolver: schemaResolver, + } +} -// Create resolver from CRDs (fallback path) -func NewCombinedResolverFromCRDs(crdResolver *CRDSchemaResolver) resolver.SchemaResolver +type Builder struct { + // schemaResolver is used to resolve the OpenAPI schema for the resources. + schemaResolver resolver.SchemaResolver + // NOTE: No restMapper field. Crossplane handles namespace scoping. +} ``` -Both functions create a `combinedResolver` that: -1. Tries the provided resolver first (Crossplane-provided schemas) -2. Falls back to core types resolver for built-in Kubernetes types (Deployment, Service, etc.) +--- + +## Adaptation 2: NewResourceGraphDefinition Signature (`kro/graph/builder.go`) + +### What to look for -The `combinedResolver` type is our own implementation because the upstream `DefinitionsSchemaResolver.Combine()` puts core types as primary. We need the opposite priority: Crossplane-provided schemas first, core types as fallback. +Upstream accepts a full Kubernetes CR: -**Note:** `NewCombinedResolverFromCRDs` accepts a pre-constructed `*CRDSchemaResolver` (not a raw CRD slice). The caller constructs the CRD resolver first, then passes it in. +```go +func (b *Builder) NewResourceGraphDefinition(originalCR *v1alpha1.ResourceGraphDefinition) (*Graph, error) { + rgd := originalCR.DeepCopy() + // accesses rgd.Spec.Resources, rgd.Spec.Schema, rgd.Name, etc. +``` -### 4. Schema Map Resolver (`kro/graph/schema/resolver/schema_map_resolver.go`) +### What to do -**Intent:** Resolve schemas from the map that Crossplane provides via `required_schemas`. +Change the signature to accept our `ResourceGraph` input type plus an explicit XR schema. Update all field accesses per the "Field Access Mapping" table above. -- `SchemaMapResolver` — holds a `map[GVK]*spec.Schema` -- `NewSchemaMapResolver(schemas)` — constructor -- `ResolveSchema(gvk)` — looks up the GVK in the map +**If upstream renamed this method:** Apply the same adaptation to the new name. +**If upstream split this into multiple methods:** Apply the input type adaptation to whichever method is the entry point. -### 5. CRD Schema Resolver (`kro/graph/schema/resolver/crd_resolver.go`) +### Function-kro code -**Intent:** Resolve schemas by extracting them from CRD objects (fallback for older Crossplane). +```go +func (b *Builder) NewResourceGraphDefinition(rg *v1beta1.ResourceGraph, xrSchema *spec.Schema) (*Graph, error) { + rgd := rg.DeepCopy() + + err := validateResourceIDs(rgd) + if err != nil { + return nil, fmt.Errorf("failed to validate resource graph: %w", err) + } + + nodes := make(map[string]*Node) + schemas := make(map[string]*spec.Schema) + for i, rgResource := range rgd.Resources { // NOTE: rgd.Resources, not rgd.Spec.Resources + id := rgResource.ID + node, nodeSchema, err := b.buildRGResource(rgResource, i) + // ... same as upstream from here ... + } + + // Build instance from xrSchema parameter (not from SimpleSchema) + instance, err := b.buildInstanceNode(xrSchema, rgd, nodes, schemas) + if err != nil { + return nil, fmt.Errorf("failed to build instance node: %w", err) + } + + // ... CEL validation same as upstream ... + + schemaWithoutStatus := getSchemaWithoutStatus(xrSchema) // NOTE: xrSchema, not CRD + celSchemas[SchemaVarName] = schemaWithoutStatus + + // ... dependency graph, topological sort, CEL validation same as upstream ... + + resourceGraphDefinition := &Graph{ + DAG: dag, + Instance: instance, + Nodes: nodes, + Resources: nodes, + TopologicalOrder: topologicalOrder, + // NOTE: No CRD field + } + return resourceGraphDefinition, nil +} +``` -- `CRDSchemaResolver` — extracts OpenAPI schemas from `CustomResourceDefinition` objects -- `NewCRDSchemaResolver(crds)` — constructor, indexes CRDs by GVK -- Converts CRD validation schemas (`JSONSchemaProps`) to `spec.Schema` +### Also: Remove `CRD` from Graph struct -### 6. Schema Utilities (`kro/graph/schema/schema.go`) +If upstream's `Graph` struct has a `CRD *extv1.CustomResourceDefinition` field, remove it. -**Additions:** +--- -1. **`ObjectMetaSchema`** — Fully resolved ObjectMeta schema, populated once at init time using `metav1.ObjectMeta{}.OpenAPIModelName()` and `resolver.PopulateRefs()`. +## Adaptation 3: Replace `buildInstanceNode` (`kro/graph/builder.go`) - Uses dynamic model name lookup via `OpenAPIModelName()` instead of a hardcoded string to support Kubernetes v0.35.0+ which changed model name format from Go import paths to dot-separated names. +### What to look for -2. **`WrapSchemaAsList(itemSchema)`** — Wraps an OpenAPI schema as an array schema. Used for collection resources so they're typed as `list(ResourceType)` in CEL. +Upstream's `buildInstanceNode` builds the instance from SimpleSchema, synthesizes a CRD, and returns `(*Node, *extv1.CustomResourceDefinition, error)`. It calls `buildInstanceSpecSchema()`, `buildStatusSchema()`, and `crd.SynthesizeCRD()`. -3. **`DeepCopySchema(schema)`** — JSON round-trip deep copy for `spec.Schema`. +### What to do -### 7. Input Types (`input/v1beta1/input.go`) +Replace the entire function. Our version is much simpler — it receives the XR schema directly and only needs to parse status CEL expressions. Also delete `buildInstanceSpecSchema()` and `buildStatusSchema()` if they still exist. -**Intent:** Define the function-kro specific input API that Crossplane sends to us. +**If upstream refactored this into different helper functions:** The key insight is that we don't need ANY of the SimpleSchema/CRD generation logic. We only need the status CEL expression parsing and dependency extraction. -**Types:** +### Function-kro code (complete replacement) ```go -const KRODomainName = "kro.run" +// buildInstanceNode builds the instance node from the XR schema and status CEL expressions. +// This is a simplified version for Crossplane functions where the XR schema is provided +// directly rather than being built from SimpleSchema. +func (b *Builder) buildInstanceNode( + xrSchema *spec.Schema, + rgd *v1beta1.ResourceGraph, + nodes map[string]*Node, + nodeSchemas map[string]*spec.Schema, +) (*Node, error) { + // Parse status CEL expressions if provided + statusVariables := []*variable.ResourceField{} + var statusTemplate map[string]interface{} + var instanceDeps []string + + if len(rgd.Status.Raw) > 0 { + unstructuredStatus := map[string]interface{}{} + err := yaml.UnmarshalStrict(rgd.Status.Raw, &unstructuredStatus) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal status schema: %w", err) + } + statusTemplate = unstructuredStatus + + fieldDescriptors, err := parser.ParseSchemalessResource(unstructuredStatus) + if err != nil { + return nil, fmt.Errorf("failed to extract CEL expressions from status: %w", err) + } + + nodeNames := maps.Keys(nodes) + env, err := krocel.DefaultEnvironment(krocel.WithResourceIDs(nodeNames)) + if err != nil { + return nil, fmt.Errorf("failed to create CEL environment: %w", err) + } + + // Collect dependencies for instance status fields + for _, fd := range fieldDescriptors { + path := "status." + fd.Path + fd.Path = path + + // Extract dependencies from ALL expressions in the field (for multi-expression templates) + var resourceDeps []string + for _, expr := range fd.Expressions { + deps, _, err := extractDependencies(env, expr, nodeNames, nil) + if err != nil { + return nil, fmt.Errorf("failed to extract dependencies from expression %q: %w", expr, err) + } + for _, dep := range deps { + if !slices.Contains(resourceDeps, dep) { + resourceDeps = append(resourceDeps, dep) + } + } + } + if len(resourceDeps) == 0 { + return nil, fmt.Errorf("instance status field must refer to a resource: %s", fd.Path) + } + instanceDeps = append(instanceDeps, resourceDeps...) + + statusVariables = append(statusVariables, &variable.ResourceField{ + FieldDescriptor: fd, + Kind: variable.ResourceVariableKindDynamic, + Dependencies: resourceDeps, + }) + } + } + + // Deduplicate instance dependencies. + var uniqueInstanceDeps []string + seen := make(map[string]bool) + for _, dep := range instanceDeps { + if !seen[dep] { + seen[dep] = true + uniqueInstanceDeps = append(uniqueInstanceDeps, dep) + } + } + + // Create the instance node. + // Instance doesn't have IncludeWhen, ReadyWhen, or ForEach. + instance := &Node{ + Meta: NodeMeta{ + ID: InstanceNodeID, + Type: NodeTypeInstance, + Dependencies: uniqueInstanceDeps, + }, + Template: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "status": statusTemplate, + }, + }, + Variables: statusVariables, + } + + return instance, nil +} +``` + +### Also: Delete these upstream functions if they exist + +- `buildInstanceSpecSchema()` — SimpleSchema processing +- `buildStatusSchema()` — CEL type inference for status schema → CRD generation + +--- + +## Adaptation 4: Replace `getSchemaWithoutStatus` (`kro/graph/builder.go`) -type ResourceGraph struct { ... } // Top-level input with Status and Resources -type Resource struct { ... } // A resource in the graph -type ExternalRef struct { ... } // Reference to existing cluster resource -type ExternalRefMetadata struct { ... } // Name/namespace for external refs (supports CEL) -type ForEachDimension map[string]string // Iterator variable → CEL list expression +### What to look for + +Upstream extracts the schema from a CRD, validates it has one version, converts `JSONSchemaProps` to `spec.Schema`, then removes status: + +```go +func getSchemaWithoutStatus(crd *extv1.CustomResourceDefinition) (*spec.Schema, error) { + // validates CRD has exactly one version + // converts JSONSchemaProps → spec.Schema + // removes status + // adds metadata ``` -The `Resource` struct supports: -- `Template` — Kubernetes manifest with `${...}` CEL expressions -- `ExternalRef` — Alternative to Template for reading existing resources (mutually exclusive with Template) -- `ReadyWhen` — CEL conditions for readiness (AND logic) -- `IncludeWhen` — CEL conditions for inclusion (AND logic) -- `ForEach` — Dimensions for collection expansion +### What to do -### 8. Metadata Package Changes +Replace with our simpler version that operates directly on `*spec.Schema`. -Files in `kro/metadata/` were updated to import from `input/v1beta1` instead of the upstream `api/v1alpha1`: -- `finalizers.go` — uses `v1beta1.KRODomainName` -- `groupversion.go` — uses `v1beta1.KRODomainName` -- `labels.go` — uses `v1beta1.KRODomainName` -- `selectors.go` — upstream file, no modifications needed +**If upstream changed `getSchemaWithoutStatus` but still takes a CRD:** Replace entirely. +**If upstream already takes `*spec.Schema`:** Just ensure status is removed. -**Deleted:** `owner_reference.go` — contained owner reference helper functions that are unused in function-kro (we don't set owner references; Crossplane manages resource ownership). +**Note:** We do NOT inject `ObjectMetaSchema` here. Crossplane's `required_schemas` sends fully-flattened schemas with metadata already resolved (see [crossplane/crossplane#7022](https://github.com/crossplane/crossplane/pull/7022)). The CRD fallback path handles metadata injection separately in `crd_resolver.go` where CRD schemas may have minimal metadata. -### 9. Runtime Node (`kro/runtime/node.go`) +### Function-kro code (complete replacement) -**Intent:** The runtime node is the mutable execution handle that wraps an immutable `graph.Node`. This was substantially rewritten for function-kro. +```go +// getSchemaWithoutStatus creates a copy of the schema with status removed. +// This is used for the "schema" CEL variable which should only include spec +// and metadata fields (not status) for templating. +func getSchemaWithoutStatus(schema *spec.Schema) (*spec.Schema, error) { + if schema == nil { + return nil, nil + } + + // Deep copy the schema to avoid modifying the original + schemaCopy, err := kroschema.DeepCopySchema(schema) + if err != nil { + return nil, fmt.Errorf("failed to deep copy schema: %w", err) + } + + // Remove status property + delete(schemaCopy.Properties, "status") + + return schemaCopy, nil +} +``` -**Key methods:** +--- -- **`GetDesired()`** — Computes desired state. Returns `[]*unstructured.Unstructured` (slice for collection support). Behavior varies by node type: - - `Resource`/`External`: strict evaluation via `hardResolveSingleResource()` - - `Collection`: strict evaluation with forEach expansion via `hardResolveCollection()` - - `Instance`: best-effort partial evaluation via `softResolve()` (skips unresolvable fields) +## Adaptation 5: Remove REST Mapper from `buildRGResource` (`kro/graph/builder.go`) -- **`GetDesiredIdentity()`** — Resolves only `metadata.name` and `metadata.namespace` fields. Skips readiness gating. Used for external reference identity resolution where we only need to know *which* resource to fetch, not its full desired state. Does NOT cache results in `n.desired`. +### What to look for -- **`IsIgnored()`** — Checks includeWhen conditions. Ignored state is contagious (propagates to dependents). +Inside `buildRGResource()`, upstream calls the REST mapper after resolving the schema: -- **`IsReady()`** — Evaluates readyWhen expressions. Ignored nodes are treated as ready for dependency gating. Collections check all items via `isCollectionReady()`. +```go +mapping, err := b.restMapper.RESTMapping(gvk.GroupKind(), gvk.Version) +if err != nil { ... } +if err := validateTemplateConstraints(rgResource, resourceObject, mapping.Scope.Name() == meta.RESTScopeNameNamespace); err != nil { ... } +``` -- **`SetObserved()`** — Stores observed state. For collections, uses `orderedIntersection()` to align observed items with desired items. +And sets `GVR` and `Namespaced` on the node: -- **`softResolve()`** — Best-effort partial evaluation for instance status. Only includes fields where ALL expressions are resolved, preventing template strings like `${expr}` from leaking into status. +```go +node := &Node{ + Meta: NodeMeta{ + GVR: mapping.Resource, + Namespaced: mapping.Scope.Name() == meta.RESTScopeNameNamespace, +``` -### 10. Function Entry Point (`fn.go`) +### What to do -**Major rewrite** covering schema resolution, external references, readiness, and SSA compliance. +1. Delete the `restMapper.RESTMapping()` call and `validateTemplateConstraints()` call +2. Remove `GVR` and `Namespaced` from the `NodeMeta` initialization +3. Delete the `validateTemplateConstraints()` function from `validation.go` if it still exists -#### Capability-Based Schema Resolution +**If upstream removed REST mapper usage:** This adaptation may already be done. +**If upstream added new REST mapper calls:** Remove those too — function-kro has no API access. -Uses `request.HasCapability()` to choose between resolution paths: +### Function-kro node construction ```go -// If Crossplane supports required_schemas (v2.2+), use those exclusively. -if request.HasCapability(req, fnv1.Capability_CAPABILITY_REQUIRED_SCHEMAS) { - // request schemas, build SchemaMapResolver -} else { - // fall back to requesting CRDs via required_resources +node := &Node{ + Meta: NodeMeta{ + ID: rgResource.ID, + Index: order, + Type: nodeType, + // NOTE: No GVR or Namespaced fields. Dependencies set by buildDependencyGraph. + }, + Template: &unstructured.Unstructured{Object: resourceObject}, + Variables: templateVariables, + IncludeWhen: includeWhen, + ReadyWhen: readyWhen, + ForEach: forEachDimensions, } ``` -This is a clean either/or choice — the function never requests both. The `requireSchemas()` method handles schema requests, and `buildResolver()` dispatches to `buildResolverFromSchemas()` or `buildResolverFromCRDs()`. +--- -#### External Reference Support +## Adaptation 6: Remove `GVR` and `Namespaced` from NodeMeta (`kro/graph/node.go`) -External references allow reading existing cluster resources without creating them: +### What to look for -1. **Identity Resolution:** `externalRefSelectorsFromRuntime()` uses `node.GetDesiredIdentity()` to evaluate CEL expressions in external ref metadata (name/namespace may contain `${schema.spec.configMapName}` etc.) -2. **Resource Fetching:** External ref selectors are included in `rsp.Requirements.Resources` so Crossplane fetches them -3. **Observed State Injection:** Fetched external resources are set as observed state on their runtime nodes via `node.SetObserved()` -4. **Exclusion from Desired:** External ref nodes are skipped when building desired composed resources (they're read-only) +```go +type NodeMeta struct { + ID string + Index int + Type NodeType + GVR schema.GroupVersionResource // REMOVE + Namespaced bool // REMOVE + Dependencies []string +} +``` -Multi-phase execution: If an external ref's identity can't be resolved yet (depends on unobserved data), it's skipped and resolved on a subsequent function invocation. +Also in `DeepCopy()`: +```go +GVR: n.Meta.GVR, // REMOVE +Namespaced: n.Meta.Namespaced, // REMOVE +``` -#### Readiness Handling +### What to do + +Remove both fields from the struct and from `DeepCopy()`. Remove the `schema` import if it's no longer used. + +**If upstream renamed these fields:** Remove whatever fields serve the same purpose (GVK-to-resource mapping, namespace scope). +**If upstream already removed them:** No action needed. + +### Function-kro NodeMeta + +```go +type NodeMeta struct { + ID string + Index int + Type NodeType + Dependencies []string +} +``` + +--- + +## Adaptation 7: Iterator Identity Simplification (`kro/graph/builder.go`) + +### What to look for + +In `extractTemplateDependencies()`, upstream has separate cases for `MetadataNamePath` and `MetadataNamespacePath`, where the namespace case is conditional on `node.Meta.Namespaced`: ```go -readyState := resource.ReadyUnspecified -if len(node.Spec.ReadyWhen) > 0 { - readyState = resource.ReadyFalse - if isReady, err := node.IsReady(); err != nil { - // log error - } else if isReady { - readyState = resource.ReadyTrue +case MetadataNamePath: + for _, iter := range iteratorRefs { + if !slices.Contains(iteratorsInIdentity, iter) { + iteratorsInIdentity = append(iteratorsInIdentity, iter) + } } +case MetadataNamespacePath: + if node.Meta.Namespaced { + for _, iter := range iteratorRefs { ... } + } +``` + +### What to do + +Merge the two cases since `Namespaced` no longer exists: + +```go +case MetadataNamePath, MetadataNamespacePath: + for _, iter := range iteratorRefs { + if !slices.Contains(iteratorsInIdentity, iter) { + iteratorsInIdentity = append(iteratorsInIdentity, iter) + } + } +``` + +**If upstream removed the `Namespaced` check:** May already match our version. +**If upstream restructured this logic entirely:** Apply the same principle — always track namespace iterators in identity, unconditionally. + +--- + +## Adaptation 8: Remove Namespace Normalization (`kro/runtime/node.go`) + +### What to look for + +Upstream has a `normalizeNamespaces()` function and calls it from `GetDesired()` and `GetDesiredIdentity()`: + +```go +func normalizeNamespaces(objs []*unstructured.Unstructured, namespace string) { + for _, obj := range objs { + if obj.GetNamespace() != "" { continue } + obj.SetNamespace(namespace) + } +} +``` + +Called like: +```go +if n.Spec.Meta.Namespaced && n.Spec.Meta.Type != graph.NodeTypeInstance { + inst := n.deps[graph.InstanceNodeID] + normalizeNamespaces(result, inst.observed[0].GetNamespace()) } ``` -- If `readyWhen` expressions are defined: explicitly sets `ReadyTrue`/`ReadyFalse` -- If NO `readyWhen` defined: leaves `ReadyUnspecified` so downstream functions (like `function-auto-ready`) can determine readiness +### What to do -#### Collection Handling +1. Delete the `normalizeNamespaces()` function entirely +2. Remove all calls to it — simplify to direct returns +3. In `GetDesiredIdentity()`, the multi-line error-check-then-normalize patterns become direct returns -Collections expand into multiple composed resources with index-based naming: +**If upstream removed `normalizeNamespaces()`:** No action needed. +**If upstream renamed it or changed the logic:** Remove it — Crossplane handles namespace assignment. + +### Function-kro `GetDesiredIdentity` patterns + +```go +// Upstream (verbose, with namespace normalization): +case graph.NodeTypeCollection: + result, err := n.hardResolveCollection(vars, false) + if err != nil { return nil, err } + if n.Spec.Meta.Namespaced { + inst := n.deps[graph.InstanceNodeID] + normalizeNamespaces(result, inst.observed[0].GetNamespace()) + } + return result, nil + +// Function-kro (simplified): +case graph.NodeTypeCollection: + return n.hardResolveCollection(vars, false) ``` -resourceId-0, resourceId-1, resourceId-2, ... + +Same pattern for `NodeTypeResource`/`NodeTypeExternal` case. + +--- + +## Adaptation 9: Validation Type Changes (`kro/graph/validation.go`) + +### What to look for + +Upstream validation functions take `*v1alpha1.ResourceGraphDefinition` and `*v1alpha1.Resource`. + +### What to do + +1. Change all type references per the Type Mapping table +2. Change field accesses per the Field Access Mapping table +3. Remove `validateResourceGraphDefinitionNamingConventions()` wrapper — call `validateResourceIDs()` directly +4. Remove `validateTemplateConstraints()` entirely (needs REST mapper) +5. Remove `k8s.io/apimachinery/pkg/api/meta` import if only used by `validateTemplateConstraints` (provides `RESTScopeNameNamespace`) + +**If upstream renamed these functions:** Apply the same type changes to the new names. +**If upstream added new validation that uses REST mapper:** Remove that validation. + +### Functions that need type changes + +```go +// Change parameter types: +func validateResourceIDs(rgd *v1beta1.ResourceGraph) error { + // Change: rgd.Spec.Resources → rgd.Resources (two places in loops) +} + +func validateForEachDimensions(res *v1beta1.Resource, resourceIDs sets.String) error { + // Internal logic unchanged +} + +func validateCombinableResourceFields(res *v1beta1.Resource) error { + // Internal logic unchanged +} +``` + +### Functions to DELETE + +```go +// DELETE: validateResourceGraphDefinitionNamingConventions() +// It validated the Kind name (UpperCamelCase) which is irrelevant for Crossplane. +// The caller should call validateResourceIDs() directly instead. + +// DELETE: validateTemplateConstraints() +// It validated cluster-scoped resources don't set namespace (needs REST mapper). +``` + +--- + +## Adaptation 10: Metadata Package (3 files) + +### What to look for + +Import of `github.com/kubernetes-sigs/kro/api/v1alpha1` (or `sigs.k8s.io/kro/api/v1alpha1`). + +### What to do + +**`finalizers.go`**, **`labels.go`**: Change import path only. The code references `v1alpha1.KRODomainName` which maps to `v1beta1.KRODomainName` (same constant value `"kro.run"`). If the constant is inlined, no change needed. + +**`groupversion.go`**: Same import change, plus delete `GetResourceGraphDefinitionInstanceGVR()` and the `github.com/gobuffalo/flect` import. This function computed the GVR for the instance using pluralization — not needed in function-kro. + +**`owner_reference.go`**: Delete entirely if it exists. Function-kro doesn't set owner references. + +**If upstream moved `KRODomainName` to a different package:** Find where it moved and update the import, or inline the constant `"kro.run"`. + +--- + +## Files Added (Not in Upstream) + +These files are function-kro additions. They should be preserved during `UPGRADE_PROCESS.md` Step 2.4 (Restore Our Additions). If the copy phase overwrites them, restore from git or recreate using the code below. + +### `kro/graph/schema/resolver/schema_map_resolver.go` + +Resolves schemas from a map provided by Crossplane's `required_schemas`. + +```go +package resolver + +import ( + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/kube-openapi/pkg/validation/spec" +) + +// SchemaMapResolver is a resolver.SchemaResolver backed by a map of GVK to +// OpenAPI schemas. This is used when schemas are provided directly (e.g., from +// Crossplane's required_schemas) rather than extracted from CRDs. +// The map is read-only after construction. +type SchemaMapResolver struct { + schemas map[schema.GroupVersionKind]*spec.Schema +} + +// NewSchemaMapResolver creates a new SchemaMapResolver from a map of GVK to +// spec.Schema. +func NewSchemaMapResolver(schemas map[schema.GroupVersionKind]*spec.Schema) *SchemaMapResolver { + return &SchemaMapResolver{schemas: schemas} +} + +// ResolveSchema returns the OpenAPI schema for the given GVK. +func (r *SchemaMapResolver) ResolveSchema(gvk schema.GroupVersionKind) (*spec.Schema, error) { + return r.schemas[gvk], nil +} ``` -Observed resources are matched back to collection nodes by checking the `kro.run/collection-index` label and stripping the `-N` suffix from the composed resource name. +### `kro/graph/schema/resolver/crd_resolver.go` -#### SSA-Compatible Desired State +Resolves schemas by extracting them from CRD objects (fallback for older Crossplane). -- Composed resources only contain template-defined fields (via `node.GetDesired()`) -- Desired XR only contains status fields declared in the ResourceGraph, populated via the instance node's soft resolution -- Unresolvable status fields are skipped (partial status updates) +```go +package resolver + +import ( + "fmt" -#### SDK Helper Functions + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/kube-openapi/pkg/validation/spec" -Uses function-sdk-go helpers throughout: -- `request.HasCapability()`, `request.GetRequiredSchemas()`, `request.GetRequiredResources()` -- `request.GetRequiredResource()` — for fetching specific external resources -- `request.GetObservedComposedResources()`, `request.GetObservedComposedResource()` -- `response.RequireSchema()` — for requesting schemas -- `response.SetDesiredComposedResources()`, `response.SetDesiredCompositeResource()` + kroschema "github.com/upbound/function-kro/kro/graph/schema" +) -## Files Removed +// CRDSchemaResolver is a resolver.SchemaResolver backed by a set of CRDs. +// It extracts OpenAPI schemas from CRD validation schemas. +// The map is read-only after construction. +type CRDSchemaResolver struct { + // schemas maps GVK to the extracted spec.Schema + schemas map[schema.GroupVersionKind]*spec.Schema +} -### Upstream files not applicable to function-kro +// NewCRDSchemaResolver creates a new CRDSchemaResolver from a set of CRDs. +// It extracts the OpenAPI schema from each CRD's validation schema and +// injects the ObjectMeta schema for the metadata field. +func NewCRDSchemaResolver(crds []*extv1.CustomResourceDefinition) (*CRDSchemaResolver, error) { + schemas := make(map[schema.GroupVersionKind]*spec.Schema) + + for _, crd := range crds { + if crd == nil { + continue + } + + group := crd.Spec.Group + kind := crd.Spec.Names.Kind + + for _, version := range crd.Spec.Versions { + if version.Schema == nil || version.Schema.OpenAPIV3Schema == nil { + continue + } + + gvk := schema.GroupVersionKind{ + Group: group, + Version: version.Name, + Kind: kind, + } + + specSchema, err := kroschema.ConvertJSONSchemaPropsToSpecSchema(version.Schema.OpenAPIV3Schema) + if err != nil { + return nil, fmt.Errorf("failed to convert schema for %s: %w", gvk, err) + } + + if specSchema.Properties == nil { + specSchema.Properties = make(map[string]spec.Schema) + } + specSchema.Properties["metadata"] = kroschema.ObjectMetaSchema + + schemas[gvk] = specSchema + } + } + + return &CRDSchemaResolver{schemas: schemas}, nil +} + +// ResolveSchema returns the OpenAPI schema for the given GVK. +func (r *CRDSchemaResolver) ResolveSchema(gvk schema.GroupVersionKind) (*spec.Schema, error) { + return r.schemas[gvk], nil +} +``` + +### Additions to `kro/graph/schema/resolver/resolver.go` + +These are appended to the upstream file (which contains `NewCombinedResolver`): + +```go +// NewCombinedResolverFromSchemas creates a schema resolver that combines +// a schema map resolver (for Crossplane-provided schemas) with a core resolver +// (for built-in Kubernetes types). This is the primary constructor for use +// with Crossplane functions that receive OpenAPI schemas via required_schemas. +func NewCombinedResolverFromSchemas(schemaMapResolver *SchemaMapResolver) resolver.SchemaResolver { + coreResolver := newCoreResolver() + return &combinedResolver{ + primary: schemaMapResolver, + fallback: coreResolver, + } +} + +// NewCombinedResolverFromCRDs creates a schema resolver that combines +// a CRD schema resolver (for schemas extracted from CRDs) with a core resolver +// (for built-in Kubernetes types). This is the constructor for use with +// Crossplane functions that receive CRDs via required_resources. +func NewCombinedResolverFromCRDs(crdResolver *CRDSchemaResolver) resolver.SchemaResolver { + coreResolver := newCoreResolver() + return &combinedResolver{ + primary: crdResolver, + fallback: coreResolver, + } +} + +// newCoreResolver creates a resolver for built-in Kubernetes types using +// compiled-in OpenAPI definitions. +func newCoreResolver() resolver.SchemaResolver { + return resolver.NewDefinitionsSchemaResolver( + openapi.GetOpenAPIDefinitions, + scheme.Scheme, + ) +} + +// combinedResolver tries resolvers in order until one returns a schema. +// We need our own rather than using DefinitionsSchemaResolver.Combine() because +// that method puts core types as primary. We need the opposite priority: +// Crossplane-provided schemas first, core types as fallback. +type combinedResolver struct { + primary resolver.SchemaResolver + fallback resolver.SchemaResolver +} + +func (c *combinedResolver) ResolveSchema(gvk schema.GroupVersionKind) (*spec.Schema, error) { + s, err := c.primary.ResolveSchema(gvk) + if err != nil { + return nil, err + } + if s != nil { + return s, nil + } + return c.fallback.ResolveSchema(gvk) +} +``` + +### Addition to `kro/graph/schema/schema.go` + +Appended to the upstream file: + +```go +// DeepCopySchema creates a deep copy of a spec.Schema. +// This is useful when you need to modify a schema without affecting the original. +func DeepCopySchema(schema *spec.Schema) (*spec.Schema, error) { + if schema == nil { + return nil, nil + } + + // Use JSON round-trip for deep copy since spec.Schema is a complex struct + // with many nested pointers and maps. + data, err := json.Marshal(schema) + if err != nil { + return nil, fmt.Errorf("failed to marshal schema: %w", err) + } + + copied := &spec.Schema{} + if err := json.Unmarshal(data, copied); err != nil { + return nil, fmt.Errorf("failed to unmarshal schema: %w", err) + } + + return copied, nil +} +``` + +Requires adding `"encoding/json"` to the import block. + +--- + +## Files Intentionally Excluded (upstream-only) + +These upstream files should NOT be copied during an upgrade. Delete them if they appear after copying. | File/Directory | Reason | |----------------|--------| -| `kro/graph/crd/` | Uses v1alpha1.Schema type we don't have | -| `kro/graph/builder_test.go` | Uses upstream test utilities (testutil/generator, testutil/k8s) | -| `kro/graph/validation_test.go` | Uses v1alpha1 types | -| `kro/metadata/owner_reference.go` | Unused — function-kro doesn't set owner references | +| `kro/graph/crd/` (entire directory) | CRD synthesis/compat/defaults — function-kro doesn't generate CRDs | +| `kro/metadata/owner_reference.go` | Owner reference helpers — Crossplane manages resource ownership | -## Files Added (not in upstream) +**Decision rule for new upstream files:** If a new file imports `simpleschema`, `crd`, controller packages, or requires `rest.Config`, it should not be copied. -| File | Purpose | -|------|---------| -| `kro/graph/schema/resolver/schema_map_resolver.go` | Resolve schemas from Crossplane's `required_schemas` | -| `kro/graph/schema/resolver/crd_resolver.go` | Resolve schemas from CRDs (fallback path) | -| `input/v1beta1/input.go` | Function-kro input types (`ResourceGraph`, `Resource`, `ExternalRef`, etc.) | +--- ## Upstream Packages We Vendor (Allowlist) @@ -289,81 +853,54 @@ These are the **only** upstream `pkg/` packages we vendor. Each maps to our `kro | `pkg/runtime/` | `kro/runtime/` | Runtime execution, template resolution | | `pkg/metadata/` | `kro/metadata/` | Labels, finalizers, GVK utilities | -**Rule: Any upstream package NOT in this list should NOT be copied.** If upstream adds new `pkg/` directories in a future release, evaluate whether they fall within the graph/CEL/runtime layer before including them. When in doubt, skip them — function-kro's entry point (`fn.go`) and Crossplane's function SDK replace everything outside these four packages. +**Rule: Any upstream package NOT in this list should NOT be copied.** If upstream adds new `pkg/` directories in a future release, evaluate whether they fall within the graph/CEL/runtime layer before including them. When in doubt, skip them. ### Known exclusions (non-exhaustive) -These upstream packages exist but are **not vendored** because their functionality is replaced by Crossplane's composition function framework or our own input types: - | Upstream Package | Why We Skip It | |------------------|----------------| | `api/` | We have our own input types (`input/v1beta1/`) | | `pkg/controller/` | Replaced by Crossplane function framework (`fn.go`) | -| `pkg/simpleschema/` | Crossplane provides OpenAPI schemas directly; we use `SchemaMapResolver` and `CRDSchemaResolver` | +| `pkg/simpleschema/` | Crossplane provides OpenAPI schemas directly | | `pkg/dynamiccontroller/` | Replaced by Crossplane function framework | | `pkg/testutil/` | Upstream test utilities tied to controller patterns we don't use | -This list is illustrative, not exhaustive. The allowlist above is the authoritative guide — if a package isn't listed there, don't copy it. - -## New Features Integrated - -### Collections (forEach) - -v0.8.x added support for expanding a single resource template into multiple resources using `forEach` dimensions: +--- -1. Input type has `ForEach []ForEachDimension` field -2. Builder parses forEach dimensions and creates `NodeTypeCollection` nodes -3. Runtime's `node.GetDesired()` returns multiple resources for collections -4. fn.go handles collections by creating uniquely-named composed resources (`id-0`, `id-1`, ...) -5. Collection readiness checks all items via `isCollectionReady()` using the `each` variable +## Schema Resolution Architecture -### External References - -Function-kro added support for referencing existing cluster resources (not in upstream KRO): - -1. `ExternalRef` type in input defines apiVersion/kind/metadata for lookup -2. External ref metadata supports CEL expressions (e.g., `${schema.spec.configMapName}`) -3. Builder creates `NodeTypeExternal` nodes that are read-only in the graph -4. Runtime resolves identity via `GetDesiredIdentity()` (no readiness gating) -5. fn.go requests external resources from Crossplane and injects observed state -6. External refs are excluded from desired composed resources - -## Schema Resolution - -Function-kro uses capability-based detection to choose a single resolution path: +Function-kro uses capability-based detection in `fn.go` to choose a schema resolution path. This is NOT in the KRO libraries — it's in our `fn.go` entry point. But it's documented here because it explains why the adaptations above exist. ### Path 1: Required Schemas (Crossplane v2.2+) ``` HasCapability(REQUIRED_SCHEMAS) → response.RequireSchema() → request.GetRequiredSchemas() - → structToSpecSchema() → SchemaMapResolver → Combined Resolver + → structToSpecSchema() → SchemaMapResolver → NewCombinedResolverFromSchemas() + → resolver.SchemaResolver passed to NewBuilder() ``` ### Path 2: CRDs (Fallback for older Crossplane) ``` !HasCapability(REQUIRED_SCHEMAS) → request CRDs via required_resources - → request.GetRequiredResources() → CRDSchemaResolver → Combined Resolver + → request.GetRequiredResources() → NewCRDSchemaResolver() + → NewCombinedResolverFromCRDs() → resolver.SchemaResolver passed to NewBuilder() ``` -Both paths produce the same `resolver.SchemaResolver` interface that the graph builder consumes. The combined resolver tries the Crossplane-provided schemas first, then falls back to compiled-in core Kubernetes type schemas. +Both paths produce the same `resolver.SchemaResolver` interface that the graph builder consumes. + +--- -## Testing +## Known Quality Issues -Test coverage in `fn_test.go`: -- `MissingSchemas` — validates CRD fallback path when schemas unavailable -- `DesiredXROnlyContainsDeclaredStatus` — verifies SSA compliance for XR status -- `DesiredComposedResourceExcludesObservedFields` — confirms composed resources only have template fields -- `ExternalRefUsedInTemplate` — external refs available in CEL and excluded from desired output -- `ExternalRefWithCELExpressionInName` — CEL expression evaluation in external ref identity metadata +These are minor issues identified during audit that a future cleanup could address: -Validation: -- `go build ./...` passes -- `go test ./...` all tests pass -- `golangci-lint run` passes +1. **`CRDSchemaResolver` uses non-local import path.** Uses `github.com/upbound/function-kro/kro/graph/schema` instead of the `sigs.k8s.io/kro` path convention. +2. **Step numbering in `buildRGResource`.** Steps 8/9/10 should be 7/8/9 after removing step 7. +3. **Trailing blank lines.** `builder.go` and `metadata/groupversion.go`. ## Known Limitations -1. **Collection resource naming:** Collections use index-based naming (`resourceId-0`, `resourceId-1`). A more robust naming scheme may be needed for stable identity across reconciliations. +1. **No cluster-scope validation.** Upstream validates that cluster-scoped resources don't set a namespace (via REST mapper). Function-kro can't perform this check. Users setting namespace on cluster-scoped resources will get an error from Crossplane/the provider rather than from the function. -2. **External refs in collections:** Not fully tested with forEach expansion. +## Upstream Behavior We Intentionally Don't Need -3. **Cluster-scoped instances:** `normalizeNamespaces()` in the runtime defaults to XR namespace. When cluster-scoped instances are supported, this may need adjustment. +1. **Namespace auto-defaulting (removed).** Upstream auto-defaults namespace from the instance for namespaced resources. We removed this (`normalizeNamespaces` in `runtime/node.go`) because Crossplane automatically sets the namespace on composed resources at the framework level. Composition authors don't need to set namespace in their resource templates. diff --git a/patches/v0.8.x_UPSTREAM_CHANGES.md b/patches/v0.8.x_UPSTREAM_CHANGES.md deleted file mode 100644 index c6f4170..0000000 --- a/patches/v0.8.x_UPSTREAM_CHANGES.md +++ /dev/null @@ -1,390 +0,0 @@ -# KRO v0.7.1 → v0.8.x Upstream Changes Analysis - -This document summarizes the changes in upstream KRO between v0.7.1 and v0.8.x that are relevant for upgrading function-kro. Use this alongside `v0.7.1_PATCHES.md` (which describes our adaptations) when performing the upgrade. - -**Source:** https://github.com/kubernetes-sigs/kro/releases/tag/v0.8.0 - ---- - -## Executive Summary - -The major change in v0.8.0 is **Collections support** (KREP-002), which allows a single resource template to expand into multiple resources at runtime via a new `forEach` directive. This involved a **runtime rewrite** but the core graph-building logic remains similar. - -### Impact Assessment for function-kro - -| Change | Impact | Action Required | -|--------|--------|-----------------| -| Collections/forEach | **Medium** | Add `ForEach` field to input types, update runtime | -| Runtime rewrite | **Low** | Copy new runtime, verify API compatibility | -| Recursive custom types | **None** | SimpleSchema feature, not used in function-kro | -| Breaking schema detection | **None** | Controller feature, not applicable | -| CEL library extensions | **Low** | Comes free with new code | -| Bug fixes | **Low** | Comes free with new code | - ---- - -## 1. Collections Support (forEach) - -### What Changed - -A new `ForEach` field was added to the Resource type, allowing one template to generate multiple resources: - -```yaml -# Example: Create a bucket in each region -resources: - - id: bucket - forEach: - - region: ${schema.spec.regions} # CEL expression returning a list - template: - apiVersion: s3.aws.upbound.io/v1beta1 - kind: Bucket - metadata: - name: ${schema.spec.name}-${region} # 'region' is the iterator variable - spec: - forProvider: - region: ${region} -``` - -### New Types - -**`api/v1alpha1/resourcegraphdefinition_types.go`:** -```go -type Resource struct { - // ... existing fields ... - - // ForEach expands this resource into a collection of resources. - // +kubebuilder:validation:Optional - ForEach []ForEachDimension `json:"forEach,omitempty"` -} - -// ForEachDimension is map[string]string with exactly one entry -// The key is the iterator variable name, the value is a CEL expression returning a list -type ForEachDimension map[string]string -``` - -### Runtime Changes - -**New file `pkg/runtime/collection.go`:** -- `evaluatedDimension` - holds evaluated forEach dimension (name + values) -- `cartesianProduct()` - generates all combinations for multi-dimensional forEach -- `evaluateForEach()` - evaluates forEach expressions, returns iterator contexts -- `hardResolveCollection()` - processes collection nodes by iterating -- `isCollectionReady()` - checks if all collection items are ready - -**Modified `pkg/runtime/node.go`:** -```go -type Node struct { - Spec *graph.Node - - // For collections: multiple instances instead of single - desired []*unstructured.Unstructured // Was: single *unstructured.Unstructured - observed []*unstructured.Unstructured - - forEachExprs []*expressionEvaluationState // NEW - // ... other fields -} -``` - -### Action for function-kro - -1. **Update `input/v1beta1/input.go`:** - ```go - type Resource struct { - // ... existing fields ... - ForEach []map[string]string `json:"forEach,omitempty"` - } - ``` - -2. **Copy new runtime files** - The collection logic should work as-is - -3. **Update `fn.go`** - Handle multiple resources per collection node: - ```go - // Instead of one resource per ID, may have multiple for forEach resources - for _, id := range rt.TopologicalOrder() { - resources := rt.GetRenderedResources(id) // Note: plural - for i, r := range resources { - name := fmt.Sprintf("%s-%d", id, i) // or use forEach key - dcds[resource.Name(name)] = &resource.DesiredComposed{Resource: r} - } - } - ``` - ---- - -## 2. Graph Package Changes - -### Builder Constructor - -**v0.8.0 upstream:** -```go -func NewBuilder(clientConfig *rest.Config, httpClient *http.Client) (*Builder, error) -``` - -**function-kro current:** -```go -func NewBuilder(schemaResolver resolver.SchemaResolver, restMapper meta.RESTMapper) *Builder -``` - -**Action:** Keep our signature. The upstream constructor builds the resolver internally from `rest.Config`, but we receive schemas from Crossplane. Our adaptation (accepting resolver as parameter) remains valid. - -### NewResourceGraphDefinition - -**v0.8.0 upstream:** -```go -func (b *Builder) NewResourceGraphDefinition(originalCR *v1alpha1.ResourceGraphDefinition) (*Graph, error) -``` - -**function-kro current:** -```go -func (b *Builder) NewResourceGraphDefinition(rg *v1beta1.ResourceGraph, xrSchema *spec.Schema) (*Graph, error) -``` - -**Action:** Keep our signature. We receive a `ResourceGraph` input (not a full RGD CR) plus XR schema from Crossplane. May need to adapt internal logic if upstream changed how the graph is built. - -### Graph Struct - -**v0.8.0 upstream:** -```go -type Graph struct { - DAG *dag.DirectedAcyclicGraph[string] - Instance *Node - Nodes map[string]*Node - Resources map[string]*Node // backward compat alias - TopologicalOrder []string - CRD *extv1.CustomResourceDefinition -} -``` - -**Changes from v0.7.1:** -- Renamed `Resources` to `Nodes` (with `Resources` as alias) -- Resource type likely renamed to `Node` - -**Action:** Adapt naming if needed. The `CRD` field is not needed in function-kro. - -### Node vs Resource Naming - -v0.8.0 appears to use `Node` terminology instead of `Resource` in the graph package. This is likely a rename for clarity (since "Resource" is overloaded in Kubernetes). - -**Action:** If the rename happened, update our code to match. The struct contents should be similar. - ---- - -## 3. Runtime Package Changes - -### Constructor Change - -**v0.8.0 upstream:** -```go -func FromGraph(g *graph.Graph, instance *unstructured.Unstructured) (*Runtime, error) -``` - -**function-kro current (via Graph method):** -```go -func (g *Graph) NewGraphRuntime(instance *unstructured.Unstructured) (*Runtime, error) -``` - -**Action:** Either adapt to use `FromGraph()` or keep our method pattern. The key is ensuring the runtime is properly initialized with the graph and instance. - -### GetRenderedResource API - -**Current function-kro:** -```go -r, state := rt.GetRenderedResource(id) -``` - -**v0.8.0 (likely):** -```go -// For non-collection resources: -r, state := rt.GetRenderedResource(id) - -// For collection resources: -resources, state := rt.GetRenderedResources(id) // Returns slice -``` - -**Action:** Check the exact API. Collections require returning multiple resources per node ID. - -### State Management - -The runtime now tracks state for each item in a collection: -- `desired []*unstructured.Unstructured` - multiple desired states -- `observed []*unstructured.Unstructured` - multiple observed states - -**Action:** The `SetResource()` call in fn.go may need to handle collections: -```go -// Current: -rt.SetResource(id, &r.Resource.Unstructured) - -// May need for collections: -rt.SetObservedResources(id, observedList) -``` - ---- - -## 4. CEL Package Changes - -### AST Inspector Rewrite - -The CEL AST inspector was rewritten to use native CEL AST instead of custom parsing. This improves accuracy for complex expressions. - -**Files affected:** `pkg/cel/ast/inspector.go` - -**Action:** Copy new files. Should be backward compatible. - -### New Library Extensions - -Added support for Kubernetes CEL library extensions: -- URL functions -- Regex functions - -**Action:** Comes free with new code. May need to ensure dependencies are updated. - -### random.* Classification Fix - -Fixed bug where `random.*` functions were being classified as resource references. - -**Action:** Comes free with new code. - ---- - -## 5. Schema Package Changes - -### Recursive Custom Types - -SimpleSchema now supports custom types referencing other custom types. Uses DAG-based topological sorting. - -**Action:** None required. Function-kro doesn't use SimpleSchema (receives pre-built schemas from Crossplane). - -### Nested Path Preservation - -Fixed preservation of nested array/object paths in status schemas. - -**Files affected:** `pkg/graph/schema/` or similar - -**Action:** Copy new files. Should be backward compatible. - -### Timestamp Format - -Changed to use `date-time` format for timestamps (RFC 3339 compliance). - -**Action:** Comes free with new code. - ---- - -## 6. Validation Changes - -### Cluster-Scoped Resource Validation - -New validation rejects cluster-scoped resources that have a namespace set. - -**Files affected:** `pkg/graph/validation.go` - -**Action:** Copy new files. Improves validation, no adaptation needed. - -### Early apiVersion/kind Validation - -Uses `schema.ParseGroupVersion` for early validation. - -**Action:** Comes free with new code. - ---- - -## 7. Files to Copy - -When upgrading, copy these directories from upstream KRO v0.8.x to function-kro's `kro/` directory: - -``` -upstream pkg/graph/ → kro/graph/ -upstream pkg/cel/ → kro/cel/ -upstream pkg/runtime/ → kro/runtime/ -upstream pkg/metadata/ → kro/metadata/ -``` - -**Do NOT copy:** -- `pkg/controller/` - Controller logic not needed -- `pkg/simpleschema/` - SimpleSchema processing not needed -- `api/` - We have our own input types - ---- - -## 8. Dependency Updates - -Check upstream's `go.mod` for dependency version changes. Key dependencies: - -```go -github.com/google/cel-go // May have version bump for new features -k8s.io/* // May have version bumps -``` - ---- - -## 9. Upgrade Checklist - -### Pre-Upgrade -- [ ] Review this document -- [ ] Review `v0.7.1_PATCHES.md` for our adaptations -- [ ] Clone upstream KRO v0.8.x locally - -### Copy Phase -- [ ] Copy `pkg/graph/` → `kro/graph/` -- [ ] Copy `pkg/cel/` → `kro/cel/` -- [ ] Copy `pkg/runtime/` → `kro/runtime/` -- [ ] Copy `pkg/metadata/` → `kro/metadata/` - -### Adaptation Phase -- [ ] Re-apply Builder constructor adaptation (accept resolver + restMapper) -- [ ] Re-apply NewResourceGraphDefinition adaptation (accept schema not CRD) -- [ ] Re-apply REST mapping fallback in buildRGResource -- [ ] Re-add ObjectMeta injection in schema resolution -- [ ] Re-add/verify schema_map_resolver.go and crd_resolver.go -- [ ] Re-add/verify resolver.go with combined resolver constructors - -### Collections Phase -- [ ] Add `ForEach` field to `input/v1beta1/input.go` -- [ ] Update `fn.go` to handle collection resources (multiple per ID) -- [ ] Verify runtime collection APIs work with our schema resolution - -### Validation Phase -- [ ] Run `go build ./...` -- [ ] Run `go test ./...` -- [ ] Test with example compositions (with and without forEach) - -### Documentation Phase -- [ ] Update `v0.7.1_PATCHES.md` → `v0.8.x_PATCHES.md` -- [ ] Document any new adaptations required - ---- - -## 10. Known Risks - -### Collection Resource Naming - -When forEach generates multiple resources, each needs a unique name for Crossplane's composed resources map. Options: -1. Use `{id}-{index}` pattern -2. Use `{id}-{iteratorValue}` pattern -3. Derive from resource metadata.name - -Need to verify what pattern works best with Crossplane's SSA and resource tracking. - -### Collection Readiness - -Collections are ready only when ALL items pass readyWhen. Need to verify this integrates correctly with Crossplane's readiness reporting. - -### CEL Environment - -If upstream changed how CEL environments are created (e.g., for new library extensions), ensure our schema-based environment creation still works. - ---- - -## Appendix: Key Upstream Commits - -Based on release notes, key commits include: - -1. **feat: add Collections support + runtime/controller rewrite (#936)** - Main Collections PR -2. **feat(simpleschema): add support for recursive custom types (#950)** - Recursive types -3. **feat(crd): Detect and prevent breaking schema changes (#352)** - Breaking change detection -4. **fix(schema): preserve nested array/object paths in status schema (#972)** - Path fix -5. **fix(schema): use `date-time` format for timestamps (#973)** - Timestamp format -6. **feat: add early validation for apiVersion and kind (#980)** - Early validation -7. **fix(graph): reject cluster-scoped resources with namespace set (#976)** - Validation fix - -For detailed diffs, see: https://github.com/kubernetes-sigs/kro/compare/v0.7.1...v0.8.0 diff --git a/scripts/diff-upstream-kro.sh b/scripts/diff-upstream-kro.sh new file mode 100755 index 0000000..951908a --- /dev/null +++ b/scripts/diff-upstream-kro.sh @@ -0,0 +1,379 @@ +#!/bin/bash +# +# diff-upstream-kro.sh - Compare function-kro's KRO libraries against upstream KRO +# +# Usage: +# ./scripts/diff-upstream-kro.sh [options] +# +# Options: +# -u, --upstream-dir DIR Use existing upstream KRO checkout (default: clone to temp dir) +# -r, --ref REF Git ref to compare against (default: main) +# -f, --file FILE Only diff a specific file (relative to kro/ dir) +# -s, --summary Show summary only, not full diffs +# -l, --lines NUM Max lines of diff to show per file (0 for unlimited, default: 100) +# -n, --no-normalize Don't normalize import paths before diffing +# -h, --help Show this help message +# +# Examples: +# ./scripts/diff-upstream-kro.sh # Full diff against main +# ./scripts/diff-upstream-kro.sh -r v0.7.1 # Diff against tag v0.7.1 +# ./scripts/diff-upstream-kro.sh -f graph/builder.go # Diff only builder.go +# ./scripts/diff-upstream-kro.sh -s # Summary only +# + +set -eo pipefail + +# Colors for output +RED='\033[0;31m' +GREEN='\033[0;32m' +YELLOW='\033[1;33m' +BLUE='\033[0;34m' +NC='\033[0m' # No Color + +# Script directory and project root +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +LOCAL_KRO_DIR="$PROJECT_ROOT/kro" + +# Default options +UPSTREAM_DIR="" +GIT_REF="main" +SPECIFIC_FILE="" +SUMMARY_ONLY=false +MAX_LINES=100 # 0 = unlimited +NORMALIZE_IMPORTS=true +CLEANUP_TEMP=false + +# Upstream KRO repository +UPSTREAM_REPO="https://github.com/kubernetes-sigs/kro.git" + +# Import path mappings (local -> upstream patterns) +LOCAL_IMPORT_PREFIX="github.com/upbound/function-kro/kro" +UPSTREAM_IMPORT_PREFIX="sigs.k8s.io/kro/pkg" + +# Input type import path mappings (we use input/v1beta1, upstream uses api/v1alpha1) +LOCAL_INPUT_PREFIX="github.com/upbound/function-kro/input/v1beta1" +UPSTREAM_INPUT_PREFIX="sigs.k8s.io/kro/api/v1alpha1" + +# Upstream packages we vendor (only show upstream-only files from these) +# Everything else in upstream (controllers, simpleschema, etc.) is intentionally excluded. +VENDORED_PACKAGES="graph/ cel/ runtime/ metadata/" + +usage() { + head -30 "$0" | grep -E '^#' | sed 's/^# \?//' + exit 0 +} + +log_info() { + echo -e "${BLUE}INFO:${NC} $*" +} + +log_success() { + echo -e "${GREEN}OK:${NC} $*" +} + +log_warning() { + echo -e "${YELLOW}WARN:${NC} $*" +} + +log_error() { + echo -e "${RED}ERROR:${NC} $*" +} + +log_header() { + echo "" + echo -e "${BLUE}=== $* ===${NC}" + echo "" +} + +# Check if a file belongs to one of the vendored upstream packages +is_vendored_package() { + local file="$1" + for pkg in $VENDORED_PACKAGES; do + if [[ "$file" == "$pkg"* ]]; then + return 0 + fi + done + return 1 +} + +# Parse command line arguments +while [[ $# -gt 0 ]]; do + case $1 in + -u|--upstream-dir) + UPSTREAM_DIR="$2" + shift 2 + ;; + -r|--ref) + GIT_REF="$2" + shift 2 + ;; + -f|--file) + SPECIFIC_FILE="$2" + shift 2 + ;; + -s|--summary) + SUMMARY_ONLY=true + shift + ;; + -l|--lines) + MAX_LINES="$2" + shift 2 + ;; + -n|--no-normalize) + NORMALIZE_IMPORTS=false + shift + ;; + -h|--help) + usage + ;; + *) + log_error "Unknown option: $1" + usage + ;; + esac +done + +# Clone or use existing upstream directory +setup_upstream() { + if [[ -n "$UPSTREAM_DIR" ]]; then + if [[ ! -d "$UPSTREAM_DIR" ]]; then + log_error "Upstream directory does not exist: $UPSTREAM_DIR" + exit 1 + fi + log_info "Using existing upstream directory: $UPSTREAM_DIR" + else + UPSTREAM_DIR=$(mktemp -d) + CLEANUP_TEMP=true + log_info "Cloning upstream KRO to $UPSTREAM_DIR..." + git clone --depth 1 --branch "$GIT_REF" "$UPSTREAM_REPO" "$UPSTREAM_DIR" 2>/dev/null || \ + git clone --depth 1 "$UPSTREAM_REPO" "$UPSTREAM_DIR" 2>/dev/null + + if [[ "$GIT_REF" != "main" ]]; then + (cd "$UPSTREAM_DIR" && git fetch --depth 1 origin "$GIT_REF" && git checkout "$GIT_REF") 2>/dev/null || true + fi + fi + + UPSTREAM_PKG_DIR="$UPSTREAM_DIR/pkg" + if [[ ! -d "$UPSTREAM_PKG_DIR" ]]; then + log_error "Upstream pkg/ directory not found at $UPSTREAM_PKG_DIR" + exit 1 + fi + + log_info "Comparing against upstream ref: $GIT_REF" +} + +cleanup() { + if [[ "$CLEANUP_TEMP" == "true" && -n "$UPSTREAM_DIR" ]]; then + log_info "Cleaning up temporary directory..." + rm -rf "$UPSTREAM_DIR" + fi +} + +trap cleanup EXIT + +# Normalize imports in a file for comparison +normalize_imports() { + local file="$1" + if [[ "$NORMALIZE_IMPORTS" == "true" ]]; then + # Normalize all known import paths to a canonical form for comparison + # This handles: our local imports, old upstream path, current upstream path, + # and input type imports (our input/v1beta1 vs upstream api/v1alpha1) + sed -e "s|$LOCAL_IMPORT_PREFIX|$UPSTREAM_IMPORT_PREFIX|g" \ + -e "s|$LOCAL_INPUT_PREFIX|$UPSTREAM_INPUT_PREFIX|g" \ + -e "s|github.com/kro-run/kro/pkg|$UPSTREAM_IMPORT_PREFIX|g" \ + -e "s|github.com/kubernetes-sigs/kro/pkg|$UPSTREAM_IMPORT_PREFIX|g" \ + -e "s|v1beta1\\.KRODomainName|v1alpha1.KRODomainName|g" \ + "$file" + else + cat "$file" + fi +} + +# Map local path to upstream path +# Our kro/ directory maps to upstream's pkg/ directory +get_upstream_path() { + local local_rel_path="$1" + # Simply replace kro/ prefix concept - local path is relative to kro/ + # upstream path is relative to pkg/ + echo "$UPSTREAM_PKG_DIR/$local_rel_path" +} + +# Compare a single file +diff_file() { + local local_rel_path="$1" # e.g., "graph/builder.go" + local local_file="$LOCAL_KRO_DIR/$local_rel_path" + local upstream_file + upstream_file=$(get_upstream_path "$local_rel_path") + + # Check if files exist + if [[ ! -f "$local_file" ]]; then + log_error "Local file not found: $local_file" + return 1 + fi + + if [[ ! -f "$upstream_file" ]]; then + if [[ "$SUMMARY_ONLY" != "true" ]]; then + echo -e "${YELLOW}[LOCAL ONLY]${NC} $local_rel_path (no upstream equivalent)" + fi + return 2 + fi + + # Create temp files with normalized imports + local tmp_local + local tmp_upstream + tmp_local=$(mktemp) + tmp_upstream=$(mktemp) + + normalize_imports "$local_file" > "$tmp_local" + normalize_imports "$upstream_file" > "$tmp_upstream" + + # Perform diff (plain for stats, colored for display) + local diff_output + diff_output=$(diff -u "$tmp_upstream" "$tmp_local" 2>/dev/null || true) + + if [[ -z "$diff_output" ]]; then + rm -f "$tmp_local" "$tmp_upstream" + if [[ "$SUMMARY_ONLY" != "true" ]]; then + echo -e "${GREEN}[IDENTICAL]${NC} $local_rel_path" + fi + return 0 + else + local added + local removed + added=$(echo "$diff_output" | grep -c '^+[^+]' || true) + removed=$(echo "$diff_output" | grep -c '^-[^-]' || true) + + if [[ "$SUMMARY_ONLY" == "true" ]]; then + rm -f "$tmp_local" "$tmp_upstream" + echo -e "${RED}[MODIFIED]${NC} $local_rel_path (+$added/-$removed lines)" + else + echo "" + echo -e "${RED}[MODIFIED]${NC} $local_rel_path (+$added/-$removed lines)" + echo "─────────────────────────────────────────────────────────────────" + + # Use git diff for colored output (green adds, red removals) + local colored_diff + colored_diff=$(git diff --no-index --color=always \ + -- "$tmp_upstream" "$tmp_local" 2>/dev/null | \ + sed -e "s|a$tmp_upstream|a/upstream/$local_rel_path|g" \ + -e "s|b$tmp_local|b/local/$local_rel_path|g" \ + -e "s|$tmp_upstream|upstream/$local_rel_path|g" \ + -e "s|$tmp_local|local/$local_rel_path|g" || true) + + rm -f "$tmp_local" "$tmp_upstream" + + # Show the colored diff, optionally limited + if [[ "$MAX_LINES" -eq 0 ]]; then + echo "$colored_diff" + else + echo "$colored_diff" | head -"$MAX_LINES" + local total_lines + total_lines=$(echo "$colored_diff" | wc -l) + if [[ $total_lines -gt $MAX_LINES ]]; then + echo "" + echo -e "${YELLOW}... (output truncated at $MAX_LINES lines, $total_lines total - use -l 0 for unlimited)${NC}" + fi + fi + echo "" + fi + return 3 # Modified + fi +} + +# Find all Go files in local kro directory (excluding tests) +find_local_files() { + find "$LOCAL_KRO_DIR" -type f -name "*.go" ! -name "*_test.go" | \ + sed "s|$LOCAL_KRO_DIR/||" | \ + sort +} + +# Find all Go files in upstream pkg directory (excluding tests) +find_upstream_files() { + find "$UPSTREAM_PKG_DIR" -type f -name "*.go" ! -name "*_test.go" | \ + sed "s|$UPSTREAM_PKG_DIR/||" | \ + sort +} + +# Main comparison logic +main() { + setup_upstream + + log_header "Comparing function-kro/kro against upstream KRO ($GIT_REF)" + + local identical=0 + local modified=0 + local local_only=0 + local upstream_only=0 + local errors=0 + + # Track which files we've seen + local seen_files_file + seen_files_file=$(mktemp) + + if [[ -n "$SPECIFIC_FILE" ]]; then + # Diff specific file + diff_file "$SPECIFIC_FILE" + rm -f "$seen_files_file" + exit $? + fi + + # Compare all local files + log_header "Comparing local files against upstream" + + while IFS= read -r rel_path; do + echo "$rel_path" >> "$seen_files_file" + + local rc=0 + diff_file "$rel_path" || rc=$? + case $rc in + 0) ((identical++)) ;; + 2) ((local_only++)) ;; + 3) ((modified++)) ;; + *) ((errors++)) ;; + esac + done < <(find_local_files) + + # Check for upstream files we don't have (only in vendored packages) + log_header "Checking for upstream-only files (in vendored packages)" + + while IFS= read -r rel_path; do + if ! grep -q "^${rel_path}$" "$seen_files_file" 2>/dev/null; then + # Only report files from packages we vendor + if is_vendored_package "$rel_path"; then + echo -e "${YELLOW}[UPSTREAM ONLY]${NC} $rel_path" + ((upstream_only++)) + fi + fi + done < <(find_upstream_files) + + rm -f "$seen_files_file" + + # Print summary + log_header "Summary" + + echo "Comparison against: $GIT_REF" + echo "" + echo -e "${GREEN}Identical:${NC} $identical files" + echo -e "${RED}Modified:${NC} $modified files" + echo -e "${YELLOW}Local only:${NC} $local_only files (function-kro specific)" + echo -e "${YELLOW}Upstream only:${NC} $upstream_only files (not in function-kro)" + + if [[ $errors -gt 0 ]]; then + echo -e "${RED}Errors:${NC} $errors files" + fi + + echo "" + + if [[ $modified -gt 0 ]]; then + echo "Modified files require manual review when updating from upstream." + echo "Run with specific file to see full diff: $0 -f " + fi + + if [[ $upstream_only -gt 0 ]]; then + echo "" + echo "Upstream-only files (in vendored packages) may need to be adopted." + fi +} + +main "$@"