#439 HTTP_bind_by_NIC_interfacename #442
Conversation
G-Ork
commented
Jun 21, 2020
G-Ork
commented
- Delegate address resolving to an global service
- Added service for RegEx matching on IPs
- Added service for RegEx matching on interface names
- Extend DocBook for jvm-agent config
- Delegate address resolving to an global service - Added service for RegEx matching on IPs - Added service for RegEx matching on interface names - Extend DocBook for jvm-agent config
|
Thanks a lot for your contribution ! I have a busy week ahead but will reserve some time on Friday to work integrate the PR (and also think about what todo with Java 6 support) have a nice week ... |
| * Optional wich isn't available in Java 6. | ||
| * @throws RuntimeException If the configuration is invalid. | ||
| */ | ||
| public AtomicReference<InetAddress> optain(Map<String, String> agentConfig) throws RuntimeException; |
There was a problem hiding this comment.
Typo (obtain instead of optain). But please rename to something like "extractAddress" or so (fits better to the existing naming scheme)
There was a problem hiding this comment.
No need to declare that it throws a RuntimeException.
| * Konstructor | ||
| */ | ||
| public DelegatingAddressConfigService() { | ||
| super(); |
| public DelegatingAddressConfigService() { | ||
| super(); | ||
|
|
||
| this.services = Collections.unmodifiableList(Arrays.asList(new AddressConfigService[] { |
There was a problem hiding this comment.
You don't need to create an array, just use the varadic form of asList.
| AtomicReference<InetAddress> ref = service.optain(agentConfig); | ||
|
|
||
| if ( null != ref) { | ||
| this.address = ref.get(); |
There was a problem hiding this comment.
you should throw an error when no address could be detected (like before).
Btw, I wouldn't mind to inline the DelegatingAdressConfigService() here (i.e. iterate directly over the the various address resolvers), to skip one level of abstraction and also make clear that there is always a default value for the address. (i..e effectively ref can't be null, but that's not clear from the call)
| </td> | ||
| <td> | ||
| <constant>localhost</constant> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td><constant>ipmatch</constant></td> |
There was a problem hiding this comment.
I wonder whether we could avoid to a new configuration key like nicmatch or ipmatch and fold everything into the existing host variable. This approach would have multiple advantages:
- Multiple config-clashes would be avoided automatically
- Don't blow up the already huge configuration surface
So my suggestion would be to introduce multiples prefix like nic: or ip: which then trigger the different modes. Maybe even better just a match: if from the value it could be determined whether its a mac or an IP (by the existence of a : in the value ?)
Wdyt ?
|
Hi thanks for the review and your feedback. I've stubled over many of the topics you suggested during the implementation. I don't mind to consider your suggestion. It would be alot easier to reuse the existing key. A prefix would be the best workaround to extend the matching without conflict with the old style dots in ip matching and the RegEx dots in the alternative matching options. Did you see a need for supporting to classify ipv4 or ipv6? For myself I decided it for the whole JVM. So I don't need a config key to only bind ipv4. I would extend the documentation by a table with examples. This should be clearly to understand. I try to change it next week. |
