geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jinmeil...@apache.org
Subject [geode] branch develop updated: GEODE-6443: log all request and response in geode management service (#3373)
Date Fri, 29 Mar 2019 22:17:36 GMT
This is an automated email from the ASF dual-hosted git repository.

jinmeiliao pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new 458e39b  GEODE-6443: log all request and response in geode management service (#3373)
458e39b is described below

commit 458e39b8ecaba4ed52f873f33e9c30df60d67c5d
Author: jinmeiliao <jiliao@pivotal.io>
AuthorDate: Fri Mar 29 15:17:26 2019 -0700

    GEODE-6443: log all request and response in geode management service (#3373)
---
 .../rest/ManagementRequestLoggingDUnitTest.java    | 33 +++++-----
 .../ClusterManagementRestLoggingTest.java          | 45 ++++++++++++++
 .../cache/configuration/RegionConfigTest.java      |  6 --
 .../geode/cache/configuration/RegionConfig.java    |  2 +-
 .../internal/ClientClusterManagementService.java   |  4 ++
 .../internal/rest/ManagementLoggingFilter.java     | 70 +++++++++++++++++-----
 6 files changed, 121 insertions(+), 39 deletions(-)

diff --git a/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/ManagementRequestLoggingDUnitTest.java
b/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/ManagementRequestLoggingDUnitTest.java
index 68f4b6f..02d4685 100644
--- a/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/ManagementRequestLoggingDUnitTest.java
+++ b/geode-assembly/src/distributedTest/java/org/apache/geode/management/internal/rest/ManagementRequestLoggingDUnitTest.java
@@ -20,7 +20,6 @@ import static org.assertj.core.api.Assertions.assertThat;
 import java.util.List;
 import java.util.Map;
 
-import com.fasterxml.jackson.databind.ObjectMapper;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.core.Appender;
 import org.apache.logging.log4j.core.LogEvent;
@@ -33,45 +32,39 @@ import org.junit.Test;
 import org.apache.geode.cache.RegionShortcut;
 import org.apache.geode.cache.configuration.RegionConfig;
 import org.apache.geode.management.api.ClusterManagementResult;
+import org.apache.geode.management.api.ClusterManagementService;
+import org.apache.geode.management.client.ClusterManagementServiceProvider;
 import org.apache.geode.test.dunit.rules.ClusterStartupRule;
 import org.apache.geode.test.dunit.rules.MemberVM;
-import org.apache.geode.test.junit.rules.GeodeDevRestClient;
 
 public class ManagementRequestLoggingDUnitTest {
 
   @ClassRule
   public static ClusterStartupRule cluster = new ClusterStartupRule();
-
   private static MemberVM locator, server;
-
-  private static GeodeDevRestClient restClient;
+  private static ClusterManagementService service;
 
   @BeforeClass
   public static void beforeClass() {
     locator = cluster.startLocatorVM(0, l -> l.withHttpService());
     server = cluster.startServerVM(1, locator.getPort());
-    restClient =
-        new GeodeDevRestClient("/geode-management/v2", "localhost", locator.getHttpPort(),
false);
+    service = ClusterManagementServiceProvider.getService("localhost", locator.getHttpPort());
   }
 
   @Test
   public void checkRequestsAreLogged() throws Exception {
     locator.invoke(() -> {
       Logger logger = (Logger) LogManager.getLogger(ManagementLoggingFilter.class);
-      logger.addAppender(new ListAppender("ListAppender"));
+      ListAppender listAppender = new ListAppender("ListAppender");
+      logger.addAppender(listAppender);
+      listAppender.start();
     });
 
     RegionConfig regionConfig = new RegionConfig();
     regionConfig.setName("customers");
     regionConfig.setType(RegionShortcut.REPLICATE);
-    ObjectMapper mapper = new ObjectMapper();
-    String json = mapper.writeValueAsString(regionConfig);
-
-    ClusterManagementResult result =
-        restClient.doPostAndAssert("/regions", json)
-            .hasStatusCode(201)
-            .getClusterManagementResult();
 
+    ClusterManagementResult result = service.create(regionConfig);
     assertThat(result.isSuccessful()).isTrue();
 
     locator.invoke(() -> {
@@ -80,9 +73,13 @@ public class ManagementRequestLoggingDUnitTest {
       ListAppender listAppender = (ListAppender) appenders.get("ListAppender");
       List<LogEvent> logEvents = listAppender.getEvents();
 
-      assertThat(logEvents.size()).as("Expected LogEvents").isEqualTo(1);
-      assertThat(logEvents.get(0).getMessage().getFormattedMessage())
-          .startsWith("Management request:");
+      assertThat(logEvents.size()).as("Expected LogEvents").isEqualTo(2);
+      String beforeMessage = logEvents.get(0).getMessage().getFormattedMessage();
+      assertThat(beforeMessage).startsWith("Management Request: POST");
+      assertThat(beforeMessage).contains("user=").contains("payload={")
+          .contains("regionAttributes");
+      assertThat(logEvents.get(1).getMessage().getFormattedMessage())
+          .startsWith("Management Response: ").contains("response={").contains("Status=");
 
       logger.removeAppender(listAppender);
     });
diff --git a/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/rest/controllers/ClusterManagementRestLoggingTest.java
b/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/rest/controllers/ClusterManagementRestLoggingTest.java
new file mode 100644
index 0000000..c549e0d
--- /dev/null
+++ b/geode-assembly/src/integrationTest/java/org/apache/geode/management/internal/rest/controllers/ClusterManagementRestLoggingTest.java
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information
regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version
2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain
a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under
the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express
+ * or implied. See the License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.rest.controllers;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import org.apache.geode.management.api.ClusterManagementService;
+import org.apache.geode.management.internal.ClientClusterManagementService;
+import org.apache.geode.test.junit.rules.LocatorStarterRule;
+
+public class ClusterManagementRestLoggingTest {
+  @ClassRule
+  public static LocatorStarterRule locator = new LocatorStarterRule().withHttpService()
+      .withSystemProperty("geode.management.request.logging", "false").withAutoStart();
+
+  private static ClusterManagementService cms;
+
+  @BeforeClass
+  public static void beforeClass() throws Exception {
+    cms = new ClientClusterManagementService("localhost", locator.getHttpPort());
+  }
+
+  @Test
+  public void ping() throws Exception {
+    assertThat(cms.isConnected()).isTrue();
+  }
+
+}
diff --git a/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java
b/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java
index d78c97f..fa7e298 100644
--- a/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java
+++ b/geode-core/src/test/java/org/apache/geode/cache/configuration/RegionConfigTest.java
@@ -46,12 +46,6 @@ public class RegionConfigTest {
   }
 
   @Test
-  public void regionNameCannotBeNull() {
-    assertThatThrownBy(() -> regionConfig.setName(null))
-        .isInstanceOf(IllegalArgumentException.class);
-  }
-
-  @Test
   public void regionNameSwallowsSlash() {
     regionConfig.setName("/regionA");
     assertThat(regionConfig.getName()).isEqualTo("regionA");
diff --git a/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java
b/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java
index 67df3ca..fb6e831 100644
--- a/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java
+++ b/geode-management/src/main/java/org/apache/geode/cache/configuration/RegionConfig.java
@@ -324,7 +324,7 @@ public class RegionConfig implements CacheElement, RestfulEndpoint {
    */
   public void setName(String value) throws IllegalArgumentException {
     if (value == null) {
-      throw new IllegalArgumentException("Region name cannot be null");
+      return;
     }
 
     boolean regionPrefixedWithSlash = value.startsWith("/");
diff --git a/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java
b/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java
index baa5db3..f3d7cff 100644
--- a/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java
+++ b/geode-management/src/main/java/org/apache/geode/management/internal/ClientClusterManagementService.java
@@ -65,6 +65,10 @@ public class ClientClusterManagementService implements ClusterManagementService
     restTemplate.setErrorHandler(DEFAULT_ERROR_HANDLER);
   }
 
+  public ClientClusterManagementService(String host, int port) {
+    this(host, port, null, null, null, null);
+  }
+
   public ClientClusterManagementService(String host, int port, SSLContext sslContext,
       HostnameVerifier hostnameVerifier, String username, String password) {
     this();
diff --git a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/ManagementLoggingFilter.java
b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/ManagementLoggingFilter.java
index d827266..90bd556 100644
--- a/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/ManagementLoggingFilter.java
+++ b/geode-web-management/src/main/java/org/apache/geode/management/internal/rest/ManagementLoggingFilter.java
@@ -15,32 +15,74 @@
 
 package org.apache.geode.management.internal.rest;
 
+import java.io.IOException;
+import java.io.UnsupportedEncodingException;
+
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
 
-import org.springframework.web.filter.AbstractRequestLoggingFilter;
+import org.springframework.web.filter.OncePerRequestFilter;
+import org.springframework.web.util.ContentCachingRequestWrapper;
+import org.springframework.web.util.ContentCachingResponseWrapper;
 
-public class ManagementLoggingFilter extends AbstractRequestLoggingFilter {
+public class ManagementLoggingFilter extends OncePerRequestFilter {
 
   // Because someone is going to want to disable this.
   private static final Boolean ENABLE_REQUEST_LOGGING =
       Boolean.parseBoolean(System.getProperty("geode.management.request.logging", "true"));
 
-  public ManagementLoggingFilter() {
-    super.setIncludeQueryString(true);
-    super.setIncludePayload(true);
-    super.setMaxPayloadLength(1000);
-    super.setAfterMessagePrefix("Management request: [");
-  }
+  private static int MAX_PAYLOAD_LENGTH = 10000;
 
   @Override
-  protected void beforeRequest(HttpServletRequest request, String message) {
-    // No logging here - this would not display the payload
+  protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response,
+      FilterChain filterChain) throws ServletException, IOException {
+
+    if (!ENABLE_REQUEST_LOGGING) {
+      filterChain.doFilter(request, response);
+      return;
+    }
+
+    // We can not log request payload before making the actual request because then the InputStream
+    // would be consumed and cannot be read again by the actual processing/server.
+    ContentCachingRequestWrapper wrappedRequest = new ContentCachingRequestWrapper(request);
+    ContentCachingResponseWrapper wrappedResponse = new ContentCachingResponseWrapper(response);
+
+    // performs the actual request before logging
+    filterChain.doFilter(wrappedRequest, wrappedResponse);
+
+    // log after the request has been made and ContentCachingRequestWrapper has cached the
request
+    // payload.
+    String requestPattern = "Management Request: %s[url=%s]; user=%s; payload=%s";
+    String requestUrl = request.getRequestURI();
+    if (request.getQueryString() != null) {
+      requestUrl = requestUrl + "?" + request.getQueryString();
+    }
+    String payload = getContentAsString(wrappedRequest.getContentAsByteArray(),
+        wrappedRequest.getCharacterEncoding());
+    logger.info(String.format(requestPattern, request.getMethod(), requestUrl,
+        request.getRemoteUser(), payload));
+
+    // construct the response message
+    String responsePattern = "Management Response: Status=%s; response=%s";
+    payload = getContentAsString(wrappedResponse.getContentAsByteArray(),
+        wrappedResponse.getCharacterEncoding());
+    payload = payload.replaceAll(System.lineSeparator(), "");
+    logger.info(String.format(responsePattern, response.getStatus(), payload));
+
+    // IMPORTANT: copy content of response back into original response
+    wrappedResponse.copyBodyToResponse();
   }
 
-  @Override
-  protected void afterRequest(HttpServletRequest request, String message) {
-    if (ENABLE_REQUEST_LOGGING) {
-      logger.info(message);
+  private String getContentAsString(byte[] buf, String encoding) {
+    if (buf == null || buf.length == 0)
+      return "";
+    int length = Math.min(buf.length, MAX_PAYLOAD_LENGTH);
+    try {
+      return new String(buf, 0, length, encoding);
+    } catch (UnsupportedEncodingException ex) {
+      return "[unknown]";
     }
   }
 }


Mime
View raw message