Integrate schema change detection and add some helpers for registering the conversion functions automatically #567
Conversation
sergenyalcin
changed the title
📝 WalkthroughWalkthroughAdds CRD schema breaking-change detection and auto-conversion: a schemadiff CLI that emits per-CRD change reports, config package functions to register auto-conversions and exclude type-changed fields from identity copying, resource-level opt-outs/options, documentation, test fixtures, and wide unit/integration test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as schemadiff CLI
participant FS as File System
participant Differ as SchemaDiffer
participant Writer as JSON Writer
User->>CLI: run with --crd-dir, --out
CLI->>FS: list YAML/YML files
FS-->>CLI: file list
loop per CRD file
CLI->>Differ: create self-diff (Upjet ext disabled)
Differ-->>CLI: raw diffs
CLI->>CLI: convert raw diffs -> ChangeReport
CLI->>CLI: if non-empty aggregate into map[group/kind]=ChangeReport
end
CLI->>Writer: marshal aggregated map to JSON
Writer->>FS: write output file (0600)
FS-->>User: success / error
sequenceDiagram
participant Provider
participant Config as config package
participant Parser as ChangeReport Parser
participant Registry as Conversion Registry
participant Resource as Resource Config
Provider->>Config: ExcludeTypeChangesFromIdentity(crdSchemaChanges)
Config->>Parser: parse change reports
Parser-->>Config: structured change map
loop per resource
Config->>Resource: add trimmed paths to IdentityConversionExcludePaths
end
Provider->>Config: RegisterAutoConversions(crdSchemaChanges)
Config->>Parser: parse change reports
loop per resource and change
alt field_added / field_deleted
Config->>Registry: register bidirectional copy conversions
else type_changed
Config->>Config: determine conversion direction (string↔number, string↔bool)
Config->>Registry: register conversion funcs and identity exclusions
end
end
Registry-->>Provider: conversions registered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (56)
📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (5)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*_test.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
pkg/**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/pkg/config/**⚙️ CodeRabbit configuration file
Files:
**/*.md⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (4)📚 Learning: 2026-01-13T21:29:32.867ZApplied to files:
📚 Learning: 2026-01-13T21:29:32.867ZApplied to files:
📚 Learning: 2026-01-13T21:29:32.867ZApplied to files:
📚 Learning: 2026-01-13T21:29:32.867ZApplied to files:
🧬 Code graph analysis (2)pkg/config/conversion.go (3)
tests/conversion/test_resource.go (1)
🔇 Additional comments (23)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@cmd/schemadiff/main.go`:
- Around line 35-37: The error message in the validation for out (the nil/empty
check using kingpin.Fatalf) is incomplete; update the kingpin.Fatalf call in
cmd/schemadiff/main.go (the block that checks if out == nil || *out == "") to a
clear, grammatically complete message such as "output directory is required" or
"must specify output directory" so the error describes what is required.
In `@docs/breaking-change-detection.md`:
- Line 20: The TOC contains a broken link to "#troubleshooting" which has no
matching "## Troubleshooting" heading; either add a "## Troubleshooting" section
with relevant troubleshooting content to the document (so the TOC link resolves)
or remove the "[Troubleshooting](`#troubleshooting`)" entry from the TOC; look for
the literal TOC line "- [Troubleshooting](`#troubleshooting`)" and the expected
heading "## Troubleshooting" when making the change.
In `@go.mod`:
- Line 64: The go.mod currently pulls in github.com/getkin/kin-openapi v0.108.0
(indirect) which is vulnerable; identify which direct module in your dependency
graph depends on github.com/getkin/kin-openapi (use go mod graph or go list -m
all to locate the parent module), then bump that direct dependency to a release
that itself requires kin-openapi v0.131.0 or later (use go get
<direct-module>@<fixed-version>), run go mod tidy to update go.mod/go.sum, and
verify that github.com/getkin/kin-openapi is now recorded at v0.131.0+ as an
indirect dependency.
In `@pkg/config/testdata/valid/type-change-bool-to-string.json.license`:
- Around line 1-3: Update the SPDX copyright year from 2026 to 2025 in the
SPDX-FileCopyrightText line (the line containing "2026 The Crossplane Authors
<https://crossplane.io>") so it reads "2025 The Crossplane Authors
<https://crossplane.io>" ensuring the SPDX-License-Identifier line remains
unchanged.
In `@tests/conversion/integration_test.go`:
- Around line 95-126: The json.Unmarshal calls into data1..data6 ignore returned
errors, so parse failures get swallowed; update each json.Unmarshal(fixtureX,
&dataX) to capture its error and fail the test on error (e.g., if err :=
json.Unmarshal(...); err != nil { t.Fatalf("unmarshal fixtureX: %v", err) }) so
invalid fixtures cause the test to fail before merging into merged and
marshalling mergedJSON.
🧹 Nitpick comments (6)
pkg/config/conversion.go (2)
118-118: Useerrors.Errorfinstead offmt.Errorffor consistency.Per coding guidelines, use
github.com/pkg/errorsfor error handling. Since crossplane-runtime's errors package wraps pkg/errors, useerrors.Errorffor consistency with the rest of the codebase.♻️ Suggested fix
- return fmt.Errorf("unsupported type for number conversion in field %q: got Terraform schema type %q", change.Path, sch.Type) + return errors.Errorf("unsupported type for number conversion in field %q: got Terraform schema type %q", change.Path, sch.Type)Also, you can remove
"fmt"from the imports since it would no longer be used.
86-136: Consider logging unhandled type change combinations.The type change handling covers
string↔numberandstring↔boolean, but other combinations (e.g.,number↔boolean,integer↔array) are silently skipped. Consider adding a log warning for unhandled combinations to aid debugging when conversions don't work as expected.♻️ Example implementation
case change.OldValue == "boolean" && change.NewValue == "string": // Boolean → String: true/false → "true"/"false" r.Conversions = append(r.Conversions, conversion.NewFieldTypeConversion(cv.OldVersion, cv.NewVersion, change.Path, conversion.BoolToString), conversion.NewFieldTypeConversion(cv.NewVersion, cv.OldVersion, change.Path, conversion.StringToBool)) + default: + // Log unhandled type change combinations for debugging + // Consider using a logger if available in Provider context }docs/breaking-change-detection.md (1)
39-44: Add language specifier to code blocks.Several fenced code blocks in this document lack language specifiers, which affects syntax highlighting and linting. For the architecture diagram at lines 41-44 and similar text blocks, consider using
textorplaintextas the language identifier.Example fix for the architecture block
**Architecture:** -``` +```text Build Time: CRDs → schemadiff tool → JSON manifest Runtime: JSON → Auto-registration → Conversion functions → Webhooks</details> </blockquote></details> <details> <summary>pkg/config/testing_helpers.go (2)</summary><blockquote> `145-156`: **Unreachable return statement after `require.NoError`.** If all file reads fail, `require.NoError` at line 154 will call `t.FailNow()` and stop test execution. Line 155 is unreachable in the error path. Consider restructuring for clarity. <details> <summary>Proposed refactor</summary> ```diff var data []byte var err error for _, path := range paths { data, err = os.ReadFile(path) //nolint:gosec if err == nil { return data } } - require.NoError(t, err, "failed to load test fixture: %s", relativePath) - return data + require.FailNow(t, "failed to load test fixture: %s (tried paths: %v)", relativePath, paths) + return nil // unreachable, satisfies compiler
29-32: Consider removing redundant unexported wrappers.Each exported helper has a corresponding unexported wrapper that simply calls the exported version. If these are solely for internal use within the same package, the wrappers add no value since package-internal code can call the exported functions directly. If the intent is to maintain a distinction, a comment explaining the rationale would help future maintainers.
Also applies to: 51-54, 68-71, 85-88, 102-105, 119-122, 158-161
tests/conversion/test_resource.go (1)
171-185: DeepCopyObject performs shallow copy of map values and omits embedded spec/status fields.The map iteration copies references to values rather than deep-copying nested structures. Additionally,
Spec.ResourceSpecandStatus.ResourceStatusembedded fields are not copied.For current test scenarios using simple scalar values, this may be sufficient. However, if tests evolve to use nested maps/slices or rely on embedded Crossplane fields, this could cause unexpected aliasing issues.
♻️ More complete DeepCopyObject (optional)
func (tr *TestResource) DeepCopyObject() runtime.Object { out := &TestResource{} out.TypeMeta = tr.TypeMeta out.ObjectMeta = *tr.ObjectMeta.DeepCopy() + out.Spec.ResourceSpec = *tr.Spec.ResourceSpec.DeepCopy() out.Spec.ForProvider = make(TestResourceParameters) for k, v := range tr.Spec.ForProvider { - out.Spec.ForProvider[k] = v + out.Spec.ForProvider[k] = deepCopyValue(v) } + out.Status.ResourceStatus = *tr.Status.ResourceStatus.DeepCopy() out.Status.AtProvider = make(TestResourceObservation) for k, v := range tr.Status.AtProvider { - out.Status.AtProvider[k] = v + out.Status.AtProvider[k] = deepCopyValue(v) } + out.TerraformResourceType = tr.TerraformResourceType return out } + +func deepCopyValue(v interface{}) interface{} { + // For test purposes, simple values are sufficient + // Add nested map/slice handling if needed + return v +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (65)
cmd/schemadiff/main.godocs/README.mddocs/breaking-change-detection.mdgo.modpkg/config/conversion.gopkg/config/conversion_test.gopkg/config/provider.gopkg/config/resource.gopkg/config/testdata/invalid/malformed.jsonpkg/config/testdata/invalid/malformed.json.licensepkg/config/testdata/invalid/missing-change-type.jsonpkg/config/testdata/invalid/missing-change-type.json.licensepkg/config/testdata/invalid/type-change-bool-schema-mismatch.jsonpkg/config/testdata/invalid/type-change-bool-schema-mismatch.json.licensepkg/config/testdata/invalid/type-change-missing-values.jsonpkg/config/testdata/invalid/type-change-missing-values.json.licensepkg/config/testdata/invalid/type-change-same-values.jsonpkg/config/testdata/invalid/type-change-same-values.json.licensepkg/config/testdata/invalid/type-change-unsupported.jsonpkg/config/testdata/invalid/type-change-unsupported.json.licensepkg/config/testdata/invalid/unknown-change-type.jsonpkg/config/testdata/invalid/unknown-change-type.json.licensepkg/config/testdata/valid/deeply-nested.jsonpkg/config/testdata/valid/deeply-nested.json.licensepkg/config/testdata/valid/empty.jsonpkg/config/testdata/valid/empty.json.licensepkg/config/testdata/valid/evolving-field.jsonpkg/config/testdata/valid/evolving-field.json.licensepkg/config/testdata/valid/exclude-multiple-paths.jsonpkg/config/testdata/valid/exclude-multiple-paths.json.licensepkg/config/testdata/valid/field-addition.jsonpkg/config/testdata/valid/field-addition.json.licensepkg/config/testdata/valid/field-deletion.jsonpkg/config/testdata/valid/field-deletion.json.licensepkg/config/testdata/valid/multiple-changes.jsonpkg/config/testdata/valid/multiple-changes.json.licensepkg/config/testdata/valid/multiple-resources.jsonpkg/config/testdata/valid/multiple-resources.json.licensepkg/config/testdata/valid/multiple-type-changes.jsonpkg/config/testdata/valid/multiple-type-changes.json.licensepkg/config/testdata/valid/no-changes.jsonpkg/config/testdata/valid/no-changes.json.licensepkg/config/testdata/valid/three-versions.jsonpkg/config/testdata/valid/three-versions.json.licensepkg/config/testdata/valid/type-change-bool-to-string.jsonpkg/config/testdata/valid/type-change-bool-to-string.json.licensepkg/config/testdata/valid/type-change-number-to-string.jsonpkg/config/testdata/valid/type-change-number-to-string.json.licensepkg/config/testdata/valid/type-change-string-to-bool.jsonpkg/config/testdata/valid/type-change-string-to-bool.json.licensepkg/config/testdata/valid/type-change-string-to-number.jsonpkg/config/testdata/valid/type-change-string-to-number.json.licensepkg/config/testing_helpers.gotests/conversion/integration_test.gotests/conversion/test_resource.gotests/testdata/bool-to-string.jsontests/testdata/bool-to-string.json.licensetests/testdata/multiple-changes.jsontests/testdata/multiple-changes.json.licensetests/testdata/number-to-string.jsontests/testdata/number-to-string.json.licensetests/testdata/string-to-bool.jsontests/testdata/string-to-bool.json.licensetests/testdata/string-to-number.jsontests/testdata/string-to-number.json.license
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Do not useanytype throughout codebase - use concrete types or type parameters instead
Use pointer types for optional fields in generated structs
Avoid type aliases in favor of explicit types
Usegithub.com/pkg/errorsfor error wrapping with context
Return errors from functions instead of panicking, except for impossible states
Wrap errors with context using patterns like:errors.Wrap(err, "cannot configure resource")
Avoid circular dependencies between packages
Files:
pkg/config/resource.gopkg/config/conversion_test.gotests/conversion/integration_test.gopkg/config/conversion.gopkg/config/provider.gocmd/schemadiff/main.gotests/conversion/test_resource.gopkg/config/testing_helpers.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Public API packages should be organized under
pkg/directory
Files:
pkg/config/resource.gopkg/config/conversion_test.gopkg/config/conversion.gopkg/config/provider.gopkg/config/testing_helpers.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Use standard Go testing only - no Ginkgo, Testify, or third-party test frameworks
Strongly prefer table-driven tests
Place test files in the same package as the code under test using*_test.gonaming
Files:
pkg/config/conversion_test.gotests/conversion/integration_test.go
go.mod
📄 CodeRabbit inference engine (CLAUDE.md)
go.mod: Module path must begithub.com/crossplane/upjet/v2
When testing in providers, addreplace github.com/crossplane/upjet/v2 => ../upjetto provider's go.mod
Runmake modules.checkto verify go.mod/go.sum are tidy before committing
Files:
go.mod
🧠 Learnings (9)
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Use Kubernetes-native patterns via crossplane-runtime
Applied to files:
tests/testdata/multiple-changes.json.licensetests/testdata/string-to-number.json.licensepkg/config/testdata/valid/field-deletion.json.licensepkg/config/testdata/valid/multiple-resources.json.licensepkg/config/testdata/invalid/type-change-bool-schema-mismatch.json.licensepkg/config/testdata/valid/empty.json.licensepkg/config/testdata/valid/type-change-string-to-number.json.licensetests/testdata/string-to-bool.json.licensepkg/config/testdata/valid/multiple-type-changes.json.licensepkg/config/testdata/invalid/unknown-change-type.json.licensepkg/config/testdata/invalid/malformed.json.licensepkg/config/testdata/valid/no-changes.json.licensetests/testdata/number-to-string.json.licensepkg/config/testdata/valid/field-addition.json.licensepkg/config/testdata/invalid/missing-change-type.json.licensepkg/config/testdata/invalid/type-change-same-values.json.licensepkg/config/testdata/invalid/type-change-unsupported.json.licensepkg/config/testdata/valid/exclude-multiple-paths.json.licensepkg/config/testdata/valid/three-versions.json.licensepkg/config/testdata/valid/type-change-string-to-bool.json.licensepkg/config/testdata/valid/type-change-bool-to-string.json.licensepkg/config/testdata/invalid/type-change-missing-values.json.licensepkg/config/testdata/valid/evolving-field.json.licensetests/testdata/bool-to-string.json.licensepkg/config/testdata/valid/deeply-nested.json.licensepkg/config/testdata/valid/type-change-number-to-string.json.licensepkg/config/testdata/valid/multiple-changes.json.license
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Applies to **/*_test.go : Strongly prefer table-driven tests
Applied to files:
pkg/config/conversion_test.gotests/conversion/integration_test.gotests/conversion/test_resource.gopkg/config/testing_helpers.go
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Applies to **/*_test.go : Place test files in the same package as the code under test using `*_test.go` naming
Applied to files:
pkg/config/conversion_test.gopkg/config/testing_helpers.go
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Use interface-based design for extensibility with `resource.Terraformed` and `controller.ExternalClient` interfaces
Applied to files:
tests/conversion/test_resource.go
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Applies to go.mod : Run `make modules.check` to verify go.mod/go.sum are tidy before committing
Applied to files:
go.mod
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Applies to go.mod : When testing in providers, add `replace github.com/crossplane/upjet/v2 => ../upjet` to provider's go.mod
Applied to files:
go.mod
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Applies to go.mod : Module path must be `github.com/crossplane/upjet/v2`
Applied to files:
go.mod
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Applies to **/*_test.go : Use standard Go testing only - no Ginkgo, Testify, or third-party test frameworks
Applied to files:
go.mod
📚 Learning: 2026-01-13T21:29:32.867Z
Learnt from: CR
Repo: crossplane/upjet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T21:29:32.867Z
Learning: Applies to **/*.go : Avoid circular dependencies between packages
Applied to files:
go.mod
🧬 Code graph analysis (5)
pkg/config/conversion_test.go (2)
pkg/config/resource.go (2)
Resource(440-674)AutoConversionRegistrationOptions(692-739)pkg/config/conversion.go (2)
RegisterAutoConversions(51-142)ExcludeTypeChangesFromIdentity(163-190)
tests/conversion/integration_test.go (5)
pkg/config/testing_helpers.go (6)
NewTestProvider(18-27)NewTestResource(37-49)NewTestResourceWithIntField(59-66)NewTestResourceWithStringField(93-100)NewTestResourceWithBoolField(110-117)LoadTestFixture(129-156)pkg/resource/fake/terraformed.go (2)
Kind(242-242)AddToScheme(251-251)pkg/controller/conversion/registry.go (1)
RegisterConversions(80-92)tests/conversion/test_resource.go (3)
TestResource(18-28)TestResourceSpec(31-34)TestResourceParameters(37-37)pkg/controller/conversion/functions.go (1)
RoundTrip(102-104)
pkg/config/conversion.go (2)
pkg/config/resource.go (2)
AutoConversionRegistrationOptions(692-739)Resource(440-674)pkg/config/common.go (1)
GetSchema(155-179)
tests/conversion/test_resource.go (1)
pkg/config/resource.go (1)
Reference(209-236)
pkg/config/testing_helpers.go (2)
pkg/config/provider.go (1)
Provider(79-188)pkg/config/resource.go (2)
Resource(440-674)AutoConversionRegistrationOptions(692-739)
🪛 Biome (2.1.2)
pkg/config/testdata/invalid/malformed.json
[error] 12-12: expected , but instead found // Missing comma - malformed JSON
Remove // Missing comma - malformed JSON
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 14-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
🪛 markdownlint-cli2 (0.18.1)
docs/breaking-change-detection.md
20-20: Link fragments should be valid
(MD051, link-fragments)
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
186-186: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
191-191: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
196-196: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
539-539: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
555-555: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OSV Scanner (2.3.1)
go.mod
[HIGH] 64-64: github.com/getkin/kin-openapi 0.108.0: Improper Handling of Highly Compressed Data (Data Amplification) in github.com/getkin/kin-openapi/openapi3filter
(GO-2025-3533)
[HIGH] 64-64: github.com/getkin/kin-openapi 0.108.0: Improper Handling of Highly Compressed Data (Data Amplification) in github.com/getkin/kin-openapi/openapi3filter
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: check-diff
🔇 Additional comments (81)
pkg/config/testdata/valid/evolving-field.json (1)
1-32: LGTM!Well-structured test data that covers an important multi-version type evolution scenario. The progressive type changes (string → number → boolean) across three API versions effectively tests the conversion chain handling in the auto-conversion system.
pkg/config/testdata/valid/evolving-field.json.license (1)
1-3: LGTM!Standard SPDX license header following the REUSE specification for companion license files. This correctly provides Apache-2.0 licensing metadata for the associated JSON test data file.
pkg/config/testdata/invalid/type-change-bool-schema-mismatch.json.license (1)
1-3: LGTM! Standard SPDX license header.The SPDX license header is correctly formatted with proper copyright attribution and Apache-2.0 license identifier, appropriate for Crossplane test data files.
pkg/config/testdata/valid/multiple-resources.json.license (1)
1-3: LGTM! Standard SPDX license header.The SPDX license header is correctly formatted with proper copyright attribution and Apache-2.0 license identifier.
pkg/config/testdata/valid/field-deletion.json.license (1)
1-3: LGTM! Standard SPDX license header.The SPDX license header is correctly formatted with proper copyright attribution and Apache-2.0 license identifier.
pkg/config/testdata/invalid/unknown-change-type.json.license (1)
1-3: LGTM! Standard SPDX license header.The SPDX license header is correctly formatted with proper copyright attribution and Apache-2.0 license identifier.
pkg/config/testdata/valid/multiple-changes.json.license (1)
1-3: LGTM! Standard SPDX license header.The SPDX license header is correctly formatted with proper copyright attribution and Apache-2.0 license identifier.
pkg/config/testdata/valid/empty.json (1)
1-1: LGTM!Valid empty JSON object, appropriate for testing edge cases in the schema diff/conversion logic.
pkg/config/testdata/valid/type-change-string-to-bool.json.license (1)
1-3: LGTM!Standard SPDX license headers correctly formatted for the test data file.
pkg/config/testdata/valid/type-change-number-to-string.json.license (1)
1-3: LGTM!Standard SPDX license headers correctly formatted.
tests/testdata/string-to-number.json.license (1)
1-3: LGTM!Standard SPDX license headers correctly formatted.
tests/testdata/multiple-changes.json.license (1)
1-3: LGTM!Standard SPDX license headers correctly formatted.
tests/testdata/string-to-bool.json.license (1)
1-3: LGTM!Standard SPDX license metadata following project conventions.
pkg/config/testdata/valid/no-changes.json.license (1)
1-3: LGTM!Standard SPDX license metadata following project conventions.
tests/testdata/bool-to-string.json.license (1)
1-3: LGTM!Standard SPDX license metadata following project conventions.
pkg/config/testdata/valid/type-change-string-to-number.json.license (1)
1-3: LGTM!Standard SPDX license metadata following project conventions.
pkg/config/testdata/valid/type-change-string-to-bool.json (1)
1-19: LGTM!Well-structured test fixture for the string-to-boolean type conversion scenario. The JSON schema is consistent with other type-change test fixtures in the PR, with correct
pathPartsdecomposition and appropriatechangeTypedesignation.pkg/config/testdata/invalid/missing-change-type.json.license (1)
1-3: LGTM!Standard SPDX license header for the companion JSON test data file. Follows the REUSE specification pattern appropriately.
pkg/config/testdata/valid/empty.json.license (1)
1-3: LGTM!License header is consistent with other testdata license files in this PR.
pkg/config/testdata/valid/exclude-multiple-paths.json (1)
1-27: LGTM!Well-structured test fixture for validating multiple path exclusion scenarios. The JSON structure is consistent with the schema change detection format, and the
pathPartsarrays correctly decompose the dot-notation paths.pkg/config/testdata/valid/exclude-multiple-paths.json.license (1)
1-3: LGTM!License header follows the established pattern for testdata files.
pkg/config/testdata/invalid/type-change-same-values.json.license (1)
1-3: LGTM!License header is consistent with the project's testdata licensing approach.
tests/testdata/string-to-bool.json (1)
1-19: LGTM!The test fixture correctly defines the string-to-boolean type change scenario with proper structure. The
pathPartsarray accurately reflects the dot-separated path, and all required fields (path,pathParts,changeType,oldValue,newValue) are present and consistent with other type-change fixtures in the test suite.pkg/config/testdata/valid/multiple-type-changes.json.license (1)
1-3: LGTM!Standard SPDX license header consistent with other testdata license files in the repository.
tests/testdata/string-to-number.json (1)
1-19: LGTM!The test fixture correctly defines the string-to-number type change scenario. Structure and field values are consistent with the established testdata pattern.
pkg/config/testdata/invalid/type-change-missing-values.json (1)
1-17: LGTM!This invalid test fixture correctly omits
oldValueandnewValuefields for atype_changedentry, which should trigger validation errors in the conversion logic. Well-structured negative test case for error handling coverage.pkg/config/testdata/invalid/type-change-missing-values.json.license (1)
1-3: LGTM!Standard SPDX license header consistent with the repository's testdata licensing pattern.
tests/testdata/number-to-string.json (1)
1-19: LGTM!Well-structured test fixture for the number-to-string type conversion scenario. The resource name is descriptive, the JSON structure is valid, and the
pathPartsarray correctly matches the dot-notationpath. This complements the reversestring-to-number.jsonfixture for comprehensive bidirectional testing.tests/testdata/number-to-string.json.license (1)
1-3: LGTM!Standard SPDX license header with correct Apache-2.0 identifier, consistent with the project's licensing.
pkg/config/testdata/invalid/unknown-change-type.json (1)
1-17: LGTM!Correctly structured test fixture for validating the handling of unknown/unsupported change types. The
changeType: "field_modified"is intentionally invalid (valid types arefield_added,field_deleted,type_changed), and the file is appropriately placed in theinvalid/directory.pkg/config/testdata/valid/no-changes.json (1)
1-11: LGTM!Good edge case test fixture for validating that version migrations with no recorded changes are handled gracefully. The empty
changesarray is a valid scenario that should be supported.pkg/config/testdata/valid/field-deletion.json (1)
1-17: LGTM!Well-structured test fixture for the field deletion scenario. The
changeType: "field_deleted"is valid, and thepathPartsarray correctly corresponds to the dot-notationpath. This complements thefield-addition.jsonfixture for comprehensive coverage of field lifecycle changes.pkg/config/testdata/valid/field-addition.json (1)
1-17: LGTM!The test data structure is well-formed with consistent
pathandpathPartsvalues. The change typefield_addedcorrectly describes the scenario being tested.pkg/config/testdata/valid/field-addition.json.license (1)
1-3: LGTM!Standard SPDX license header following project conventions.
pkg/config/provider.go (1)
23-27: LGTM!The path constants are appropriately scoped as package-private and correctly represent the standard Crossplane resource paths for
forProvider,initProvider, andatProvidersections. This provides a centralized location for these commonly-used path strings.pkg/config/testdata/valid/type-change-number-to-string.json (1)
1-19: LGTM!The test data structure is well-formed and correctly captures the type change scenario from
numbertostring. ThepathandpathPartsvalues are consistent, and theoldValue/newValuefields properly document the type transition.pkg/config/testdata/invalid/missing-change-type.json (1)
1-16: LGTM!This is appropriate negative test data for validating error handling when the required
changeTypefield is missing. The file is correctly placed in theinvaliddirectory and will help ensure proper validation of change specifications.pkg/config/testdata/invalid/type-change-bool-schema-mismatch.json (1)
1-20: LGTM!Well-structured test data for validating the schema mismatch scenario. The comment clearly documents why this case is invalid (TF schema expects
TypeBoolbut the change describesstring→number).pkg/config/testdata/invalid/type-change-same-values.json (1)
1-19: LGTM!Good test case for validating that
type_changedentries with identicaloldValueandnewValueare rejected. The semantic invalidity is clear.pkg/config/testdata/valid/type-change-string-to-number.json (1)
1-19: LGTM!Valid test data for the string-to-number type conversion scenario, which is one of the supported primitive type conversions mentioned in the PR objectives.
pkg/config/testdata/invalid/malformed.json (1)
1-19: LGTM!Intentionally malformed JSON test data for exercising error handling during parsing. The static analysis errors (missing comma, invalid comment syntax) are expected and by design.
pkg/config/testdata/invalid/malformed.json.license (1)
1-3: LGTM!Standard SPDX license header consistent with other testdata license files in the repository.
tests/testdata/multiple-changes.json (1)
1-33: LGTM - Well-structured test data for multiple change types.The JSON structure is valid and covers a good combination of change scenarios (field addition and two type changes). The resource name
MultiChangeResourceappropriately distinguishes this from other test fixtures.Note: The AI summary mentions a
field_deletedchange forspec.forProvider.oldField, but this change is not present in the actual file. The file only containsfield_addedand twotype_changedentries.pkg/config/testdata/invalid/type-change-unsupported.json (1)
1-19: LGTM - Appropriate invalid test case for unsupported type conversion.This correctly tests the
array→stringtype change scenario, which is outside the supported primitive conversions (string↔int/float/bool) mentioned in the PR objectives.pkg/config/testdata/invalid/type-change-unsupported.json.license (1)
1-3: LGTM - Standard SPDX license header.pkg/config/testdata/valid/multiple-changes.json (1)
1-29: LGTM - Comprehensive test data covering all change types.This fixture appropriately covers all three change types (
field_added,field_deleted,type_changed) in a single resource version transition, providing good test coverage for the change detection logic.pkg/config/testdata/valid/three-versions.json (1)
1-40: LGTM - Excellent test case for multi-version conversion chains.This fixture tests an important scenario where a field (
newField) is added in one version transition and then deleted in the subsequent transition. This validates that the conversion system correctly handles cumulative changes across multiple API version upgrades.pkg/config/testdata/valid/three-versions.json.license (1)
1-3: LGTM!Standard SPDX license metadata for the accompanying test data file.
pkg/config/testdata/valid/multiple-resources.json (1)
1-80: LGTM!Comprehensive test data covering the key conversion scenarios:
- Field additions (ResourceOne)
- Type changes with
string→number(ResourceTwo) andstring→boolean(ResourceFour)- Field deletions (ResourceThree)
- Multiple changes per resource (ResourceFour)
- Empty changes array edge case (ResourceFive)
- Multiple API groups (
test.example.io,other.example.io)This provides good coverage for testing the auto-conversion registration logic.
pkg/config/resource.go (1)
668-739: LGTM!Well-designed configuration struct with:
- Clear opt-out mechanism via
SkipAutoRegistration- Separation of concerns between identity conversion exclusions (auto-populated) and manual exclusions (
AutoRegisterExcludePaths)- Thorough documentation with usage examples and technical notes
The design appropriately allows resources to opt out completely or exclude specific paths from automatic conversion registration.
pkg/config/conversion.go (4)
7-17: LGTM!Clean import organization with proper use of the
errorspackage from crossplane-runtime per coding guidelines.
192-248: LGTM!Helper functions are well-implemented:
parseChangeReports: Proper JSON unmarshaling with error wrappingsetIdentityConversionExcludePath: Correct deduplication using map, handles empty pathsisExcludedPath: Simple and clear linear search
144-190: LGTM!Well-documented function with clear timing requirements. The deduplication logic using a map prevents duplicate exclusion paths across multiple version transitions.
209-222: The path constants are properly defined inpkg/config/provider.go(lines 24-26) and are accessible to thetrimPathPrefixfunction. No issues found.go.mod (1)
30-30: Testify usage complies with coding guidelines.Testify is imported only in
pkg/config/testing_helpers.go, not in any*_test.gofiles. The coding guideline restricts third-party test frameworks like Testify to test files only. Since testify is used exclusively in a helper/utility file to provide testing utilities, this dependency is compliant and necessary.cmd/schemadiff/main.go (2)
27-99: Main function implementation looks correct.The overall flow is well-structured: enumerate YAML files, create self-diffs for each CRD, aggregate non-empty change reports, and write JSON output. The use of secure file permissions (0600) and proper error handling with
kingpin.FatalIfErroris appropriate for a CLI tool.
101-128: LGTM!The helper function correctly filters for YAML/YML files in a case-insensitive manner and only processes top-level files. Pre-allocating the slice with
make([]string, 0, len(entries))is a good optimization.docs/breaking-change-detection.md (2)
7-98: Documentation is comprehensive and well-structured.The document provides excellent coverage of the breaking change detection system, including clear explanations of build-time analysis, runtime registration phases, and conversion execution flow. The JSON examples effectively illustrate the output format.
1-5: Copyright year appears to be set in the future.The copyright year is 2026, but the current year is 2025 based on other files in this PR. Consider updating for consistency.
Proposed fix
<!-- -SPDX-FileCopyrightText: 2026 The Crossplane Authors <https://crossplane.io> +SPDX-FileCopyrightText: 2025 The Crossplane Authors <https://crossplane.io> SPDX-License-Identifier: CC-BY-4.0 -->⛔ Skipped due to learnings
Learnt from: CR Repo: crossplane/upjet PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-13T21:29:32.867Z Learning: Use Kubernetes-native patterns via crossplane-runtimedocs/README.md (1)
43-43: LGTM!The new documentation entry follows the established format and provides a clear description of the linked content.
pkg/config/testing_helpers.go (1)
18-49: Test helpers are well-designed.The helpers provide good flexibility for creating test providers and resources with various Terraform schema configurations. The use of
t.Helper()ensures proper stack trace reporting in test failures.pkg/config/testdata/valid/deeply-nested.json.license (1)
1-3: LGTM!Standard SPDX license header following the project's licensing conventions.
tests/testdata/bool-to-string.json (1)
1-19: LGTM!Test data correctly describes a boolean-to-string type change scenario. The
pathPartsarray properly matches the dot-separatedpath, and all required fields are present.pkg/config/testdata/valid/type-change-bool-to-string.json (1)
1-19: LGTM!Test data correctly describes a boolean-to-string type change for the
enabledfield. Structure is consistent with other type-change test fixtures in the test suite.pkg/config/testdata/valid/multiple-type-changes.json (1)
1-33: LGTM!Test data provides good coverage of multiple type changes in a single version transition, including both
forProviderandinitProviderpaths with different type conversion directions (string↔number, string↔boolean).pkg/config/testdata/valid/deeply-nested.json (1)
1-34: LGTM!Excellent test coverage for deeply nested path scenarios. The data exercises:
- Deep nesting (5+ levels)
- Array wildcard notation (
[*])- All change types (
field_added,field_deleted,type_changed)- Multiple path roots (
spec.forProvider,spec.initProvider,status.atProvider)The
pathPartsarrays correctly correspond to their dot-separated paths with proper handling of array wildcards as discrete elements.pkg/config/conversion_test.go (9)
1-12: LGTM! Well-structured test file with appropriate imports.The file follows coding guidelines: uses standard Go testing, places tests in the same package as code under test, and uses
google/go-cmpfor comparisons which is an established library in the codebase.
14-480: Comprehensive table-driven test with excellent coverage.The test covers a wide range of scenarios including happy paths (field additions, deletions, type changes), edge cases (nil TerraformResource, empty JSON), and error conditions (malformed JSON, schema mismatches). The test structure follows best practices with clear reason strings for each case. Based on learnings, this follows the strongly preferred table-driven test pattern.
482-554: Good multi-resource test scenario.Tests the registration logic across multiple resources with different configurations (short groups, change types), ensuring isolation and correct conversion counts.
556-741: Well-designed test for identity exclusion logic.Covers type change detection, path exclusion interactions, and deduplication across versions. The use of
cmpopts.SortSlicesis appropriate since path order shouldn't matter.
743-879: Thorough edge case coverage for path exclusion logic.Tests handle nil/empty lists, exact vs partial matching, case sensitivity, and multiple path scenarios.
881-986: Good test for path prefix handling and deduplication.Covers the trimming of
spec.forProvider,spec.initProvider, andstatus.atProviderprefixes, and verifies deduplication behavior.
988-1102: Valuable integration test verifying function interoperability.Tests that
ExcludeTypeChangesFromIdentityandRegisterAutoConversionswork correctly regardless of call order, which is important for provider integration flexibility.
1104-1179: Good validation test ensuring conversion integrity.Verifies that all registered conversions are non-nil, which is an important sanity check to prevent runtime panics.
1181-1249: Complete coverage for path prefix trimming.Tests all expected prefixes and correctly handles non-matching paths returning empty strings.
tests/conversion/integration_test.go (4)
1-33: LGTM! Clean imports and well-documented purpose.The Level 2 integration test documentation clearly explains the distinction from Level 1 tests (registration vs. actual execution).
146-334: Well-structured field addition/deletion tests.Tests cover annotation-based storage and restoration during version conversions, including roundtrip verification. The test assertions are thorough.
340-534: Comprehensive type conversion tests with proper numeric handling.The type switch pattern (lines 388-398, 519-529) correctly handles JSON's tendency to unmarshal numbers as
float64, while also supportingint64andinttypes. This defensive approach ensures test stability.
800-1079: Excellent integration test combining multiple change types.Tests verify that field additions, type conversions (string→int, string→bool), and annotation-based storage all work correctly together in both directions and through roundtrips.
tests/conversion/test_resource.go (2)
1-49: Well-structured test resource implementing Terraformed interface.The struct layout properly embeds Crossplane runtime types and uses map-based parameters for flexible test data. The interface assertion ensures compile-time verification.
51-165: Interface methods correctly implemented for test purposes.The implementations are minimal but sufficient for conversion testing. The
GetTerraformResourceTypefallback to"test_resource"and the configurable override via field is a good design for flexible testing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if out == nil || *out == "" { | ||
| kingpin.Fatalf("output directory file") | ||
| } |
There was a problem hiding this comment.
Fix incomplete error message.
The error message is grammatically incomplete - it should describe what is required.
Proposed fix
if out == nil || *out == "" {
- kingpin.Fatalf("output directory file")
+ kingpin.Fatalf("output file required")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if out == nil || *out == "" { | |
| kingpin.Fatalf("output directory file") | |
| } | |
| if out == nil || *out == "" { | |
| kingpin.Fatalf("output file required") | |
| } |
🤖 Prompt for AI Agents
In `@cmd/schemadiff/main.go` around lines 35 - 37, The error message in the
validation for out (the nil/empty check using kingpin.Fatalf) is incomplete;
update the kingpin.Fatalf call in cmd/schemadiff/main.go (the block that checks
if out == nil || *out == "") to a clear, grammatically complete message such as
"output directory is required" or "must specify output directory" so the error
describes what is required.
| github.com/fatih/color v1.18.0 // indirect | ||
| github.com/fsnotify/fsnotify v1.8.0 // indirect | ||
| github.com/fxamacker/cbor/v2 v2.7.0 // indirect | ||
| github.com/getkin/kin-openapi v0.108.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the directory context and read the full go.mod
pwd
head -70 go.mod | tail -20Repository: crossplane/upjet
Length of output: 1139
🏁 Script executed:
# Find which direct dependency brings in kin-openapi
go mod why github.com/getkin/kin-openapi
# Also check the full go.mod to find all direct dependencies
echo "=== Direct dependencies ==="
grep -v "// indirect" go.mod | grep "github.com" | head -30Repository: crossplane/upjet
Length of output: 9764
🌐 Web query:
GO-2025-3533 GHSA-wq9g-9vfc-cfq9 kin-openapi data amplification vulnerability
💡 Result:
Summary — GO-2025-3533 / GHSA-wq9g-9vfc-cfq9 (CVE-2025-30153)
- Issue: Improper handling of highly-compressed data (data amplification / ZIP-bomb) in github.com/getkin/kin-openapi openapi3filter (ZipFileBodyDecoder). An attacker can upload a crafted ZIP via multipart/form-data and cause the server to exhaust memory (DoS). (osv.dev)
- Affected versions: kin-openapi before v0.131.0 (openapi3filter symbols such as ValidateRequest / ValidateRequestBody / ValidationHandler.ServeHTTP). (developers.golang.nutanix.com)
- Severity: CVSS 3.1 7.5 (High). (osv.dev)
- Fix / mitigation: Upgrade kin-openapi to v0.131.0 or later (patch/PR and commit linked in advisories). As a mitigation, avoid automatically trusting/unrestricted zip file decoding in request validation and apply input size/decompression limits where feasible. (developers.golang.nutanix.com)
If you want, I can fetch the exact advisory and patch commit links or produce a short patch/upgrade checklist for your project.
Citations:
- 1: https://osv.dev/vulnerability/GHSA-wq9g-9vfc-cfq9?utm_source=openai
- 2: https://developers.golang.nutanix.com/vuln/GO-2025-3533?utm_source=openai
- 3: https://osv.dev/vulnerability/CVE-2025-30153?utm_source=openai
- 4: https://developers.golang.nutanix.com/vuln/GO-2025-3533?utm_source=openai
🏁 Script executed:
# Find which direct dependency brings in kin-openapi
rg "getkin/kin-openapi" --type go
rg "openapi3filter" --type go
# Check go.mod details to see if we can identify the chain
cat go.mod | grep -A 100 "require ("Repository: crossplane/upjet
Length of output: 6273
🌐 Web query:
kin-openapi v0.131.0 release date fix version GO-2025-3533
💡 Result:
kin-openapi v0.131.0 was released on March 19, 2025. It fixes vulnerability GO-2025-3533 (alias CVE-2025-30153 / GHSA-wq9g-9vfc-cfq9); the vuln is fixed in v0.131.0. [1][2]
Sources:
- Release: getkin/kin-openapi v0.131.0 (GitHub). [1]
- Vulnerability: GO-2025-3533 / CVE-2025-30153 (pkg.go.dev / OSV). [2]
Correct HIGH severity vulnerability fix version for indirect dependency.
The vulnerability GO-2025-3533 (GHSA-wq9g-9vfc-cfq9) in github.com/getkin/kin-openapi v0.108.0 is confirmed: a data amplification vulnerability in openapi3filter that allows denial-of-service attacks. However, the fix version is v0.131.0 or later (not v0.121.0+). Identify and update the direct dependency that pulls in kin-openapi to a version using the patched kin-openapi.
🧰 Tools
🪛 OSV Scanner (2.3.1)
[HIGH] 64-64: github.com/getkin/kin-openapi 0.108.0: Improper Handling of Highly Compressed Data (Data Amplification) in github.com/getkin/kin-openapi/openapi3filter
(GO-2025-3533)
[HIGH] 64-64: github.com/getkin/kin-openapi 0.108.0: Improper Handling of Highly Compressed Data (Data Amplification) in github.com/getkin/kin-openapi/openapi3filter
🤖 Prompt for AI Agents
In `@go.mod` at line 64, The go.mod currently pulls in
github.com/getkin/kin-openapi v0.108.0 (indirect) which is vulnerable; identify
which direct module in your dependency graph depends on
github.com/getkin/kin-openapi (use go mod graph or go list -m all to locate the
parent module), then bump that direct dependency to a release that itself
requires kin-openapi v0.131.0 or later (use go get
<direct-module>@<fixed-version>), run go mod tidy to update go.mod/go.sum, and
verify that github.com/getkin/kin-openapi is now recorded at v0.131.0+ as an
indirect dependency.
| var data1, data2, data3, data4, data5, data6 map[string]interface{} | ||
| json.Unmarshal(fixture1, &data1) | ||
| json.Unmarshal(fixture2, &data2) | ||
| json.Unmarshal(fixture3, &data3) | ||
| json.Unmarshal(fixture4, &data4) | ||
| json.Unmarshal(fixture5, &data5) | ||
| json.Unmarshal(fixture6, &data6) | ||
|
|
||
| merged := make(map[string]interface{}) | ||
| for k, v := range data1 { | ||
| merged[k] = v | ||
| } | ||
| for k, v := range data2 { | ||
| merged[k] = v | ||
| } | ||
| for k, v := range data3 { | ||
| merged[k] = v | ||
| } | ||
| for k, v := range data4 { | ||
| merged[k] = v | ||
| } | ||
| for k, v := range data5 { | ||
| merged[k] = v | ||
| } | ||
| for k, v := range data6 { | ||
| merged[k] = v | ||
| } | ||
|
|
||
| mergedJSON, err := json.Marshal(merged) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Unchecked json.Unmarshal errors could mask test failures.
The return values from json.Unmarshal calls (lines 96-101) are discarded. If any fixture contains invalid JSON, the test would silently proceed with empty/partial data instead of failing explicitly.
🔧 Proposed fix
var data1, data2, data3, data4, data5, data6 map[string]interface{}
- json.Unmarshal(fixture1, &data1)
- json.Unmarshal(fixture2, &data2)
- json.Unmarshal(fixture3, &data3)
- json.Unmarshal(fixture4, &data4)
- json.Unmarshal(fixture5, &data5)
- json.Unmarshal(fixture6, &data6)
+ if err := json.Unmarshal(fixture1, &data1); err != nil {
+ t.Fatalf("Failed to unmarshal fixture1: %v", err)
+ }
+ if err := json.Unmarshal(fixture2, &data2); err != nil {
+ t.Fatalf("Failed to unmarshal fixture2: %v", err)
+ }
+ if err := json.Unmarshal(fixture3, &data3); err != nil {
+ t.Fatalf("Failed to unmarshal fixture3: %v", err)
+ }
+ if err := json.Unmarshal(fixture4, &data4); err != nil {
+ t.Fatalf("Failed to unmarshal fixture4: %v", err)
+ }
+ if err := json.Unmarshal(fixture5, &data5); err != nil {
+ t.Fatalf("Failed to unmarshal fixture5: %v", err)
+ }
+ if err := json.Unmarshal(fixture6, &data6); err != nil {
+ t.Fatalf("Failed to unmarshal fixture6: %v", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var data1, data2, data3, data4, data5, data6 map[string]interface{} | |
| json.Unmarshal(fixture1, &data1) | |
| json.Unmarshal(fixture2, &data2) | |
| json.Unmarshal(fixture3, &data3) | |
| json.Unmarshal(fixture4, &data4) | |
| json.Unmarshal(fixture5, &data5) | |
| json.Unmarshal(fixture6, &data6) | |
| merged := make(map[string]interface{}) | |
| for k, v := range data1 { | |
| merged[k] = v | |
| } | |
| for k, v := range data2 { | |
| merged[k] = v | |
| } | |
| for k, v := range data3 { | |
| merged[k] = v | |
| } | |
| for k, v := range data4 { | |
| merged[k] = v | |
| } | |
| for k, v := range data5 { | |
| merged[k] = v | |
| } | |
| for k, v := range data6 { | |
| merged[k] = v | |
| } | |
| mergedJSON, err := json.Marshal(merged) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| var data1, data2, data3, data4, data5, data6 map[string]interface{} | |
| if err := json.Unmarshal(fixture1, &data1); err != nil { | |
| t.Fatalf("Failed to unmarshal fixture1: %v", err) | |
| } | |
| if err := json.Unmarshal(fixture2, &data2); err != nil { | |
| t.Fatalf("Failed to unmarshal fixture2: %v", err) | |
| } | |
| if err := json.Unmarshal(fixture3, &data3); err != nil { | |
| t.Fatalf("Failed to unmarshal fixture3: %v", err) | |
| } | |
| if err := json.Unmarshal(fixture4, &data4); err != nil { | |
| t.Fatalf("Failed to unmarshal fixture4: %v", err) | |
| } | |
| if err := json.Unmarshal(fixture5, &data5); err != nil { | |
| t.Fatalf("Failed to unmarshal fixture5: %v", err) | |
| } | |
| if err := json.Unmarshal(fixture6, &data6); err != nil { | |
| t.Fatalf("Failed to unmarshal fixture6: %v", err) | |
| } | |
| merged := make(map[string]interface{}) | |
| for k, v := range data1 { | |
| merged[k] = v | |
| } | |
| for k, v := range data2 { | |
| merged[k] = v | |
| } | |
| for k, v := range data3 { | |
| merged[k] = v | |
| } | |
| for k, v := range data4 { | |
| merged[k] = v | |
| } | |
| for k, v := range data5 { | |
| merged[k] = v | |
| } | |
| for k, v := range data6 { | |
| merged[k] = v | |
| } | |
| mergedJSON, err := json.Marshal(merged) | |
| if err != nil { | |
| t.Fatal(err) | |
| } |
🤖 Prompt for AI Agents
In `@tests/conversion/integration_test.go` around lines 95 - 126, The
json.Unmarshal calls into data1..data6 ignore returned errors, so parse failures
get swallowed; update each json.Unmarshal(fixtureX, &dataX) to capture its error
and fail the test on error (e.g., if err := json.Unmarshal(...); err != nil {
t.Fatalf("unmarshal fixtureX: %v", err) }) so invalid fixtures cause the test to
fail before merging into merged and marshalling mergedJSON.
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
…onversion logics Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
erhancagirici
force-pushed
the
integrate-breaking-change-detection
branch
from
d3e6d6a to
538dd34
Compare
1 participant
Description of your changes
Overview
This PR introduces an automated system for detecting and handling schema changes in CRD API versions across upjet-based providers. When Terraform provider schemas evolve (field additions, deletions, or type changes), CRD API versions must adapt, creating potential data loss during upgrades/downgrades. This system automatically generates and registers conversion functions to preserve data integrity across API version transitions.
Problem Statement
When a Terraform provider introduces schema changes (e.g.,
stringfield becomesnumber, or a field is added/removed), upjet-based Crossplane providers face several challenges:Previously, handling these scenarios required:
This manual process is error-prone, time-consuming, and doesn't scale across 100+ resources in a provider.
Solution Architecture
Controller Version Architecture
Important Context: This system includes two types of conversion logic:
CRD Conversion Webhook Logic (Permanent)
RegisterAutoConversions()Runtime Annotation Handling (Temporary - Being Deprecated)
mergeAnnotationFieldsWithSpec(),moveTFStateValuesToAnnotation(),mergeAnnotationFieldsWithStatus()ControllerReconcileVersionconfiguration is deprecated and will be removed in future upjet releasesMigration Strategy: As part of adopting this system, providers update controller reconcile versions to match the latest CRD API version (see provider-upjet-azuread example). This eliminates the need for runtime annotation handling going forward.
High-Level Flow
Components
1. schemadiff Tool (
cmd/schemadiff/main.go)upbound/uptest/pkg/crdschemalibrary (now official-providers-ci){group}/{kind}→ list of changes per version transitiongo generateafter CRD generationExample Output:
{ "ec2.aws.upbound.io/VPC": { "versions": { "v1beta2": { "oldVersion": "v1beta1", "newVersion": "v1beta2", "changes": [ { "path": "spec.forProvider.maxSize", "changeType": "type_changed", "oldValue": "string", "newValue": "number" }, { "path": "spec.forProvider.newField", "changeType": "field_added" } ] } } } }2. Auto-Registration Functions (
pkg/config/conversion.go)ExcludeTypeChangesFromIdentity()type_changedentriesr.AutoConversionRegistrationOptions.IdentityConversionExcludePathsbumpVersionsWithEmbeddedLists()(or similar version bumping)RegisterAutoConversions()internal.upjet.crossplane.io/field-conversionsannotationgenerationProviderflag (no need for conversions during code gen)3. Conversion Implementations (
pkg/config/conversion/conversions.go)newlyIntroducedFieldConverterfieldTypeConverterIntToString/StringToInt: int64 ↔ stringFloatToString/StringToFloat: float64 ↔ stringBoolToString/StringToBool: bool ↔ "true"/"false"strconvpackage for conversion4. Runtime Annotation Handling (Temporary - Being Deprecated) (
pkg/controller/annotation_conversions.go)Purpose: These functions handle version mismatches between
ControllerReconcileVersionand the API version. With this PR, providers update controller versions to match API versions, making these functions inactive.mergeAnnotationFieldsWithSpec()getExtendedParameters())internal.upjet.crossplane.io/field-conversionsannotationspec.forProvider.*andspec.initProvider.*values back into parameter mapsControllerReconcileVersion == Versionand returns early if equal (most common case)moveTFStateValuesToAnnotation()r.TfStatusConversionPaths(only needed during version mismatch)ControllerReconcileVersion == Versionand returns early if equalmergeAnnotationFieldsWithStatus()status.atProvider.*values back into status mapControllerReconcileVersion == Versionand returns early if equalDeprecation Timeline:
ControllerReconcileVersionfield marked as deprecatedDesign Decision: Why schemadiff Runs in Provider Generation Pipeline
Decision: The schemadiff tool is invoked in the provider's generate/generate.go file, not within upjet's code generation logic.
Architectural Constraint:
Upjet's architecture has a clear separation of concerns:
This creates a dependency chain that prevents integration into upjet:
Upjet generates Go types
↓
Provider runs controller-gen (generates CRDs from Go types)
↓
Provider runs schemadiff (analyzes generated CRDs)
↓
Provider embeds JSON (makes analysis available at runtime)
Why schemadiff can't run earlier:
Trade-off accepted: Providers must add two lines to their generate/generate.go file (go:generate directive + embed statement). This manual step is necessary due to the architectural boundary between upjet and CRD generation tooling.
The current design respects existing architectural boundaries while providing maximum flexibility to providers.
Key Changes
New Files
cmd/schemadiff/main.gopkg/config/conversion.gopkg/controller/annotation_conversions.gotests/conversion/integration_test.gopkg/config/conversion_test.goModified Files
pkg/config/conversion/conversions.gopkg/controller/external_tfpluginsdk.gopkg/controller/external_tfpluginfw.gopkg/config/resource.goConfiguration API Additions
Resource.AutoConversionRegistrationOptionsResource.TfStatusConversionPathsDesign Decisions & Trade-offs
1. Number→String Defaults to Integer Conversion
Decision: When auto-detecting
number→stringtype changes, default to int64 conversion.Rationale:
AutoRegisterExcludePathsfor manual registrationLimitation: Float fields (e.g.,
3.14159) will be truncated to"3"if not manually handled.Mitigation: Will be documented prominently in README and inline comments. Developers can exclude and manually register float conversions.
2. Function Call Ordering is Manual
Decision: Keep
ExcludeTypeChangesFromIdentity()andRegisterAutoConversions()as separate functions.Required Order:
Rationale:
bumpVersionsWithEmbeddedLists()is provider-specific logic (not in upjet)Limitation: Order is not enforced by type system.
Mitigation:
3. TfStatusConversionPaths Requires Manual Configuration (Temporary Feature)
Decision: Auto-detect and handle
spec.*fields automatically, but require manual configuration forstatus.atProvider.*fields.Rationale:
ControllerReconcileVersion ≠ Version(controller on older API version)ControllerReconcileVersionis deprecated and will be removed in future upjet releasesControllerReconcileVersionCurrent Status:
Legacy Usage (if needed during migration):
Deprecation Path:
ControllerReconcileVersionfield marked deprecatedControllerReconcileVersionandTfStatusConversionPathsProvider Integration Guide
Step 1: Update Controller Versions to Match API Versions
Suggested First Step: Ensure all resources reconcile on their latest API version. This will decrease runtime complexity.
In your provider-specific version bumping logic (e.g.,
bumpVersionsWithEmbeddedLists()):Why this matters:
Step 2: Add go:generate Directive
In
generate/generate.go, after CRD generation:Step 3: Embed JSON in Provider Config
In
config/provider.go:Step 4: Integrate During Provider Construction
In
GetProvider():Step 5: (Optional) Exclude Specific Paths for Custom Conversions
Only needed for float fields or custom conversion logic:
Step 6: (Legacy) Configure Status Field PreservationNot needed for new integrations. This step only applies if maintaining backwards compatibility with
ControllerReconcileVersion ≠ Version.Click to expand legacy configuration (deprecated)
Supported Conversion Types
1. Field Additions
Scenario: Field exists in v1beta2 but not in v1beta1
Behavior:
Annotation Key:
internal.upjet.crossplane.io/field-conversions2. Field Deletions
Scenario: Field exists in v1beta1 but not in v1beta2
Behavior: Same as field additions (symmetric)
3. Type Changes: String ↔ Number
Scenario: Field changes from string to number (or vice versa)
String → Number:
Number → String:
AutoRegisterExcludePathsand register manual FloatToString converter4. Type Changes: String ↔ Boolean
Scenario: Field changes from string to boolean (or vice versa)
Behavior:
strconv.ParseBoolTesting
Unit Tests (
pkg/config/conversion_test.go)Test Coverage:
Example Test Cases:
TestRegisterAutoConversions_ValidJSON_MultipleResourcesTestRegisterAutoConversions_TypeChange_MissingTerraformSchemaTestExcludeTypeChangesFromIdentity_ValidJSONTestRegisterAutoConversions_InvalidJSON_MalformedIntegration Tests (
tests/conversion/integration_test.go)Test Coverage:
Test Resources:
tests/testdata/Example Test Cases:
TestConversionIntegration_StringToIntTestConversionIntegration_BoolToStringTestConversionIntegration_FieldAdditionTestConversionIntegration_MultipleChangesKnown Limitations & Considerations
1. Float Precision in Number→String Conversions
Impact: Float fields converting to string will lose precision if not manually configured.
Workaround: Use
AutoRegisterExcludePathsand register manual conversion:2. Function Call Ordering
Impact: Incorrect order causes runtime conversion failures.
Workaround: Follow documented order strictly. Integration tests will catch violations.
3. Annotation Size Limits
Impact: Kubernetes annotations limited to 256KB total per resource.
Unlikely Scenario: Would require many large fields to be added/removed simultaneously.
Monitoring: System doesn't enforce limit currently (future enhancement).
4. Status Field Conversions Require Manual Config (Deprecated Feature)
Impact: Status fields not automatically preserved during version transitions when
ControllerReconcileVersion ≠ Version.Current Status:
ControllerReconcileVersionis removed in future upjet releaseLegacy Workaround (if needed): Configure
TfStatusConversionPathsexplicitly.Recommended: Update controllers to latest API version (Integration Guide Step 1) to avoid this entirely.
Note: Depends on upbound/official-providers-ci#245
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Tested in the provider-upjet-azuread: https://github.com/sergenyalcin/provider-upjet-azuread/tree/conversion-functions-demo. This branch consumes these changes and creates a toy problem for
applications.Applicationresource for testing. This contains field_addition and field_deletion scenario.