Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Apollo 2.5.0
* [Fix: Operate the AccessKey multiple times within one second](https://github.com/apolloconfig/apollo/pull/5490)
* [Bugfix: Prevent accidental cache deletion when recreating AppNamespace with the same name and appid](https://github.com/apolloconfig/apollo/issues/5502)
* [Feature: Support ordinary users to modify personal information](https://github.com/apolloconfig/apollo/pull/5511)
* [Feature: Support exporting and importing configurations for specified applications and clusters](https://github.com/apolloconfig/apollo/pull/5517)
* [doc: Add rust apollo client link](https://github.com/apolloconfig/apollo/pull/5514)
------------------
All issues and pull requests are [here](https://github.com/apolloconfig/apollo/milestone/16?closed=1)
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,33 @@ public void exportAll(@RequestParam(value = "envs") String envs, HttpServletRequ
// set downloaded filename
response.setHeader(HttpHeaders.CONTENT_DISPOSITION, "attachment;filename=" + filename);

List<Env> exportEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream()
.map(env -> Env.valueOf(env)).collect(Collectors.toList());
List<Env> exportEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf)
.collect(Collectors.toList());
Comment on lines +127 to +128
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Validate env values before calling Env.valueOf in export endpoints.

Both the bulk export (/configs/export) and the new app-level export (/apps/{appId}/envs/{env}/clusters/{clusterName}/export) call Env.valueOf on user input. For unknown env names this throws IllegalArgumentException, which will be surfaced as a 500.

To keep the API contract clear, consider translating invalid envs into a client error instead, e.g.:

List<Env> exportEnvs;
try {
  exportEnvs = Splitter.on(ENV_SEPARATOR)
      .splitToList(envs)
      .stream()
      .map(Env::valueOf)
      .collect(Collectors.toList());
} catch (IllegalArgumentException ex) {
  throw new BadRequestException("Invalid environment(s): " + envs);
}

And similarly wrap the single env path variable before passing it to Env.valueOf in exportAppConfig.

Also applies to: 137-152

🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsExportController.java
around lines 127-128 (and similarly lines 137-152), the code calls Env.valueOf
on unvalidated user input which will throw IllegalArgumentException and surface
as a 500; change the flow to validate and translate invalid env names into a
client error by catching IllegalArgumentException when mapping split envs to Env
(or when converting the single env path variable) and throw a
BadRequestException (or equivalent 4xx) with a clear message like "Invalid
environment(s): <input>" so invalid inputs return a 400 instead of a 500.


try (OutputStream outputStream = response.getOutputStream()) {
configsExportService.exportData(outputStream, exportEnvs);
}
}

@PreAuthorize(value = "@unifiedPermissionValidator.isAppAdmin(#appId)")
@GetMapping("/apps/{appId}/envs/{env}/clusters/{clusterName}/export")
public void exportAppConfig(@PathVariable String appId, @PathVariable String env,
@PathVariable String clusterName, HttpServletRequest request, HttpServletResponse response)
throws IOException {
// filename must contain the information of time
final String filename = String.format("%s+%s+%s+%s.zip", appId, env, clusterName,
DateFormatUtils.format(new Date(), "yyyy_MMdd_HH_mm_ss"));
// log who download the configs
logger.info(
"Download configs, remote addr [{}], remote host [{}]. Filename is [{}]. AppId is [{}], env is [{}], clusterName is [{}]",
request.getRemoteAddr(), request.getRemoteHost(), filename, appId, env, clusterName);
// set downloaded filename
response.setHeader(HttpHeaders.CONTENT_DISPOSITION, "attachment;filename=" + filename);

try (OutputStream outputStream = response.getOutputStream()) {
configsExportService.exportAppConfigByEnvAndCluster(appId, Env.valueOf(env), clusterName,
outputStream);
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package com.ctrip.framework.apollo.portal.controller;

import com.ctrip.framework.apollo.common.exception.BadRequestException;
import com.google.common.base.Splitter;

import com.ctrip.framework.apollo.core.enums.ConfigFileFormat;
Expand Down Expand Up @@ -44,10 +45,10 @@
@RestController
public class ConfigsImportController {
private static final String ENV_SEPARATOR = ",";

private static final String CONFLICT_ACTION_IGNORE = "ignore";
private static final String CONFLICT_ACTION_COVER = "cover";
private final ConfigsImportService configsImportService;


public ConfigsImportController(final ConfigsImportService configsImportService) {
this.configsImportService = configsImportService;
}
Expand Down Expand Up @@ -76,31 +77,42 @@ public void importConfigFile(@PathVariable String appId, @PathVariable String en
}

@PreAuthorize(value = "@unifiedPermissionValidator.isSuperAdmin()")
@PostMapping(value = "/configs/import", params = "conflictAction=cover")
public void importConfigByZipWithCoverConflictNamespace(@RequestParam(value = "envs") String envs,
@PostMapping(value = "/configs/import")
public void importConfigByZip(@RequestParam(value = "envs") String envs,
@RequestParam(defaultValue = CONFLICT_ACTION_IGNORE) String conflictAction,
@RequestParam("file") MultipartFile file) throws IOException {
validateConflictAction(conflictAction);
boolean ignoreConflictNamespace = conflictAction.equals(CONFLICT_ACTION_IGNORE);

List<Env> importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream()
.map(env -> Env.valueOf(env)).collect(Collectors.toList());
List<Env> importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf)
.collect(Collectors.toList());
Comment on lines +87 to +88
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Potential unhandled IllegalArgumentException from Env::valueOf.

Per the Env.valueOf implementation in the relevant code snippets, it throws IllegalArgumentException if the environment name doesn't exist. This would result in an HTTP 500 response instead of a user-friendly 400 Bad Request.

Consider wrapping with proper exception handling:

-    List<Env> importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf)
-        .collect(Collectors.toList());
+    List<Env> importEnvs;
+    try {
+      importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf)
+          .collect(Collectors.toList());
+    } catch (IllegalArgumentException e) {
+      throw new BadRequestException("Invalid environment: " + e.getMessage());
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
List<Env> importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf)
.collect(Collectors.toList());
List<Env> importEnvs;
try {
importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream().map(Env::valueOf)
.collect(Collectors.toList());
} catch (IllegalArgumentException e) {
throw new BadRequestException("Invalid environment: " + e.getMessage());
}


byte[] bytes = file.getBytes();
try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) {
configsImportService.importDataFromZipFile(importEnvs, zipInputStream, false);
configsImportService.importDataFromZipFile(importEnvs, zipInputStream,
ignoreConflictNamespace);
}
Comment on lines 90 to 94
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid file.getBytes() for ZIP uploads (heap pressure / DoS risk).

Both ZIP endpoints buffer the entire upload into memory before creating ZipInputStream. Prefer streaming:

-    byte[] bytes = file.getBytes();
-    try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) {
+    try (ZipInputStream zipInputStream = new ZipInputStream(file.getInputStream())) {
       ...
     }

Optionally also validate “zip-ness” (content-type/filename) and enforce/verify upload size limits.

Also applies to: 105-109

🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
around lines 90-94 (and similarly 105-109), the code currently calls
file.getBytes() which loads the entire upload into memory; change it to stream
the upload instead by obtaining an InputStream (e.g. file.getInputStream() or
the servlet request InputStream) and wrap that in a ZipInputStream so the ZIP is
processed incrementally. Additionally enforce/verify upload size limits before
processing, and perform basic ZIP validation (check filename/content-type and/or
the ZIP magic header) before iterating entries to reject non-ZIP or oversized
uploads. Ensure resources are closed via try-with-resources on the
InputStream/ZipInputStream.

}

@PreAuthorize(value = "@unifiedPermissionValidator.isSuperAdmin()")
@PostMapping(value = "/configs/import", params = "conflictAction=ignore")
public void importConfigByZipWithIgnoreConflictNamespace(
@RequestParam(value = "envs") String envs, @RequestParam("file") MultipartFile file)
throws IOException {

List<Env> importEnvs = Splitter.on(ENV_SEPARATOR).splitToList(envs).stream()
.map(env -> Env.valueOf(env)).collect(Collectors.toList());

@PreAuthorize(value = "@unifiedPermissionValidator.isAppAdmin(#appId)")
@PostMapping(value = "/apps/{appId}/envs/{env}/clusters/{clusterName}/import")
public void importAppConfigByZip(@PathVariable String appId, @PathVariable String env,
@PathVariable String clusterName,
@RequestParam(defaultValue = CONFLICT_ACTION_IGNORE) String conflictAction,
@RequestParam("file") MultipartFile file) throws IOException {
validateConflictAction(conflictAction);
boolean ignoreConflictNamespace = conflictAction.equals(CONFLICT_ACTION_IGNORE);
byte[] bytes = file.getBytes();
try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) {
configsImportService.importDataFromZipFile(importEnvs, zipInputStream, true);
configsImportService.importAppConfigFromZipFile(appId, Env.valueOf(env), clusterName,
zipInputStream, ignoreConflictNamespace);
Comment on lines +107 to +108
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Same IllegalArgumentException risk for single env parsing.

The Env.valueOf(env) call can throw IllegalArgumentException for invalid environment names, resulting in HTTP 500 instead of 400.

+    Env envObj;
+    try {
+      envObj = Env.valueOf(env);
+    } catch (IllegalArgumentException e) {
+      throw new BadRequestException("Invalid environment: " + env);
+    }
     byte[] bytes = file.getBytes();
     try (ZipInputStream zipInputStream = new ZipInputStream(new ByteArrayInputStream(bytes))) {
-      configsImportService.importAppConfigFromZipFile(appId, Env.valueOf(env), clusterName,
+      configsImportService.importAppConfigFromZipFile(appId, envObj, clusterName,
           zipInputStream, ignoreConflictNamespace);
     }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
around lines 107-108, the direct call to Env.valueOf(env) can throw
IllegalArgumentException for invalid env names causing a 500; validate or safely
parse the env string and convert it to an Env in a try-catch (or use a safe
helper) and if parsing fails return a 400 Bad Request (e.g., throw or return an
appropriate client error with a clear message) before calling
configsImportService.importAppConfigFromZipFile.

}
}

private void validateConflictAction(String conflictAction) {
if (!conflictAction.equals(CONFLICT_ACTION_COVER)
&& !conflictAction.equals(CONFLICT_ACTION_IGNORE)) {
throw new BadRequestException("ConflictAction is incorrect.");
}
}
Comment on lines +112 to 117
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Make conflictAction validation a bit more robust/clear.

Minor hardening/readability:

  • Prefer constant-first equals to be null-safe: CONFLICT_ACTION_IGNORE.equals(conflictAction)
  • Consider returning a more actionable 400 message (e.g., include allowed values: ignore|cover).
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/ConfigsImportController.java
around lines 112 to 117, make the conflictAction validation null-safe and more
actionable by using constant-first equals checks (e.g.,
CONFLICT_ACTION_IGNORE.equals(conflictAction) ||
CONFLICT_ACTION_COVER.equals(conflictAction)) and, if invalid, throw
BadRequestException with a clearer message that includes the allowed values (for
example: "ConflictAction is incorrect. Allowed values: ignore|cover"). Ensure
you do not introduce additional behavioral changes beyond the null-safe equals
order and improved error text.

}
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,43 @@ public void exportData(OutputStream outputStream, List<Env> exportEnvs) {
exportApps(exportEnvs, outputStream);
}

/**
* Export all configurations of an application in a specified environment and cluster
* <p>
* File Struts:
* <p>
*
* List<App> -> List<Env> -> List<Namespace>
*
* @param outputStream network file download stream to user
*/
public void exportAppConfigByEnvAndCluster(String appId, Env env, String clusterName,
OutputStream outputStream) {
App app = appService.load(appId);
if (app == null) {
throw new BadRequestException("App not found: " + appId);
}
ClusterDTO cluster = clusterService.loadCluster(appId, env, clusterName);
if (cluster == null) {
throw new BadRequestException(
"The app does not exist in the specified environment and cluster.");
}

try (final ZipOutputStream zipOutputStream = new ZipOutputStream(outputStream)) {
try {
this.exportNamespaces(env, app, cluster, zipOutputStream, true);
} catch (BadRequestException badRequestException) {
Comment thread
nobodyiam marked this conversation as resolved.
// ignore
} catch (Exception e) {
logger.error("export namespace error. appId = {}, env = {}, cluster = {}", app.getAppId(),
env.getName(), cluster.getName(), e);
}
Comment on lines +126 to +133
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Silently ignoring BadRequestException may hide legitimate errors.

Catching and ignoring BadRequestException without any logging could mask issues like missing namespaces or configuration problems. While the env-level export (line 258) has the same pattern, for app-level export this is the primary operation, so failures are more significant.

Consider at least logging a warning:

       try {
         this.exportNamespaces(env, app, cluster, zipOutputStream, true);
       } catch (BadRequestException badRequestException) {
-        // ignore
+        logger.warn("export namespace warning. appId = {}, env = {}, cluster = {}, message = {}",
+            app.getAppId(), env.getName(), cluster.getName(), badRequestException.getMessage());
       } catch (Exception e) {
🤖 Prompt for AI Agents
In
apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/service/ConfigsExportService.java
around lines 126-133, the catch block for BadRequestException currently swallows
the exception; change it to log a warning (including appId, env, cluster and the
exception) instead of ignoring it so legitimate problems are visible; use
logger.warn with a descriptive message and pass the exception (or at least
exception.getMessage()) to preserve stack/diagnostic info while keeping current
control flow.

} catch (IOException e) {
logger.error("export app config error", e);
throw new ServiceException("export app config error", e);
}
}

private void exportApps(final Collection<Env> exportEnvs, OutputStream outputStream) {
List<App> hasPermissionApps = findHasPermissionApps();

Expand Down Expand Up @@ -217,7 +254,7 @@ private void exportCluster(final Env env, final App exportApp, ZipOutputStream z
// export namespaces
exportClusters.parallelStream().forEach(cluster -> {
try {
this.exportNamespaces(env, exportApp, cluster, zipOutputStream);
this.exportNamespaces(env, exportApp, cluster, zipOutputStream, false);
} catch (BadRequestException badRequestException) {
// ignore
} catch (Exception e) {
Expand All @@ -228,7 +265,7 @@ private void exportCluster(final Env env, final App exportApp, ZipOutputStream z
}

private void exportNamespaces(final Env env, final App exportApp, final ClusterDTO exportCluster,
ZipOutputStream zipOutputStream) {
ZipOutputStream zipOutputStream, boolean ignoreUserDir) {
String clusterName = exportCluster.getName();

List<NamespaceBO> namespaceBOS =
Expand All @@ -241,11 +278,11 @@ private void exportNamespaces(final Env env, final App exportApp, final ClusterD
Stream<ConfigBO> configBOStream = namespaceBOS.stream().map(namespaceBO -> new ConfigBO(env,
exportApp.getOwnerName(), exportApp.getAppId(), clusterName, namespaceBO));

writeNamespacesToZip(configBOStream, zipOutputStream);
writeNamespacesToZip(configBOStream, zipOutputStream, ignoreUserDir);
}

private void writeNamespacesToZip(Stream<ConfigBO> configBOStream,
ZipOutputStream zipOutputStream) {
ZipOutputStream zipOutputStream, boolean ignoreUserDir) {
final Consumer<ConfigBO> configBOConsumer = configBO -> {
try {
synchronized (zipOutputStream) {
Expand All @@ -257,8 +294,10 @@ private void writeNamespacesToZip(Stream<ConfigBO> configBOStream,

String configFileName =
ConfigFileUtils.toFilename(appId, clusterName, namespace, configFileFormat);
String filePath = ConfigFileUtils.genNamespacePath(configBO.getOwnerName(), appId,
configBO.getEnv(), configFileName);
String filePath = ignoreUserDir
? ConfigFileUtils.genNamespacePathIgnoreUser(appId, configBO.getEnv(), configFileName)
: ConfigFileUtils.genNamespacePath(configBO.getOwnerName(), appId, configBO.getEnv(),
configFileName);

writeToZip(filePath, configFileContent, zipOutputStream);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package com.ctrip.framework.apollo.portal.service;

import com.ctrip.framework.apollo.common.exception.BadRequestException;
import com.google.common.collect.Lists;
import com.google.gson.Gson;

Expand Down Expand Up @@ -46,7 +47,6 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.rmi.ServerException;
import java.util.Date;
import java.util.List;
import java.util.Objects;
Expand Down Expand Up @@ -167,7 +167,62 @@ public void importDataFromZipFile(List<Env> importEnvs, ZipInputStream dataZip,

} catch (Exception e) {
LOGGER.error("import config error.", e);
throw new ServerException("import config error.", e);
throw new ServiceException("import config error.", e);
}
}

/**
* import all configurations of an application in a specified environment and cluster
*/
public void importAppConfigFromZipFile(String appId, Env env, String clusterName,
ZipInputStream dataZip, boolean ignoreConflictNamespace) throws IOException {
ClusterDTO clusterDTO = clusterService.loadCluster(appId, env, clusterName);
if (clusterDTO == null) {
throw new BadRequestException(
"The app does not exist in the specified environment and cluster.");
}

List<ImportNamespaceData> toImportNSs = Lists.newArrayList();
ZipEntry entry;
while ((entry = dataZip.getNextEntry()) != null) {
if (entry.isDirectory()) {
continue;
}

// file.path format :
// ${appId}/${env}/${appId}+${cluster}+${namespaceName}
String filePath = entry.getName();
String content = readContent(dataZip);
if (content == null) {
throw new BadRequestException("Failed to read file content.");
}
String[] info = filePath.replace('\\', '/').split("/");
if (info.length != 3) {
throw new BadRequestException("Invalid file path in ZIP.");
}
String fileName = info[2];
String fileNamePrefix = String.format("%s+%s+", appId, clusterName);

if (!info[0].equals(appId) || !info[1].equalsIgnoreCase(env.getName())
|| !fileName.startsWith(fileNamePrefix)) {
throw new BadRequestException("The content of the file to be imported is incorrect.");
}
if (!fileName.endsWith(ConfigFileUtils.CLUSTER_METADATA_FILE_SUFFIX)) {
toImportNSs.add(new ImportNamespaceData(env, fileName, content, ignoreConflictNamespace));
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if (CollectionUtils.isEmpty(toImportNSs)) {
throw new BadRequestException("The configuration to be imported is empty.");
}

try {
LOGGER.info("Import namespace. namespace = {}", toImportNSs.size());
doImport(Lists.newArrayList(), Lists.newArrayList(), Lists.newArrayList(),
Lists.newArrayList(), toImportNSs);
} catch (Exception e) {
LOGGER.error("import app config error.", e);
throw new ServiceException("import app config error.", e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,15 @@ public static String genNamespacePath(final String ownerName, final String appId
return String.join(File.separator, ownerName, appId, env.getName(), configFilename);
}

/**
* file path = appId/env/configFilename
* @return file path in compressed file
*/
public static String genNamespacePathIgnoreUser(final String appId, final Env env,
final String configFilename) {
return String.join(File.separator, appId, env.getName(), configFilename);
}

/**
* path = ownerName/appId/app.metadata
*/
Expand Down
Loading