neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r833600467
##########
File path:
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient
getOzoneClientInstance(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)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
Thanks @hanishakoneru for following up on this null check. Yes, we were
discussing the case where `caCertPems` is null and if it would be guaranteed to
have at least one of `caCertPems` or `caCertPem` to be non null. We came up
with that we can't make the assumption so to handle that case here ourselves.
In this case with the Precondition check, it is after we determine that
`caCertPems` is empty or null (`caCertPem` can't be null here). In this case
we need the `caCertPem` to be non-null or else it causes a NPE in the following
`OzoneSecurityUtil.convertToX509 `call. I put in the precondition check to
catch this null violation to intentionally throw an exception so the caller
knows that in this case the `caCertPem` must be non null.
What do think if we,
i.) update the code to check the precondition that `caCertPem` is not null
in the case that `caCertPems` is either empty or null with a message such as,
`Preconditions.checkNotNull(caCertPem, "caCertPem can not be null when
caCertPems is empty or null");`
_OR_,
ii.) log the error and throw an exception.
However if we just log the error (`caCertPem` is null) and continue,
unfortunately the call that follows to `OzoneSecurityUtil.convertToX509` will
throw a NPE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
