Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

<fix>[conf]: internal port #1329

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

<fix>[conf]: internal port #1329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion conf/globalConfig/consoleConfig.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
<category>console</category>
<name>vnc.allow.ports</name>
<description>tcp or udp ports allowed by vnc, udp port start with 'u' like 'u67', port range can be like this: 1100:1200, multiple port or port range can be separated by ”,“</description>
<defaultValue>4900,4901</defaultValue>
<defaultValue>{consoleProxyPort},4901</defaultValue>
<type>java.lang.String</type>
</config>
</globalConfig>
Copy link

Choose a reason for hiding this comment

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

  1. The patch changes the default port from 4900,4901 to {consoleProxyPort},4901 - this could introduce a security risk if consoleProxyPort is not validated properly or if it is empty.

  2. It would be better to validate the value of consoleProxyPort before using it in the defaultValue.

  3. If the intention is to allow multiple ports, then the syntax used for defaultValue should be according to the description given in the code. For example: 1100:1200,u67 etc.

  4. It might be beneficial to add some more comments to the code to make it more readable and understandable.

2 changes: 1 addition & 1 deletion conf/globalConfig/kvm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@
<category>kvm</category>
<name>kvmagent.allow.ports</name>
<description>tcp or udp ports allowed by kvmagent, udp port start with 'u' like 'u67', port range can be like this: 1100:1200, multiple port or port range can be separated by ”,“</description>
<defaultValue>22,7070,16509,49152:49261,2049,20000:30000,u4789,u8472,7069,9100,9103,9092</defaultValue>
<defaultValue>22,{KvmAgent.port},16509,49152:49261,2049,20000:30000,u4789,u8472,{KvmAgent.prometheusPort},{KvmAgent.nodeExportPort},{KvmAgent.collectdExposePort},{kvmhost.pushgateway.webListenPort}</defaultValue>
<type>java.lang.String</type>
</config>

Copy link

Choose a reason for hiding this comment

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

with a brief overview of the code patch

The code patch is adding new ports to the kvmagent.allow.ports configuration, by replacing the existing port values with references to variables. The four new variables being used are KvmAgent.port, KvmAgent.prometheusPort, KvmAgent.nodeExportPort and kvmhost.pushgateway.webListenPort.

Now, let's look at potential risks:

  1. Make sure that all the variables being referenced actually exist and have valid values.
  2. Make sure that the new ports don't conflict with existing services on the system.
  3. Ensure that the ports are secure (for example, not allowing external access).

Suggestions for improvement:

  1. Consider using a more secure format for the defaultValue, like an array of port numbers.
  2. If possible, consider using environment variables instead of hard-coded values.
  3. If possible, consider using a configuration file for the port values instead of hard-coding them into the code.

Expand Down
2 changes: 0 additions & 2 deletions conf/springConfigXml/ConsoleManager.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,5 @@
<zstack:extension interface="org.zstack.header.console.ConsoleBackend" />
<zstack:extension interface="org.zstack.header.managementnode.ManagementNodeReadyExtensionPoint" />
</zstack:plugin>

<property name="agentPort" value="${ManagementServerConsoleProxyBackend.agentPort:7758}" />
</bean>
</beans>
Copy link

Choose a reason for hiding this comment

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

the review:

  1. The code looks alright, but there is a risk that the value of the property "agentPort" is not set correctly (i.e. the default value of 7758 is used). To improve the code, you should add a check for the value of the property before setting it.

  2. Also, it might be a good idea to add a comment to explain why the property is set and what its purpose is, as this will make it easier for someone else to understand the code in the future.

2 changes: 2 additions & 0 deletions console/src/main/java/org/zstack/console/ConsoleGlobalProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ public class ConsoleGlobalProperty {
public static String AGENT_PACKAGE_NAME;
@GlobalProperty(name="MN.network.", defaultValue = "")
public static List<String> MN_NETWORKS;
@GlobalProperty(name="ConsoleProxy.agentPort", defaultValue = "7758")
public static int AGENT_PORT;
}
Copy link

Choose a reason for hiding this comment

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

code review

  1. The code looks correct, but there is a potential bug risk regarding the AGENT_PORT variable. It should be checked to make sure it is an integer value and not any other data type like a string.

  2. The naming of the variables could be improved. For example, AGENT_PACKAGE_NAME and MN_NETWORKS are not descriptive enough. It would be better to name them something more specific like AGENT_PACKAGE_NAME_LIST and MN_NETWORKS_LIST.

  3. The default value for the AGENT_PORT variable should also be changed from '7758' to 7758, as it is currently set as a string which may cause issues with the code.

