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

Feature 65611 #673

Closed

Conversation

cse-peter-account
Copy link

@cse-peter-account cse-peter-account commented Oct 3, 2021

This pull request implements Feature #65611 (https://bz.apache.org/bugzilla/show_bug.cgi?id=65611).

According to step 2 of the following URL, JMeter only accepts IPv4 addresses when specifying a remote worker node.

https://jmeter.apache.org/usermanual/remote-test.html

In this scenario, entering an IPv6 address results in a NumberFormatException. The purpose of this pull request is to eliminate this NumberFormatException.

port = Integer.parseInt(portAsString);
}
} else {
System.out.println("IPv6 address detected. No parsing is required.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this else block and sout be removed?

Copy link
Author

Choose a reason for hiding this comment

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

Done:

commit 2fd1f1e

Please kindly review. Thanks!

@FSchumacher
Copy link
Contributor

What happens, when a user wants to use IPv6 and a port?
Is it worth adding a complete library, to check for a IPv6 address, only?

@cse-peter-account
Copy link
Author

cse-peter-account commented Oct 4, 2021

What happens, when a user wants to use IPv6 and a port? Is it worth adding a complete library, to check for a IPv6 address, only?


The following URL said IPv6 ports are not needed in Java:

https://docs.oracle.com/javase/8/docs/technotes/guides/net/ipv6_guide/#:~:text=IPv6%20in%20Java%20is%20transparent%20and%20automatic.%20Porting%20is%20not%20necessary

Quote:
"IPv6 in Java is transparent and automatic. Porting is not necessary;"


I added a small, specialized library to check for IPv6, since that is what the library is specialized to do. If we add manual code to check for IPv6 format, human errors could be introduced either now or later. Also, if the IPv6 standard is revised in the future, the specialized library will take care of following the latest IPv6 format.

Revision History of IPv6:
https://www.arin.net/resources/guide/ipv6/history/


Please let me know what you think of the above URLs.

Thanks!

@FSchumacher
Copy link
Contributor

FSchumacher commented Oct 9, 2021

I think, you misread the docs. The porting does not mean the port part, but the recompilation/adaption of the java source, when using the Java API with IPv6.

What I meant is, that currently we can give JMeter an IP address plus a port in one string ("1.2.3.4:80" for IPv4 1.2.3.4 with port 80), which as you found out is problematic, when you specify an IPv6 address (as it contains colons, which are used currently to separate ip and port). Your solution drops the port part completely, which is bad (in my opinion) and probably unneeded.

What I have seen is, that if you want to specify an IPv6 plus port combination, you wrap the IPv6 address in square brackets like this: "[0::1]:80". If we use this standard notation, the parsing will be simplified to looking at the first non space char. If it is a opening square bracket ('['), we know, that the user wants to specify an IPv6 address. We can look for the closing bracket extract the IPv6 and are happy. (As a side effect, we can drop adding the new library and ask the normal Java API to confirm the validity of the IPv6 address)

What do you think of this patch:

diff --git a/src/core/src/main/java/org/apache/jmeter/engine/ClientJMeterEngine.java b/src/core/src/main/java/org/apache/jmeter/engine/ClientJMeterEngine.java
index 508d13e7bf..44a8d51294 100644
--- a/src/core/src/main/java/org/apache/jmeter/engine/ClientJMeterEngine.java
+++ b/src/core/src/main/java/org/apache/jmeter/engine/ClientJMeterEngine.java
@@ -58,7 +58,8 @@ public class ClientJMeterEngine implements JMeterEngine {
         final String name = RemoteJMeterEngineImpl.JMETER_ENGINE_RMI_NAME; // $NON-NLS-1$ $NON-NLS-2$
         String host = hostAndPort;
         int port = RmiUtils.DEFAULT_RMI_PORT;
-        int indexOfSeparator = hostAndPort.indexOf(':');
+        int closingBracket = hostAndPort.indexOf(']');
+        int indexOfSeparator = hostAndPort.indexOf(':', closingBracket);
         if (indexOfSeparator >= 0) {
             host = hostAndPort.substring(0, indexOfSeparator);
             String portAsString = hostAndPort.substring(indexOfSeparator+1);

@cse-peter-account
Copy link
Author

cse-peter-account commented Oct 10, 2021

I think, you misread the docs. The porting does not mean the port part, but the recompilation/adaption of the java source, when using the Java API with IPv6.

What I meant is, that currently we can give JMeter an IP address plus a port in one string ("1.2.3.4:80" for IPv4 1.2.3.4 with port 80), which as you found out is problematic, when you specify an IPv6 address (as it contains colons, which are used currently to separate ip and port). Your solution drops the port part completely, which is bad (in my opinion) and probably unneeded.

What I have seen is, that if you want to specify an IPv6 plus port combination, you wrap the IPv6 address in square brackets like this: "[0::1]:80". If we use this standard notation, the parsing will be simplified to looking at the first non space char. If it is a opening square bracket ('['), we know, that the user wants to specify an IPv6 address. We can look for the closing bracket extract the IPv6 and are happy. (As a side effect, we can drop adding the new library and ask the normal Java API to confirm the validity of the IPv6 address)

What do you think of this patch:

diff --git a/src/core/src/main/java/org/apache/jmeter/engine/ClientJMeterEngine.java b/src/core/src/main/java/org/apache/jmeter/engine/ClientJMeterEngine.java
index 508d13e7bf..44a8d51294 100644
--- a/src/core/src/main/java/org/apache/jmeter/engine/ClientJMeterEngine.java
+++ b/src/core/src/main/java/org/apache/jmeter/engine/ClientJMeterEngine.java
@@ -58,7 +58,8 @@ public class ClientJMeterEngine implements JMeterEngine {
         final String name = RemoteJMeterEngineImpl.JMETER_ENGINE_RMI_NAME; // $NON-NLS-1$ $NON-NLS-2$
         String host = hostAndPort;
         int port = RmiUtils.DEFAULT_RMI_PORT;
-        int indexOfSeparator = hostAndPort.indexOf(':');
+        int closingBracket = hostAndPort.indexOf(']');
+        int indexOfSeparator = hostAndPort.indexOf(':', closingBracket);
         if (indexOfSeparator >= 0) {
             host = hostAndPort.substring(0, indexOfSeparator);
             String portAsString = hostAndPort.substring(indexOfSeparator+1);

This patch looks good to me. Thanks!

@asfgit asfgit closed this in d074091 Oct 10, 2021
@FSchumacher
Copy link
Contributor

Thanks for the feature request and the initial implementation. It would be nice, if you could check next nightly and report back (probably better on bugzilla), if it works for you.

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.

3 participants