Copilot commented on code in PR #13345:
URL: https://github.com/apache/cloudstack/pull/13345#discussion_r3913181040
##########
agent/src/main/java/com/cloud/agent/AgentShell.java:
##########
@@ -410,7 +420,7 @@ public void init(String[] args) throws
ConfigurationException {
if (LOGGER.isDebugEnabled()) {
List<String> properties =
Collections.list((Enumeration<String>)_properties.propertyNames());
for (String property : properties) {
- LOGGER.debug("Found property: {}", property);
+ LOGGER.debug("Found property: {}, value: {}", property,
_properties.getProperty(property));
}
Review Comment:
This debug logging prints *all* agent properties including their values.
Agent properties commonly include sensitive data (passwords, keys, tokens), so
emitting them to logs is a security risk and can leak secrets in support
bundles.
Consider logging only the property names (previous behavior), or explicitly
redacting known-sensitive keys before logging values.
##########
framework/cluster/src/main/java/com/cloud/cluster/ClusterServiceServletHttpHandler.java:
##########
@@ -113,6 +117,14 @@ private void writeResponse(HttpResponse response, int
statusCode, String content
response.setEntity(body);
}
+ private void logRequest(HttpRequest request, String requestBody) {
+ Optional<HttpRequest> requestOpt = Optional.ofNullable(request);
+ Optional<RequestLine> requestLineOpt =
requestOpt.map(HttpRequest::getRequestLine);
+ String method =
requestLineOpt.map(RequestLine::getMethod).orElse(null);
+ String uri = requestLineOpt.map(RequestLine::getUri).orElse(null);
+ logger.debug("{} {} {}", method, uri, requestBody);
+ }
Review Comment:
`logRequest` logs the full raw POST body at DEBUG level. Cluster-service
requests can include operational identifiers and potentially sensitive
parameters; logging the entire body at DEBUG makes accidental data exposure
much more likely (and duplicates the per-parameter TRACE logging above).
Suggestion: log only method/URI (or body length / a sanitized subset) unless
TRACE is enabled and the data is explicitly safe to log.
##########
utils/src/main/java/com/cloud/utils/backoff/BackoffFactory.java:
##########
@@ -0,0 +1,96 @@
+// 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 com.cloud.utils.backoff;
+
+import com.cloud.utils.backoff.impl.ConstantTimeBackoff;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.stream.Collectors;
+
+/**
+ * Backoff implementation factory.
+ *
+ * @author mprokopchuk
+ */
+public interface BackoffFactory {
+ Logger logger = LogManager.getLogger(BackoffFactory.class);
+ /**
+ * Property name for the implementation class (that extends {@link
BackoffAlgorithm}) to be used either
+ * by {@code agent.properties} file or by configuration key.
+ */
+ String BACKOFF_IMPLEMENTATION_KEY = "backoff.implementation";
+
+ /**
+ * Default backoff implementation class name ({@link ConstantTimeBackoff}).
+ */
+ String DEFAULT_BACKOFF_IMPLEMENTATION =
ConstantTimeBackoff.class.getName();
+
+ /**
+ * Creates default {@link BackoffAlgorithm} implementation object ({@link
ConstantTimeBackoff}).
+ *
+ * @param properties configuration properties
+ * @return {@link BackoffAlgorithm} implementation object
+ */
+ static BackoffAlgorithm createDefault(Properties properties) {
+ Properties newProperties = new Properties(properties);
+ newProperties.put(BACKOFF_IMPLEMENTATION_KEY,
DEFAULT_BACKOFF_IMPLEMENTATION);
+ return create(newProperties);
+ }
Review Comment:
`createDefault` currently uses `new Properties(properties)`, which sets the
passed `properties` as *defaults* rather than copying entries. Since
`create(Properties)` builds params from `properties.entrySet()`, the default
backoff will be created with only `backoff.implementation` set and will
silently drop other backoff-related settings (e.g. `backoff.seconds`, min/max
delay), changing behavior when falling back to the default implementation.
Copy the provided properties into a new `Properties` instance before
overriding the implementation key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
