Skip to content

Commit

Permalink
Fix fabric8io#2781: RawCustomResourceOperationsImpl#delete should ret…
Browse files Browse the repository at this point in the history 
…urn a boolean value

Align RawCustomResource delete API to return a boolean value indicating
status for deletion just like we have in regular DSL. Right now we're
just throwing an exception in case item doesn't exist in server.
  • Loading branch information
@rohanKanojia
rohanKanojia committed Feb 22, 2021
1 parent 6d93cbc commit f45923e
Show file tree
Hide file tree
Showing 6 changed files with 265 additions and 56 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#### Bugs

#### Improvements
* Fix #2781: RawCustomResourceOperationsImpl#delete now returns a boolean value for deletion status

#### Dependency Upgrade

Expand Down
67 changes: 45 additions & 22 deletions .../main/java/io/fabric8/kubernetes/client/dsl/internal/RawCustomResourceOperationsImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import io.fabric8.kubernetes.api.model.DeletionPropagation;
import io.fabric8.kubernetes.api.model.ListOptions;
import io.fabric8.kubernetes.api.model.ListOptionsBuilder;
import io.fabric8.kubernetes.api.model.Status;
import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.client.Watch;
Expand Down Expand Up @@ -422,10 +423,10 @@ public Map<String, Object> list(String namespace, Map<String, String> labels) {
* Delete all custom resources in a specific namespace
*
* @param namespace desired namespace
* @return deleted objects as HashMap
* @return a boolean value whether item was deleted or item didn't exist in server
*/
public Map<String, Object> delete(String namespace) {
return makeCall(fetchUrl(namespace, null, null), null, HttpCallMethod.DELETE);
public boolean delete(String namespace) {
return handleDelete(namespace, null, null);
}

/**
Expand All @@ -434,11 +435,11 @@ public Map<String, Object> delete(String namespace) {
* @param namespace desired namespace
* @param cascading whether dependent object need to be orphaned or not. If true/false, the "orphan"
* finalizer will be added to/removed from the object's finalizers list.
* @return deleted objects as HashMap
* @return a boolean value whether item was deleted or item didn't exist in server
* @throws IOException in case of any network/parsing exception
*/
public Map<String, Object> delete(String namespace, boolean cascading) throws IOException {
return makeCall(fetchUrl(namespace, null, null), objectMapper.writeValueAsString(fetchDeleteOptions(cascading, null)), HttpCallMethod.DELETE);
public boolean delete(String namespace, boolean cascading) throws IOException {
return handleDelete(namespace, null, objectMapper.writeValueAsString(fetchDeleteOptions(cascading, null)));
}

/**
Expand All @@ -447,23 +448,23 @@ public Map<String, Object> delete(String namespace, boolean cascading) throws IO
* @param namespace desired namespace
* @param deleteOptions object provided by Kubernetes API for more fine grained control over deletion.
* For more information please see https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.16/#deleteoptions-v1-meta
* @return deleted object as HashMap
* @return a boolean value whether item was deleted or item didn't exist in server
* @throws IOException in case of any network/object parse problems
*/
public Map<String, Object> delete(String namespace, DeleteOptions deleteOptions) throws IOException {
return makeCall(fetchUrl(namespace, null, null), objectMapper.writeValueAsString(deleteOptions), HttpCallMethod.DELETE);
public boolean delete(String namespace, DeleteOptions deleteOptions) throws IOException {
return handleDelete(namespace, null, objectMapper.writeValueAsString(deleteOptions));
}

/**
* Delete a custom resource in a specific namespace
*
* @param namespace desired namespace
* @param name custom resource's name
* @return object as HashMap
* @return a boolean value whether item was deleted or item didn't exist in server
* @throws IOException in case of any network/object parse problems
*/
public Map<String, Object> delete(String namespace, String name) throws IOException {
return makeCall(fetchUrl(namespace, name, null), objectMapper.writeValueAsString(fetchDeleteOptions(false, DeletionPropagation.BACKGROUND.toString())), HttpCallMethod.DELETE);
public boolean delete(String namespace, String name) throws IOException {
return handleDelete(namespace, name, objectMapper.writeValueAsString(fetchDeleteOptions(false, DeletionPropagation.BACKGROUND.toString())));
}

/**
Expand All @@ -473,11 +474,11 @@ public Map<String, Object> delete(String namespace, String name) throws IOExcept
* @param name required name of custom resource
* @param cascading whether dependent object need to be orphaned or not. If true/false, the "orphan"
* finalizer will be added to/removed from the object's finalizers list.
* @return deleted objects as HashMap
* @return a boolean value whether item was deleted or item didn't exist in server
* @throws IOException exception related to network/object parsing
*/
public Map<String, Object> delete(String namespace, String name, boolean cascading) throws IOException {
return makeCall(fetchUrl(namespace, name, null), objectMapper.writeValueAsString(fetchDeleteOptions(cascading, null)), HttpCallMethod.DELETE);
public boolean delete(String namespace, String name, boolean cascading) throws IOException {
return handleDelete(namespace, name, objectMapper.writeValueAsString(fetchDeleteOptions(cascading, null)));
}

/**
Expand All @@ -492,11 +493,11 @@ public Map<String, Object> delete(String namespace, String name, boolean cascadi
* 'Orphan' - orphan the dependents;
* 'Background' - allow the garbage collector to delete the dependents in the background;
* 'Foreground' - a cascading policy that deletes all dependents in the foreground.
* @return deleted object as HashMap
* @return a boolean value whether item was deleted or item didn't exist in server
* @throws IOException in case of network/object parse exception
*/
public Map<String, Object> delete(String namespace, String name, String propagationPolicy) throws IOException {
return makeCall(fetchUrl(namespace, name, null), objectMapper.writeValueAsString(fetchDeleteOptions(false, propagationPolicy)) , HttpCallMethod.DELETE);
public boolean delete(String namespace, String name, String propagationPolicy) throws IOException {
return handleDelete(namespace, name, objectMapper.writeValueAsString(fetchDeleteOptions(false, propagationPolicy)));
}

/**
Expand All @@ -506,11 +507,11 @@ public Map<String, Object> delete(String namespace, String name, String propagat
* @param name name of custom resource
* @param deleteOptions object provided by Kubernetes API for more fine grained control over deletion.
* For more information please see https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.16/#deleteoptions-v1-meta
* @return deleted object as HashMap
* @return a boolean value whether item was deleted or item didn't exist in server
* @throws IOException in case of any network/object parse exception
*/
public Map<String, Object> delete(String namespace, String name, DeleteOptions deleteOptions) throws IOException {
return makeCall(fetchUrl(namespace, name, null), objectMapper.writeValueAsString(deleteOptions), HttpCallMethod.DELETE);
public boolean delete(String namespace, String name, DeleteOptions deleteOptions) throws IOException {
return handleDelete(namespace, name, objectMapper.writeValueAsString(deleteOptions));
}

/**
Expand Down Expand Up @@ -760,6 +761,10 @@ private String getLabelsQueryParam(Map<String, String> labels) {
}

private Map<String, Object> makeCall(String url, String body, HttpCallMethod callMethod) {
return makeCall(url, body, callMethod, true);
}

private Map<String, Object> makeCall(String url, String body, HttpCallMethod callMethod, boolean shouldRequestFailure) {
Request request = (body == null) ? getRequest(url, callMethod) : getRequest(url, body, callMethod);
try (Response response = client.newCall(request).execute()) {
if (response.isSuccessful()) {
Expand All @@ -769,13 +774,31 @@ private Map<String, Object> makeCall(String url, String body, HttpCallMethod cal
else
return objectMapper.readValue(respBody, HashMap.class);
} else {
throw requestFailure(request, createStatus(response));
return handleFailure(request, response, shouldRequestFailure);
}
} catch(Exception e) {
throw KubernetesClientException.launderThrowable(e);
}
}

private Map<String, Object> handleFailure(Request request, Response response, boolean shouldRequestFailure) throws IOException {
if (shouldRequestFailure) {
throw requestFailure(request, createStatus(response));
}
return objectMapper.readValue(response.body().string(), HashMap.class);
}

private boolean handleDelete(String namespace, String name, String requestBody) {
Map<String, Object> response = makeCall(fetchUrl(namespace, name, null), requestBody, HttpCallMethod.DELETE, false);

// In most cases Status object is sent on deletion, but when deprecated DeleteOptions.orphanDependents
// is used; object which is being deleted is sent
if (response.get("kind").toString().equals("Status")) {
return response.get("status").toString().equals("Success");
}
return true;
}

private Map<String, Object> validateAndSubmitRequest(String namespace, String name, String objectAsString, HttpCallMethod httpCallMethod) throws IOException {
return validateAndSubmitRequest(fetchUrl(namespace, name, null), objectAsString, httpCallMethod);
}
Expand Down
165 changes: 156 additions & 9 deletions ...t/java/io/fabric8/kubernetes/client/dsl/internal/RawCustomResourceOperationsImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
package io.fabric8.kubernetes.client.dsl.internal;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.Mockito.*;

Expand All @@ -25,6 +27,8 @@
import java.util.HashMap;
import java.util.Map;

import io.fabric8.kubernetes.api.model.DeleteOptionsBuilder;
import io.fabric8.kubernetes.api.model.DeletionPropagation;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
Expand Down Expand Up @@ -109,14 +113,132 @@ void testCreateOrReplaceUrl() throws IOException {
assertEquals("POST", captor.getAllValues().get(3).method());
}

private Response mockResponse(int code) {
return new Response.Builder()
.request(new Request.Builder().url("http://mock").build())
.protocol(Protocol.HTTP_1_1)
.code(code)
.body(ResponseBody.create(MediaType.get("application/json"), ""))
.message("mock")
.build();
@Test
void testDeleteWithNamespaceAndNameForNonExistentResource() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_NOT_FOUND, "{\"kind\":\"Status\",\"status\":\"Failure\"}");
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);

// When
boolean result = rawCustomResourceOperations.delete("ns1", "foo");

// Then
assertFalse(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/ns1/hellos/foo", captor.getValue().url().encodedPath());
}

@Test
void testDeleteWithNamespaceForNonExistentResource() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_NOT_FOUND, "{\"kind\":\"Status\",\"status\":\"Failure\"}");
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);

// When
boolean result = rawCustomResourceOperations.delete("ns1");

// Then
assertFalse(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/ns1/hellos", captor.getValue().url().encodedPath());
}

@Test
void testDeleteWithNamespaceAndCascadingForNonExistentResource() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_NOT_FOUND, "{\"kind\":\"Status\",\"status\":\"Failure\"}");
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);

// When
boolean result = rawCustomResourceOperations.delete("ns1", true);

// Then
assertFalse(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/ns1/hellos", captor.getValue().url().encodedPath());
}

@Test
void testDeleteWithNamespaceAndDeleteOptionsForNonExistentResource() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_NOT_FOUND, "{\"kind\":\"Status\",\"status\":\"Failure\"}");
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);

// When
boolean result = rawCustomResourceOperations.delete("ns1", new DeleteOptionsBuilder().withOrphanDependents(true).build());

// Then
assertFalse(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/ns1/hellos", captor.getValue().url().encodedPath());
}

@Test
void testDeleteWithNamespaceNameAndCascading() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_OK, "{\"kind\":\"Hello\",\"metadata\":{\"name\":\"Failure\"}}");
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);

// When
boolean result = rawCustomResourceOperations.delete("ns1", "foo", true);

// Then
assertTrue(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/ns1/hellos/foo", captor.getValue().url().encodedPath());
}

@Test
void testDeleteWithNamespaceNameAndCascadingForNonExistentResource() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_NOT_FOUND, "{\"kind\":\"Status\",\"status\":\"Failure\"}");
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);

// When
boolean result = rawCustomResourceOperations.delete("ns1", "foo", true);

// Then
assertFalse(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/ns1/hellos/foo", captor.getValue().url().encodedPath());
}

@Test
void testDeleteWithNamespaceNameAndPropagationPolicyForNonExistentResource() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_NOT_FOUND, "{\"kind\":\"Status\",\"status\":\"Failure\"}");
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);

// When
boolean result = rawCustomResourceOperations.delete("ns1", "foo", DeletionPropagation.BACKGROUND.toString());

// Then
assertFalse(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/ns1/hellos/foo", captor.getValue().url().encodedPath());
}

@Test
void testDeleteWithNamespaceNameAndDeleteOptionsForNonExistentResource() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_NOT_FOUND, "{\"kind\":\"Status\",\"status\":\"Failure\"}");
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);

// When
boolean result = rawCustomResourceOperations.delete("ns1", "foo", new DeleteOptionsBuilder().withPropagationPolicy(DeletionPropagation.FOREGROUND.toString()).build());

// Then
assertFalse(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/ns1/hellos/foo", captor.getValue().url().encodedPath());
}

@Test
Expand All @@ -138,11 +260,13 @@ void testDeleteUrl() throws IOException {
// Given
RawCustomResourceOperationsImpl rawCustomResourceOperations = new RawCustomResourceOperationsImpl(mockClient, config, customResourceDefinitionContext);
ArgumentCaptor<Request> captor = ArgumentCaptor.forClass(Request.class);
mockDeletionCallWithResponse(HttpURLConnection.HTTP_OK, "{\"kind\":\"Status\",\"status\":\"Success\"}");

// When
rawCustomResourceOperations.delete("myns", "myresource");
boolean result = rawCustomResourceOperations.delete("myns", "myresource");

// Then
assertTrue(result);
verify(mockClient).newCall(captor.capture());
assertEquals("/apis/test.fabric8.io/v1alpha1/namespaces/myns/hellos/myresource", captor.getValue().url().encodedPath());
}
Expand Down Expand Up @@ -252,4 +376,27 @@ void testGetConfigShouldNotReturnNull() {
assertThat(configFromRawOp.getWatchReconnectInterval()).isEqualTo(10);
assertThat(configFromRawOp.getWatchReconnectLimit()).isEqualTo(1);
}

private void mockDeletionCallWithResponse(int code, String status) throws IOException {
Call mockCall = mock(Call.class);
Response mockNotFoundResponse = mockResponse(code, status);
when(mockCall.execute())
.thenReturn(mockNotFoundResponse);
when(mockClient.newCall(any())).thenReturn(mockCall);
}

private Response mockResponse(int code) {
return mockResponse(code, "");
}

private Response mockResponse(int code, String body) {
return new Response.Builder()
.request(new Request.Builder().url("http://mock").build())
.protocol(Protocol.HTTP_1_1)
.code(code)
.body(ResponseBody.create(MediaType.get("application/json"), body))
.message("mock")
.build();
}

}
Loading

0 comments on commit f45923e

Please sign in to comment.