From 53700809ed38e0332c14bbd9f4ed2d2398c38348 Mon Sep 17 00:00:00 2001 From: Pearl Dsilva Date: Sun, 13 Apr 2025 23:25:59 -0400 Subject: [PATCH 01/14] fix conflict - logger --- .../src/main/java/com/cloud/cluster/ClusterManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java index e6ea0d8be32..b8102c63654 100644 --- a/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java +++ b/framework/cluster/src/main/java/com/cloud/cluster/ClusterManagerImpl.java @@ -1110,7 +1110,7 @@ public class ClusterManagerImpl extends ManagerBase implements ClusterManager, C _mshostDao.update(_mshostId, mshost); mshostStatusDao.update(mshostStatus.getId(), mshostStatus); } else { - s_logger.warn(String.format("Found a management server host [%s] without a status. This should never happen!", mshost)); + logger.warn(String.format("Found a management server host [%s] without a status. This should never happen!", mshost)); mshostStatus = new ManagementServerStatusVO(); mshostStatus.setMsId(mshost.getUuid()); mshostStatus.setLastSystemBoot(new Date()); From dd1c32cd262ff4f69f1287ae626efb29612822fd Mon Sep 17 00:00:00 2001 From: dahn Date: Mon, 14 Apr 2025 10:17:56 +0200 Subject: [PATCH 02/14] 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); } From 99ea77dc83901fd88db4e1fcb2f7623bd343b5b1 Mon Sep 17 00:00:00 2001 From: dahn Date: Mon, 14 Apr 2025 10:24:35 +0200 Subject: [PATCH 03/14] Usage server: remove logging of prameters including secret keys (#10649) --- usage/src/main/java/com/cloud/usage/UsageManagerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java b/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java index 6125f6f604d..63624cdc3c0 100644 --- a/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java +++ b/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java @@ -232,7 +232,6 @@ public class UsageManagerImpl extends ManagerBase implements UsageManager, Runna if (params != null) { mergeConfigs(configs, params); - s_logger.info("configs = " + configs); } } catch (CloudRuntimeException e) { s_logger.error("Unhandled configuration exception: " + e.getMessage()); From f13cf597a2e00054033ab5851db628680a64f8dc Mon Sep 17 00:00:00 2001 From: Rene Glover Date: Mon, 14 Apr 2025 05:59:43 -0500 Subject: [PATCH 04/14] 4.19 fix saml account selector (#10311) --- .../command/ListAndSwitchSAMLAccountCmd.java | 15 ++- .../SAML2LoginAPIAuthenticatorCmd.java | 2 +- .../cloudstack/saml/SAML2AuthManager.java | 4 + .../cloudstack/saml/SAML2AuthManagerImpl.java | 2 +- .../org/apache/cloudstack/saml/SAMLUtils.java | 94 +++++++++++-------- .../org/apache/cloudstack/SAMLUtilsTest.java | 6 +- .../ListAndSwitchSAMLAccountCmdTest.java | 2 - .../main/java/com/cloud/api/ApiServer.java | 9 +- .../com/cloud/user/AccountManagerImpl.java | 64 ++++++++++--- .../cloud/user/AccountManagerImplTest.java | 71 ++++++++++++++ .../components/header/SamlDomainSwitcher.vue | 3 + ui/src/store/modules/user.js | 4 +- ui/vue.config.js | 2 +- 13 files changed, 214 insertions(+), 64 deletions(-) diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java index c2f81cd3356..040d5414f26 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java @@ -133,10 +133,12 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic } if (userUuid != null && domainUuid != null) { + s_logger.debug("User [" + currentUserAccount.getUsername() + "] is requesting to switch from user profile [" + currentUserAccount.getId() + "] to useraccount [" + userUuid + "] in domain [" + domainUuid + "]"); final User user = _userDao.findByUuid(userUuid); final Domain domain = _domainDao.findByUuid(domainUuid); final UserAccount nextUserAccount = _accountService.getUserAccountById(user.getId()); if (nextUserAccount != null && !nextUserAccount.getAccountState().equals(Account.State.ENABLED.toString())) { + s_logger.warn("User [" + currentUserAccount.getUsername() + "] is requesting to switch from user profile [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] but the associated target account [" + nextUserAccount.getAccountName() + "] is not enabled"); throw new ServerApiException(ApiErrorCode.ACCOUNT_ERROR, _apiServer.getSerializedApiError(ApiErrorCode.PARAM_ERROR.getHttpCode(), "The requested user account is locked and cannot be switched to, please contact your administrator.", params, responseType)); @@ -147,20 +149,26 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic || !nextUserAccount.getExternalEntity().equals(currentUserAccount.getExternalEntity()) || (nextUserAccount.getDomainId() != domain.getId()) || (nextUserAccount.getSource() != User.Source.SAML2)) { + s_logger.warn("User [" + currentUserAccount.getUsername() + "] is requesting to switch from user profile [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] but the associated target account is not found or invalid"); throw new ServerApiException(ApiErrorCode.PARAM_ERROR, _apiServer.getSerializedApiError(ApiErrorCode.PARAM_ERROR.getHttpCode(), "User account is not allowed to switch to the requested account", params, responseType)); } try { if (_apiServer.verifyUser(nextUserAccount.getId())) { + s_logger.info("User [" + currentUserAccount.getUsername() + "] user profile switch is accepted: from [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] with account [" + nextUserAccount.getAccountName() + "]"); + // need to set a sessoin variable to inform the login function of the specific user to login as, rather than using email only (which could have multiple matches) + session.setAttribute("nextUserId", user.getId()); final LoginCmdResponse loginResponse = (LoginCmdResponse) _apiServer.loginUser(session, nextUserAccount.getUsername(), nextUserAccount.getUsername() + nextUserAccount.getSource().toString(), nextUserAccount.getDomainId(), null, remoteAddress, params); SAMLUtils.setupSamlUserCookies(loginResponse, resp); - resp.sendRedirect(SAML2AuthManager.SAMLCloudStackRedirectionUrl.value()); + session.removeAttribute("nextUserId"); + s_logger.debug("User [" + currentUserAccount.getUsername() + "] user profile switch cookies set: from [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] with account [" + nextUserAccount.getAccountName() + "]"); + //resp.sendRedirect(SAML2AuthManager.SAMLCloudStackRedirectionUrl.value()); return ApiResponseSerializer.toSerializedString(loginResponse, responseType); } } catch (CloudAuthenticationException | IOException exception) { - s_logger.debug("Failed to switch to request SAML user account due to: " + exception.getMessage()); + s_logger.debug("User [" + currentUserAccount.getUsername() + "] user profile switch cookies set FAILED: from [" + currentUserId + "] to user profile [" + userUuid + "] in domain [" + domainUuid + "] with account [" + nextUserAccount.getAccountName() + "]", exception); } } else { List switchableAccounts = _userAccountDao.getAllUsersByNameAndEntity(currentUserAccount.getUsername(), currentUserAccount.getExternalEntity()); @@ -178,6 +186,9 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic accountResponse.setAccountName(userAccount.getAccountName()); accountResponse.setIdpId(user.getExternalEntity()); accountResponses.add(accountResponse); + if (s_logger.isDebugEnabled()) { + s_logger.debug("Returning available useraccount for [" + currentUserAccount.getUsername() + "]: UserUUID: [" + user.getUuid() + "], DomainUUID: [" + domain.getUuid() + "], Account: [" + userAccount.getAccountName() + "]"); + } } ListResponse response = new ListResponse(); response.setResponses(accountResponses); diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java index 332e0602784..0f25123ff88 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java @@ -192,7 +192,7 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent String authnId = SAMLUtils.generateSecureRandomId(); samlAuthManager.saveToken(authnId, domainPath, idpMetadata.getEntityId()); s_logger.debug("Sending SAMLRequest id=" + authnId); - String redirectUrl = SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value()); + String redirectUrl = SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value(), SAML2AuthManager.SAMLRequirePasswordLogin.value()); resp.sendRedirect(redirectUrl); return ""; } if (params.containsKey("SAMLart")) { diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java index 4e8ba16c739..523f694d80b 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java @@ -79,6 +79,10 @@ public interface SAML2AuthManager extends PluggableAPIAuthenticator, PluggableSe ConfigKey SAMLUserSessionKeyPathAttribute = new ConfigKey("Advanced", String.class, "saml2.user.sessionkey.path", "", "The Path attribute of sessionkey cookie when SAML users have logged in. If not set, it will be set to the path of SAML redirection URL (saml2.redirect.url).", true); + ConfigKey SAMLRequirePasswordLogin = new ConfigKey("Advanced", Boolean.class, "saml2.require.password", "true", + "When enabled SAML2 will validate that the SAML login was performed with a password. If disabled, other forms of authentication are allowed (two-factor, certificate, etc) on the SAML Authentication Provider", true); + + SAMLProviderMetadata getSPMetadata(); SAMLProviderMetadata getIdPMetadata(String entityId); Collection getAllIdPMetadata(); diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index a7524ec63a7..92408141ef2 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java @@ -543,6 +543,6 @@ public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage SAMLCloudStackRedirectionUrl, SAMLUserAttributeName, SAMLIdentityProviderMetadataURL, SAMLDefaultIdentityProviderId, SAMLSignatureAlgorithm, SAMLAppendDomainSuffix, SAMLTimeout, SAMLCheckSignature, - SAMLForceAuthn, SAMLUserSessionKeyPathAttribute}; + SAMLForceAuthn, SAMLUserSessionKeyPathAttribute, SAMLRequirePasswordLogin}; } } diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java index fd68e2be1ae..2460e3826c6 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java @@ -151,11 +151,11 @@ public class SAMLUtils { return null; } - public static String buildAuthnRequestUrl(final String authnId, final SAMLProviderMetadata spMetadata, final SAMLProviderMetadata idpMetadata, final String signatureAlgorithm) { + public static String buildAuthnRequestUrl(final String authnId, final SAMLProviderMetadata spMetadata, final SAMLProviderMetadata idpMetadata, final String signatureAlgorithm, boolean requirePasswordAuthentication) { String redirectUrl = ""; try { DefaultBootstrap.bootstrap(); - AuthnRequest authnRequest = SAMLUtils.buildAuthnRequestObject(authnId, spMetadata.getEntityId(), idpMetadata.getSsoUrl(), spMetadata.getSsoUrl()); + AuthnRequest authnRequest = SAMLUtils.buildAuthnRequestObject(authnId, spMetadata.getEntityId(), idpMetadata.getSsoUrl(), spMetadata.getSsoUrl(), requirePasswordAuthentication); PrivateKey privateKey = null; if (spMetadata.getKeyPair() != null) { privateKey = spMetadata.getKeyPair().getPrivate(); @@ -168,13 +168,36 @@ public class SAMLUtils { return redirectUrl; } - public static AuthnRequest buildAuthnRequestObject(final String authnId, final String spId, final String idpUrl, final String consumerUrl) { + public static AuthnRequest buildAuthnRequestObject(final String authnId, final String spId, final String idpUrl, final String consumerUrl, boolean requirePasswordAuthentication) { // Issuer object IssuerBuilder issuerBuilder = new IssuerBuilder(); Issuer issuer = issuerBuilder.buildObject(); issuer.setValue(spId); - // AuthnContextClass + // Creation of AuthRequestObject + AuthnRequestBuilder authRequestBuilder = new AuthnRequestBuilder(); + AuthnRequest authnRequest = authRequestBuilder.buildObject(); + + // AuthnContextClass. When this is false, the authentication requirements are defered to the SAML IDP and its default or configured workflow + if (requirePasswordAuthentication) { + setRequestedAuthnContext(authnRequest, requirePasswordAuthentication); + } + + authnRequest.setID(authnId); + authnRequest.setDestination(idpUrl); + authnRequest.setVersion(SAMLVersion.VERSION_20); + authnRequest.setForceAuthn(SAML2AuthManager.SAMLForceAuthn.value()); + authnRequest.setIsPassive(false); + authnRequest.setIssueInstant(new DateTime()); + authnRequest.setProtocolBinding(SAMLConstants.SAML2_POST_BINDING_URI); + authnRequest.setAssertionConsumerServiceURL(consumerUrl); + authnRequest.setProviderName(spId); + authnRequest.setIssuer(issuer); + + return authnRequest; + } + + public static void setRequestedAuthnContext(AuthnRequest authnRequest, boolean requirePasswordAuthentication) { AuthnContextClassRefBuilder authnContextClassRefBuilder = new AuthnContextClassRefBuilder(); AuthnContextClassRef authnContextClassRef = authnContextClassRefBuilder.buildObject( SAMLConstants.SAML20_NS, @@ -186,23 +209,7 @@ public class SAMLUtils { RequestedAuthnContext requestedAuthnContext = requestedAuthnContextBuilder.buildObject(); requestedAuthnContext.setComparison(AuthnContextComparisonTypeEnumeration.EXACT); requestedAuthnContext.getAuthnContextClassRefs().add(authnContextClassRef); - - // Creation of AuthRequestObject - AuthnRequestBuilder authRequestBuilder = new AuthnRequestBuilder(); - AuthnRequest authnRequest = authRequestBuilder.buildObject(); - authnRequest.setID(authnId); - authnRequest.setDestination(idpUrl); - authnRequest.setVersion(SAMLVersion.VERSION_20); - authnRequest.setForceAuthn(SAML2AuthManager.SAMLForceAuthn.value()); - authnRequest.setIsPassive(false); - authnRequest.setIssueInstant(new DateTime()); - authnRequest.setProtocolBinding(SAMLConstants.SAML2_POST_BINDING_URI); - authnRequest.setAssertionConsumerServiceURL(consumerUrl); - authnRequest.setProviderName(spId); - authnRequest.setIssuer(issuer); authnRequest.setRequestedAuthnContext(requestedAuthnContext); - - return authnRequest; } public static LogoutRequest buildLogoutRequest(String logoutUrl, String spId, String nameIdString) { @@ -284,23 +291,6 @@ public class SAMLUtils { } public static void setupSamlUserCookies(final LoginCmdResponse loginResponse, final HttpServletResponse resp) throws IOException { - resp.addCookie(new Cookie("userid", URLEncoder.encode(loginResponse.getUserId(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8))); - resp.addCookie(new Cookie("isSAML", URLEncoder.encode("true", HttpUtils.UTF_8))); - resp.addCookie(new Cookie("twoFaEnabled", URLEncoder.encode(loginResponse.is2FAenabled(), HttpUtils.UTF_8))); - String providerFor2FA = loginResponse.getProviderFor2FA(); - if (StringUtils.isNotEmpty(providerFor2FA)) { - resp.addCookie(new Cookie("twoFaProvider", URLEncoder.encode(loginResponse.getProviderFor2FA(), HttpUtils.UTF_8))); - } - String timezone = loginResponse.getTimeZone(); - if (timezone != null) { - resp.addCookie(new Cookie("timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8))); - } - resp.addCookie(new Cookie("userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20"))); - String redirectUrl = SAML2AuthManager.SAMLCloudStackRedirectionUrl.value(); String path = SAML2AuthManager.SAMLUserSessionKeyPathAttribute.value(); String domain = null; @@ -316,6 +306,18 @@ public class SAMLUtils { } catch (URISyntaxException ex) { throw new CloudRuntimeException("Invalid URI: " + redirectUrl); } + + addBaseCookies(loginResponse, resp, domain, path); + + String providerFor2FA = loginResponse.getProviderFor2FA(); + if (StringUtils.isNotEmpty(providerFor2FA)) { + resp.addCookie(newCookie(domain, path,"twoFaProvider", URLEncoder.encode(loginResponse.getProviderFor2FA(), HttpUtils.UTF_8))); + } + String timezone = loginResponse.getTimeZone(); + if (timezone != null) { + resp.addCookie(newCookie(domain, path,"timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8))); + } + String sameSite = ApiServlet.getApiSessionKeySameSite(); String sessionKeyCookie = String.format("%s=%s;Domain=%s;Path=%s;%s", ApiConstants.SESSIONKEY, loginResponse.getSessionKey(), domain, path, sameSite); s_logger.debug("Adding sessionkey cookie to response: " + sessionKeyCookie); @@ -323,6 +325,24 @@ public class SAMLUtils { resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly;Path=/client/api;%s", ApiConstants.SESSIONKEY, loginResponse.getSessionKey(), sameSite)); } + private static void addBaseCookies(final LoginCmdResponse loginResponse, final HttpServletResponse resp, String domain, String path) throws IOException { + resp.addCookie(newCookie(domain, path, "userid", URLEncoder.encode(loginResponse.getUserId(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"domainid", URLEncoder.encode(loginResponse.getDomainId(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"role", URLEncoder.encode(loginResponse.getType(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"username", URLEncoder.encode(loginResponse.getUsername(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"account", URLEncoder.encode(loginResponse.getAccount(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"isSAML", URLEncoder.encode("true", HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"twoFaEnabled", URLEncoder.encode(loginResponse.is2FAenabled(), HttpUtils.UTF_8))); + resp.addCookie(newCookie(domain, path,"userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20"))); + } + + private static Cookie newCookie(final String domain, final String path, final String name, final String value) { + Cookie cookie = new Cookie(name, value); + cookie.setDomain(domain); + cookie.setPath(path); + return cookie; + } + /** * Returns base64 encoded PublicKey * @param key PublicKey diff --git a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java index 752845edb64..891d028aebf 100644 --- a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java +++ b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java @@ -58,7 +58,7 @@ public class SAMLUtilsTest extends TestCase { String idpUrl = "http://idp.domain.example"; String spId = "cloudstack"; String authnId = SAMLUtils.generateSecureRandomId(); - AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl); + AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl, true); assertEquals(req.getAssertionConsumerServiceURL(), consumerUrl); assertEquals(req.getDestination(), idpUrl); assertEquals(req.getIssuer().getValue(), spId); @@ -86,7 +86,7 @@ public class SAMLUtilsTest extends TestCase { idpMetadata.setSsoUrl(idpUrl); idpMetadata.setEntityId(idpId); - URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value(), true)); assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("SAMLRequest"); assertEquals(urlScheme, redirectUrl.getScheme()); assertEquals(idpDomain, redirectUrl.getHost()); @@ -115,7 +115,7 @@ public class SAMLUtilsTest extends TestCase { idpMetadata.setSsoUrl(idpUrl); idpMetadata.setEntityId(idpId); - URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value(), true)); assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("idpid").hasParameter("SAMLRequest"); assertEquals(urlScheme, redirectUrl.getScheme()); assertEquals(idpDomain, redirectUrl.getHost()); diff --git a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java index 729334d22ce..bfee28a7e3b 100644 --- a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java +++ b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java @@ -213,7 +213,6 @@ public class ListAndSwitchSAMLAccountCmdTest extends TestCase { loginCmdResponse.set2FAenabled("false"); Mockito.when(apiServer.loginUser(nullable(HttpSession.class), nullable(String.class), nullable(String.class), nullable(Long.class), nullable(String.class), nullable(InetAddress.class), nullable(Map.class))).thenReturn(loginCmdResponse); - Mockito.doNothing().when(resp).sendRedirect(nullable(String.class)); try { cmd.authenticate("command", params, session, null, HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp); } catch (ServerApiException exception) { @@ -221,7 +220,6 @@ public class ListAndSwitchSAMLAccountCmdTest extends TestCase { } finally { // accountService should have been called 4 times by now, for this case twice and 2 for cases above Mockito.verify(accountService, Mockito.times(4)).getUserAccountById(Mockito.anyLong()); - Mockito.verify(resp, Mockito.times(1)).sendRedirect(anyString()); } } diff --git a/server/src/main/java/com/cloud/api/ApiServer.java b/server/src/main/java/com/cloud/api/ApiServer.java index c78f8e68c2b..afa5f07c826 100644 --- a/server/src/main/java/com/cloud/api/ApiServer.java +++ b/server/src/main/java/com/cloud/api/ApiServer.java @@ -1159,7 +1159,14 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer domainId = userDomain.getId(); } - UserAccount userAcct = accountMgr.authenticateUser(username, password, domainId, loginIpAddress, requestParameters); + Long userId = (Long)session.getAttribute("nextUserId"); + UserAccount userAcct = null; + if (userId != null) { + userAcct = accountMgr.getUserAccountById(userId); + } else { + userAcct = accountMgr.authenticateUser(username, password, domainId, loginIpAddress, requestParameters); + } + if (userAcct != null) { final String timezone = userAcct.getTimezone(); float offsetInHrs = 0f; diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 8e06c576881..4e3a2e98564 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -372,6 +372,14 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M "totp", "The default user two factor authentication provider. Eg. totp, staticpin", true, ConfigKey.Scope.Domain); + static ConfigKey userAllowMultipleAccounts = new ConfigKey<>("Advanced", + Boolean.class, + "user.allow.multiple.accounts", + "false", + "Determines if the same username can be added to more than one account in the same domain (SAML-only).", + true, + ConfigKey.Scope.Domain); + protected AccountManagerImpl() { super(); } @@ -1252,8 +1260,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // Check permissions checkAccess(getCurrentCallingAccount(), domain); - if (!_userAccountDao.validateUsernameInDomain(userName, domainId)) { - throw new InvalidParameterValueException("The user " + userName + " already exists in domain " + domainId); + if (!userAllowMultipleAccounts.valueInDomain(domainId) && !_userAccountDao.validateUsernameInDomain(userName, domainId)) { + throw new CloudRuntimeException("The user " + userName + " already exists in domain " + domainId); } if (networkDomain != null && networkDomain.length() > 0) { @@ -1436,9 +1444,16 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M throw new PermissionDeniedException("Account id : " + account.getId() + " is a system account, can't add a user to it"); } - if (!_userAccountDao.validateUsernameInDomain(userName, domainId)) { + if (!userAllowMultipleAccounts.valueInDomain(domainId) && !_userAccountDao.validateUsernameInDomain(userName, domainId)) { throw new CloudRuntimeException("The user " + userName + " already exists in domain " + domainId); } + + List duplicatedUsers = _userDao.findUsersByName(userName); + for (UserVO duplicatedUser : duplicatedUsers) { + // users can't exist in same account + assertUserNotAlreadyInAccount(duplicatedUser, account); + } + UserVO user = null; user = createUser(account.getId(), userName, password, firstName, lastName, email, timeZone, userUUID, source); return user; @@ -1564,7 +1579,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M *
  • The username must be unique in each domain. Therefore, if there is already another user with the same username, an {@link InvalidParameterValueException} is thrown. * */ - protected void validateAndUpdateUsernameIfNeeded(UpdateUserCmd updateUserCmd, UserVO user, Account account) { + protected void validateAndUpdateUsernameIfNeeded(UpdateUserCmd updateUserCmd, UserVO newUser, Account newAccount) { String userName = updateUserCmd.getUsername(); if (userName == null) { return; @@ -1572,18 +1587,21 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M if (StringUtils.isBlank(userName)) { throw new InvalidParameterValueException("Username cannot be empty."); } - List duplicatedUsers = _userDao.findUsersByName(userName); - for (UserVO duplicatedUser : duplicatedUsers) { - if (duplicatedUser.getId() == user.getId()) { + List existingUsers = _userDao.findUsersByName(userName); + for (UserVO existingUser : existingUsers) { + if (existingUser.getId() == newUser.getId()) { continue; } - Account duplicatedUserAccountWithUserThatHasTheSameUserName = _accountDao.findById(duplicatedUser.getAccountId()); - if (duplicatedUserAccountWithUserThatHasTheSameUserName.getDomainId() == account.getDomainId()) { - DomainVO domain = _domainDao.findById(duplicatedUserAccountWithUserThatHasTheSameUserName.getDomainId()); - throw new InvalidParameterValueException(String.format("Username [%s] already exists in domain [id=%s,name=%s]", duplicatedUser.getUsername(), domain.getUuid(), domain.getName())); + + // duplicate usernames cannot exist in same domain unless explicitly configured + if (!userAllowMultipleAccounts.valueInDomain(newAccount.getDomainId())) { + assertUserNotAlreadyInDomain(existingUser, newAccount); } + + // can't rename a username to an existing one in the same account + assertUserNotAlreadyInAccount(existingUser, newAccount); } - user.setUsername(userName); + newUser.setUsername(userName); } /** @@ -1820,7 +1838,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // make sure the account is enabled too // if the user is either locked already or disabled already, don't change state...only lock currently enabled -// users + // users boolean success = true; if (user.getState().equals(State.LOCKED)) { // already locked...no-op @@ -3317,7 +3335,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] {UseSecretKeyInResponse, enableUserTwoFactorAuthentication, - userTwoFactorAuthenticationDefaultProvider, mandateUserTwoFactorAuthentication, userTwoFactorAuthenticationIssuer}; + userTwoFactorAuthenticationDefaultProvider, mandateUserTwoFactorAuthentication, userTwoFactorAuthenticationIssuer, + userAllowMultipleAccounts}; } public List getUserTwoFactorAuthenticationProviders() { @@ -3502,4 +3521,21 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M return userAccountVO; }); } + + void assertUserNotAlreadyInAccount(User existingUser, Account newAccount) { + System.out.println(existingUser.getAccountId()); + System.out.println(newAccount.getId()); + if (existingUser.getAccountId() == newAccount.getId()) { + AccountVO existingAccount = _accountDao.findById(newAccount.getId()); + throw new InvalidParameterValueException(String.format("Username [%s] already exists in account [id=%s,name=%s]", existingUser.getUsername(), existingAccount.getUuid(), existingAccount.getAccountName())); + } + } + + void assertUserNotAlreadyInDomain(User existingUser, Account originalAccount) { + Account existingAccount = _accountDao.findById(existingUser.getAccountId()); + if (existingAccount.getDomainId() == originalAccount.getDomainId()) { + DomainVO existingDomain = _domainDao.findById(existingAccount.getDomainId()); + throw new InvalidParameterValueException(String.format("Username [%s] already exists in domain [id=%s,name=%s] user account [id=%s,name=%s]", existingUser.getUsername(), existingDomain.getUuid(), existingDomain.getName(), existingAccount.getUuid(), existingAccount.getAccountName())); + } + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index e68de194f01..f1cf0008676 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1270,4 +1270,75 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Assert.assertNull(updatedUser.getUser2faProvider()); Assert.assertNull(updatedUser.getKeyFor2fa()); } + + @Test(expected = InvalidParameterValueException.class) + public void testAssertUserNotAlreadyInAccount_UserExistsInAccount() { + User existingUser = new UserVO(); + existingUser.setUsername("testuser"); + existingUser.setAccountId(1L); + + Account newAccount = Mockito.mock(Account.class); + Mockito.when(newAccount.getId()).thenReturn(1L); + + AccountVO existingAccount = Mockito.mock(AccountVO.class); + Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid"); + Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account"); + + Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount); + + accountManagerImpl.assertUserNotAlreadyInAccount(existingUser, newAccount); + } + + @Test + public void testAssertUserNotAlreadyInAccount_UserExistsInDiffAccount() { + User existingUser = new UserVO(); + existingUser.setUsername("testuser"); + existingUser.setAccountId(2L); + + Account newAccount = Mockito.mock(Account.class); + Mockito.when(newAccount.getId()).thenReturn(1L); + + accountManagerImpl.assertUserNotAlreadyInAccount(existingUser, newAccount); + } + + @Test(expected = InvalidParameterValueException.class) + public void testAssertUserNotAlreadyInDomain_UserExistsInDomain() { + User existingUser = new UserVO(); + existingUser.setUsername("testuser"); + existingUser.setAccountId(1L); + + Account originalAccount = Mockito.mock(Account.class); + Mockito.when(originalAccount.getDomainId()).thenReturn(1L); + + AccountVO existingAccount = Mockito.mock(AccountVO.class); + Mockito.when(existingAccount.getDomainId()).thenReturn(1L); + Mockito.when(existingAccount.getUuid()).thenReturn("existing-account-uuid"); + Mockito.when(existingAccount.getAccountName()).thenReturn("existing-account"); + + DomainVO existingDomain = Mockito.mock(DomainVO.class); + Mockito.when(existingDomain.getUuid()).thenReturn("existing-domain-uuid"); + Mockito.when(existingDomain.getName()).thenReturn("existing-domain"); + + Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount); + Mockito.when(_domainDao.findById(1L)).thenReturn(existingDomain); + + accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount); + } + + @Test + public void testAssertUserNotAlreadyInDomain_UserExistsInDiffDomain() { + User existingUser = new UserVO(); + existingUser.setUsername("testuser"); + existingUser.setAccountId(1L); + + Account originalAccount = Mockito.mock(Account.class); + Mockito.when(originalAccount.getDomainId()).thenReturn(1L); + + AccountVO existingAccount = Mockito.mock(AccountVO.class); + Mockito.when(existingAccount.getDomainId()).thenReturn(2L); + + Mockito.when(_accountDao.findById(1L)).thenReturn(existingAccount); + + accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount); + } } diff --git a/ui/src/components/header/SamlDomainSwitcher.vue b/ui/src/components/header/SamlDomainSwitcher.vue index 1d820dcbcff..082bab7bf13 100644 --- a/ui/src/components/header/SamlDomainSwitcher.vue +++ b/ui/src/components/header/SamlDomainSwitcher.vue @@ -88,6 +88,7 @@ export default { this.showSwitcher = false return } + this.samlAccounts = samlAccounts this.samlAccounts = _.orderBy(samlAccounts, ['domainPath'], ['asc']) const currentAccount = this.samlAccounts.filter(x => { return x.userId === store.getters.userInfo.id @@ -109,6 +110,8 @@ export default { this.$message.success(`Switched to "${account.accountName} (${account.domainPath})"`) this.$router.go() }) + }).else(error => { + console.log('error refreshing with new user context: ' + error) }) } } diff --git a/ui/src/store/modules/user.js b/ui/src/store/modules/user.js index 46a1905619f..c0a45259a53 100644 --- a/ui/src/store/modules/user.js +++ b/ui/src/store/modules/user.js @@ -290,7 +290,7 @@ const user = { commit('SET_CUSTOM_COLUMNS', cachedCustomColumns) // Ensuring we get the user info so that store.getters.user is never empty when the page is freshly loaded - api('listUsers', { username: Cookies.get('username'), listall: true }).then(response => { + api('listUsers', { id: Cookies.get('userid'), listall: true }).then(response => { const result = response.listusersresponse.user[0] commit('SET_INFO', result) commit('SET_NAME', result.firstname + ' ' + result.lastname) @@ -331,7 +331,7 @@ const user = { }) } - api('listUsers', { username: Cookies.get('username') }).then(response => { + api('listUsers', { id: Cookies.get('userid') }).then(response => { const result = response.listusersresponse.user[0] commit('SET_INFO', result) commit('SET_NAME', result.firstname + ' ' + result.lastname) diff --git a/ui/vue.config.js b/ui/vue.config.js index 9cae2ff66fb..a0e795531fb 100644 --- a/ui/vue.config.js +++ b/ui/vue.config.js @@ -143,7 +143,7 @@ const vueConfig = { ws: false, changeOrigin: true, proxyTimeout: 10 * 60 * 1000, // 10 minutes - cookieDomainRewrite: '*', + cookieDomainRewrite: process.env.CS_COOKIE_HOST || 'localhost', cookiePathRewrite: { '/client': '/' } From 53001417a049f04be8bdd3bb1b92119d9bb3cd61 Mon Sep 17 00:00:00 2001 From: Pearl Dsilva Date: Mon, 14 Apr 2025 09:32:24 -0400 Subject: [PATCH 05/14] UI: Fix column name in Usage view (#10700) --- ui/src/views/infra/UsageRecords.vue | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/src/views/infra/UsageRecords.vue b/ui/src/views/infra/UsageRecords.vue index 16ff9b5dd13..feb1d88bd9b 100644 --- a/ui/src/views/infra/UsageRecords.vue +++ b/ui/src/views/infra/UsageRecords.vue @@ -703,6 +703,7 @@ export default { title = this.$t('label.view') break case 'virtualmachinename': + title = this.$t('label.virtualmachinename') dataIndex = 'name' break default: From 6de084ca976b2a6cb33fb0f47613d3d4a0e81201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernardo=20De=20Marco=20Gon=C3=A7alves?= Date: Tue, 15 Apr 2025 05:24:28 -0300 Subject: [PATCH 06/14] Add download link of volumes, templates and ISOs to the download event details (#10564) --- .../api/command/user/iso/ExtractIsoCmd.java | 11 ++++++++++- .../user/template/ExtractTemplateCmd.java | 10 +++++++++- .../command/user/volume/ExtractVolumeCmd.java | 8 ++++++-- .../com/cloud/storage/VolumeApiServiceImpl.java | 16 ++++++++++++---- .../com/cloud/template/TemplateManagerImpl.java | 8 ++++++-- 5 files changed, 43 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/iso/ExtractIsoCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/iso/ExtractIsoCmd.java index 7861c1e5d41..4d11f0dc3d2 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/iso/ExtractIsoCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/iso/ExtractIsoCmd.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.api.command.user.iso; +import com.cloud.dc.DataCenter; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.ApiConstants; @@ -101,7 +102,15 @@ public class ExtractIsoCmd extends BaseAsyncCmd { @Override public String getEventDescription() { - return "extracting ISO: " + getId() + " from zone: " + getZoneId(); + String isoId = this._uuidMgr.getUuid(VirtualMachineTemplate.class, getId()); + String baseDescription = String.format("Extracting ISO: %s", isoId); + + Long zoneId = getZoneId(); + if (zoneId == null) { + return baseDescription; + } + + return String.format("%s from zone: %s", baseDescription, this._uuidMgr.getUuid(DataCenter.class, zoneId)); } @Override diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/template/ExtractTemplateCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/template/ExtractTemplateCmd.java index 0fa0679bfd9..22f59351e9a 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/template/ExtractTemplateCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/template/ExtractTemplateCmd.java @@ -101,7 +101,15 @@ public class ExtractTemplateCmd extends BaseAsyncCmd { @Override public String getEventDescription() { - return "extracting template: " + this._uuidMgr.getUuid(VirtualMachineTemplate.class, getId()) + ((getZoneId() != null) ? " from zone: " + this._uuidMgr.getUuid(DataCenter.class, getZoneId()) : ""); + String templateId = this._uuidMgr.getUuid(VirtualMachineTemplate.class, getId()); + String baseDescription = String.format("Extracting template: %s", templateId); + + Long zoneId = getZoneId(); + if (zoneId == null) { + return baseDescription; + } + + return String.format("%s from zone: %s", baseDescription, this._uuidMgr.getUuid(DataCenter.class, zoneId)); } @Override diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ExtractVolumeCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ExtractVolumeCmd.java index 9445aba23c0..6ddb5f1e567 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ExtractVolumeCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/volume/ExtractVolumeCmd.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.api.command.user.volume; +import com.cloud.dc.DataCenter; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.ACL; import org.apache.cloudstack.api.APICommand; @@ -114,12 +115,15 @@ public class ExtractVolumeCmd extends BaseAsyncCmd { @Override public String getEventDescription() { - return "Extraction job"; + String volumeId = this._uuidMgr.getUuid(Volume.class, getId()); + String zoneId = this._uuidMgr.getUuid(DataCenter.class, getZoneId()); + + return String.format("Extracting volume: %s from zone: %s", volumeId, zoneId); } @Override public void execute() { - CallContext.current().setEventDetails("Volume Id: " + this._uuidMgr.getUuid(Volume.class, getId())); + CallContext.current().setEventDetails(getEventDescription()); String uploadUrl = _volumeService.extractVolume(this); if (uploadUrl != null) { ExtractResponse response = _responseGenerator.createVolumeExtractResponse(id, zoneId, getEntityOwnerId(), mode, uploadUrl); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 15978bf1dc7..4719d2c9194 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -4115,7 +4115,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic Optional extractUrl = setExtractVolumeSearchCriteria(sc, volume); if (extractUrl.isPresent()) { - return extractUrl.get(); + String url = extractUrl.get(); + CallContext.current().setEventDetails(String.format("Download URL: %s, volume ID: %s", url, volume.getUuid())); + return url; } VMInstanceVO vm = null; @@ -4132,7 +4134,9 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic VmWorkJobVO placeHolder = null; placeHolder = createPlaceHolderWork(vm.getId()); try { - return orchestrateExtractVolume(volume.getId(), zoneId); + String url = orchestrateExtractVolume(volume.getId(), zoneId); + CallContext.current().setEventDetails(String.format("Download URL: %s, volume ID: %s", url, volume.getUuid())); + return url; } finally { _workJobDao.expunge(placeHolder.getId()); } @@ -4161,13 +4165,17 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic // retrieve the entity url from job result if (jobResult != null && jobResult instanceof String) { - return (String)jobResult; + String url = (String) jobResult; + CallContext.current().setEventDetails(String.format("Download URL: %s, volume ID: %s", url, volume.getUuid())); + return url; } return null; } } - return orchestrateExtractVolume(volume.getId(), zoneId); + String url = orchestrateExtractVolume(volume.getId(), zoneId); + CallContext.current().setEventDetails(String.format("Download URL: %s, volume ID: %s", url, volume.getUuid())); + return url; } @Override diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 6073b4f0bb7..9f23bdef142 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -488,7 +488,9 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, String mode = cmd.getMode(); Long eventId = cmd.getStartEventId(); - return extract(account, templateId, url, zoneId, mode, eventId, true); + String extractUrl = extract(account, templateId, url, zoneId, mode, eventId, true); + CallContext.current().setEventDetails(String.format("Download URL: %s, ISO ID: %s", extractUrl, _tmpltDao.findById(templateId).getUuid())); + return extractUrl; } @Override @@ -506,7 +508,9 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, throw new InvalidParameterValueException("unable to find template with id " + templateId); } - return extract(caller, templateId, url, zoneId, mode, eventId, false); + String extractUrl = extract(caller, templateId, url, zoneId, mode, eventId, false); + CallContext.current().setEventDetails(String.format("Download URL: %s, template ID: %s", extractUrl, template.getUuid())); + return extractUrl; } @Override From ac6b1b382cfd552a1d5311e8c2172c563e39a631 Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Tue, 15 Apr 2025 08:48:45 -0300 Subject: [PATCH 07/14] Migrate public templates that have URLs on data migration across secondary storages (#10364) Co-authored-by: Fabricio Duarte --- .../VMTemplateStorageResourceAssoc.java | 3 + .../orchestration/DataMigrationUtility.java | 32 +++- .../DataMigrationUtilityTest.java | 88 +++++++++++ .../image/SecondaryStorageServiceImpl.java | 91 ++++++++++-- .../SecondaryStorageServiceImplTest.java | 138 ++++++++++++++++++ .../api/query/dao/TemplateJoinDaoImpl.java | 7 - .../storage/download/DownloadActiveState.java | 2 +- .../storage/download/DownloadListener.java | 1 + 8 files changed, 336 insertions(+), 26 deletions(-) create mode 100644 engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtilityTest.java create mode 100644 engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImplTest.java diff --git a/api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java b/api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java index f43d5331222..db702a61f2b 100644 --- a/api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java +++ b/api/src/main/java/com/cloud/storage/VMTemplateStorageResourceAssoc.java @@ -17,6 +17,7 @@ package com.cloud.storage; import java.util.Date; +import java.util.List; import org.apache.cloudstack.api.InternalIdentity; @@ -25,6 +26,8 @@ public interface VMTemplateStorageResourceAssoc extends InternalIdentity { UNKNOWN, DOWNLOAD_ERROR, NOT_DOWNLOADED, DOWNLOAD_IN_PROGRESS, DOWNLOADED, ABANDONED, UPLOADED, NOT_UPLOADED, UPLOAD_ERROR, UPLOAD_IN_PROGRESS, CREATING, CREATED, BYPASSED } + List PENDING_DOWNLOAD_STATES = List.of(Status.NOT_DOWNLOADED, Status.DOWNLOAD_IN_PROGRESS); + String getInstallPath(); long getTemplateId(); diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java index c260f48dcf8..9609ba7751d 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtility.java @@ -208,9 +208,7 @@ public class DataMigrationUtility { List files = new LinkedList<>(); for (TemplateDataStoreVO template : templates) { VMTemplateVO templateVO = templateDao.findById(template.getTemplateId()); - if (template.getState() == ObjectInDataStoreStateMachine.State.Ready && templateVO != null && - (!templateVO.isPublicTemplate() || (templateVO.isPublicTemplate() && templateVO.getUrl() == null)) && - templateVO.getHypervisorType() != Hypervisor.HypervisorType.Simulator && templateVO.getParentTemplateId() == null) { + if (shouldMigrateTemplate(template, templateVO)) { files.add(templateFactory.getTemplate(template.getTemplateId(), srcDataStore)); } } @@ -231,6 +229,34 @@ public class DataMigrationUtility { return getAllReadyTemplates(srcDataStore, childTemplates, templates); } + /** + * Returns whether a template should be migrated. A template should be migrated if: + *
      + *
    1. its state is ready, and
      2. + *
    2. its hypervisor type is not simulator, and
      3. + *
    3. it is not a child template.
      4. + *
    + */ + protected boolean shouldMigrateTemplate(TemplateDataStoreVO template, VMTemplateVO templateVO) { + if (template.getState() != State.Ready) { + logger.debug("Template [{}] should not be migrated as it is not ready.", template); + return false; + } + + if (templateVO.getHypervisorType() == Hypervisor.HypervisorType.Simulator) { + logger.debug("Template [{}] should not be migrated as its hypervisor type is simulator.", template); + return false; + } + + if (templateVO.getParentTemplateId() != null) { + logger.debug("Template [{}] should not be migrated as it has a parent template.", template); + return false; + } + + logger.debug("Template [{}] should be migrated.", template); + return true; + } + /** Returns parent snapshots and snapshots that do not have any children; snapshotChains comprises of the snapshot chain info * for each parent snapshot and the cumulative size of the chain - this is done to ensure that all the snapshots in a chain * are migrated to the same datastore diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtilityTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtilityTest.java new file mode 100644 index 00000000000..acd98e1cbff --- /dev/null +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/DataMigrationUtilityTest.java @@ -0,0 +1,88 @@ +// 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.cloudstack.engine.orchestration; + +import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.VMTemplateVO; +import junit.framework.TestCase; +import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; +import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class DataMigrationUtilityTest extends TestCase { + + @Spy + private DataMigrationUtility dataMigrationUtility; + + @Mock + private VMTemplateVO templateVoMock; + + @Mock + private TemplateDataStoreVO templateDataStoreVoMock; + + private void prepareForShouldMigrateTemplateTests() { + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Ready); + Mockito.when(templateVoMock.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.KVM); + Mockito.when(templateVoMock.getParentTemplateId()).thenReturn(null); + } + + @Test + public void shouldMigrateTemplateTestReturnsFalseWhenTemplateIsNotReady() { + prepareForShouldMigrateTemplateTests(); + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Migrating); + + boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock); + + Assert.assertFalse(result); + } + + @Test + public void shouldMigrateTemplateTestReturnsFalseWhenHypervisorTypeIsSimulator() { + prepareForShouldMigrateTemplateTests(); + Mockito.when(templateVoMock.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.Simulator); + + boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock); + + Assert.assertFalse(result); + } + + @Test + public void shouldMigrateTemplateTestReturnsFalseWhenTemplateHasParentTemplate() { + prepareForShouldMigrateTemplateTests(); + Mockito.when(templateVoMock.getParentTemplateId()).thenReturn(1L); + + boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock); + + Assert.assertFalse(result); + } + + @Test + public void shouldMigrateTemplateTestReturnsTrueWhenTemplateIsReadyAndDoesNotHaveParentTemplateAndHypervisorTypeIsNotSimulator() { + prepareForShouldMigrateTemplateTests(); + + boolean result = dataMigrationUtility.shouldMigrateTemplate(templateDataStoreVoMock, templateVoMock); + + Assert.assertTrue(result); + } +} diff --git a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java index 730b003fcb0..502bbaf9e49 100644 --- a/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java +++ b/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImpl.java @@ -24,6 +24,9 @@ import java.util.concurrent.ExecutionException; import javax.inject.Inject; +import com.cloud.storage.VMTemplateStorageResourceAssoc; +import com.cloud.storage.download.DownloadListener; +import com.cloud.utils.exception.CloudRuntimeException; import org.apache.cloudstack.engine.subsystem.api.storage.CopyCommandResult; import org.apache.cloudstack.engine.subsystem.api.storage.DataMotionService; import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; @@ -118,26 +121,21 @@ public class SecondaryStorageServiceImpl implements SecondaryStorageService { } } else if (srcDataObject instanceof TemplateInfo && templateChain != null && templateChain.containsKey(srcDataObject)) { for (TemplateInfo templateInfo : templateChain.get(srcDataObject).first()) { + if (templateIsOnDestination(templateInfo, destDatastore)) { + res.setResult("Template already exists on destination."); + res.setSuccess(true); + logger.debug("Deleting template {} from source data store [{}].", srcDataObject.getTO().toString(), + srcDataObject.getDataStore().getTO().toString()); + srcDataObject.getDataStore().delete(srcDataObject); + future.complete(res); + continue; + } destDataObject = destDatastore.create(templateInfo); templateInfo.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested); destDataObject.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested); migrateJob(future, templateInfo, destDataObject, destDatastore); } - } - else { - // Check if template in destination store, if yes, do not proceed - if (srcDataObject instanceof TemplateInfo) { - logger.debug("Checking if template present at destination"); - TemplateDataStoreVO templateStoreVO = templateStoreDao.findByStoreTemplate(destDatastore.getId(), srcDataObject.getId()); - if (templateStoreVO != null) { - String msg = "Template already exists in destination store"; - logger.debug(msg); - res.setResult(msg); - res.setSuccess(true); - future.complete(res); - return future; - } - } + } else { destDataObject = destDatastore.create(srcDataObject); srcDataObject.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested); destDataObject.processEvent(ObjectInDataStoreStateMachine.Event.MigrateDataRequested); @@ -160,6 +158,69 @@ public class SecondaryStorageServiceImpl implements SecondaryStorageService { return future; } + /** + * Returns a boolean indicating whether a template is ready on the provided data store. If the template is being downloaded, + * waits until the download finishes. + * @param srcDataObject the template. + * @param destDatastore the data store. + */ + protected boolean templateIsOnDestination(DataObject srcDataObject, DataStore destDatastore) { + if (!(srcDataObject instanceof TemplateInfo)) { + return false; + } + + String templateAsString = srcDataObject.getTO().toString(); + String destDatastoreAsString = destDatastore.getTO().toString(); + TemplateDataStoreVO templateStoreVO; + + long timer = getTemplateDownloadTimeout(); + long msToSleep = 10000L; + int previousDownloadPercentage = -1; + + while (true) { + templateStoreVO = templateStoreDao.findByStoreTemplate(destDatastore.getId(), srcDataObject.getId()); + if (templateStoreVO == null) { + logger.debug("{} is not present at destination [{}].", templateAsString, destDatastoreAsString); + return false; + } + VMTemplateStorageResourceAssoc.Status downloadState = templateStoreVO.getDownloadState(); + if (downloadState == null || !VMTemplateStorageResourceAssoc.PENDING_DOWNLOAD_STATES.contains(downloadState)) { + break; + } + if (previousDownloadPercentage == templateStoreVO.getDownloadPercent()) { + timer -= msToSleep; + } else { + timer = getTemplateDownloadTimeout(); + } + if (timer <= 0) { + throw new CloudRuntimeException(String.format("Timeout while waiting for %s to be downloaded to image store [%s]. " + + "The download percentage has not changed for %d milliseconds.", templateAsString, destDatastoreAsString, getTemplateDownloadTimeout())); + } + waitForTemplateDownload(msToSleep, templateAsString, destDatastoreAsString); + } + + if (templateStoreVO.getState() == ObjectInDataStoreStateMachine.State.Ready) { + logger.debug("{} already exists on destination [{}].", templateAsString, destDatastoreAsString); + return true; + } + return false; + } + + protected long getTemplateDownloadTimeout() { + return DownloadListener.DOWNLOAD_TIMEOUT; + } + + protected void waitForTemplateDownload(long msToSleep, String templateAsString, String destDatastoreAsString) { + logger.debug("{} is being downloaded to destination [{}]; we will verify in {} milliseconds if the download has finished.", + templateAsString, destDatastoreAsString, msToSleep); + try { + Thread.sleep(msToSleep); + } catch (InterruptedException e) { + logger.warn("[ignored] interrupted while waiting for template {} download to finish before trying to migrate it to data store [{}].", + templateAsString, destDatastoreAsString); + } + } + protected void migrateJob(AsyncCallFuture future, DataObject srcDataObject, DataObject destDataObject, DataStore destDatastore) throws ExecutionException, InterruptedException { MigrateDataContext context = new MigrateDataContext(null, future, srcDataObject, destDataObject, destDatastore); AsyncCallbackDispatcher caller = AsyncCallbackDispatcher.create(this); diff --git a/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImplTest.java b/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImplTest.java new file mode 100644 index 00000000000..740194ea253 --- /dev/null +++ b/engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/SecondaryStorageServiceImplTest.java @@ -0,0 +1,138 @@ +// 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.cloudstack.storage.image; + +import com.cloud.agent.api.to.DataStoreTO; +import com.cloud.storage.VMTemplateStorageResourceAssoc; +import com.cloud.utils.exception.CloudRuntimeException; +import junit.framework.TestCase; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; +import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo; +import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; +import org.apache.cloudstack.storage.to.TemplateObjectTO; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class SecondaryStorageServiceImplTest extends TestCase { + + @Spy + @InjectMocks + private SecondaryStorageServiceImpl secondaryStorageService; + + @Mock + TemplateDataStoreDao templateDataStoreDaoMock; + + @Mock + TemplateDataStoreVO templateDataStoreVoMock; + + @Mock + TemplateInfo templateInfoMock; + + @Mock + TemplateObjectTO templateObjectToMock; + + @Mock + DataStore dataStoreMock; + + @Mock + DataStoreTO dataStoreToMock; + + private void prepareForTemplateIsOnDestinationTests() { + long dataStoreId = 1; + long templateId = 2; + + Mockito.when(dataStoreMock.getId()).thenReturn(dataStoreId); + Mockito.when(dataStoreMock.getTO()).thenReturn(dataStoreToMock); + Mockito.when(templateInfoMock.getId()).thenReturn(templateId); + Mockito.when(templateInfoMock.getTO()).thenReturn(templateObjectToMock); + Mockito.doReturn(templateDataStoreVoMock).when(templateDataStoreDaoMock).findByStoreTemplate(dataStoreId, templateId); + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Ready); + } + + @Test + public void templateIsOnDestinationTestReturnsFalseWhenTemplateStoreRefDoesNotExist() { + prepareForTemplateIsOnDestinationTests(); + Mockito.doReturn(null).when(templateDataStoreDaoMock).findByStoreTemplate(Mockito.anyLong(), Mockito.anyLong()); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertFalse(result); + } + + @Test + public void templateIsOnDestinationTestReturnsTrueWhenTemplateIsReady() { + prepareForTemplateIsOnDestinationTests(); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertTrue(result); + } + + @Test + public void templateIsOnDestinationTestReturnsFalseWhenTemplateIsNotReady() { + prepareForTemplateIsOnDestinationTests(); + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Creating); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertFalse(result); + } + + @Test + public void templateIsOnDestinationTestReturnsTrueIfTemplateIsDownloadedSuccessfully() { + prepareForTemplateIsOnDestinationTests(); + Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS); + Mockito.doAnswer(I -> Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOADED)).when(secondaryStorageService).waitForTemplateDownload(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString()); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertTrue(result); + } + + @Test + public void templateIsOnDestinationTestReturnsFalseIfTemplateIsNotDownloadedSuccessfully() { + prepareForTemplateIsOnDestinationTests(); + Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS); + Mockito.doAnswer(I -> { + Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_ERROR); + Mockito.when(templateDataStoreVoMock.getState()).thenReturn(ObjectInDataStoreStateMachine.State.Failed); + return "mocked download fail"; + }).when(secondaryStorageService).waitForTemplateDownload(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString()); + + boolean result = secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + + Assert.assertFalse(result); + } + + @Test(expected = CloudRuntimeException.class) + public void templateIsOnDestinationTestThrowsExceptionIfDownloadTimesOut() { + prepareForTemplateIsOnDestinationTests(); + Mockito.when(templateDataStoreVoMock.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOAD_IN_PROGRESS); + Mockito.doReturn(0L).when(secondaryStorageService).getTemplateDownloadTimeout(); + + secondaryStorageService.templateIsOnDestination(templateInfoMock, dataStoreMock); + } +} diff --git a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java index 0bdf5040c82..ac24f14b781 100644 --- a/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java +++ b/server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java @@ -151,11 +151,6 @@ public class TemplateJoinDaoImpl extends GenericDaoBaseWithTagInformation Date: Wed, 16 Apr 2025 14:28:06 +0530 Subject: [PATCH 09/14] Fix volume migration failure response (#10707) --- .../vmware/resource/VmwareResource.java | 4 +-- .../motion/VmwareStorageMotionStrategy.java | 29 ++++++++++++------- .../cloud/storage/VolumeApiServiceImpl.java | 2 +- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java index 08fa4b438f6..a6bd95ee897 100644 --- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java +++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java @@ -5104,9 +5104,9 @@ public class VmwareResource extends ServerResourceBase implements StoragePoolRes answer.setVolumeChainInfo(chainInfo); return answer; } catch (Exception e) { - String msg = "Catch Exception " + e.getClass().getName() + " due to " + e.toString(); + String msg = "Catch Exception " + e.getClass().getName() + " due to " + e.getMessage(); logger.error(msg, e); - return new MigrateVolumeAnswer(cmd, false, msg, null); + return new MigrateVolumeAnswer(cmd, false, e.getMessage(), null); } } diff --git a/plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java b/plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java index 5b389e0d9df..b0cacf60a17 100644 --- a/plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java +++ b/plugins/hypervisors/vmware/src/main/java/org/apache/cloudstack/storage/motion/VmwareStorageMotionStrategy.java @@ -260,7 +260,7 @@ public class VmwareStorageMotionStrategy implements DataMotionStrategy { } else { answer = agentMgr.sendTo(sourcePool.getDataCenterId(), HypervisorType.VMware, cmd); } - updateVolumeAfterMigration(answer, srcData, destData); + handleAnswerAndUpdateVolumeAfterMigration(answer, srcData, destData); CopyCommandResult result = new CopyCommandResult(null, answer); callback.complete(result); } @@ -286,21 +286,19 @@ public class VmwareStorageMotionStrategy implements DataMotionStrategy { return hostId; } - private void updateVolumeAfterMigration(Answer answer, DataObject srcData, DataObject destData) { + private void handleAnswerAndUpdateVolumeAfterMigration(Answer answer, DataObject srcData, DataObject destData) { VolumeVO destinationVO = volDao.findById(destData.getId()); if (!(answer instanceof MigrateVolumeAnswer)) { // OfflineVmwareMigration: reset states and such - VolumeVO sourceVO = volDao.findById(srcData.getId()); - sourceVO.setState(Volume.State.Ready); - volDao.update(sourceVO.getId(), sourceVO); - if (destinationVO.getId() != sourceVO.getId()) { - destinationVO.setState(Volume.State.Expunged); - destinationVO.setRemoved(new Date()); - volDao.update(destinationVO.getId(), destinationVO); - } + resetVolumeState(srcData, destinationVO); throw new CloudRuntimeException("unexpected answer from hypervisor agent: " + answer.getDetails()); } MigrateVolumeAnswer ans = (MigrateVolumeAnswer) answer; + if (!answer.getResult()) { + String msg = "Unable to migrate volume: " + srcData.getName() + " due to " + answer.getDetails(); + resetVolumeState(srcData, destinationVO); + throw new CloudRuntimeException(msg); + } if (logger.isDebugEnabled()) { String format = "retrieved '%s' as new path for volume(%d)"; logger.debug(String.format(format, ans.getVolumePath(), destData.getId())); @@ -311,6 +309,17 @@ public class VmwareStorageMotionStrategy implements DataMotionStrategy { volDao.update(destinationVO.getId(), destinationVO); } + private void resetVolumeState(DataObject srcData, VolumeVO destinationVO) { + VolumeVO sourceVO = volDao.findById(srcData.getId()); + sourceVO.setState(Volume.State.Ready); + volDao.update(sourceVO.getId(), sourceVO); + if (destinationVO.getId() != sourceVO.getId()) { + destinationVO.setState(Volume.State.Expunged); + destinationVO.setRemoved(new Date()); + volDao.update(destinationVO.getId(), destinationVO); + } + } + @Override public void copyAsync(Map volumeMap, VirtualMachineTO vmTo, Host srcHost, Host destHost, AsyncCompletionCallback callback) { Answer answer = null; diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 4719d2c9194..6a2aa09c38a 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -3738,7 +3738,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic VolumeApiResult result = future.get(); if (result.isFailed()) { logger.debug("migrate volume failed:" + result.getResult()); - throw new StorageUnavailableException("Migrate volume failed: " + result.getResult(), destPool.getId()); + throw new CloudRuntimeException("Migrate volume failed: " + result.getResult()); } return result.getVolume(); } catch (InterruptedException e) { From 207a2c1da35c00647f16ea06c2ef0f8476acec56 Mon Sep 17 00:00:00 2001 From: Vishesh Date: Wed, 16 Apr 2025 18:28:17 +0530 Subject: [PATCH 10/14] Support ConfigDrive with VPC (#10495) --- .../cloud/network/element/ConfigDriveNetworkElement.java | 6 +++--- ui/src/views/offering/AddVpcOffering.vue | 8 ++++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java b/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java index 47c9979c2f6..5f1c1e58d93 100644 --- a/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java +++ b/server/src/main/java/com/cloud/network/element/ConfigDriveNetworkElement.java @@ -203,8 +203,8 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle private static Map> setCapabilities() { Map> capabilities = new HashMap<>(); capabilities.put(Service.UserData, null); - capabilities.put(Service.Dhcp, new HashMap<>()); - capabilities.put(Service.Dns, new HashMap<>()); + capabilities.put(Service.Dhcp, Map.of(Network.Capability.DhcpAccrossMultipleSubnets, "true")); + capabilities.put(Service.Dns, Map.of(Capability.AllowDnsSuffixModification, "true")); return capabilities; } @@ -841,7 +841,7 @@ public class ConfigDriveNetworkElement extends AdapterBase implements NetworkEle public boolean configDhcpSupportForSubnet(Network network, NicProfile nic, VirtualMachineProfile vm, DeployDestination dest, ReservationContext context) throws ConcurrentOperationException, InsufficientCapacityException, ResourceUnavailableException { - return false; + return true; } @Override diff --git a/ui/src/views/offering/AddVpcOffering.vue b/ui/src/views/offering/AddVpcOffering.vue index 450ee117715..99227d2de50 100644 --- a/ui/src/views/offering/AddVpcOffering.vue +++ b/ui/src/views/offering/AddVpcOffering.vue @@ -449,12 +449,16 @@ export default { services.push({ name: 'Dhcp', provider: [ - { name: 'VpcVirtualRouter' } + { name: 'VpcVirtualRouter' }, + { name: 'ConfigDrive' } ] }) services.push({ name: 'Dns', - provider: [{ name: 'VpcVirtualRouter' }] + provider: [ + { name: 'VpcVirtualRouter' }, + { name: 'ConfigDrive' } + ] }) services.push({ name: 'Lb', From 53d3d19606bcf332753f4b51b31b851fe53cbd66 Mon Sep 17 00:00:00 2001 From: dahn Date: Wed, 16 Apr 2025 17:26:26 +0200 Subject: [PATCH 11/14] server: check startip and endip of shared network (#10704) revert part of #10168 --- .../com/cloud/network/NetworkServiceImpl.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/com/cloud/network/NetworkServiceImpl.java b/server/src/main/java/com/cloud/network/NetworkServiceImpl.java index bdb928ae919..7655e6486c5 100644 --- a/server/src/main/java/com/cloud/network/NetworkServiceImpl.java +++ b/server/src/main/java/com/cloud/network/NetworkServiceImpl.java @@ -1635,10 +1635,19 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService, C throwInvalidIdException("Network offering with specified id doesn't support adding multiple ip ranges", ntwkOff.getUuid(), NETWORK_OFFERING_ID); } - if (GuestType.Shared == ntwkOff.getGuestType() && !ntwkOff.isSpecifyVlan() && Objects.isNull(associatedNetworkId)) { - throw new CloudRuntimeException("Associated network must be provided when creating Shared networks when specifyVlan is false"); - } + + if (GuestType.Shared == ntwkOff.getGuestType()) { + if (!ntwkOff.isSpecifyIpRanges()) { + throw new CloudRuntimeException("The 'specifyipranges' parameter should be true for Shared Networks"); + } + if (ipv4 && Objects.isNull(startIP)) { + throw new CloudRuntimeException("IPv4 address range needs to be provided"); + } + if (ipv6 && Objects.isNull(startIPv6)) { + throw new CloudRuntimeException("IPv6 address range needs to be provided"); + } + } Pair interfaceMTUs = validateMtuConfig(publicMtu, privateMtu, zone.getId()); mtuCheckForVpcNetwork(vpcId, interfaceMTUs, publicMtu); From 8db248e4b40b96c0f70a4eaaf41848087d1f24ca Mon Sep 17 00:00:00 2001 From: Nicolas Vazquez Date: Thu, 17 Apr 2025 04:54:11 -0300 Subject: [PATCH 12/14] UI: Move templates creation date to the Zones tab (#10709) * UI: Move templates creation date to the Zones tab * Extend changes to ISOs --- ui/src/components/view/InfoCard.vue | 4 ++-- ui/src/views/image/IsoZones.vue | 8 ++++++++ ui/src/views/image/TemplateZones.vue | 8 ++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/ui/src/components/view/InfoCard.vue b/ui/src/components/view/InfoCard.vue index d805f7b2719..00cb4748a88 100644 --- a/ui/src/components/view/InfoCard.vue +++ b/ui/src/components/view/InfoCard.vue @@ -629,7 +629,7 @@ {{ resource.podname || resource.pod || resource.podid }} -
    +
    {{ $t('label.zone') }}
    @@ -700,7 +700,7 @@ {{ resource.managementserver || resource.managementserverid }}
    -
    +
    {{ $t('label.created') }}
    {{ $toLocaleDate(resource.created) }} diff --git a/ui/src/views/image/IsoZones.vue b/ui/src/views/image/IsoZones.vue index daf1e7e21e0..75eac8fd97f 100644 --- a/ui/src/views/image/IsoZones.vue +++ b/ui/src/views/image/IsoZones.vue @@ -48,6 +48,9 @@ {{ $t('label.yes') }} {{ $t('label.no') }} + +