40 changes: 22 additions & 18 deletions console/src/main/java/org/zstack/console/ManagementServerConsoleProxyBackend.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
*/
public class ManagementServerConsoleProxyBackend extends AbstractConsoleProxyBackend {
private static final CLogger logger = Utils.getLogger(ManagementServerConsoleProxyBackend.class);
private int agentPort = 7758;
private String agentPackageName = ConsoleGlobalProperty.AGENT_PACKAGE_NAME;
private boolean connected = false;

Expand Down Expand Up @@ -91,7 +90,7 @@ protected int setConsoleProxyPort(int Port) {
}

protected ConsoleProxy getConsoleProxy(VmInstanceInventory vm, ConsoleProxyVO vo) {
return new ConsoleProxyBase(vo, getAgentPort());
return new ConsoleProxyBase(vo, ConsoleGlobalProperty.AGENT_PORT);
}

@Override
Expand All @@ -104,7 +103,7 @@ protected ConsoleProxy getConsoleProxy(SessionInventory session, VmInstanceInven
inv.setAgentType(getConsoleBackendType());
inv.setToken(session.getUuid() + "_" + vm.getUuid());
inv.setVmInstanceUuid(vm.getUuid());
return new ConsoleProxyBase(inv, getAgentPort());
return new ConsoleProxyBase(inv, ConsoleGlobalProperty.AGENT_PORT);
}

private void setupPublicKey() {
Expand Down Expand Up @@ -169,17 +168,17 @@ public void run() {
builder.append(String.format(";sudo iptables -w -I INPUT -m set --match-set ZS-MN src -p tcp -m comment --comment %s -m tcp --dport %s -j ACCEPT",
ConsoleConstants.VNC_IPTABLES_COMMENTS, dport));
}
builder.append(String.format(";sudo iptables -w -I INPUT -m set --match-set ZS-MN src -p tcp -m comment --comment %s -m tcp ! -s 127.0.0.1 --dport %s -j REJECT",
ConsoleConstants.VNC_IPTABLES_COMMENTS, agentPort));
builder.append(String.format(";sudo iptables -I INPUT -m set --match-set ZS-MN src -p tcp -m comment --comment %s -m tcp ! -s 127.0.0.1 --dport %s -j REJECT",
ConsoleConstants.VNC_IPTABLES_COMMENTS, ConsoleGlobalProperty.AGENT_PORT));
} else {
builder.append(String.format("sudo iptables-save | grep '%s' | while read LINE;do drule=${LINE//\\\"/};sudo iptables -w ${drule/-A/-D};done",
ConsoleConstants.VNC_IPTABLES_COMMENTS));
for (String dport : ports) {
builder.append(String.format(";sudo iptables -w -I INPUT -p tcp -m comment --comment %s -m tcp --dport %s -j ACCEPT",
ConsoleConstants.VNC_IPTABLES_COMMENTS, dport));
}
builder.append(String.format(";sudo iptables -w -I INPUT -p tcp -m comment --comment %s -m tcp ! -s 127.0.0.1 --dport %s -j REJECT",
ConsoleConstants.VNC_IPTABLES_COMMENTS, agentPort));
builder.append(String.format(";sudo iptables -I INPUT -p tcp -m comment --comment %s -m tcp ! -s 127.0.0.1 --dport %s -j REJECT",
ConsoleConstants.VNC_IPTABLES_COMMENTS, ConsoleGlobalProperty.AGENT_PORT));
}
ShellUtils.run(builder.toString());

Expand Down Expand Up @@ -208,15 +207,29 @@ public void run() {
runner.installChecker(checker);
runner.setUsername("root");
runner.setPrivateKey(privKey);
runner.setAgentPort(7758);
runner.setAgentPort(ConsoleGlobalProperty.AGENT_PORT);
runner.setTargetIp(Platform.getManagementServerIp());
runner.setTargetUuid(Platform.getManagementServerId());
runner.setPlayBookName(ANSIBLE_PLAYBOOK_NAME);
runner.setFullDeploy(fullDeploy);
<<<<<<< HEAD

ConsoleProxyDeployArguments deployArguments = new ConsoleProxyDeployArguments();
deployArguments.setHttpConsoleProxyPort(String.valueOf(CoreGlobalProperty.HTTP_CONSOLE_PROXY_PORT));
runner.setDeployArguments(deployArguments);
=======
runner.putArgument("pkg_consoleproxy", agentPackageName);
runner.putArgument("http_console_proxy_port", CoreGlobalProperty.HTTP_CONSOLE_PROXY_PORT);
runner.putArgument("console_proxy_agent_port", ConsoleGlobalProperty.AGENT_PORT);
if (CoreGlobalProperty.SYNC_NODE_TIME) {
if (CoreGlobalProperty.CHRONY_SERVERS == null || CoreGlobalProperty.CHRONY_SERVERS.isEmpty()) {
completion.fail(operr("chrony server not configured!"));
chain.next();
return;
}
runner.putArgument("chrony_servers", String.join(",", CoreGlobalProperty.CHRONY_SERVERS));
}
>>>>>>> 79bcc5c08d ([BugFix: ZSTACK-25869]internal-port)
runner.run(new ReturnValueCompletion<Boolean>(completion, chain) {
@Override
public void success(Boolean deployed) {
Expand Down Expand Up @@ -298,7 +311,7 @@ private void handleLocalMessage(Message msg) {

private void handle(PingConsoleProxyAgentMsg msg) {
ConsoleProxyCommands.PingCmd cmd = new ConsoleProxyCommands.PingCmd();
String url = URLBuilder.buildHttpUrl("127.0.0.1", agentPort, ConsoleConstants.CONSOLE_PROXY_PING_PATH);
String url = URLBuilder.buildHttpUrl("127.0.0.1", ConsoleGlobalProperty.AGENT_PORT, ConsoleConstants.CONSOLE_PROXY_PING_PATH);
ConsoleProxyAgentVO vo = dbf.findByUuid(Platform.getManagementServerId(), ConsoleProxyAgentVO.class);

boolean success;
Expand Down Expand Up @@ -574,13 +587,4 @@ public boolean start() {
tracker.track(Platform.getManagementServerId());
return super.start();
}


public int getAgentPort() {
return agentPort;
}

public void setAgentPort(int agentPort) {
this.agentPort = agentPort;
}
}
1 change: 1 addition & 0 deletions core/src/main/java/org/zstack/core/config/GlobalConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ public void run(Map tokens, Object data) {
private void update(String newValue, boolean localUpdate) {
// substitute system properties in newValue
newValue = StringTemplate.substitute(newValue, propertiesMap);
newValue = StringTemplate.substitute(newValue, Platform.getGlobalProperties());

validate(newValue);

Copy link

Choose a reason for hiding this comment

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

with the code patch

The first thing I would do is check for any potential security issues or vulnerabilities. Since the code is accessing global properties, I would make sure that these are properly sanitized and validated before being used. Additionally, I would investigate if the newValue parameter is properly escaped to prevent any type of code injection.

Next, I would review the logic of the code to make sure that it is doing what is expected. I would also check for any dead code or redundant operations.

Finally, I would consider if there is any potential for performance improvements. For example, are there any operations that can be cached or optimized?

Overall, the code looks like it is doing what it is supposed to, but it would be beneficial to do a thorough review in order to identify any potential problems.

Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/org/zstack/core/config/GlobalConfigFacadeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.zstack.core.config;

import org.springframework.beans.factory.annotation.Autowired;
import org.zstack.core.Platform;
import org.zstack.core.cloudbus.CloudBus;
import org.zstack.core.cloudbus.MessageSafe;
import org.zstack.core.componentloader.PluginRegistry;
Expand Down Expand Up @@ -224,6 +225,7 @@ private void loadConfigFromJava() {
}
// substitute system properties in defaultValue
String defaultValue = StringTemplate.substitute(d.defaultValue(), propertiesMap);
defaultValue = StringTemplate.substitute(defaultValue, Platform.getGlobalProperties());

GlobalConfig c = new GlobalConfig();
c.setCategory(config.getCategory());
Expand Down Expand Up @@ -512,11 +514,13 @@ private void parseConfig(File file) throws JAXBException {
throw new IllegalArgumentException(String.format("GlobalConfig[category:%s, name:%s] must have a default value", c.getCategory(), c.getName()));
} else {
c.setDefaultValue(StringTemplate.substitute(c.getDefaultValue(), propertiesMap));
c.setDefaultValue(StringTemplate.substitute(c.getDefaultValue(), Platform.getGlobalProperties()));
}
if (c.getValue() == null) {
c.setValue(c.getDefaultValue());
} else {
c.setValue(StringTemplate.substitute(c.getValue(), propertiesMap));
c.setValue(StringTemplate.substitute(c.getValue(), Platform.getGlobalProperties()));
}
GlobalConfig config = GlobalConfig.valueOf(c);
if (configsFromXml.containsKey(config.getIdentity())) {
Copy link

Choose a reason for hiding this comment

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

with a brief code review:

  1. The code adds a new import statement for the Platform class. This is necessary, as the code needs to reference the Platform class in order to substitute system properties in the default value of the GlobalConfig object.
  2. The code also adds new lines of code which uses the Platform class to substitute system properties in the default value and value of the GlobalConfig object. This ensures that the GlobalConfig object will have the correct, up-to-date values when it is created.
  3. The code also checks if the GlobalConfig object already exists in the configsFromXml map before adding it. This prevents duplicate entries in the map, which could potentially cause errors.

Expand Down
11 changes: 11 additions & 0 deletions plugin/kvm/src/main/java/org/zstack/kvm/KVMGlobalProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,18 @@ public class KVMGlobalProperty {
public static String TAKEVOERFLAGPATH;
@GlobalProperty(name="MN.network.", defaultValue = "")
public static List<String> MN_NETWORKS;
<<<<<<< HEAD
@GlobalProperty(name = "host.skip.packages", defaultValue = "qemu, qemu-kvm, qemu-kvm-ev, qemu-img, qemu-img-ev")
public static String SKIP_PACKAGES;

=======
@GlobalProperty(name="KvmAgent.prometheusPort", defaultValue = "7069")
public static int PROMETHEUS_PORT;
@GlobalProperty(name="KvmAgent.collectdExposePort", defaultValue = "9103")
public static int COLLECTD_EXPOSE_PORT;
@GlobalProperty(name="KvmAgent.nodeExportPort", defaultValue = "9100")
public static int NODE_EXPORT_PORT;
@GlobalProperty(name="KvmAgent.collectdAcceptPort", defaultValue = "25826")
public static int COLLECTD_ACCEPT_PORT;
>>>>>>> 79bcc5c08d ([BugFix: ZSTACK-25869]internal-port)
}
Copy link

Choose a reason for hiding this comment

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

:

  1. This code patch contains the addition of four global properties related to the KVM Agent.
  2. The code looks properly structured and commented, which is a good sign.
  3. It would be better if there is a validation of the property value like if the port number supplied is valid.
  4. The code should also check if the port numbers supplied are already in use or not.
  5. The code should also provide an error message if the port numbers are already in use.

6 changes: 6 additions & 0 deletions plugin/kvm/src/main/java/org/zstack/kvm/KVMHost.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4122,8 +4122,14 @@ public void run(final FlowTrigger trigger, Map data) {
ub.path(new StringBind(KVMConstant.KVM_ANSIBLE_LOG_PATH_FROMAT).bind("uuid", self.getUuid()).toString());
String postUrl = ub.build().toString();

<<<<<<< HEAD
deployArguments.setPostUrl(postUrl);
runner.setDeployArguments(deployArguments);
=======
runner.putArgument("post_url", postUrl);
runner.putArgument("kvmagent_prometheus_port", String.valueOf(KVMGlobalProperty.PROMETHEUS_PORT));
runner.putArgument("kvmagent_port", String.valueOf(KVMGlobalProperty.AGENT_PORT));
>>>>>>> 79bcc5c08d ([BugFix: ZSTACK-25869]internal-port)
runner.run(new ReturnValueCompletion<Boolean>(trigger) {
@Override
public void success(Boolean run) {
Copy link

Choose a reason for hiding this comment

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

with a brief code review of the above patch

The code patch adds additional arguments to the 'runner` object, specifically the postUrl and kvmagent_prometheus_port & kvmagent_port. Adding these parameters should improve the performance of the application by allowing for more efficient communication with the KVM agent. Additionally, it looks like the patch is also fixing an issue with the StringBind class, which could potentially lead to issues with the application. Overall, this patch looks like a positive change and should improve the performance of the application.

Expand Down
3 changes: 2 additions & 1 deletion plugin/loadBalancer/src/main/java/org/zstack/network/service/lb/LoadBalancerConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.zstack.network.service.lb;

import org.zstack.core.Platform;
import org.zstack.header.network.service.NetworkServiceType;
import org.zstack.header.vm.VmInstanceConstant;

Expand Down Expand Up @@ -64,7 +65,7 @@ public static enum HealthCheckStatusCode {

public static final int DNS_PORT = 53;
public static final int SSH_PORT = 22;
public static final int ZVR_PORT = 7272;
public static final int ZVR_PORT = Integer.parseInt(Platform.getGlobalProperties().get("VirtualRouter.agentPort"));

/*max concurrent connect no more than MAX_CONNECTION_LIMIT per listener*/
public static final long MAX_CONNECTION_LIMIT = 10000000;
Copy link

Choose a reason for hiding this comment

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

the code review:

  1. The code looks good, there is no bug risk.
  2. In line 65, instead of hardcoding the port, it is better to get the port from Platform.getGlobalProperties() which is more flexible and secure.

Expand Down