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.

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

<fix>[conf]: internal port #1329

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

Conversation

MatheMatrix
Copy link
Member

Resolve: ZSTAC-123

Change-Id: I6e6c697a766368787476667875886f6e6e677974

Resolve: ZSTAC-123

Change-Id: I6e6c697a766368787476667875886f6e6e677974
@@ -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.

@@ -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.

@@ -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.

@@ -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.

@@ -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.

}
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.

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.

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.

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant