Skip to content

Commit bc65b1c

Browse files
kirek007kirek007
authored
Merge pull request #223 from psycho-ir/controller-name-as-finalizer
Controller name as finalizer by default
2 parents 39f94ca + fe4ef59 commit bc65b1c

File tree

14 files changed

+103
-120
lines changed

14 files changed

+103
-120
lines changed

‎.github/workflows/ci.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ jobs:
2929
- name: Set up Minikube
3030
uses: manusa/actions-setup-minikube@v2.0.1
3131
with:
32-
minikube version: 'v1.15.0'
32+
minikube version: 'v1.15.1'
3333
kubernetes version: ${{ matrix.kubernetes }}
3434
driver: 'docker'
3535
- name: Run integration tests

‎operator-framework/src/main/java/io/javaoperatorsdk/operator/ControllerUtils.java

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,27 @@
1616
public class ControllerUtils {
1717

1818
private final static double JAVA_VERSION = Double.parseDouble(System.getProperty("java.specification.version"));
19-
20-
private final static Logger log = LoggerFactory.getLogger(ControllerUtils.class);
19+
private static final String FINALIZER_NAME_SUFFIX = "/finalizer";
2120

2221
// this is just to support testing, this way we don't try to create class multiple times in memory with same name.
2322
// note that other solution is to add a random string to doneable class name
2423
private static Map<Class<? extends CustomResource>, Class<? extends CustomResourceDoneable<? extends CustomResource>>>
2524
doneableClassCache = new HashMap<>();
2625

27-
static String getDefaultFinalizer(ResourceController controller) {
28-
return getAnnotation(controller).finalizerName();
26+
static String getFinalizer(ResourceController controller) {
27+
final String annotationFinalizerName = getAnnotation(controller).finalizerName();
28+
if (!Controller.NULL.equals(annotationFinalizerName)) {
29+
return annotationFinalizerName;
30+
}
31+
final String crdName = getAnnotation(controller).crdName() + FINALIZER_NAME_SUFFIX;
32+
return crdName;
2933
}
3034

3135
static boolean getGenerationEventProcessing(ResourceController controller) {
3236
return getAnnotation(controller).generationAwareEventProcessing();
3337
}
3438

35-
static <R extends CustomResource> Class<R> getCustomResourceClass(ResourceController controller) {
39+
static <R extends CustomResource> Class<R> getCustomResourceClass(ResourceController<R> controller) {
3640
return (Class<R>) getAnnotation(controller).customResourceClass();
3741
}
3842

@@ -79,10 +83,7 @@ private static Controller getAnnotation(ResourceController controller) {
7983
return controller.getClass().getAnnotation(Controller.class);
8084
}
8185

82-
public static boolean hasDefaultFinalizer(CustomResource resource, String finalizer) {
83-
if (resource.getMetadata().getFinalizers() != null) {
84-
return resource.getMetadata().getFinalizers().contains(finalizer);
85-
}
86-
return false;
86+
public static boolean hasGivenFinalizer(CustomResource resource, String finalizer) {
87+
return resource.getMetadata().getFinalizers() != null && resource.getMetadata().getFinalizers().contains(finalizer);
8788
}
8889
}

‎operator-framework/src/main/java/io/javaoperatorsdk/operator/Operator.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ private <R extends CustomResource> void registerController(ResourceController<R>
5555
Class<R> resClass = ControllerUtils.getCustomResourceClass(controller);
5656
CustomResourceDefinitionContext crd = getCustomResourceDefinitionForController(controller);
5757
KubernetesDeserializer.registerCustomKind(crd.getVersion(), crd.getKind(), resClass);
58-
String finalizer = ControllerUtils.getDefaultFinalizer(controller);
58+
String finalizer = ControllerUtils.getFinalizer(controller);
5959
MixedOperation client = k8sClient.customResources(crd, resClass, CustomResourceList.class, ControllerUtils.getCustomResourceDoneableClass(controller));
6060
EventDispatcher eventDispatcher = new EventDispatcher(controller,
6161
finalizer, new EventDispatcher.CustomResourceFacade(client), ControllerUtils.getGenerationEventProcessing(controller));

‎operator-framework/src/main/java/io/javaoperatorsdk/operator/api/Controller.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,17 @@
1010
@Retention(RetentionPolicy.RUNTIME)
1111
@Target({ElementType.TYPE})
1212
public @interface Controller {
13-
14-
String DEFAULT_FINALIZER = "operator.default.finalizer";
13+
String NULL = "";
1514

1615
String crdName();
1716

1817
Class<? extends CustomResource> customResourceClass();
1918

20-
String finalizerName() default DEFAULT_FINALIZER;
19+
/**
20+
* Optional finalizer name, if it is not,
21+
* the crdName will be used as the name of the finalizer too.
22+
*/
23+
String finalizerName() default NULL;
2124

2225
/**
2326
* If true, will dispatch new event to the controller if generation increased since the last processing, otherwise will

‎operator-framework/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ public interface ResourceController<R extends CustomResource> {
66

77
/**
88
* The implementation should delete the associated component(s). Note that this is method is called when an object
9-
* is marked for deletion. After its executed the default finalizer is automatically removed by the framework;
9+
* is marked for deletion. After its executed the custom resource finalizer is automatically removed by the framework;
1010
* unless the return value is false - note that this is almost never the case.
1111
* Its important to have the implementation also idempotent, in the current implementation to cover all edge cases
1212
* actually will be executed mostly twice.
@@ -27,5 +27,4 @@ public interface ResourceController<R extends CustomResource> {
2727
* <b>However we will always call an update if there is no finalizer on object and its not marked for deletion.</b>
2828
*/
2929
UpdateControl<R> createOrUpdateResource(R resource, Context<R> context);
30-
3130
}

‎operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,17 @@ public class EventDispatcher {
2121
private final static Logger log = LoggerFactory.getLogger(EventDispatcher.class);
2222

2323
private final ResourceController controller;
24-
private final String resourceDefaultFinalizer;
24+
private final String resourceFinalizer;
2525
private final CustomResourceFacade customResourceFacade;
2626
private final boolean generationAware;
2727
private final Map<String, Long> lastGenerationProcessedSuccessfully = new ConcurrentHashMap<>();
2828

2929
public EventDispatcher(ResourceController controller,
30-
String defaultFinalizer,
30+
String finalizer,
3131
CustomResourceFacade customResourceFacade, boolean generationAware) {
3232
this.controller = controller;
3333
this.customResourceFacade = customResourceFacade;
34-
this.resourceDefaultFinalizer = defaultFinalizer;
34+
this.resourceFinalizer = finalizer;
3535
this.generationAware = generationAware;
3636
}
3737

@@ -43,24 +43,24 @@ public void handleEvent(CustomResourceEvent event) {
4343
log.error("Received error for resource: {}", resource.getMetadata().getName());
4444
return;
4545
}
46-
if (markedForDeletion(resource) && !ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer)) {
47-
log.debug("Skipping event dispatching since its marked for deletion but has no default finalizer: {}", event);
46+
if (markedForDeletion(resource) && !ControllerUtils.hasGivenFinalizer(resource, resourceFinalizer)) {
47+
log.debug("Skipping event dispatching since its marked for deletion but does not have finalizer: {}", event);
4848
return;
4949
}
5050
Context context = new DefaultContext(new RetryInfo(event.getRetryCount(), event.getRetryExecution().isLastExecution()));
5151
if (markedForDeletion(resource)) {
5252
boolean removeFinalizer = controller.deleteResource(resource, context);
53-
boolean hasDefaultFinalizer = ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer);
54-
if (removeFinalizer && hasDefaultFinalizer) {
55-
removeDefaultFinalizer(resource);
53+
boolean hasFinalizer = ControllerUtils.hasGivenFinalizer(resource, resourceFinalizer);
54+
if (removeFinalizer && hasFinalizer) {
55+
removeFinalizer(resource);
5656
} else {
57-
log.debug("Skipping finalizer remove. removeFinalizer: {}, hasDefaultFinalizer: {} ",
58-
removeFinalizer, hasDefaultFinalizer);
57+
log.debug("Skipping finalizer remove. removeFinalizer: {}, hasFinalizer: {} ",
58+
removeFinalizer, hasFinalizer);
5959
}
6060
cleanup(resource);
6161
} else {
62-
if (!ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer) && !markedForDeletion(resource)) {
63-
/* We always add the default finalizer if missing and not marked for deletion.
62+
if (!ControllerUtils.hasGivenFinalizer(resource, resourceFinalizer) && !markedForDeletion(resource)) {
63+
/* We always add a finalizer if missing and not marked for deletion.
6464
We execute the controller processing only for processing the event sent as a results
6565
of the finalizer add. This will make sure that the resources are not created before
6666
there is a finalizer.
@@ -118,9 +118,9 @@ private void updateCustomResource(CustomResource updatedResource) {
118118
}
119119

120120

121-
private void removeDefaultFinalizer(CustomResource resource) {
121+
private void removeFinalizer(CustomResource resource) {
122122
log.debug("Removing finalizer on resource {}:", resource);
123-
resource.getMetadata().getFinalizers().remove(resourceDefaultFinalizer);
123+
resource.getMetadata().getFinalizers().remove(resourceFinalizer);
124124
customResourceFacade.replaceWithLock(resource);
125125
}
126126

@@ -130,12 +130,12 @@ private void replace(CustomResource resource) {
130130
}
131131

132132
private void addFinalizerIfNotPresent(CustomResource resource) {
133-
if (!ControllerUtils.hasDefaultFinalizer(resource, resourceDefaultFinalizer) && !markedForDeletion(resource)) {
134-
log.info("Adding default finalizer to {}", resource.getMetadata());
133+
if (!ControllerUtils.hasGivenFinalizer(resource, resourceFinalizer) && !markedForDeletion(resource)) {
134+
log.info("Adding finalizer {} to {}", resourceFinalizer, resource.getMetadata());
135135
if (resource.getMetadata().getFinalizers() == null) {
136136
resource.getMetadata().setFinalizers(new ArrayList<>(1));
137137
}
138-
resource.getMetadata().getFinalizers().add(resourceDefaultFinalizer);
138+
resource.getMetadata().getFinalizers().add(resourceFinalizer);
139139
}
140140
}
141141

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/ControllerExecutionIT.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
@TestInstance(TestInstance.Lifecycle.PER_METHOD)
2222
public class ControllerExecutionIT {
2323

24-
private final static Logger log = LoggerFactory.getLogger(ControllerExecutionIT.class);
2524
public static final String TEST_CUSTOM_RESOURCE_NAME = "test-custom-resource";
2625
private IntegrationTestSupport integrationTestSupport = new IntegrationTestSupport();
2726

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/ControllerUtilsTest.java

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,46 @@
11
package io.javaoperatorsdk.operator;
22

3-
import io.javaoperatorsdk.operator.sample.TestCustomResource;
4-
import io.javaoperatorsdk.operator.sample.TestCustomResourceController;
53
import io.fabric8.kubernetes.client.CustomResourceDoneable;
4+
import io.javaoperatorsdk.operator.api.Context;
65
import io.javaoperatorsdk.operator.api.Controller;
6+
import io.javaoperatorsdk.operator.api.ResourceController;
7+
import io.javaoperatorsdk.operator.api.UpdateControl;
8+
import io.javaoperatorsdk.operator.sample.TestCustomResource;
9+
import io.javaoperatorsdk.operator.sample.TestCustomResourceController;
710
import org.junit.jupiter.api.Assertions;
811
import org.junit.jupiter.api.Test;
912

10-
import static org.junit.jupiter.api.Assertions.assertEquals;
11-
import static org.junit.jupiter.api.Assertions.assertTrue;
13+
import static org.junit.jupiter.api.Assertions.*;
1214

1315
class ControllerUtilsTest {
1416

17+
public static final String CUSTOM_FINALIZER_NAME = "a.custom/finalizer";
18+
1519
@Test
1620
public void returnsValuesFromControllerAnnotationFinalizer() {
17-
Assertions.assertEquals(Controller.DEFAULT_FINALIZER, ControllerUtils.getDefaultFinalizer(new TestCustomResourceController(null)));
21+
Assertions.assertEquals(TestCustomResourceController.CRD_NAME + "/finalizer", ControllerUtils.getFinalizer(new TestCustomResourceController(null)));
1822
assertEquals(TestCustomResource.class, ControllerUtils.getCustomResourceClass(new TestCustomResourceController(null)));
1923
Assertions.assertEquals(TestCustomResourceController.CRD_NAME, ControllerUtils.getCrdName(new TestCustomResourceController(null)));
20-
assertEquals(false, ControllerUtils.getGenerationEventProcessing(new TestCustomResourceController(null)));
24+
assertFalse(ControllerUtils.getGenerationEventProcessing(new TestCustomResourceController(null)));
2125
assertTrue(CustomResourceDoneable.class.isAssignableFrom(ControllerUtils.getCustomResourceDoneableClass(new TestCustomResourceController(null))));
2226
}
27+
28+
@Controller(crdName = "test.crd", customResourceClass = TestCustomResource.class, finalizerName = CUSTOM_FINALIZER_NAME)
29+
static class TestCustomFinalizerController implements ResourceController<TestCustomResource> {
30+
31+
@Override
32+
public boolean deleteResource(TestCustomResource resource, Context<TestCustomResource> context) {
33+
return false;
34+
}
35+
36+
@Override
37+
public UpdateControl<TestCustomResource> createOrUpdateResource(TestCustomResource resource, Context<TestCustomResource> context) {
38+
return null;
39+
}
40+
}
41+
42+
@Test
43+
public void returnCustomerFinalizerNameIfSet() {
44+
assertEquals(CUSTOM_FINALIZER_NAME, ControllerUtils.getFinalizer(new TestCustomFinalizerController()));
45+
}
2346
}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/EventDispatcherTest.java

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,16 @@
2020

2121
class EventDispatcherTest {
2222

23+
public static final String FINALIZER_NAME = "finalizer";
2324
private CustomResource testCustomResource;
2425
private EventDispatcher eventDispatcher;
25-
private ResourceController<CustomResource> controller = mock(ResourceController.class);
26-
private EventDispatcher.CustomResourceFacade customResourceFacade = mock(EventDispatcher.CustomResourceFacade.class);
26+
private final ResourceController<CustomResource> controller = mock(ResourceController.class);
27+
private final EventDispatcher.CustomResourceFacade customResourceFacade = mock(EventDispatcher.CustomResourceFacade.class);
2728

2829
@BeforeEach
2930
void setup() {
3031
eventDispatcher = new EventDispatcher(controller,
31-
Controller.DEFAULT_FINALIZER, customResourceFacade, false);
32+
FINALIZER_NAME, customResourceFacade, false);
3233

3334
testCustomResource = getResource();
3435

@@ -45,7 +46,7 @@ void callCreateOrUpdateOnNewResource() {
4546

4647
@Test
4748
void updatesOnlyStatusSubResource() {
48-
testCustomResource.getMetadata().getFinalizers().add(Controller.DEFAULT_FINALIZER);
49+
testCustomResource.getMetadata().getFinalizers().add(FINALIZER_NAME);
4950
when(controller.createOrUpdateResource(eq(testCustomResource), any()))
5051
.thenReturn(UpdateControl.updateStatusSubResource(testCustomResource));
5152

@@ -63,17 +64,17 @@ void callCreateOrUpdateOnModifiedResource() {
6364
}
6465

6566
@Test
66-
void adsDefaultFinalizerOnCreateIfNotThere() {
67+
void addsFinalizerOnCreateIfNotThere() {
6768
eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
6869
verify(controller, times(1))
6970
.createOrUpdateResource(argThat(testCustomResource ->
70-
testCustomResource.getMetadata().getFinalizers().contains(Controller.DEFAULT_FINALIZER)), any());
71+
testCustomResource.getMetadata().getFinalizers().contains(FINALIZER_NAME)), any());
7172
}
7273

7374
@Test
7475
void callsDeleteIfObjectHasFinalizerAndMarkedForDelete() {
7576
testCustomResource.getMetadata().setDeletionTimestamp("2019-8-10");
76-
testCustomResource.getMetadata().getFinalizers().add(Controller.DEFAULT_FINALIZER);
77+
testCustomResource.getMetadata().getFinalizers().add(FINALIZER_NAME);
7778

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

@@ -84,7 +85,7 @@ void callsDeleteIfObjectHasFinalizerAndMarkedForDelete() {
8485
* Note that there could be more finalizers. Out of our control.
8586
*/
8687
@Test
87-
void callDeleteOnControllerIfMarkedForDeletionButThereIsNoDefaultFinalizer() {
88+
void callDeleteOnControllerIfMarkedForDeletionButThereIsNoFinalizer() {
8889
markForDeletion(testCustomResource);
8990

9091
eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
@@ -93,7 +94,7 @@ void callDeleteOnControllerIfMarkedForDeletionButThereIsNoDefaultFinalizer() {
9394
}
9495

9596
@Test
96-
void removesDefaultFinalizerOnDelete() {
97+
void removesFinalizerOnDelete() {
9798
markForDeletion(testCustomResource);
9899

99100
eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
@@ -172,9 +173,8 @@ void doesNotMarkNewGenerationInCaseOfException() {
172173
.thenThrow(new IllegalStateException("Exception for testing purposes"))
173174
.thenReturn(UpdateControl.noUpdate());
174175

175-
Assertions.assertThrows(IllegalStateException.class, () -> {
176-
eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
177-
});
176+
Assertions.assertThrows(IllegalStateException.class, () ->
177+
eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource)));
178178
eventDispatcher.handleEvent(customResourceEvent(Watcher.Action.MODIFIED, testCustomResource));
179179

180180
verify(controller, times(2)).createOrUpdateResource(eq(testCustomResource), any());
@@ -183,7 +183,7 @@ void doesNotMarkNewGenerationInCaseOfException() {
183183

184184
void generationAwareMode() {
185185
eventDispatcher = new EventDispatcher(controller,
186-
Controller.DEFAULT_FINALIZER, customResourceFacade, true);
186+
FINALIZER_NAME, customResourceFacade, true);
187187
}
188188

189189
private void markForDeletion(CustomResource customResource) {
@@ -202,7 +202,7 @@ CustomResource getResource() {
202202
.withDeletionGracePeriodSeconds(10L)
203203
.withGeneration(10L)
204204
.withName("name")
205-
.withFinalizers(Controller.DEFAULT_FINALIZER)
205+
.withFinalizers(FINALIZER_NAME)
206206
.withNamespace("namespace")
207207
.withResourceVersion("resourceVersion")
208208
.withSelfLink("selfLink")

0 commit comments

Comments
 (0)