From dd1c32cd262ff4f69f1287ae626efb29612822fd Mon Sep 17 00:00:00 2001 From: dahn Date: Mon, 14 Apr 2025 10:17:56 +0200 Subject: [PATCH] undo removal of accessLogger and deal with some warnings (#10567) * undo removval of accessLogger and deal with some warnings * Apply suggestions from code review * add accesslog to servlet --- .../main/java/com/cloud/api/ApiServer.java | 135 ++++++++---------- .../main/java/com/cloud/api/ApiServlet.java | 7 +- 2 files changed, 65 insertions(+), 77 deletions(-) diff --git a/server/src/main/java/com/cloud/api/ApiServer.java b/server/src/main/java/com/cloud/api/ApiServer.java index 7623351cca5..de875f67944 100644 --- a/server/src/main/java/com/cloud/api/ApiServer.java +++ b/server/src/main/java/com/cloud/api/ApiServer.java @@ -193,6 +193,7 @@ import static org.apache.cloudstack.user.UserPasswordResetManager.UserPasswordRe @Component public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiServerService, Configurable { + private static final Logger ACCESSLOGGER = LogManager.getLogger("apiserver." + ApiServer.class.getName()); private static final String SANITIZATION_REGEX = "[\n\r]"; @@ -235,9 +236,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer private EventDistributor eventDistributor = null; private static int s_workerCount = 0; - private static Map>> s_apiNameCmdClassMap = new HashMap>>(); + private static Map>> s_apiNameCmdClassMap = new HashMap<>(); - private static ExecutorService s_executor = new ThreadPoolExecutor(10, 150, 60, TimeUnit.SECONDS, new LinkedBlockingQueue(), new NamedThreadFactory( + private static ExecutorService s_executor = new ThreadPoolExecutor(10, 150, 60, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), new NamedThreadFactory( "ApiServer")); @Inject @@ -353,7 +354,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer String jobEvent = eventInfo.second(); if (logger.isTraceEnabled()) - logger.trace("Handle asyjob publish event " + jobEvent); + logger.trace("Handle asyjob publish event {}", jobEvent); if (eventDistributor == null) { setEventDistributor(ComponentContext.getComponent(EventDistributor.class)); } @@ -378,7 +379,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer cmdEventType = eventTypeObj; if (logger.isDebugEnabled()) - logger.debug("Retrieved cmdEventType from job info: " + cmdEventType); + logger.debug("Retrieved cmdEventType from job info: {}", cmdEventType); } else { if (logger.isDebugEnabled()) logger.debug("Unable to locate cmdEventType marker in job info. publish as unknown event"); @@ -422,11 +423,11 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer protected void setupIntegrationPortListener(Integer apiPort) { if (apiPort == null || apiPort <= 0) { - logger.trace(String.format("Skipping setting up listener for integration port as %s is set to %d", - IntegrationAPIPort.key(), apiPort)); + logger.trace("Skipping setting up listener for integration port as {} is set to {}", + IntegrationAPIPort.key(), apiPort); return; } - logger.debug(String.format("Setting up integration API service listener on port: %d", apiPort)); + logger.debug("Setting up integration API service listener on port: {}", apiPort); final ListenerThread listenerThread = new ListenerThread(this, apiPort); listenerThread.start(); } @@ -437,24 +438,24 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer Integer apiPort = IntegrationAPIPort.value(); // api port, null by default final Long snapshotLimit = ConcurrentSnapshotsThresholdPerHost.value(); - if (snapshotLimit == null || snapshotLimit.longValue() <= 0) { + if (snapshotLimit == null || snapshotLimit <= 0) { logger.debug("Global concurrent snapshot config parameter " + ConcurrentSnapshotsThresholdPerHost.value() + " is less or equal 0; defaulting to unlimited"); } else { dispatcher.setCreateSnapshotQueueSizeLimit(snapshotLimit); } final Long migrationLimit = VolumeApiService.ConcurrentMigrationsThresholdPerDatastore.value(); - if (migrationLimit == null || migrationLimit.longValue() <= 0) { + if (migrationLimit == null || migrationLimit <= 0) { logger.debug("Global concurrent migration config parameter " + VolumeApiService.ConcurrentMigrationsThresholdPerDatastore.value() + " is less or equal 0; defaulting to unlimited"); } else { dispatcher.setMigrateQueueSizeLimit(migrationLimit); } - final Set> cmdClasses = new HashSet>(); + final Set> cmdClasses = new HashSet<>(); for (final PluggableService pluggableService : pluggableServices) { cmdClasses.addAll(pluggableService.getCommands()); if (logger.isDebugEnabled()) { - logger.debug("Discovered plugin " + pluggableService.getClass().getSimpleName()); + logger.debug("Discovered plugin {}", pluggableService.getClass().getSimpleName()); } } @@ -467,7 +468,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer String apiName = at.name(); List> apiCmdList = s_apiNameCmdClassMap.get(apiName); if (apiCmdList == null) { - apiCmdList = new ArrayList>(); + apiCmdList = new ArrayList<>(); s_apiNameCmdClassMap.put(apiName, apiCmdList); } apiCmdList.add(cmdClass); @@ -570,14 +571,14 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer throw e; } } finally { - logger.info(sb.toString()); + ACCESSLOGGER.info(sb.toString()); CallContext.unregister(); } } @SuppressWarnings({"unchecked", "rawtypes"}) public void checkCharacterInkParams(final Map params) { - final Map stringMap = new HashMap(); + final Map stringMap = new HashMap<>(); final Set keys = params.keySet(); final Iterator keysIter = keys.iterator(); while (keysIter.hasNext()) { @@ -600,7 +601,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer public String handleRequest(final Map params, final String responseType, final StringBuilder auditTrailSb) throws ServerApiException { checkCharacterInkParams(params); - String response = null; + String response; String[] command = null; try { @@ -621,7 +622,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (authManager.getAPIAuthenticator(command[0]) != null) { return null; } - final Map paramMap = new HashMap(); + final Map paramMap = new HashMap<>(); final Set keys = params.keySet(); final Iterator keysIter = keys.iterator(); while (keysIter.hasNext()) { @@ -637,16 +638,16 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (cmdClass != null) { APICommand annotation = cmdClass.getAnnotation(APICommand.class); if (annotation == null) { - logger.error("No APICommand annotation found for class " + cmdClass.getCanonicalName()); + logger.error("No APICommand annotation found for class {}", cmdClass.getCanonicalName()); throw new CloudRuntimeException("No APICommand annotation found for class " + cmdClass.getCanonicalName()); } - BaseCmd cmdObj = (BaseCmd)cmdClass.newInstance(); + BaseCmd cmdObj = (BaseCmd)cmdClass.getDeclaredConstructor().newInstance(); cmdObj = ComponentContext.inject(cmdObj); cmdObj.configure(); cmdObj.setFullUrlParams(paramMap); cmdObj.setResponseType(responseType); - cmdObj.setHttpMethod(paramMap.get(ApiConstants.HTTPMETHOD).toString()); + cmdObj.setHttpMethod(paramMap.get(ApiConstants.HTTPMETHOD)); // This is where the command is either serialized, or directly dispatched StringBuilder log = new StringBuilder(); @@ -655,14 +656,11 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer } else { final String errorString = "Unknown API command: " + command[0]; logger.warn(errorString); - auditTrailSb.append(" " + errorString); + auditTrailSb.append(" ").append(errorString); throw new ServerApiException(ApiErrorCode.UNSUPPORTED_ACTION_ERROR, errorString); } } - } catch (final InvalidParameterValueException ex) { - logger.info(ex.getMessage()); - throw new ServerApiException(ApiErrorCode.PARAM_ERROR, ex.getMessage(), ex); - } catch (final IllegalArgumentException ex) { + } catch (final InvalidParameterValueException | IllegalArgumentException ex) { logger.info(ex.getMessage()); throw new ServerApiException(ApiErrorCode.PARAM_ERROR, ex.getMessage(), ex); } catch (final PermissionDeniedException ex) { @@ -675,9 +673,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer buf.append(obj.getUuid()); buf.append(" "); } - logger.info("PermissionDenied: " + ex.getMessage() + " on objs: [" + buf.toString() + "]"); + logger.info("PermissionDenied: " + ex.getMessage() + " on objs: [" + buf + "]"); } else { - logger.info("PermissionDenied: " + ex.getMessage()); + logger.info("PermissionDenied: {}", ex.getMessage()); } throw new ServerApiException(ApiErrorCode.ACCOUNT_ERROR, ex.getMessage(), ex); } catch (final AccountLimitException ex) { @@ -747,7 +745,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer // BaseAsyncCmd: cmd is processed and submitted as an AsyncJob, job related info is serialized and returned. if (cmdObj instanceof BaseAsyncCmd) { Long objectId = null; - String objectUuid = null; + String objectUuid; if (cmdObj instanceof BaseAsyncCreateCmd) { final BaseAsyncCreateCmd createCmd = (BaseAsyncCreateCmd)cmdObj; dispatcher.dispatchCreateCmd(createCmd, params); @@ -788,7 +786,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer } params.put("ctxStartEventId", String.valueOf(startEventId)); - params.put("cmdEventType", asyncCmd.getEventType().toString()); + params.put("cmdEventType", asyncCmd.getEventType()); params.put("ctxDetails", ApiGsonHelper.getBuilder().create().toJson(ctx.getContextParameters())); if (asyncCmd.getHttpMethod() != null) { params.put(ApiConstants.HTTPMETHOD, asyncCmd.getHttpMethod().toString()); @@ -851,9 +849,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @SuppressWarnings("unchecked") private void buildAsyncListResponse(final BaseListCmd command, final Account account) { - final List responses = ((ListResponse)command.getResponseObject()).getResponses(); - if (responses != null && responses.size() > 0) { - List jobs = null; + final List responses = ((ListResponse)command.getResponseObject()).getResponses(); + if (responses != null && !responses.isEmpty()) { + List jobs; // list all jobs for ROOT admin if (accountMgr.isRootAdmin(account.getId())) { @@ -862,11 +860,11 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer jobs = asyncMgr.findInstancePendingAsyncJobs(command.getApiResourceType().toString(), account.getId()); } - if (jobs.size() == 0) { + if (jobs.isEmpty()) { return; } - final Map objectJobMap = new HashMap(); + final Map objectJobMap = new HashMap<>(); for (final AsyncJob job : jobs) { if (job.getInstanceId() == null) { continue; @@ -903,7 +901,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (Boolean.TRUE.equals(apiKeyAccessEnabled)) { return true; } else { - logger.info("Api-Key access is disabled for the User " + user.toString()); + logger.info("Api-Key access is disabled for the User {}", user); return false; } } @@ -912,7 +910,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (Boolean.TRUE.equals(apiKeyAccessEnabled)) { return true; } else { - logger.info("Api-Key access is disabled for the Account " + account.toString()); + logger.info("Api-Key access is disabled for the Account {}", account); return false; } } @@ -929,7 +927,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer public boolean verifyRequest(final Map requestParameters, final Long userId, InetAddress remoteAddress) throws ServerApiException { try { String apiKey = null; - String secretKey = null; + String secretKey; String signature = null; String unsignedRequest = null; @@ -957,11 +955,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer // - build a request string with sorted params, make sure it's all lowercase // - sign the request, verify the signature is the same - final List parameterNames = new ArrayList(); - for (final Object paramNameObj : requestParameters.keySet()) { - parameterNames.add((String)paramNameObj); // put the name in a list that we'll sort later - } + // put the name in a list that we'll sort later + final List parameterNames = new ArrayList<>(requestParameters.keySet()); Collections.sort(parameterNames); @@ -997,7 +993,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer return false; // no signature, bad request } - Date expiresTS = null; + Date expiresTS; // FIXME: Hard coded signature, why not have an enum if ("3".equals(signatureVersion)) { // New signature authentication. Check for expire parameter and its validity @@ -1017,18 +1013,18 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (expiresTS.before(now)) { signature = signature.replaceAll(SANITIZATION_REGEX, "_"); apiKey = apiKey.replaceAll(SANITIZATION_REGEX, "_"); - logger.debug(String.format("Request expired -- ignoring ...sig [%s], apiKey [%s].", signature, apiKey)); + logger.debug("Request expired -- ignoring ...sig [{}], apiKey [{}].", signature, apiKey); return false; } } final TransactionLegacy txn = TransactionLegacy.open(TransactionLegacy.CLOUD_DB); txn.close(); - User user = null; + User user; // verify there is a user with this api key final Pair userAcctPair = accountMgr.findUserByApiKey(apiKey); if (userAcctPair == null) { - logger.debug("apiKey does not map to a valid user -- ignoring request, apiKey: " + apiKey); + logger.debug("apiKey does not map to a valid user -- ignoring request, apiKey: {}", apiKey); return false; } @@ -1069,7 +1065,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer if (!equalSig) { signature = signature.replaceAll(SANITIZATION_REGEX, "_"); - logger.info(String.format("User signature [%s] is not equaled to computed signature [%s].", signature, computedSignature)); + logger.info("User signature [{}] is not equaled to computed signature [{}].", signature, computedSignature); } else { CallContext.register(user, account); } @@ -1128,10 +1124,10 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer session.removeAttribute("domain_UUID"); } - final Enumeration attrNames = session.getAttributeNames(); + final Enumeration attrNames = session.getAttributeNames(); if (attrNames != null) { while (attrNames.hasMoreElements()) { - final String attrName = (String) attrNames.nextElement(); + final String attrName = attrNames.nextElement(); final Object attrObj = session.getAttribute(attrName); if (ApiConstants.USERNAME.equalsIgnoreCase(attrName)) { response.setUsername(attrObj.toString()); @@ -1202,7 +1198,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer final long longDate = date.getTime(); final float offsetInMs = (t.getOffset(longDate)); offsetInHrs = offsetInMs / (1000 * 60 * 60); - logger.info("Timezone offset from UTC is: " + offsetInHrs); + logger.info("Timezone offset from UTC is: {}", offsetInHrs); } final Account account = accountMgr.getAccount(userAcct.getAccountId()); @@ -1253,7 +1249,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer // (bug 5483) generate a session key that the user must submit on every request to prevent CSRF, add that // to the login response so that session-based authenticators know to send the key back final SecureRandom sesssionKeyRandom = new SecureRandom(); - final byte sessionKeyBytes[] = new byte[20]; + final byte[] sessionKeyBytes = new byte[20]; sesssionKeyRandom.nextBytes(sessionKeyBytes); final String sessionKey = Base64.encodeBase64URLSafeString(sessionKeyBytes); session.setAttribute(ApiConstants.SESSIONKEY, sessionKey); @@ -1266,7 +1262,6 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @Override public void logoutUser(final long userId) { accountMgr.logoutUser(userId); - return; } @Override @@ -1294,30 +1289,26 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer throw new CloudRuntimeException(errorMessage); } if (StringUtils.isBlank(userAccount.getEmail())) { - logger.error(String.format( - "Email is not set. username: %s account id: %d domain id: %d", - userAccount.getUsername(), userAccount.getAccountId(), userAccount.getDomainId())); + logger.error("Email is not set. username: {} account id: {} domain id: {}", + userAccount.getUsername(), userAccount.getAccountId(), userAccount.getDomainId()); throw new CloudRuntimeException("Email is not set for the user."); } if (!EnumUtils.getEnumIgnoreCase(Account.State.class, userAccount.getState()).equals(Account.State.ENABLED)) { - logger.error(String.format( - "User is not enabled. username: %s account id: %d domain id: %s", - userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid())); + logger.error("User is not enabled. username: {} account id: {} domain id: {}", + userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid()); throw new CloudRuntimeException("User is not enabled."); } if (!EnumUtils.getEnumIgnoreCase(Account.State.class, userAccount.getAccountState()).equals(Account.State.ENABLED)) { - logger.error(String.format( - "Account is not enabled. username: %s account id: %d domain id: %s", - userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid())); + logger.error("Account is not enabled. username: {} account id: {} domain id: {}", + userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid()); throw new CloudRuntimeException("Account is not enabled."); } if (!domain.getState().equals(Domain.State.Active)) { - logger.error(String.format( - "Domain is not active. username: %s account id: %d domain id: %s", - userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid())); + logger.error("Domain is not active. username: {} account id: {} domain id: {}", + userAccount.getUsername(), userAccount.getAccountId(), domain.getUuid()); throw new CloudRuntimeException("Domain is not active."); } @@ -1425,7 +1416,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer // code to be very specific to our needs static class ListenerThread extends Thread { - private static Logger LOGGER = LogManager.getLogger(ListenerThread.class); + private static final Logger LOGGER = LogManager.getLogger(ListenerThread.class); private HttpService _httpService = null; private ServerSocket _serverSocket = null; private HttpParams _params = null; @@ -1464,7 +1455,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @Override public void run() { - LOGGER.info("ApiServer listening on port " + _serverSocket.getLocalPort()); + LOGGER.info("ApiServer listening on port {}", _serverSocket.getLocalPort()); while (!Thread.interrupted()) { try { // Set up HTTP connection @@ -1507,10 +1498,10 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer } } catch (final IOException ex) { if (logger.isTraceEnabled()) { - logger.trace("ApiServer: IOException - " + ex); + logger.trace("ApiServer: IOException - {}", ex.toString()); } } catch (final HttpException ex) { - logger.warn("ApiServer: Unrecoverable HTTP protocol violation" + ex); + logger.warn("ApiServer: Unrecoverable HTTP protocol violation {}", ex.toString()); } finally { try { _conn.shutdown(); @@ -1523,7 +1514,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @Override public String getSerializedApiError(final int errorCode, final String errorText, final Map apiCommandParams, final String responseType) { String responseName = null; - Class cmdClass = null; + Class cmdClass; String responseText = null; try { @@ -1536,7 +1527,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer final String cmdName = ((String[])cmdObj)[0]; cmdClass = getCmdClass(cmdName); if (cmdClass != null) { - responseName = ((BaseCmd)cmdClass.newInstance()).getCommandName(); + responseName = ((BaseCmd)cmdClass.getDeclaredConstructor().newInstance()).getCommandName(); } else { responseName = "errorresponse"; } @@ -1558,7 +1549,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer @Override public String getSerializedApiError(final ServerApiException ex, final Map apiCommandParams, final String responseType) { String responseName = null; - Class cmdClass = null; + Class cmdClass; String responseText = null; if (ex == null) { @@ -1576,7 +1567,7 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer final String cmdName = ((String[])cmdObj)[0]; cmdClass = getCmdClass(cmdName); if (cmdClass != null) { - responseName = ((BaseCmd)cmdClass.newInstance()).getCommandName(); + responseName = ((BaseCmd)cmdClass.getDeclaredConstructor().newInstance()).getCommandName(); } else { responseName = "errorresponse"; } @@ -1588,8 +1579,8 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer apiResponse.setResponseName(responseName); final ArrayList idList = ex.getIdProxyList(); if (idList != null) { - for (int i = 0; i < idList.size(); i++) { - apiResponse.addProxyObject(idList.get(i)); + for (ExceptionProxyObject exceptionProxyObject : idList) { + apiResponse.addProxyObject(exceptionProxyObject); } } // Also copy over the cserror code and the function/layer in which diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index e2ff411f8f4..4994c42bb4d 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -21,7 +21,6 @@ import java.net.InetAddress; import java.net.URLDecoder; import java.net.UnknownHostException; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -76,9 +75,7 @@ import com.cloud.utils.net.NetUtils; @Component("apiServlet") public class ApiServlet extends HttpServlet { protected static Logger LOGGER = LogManager.getLogger(ApiServlet.class); - private final static List s_clientAddressHeaders = Collections - .unmodifiableList(Arrays.asList("X-Forwarded-For", - "HTTP_CLIENT_IP", "HTTP_X_FORWARDED_FOR", "Remote_Addr")); + private static final Logger ACCESSLOGGER = LogManager.getLogger("apiserver." + ApiServlet.class.getName()); private static final String REPLACEMENT = "_"; private static final String LOGGER_REPLACEMENTS = "[\n\r\t]"; @@ -374,7 +371,7 @@ public class ApiServlet extends HttpServlet { LOGGER.error("unknown exception writing api response", ex); auditTrailSb.append(" unknown exception writing api response"); } finally { - LOGGER.info(auditTrailSb.toString()); + ACCESSLOGGER.info(auditTrailSb.toString()); if (LOGGER.isDebugEnabled()) { LOGGER.debug("===END=== " + reqStr); }