HDDS-11539. OzoneClientCache @PreDestroy is never called

hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/S3GatewayService.java

@@ -42,8 +42,6 @@ public void start(OzoneConfiguration conf) throws Exception {
   public void stop() throws Exception {
     Preconditions.assertNotNull(s3g, "S3 Gateway not running");
     s3g.stop();
-    // TODO (HDDS-11539): Remove this workaround once the @PreDestroy issue is fixed
-    OzoneClientCache.closeClient();
   }
 
   @Override

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java

@@ -26,41 +26,63 @@
 import java.security.cert.CertificateException;
 import java.util.Collections;
 import java.util.List;
+import javax.annotation.PostConstruct;
 import javax.annotation.PreDestroy;
-import javax.enterprise.context.ApplicationScoped;
+import javax.inject.Inject;
+import javax.inject.Singleton;
 import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.hdds.security.SecurityConfig;
+import org.apache.hadoop.hdds.utils.IOUtils;
 import org.apache.hadoop.ozone.OmUtils;
 import org.apache.hadoop.ozone.OzoneSecurityUtil;
 import org.apache.hadoop.ozone.client.OzoneClient;
 import org.apache.hadoop.ozone.client.OzoneClientFactory;
 import org.apache.hadoop.ozone.om.helpers.ServiceInfoEx;
 import org.apache.hadoop.ozone.om.protocol.S3Auth;
 import org.apache.hadoop.ozone.om.protocolPB.GrpcOmTransport;
+import org.apache.ratis.util.Preconditions;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
  * Cached ozone client for s3 requests.
  */
-@ApplicationScoped
-public final class OzoneClientCache {
+@Singleton
+public class OzoneClientCache {
+
   private static final Logger LOG =
       LoggerFactory.getLogger(OzoneClientCache.class);
-  // single, cached OzoneClient established on first connection
-  // for s3g gRPC OmTransport, OmRequest - OmResponse channel
-  private static OzoneClientCache instance;
+
+  private final OzoneConfiguration conf;
   private OzoneClient client;
-  private SecurityConfig secConfig;
 
-  private OzoneClientCache(OzoneConfiguration ozoneConfiguration)
-      throws IOException {
+  @Inject
+  OzoneClientCache(OzoneConfiguration conf) {
+    this.conf = conf;
+    LOG.debug("{}: Created", this);
+  }
+
+  @PostConstruct
+  public void initialize() throws IOException {
+    conf.set("ozone.om.group.rights", "NONE");
     // Set the expected OM version if not set via config.
-    ozoneConfiguration.setIfUnset(OZONE_CLIENT_REQUIRED_OM_VERSION_MIN_KEY,
+    conf.setIfUnset(OZONE_CLIENT_REQUIRED_OM_VERSION_MIN_KEY,
         OZONE_CLIENT_REQUIRED_OM_VERSION_MIN_DEFAULT);
+    conf.setBoolean(S3Auth.S3_AUTH_CHECK, true);
+
+    client = createClient(conf);
+    Preconditions.assertTrue(conf.getBoolean(S3Auth.S3_AUTH_CHECK, false), S3Auth.S3_AUTH_CHECK);
+
+    LOG.debug("{}: Initialized", this);
+  }
+
+  public OzoneClient getClient() {
+    return client;
+  }
+
+  public static OzoneClient createClient(OzoneConfiguration ozoneConfiguration) throws IOException {
     String omServiceID = OmUtils.getOzoneManagerServiceId(ozoneConfiguration);
-    secConfig = new SecurityConfig(ozoneConfiguration);
-    client = null;
+    SecurityConfig secConfig = new SecurityConfig(ozoneConfiguration);
     try {
       if (secConfig.isGrpcTlsEnabled()) {
         if (ozoneConfiguration
@@ -70,43 +92,23 @@ private OzoneClientCache(OzoneConfiguration ozoneConfiguration)
           // Grpc transport selected
           // need to get certificate for TLS through
           // hadoop rpc first via ServiceInfo
-          setCertificate(omServiceID,
-              ozoneConfiguration);
+          setCertificate(secConfig, omServiceID, ozoneConfiguration);
         }
       }
       if (omServiceID == null) {
-        client = OzoneClientFactory.getRpcClient(ozoneConfiguration);
+        return OzoneClientFactory.getRpcClient(ozoneConfiguration);
       } else {
         // As in HA case, we need to pass om service ID.
-        client = OzoneClientFactory.getRpcClient(omServiceID,
+        return OzoneClientFactory.getRpcClient(omServiceID,
             ozoneConfiguration);
       }
     } catch (IOException e) {
       LOG.warn("cannot create OzoneClient", e);
       throw e;
     }
-    // S3 Gateway should always set the S3 Auth.
-    ozoneConfiguration.setBoolean(S3Auth.S3_AUTH_CHECK, true);
-  }
-
-  public static OzoneClient getOzoneClientInstance(OzoneConfiguration
-                                                      ozoneConfiguration)
-      throws IOException {
-    if (instance == null) {
-      instance = new OzoneClientCache(ozoneConfiguration);
-    }
-    return instance.client;
-  }
-
-  public static void closeClient() throws IOException {
-    if (instance != null) {
-      instance.client.close();
-      instance = null;
-    }
   }
 
-  private void setCertificate(String omServiceID,
-                              OzoneConfiguration conf)
+  private static void setCertificate(SecurityConfig secConfig, String omServiceID, OzoneConfiguration conf)
       throws IOException {
 
     // create local copy of config incase exception occurs
@@ -119,6 +121,7 @@ private void setCertificate(String omServiceID,
       // get certificates with service list request
       config.set(OZONE_OM_TRANSPORT_CLASS,
           OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+      config.setBoolean(S3Auth.S3_AUTH_CHECK, false);
 
       if (omServiceID == null) {
         certClient = OzoneClientFactory.getRpcClient(config);
@@ -161,7 +164,8 @@ private void setCertificate(String omServiceID,
   }
 
   @PreDestroy
-  public void destroy() throws IOException {
-    OzoneClientCache.closeClient();
+  public void cleanup() {
+    LOG.debug("{}: Closing cached client", this);
+    IOUtils.close(LOG, client);
   }
 }

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientProducer.java

@@ -22,10 +22,6 @@
 import javax.enterprise.context.RequestScoped;
 import javax.enterprise.inject.Produces;
 import javax.inject.Inject;
-import javax.ws.rs.WebApplicationException;
-import javax.ws.rs.container.ContainerRequestContext;
-import javax.ws.rs.core.Context;
-import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.ozone.client.OzoneClient;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -42,42 +38,17 @@ public class OzoneClientProducer {
   private OzoneClient client;
 
   @Inject
-  private OzoneConfiguration ozoneConfiguration;
-
-  @Context
-  private ContainerRequestContext context;
+  private OzoneClientCache clientCache;
 
   @Produces
-  public synchronized OzoneClient createClient() throws WebApplicationException,
-      IOException {
-    ozoneConfiguration.set("ozone.om.group.rights", "NONE");
-    client = getClient(ozoneConfiguration);
+  public OzoneClient createClient() {
+    client = clientCache.getClient();
     return client;
   }
 
   @PreDestroy
   public void destroy() throws IOException {
+    LOG.debug("{}: Clearing thread-local auth", this);
     client.getObjectStore().getClientProxy().clearThreadLocalS3Auth();
   }
-
-  private OzoneClient getClient(OzoneConfiguration config)
-      throws IOException {
-    OzoneClient ozoneClient = null;
-    try {
-      ozoneClient =
-          OzoneClientCache.getOzoneClientInstance(ozoneConfiguration);
-    } catch (Exception e) {
-      // For any other critical errors during object creation throw Internal
-      // error.
-      if (LOG.isDebugEnabled()) {
-        LOG.debug("Error during Client Creation: ", e);
-      }
-      throw e;
-    }
-    return ozoneClient;
-  }
-
-  public synchronized void setOzoneConfiguration(OzoneConfiguration config) {
-    this.ozoneConfiguration = config;
-  }
 }

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretEndpointBase.java

@@ -18,33 +18,49 @@
 package org.apache.hadoop.ozone.s3secret;
 
 import com.google.common.annotations.VisibleForTesting;
+import java.io.IOException;
 import java.util.Map;
+import javax.annotation.PostConstruct;
 import javax.inject.Inject;
 import javax.ws.rs.container.ContainerRequestContext;
 import javax.ws.rs.core.Context;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.ozone.audit.AuditAction;
 import org.apache.hadoop.ozone.audit.AuditEventStatus;
 import org.apache.hadoop.ozone.audit.AuditLogger;
 import org.apache.hadoop.ozone.audit.AuditLoggerType;
 import org.apache.hadoop.ozone.audit.AuditMessage;
 import org.apache.hadoop.ozone.audit.Auditor;
 import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.om.protocol.S3Auth;
+import org.apache.hadoop.ozone.s3.OzoneClientCache;
 import org.apache.hadoop.ozone.s3.util.AuditUtils;
 
 /**
  * Base implementation of endpoint for working with S3 secret.
  */
 public class S3SecretEndpointBase implements Auditor {
 
+  private final OzoneConfiguration conf;
+  private OzoneClient client;
+
   @Context
   private ContainerRequestContext context;
 
-  @Inject
-  private OzoneClient client;
-
   protected static final AuditLogger AUDIT =
       new AuditLogger(AuditLoggerType.S3GLOGGER);
 
+  @Inject
+  S3SecretEndpointBase(OzoneConfiguration conf) {
+    this.conf = new OzoneConfiguration(conf);
+    this.conf.setBoolean(S3Auth.S3_AUTH_CHECK, false);
+  }
+
+  @PostConstruct
+  void initialize() throws IOException {
+    client = OzoneClientCache.createClient(conf);
+  }
+
   protected String userNameFromRequest() {
     return context.getSecurityContext().getUserPrincipal().getName();
   }

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java

@@ -22,11 +22,13 @@
 
 import jakarta.annotation.Nullable;
 import java.io.IOException;
+import javax.inject.Inject;
 import javax.ws.rs.DELETE;
 import javax.ws.rs.PUT;
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
 import javax.ws.rs.core.Response;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.ozone.audit.S3GAction;
 import org.apache.hadoop.ozone.om.exceptions.OMException;
 import org.apache.hadoop.ozone.om.helpers.S3SecretValue;
@@ -43,6 +45,11 @@ public class S3SecretManagementEndpoint extends S3SecretEndpointBase {
   private static final Logger LOG =
       LoggerFactory.getLogger(S3SecretManagementEndpoint.class);
 
+  @Inject
+  S3SecretManagementEndpoint(OzoneConfiguration conf) {
+    super(conf);
+  }
+
   @PUT
   public Response generate() throws IOException {
     return generateInternal(null);

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/TestOzoneClientProducer.java → hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/TestOzoneClientCache.java

@@ -27,35 +27,28 @@
 import org.junit.jupiter.api.Test;
 
 /**
- * Test class for @{@link OzoneClientProducer}.
+ * Test class for @{@link OzoneClientCache}.
  */
-public class TestOzoneClientProducer {
+public class TestOzoneClientCache {
 
-  private OzoneClientProducer producer;
-
-  public TestOzoneClientProducer()
-      throws Exception {
-    producer = new OzoneClientProducer();
+  @Test
+  public void testGetClientFailure() {
     OzoneConfiguration config = new OzoneConfiguration();
     config.setBoolean(OzoneConfigKeys.OZONE_SECURITY_ENABLED_KEY, true);
     config.set(OMConfigKeys.OZONE_OM_ADDRESS_KEY, "");
-    producer.setOzoneConfiguration(config);
-  }
+    OzoneClientCache subject = new OzoneClientCache(config);
 
-  @Test
-  public void testGetClientFailure() {
-    assertThrows(IOException.class, () -> producer.createClient(),
-        "testGetClientFailure");
+    assertThrows(IOException.class, subject::initialize);
   }
 
   @Test
   public void testGetClientFailureWithMultipleServiceIds() {
     OzoneConfiguration configuration = new OzoneConfiguration();
     configuration.set(OMConfigKeys.OZONE_OM_SERVICE_IDS_KEY, "ozone1,ozone2");
-    producer.setOzoneConfiguration(configuration);
-    IOException testGetClientFailure = assertThrows(IOException.class, () ->
-        producer.createClient(), "testGetClientFailureWithMultipleServiceIds");
-    assertThat(testGetClientFailure.getMessage())
+    OzoneClientCache subject = new OzoneClientCache(configuration);
+
+    IOException e = assertThrows(IOException.class, subject::initialize);
+    assertThat(e.getMessage())
         .contains("More than 1 OzoneManager ServiceID");
   }
 
@@ -64,13 +57,12 @@ public void testGetClientFailureWithMultipleServiceIdsAndInternalServiceId() {
     OzoneConfiguration configuration = new OzoneConfiguration();
     configuration.set(OMConfigKeys.OZONE_OM_INTERNAL_SERVICE_ID, "ozone1");
     configuration.set(OMConfigKeys.OZONE_OM_SERVICE_IDS_KEY, "ozone1,ozone2");
-    producer.setOzoneConfiguration(configuration);
+    OzoneClientCache subject = new OzoneClientCache(configuration);
+
     // Still test will fail, as config is not complete. But it should pass
     // the service id check.
-    IOException testGetClientFailure = assertThrows(IOException.class, () ->
-        producer.createClient(),
-        "testGetClientFailureWithMultipleServiceIdsAndInternalServiceId");
-    assertThat(testGetClientFailure.getMessage())
+    IOException e = assertThrows(IOException.class, subject::initialize);
+    assertThat(e.getMessage())
         .doesNotContain("More than 1 OzoneManager ServiceID");
   }
 

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java

@@ -79,7 +79,7 @@ void setUp() {
     when(uriInfo.getQueryParameters()).thenReturn(new MultivaluedHashMap<>());
     when(context.getUriInfo()).thenReturn(uriInfo);
 
-    endpoint = new S3SecretManagementEndpoint();
+    endpoint = new S3SecretManagementEndpoint(conf);
     endpoint.setClient(client);
     endpoint.setContext(context);
   }

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java

@@ -37,6 +37,7 @@
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.SecurityContext;
 import javax.ws.rs.core.UriInfo;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.ozone.client.ObjectStoreStub;
 import org.apache.hadoop.ozone.client.OzoneClient;
 import org.apache.hadoop.ozone.client.OzoneClientStub;
@@ -70,13 +71,14 @@ public class TestSecretRevoke {
 
   @BeforeEach
   void setUp() {
+    OzoneConfiguration conf = new OzoneConfiguration();
     OzoneClient client = new OzoneClientStub(objectStore);
 
     when(uriInfo.getPathParameters()).thenReturn(new MultivaluedHashMap<>());
     when(uriInfo.getQueryParameters()).thenReturn(new MultivaluedHashMap<>());
     when(context.getUriInfo()).thenReturn(uriInfo);
 
-    endpoint = new S3SecretManagementEndpoint();
+    endpoint = new S3SecretManagementEndpoint(conf);
     endpoint.setClient(client);
     endpoint.setContext(context);
   }