fix(crd-generator): Fail if multiple versions are marked as stored #5846

baloo42 · 2024-03-30T15:20:40Z

Description

Fixes a bug where the CRDGenerator allows generating CRDs with multiple versions marked as stored.

Additional changes:

Extract some assertions to a seperate class (reusing is planned in the next PRs)
Allow debugging decorator sequence by changing the log level to trace
(refer to simplelogger.properties during test runs)

Fixes #5845

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

…orAssertions utility class

…uence

andreaTP

Added a couple of notes, but they can be tackled in follow up PRs IMHO.

Would be great to have a quick review from @metacosm here, or maybe @csviri is interested too ...

crd-generator/api/src/test/java/io/fabric8/crd/generator/CRDGeneratorTest.java

andreaTP · 2024-04-01T09:31:53Z

...src/main/java/io/fabric8/crd/generator/v1/decorator/EnsureSingleStorageVersionDecorator.java

-    if (spec.hasVersions() && !spec.hasMatchingVersion(hasStorageVersion)) {
-      spec.editFirstVersion().withStorage(true).endVersion();
-    }
+    List<String> storageVersions = versions.stream()


I recall that, at some point, someone unrolled the streams into plain loops because of this feature and I believe that we are not testing it quite enough.

As ugly as it may sound 😞 , have you tried those changes with that flag enabled?

Yes, that's why I added ParallelCRDGeneratorExamplesTest. It should work for parallel generation, too.

You are also right it's not the nicest way, but it's also hard to improve the situation here.
The core problem is not that we are working e.g. with streams or a predicate or a atomic reference. (That's just optimization of the error output).

The real problem is that the decorator sort algorithm is not working as expected on "multi level after/before" dependencies.

That's why I changed...

@Override public Class<? extends Decorator>[] after() { return new Class[] { AddCustomResourceDefinitionResourceDecorator.class, AddCustomResourceDefinitionVersionDecorator.class, SetStorageVersionDecorator.class }; }

Toggle me to see Decorator order...

[main] TRACE io.fabric8.crd.generator.v1.decorator.AddCustomResourceDefinitionResourceDecorator [apiGroup=sample.fabric8.io, kind=Multiple, name=multiples.sample.fabric8.io, plural=multiples, scope=Cluster] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddCustomResourceDefinitionResourceDecorator [apiGroup=sample.fabric8.io, kind=Multiple, name=multiples.sample.fabric8.io, plural=multiples, scope=Cluster] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io, version:v1] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io, version:v2] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddSchemaToCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io, version:v1] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddSchemaToCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io, version:v2] [main] TRACE io.fabric8.crd.generator.v1.decorator.EnsureSingleStorageVersionDecorator [name:multiples.sample.fabric8.io] [main] TRACE io.fabric8.crd.generator.v1.decorator.EnsureSingleStorageVersionDecorator [name:multiples.sample.fabric8.io] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetDeprecatedVersionDecorator [name:multiples.sample.fabric8.io, version:v1, deprecated:false, deprecationWarning:null] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetDeprecatedVersionDecorator [name:multiples.sample.fabric8.io, version:v2, deprecated:false, deprecationWarning:null] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetServedVersionDecorator [name:multiples.sample.fabric8.io, version:v1, served:true] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetServedVersionDecorator [name:multiples.sample.fabric8.io, version:v2, served:true] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetStorageVersionDecorator [name:multiples.sample.fabric8.io, version:v1, storage:false] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetStorageVersionDecorator [name:multiples.sample.fabric8.io, version:v2, storage:true] [main] TRACE io.fabric8.crd.generator.v1.decorator.SortCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io] [main] TRACE io.fabric8.crd.generator.v1.decorator.SortCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io] [main] TRACE io.fabric8.crd.generator.v1.decorator.SortPrinterColumnsDecorator [name:multiples.sample.fabric8.io, version:v2] [main] TRACE io.fabric8.crd.generator.v1.decorator.SortPrinterColumnsDecorator [name:multiples.sample.fabric8.io, version:v1]

to

@Override public Class<? extends Decorator>[] after() { return new Class[] { CustomResourceDefinitionVersionDecorator.class }; }

Toggle me to see Decorator order...

