feat: Support custom type recursive reference #714

AlessioDiama · 2025-10-08T22:20:13Z

With this PR we add support for recursive references in custom type definitions, i.e. a custom type can reference another custom type. Cyclic references are detected and not allowed.
Tests are extended to cover the new functionality and an example is provided

linux-foundation-easycla · 2025-10-08T22:20:17Z

The committers listed above are authorized under a signed CLA.

✅ login: AlessioDiama / name: Alessio Diamanti (7361d8e, 7b156e1, 8a15e8e, 8ee77f2, ba95c11, bf5f540, f7b2bc8)

k8s-ci-robot · 2025-10-08T22:20:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AlessioDiama
Once this PR has been reviewed and has the lgtm label, please assign elmiko for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-10-08T22:20:22Z

Welcome @AlessioDiama!

It looks like this is your first PR to kubernetes-sigs/kro 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kro has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-10-08T22:20:23Z

Hi @AlessioDiama. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

jakobmoellerdev

Thanks for the PR, this is a super cool feature 🎉 . Im certain we can make this work for KRO but I do believe it needs a bit of polish before a confident merge. PTAL at my comments and let me know what you think!

jakobmoellerdev · 2025-10-09T06:58:27Z

examples/kubernetes/recursive-custom-types/instance.yaml

@@ -0,0 +1,51 @@
+---


I believe this warrants an E2E / Chainsaw test

jakobmoellerdev · 2025-10-09T07:01:13Z

pkg/simpleschema/transform.go

