Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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 @@ -24,15 +24,25 @@ public class ControllerUtils {
private static Map<Class<? extends CustomResource>, Class<? extends CustomResourceDoneable<? extends CustomResource>>>
doneableClassCache = new HashMap<>();

static String getDefaultFinalizer(ResourceController controller) {
return getAnnotation(controller).finalizerName();
static String getFinalizer(ResourceController controller) {
final String annotationFinalizerName = getAnnotation(controller).finalizerName();
final String finalizerName;
if (Controller.NULL.equals(annotationFinalizerName)) {
finalizerName = controller.getDefaultFinalizerName();
if (finalizerName == null) {
throw new IllegalStateException("Controller annotation cannot be used on Local, Anonymous or Hidden classes");
}
} else {
finalizerName = annotationFinalizerName;
}
return finalizerName;
}

static boolean getGenerationEventProcessing(ResourceController controller) {
return getAnnotation(controller).generationAwareEventProcessing();
}

static <R extends CustomResource> Class<R> getCustomResourceClass(ResourceController controller) {
static <R extends CustomResource> Class<R> getCustomResourceClass(ResourceController<R> controller) {
return (Class<R>) getAnnotation(controller).customResourceClass();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ private <R extends CustomResource> void registerController(ResourceController<R>
Class<R> resClass = ControllerUtils.getCustomResourceClass(controller);
CustomResourceDefinitionContext crd = getCustomResourceDefinitionForController(controller);
KubernetesDeserializer.registerCustomKind(crd.getVersion(), crd.getKind(), resClass);
String finalizer = ControllerUtils.getDefaultFinalizer(controller);
String finalizer = ControllerUtils.getFinalizer(controller);
MixedOperation client = k8sClient.customResources(crd, resClass, CustomResourceList.class, ControllerUtils.getCustomResourceDoneableClass(controller));
EventDispatcher eventDispatcher = new EventDispatcher(controller,
finalizer, new EventDispatcher.CustomResourceFacade(client), ControllerUtils.getGenerationEventProcessing(controller));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,17 @@
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
public @interface Controller {

String DEFAULT_FINALIZER = "operator.default.finalizer";
String NULL = "";

String crdName();

Class<? extends CustomResource> customResourceClass();

String finalizerName() default DEFAULT_FINALIZER;
/**
* Optional finalizer name, if it is not,
* the canonical name of the Controller class will be used as the name of the finalizer.
*/
String finalizerName() default NULL;

/**
* If true, will dispatch new event to the controller if generation increased since the last processing, otherwise will
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,10 @@ public interface ResourceController<R extends CustomResource> {
*/
UpdateControl<R> createOrUpdateResource(R resource, Context<R> context);

// What about we let developer to set the finalizer Name through overriding
// this method rather than setting it as attribute in annotation?
default String getDefaultFinalizerName() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Several places in the code finalizer references are named defaultFinalzer. Any objection to renaming them all to finalizer? I suppose they are indeed referring to the finalizer and not the default one.
e.g.
Several spots in this file:
https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java#L24

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree.. sounds like we're calling everything defaultFinalizer even though we just mean the finalizer. This is actually confusing as there a thing called the default finalizer name. @csviri can you confirm we're seeing this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the default finalizer to finalizer or similar everywhere in docs, logs and code for now.
LMK if you have any objection/thoughts about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@psycho-ir looks good, yep it jus tfinalizer in most of the cases not default finalizer.
I don't think we should use default method in the interface however. Would stick to the annotation to override the finalizer name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I have removed this method already.

return this.getClass().getCanonicalName();
}

}
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package io.javaoperatorsdk.operator;

import io.javaoperatorsdk.operator.sample.TestCustomResource;
import io.javaoperatorsdk.operator.sample.TestCustomResourceController;
import io.fabric8.kubernetes.client.CustomResourceDoneable;
import io.javaoperatorsdk.operator.api.Context;
import io.javaoperatorsdk.operator.api.Controller;
import io.javaoperatorsdk.operator.api.ResourceController;
import io.javaoperatorsdk.operator.api.UpdateControl;
import io.javaoperatorsdk.operator.sample.TestCustomResource;
import io.javaoperatorsdk.operator.sample.TestCustomResourceController;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

Expand All @@ -12,12 +15,33 @@

class ControllerUtilsTest {

public static final String CUSTOM_FINALIZER_NAME = "a.customer.finalizer";

@Test
public void returnsValuesFromControllerAnnotationFinalizer() {
Assertions.assertEquals(Controller.DEFAULT_FINALIZER, ControllerUtils.getDefaultFinalizer(new TestCustomResourceController(null)));
Assertions.assertEquals(TestCustomResourceController.class.getCanonicalName(), ControllerUtils.getFinalizer(new TestCustomResourceController(null)));
assertEquals(TestCustomResource.class, ControllerUtils.getCustomResourceClass(new TestCustomResourceController(null)));
Assertions.assertEquals(TestCustomResourceController.CRD_NAME, ControllerUtils.getCrdName(new TestCustomResourceController(null)));
assertEquals(false, ControllerUtils.getGenerationEventProcessing(new TestCustomResourceController(null)));
assertTrue(CustomResourceDoneable.class.isAssignableFrom(ControllerUtils.getCustomResourceDoneableClass(new TestCustomResourceController(null))));
}

@Controller(crdName = "test.crd", customResourceClass = TestCustomResource.class, finalizerName = CUSTOM_FINALIZER_NAME)
static class TestCustomFinalizerController implements ResourceController<TestCustomResource> {

@Override
public boolean deleteResource(TestCustomResource resource, Context<TestCustomResource> context) {
return false;
}

@Override
public UpdateControl<TestCustomResource> createOrUpdateResource(TestCustomResource resource, Context<TestCustomResource> context) {
return null;
}
}

@Test
public void returnCustomerFinalizerNameIfSet() {
assertEquals(CUSTOM_FINALIZER_NAME, ControllerUtils.getFinalizer(new TestCustomFinalizerController()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class EventDispatcherTest {
@BeforeEach
void setup() {
eventDispatcher = new EventDispatcher(controller,
Controller.DEFAULT_FINALIZER, customResourceFacade, false);
controller.getDefaultFinalizerName(), customResourceFacade, false);

testCustomResource = getResource();

Expand All @@ -45,7 +45,7 @@ void callCreateOrUpdateOnNewResource() {

@Test
void updatesOnlyStatusSubResource() {
testCustomResource.getMetadata().getFinalizers().add(Controller.DEFAULT_FINALIZER);
testCustomResource.getMetadata().getFinalizers().add(controller.getDefaultFinalizerName());
when(controller.createOrUpdateResource(eq(testCustomResource), any()))
.thenReturn(UpdateControl.updateStatusSubResource(testCustomResource));

Expand All @@ -67,13 +67,13 @@ void adsDefaultFinalizerOnCreateIfNotThere() {
eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
verify(controller, times(1))
.createOrUpdateResource(argThat(testCustomResource ->
testCustomResource.getMetadata().getFinalizers().contains(Controller.DEFAULT_FINALIZER)), any());
testCustomResource.getMetadata().getFinalizers().contains(controller.getDefaultFinalizerName())), any());
}

@Test
void callsDeleteIfObjectHasFinalizerAndMarkedForDelete() {
testCustomResource.getMetadata().setDeletionTimestamp("2019-8-10");
testCustomResource.getMetadata().getFinalizers().add(Controller.DEFAULT_FINALIZER);
testCustomResource.getMetadata().getFinalizers().add(controller.getDefaultFinalizerName());

eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));

Expand Down Expand Up @@ -183,7 +183,7 @@ void doesNotMarkNewGenerationInCaseOfException() {

void generationAwareMode() {
eventDispatcher = new EventDispatcher(controller,
Controller.DEFAULT_FINALIZER, customResourceFacade, true);
controller.getDefaultFinalizerName(), customResourceFacade, true);
}

private void markForDeletion(CustomResource customResource) {
Expand All @@ -202,7 +202,7 @@ CustomResource getResource() {
.withDeletionGracePeriodSeconds(10L)
.withGeneration(10L)
.withName("name")
.withFinalizers(Controller.DEFAULT_FINALIZER)
.withFinalizers(controller.getDefaultFinalizerName())
.withNamespace("namespace")
.withResourceVersion("resourceVersion")
.withSelfLink("selfLink")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public SubResourceTestCustomResource createTestCustomResource(String id) {
resource.setMetadata(new ObjectMetaBuilder()
.withName("subresource-" + id)
.withNamespace(TEST_NAMESPACE)
.withFinalizers(Controller.DEFAULT_FINALIZER)
.withFinalizers("finalizer")
.build());
resource.setKind("SubresourceSample");
resource.setSpec(new SubResourceTestCustomResourceSpec());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public boolean deleteResource(TestCustomResource resource, Context<TestCustomRes
public UpdateControl<TestCustomResource> createOrUpdateResource(TestCustomResource resource,
Context<TestCustomResource> context) {
numberOfExecutions.addAndGet(1);
if (!resource.getMetadata().getFinalizers().contains(Controller.DEFAULT_FINALIZER)) {
if (!resource.getMetadata().getFinalizers().contains(this.getDefaultFinalizerName())) {
throw new IllegalStateException("Finalizer is not present.");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public class SubResourceTestCustomResourceController implements ResourceControll

public static final String CRD_NAME = "subresourcesample.sample.javaoperatorsdk";
private static final Logger log = LoggerFactory.getLogger(SubResourceTestCustomResourceController.class);
private AtomicInteger numberOfExecutions = new AtomicInteger(0);
private final AtomicInteger numberOfExecutions = new AtomicInteger(0);

@Override
public boolean deleteResource(SubResourceTestCustomResource resource, Context<SubResourceTestCustomResource> context) {
Expand All @@ -30,7 +30,7 @@ public boolean deleteResource(SubResourceTestCustomResource resource, Context<Su
public UpdateControl<SubResourceTestCustomResource> createOrUpdateResource(SubResourceTestCustomResource resource,
Context<SubResourceTestCustomResource> context) {
numberOfExecutions.addAndGet(1);
if (!resource.getMetadata().getFinalizers().contains(Controller.DEFAULT_FINALIZER)) {
if (!resource.getMetadata().getFinalizers().contains(this.getDefaultFinalizerName())) {
throw new IllegalStateException("Finalizer is not present.");
}
log.info("Value: " + resource.getSpec().getValue());
Expand Down