Fix handling of Windows paths in -javaagent config arguments (#312) #371
Conversation
|
|
||
| private static Pattern pattern = Pattern.compile( | ||
| "^(([\\w.]+):)?" + // optional host portion (not ipv6) | ||
| "(\\d{1,5})" + // port |
There was a problem hiding this comment.
Ports can be letters.
I'd suggest counting the colons, and if there's > 2, parsing them from left to right and what's left is the filename.
There was a problem hiding this comment.
Hmm? Can you clarify when a port can be letters? I'm not familiar with this concept.
There was a problem hiding this comment.
http is the same as 80, per /etc/services.
There was a problem hiding this comment.
Ah yes, I see. However I'm not sure that's possible in this code base since we are in Java. Java does not have an API to find ports by name. All the APIs that are used in this code base require a port to be an integer. For example all constructor calls to InetSocketAddress. Maybe that part can be added as a new feature down the road?
There was a problem hiding this comment.
So it is.
This code still isn't quite right. Someone can do [ipv6]:host:1234:path
There was a problem hiding this comment.
If you are talking about ipv6 addresses, I believe that functionality is captured. I left a Github comment on the unit test where this is demonstrated. That functionality was introduced by a previous pull request, and I left that logic as-is. That logic remains at the top of the parseConfig method. I tried to make these changes non-invasive.
There was a problem hiding this comment.
I'm talking specifically about the invalid input [ipv6]:host:1234:path, which should be rejected.
I tried to make these changes non-invasive.
It's more important that it works, and the end code is clean.
There was a problem hiding this comment.
Having [ipv6]:host:1234:path as accepted input was an existing issue in the codebase, but I did go ahead and fix it 😁. I also cleaned up the solution to use a single regex, instead of having separate logic for the ipv6 address.
jmx_prometheus_javaagent/src/test/java/io/prometheus/jmx/JavaAgentIT.java
Show resolved
Hide resolved
jmx_prometheus_javaagent/src/main/java/io/prometheus/jmx/JavaAgent.java
Outdated
Show resolved
Hide resolved
jmx_prometheus_javaagent/src/main/java/io/prometheus/jmx/JavaAgent.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private static Pattern pattern = Pattern.compile( | ||
| "^(?:((?:[\\w.]+)|(?:\\[.+])):)" + // host name, or ipv4, or ipv6 address in brackets |
There was a problem hiding this comment.
This is considered a redundant escape in Java.
|
|
||
| Config(String host, int port, String file, InetSocketAddress socket) { | ||
| this.host = host; | ||
| this.port = port; |
There was a problem hiding this comment.
Port and host aren't used.
There was a problem hiding this comment.
These are used for the unit tests.
Signed-off-by: Mikhail Dobrinin <mikhail.dobrinin@jda.com>
|
Thanks! |
|
Hmm, this change is making |
|
It is passing for me locally. Can you share the error? |
…eus#312) (prometheus#371) Signed-off-by: Mikhail Dobrinin <mikhail.dobrinin@jda.com>
This fixes Windows path handling.