+		// Add dependencies to the DAG and check for cycles
+		err := dagInstance.AddDependencies(k, dependencies)
+		if err != nil {
+			if cycleErr, isCycle := err.(*dag.CycleError[string]); isCycle {


couldnt we change this to errors.Is

jakobmoellerdev · 2025-10-09T07:01:34Z

pkg/simpleschema/transform.go

+	orderedVertexes, err := dagInstance.TopologicalSort()
 	if err != nil {
-		return fmt.Errorf("failed to build pre-defined types schema: %w", err)
+		return fmt.Errorf("failed to sort DAG: %w", err)


it will be unaparrent to users which DAG youre talking about here

not sure what output as a user-understandable error here. Something like:

return fmt.Errorf("failed to get topological order for the types: %w", err)

WDYT?

jakobmoellerdev · 2025-10-09T07:02:10Z

pkg/simpleschema/transform.go

+	for _, vertex := range orderedVertexes {
+		objValueAtKey, ok := obj[vertex]
+		if !ok {
+			return fmt.Errorf("failed to assert type for vertex %s", vertex)


this is not the correct message and also it can never happen. you never look for a key that you didnt add, so this is a noop.

thanks, removed.

jakobmoellerdev · 2025-10-09T07:03:02Z

pkg/simpleschema/transform.go

+		objMap := map[string]interface{}{
+			vertex: objValueAtKey,
+		}
+		if schemaProps, err := t.buildOpenAPISchema(objMap); err == nil {


why dont you simply do a normal early return? no need for an if / else check because on error you return from the function. I think this was the behavior befor as well

thanks for the comment. Implemented as suggested

jakobmoellerdev · 2025-10-09T07:05:14Z

pkg/simpleschema/transform.go

+	// Remove duplicates using a set
+	dependencySet := make(map[string]struct{})
+	for _, dep := range dependencies {
+		dependencySet[dep] = struct{}{}


why did you do this after going through parseMap? wouldnt it be perfectly fine to collect that from the start instead of starting with an array?

As we already have a dependency on k8s apimachinery it could be interesting to re-use the existing Set[comparable] that could help code readability

depedencies := sets.Set[string]{} [... in the loop] dependencies.Insert(depName) [...] return dependencies.UnsortedList()

thanks for the hint! Changed to use sets.Set[string]{} as suggested

jakobmoellerdev · 2025-10-09T07:07:46Z

pkg/simpleschema/transform.go

+	parseMap = func(m map[string]interface{}) {
+		for _, value := range m {
+			switch v := value.(type) {
+			case string:


Because you now have semantic meaning attached to types, I believe the types that are allowed need much stricter naming restrictions to avoid issues in the future. Things I am thinking about:

Disallow Special Characters (for example "|" would be a problem, or starting with "[" and "]")

Optionally enforce UpperCamelCase

Maybe some other things that im not aware of that could actually break dependency extraction What happens with "[][]Type" for example

In general, maybe think what can break the parser you built here and try to make it a bit more robust. I think those restrictions should be added onto the CRD itself as part of input validation, and not here in the parser.

We could reuse some of the functions already defined in https://github.com/kubernetes-sigs/kro/blob/main/pkg/simpleschema/atomic.go#L49-L138

i've pushed a new implementation for the extractDependenciesFromMap func. Now i am using the functions from https://github.com/kubernetes-sigs/kro/blob/main/pkg/simpleschema/atomic.go#L49-L138 to make sure to type parsing is more effective.

I believe it is still not 100% safe regarding to all the cases described by @jakobmoellerdev. I would need as well some indications on how to enforce restrictions "onto the CRD itself as part of input validation" as suggested above

jakobmoellerdev · 2025-10-09T07:12:50Z

/ok-to-test

barney-s · 2025-10-09T20:34:52Z

I think its time we split off the simple schema into its own top level package with tests. In KRO we should just include new versions of the simple schema.

We should be able to be faster evolving simple-schema if we decouple KRO usage from the schema.

WDYT @jakobmoellerdev @a-hilaly @matthchr

jakobmoellerdev · 2025-10-10T07:14:44Z

I understand that thought but would like separate that out into its own issue. Dont think we have to discuss it here on this PR, as this should be tackled separately.

fabianburth · 2025-10-11T14:19:43Z

cmd/controller/__debug_bin4030145734

Accidental check in, I assume?

fabianburth · 2025-10-11T14:40:42Z

pkg/simpleschema/transform.go

+	//Constructs a dag of the dependencies between the types
+	//If there is a cycle in the graph, then there is a cyclic dependency between the types
+	//and we cannot load the types


nit:

Suggested change

//Constructs a dag of the dependencies between the types

//If there is a cycle in the graph, then there is a cyclic dependency between the types

//and we cannot load the types

// Constructs a dag of the dependencies between the types

// If there is a cycle in the graph, then there is a cyclic dependency between the types

// and we cannot load the types

fabianburth · 2025-10-11T14:55:23Z

pkg/simpleschema/transform.go

+	var parseMap func(map[string]interface{})
+	parseMap = func(m map[string]interface{}) {


This anonymous function calling itself recursively, with the dependencies closure, is kind of wild! :)
I think we should make this a named unexported function.

fabianburth · 2025-10-11T15:01:36Z

pkg/simpleschema/transform.go

+	result := make([]string, 0, len(dependencySet))
+	for dep := range dependencySet {
+		result = append(result, dep)
+	}


nit:

Suggested change

result := make([]string, 0, len(dependencySet))

for dep := range dependencySet {

result = append(result, dep)

}

result := slices.Collect(maps.Keys(dependencySet))

Could also use standard lib functionality here.

a-hilaly · 2025-10-13T06:47:35Z

pkg/simpleschema/transform.go

+// Define the set of basic types as
+// defined in https://kro.run/docs/concepts/simple-schema#basic-types
+var excludedTypes = map[string]struct{}{
+	"string":  {},
+	"integer": {},
+	"bool":    {},
+	"float":   {},
+	"object":  {},
+}


I like this idea, let's address this in a seperate PR?

tjamet · 2025-10-17T16:34:00Z

examples/kubernetes/recursive-custom-types/rg.yaml

+  schema:
+    types:
+      Person:
+        name: string
+        role: "Role"
+      Division:
+        name: string
+        employees: "[]Person"
+      Role:
+        name: string
+        seniority: string


Will types get inside the CompanyConfig CRD? or is it a construct that stays private within the RGD?

From birds view, it would be interesting to keep it as a construct and exclude it from the CompanyConfig CRD. I would hence advocate for moving it under spec

Suggested change

schema:

types:

Person:

name: string

role: "Role"

Division:

name: string

employees: "[]Person"

Role:

name: string

seniority: string

types:

Person:

name: string

role: "Role"

Division:

name: string

employees: "[]Person"

Role:

name: string

seniority: string

schema:

a-hilaly · 2025-10-28T07:20:05Z

ping @AlessioDiama - are still iterating on this?

AlessioDiama · 2025-10-28T12:38:33Z

hi @a-hilaly, i am a little bit busy, will try to continue as soon as possible.

a-hilaly · 2025-11-05T08:09:30Z

/check-cla
/verify-cla

a-hilaly · 2025-11-05T08:15:53Z

@AlessioDiama can you please sign the CLA?

AlessioDiama · 2025-11-07T14:30:37Z

@AlessioDiama can you please sign the CLA?

it is now done.

jakobmoellerdev

For me this needs 2 more things before we can lgtm: I think it would be best if you can include a reference documentation on usage in the website.
I also still think an integration and/or e2e test is useful.
Once we have this, I think this is good for merge!

k8s-ci-robot · 2025-11-14T00:25:55Z

@AlessioDiama: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
presubmits-build-image	`8ee77f2`	link	true	`/test presubmits-build-image`
presubmits-integration-tests	`8ee77f2`	link	true	`/test presubmits-integration-tests`
presubmits-e2e-tests	`8ee77f2`	link	true	`/test presubmits-e2e-tests`
presubmits-verify-lint	`8ee77f2`	link	true	`/test presubmits-verify-lint`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

a-hilaly · 2025-11-18T08:33:39Z

/milestone 0.7

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 8, 2025

k8s-ci-robot requested a review from barney-s October 8, 2025 22:20

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 8, 2025

k8s-ci-robot requested a review from jakobmoellerdev October 8, 2025 22:20

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 8, 2025

AlessioDiama force-pushed the main branch from f40a98c to efa7321 Compare October 8, 2025 22:22

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 8, 2025

jakobmoellerdev reviewed Oct 9, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 9, 2025

fabianburth reviewed Oct 11, 2025

View reviewed changes

a-hilaly reviewed Oct 13, 2025

View reviewed changes

tjamet reviewed Oct 17, 2025

View reviewed changes

feat: support custom type recursive reference

7361d8e

AlessioDiama added 3 commits November 7, 2025 14:53

implement suggestions

f7b2bc8

implement other suggestions

8a15e8e

extractDependenciesFromMap new implementation

bf5f540

AlessioDiama force-pushed the main branch from 3565992 to bf5f540 Compare November 7, 2025 14:00

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 7, 2025

AlessioDiama added 3 commits November 7, 2025 15:11

fix .gitignore

7b156e1

fix .gitignore

ba95c11

Merge branch 'main' into main

8ee77f2

jakobmoellerdev reviewed Nov 8, 2025

View reviewed changes

		var parseMap func(map[string]interface{})
		parseMap = func(m map[string]interface{}) {

feat: Support custom type recursive reference #714

Are you sure you want to change the base?

feat: Support custom type recursive reference #714

Conversation

AlessioDiama commented Oct 8, 2025

Uh oh!

linux-foundation-easycla bot commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 8, 2025

Uh oh!

k8s-ci-robot commented Oct 8, 2025

Uh oh!

k8s-ci-robot commented Oct 8, 2025

Uh oh!

jakobmoellerdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobmoellerdev commented Oct 9, 2025

Uh oh!

barney-s commented Oct 9, 2025

Uh oh!

jakobmoellerdev commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Oct 28, 2025

Uh oh!

AlessioDiama commented Oct 28, 2025

Uh oh!

a-hilaly commented Nov 5, 2025

Uh oh!

a-hilaly commented Nov 5, 2025

Uh oh!

AlessioDiama commented Nov 7, 2025

Uh oh!

jakobmoellerdev left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 14, 2025

Uh oh!

a-hilaly commented Nov 18, 2025

linux-foundation-easycla bot commented Oct 8, 2025 •

edited

Loading