Skip to content

Commit d20177c

Browse files
Damans227claude
authored and
weizhouapache
committed
Fix nested TO renaming, log redaction and cleanup in compat TypeAdaptors
AbstractTOAdaptor built its own private Gson to run the pre-rename serialization step through, which meant it never honoured the LoggingExclusionStrategy the enclosing Gson (GsonHelper's logging instance) was configured with, so fields marked @loglevel(Off) (e.g. VirtualMachineTO.vncPassword) leaked in plaintext when logged. It also had no adapters for the sibling compat TOs, so nested TOs (disks/nics inside a VirtualMachineTO, or a VirtualMachineTO inside a MigrateCommand) kept their new field names instead of being renamed for backward compatibility with older Agents. AbstractTOAdaptor no longer owns a Gson at all: it takes one via initGson(), mirroring the existing InterfaceTypeAdaptor pattern. GsonHelper.setDefaultGsonConfig now wires each compat adaptor's delegate Gson incrementally off the same builder, snapshotting it via builder.create() right before each adaptor registers itself, so every adaptor's delegate carries its sibling adaptors (for correct nested renaming) without ever routing back into itself and recursing forever. NetworkTO is now registered via registerTypeHierarchyAdapter since VirtualMachineTO.nics is declared as NicTO[] (a NetworkTO subclass) and was never matched by the previous exact-type registration. This also removes AbstractTOAdaptor's now-unused loggerBuilder/LOGGER and its duplicate copy of GsonHelper.setDefaultGsonConfig, and replaces a dead null check (getAsJsonObject() never returns null) with a real isJsonObject() check. Added RequestTest#testCompatFieldRenamingNestedTOs covering a StartCommand and a MigrateCommand with nested disks/nics, asserting old field names appear at every nesting level on the wire and that vncPassword never appears in the logging serialization. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 15b39f4 commit d20177c

3 files changed

Lines changed: 130 additions & 60 deletions

File tree

‎core/src/main/java/com/cloud/agent/transport/compat/AbstractTOAdaptor.java‎

Lines changed: 15 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -16,54 +16,33 @@
1616
// under the License.
1717
package com.cloud.agent.transport.compat;
1818

19-
import com.cloud.agent.api.Answer;
20-
import com.cloud.agent.api.Command;
21-
import com.cloud.agent.api.SecStorageFirewallCfgCommand;
22-
import com.cloud.agent.api.to.DataStoreTO;
23-
import com.cloud.agent.api.to.DataTO;
24-
import com.cloud.agent.transport.ArrayTypeAdaptor;
25-
import com.cloud.agent.transport.InterfaceTypeAdaptor;
26-
import com.cloud.agent.transport.LoggingExclusionStrategy;
27-
import com.cloud.agent.transport.Request;
28-
import com.cloud.agent.transport.StoragePoolTypeAdaptor;
29-
import com.cloud.hypervisor.Hypervisor;
30-
import com.cloud.storage.Storage;
31-
import com.cloud.utils.Pair;
3219
import com.cloud.utils.StringUtils;
3320
import com.cloud.utils.exception.CloudRuntimeException;
3421
import com.google.gson.Gson;
35-
import com.google.gson.GsonBuilder;
3622
import com.google.gson.JsonElement;
3723
import com.google.gson.JsonObject;
3824
import com.google.gson.JsonSerializationContext;
3925
import com.google.gson.JsonSerializer;
40-
import com.google.gson.reflect.TypeToken;
41-
import org.apache.cloudstack.transport.HypervisorTypeAdaptor;
42-
import org.apache.logging.log4j.Logger;
43-
import org.apache.logging.log4j.LogManager;
4426

4527
import java.lang.reflect.Type;
4628
import java.util.LinkedHashMap;
47-
import java.util.List;
4829
import java.util.Map;
4930

5031
/**
5132
* JSON serializer adapter for transport classes (com.cloud.agent.api.to.*) that ensures backward compatibility
5233
* with older Agent versions due to rename of the fields
5334
* (see https://github.com/apache/cloudstack/pull/10514)
35+
*
36+
* This class does not build its own Gson instance: doing so would silently drop whichever exclusion
37+
* strategy (e.g. log redaction) and sibling compat adaptors (for nested TOs) the enclosing Gson was
38+
* configured with. Instead, whoever registers an instance of this class into a GsonBuilder is
39+
* responsible for also calling {@link #initGson(Gson)} with a Gson that (a) carries that same
40+
* exclusion strategy and (b) has adapters registered for any nested TO types that also need field
41+
* renaming, but not for this adaptor's own type (to avoid infinite recursion). See
42+
* {@link com.cloud.serializer.GsonHelper#setDefaultGsonConfig(com.google.gson.GsonBuilder)}.
5443
*/
5544
public class AbstractTOAdaptor<T> implements JsonSerializer<T> {
56-
private static final Logger LOGGER = LogManager.getLogger(AbstractTOAdaptor.class);
57-
private static final Gson gson;
58-
59-
static {
60-
GsonBuilder gsonBuilder = new GsonBuilder();
61-
gson = setDefaultGsonConfig(gsonBuilder);
62-
GsonBuilder loggerBuilder = new GsonBuilder();
63-
loggerBuilder.disableHtmlEscaping();
64-
loggerBuilder.setExclusionStrategies(new LoggingExclusionStrategy(LOGGER));
65-
}
66-
45+
private Gson gson;
6746
private Map<String, String> fieldMappings;
6847

6948
protected AbstractTOAdaptor(String... fields) {
@@ -82,38 +61,18 @@ protected AbstractTOAdaptor(String... fields) {
8261
}
8362
}
8463

85-
private static Gson setDefaultGsonConfig(GsonBuilder builder) {
86-
builder.setVersion(1.5);
87-
InterfaceTypeAdaptor<DataStoreTO> dsAdaptor = new InterfaceTypeAdaptor<DataStoreTO>();
88-
builder.registerTypeAdapter(DataStoreTO.class, dsAdaptor);
89-
InterfaceTypeAdaptor<DataTO> dtAdaptor = new InterfaceTypeAdaptor<DataTO>();
90-
builder.registerTypeAdapter(DataTO.class, dtAdaptor);
91-
ArrayTypeAdaptor<Command> cmdAdaptor = new ArrayTypeAdaptor<Command>();
92-
builder.registerTypeAdapter(Command[].class, cmdAdaptor);
93-
ArrayTypeAdaptor<Answer> ansAdaptor = new ArrayTypeAdaptor<Answer>();
94-
builder.registerTypeAdapter(Answer[].class, ansAdaptor);
95-
builder.registerTypeAdapter(new TypeToken<List<SecStorageFirewallCfgCommand.PortConfig>>() {
96-
}.getType(), new Request.PortConfigListTypeAdaptor());
97-
builder.registerTypeAdapter(new TypeToken<Pair<Long, Long>>() {
98-
}.getType(), new Request.NwGroupsCommandTypeAdaptor());
99-
builder.registerTypeAdapter(Storage.StoragePoolType.class, new StoragePoolTypeAdaptor());
100-
builder.registerTypeAdapter(Hypervisor.HypervisorType.class, new HypervisorTypeAdaptor());
101-
102-
Gson gson = builder.create();
103-
dsAdaptor.initGson(gson);
104-
dtAdaptor.initGson(gson);
105-
cmdAdaptor.initGson(gson);
106-
ansAdaptor.initGson(gson);
107-
return gson;
64+
public void initGson(Gson gson) {
65+
this.gson = gson;
10866
}
10967

11068
@Override
11169
public JsonElement serialize(T src, Type typeOfSrc, JsonSerializationContext context) {
11270
if (src == null) {
11371
return null;
11472
}
115-
JsonObject obj = gson.toJsonTree(src).getAsJsonObject();
116-
if (obj != null) {
73+
JsonElement tree = gson.toJsonTree(src);
74+
if (tree.isJsonObject()) {
75+
JsonObject obj = tree.getAsJsonObject();
11776
for (Map.Entry<String, String> field : fieldMappings.entrySet()) {
11877
String sourceField = field.getKey();
11978
String destinationField = field.getValue();
@@ -122,6 +81,6 @@ public JsonElement serialize(T src, Type typeOfSrc, JsonSerializationContext con
12281
}
12382
}
12483
}
125-
return obj;
84+
return tree;
12685
}
12786
}

‎core/src/main/java/com/cloud/serializer/GsonHelper.java‎

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,11 +86,42 @@ public static Gson setDefaultGsonConfig(GsonBuilder builder) {
8686
}.getType(), new NwGroupsCommandTypeAdaptor());
8787
builder.registerTypeAdapter(Storage.StoragePoolType.class, new StoragePoolTypeAdaptor());
8888
builder.registerTypeAdapter(Hypervisor.HypervisorType.class, new HypervisorTypeAdaptor());
89+
8990
// added for compatibility purposes, remove after all Agents migrate to the new version
90-
builder.registerTypeAdapter(VirtualMachineTO.class, new VirtualMachineTOAdaptor());
91-
builder.registerTypeAdapter(DiskTO.class, new DiskTOAdaptor());
92-
builder.registerTypeAdapter(NetworkTO.class, new NetworkTOAdaptor());
93-
builder.registerTypeAdapter(MigrateCommand.class, new MigrateCommandAdaptor());
91+
//
92+
// Each compat adaptor below needs a "base" Gson to run its own reflective (pre-rename)
93+
// serialization through, so that nested TOs are renamed too and the exclusion strategy set
94+
// on `builder` (e.g. log redaction) is honoured consistently at every nesting level. That base
95+
// Gson is built incrementally off the same builder, snapshotted (via builder.create()) just
96+
// before each adaptor's own type is registered on it, so it carries every sibling adaptor it
97+
// can nest without ever routing back into itself and recursing forever.
98+
DiskTOAdaptor diskAdaptor = new DiskTOAdaptor();
99+
NetworkTOAdaptor netAdaptor = new NetworkTOAdaptor();
100+
VirtualMachineTOAdaptor vmAdaptor = new VirtualMachineTOAdaptor();
101+
MigrateCommandAdaptor migrateAdaptor = new MigrateCommandAdaptor();
102+
103+
// DiskTO and NetworkTO don't nest any other compat TO, so the plain config built so far is
104+
// already the correct base Gson for them.
105+
Gson leafDelegateGson = builder.create();
106+
diskAdaptor.initGson(leafDelegateGson);
107+
netAdaptor.initGson(leafDelegateGson);
108+
109+
// VirtualMachineTO nests DiskTO[] and NicTO[] (NicTO extends NetworkTO), so its base Gson needs
110+
// Disk/Network adapters too. registerTypeHierarchyAdapter is used for NetworkTO so that the
111+
// NicTO[]-declared "nics" field is matched via its supertype.
112+
builder.registerTypeAdapter(DiskTO.class, diskAdaptor);
113+
builder.registerTypeHierarchyAdapter(NetworkTO.class, netAdaptor);
114+
Gson vmDelegateGson = builder.create();
115+
vmAdaptor.initGson(vmDelegateGson);
116+
117+
// MigrateCommand nests a VirtualMachineTO, so its base Gson needs the VirtualMachineTO adapter
118+
// (which already renames the nested disks/nics above).
119+
builder.registerTypeAdapter(VirtualMachineTO.class, vmAdaptor);
120+
Gson migrateDelegateGson = builder.create();
121+
migrateAdaptor.initGson(migrateDelegateGson);
122+
123+
builder.registerTypeAdapter(MigrateCommand.class, migrateAdaptor);
124+
94125
Gson gson = builder.create();
95126
dsAdaptor.initGson(gson);
96127
dtAdaptor.initGson(gson);

‎core/src/test/java/com/cloud/agent/transport/RequestTest.java‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package com.cloud.agent.transport;
2121

2222
import java.nio.ByteBuffer;
23+
import java.util.HashMap;
2324
import junit.framework.TestCase;
2425

2526
import org.apache.logging.log4j.Logger;
@@ -35,19 +36,27 @@
3536
import com.cloud.agent.api.Command;
3637
import com.cloud.agent.api.GetHostStatsCommand;
3738
import com.cloud.agent.api.GetVolumeStatsCommand;
39+
import com.cloud.agent.api.MigrateCommand;
3840
import com.cloud.agent.api.SecStorageFirewallCfgCommand;
41+
import com.cloud.agent.api.StartCommand;
3942
import com.cloud.agent.api.UpdateHostPasswordCommand;
4043
import com.cloud.agent.api.storage.DownloadAnswer;
4144
import com.cloud.agent.api.storage.ListTemplateCommand;
45+
import com.cloud.agent.api.to.DiskTO;
4246
import com.cloud.agent.api.to.NfsTO;
47+
import com.cloud.agent.api.to.NicTO;
48+
import com.cloud.agent.api.to.VirtualMachineTO;
4349
import com.cloud.agent.transport.Request.Version;
4450
import com.cloud.exception.UnsupportedVersionException;
51+
import com.cloud.host.Host;
4552
import com.cloud.hypervisor.Hypervisor.HypervisorType;
4653
import com.cloud.storage.DataStoreRole;
4754
import com.cloud.storage.Storage.ImageFormat;
4855
import com.cloud.storage.Storage.TemplateType;
4956
import com.cloud.storage.VMTemplateStorageResourceAssoc.Status;
5057
import com.cloud.template.VirtualMachineTemplate;
58+
import com.cloud.template.VirtualMachineTemplate.BootloaderType;
59+
import com.cloud.vm.VirtualMachine;
5160

5261
/**
5362
*
@@ -186,6 +195,77 @@ public void testCompress() {
186195
}
187196
}
188197

198+
public void testLogging() {
199+
logger.info("Testing Logging");
200+
GetHostStatsCommand cmd3 = new GetHostStatsCommand("hostguid", "hostname", 101);
201+
Request sreq = new Request(2, 3, new Command[] {cmd3}, true, true);
202+
sreq.setSequence(1);
203+
Logger logger = Logger.getLogger(GsonHelper.class);
204+
Level level = logger.getLevel();
205+
206+
logger.setLevel(Level.DEBUG);
207+
String log = sreq.log("Debug", true, Level.DEBUG);
208+
assert (log == null);
209+
210+
log = sreq.log("Debug", false, Level.DEBUG);
211+
assert (log != null);
212+
213+
logger.setLevel(Level.TRACE);
214+
log = sreq.log("Trace", true, Level.TRACE);
215+
assert (log.contains(GetHostStatsCommand.class.getSimpleName()));
216+
logger.debug(log);
217+
218+
logger.setLevel(level);
219+
}
220+
221+
public void testCompatFieldRenamingNestedTOs() {
222+
logger.info("Testing that renamed fields are restored on nested TOs too, for backward compatibility with older Agents");
223+
224+
DiskTO diskTO = new DiskTO();
225+
diskTO.setDetails(new HashMap<String, String>());
226+
227+
NicTO nicTO = new NicTO();
228+
nicTO.setSecurityGroupEnabled(true);
229+
230+
VirtualMachineTO vmTO = new VirtualMachineTO(1, "i-2-3-VM", VirtualMachine.Type.User, 1, 512, 512L * 1024 * 1024, 512L * 1024 * 1024,
231+
BootloaderType.HVM, "Other PV (64-bit)", true, true, "vncpassword123");
232+
vmTO.setDetails(new HashMap<String, String>());
233+
vmTO.setDisks(new DiskTO[] {diskTO});
234+
vmTO.setNics(new NicTO[] {nicTO});
235+
236+
Host host = Mockito.mock(Host.class);
237+
Mockito.when(host.getPrivateIpAddress()).thenReturn("10.1.1.1");
238+
StartCommand startCmd = new StartCommand(vmTO, host, false);
239+
240+
Request startReq = new Request(1, 1, startCmd, true);
241+
String startWireJson = GsonHelper.getGson().toJson(new Command[] {startCmd});
242+
assert startWireJson.contains("\"params\"") : "VirtualMachineTO.details should be serialized under its old name 'params'";
243+
assert startWireJson.contains("\"_details\"") : "nested DiskTO.details should be serialized under its old name '_details'";
244+
assert startWireJson.contains("\"isSecurityGroupEnabled\"") : "nested NicTO.securityGroupEnabled should be serialized under its old name 'isSecurityGroupEnabled'";
245+
assert startWireJson.contains("vncpassword123") : "wire serialization should still contain the real vncPassword value";
246+
247+
Logger gsonLogger = Logger.getLogger(GsonHelper.class);
248+
Level gsonLoggerLevel = gsonLogger.getLevel();
249+
gsonLogger.setLevel(Level.TRACE);
250+
String startLogJson;
251+
try {
252+
startLogJson = startReq.log("Trace", true, Level.TRACE);
253+
} finally {
254+
gsonLogger.setLevel(gsonLoggerLevel);
255+
}
256+
assert startLogJson.contains("\"isSecurityGroupEnabled\"") : "renamed fields should still show up in the logging serialization";
257+
assert !startLogJson.contains("vncpassword123") : "logging serialization should never contain the plaintext vncPassword value";
258+
259+
MigrateCommand migrateCmd = new MigrateCommand("i-2-3-VM", "10.1.1.2", true, vmTO, false);
260+
String migrateWireJson = GsonHelper.getGson().toJson(new Command[] {migrateCmd});
261+
assert migrateWireJson.contains("\"destIp\"") : "MigrateCommand.destinationIp should be serialized under its old name 'destIp'";
262+
assert migrateWireJson.contains("\"isWindows\"") : "MigrateCommand.windows should be serialized under its old name 'isWindows'";
263+
assert migrateWireJson.contains("\"vmTO\"") : "MigrateCommand.virtualMachine should be serialized under its old name 'vmTO'";
264+
assert migrateWireJson.contains("\"params\"") : "VirtualMachineTO nested in MigrateCommand should still be renamed";
265+
assert migrateWireJson.contains("\"_details\"") : "DiskTO nested inside the VirtualMachineTO nested in MigrateCommand should still be renamed";
266+
assert migrateWireJson.contains("\"isSecurityGroupEnabled\"") : "NicTO nested inside the VirtualMachineTO nested in MigrateCommand should still be renamed";
267+
}
268+
189269
protected void compareRequest(Request req1, Request req2) {
190270
assert req1.getSequence() == req2.getSequence();
191271
assert req1.getAgentId() == req2.getAgentId();

0 commit comments

Comments
 (0)