fix (kubernetes-client-api) : Remove opinionated messages from Config…

…'s `errorMessages` and deprecate it (#5166) + Remove opinionated status code to message mappings from Config + Deprecate Config.errorMessages field in favor of kubernetes status + Update OperationSupport to only rely on Kubernetes status for error messages Signed-off-by: Rohan Kumar <[email protected]>
fabric8io · Jun 13, 2023 · 879954b · 879954b
1 parent 6997cc2
commit 879954b
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 51 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 * Fix #5224: Ensuring jetty sets the User-Agent header
 
 #### Improvements
+* Fix #5166: Remove opinionated messages from Config's `errorMessages` and deprecate it
 
 #### Dependency Upgrade
 

diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java
@@ -220,6 +220,10 @@ public class Config {
   private String userAgent = "fabric8-kubernetes-client/" + Version.clientVersion();
   private TlsVersion[] tlsVersions = new TlsVersion[] { TlsVersion.TLS_1_3, TlsVersion.TLS_1_2 };
 
+  /**
+   * @deprecated Use Kubernetes Status directly for extracting error messages.
+   */
+  @Deprecated
   private Map<Integer, String> errorMessages = new HashMap<>();
 
   /**
@@ -551,9 +555,6 @@ private static boolean tryServiceAccount(Config config) {
           String serviceTokenCandidate = new String(Files.readAllBytes(saTokenPathFile.toPath()));
           LOGGER.debug("Found service account token at: [{}].", saTokenPathLocation);
           config.setAutoOAuthToken(serviceTokenCandidate);
-          String txt = "Configured service account doesn't have access. Service account may have been revoked.";
-          config.getErrorMessages().put(401, "Unauthorized! " + txt);
-          config.getErrorMessages().put(403, "Forbidden!" + txt);
           return true;
         } catch (IOException e) {
           // No service account token available...
@@ -758,10 +759,6 @@ private static boolean loadFromKubeconfig(Config config, String context, String
               }
             }
           }
-
-          config.getErrorMessages().put(401, "Unauthorized! Token may have expired! Please log-in again.");
-          config.getErrorMessages().put(403,
-              "Forbidden! User " + (currentContext != null ? currentContext.getUser() : "") + " doesn't have permission.");
         }
         return true;
       }
@@ -1150,6 +1147,11 @@ public void setWatchReconnectLimit(int watchReconnectLimit) {
     this.requestConfig.setWatchReconnectLimit(watchReconnectLimit);
   }
 
+  /**
+   * @deprecated Use Kubernetes status messages directly
+   * @return map of error codes to message mappings
+   */
+  @Deprecated
   @JsonProperty("errorMessages")
   public Map<Integer, String> getErrorMessages() {
     return errorMessages;

diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java
@@ -645,6 +645,7 @@ private void assertConfig(Config config, boolean autoToken) {
 
     assertEquals(120, config.getMaxConcurrentRequests());
     assertEquals(20, config.getMaxConcurrentRequestsPerHost());
+    assertThat(config.getErrorMessages()).isEmpty();
   }
 
   @Test

diff --git a/...-client/src/test/java/io/fabric8/kubernetes/client/dsl/internal/OperationSupportTest.java b/...-client/src/test/java/io/fabric8/kubernetes/client/dsl/internal/OperationSupportTest.java
@@ -127,14 +127,30 @@ void assertResponseCodeClientErrorAndCustomMessage() throws Exception {
     // Given
     operationSupport.getConfig().getErrorMessages().put(400, "Custom message");
     final HttpRequest request = new StandardHttpRequest.Builder().method("GET", null, null).uri(new URI("https://example.com"))
-        .build();
+      .build();
     final HttpResponse<String> response = new TestHttpResponse<String>().withCode(400);
     // When
+    final KubernetesClientException result = assertThrows(KubernetesClientException.class,
+      () -> operationSupport.assertResponseCode(request, response));
+    // Then
+    assertThat(result)
+      .hasMessageContaining("Failure executing: GET at: https://example.com. Message: Custom message Bad Request.");
+  }
+
+  @Test
+  @DisplayName("assertResponse, with client error, should throw exception with server response body")
+  void assertResponseCodeClientErrorAndStatus() throws Exception {
+    // Given
+    final HttpRequest request = new StandardHttpRequest.Builder().method("GET", null, null).uri(new URI("https://example.com"))
+        .build();
+    final HttpResponse<String> response = new TestHttpResponse<String>().withCode(400)
+        .withBody("{\"kind\":\"Status\",\"apiVersion\":\"v1\",\"status\":\"Failure\",\"code\":400,\"message\":\"Invalid\"}");
+    // When
     final KubernetesClientException result = assertThrows(KubernetesClientException.class,
         () -> operationSupport.assertResponseCode(request, response));
     // Then
     assertThat(result)
-        .hasMessageContaining("Failure executing: GET at: https://example.com. Message: Custom message Bad Request.");
+        .hasMessageContaining("Failure executing: GET at: https://example.com. Message: Invalid. Received status: Status(");
   }
 
   @Test

diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ErrorMessageTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ErrorMessageTest.java
@@ -15,67 +15,71 @@
  */
 package io.fabric8.kubernetes.client.mock;
 
+import io.fabric8.kubernetes.api.model.Event;
 import io.fabric8.kubernetes.api.model.EventBuilder;
+import io.fabric8.kubernetes.api.model.EventList;
+import io.fabric8.kubernetes.api.model.Status;
 import io.fabric8.kubernetes.api.model.StatusBuilder;
 import io.fabric8.kubernetes.client.KubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClientException;
+import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation;
+import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient;
 import io.fabric8.kubernetes.client.server.mock.KubernetesMockServer;
-import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Test;
 
-import static org.hamcrest.CoreMatchers.not;
+import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
+import static java.net.HttpURLConnection.HTTP_FORBIDDEN;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
 
 @EnableKubernetesMockClient
-public class ErrorMessageTest {
+class ErrorMessageTest {
 
   KubernetesMockServer server;
   KubernetesClient client;
 
   @Test
-  public void testCustomMessage() {
-
-    client.getConfiguration().getErrorMessages().put(403, "MSG");
-    server.expect().withPath("/api/v1/namespaces/test/events")
-        .andReturn(200, new io.fabric8.kubernetes.api.model.EventListBuilder()
-            .addNewItem()
-            .withNewMetadata()
-            .withName("event1")
-            .endMetadata()
-            .endItem().build())
+  void whenCustomErrorMessageInConfig_thenErrorMessageShouldNotContainCustomMessage() {
+    // Given
+    String customMessage = "INVALID";
+    client.getConfiguration().getErrorMessages().put(403, customMessage);
+    server.expect().delete()
+        .withPath("/api/v1/namespaces/test/events")
+        .andReturn(HTTP_FORBIDDEN, new StatusBuilder()
+            .withCode(HTTP_FORBIDDEN)
+            .withMessage("forbidden")
+            .build())
         .once();
-    server.expect().withPath("/api/v1/namespaces/test/events/event1").andReturn(403, Boolean.FALSE).once();
-
-    try {
-      client.v1().events().inNamespace("test").delete();
-      fail();
-    } catch (Exception e) {
-      System.out.println("exception: " + e);
-      Assertions.assertThat(e.getMessage().startsWith("Failure executing: DELETE"));
-      Assertions.assertThat(e.getMessage().contains("Message: MSG"));
-      Assertions.assertThat(not(e.getMessage().contains("Received status")));
-    }
-  }
+    NonNamespaceOperation<Event, EventList, Resource<Event>> eventResource = client.v1().events().inNamespace("test");
 
-  private void fail() {
+    // When + Then
+    assertThatExceptionOfType(KubernetesClientException.class)
+        .isThrownBy(eventResource::delete)
+        .withMessageContaining(customMessage)
+        .hasFieldOrPropertyWithValue("code", HTTP_FORBIDDEN);
   }
 
   @Test
-  public void testServerErrorWithStatus() {
-
-    server.expect().withPath("/api/v1/namespaces/test/events").andReturn(500, new StatusBuilder()
-        .withMessage("This operation is not allowed for some reason")
-        .withReason("Some reason")
-        .withCode(500)
-        .build()).once();
+  void whenResponseBodyContainsKubernetesStatus_thenErrorMessageShouldContainStatusMessage() {
+    // Given
+    Status badRequestStatus = new StatusBuilder()
+        .withMessage("This operation invalid for some reason")
+        .withReason("Invalid")
+        .withCode(HTTP_BAD_REQUEST)
+        .build();
+    server.expect().post()
+        .withPath("/api/v1/namespaces/test/events")
+        .andReturn(HTTP_BAD_REQUEST, badRequestStatus)
+        .always();
+    Resource<Event> eventResource = client.v1().events().inNamespace("test")
+        .resource(new EventBuilder().withNewMetadata().withName("event1").endMetadata().build());
 
-    try {
-      client.v1().events().inNamespace("test")
-          .create(new EventBuilder().withNewMetadata().withName("event1").endMetadata().build());
-      fail();
-    } catch (Exception e) {
-      Assertions.assertThat(e.getMessage().startsWith("Failure executing: POST"));
-      Assertions.assertThat(e.getMessage().contains("Received status"));
-      Assertions.assertThat(not(e.getMessage().contains("Message: This operation")));
-    }
+    // When + Then
+    assertThatExceptionOfType(KubernetesClientException.class)
+        .isThrownBy(eventResource::create)
+        .withMessageContaining("Message: This operation invalid for some reason.")
+        .hasFieldOrPropertyWithValue("code", HTTP_BAD_REQUEST)
+        .extracting(KubernetesClientException::getStatus)
+        .isEqualTo(badRequestStatus);
   }
 }
-Original file line number
+Diff line change
@@ Expand Up @@
         assertEquals(120, config.getMaxConcurrentRequests());
         assertEquals(20, config.getMaxConcurrentRequestsPerHost());
+        assertThat(config.getErrorMessages()).isEmpty();
       }
       @Test
@@ Expand Down @@