Skip to content

Multiple version in CRD Support #879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 31, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ public synchronized void stop() {

public synchronized void add(Controller controller) {
final var configuration = controller.getConfiguration();
final var resourceTypeName = configuration.getResourceTypeName();
final var resourceTypeName = ReconcilerUtils
.getResourceTypeNameWithVersion(configuration.getResourceClass());
final var existing = controllers.get(resourceTypeName);
if (existing != null) {
throw new OperatorException("Cannot register controller '" + configuration.getName()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,13 @@ public void setApiVersion(String s) {
return Constants.NO_FINALIZER.equals(finalizer) || validator.isFinalizerValid(finalizer);
}

public static String getResourceTypeName(Class<? extends HasMetadata> resourceClass) {
// todo: use fabric8 method when 5.12 is released
// return HasMetadata.getFullResourceName(resourceClass);
public static String getResourceTypeNameWithVersion(Class<? extends HasMetadata> resourceClass) {
final var version = HasMetadata.getVersion(resourceClass);
return getResourceTypeName(resourceClass) + "/" + version;
}

public static String getResourceTypeName(
Class<? extends HasMetadata> resourceClass) {
final var group = HasMetadata.getGroup(resourceClass);
final var plural = HasMetadata.getPlural(resourceClass);
return (group == null || group.isEmpty()) ? plural : plural + "." + group;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void start() {
try {
eventSource.start();
} catch (Exception e) {
log.warn("Error starting {} -> {}", eventSource, e);
log.warn("Error starting {}", eventSource, e);
}
}
eventProcessor.start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@
import io.javaoperatorsdk.operator.processing.Controller;
import io.javaoperatorsdk.operator.sample.simple.DuplicateCRController;
import io.javaoperatorsdk.operator.sample.simple.TestCustomReconciler;
import io.javaoperatorsdk.operator.sample.simple.TestCustomReconcilerV2;
import io.javaoperatorsdk.operator.sample.simple.TestCustomReconcilerOtherV1;
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;
import io.javaoperatorsdk.operator.sample.simple.TestCustomResourceV2;
import io.javaoperatorsdk.operator.sample.simple.TestCustomResourceOtherV1;

import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand All @@ -30,11 +30,11 @@ public void shouldNotAddMultipleControllersForSameCustomResource() {
}

@Test
public void addingMultipleControllersForCustomResourcesWithDifferentVersionsShouldNotWork() {
public void addingMultipleControllersForCustomResourcesWithSameVersionsShouldNotWork() {
final var registered = new TestControllerConfiguration<>(new TestCustomReconciler(null),
TestCustomResource.class);
final var duplicated = new TestControllerConfiguration<>(new TestCustomReconcilerV2(),
TestCustomResourceV2.class);
final var duplicated = new TestControllerConfiguration<>(new TestCustomReconcilerOtherV1(),
TestCustomResourceOtherV1.class);

checkException(registered, duplicated);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;

@ControllerConfiguration
public class TestCustomReconcilerV2 implements Reconciler<TestCustomResourceV2> {
public class TestCustomReconcilerOtherV1 implements Reconciler<TestCustomResourceOtherV1> {

@Override
public UpdateControl<TestCustomResourceV2> reconcile(TestCustomResourceV2 resource,
public UpdateControl<TestCustomResourceOtherV1> reconcile(TestCustomResourceOtherV1 resource,
Context context) {
return UpdateControl.noUpdate();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
import io.fabric8.kubernetes.model.annotation.Version;

@Group("sample.javaoperatorsdk.io")
@Version("v2")
@Version("v1")
@Kind("TestCustomResource") // this is needed to override the automatically generated kind
public class TestCustomResourceV2
public class TestCustomResourceOtherV1
extends CustomResource<TestCustomResourceSpec, TestCustomResourceStatus> {

}
11 changes: 11 additions & 0 deletions smoke-test-samples/multi-version-crd/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
## Description

This sample shows how to use CRD with multiple versions.

## How-to

For getting the resource with target version use:

`k get MultiVersionCRDTestCustomResource.v1.sample.javaoperatorsdk mvcv1 -o yaml`

`k get MultiVersionCRDTestCustomResource.v2.sample.javaoperatorsdk mvcv1 -o yaml`
9 changes: 9 additions & 0 deletions smoke-test-samples/multi-version-crd/k8s/crv1.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
apiVersion: "sample.javaoperatorsdk/v1"
kind: MultiVersionCRDTestCustomResource
metadata:
name: mvcv1
labels:
version: v1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an example to show what happens if one of the label is missing and the Spec is incompatible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To cover that we would need to add a conversion web-hook, build it and etc, what would be a relatively significant effort. Without conversion hook that would fail, but that is expected; it would not be a bug; We can create a separate example to cover that, here just plan to add an integration test.

But the goal should not be to "test Kubernetes", maybe show best practices also. But that again not sure if it should go to core JOSDK. The current goal is to showcase features of sdk, and show how operators are implement / tested, not best practices - that is already covered in kubernetes.

On the other hand I recognize that this might be not trivial case. So I would propose to create a separate issues, first discuss where to put such sample - with conversion hook - and implement it after.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the SDK should not simply fails if there are no labels and no conversion webhooks, but should gracefully handle the situation.

Consider the following scenario:

  • the webhook is not in place
  • a user creates a CR without labels
  • the operator will start crashing

I do believe that this is not the desired nor expected behaviour.

Copy link
Collaborator Author

@csviri csviri Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreaTP I updated the sample, pls check last commit. Now every CR that has no label is processed only by v1 reconciler. Maybe this is a better example this way to shed a light how this is meant. (The integration test is not there yet)

Basically we can create a separate issues and discuss the graceful handling of error case. But this is not how it is meant, if there is missing conversion hook and the custom resources are not compatible without it, it is a bug in the system. These are typed APIs for a reason.

What I can imagin is if a CR received what we are not able to par, we log an error, but no means should reach the reconciler.

Copy link
Collaborator Author

@csviri csviri Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a user creates a CR without labels

This can be easily prevented by a validation hook. (But again changed the example so its more aware of this situtation)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this sounds like this should not be part of the SDK at all if it's possible for a user to crash the operator just by using a specially crafted CR, webhooks or no webhooks because that basically means that you're trying to bypass the type system and I don't think that this should be encouraged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just an integration test that test if multiple versions of CR and Controllers can be created. (btw this changed a little bit since). Nothing more.

I think our goal should be in general to support all the possible use cases that Kubernetes supports. Unfortunately K8S is quite open how to use this feature (the multiple versions of CR in CRD). So in my opinion this approach is how it makes sense, thus to allow registering controller for different versions. From that point only the users responsibility to figure out the rest, get conversion hooks in place etc.

I think we should have a separate sample eventually in combination with the other framework (now called admission-controller-framework) - that will support conversion hooks - a complete setup.

I think it's totall valid to use labels for canary testing and add labels to the CRs only if user want it to be used with an other version of controller. But again this is just a test, don't want to set a patter. We could simplify it and just remove the labels, create as simple IT as possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also open to suggestions to alternatives how to support and/or test this.

spec:
value1: 1
value2: 2
8 changes: 8 additions & 0 deletions smoke-test-samples/multi-version-crd/k8s/crv2.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: "sample.javaoperatorsdk/v2"
kind: MultiVersionCRDTestCustomResource
metadata:
name: mvcv2
labels:
version: v2
spec:
value1: 1
48 changes: 48 additions & 0 deletions smoke-test-samples/multi-version-crd/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<parent>
<groupId>io.javaoperatorsdk</groupId>
<artifactId>java-operator-sdk-smoke-test-samples</artifactId>
<version>2.0.3-SNAPSHOT</version>
</parent>

<artifactId>operator-framework-smoke-test-samples-multi-version-crd</artifactId>
<name>Operator SDK - Smoke Test Samples - Multi Version CRD</name>
<description>Sample usage with pure java app</description>
<packaging>jar</packaging>

<dependencies>
<dependency>
<groupId>io.javaoperatorsdk</groupId>
<artifactId>operator-framework</artifactId>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>io.fabric8</groupId>
<artifactId>crd-generator-apt</artifactId>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package io.javaoperatorsdk.operator.sample.multiversioncrd;

import io.javaoperatorsdk.operator.Operator;
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceOverrider;
import io.javaoperatorsdk.operator.config.runtime.DefaultConfigurationService;

public class MultiVersionCRDOperator {

public static void main(String[] args) {
Operator operator =
new Operator(
ConfigurationServiceOverrider.override(DefaultConfigurationService.instance()).build());
operator.register(new MultiVersionCRDTestReconciler1());
operator.register(new MultiVersionCRDTestReconciler2());
operator.start();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package io.javaoperatorsdk.operator.sample.multiversioncrd;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
import io.javaoperatorsdk.operator.sample.multiversioncrd.cr.MultiVersionCRDTestCustomResource1;

import static io.javaoperatorsdk.operator.api.reconciler.Constants.NO_FINALIZER;

@ControllerConfiguration(finalizerName = NO_FINALIZER, labelSelector = "version in (v1)")
public class MultiVersionCRDTestReconciler1
implements Reconciler<MultiVersionCRDTestCustomResource1> {

private static final Logger log = LoggerFactory.getLogger(MultiVersionCRDTestReconciler1.class);

@Override
public UpdateControl<MultiVersionCRDTestCustomResource1> reconcile(
MultiVersionCRDTestCustomResource1 resource, Context context) {
log.info("Reconcile MultiVersionCRDTestCustomResource1: {}",
resource.getMetadata().getName());
resource.getStatus().setValue1(resource.getStatus().getValue1() + 1);
resource.getStatus().setValue2(resource.getStatus().getValue2() + 1);
if (!resource.getStatus().getReconciledBy().contains(getClass().getSimpleName())) {
resource.getStatus().getReconciledBy().add(getClass().getSimpleName());
}
return UpdateControl.updateStatus(resource);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package io.javaoperatorsdk.operator.sample.multiversioncrd;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
import io.javaoperatorsdk.operator.sample.multiversioncrd.cr.MultiVersionCRDTestCustomResource2;

import static io.javaoperatorsdk.operator.api.reconciler.Constants.NO_FINALIZER;

@ControllerConfiguration(
finalizerName = NO_FINALIZER,
labelSelector = "version in (v2)")
public class MultiVersionCRDTestReconciler2
implements Reconciler<MultiVersionCRDTestCustomResource2> {

private static final Logger log = LoggerFactory.getLogger(MultiVersionCRDTestReconciler2.class);

@Override
public UpdateControl<MultiVersionCRDTestCustomResource2> reconcile(
MultiVersionCRDTestCustomResource2 resource, Context context) {
log.info("Reconcile MultiVersionCRDTestCustomResource2: {}",
resource.getMetadata().getName());
resource.getStatus().setValue1(resource.getStatus().getValue1() + 1);
if (!resource.getStatus().getReconciledBy().contains(getClass().getSimpleName())) {
resource.getStatus().getReconciledBy().add(getClass().getSimpleName());
}
return UpdateControl.updateStatus(resource);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package io.javaoperatorsdk.operator.sample.multiversioncrd.cr;

import io.fabric8.kubernetes.api.model.Namespaced;
import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.model.annotation.Group;
import io.fabric8.kubernetes.model.annotation.Kind;
import io.fabric8.kubernetes.model.annotation.ShortNames;
import io.fabric8.kubernetes.model.annotation.Version;

@Group("sample.javaoperatorsdk")
@Version("v1")
@Kind("MultiVersionCRDTestCustomResource")
@ShortNames("mv1")
public class MultiVersionCRDTestCustomResource1
extends
CustomResource<MultiVersionCRDTestCustomResourceSpec1, MultiVersionCRDTestCustomResourceStatus1>
implements Namespaced {

@Override
protected MultiVersionCRDTestCustomResourceStatus1 initStatus() {
return new MultiVersionCRDTestCustomResourceStatus1();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package io.javaoperatorsdk.operator.sample.multiversioncrd.cr;

import io.fabric8.kubernetes.api.model.Namespaced;
import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.model.annotation.Group;
import io.fabric8.kubernetes.model.annotation.Kind;
import io.fabric8.kubernetes.model.annotation.ShortNames;
import io.fabric8.kubernetes.model.annotation.Version;

@Group("sample.javaoperatorsdk")
@Version(value = "v2", storage = false)
@Kind("MultiVersionCRDTestCustomResource")
@ShortNames("mv2")
public class MultiVersionCRDTestCustomResource2
extends
CustomResource<MultiVersionCRDTestCustomResourceSpec2, MultiVersionCRDTestCustomResourceStatus2>
implements Namespaced {

@Override
protected MultiVersionCRDTestCustomResourceStatus2 initStatus() {
return new MultiVersionCRDTestCustomResourceStatus2();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package io.javaoperatorsdk.operator.sample.multiversioncrd.cr;

public class MultiVersionCRDTestCustomResourceSpec1 {

private int value1;

private int value2;

public int getValue1() {
return value1;
}

public MultiVersionCRDTestCustomResourceSpec1 setValue1(int value1) {
this.value1 = value1;
return this;
}

public int getValue2() {
return value2;
}

public MultiVersionCRDTestCustomResourceSpec1 setValue2(int value2) {
this.value2 = value2;
return this;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package io.javaoperatorsdk.operator.sample.multiversioncrd.cr;

public class MultiVersionCRDTestCustomResourceSpec2 {

private int value1;

public int getValue1() {
return value1;
}

public MultiVersionCRDTestCustomResourceSpec2 setValue1(int value1) {
this.value1 = value1;
return this;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package io.javaoperatorsdk.operator.sample.multiversioncrd.cr;

import java.util.ArrayList;
import java.util.List;

public class MultiVersionCRDTestCustomResourceStatus1 {

private int value1;

private int value2;

private List<String> reconciledBy = new ArrayList<>();

public int getValue1() {
return value1;
}

public MultiVersionCRDTestCustomResourceStatus1 setValue1(int value1) {
this.value1 = value1;
return this;
}

public int getValue2() {
return value2;
}

public MultiVersionCRDTestCustomResourceStatus1 setValue2(int value2) {
this.value2 = value2;
return this;
}

public List<String> getReconciledBy() {
return reconciledBy;
}

public MultiVersionCRDTestCustomResourceStatus1 setReconciledBy(List<String> reconciledBy) {
this.reconciledBy = reconciledBy;
return this;
}
}
Loading