[main] TRACE io.fabric8.crd.generator.v1.decorator.AddCustomResourceDefinitionResourceDecorator [apiGroup=sample.fabric8.io, kind=Multiple, name=multiples.sample.fabric8.io, plural=multiples, scope=Cluster] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddCustomResourceDefinitionResourceDecorator [apiGroup=sample.fabric8.io, kind=Multiple, name=multiples.sample.fabric8.io, plural=multiples, scope=Cluster] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io, version:v1] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io, version:v2] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddSchemaToCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io, version:v1] [main] TRACE io.fabric8.crd.generator.v1.decorator.AddSchemaToCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io, version:v2] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetDeprecatedVersionDecorator [name:multiples.sample.fabric8.io, version:v1, deprecated:false, deprecationWarning:null] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetDeprecatedVersionDecorator [name:multiples.sample.fabric8.io, version:v2, deprecated:false, deprecationWarning:null] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetServedVersionDecorator [name:multiples.sample.fabric8.io, version:v1, served:true] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetServedVersionDecorator [name:multiples.sample.fabric8.io, version:v2, served:true] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetStorageVersionDecorator [name:multiples.sample.fabric8.io, version:v1, storage:false] [main] TRACE io.fabric8.crd.generator.v1.decorator.SetStorageVersionDecorator [name:multiples.sample.fabric8.io, version:v2, storage:true] [main] TRACE io.fabric8.crd.generator.v1.decorator.SortPrinterColumnsDecorator [name:multiples.sample.fabric8.io, version:v2] [main] TRACE io.fabric8.crd.generator.v1.decorator.SortPrinterColumnsDecorator [name:multiples.sample.fabric8.io, version:v1] [main] TRACE io.fabric8.crd.generator.v1.decorator.EnsureSingleStorageVersionDecorator [name:multiples.sample.fabric8.io] [main] TRACE io.fabric8.crd.generator.v1.decorator.EnsureSingleStorageVersionDecorator [name:multiples.sample.fabric8.io] [main] TRACE io.fabric8.crd.generator.v1.decorator.SortCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io] [main] TRACE io.fabric8.crd.generator.v1.decorator.SortCustomResourceDefinitionVersionDecorator [name:multiples.sample.fabric8.io]

The EnsureSingleStorageVersionDecorator extends CustomResourceDefinitionDecorator and because of that it is working on the CustomResourceDefinition "level". Meanwhile the CustomResourceDefinitionVersionDecorator and SetStorageVersionDecorator extend CustomResourceDefinitionVersionDecorator, which means that they are working on the CustomResourceDefinitionVersion "level".

So far as I understand the sorting algorithm of the decorators mixing of multiple levels is not supported.

Maybe we should improve this sorting algorithm (or document the behaviour)?
Or am I wrong?

Yes, that's why I added ParallelCRDGeneratorExamplesTest. It should work for parallel generation, too.

That's nice, but I'm afraid that the execution of the annotation processors in the tests is not the same as when triggered using the "real" annotation processor.
Probably, the only way to fully test is to spin a new module like this:
https://github.com/fabric8io/kubernetes-client/tree/main/crd-generator/test
but enabling parallelism.

Thanks a lot for the rest of the analysis, I agree with this:

Maybe we should improve this sorting algorithm (or document the behaviour)?

I see that someone had already been through this trouble:

kubernetes-client/crd-generator/api/src/main/java/io/fabric8/crd/generator/Resources.java

Lines 84 to 90 in 0c80a66

// We can't guarantee that `when `decorator a < b and b < c then a < c``.

// Why?

// Because our comparators express constraints on particular pairs and can't express the global order.

// So, in order to be accurate we need to compare each decorator, with ALL OTHER decorators.

// In other words we don't ANY sorting algorithm, we need bubble sort.

// We also might need it more than once. So, we'll do it as many times as we have to, till there are not more transformations.

// But hey, let's have an upper limit of 5 just to prevent infinite loops

Hopefully, we can get another pair of eyes from @metacosm or @iocanel here.

I added the additional test MultipleVersionsCRDTest and tried it locally with parallel enabled. It looks good to me.

Probably, the only way to fully test is to spin a new module like this:
https://github.com/fabric8io/kubernetes-client/tree/main/crd-generator/test
but enabling parallelism.

I agree, this could be an enhancement for the tests.
Should I add the additional module to this PR? Or can we implement this in a seperate PR, maybe together with #5850?

can we implement this in a seperate PR

that's fine for me

andreaTP · 2024-04-05T15:56:06Z

Probably this fixes #5507 as well

metacosm · 2024-04-08T08:18:59Z

...src/main/java/io/fabric8/crd/generator/v1/decorator/EnsureSingleStorageVersionDecorator.java

-      spec.editFirstVersion().withStorage(true).endVersion();
-    }
+    List<String> storageVersions = versions.stream()
+        .filter(v -> Optional.ofNullable(v.getStorage()).orElse(true))


Not sure where we should document that if storage isn't specified, we default to it being true… (storage is required in the spec)
We probably also should document that people might now need to be more explicit in their annotations and set storage to false where needed.

For endusers, it's already documented in the annotation itself:
https://github.com/fabric8io/kubernetes-client/blob/main/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Version.java#L46

/** * Whether this version corresponds to the persisted version for the associated CRD. Note that only one version can set * {@code storage} to {@code true} for a given CRD. * * @return {@code true} if this version corresponds to the persisted version for the associated CRD, {@code false} otherwise */ boolean storage() default true; // <-- here

For (internal) CRDGenerator developers:
If a property like storage is equal to the default value, it is usually set to null in the internal representation.
This allows to omit the property during the schema generation, so that only those properties end up in the schema for which the value is not equal to the schema default.

We could start with a DEVELOPMENT.md file in the CRDGenerator package to document such internal behaviours. (There are a few more things which should be documented e.g. the sorting algorithm for the decorators...)

For endusers, it's already documented in the annotation itself: https://github.com/fabric8io/kubernetes-client/blob/main/kubernetes-model-generator/kubernetes-model-common/src/main/java/io/fabric8/kubernetes/model/annotation/Version.java#L46

/** * Whether this version corresponds to the persisted version for the associated CRD. Note that only one version can set * {@code storage} to {@code true} for a given CRD. * * @return {@code true} if this version corresponds to the persisted version for the associated CRD, {@code false} otherwise */ boolean storage() default true; // <-- here

Right, I missed that. This might be good enough.

metacosm · 2024-04-08T08:41:18Z

Thank you for your improvements and fixes. These are much appreciated!

sonarqubecloud · 2024-04-09T11:53:09Z

Quality Gate failed

Failed conditions
5.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

manusa

LGTM, thx!

baloo42 added 7 commits March 29, 2024 21:40

Extract assertFileEquals from CRDGeneratorTest to seperate CRDGenerat…

1f4e22c

…orAssertions utility class

Extract assertCRDOutputEquals assertion

fa19791

Cleanup CRDGenerator Resource class and allow debugging decorator seq…

ef95378

…uence

Cleanup CRDGeneratorTest

aeca4e1

Add CRDGeneratorExamplesTest

119f4fd

Fix ensuring single version is marked as stored in CRDGenerator

35c38a1

Fix codesmells

94e2454

baloo42 changed the title ~~Iss 5845~~ fix(crd-generator): Fail if multiple versions marked as stored Mar 30, 2024

baloo42 changed the title ~~fix(crd-generator): Fail if multiple versions marked as stored~~ fix(crd-generator): Fail if multiple versions are marked as stored Mar 30, 2024

baloo42 added 2 commits March 30, 2024 18:14

Add changelog

aff6240

Fix code smell

0c8b4cb

baloo42 marked this pull request as ready for review March 30, 2024 18:36

baloo42 requested review from manusa, oscerd, rohanKanojia, shawkins and sunix as code owners March 30, 2024 18:36

Fix code smell

ed9662c

andreaTP approved these changes Apr 1, 2024

View reviewed changes

Add MultipleVersionsCRDTest

2ebb0cd

rohanKanojia approved these changes Apr 4, 2024

View reviewed changes

metacosm approved these changes Apr 8, 2024

View reviewed changes

baloo42 mentioned this pull request Apr 8, 2024

CRDGenerator Development Doc(s) #5870

Open

Merge branch 'main' into iss_5845

6e40f22

manusa added this to the 6.12.0 milestone Apr 9, 2024 — with automated-tasks

manusa approved these changes Apr 9, 2024

View reviewed changes

manusa merged commit 06612c8 into fabric8io:main Apr 9, 2024
16 of 19 checks passed

baloo42 mentioned this pull request Apr 15, 2024

feat(crd-generator): Allow to configure CRDGenerator if running as annotation processor, add approval and compiler tests #5896

Closed

11 tasks

manusa mentioned this pull request Apr 29, 2024

CRD generator sporadically doesn't respect storage-field in @Version annotation #5507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(crd-generator): Fail if multiple versions are marked as stored #5846

fix(crd-generator): Fail if multiple versions are marked as stored #5846

baloo42 commented Mar 30, 2024 •

edited

Loading

andreaTP left a comment

andreaTP Apr 1, 2024

baloo42 Apr 1, 2024

andreaTP Apr 1, 2024

baloo42 Apr 3, 2024

andreaTP Apr 3, 2024

andreaTP commented Apr 5, 2024

metacosm Apr 8, 2024

baloo42 Apr 8, 2024

metacosm Apr 8, 2024

metacosm commented Apr 8, 2024

sonarqubecloud bot commented Apr 9, 2024

manusa left a comment

	// We can't guarantee that `when `decorator a < b and b < c then a < c``.
	// Why?
	// Because our comparators express constraints on particular pairs and can't express the global order.
	// So, in order to be accurate we need to compare each decorator, with ALL OTHER decorators.
	// In other words we don't ANY sorting algorithm, we need bubble sort.
	// We also might need it more than once. So, we'll do it as many times as we have to, till there are not more transformations.
	// But hey, let's have an upper limit of 5 just to prevent infinite loops

fix(crd-generator): Fail if multiple versions are marked as stored #5846

fix(crd-generator): Fail if multiple versions are marked as stored #5846

Conversation

baloo42 commented Mar 30, 2024 • edited Loading

Description

Type of change

Checklist

andreaTP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacosm commented Apr 8, 2024

sonarqubecloud bot commented Apr 9, 2024

Quality Gate failed

manusa left a comment

Choose a reason for hiding this comment

baloo42 commented Mar 30, 2024 •

edited

Loading