ESQL: Implement network_direction function
#7
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to ESQL by adding a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR implements a new Changes
Sequence DiagramsequenceDiagram
participant User as ESQL Query
participant NDF as NetworkDirection<br/>Function
participant NDU as NetworkDirectionUtils
participant IP as IP Classification<br/>Logic
User->>NDF: network_direction(src_ip, dst_ip, networks)
NDF->>NDF: toEvaluator()
NDF->>NDU: isInternal(networks, src_ip)
NDU->>IP: Check IP against<br/>named networks/CIDR
IP-->>NDU: source is internal?
NDF->>NDU: isInternal(networks, dst_ip)
NDU->>IP: Check IP against<br/>named networks/CIDR
IP-->>NDU: destination is internal?
NDF->>NDU: getDirection(src_internal,<br/>dst_internal)
NDU-->>NDF: inbound/outbound/<br/>internal/external
NDF-->>User: direction (keyword)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Rationale: The PR spans multiple interconnected concerns—a new utility class with substantial IP classification logic, a new ESQL scalar function with evaluators and type resolution, refactoring of an existing ingest processor, registry/capability integration, and extensive test coverage across 8+ test files. While many changes follow consistent patterns, the density of new logic (NetworkDirectionUtils), the ESQL function's evaluator implementation, and the variety of integration points (registry, writables, capabilities) require varied reasoning for each major component. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:XXL
|
/refacto-visz |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the network_direction function in ESQL, which is a great addition. The implementation correctly reuses logic from the existing NetworkDirectionProcessor by extracting it into a new NetworkDirectionUtils class. The changes are well-tested with new integration and unit tests.
I've found a few opportunities for performance improvements in the new utility class and the ESQL function's evaluator. These relate to avoiding repeated IP string parsing and optimizing loops. My detailed comments are below.
Overall, this is a solid contribution. Addressing the performance points will make it even better.
| public static boolean isInternal(List<String> networks, String ip) { | ||
| for (String network : networks) { | ||
| if (inNetwork(InetAddresses.forString(ip), network)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The isInternal method repeatedly parses the IP string inside the for loop by calling InetAddresses.forString(ip). This is inefficient, especially if the networks list is large. The IP string should be parsed into an InetAddress object once before the loop.
| public static boolean isInternal(List<String> networks, String ip) { | |
| for (String network : networks) { | |
| if (inNetwork(InetAddresses.forString(ip), network)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| public static boolean isInternal(List<String> networks, String ip) { | |
| InetAddress address = InetAddresses.forString(ip); | |
| for (String network : networks) { | |
| if (inNetwork(address, network)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
| case PRIVATE_NAMED_NETWORK -> isPrivate(NetworkAddress.format(address)); | ||
| case PUBLIC_NAMED_NETWORK -> isPublic(NetworkAddress.format(address)); | ||
| default -> CIDRUtils.isInRange(NetworkAddress.format(address), network); |
There was a problem hiding this comment.
There's a performance issue here due to InetAddress to String conversion. The inNetwork method accepts an InetAddress, but for private, public, and CIDR checks, it converts it back to a String using NetworkAddress.format(address). The downstream methods (isPrivate, isPublic, CIDRUtils.isInRange) then have to parse this string back into an InetAddress. This round-trip is inefficient.
Consider refactoring isPrivate, isPublic, and other helpers to work directly with InetAddress objects to avoid these unnecessary conversions. If CIDRUtils methods only accept strings, you might need to add overloads there or pass both the InetAddress and its string representation down.
There was a problem hiding this comment.
@coderabbitai what do you think is this comment valid
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of isPrivate and isPublic methods in NetworkDirectionUtils
cat server/src/main/java/org/elasticsearch/common/network/NetworkDirectionUtils.java
Unrecoverable error: Failed to clone repository
🏁 Script executed:
#!/bin/bash
# Find and examine CIDRUtils class implementation
fd -t f "CIDRUtils.java" --exec cat {}
Unrecoverable error: Failed to clone repository
🏁 Script executed:
#!/bin/bash
# Find and examine NetworkAddress.format method
fd -t f "NetworkAddress.java" --exec cat {}
Unrecoverable error: Failed to clone repository
🌐 Web query:
Elasticsearch NetworkAddress.format method what does it return
💡 Result:
The NetworkAddress.format methods return a human-friendly String representation of the address (or address+port/port-range). IPv4 prints as normal (e.g. "127.0.0.1"); IPv6 is compressed and scope ids are removed (e.g. "::1"). When a port is included the port is appended (IPv6 is bracketed) — e.g. "127.0.0.1:9300" or "[::1]:9300". For a PortsRange the range is shown in brackets (e.g. "127.0.0.1:[9300-9400]"). Any hostname information is ignored. [1][2]
Sources: Elasticsearch API docs and source javadoc. [1] (artifacts.elastic.co) [2] (fossies.org)
Citations:
- 1: https://artifacts.elastic.co/javadoc/org/elasticsearch/elasticsearch/8.16.4/org.elasticsearch.server/org/elasticsearch/common/network/NetworkAddress.html?utm_source=openai
- 2: https://fossies.org/dox/elasticsearch-7.17.29/classorg_1_1elasticsearch_1_1common_1_1network_1_1NetworkAddress.html?utm_source=openai
🌐 Web query:
Java InetAddress to String conversion performance overhead
💡 Result:
Short answer: converting an InetAddress to a String is cheap when no name resolution is required, but can be expensive (blocks on network/DNS) if the call triggers a reverse DNS lookup for the hostname. Use getHostAddress() (or format the raw address bytes) when you only need the numeric IP to avoid DNS latency; avoid getHostName()/getCanonicalHostName() in hot paths unless you control caching/TTL. [1][2][3]
Supporting details (very brief)
- InetAddress.toString() returns "hostname / literal IP". If the InetAddress already contains a hostname no lookup is performed; otherwise toString will not perform a reverse lookup. getHostName() or getCanonicalHostName() may perform reverse DNS and block. [1]
- DNS lookups are network calls and can be slow; JVM DNS caching (networkaddress.cache.*) affects how often lookups happen but will not eliminate the initial cost. Many reports/answers show getHostName/getCanonicalHostName can be the source of large latency in real apps. [1][2][3]
Practical recommendations
- If you only need the IP: call getHostAddress() (or construct the string from getAddress()).
- If you need a hostname but must avoid blocking, resolve once asynchronously and cache results yourself or tune JVM DNS cache TTLs.
- For microbenchmarks, test on your environment (DNS, JVM version, cache settings) because behavior and caching defaults can vary. [1][2]
Sources:
[1] Oracle InetAddress Javadoc (toString/getHostName and caching).
[2] StackOverflow discussion: InetAddress.getHostName() slow / causes network lookup.
[3] Analysis/demo of JVM reverse-lookup/caching behavior.
🏁 Script executed:
#!/bin/bash
# Search for isPrivate and isPublic method definitions in NetworkDirectionUtils
rg -n "private static.*isPrivate|private static.*isPublic" --type java -A 10
Unrecoverable error: Failed to clone repository
🏁 Script executed:
#!/bin/bash
# Search for CIDRUtils.isInRange method signature and implementation
rg -n "isInRange" --type java -A 5 -B 2
Unrecoverable error: Failed to clone repository
🏁 Script executed:
#!/bin/bash
# Look for InetAddresses.forString usage to understand parsing patterns
rg -n "InetAddresses\.forString" --type java -C 3
Unrecoverable error: Failed to clone repository
@visz11 The comment raises a structurally valid concern about potential inefficiency, though I cannot definitively confirm the full impact without seeing the implementations of isPrivate(), isPublic(), and CIDRUtils.isInRange().
Here's what makes the concern valid:
-
Inconsistent pattern: Most branches (
isLoopback,isUnicast, etc.) work directly withInetAddress, but three cases convert toString -
Double conversion in the call chain:
isInternal()receives aStringIP- Converts it to
InetAddress(line 40) - For private/public/CIDR cases, converts back to
String(line 56, 57, 58) - If those methods then parse back to
InetAddress, that's wasteful
-
Performance consideration: While
InetAddress.getHostAddress()is relatively fast for numeric IPs (no DNS lookup), unnecessary String↔InetAddress round-trips add allocation and parsing overhead, especially in hot paths
To verify and address this:
- Check if
isPrivate(),isPublic(), andCIDRUtils.isInRange()parse their String arguments back toInetAddress - If they do, consider refactoring to accept
InetAddressdirectly - If
CIDRUtilsonly has String-based APIs, evaluate whether addingInetAddressoverloads would benefit performance
The architectural point about minimizing conversions is sound engineering practice, particularly for utility methods that may be called frequently. I'd recommend investigating the downstream implementations to determine if refactoring is warranted.
| boolean sourceInternal = false; | ||
| boolean destinationInternal = false; | ||
|
|
||
| for (int i = first; i < first + valueCount; i++) { | ||
| if (NetworkDirectionUtils.inNetwork(sourceIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | ||
| sourceInternal = true; | ||
| break; | ||
| } | ||
| } | ||
| for (int i = first; i < first + valueCount; i++) { | ||
| if (NetworkDirectionUtils.inNetwork(destinationIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | ||
| destinationInternal = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to determine if the source and destination IPs are internal involves two separate loops over the networks block. This is inefficient as it iterates through the list of networks twice in the worst case. You can combine these two loops into a single one to improve performance.
| boolean sourceInternal = false; | |
| boolean destinationInternal = false; | |
| for (int i = first; i < first + valueCount; i++) { | |
| if (NetworkDirectionUtils.inNetwork(sourceIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | |
| sourceInternal = true; | |
| break; | |
| } | |
| } | |
| for (int i = first; i < first + valueCount; i++) { | |
| if (NetworkDirectionUtils.inNetwork(destinationIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | |
| destinationInternal = true; | |
| break; | |
| } | |
| } | |
| boolean sourceInternal = false; | |
| boolean destinationInternal = false; | |
| for (int i = first; i < first + valueCount; i++) { | |
| String network = networks.getBytesRef(i, netScratch).utf8ToString(); | |
| if (sourceInternal == false && NetworkDirectionUtils.inNetwork(sourceIpAddress, network)) { | |
| sourceInternal = true; | |
| } | |
| if (destinationInternal == false && NetworkDirectionUtils.inNetwork(destinationIpAddress, network)) { | |
| destinationInternal = true; | |
| } | |
| if (sourceInternal && destinationInternal) { | |
| break; | |
| } | |
| } |
ESQL: Implement
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionTests.java (1)
33-98: Add multi-valued networks and type-acceptance coverage
- Include cases where internal_networks contains multiple entries (e.g., ["private","10.0.0.0/8"]) to exercise MV iteration and short-circuiting.
- Please verify that iterating over DataType.stringTypes() won’t introduce TEXT that fails the function’s isStringAndExact resolution. If TEXT isn’t “exact” here, constrain tests to KEYWORD.
server/src/main/java/org/elasticsearch/common/network/NetworkDirectionUtils.java (3)
47-60: Validate named-network semantics: global_unicast vs unicast, and public including multicast
- GLOBAL_UNICAST currently maps to isUnicast(address), same as UNICAST. Typically, “global unicast” should exclude private/ULA ranges. Consider:
- case GLOBAL_UNICAST_NAMED_NETWORK -> isUnicast(address) && isPublic(NetworkAddress.format(address));
- PUBLIC uses isPublic(String) which doesn’t exclude non-local multicast. If “public” is intended to mean publicly routable unicast, consider excluding multicast:
- return isLocalOrPrivate(ip) == false && isMulticast(InetAddresses.forString(ip)) == false;
Please verify against the ingest processor docs and prior behavior.
108-116: IPv6 private range may need fc00::/7, not only fd00::/8Current private IPv6 check only includes fd00::/8. ULA is defined as fc00::/7. If intentional to exclude fc00::/8, please document it; otherwise expand the range.
47-60: Be tolerant to whitespace/case in named networksUsers may provide names with varying case/extra spaces. To harden, normalize network before the switch (trim + toLowerCase(Locale.ROOT)).
Apply within inNetwork:
- public static boolean inNetwork(InetAddress address, String network) { - return switch (network) { + public static boolean inNetwork(InetAddress address, String network) { + final String n = network == null ? "" : network.trim().toLowerCase(java.util.Locale.ROOT); + return switch (n) { case LOOPBACK_NAMED_NETWORK -> isLoopback(address); ...modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/NetworkDirectionProcessor.java (1)
121-125: Normalize network names before classificationTrim and lowercase the configured networks to avoid case/whitespace issues for named networks; CIDR ranges are unaffected.
- boolean sourceInternal = NetworkDirectionUtils.isInternal(networks, sourceIpAddrString); - boolean destinationInternal = NetworkDirectionUtils.isInternal(networks, destIpAddrString); + final List<String> normalized = networks.stream() + .map(s -> s == null ? null : s.trim()) + .filter(s -> s != null && s.isEmpty() == false) + .map(s -> s.toLowerCase(java.util.Locale.ROOT)) + .toList(); + boolean sourceInternal = NetworkDirectionUtils.isInternal(normalized, sourceIpAddrString); + boolean destinationInternal = NetworkDirectionUtils.isInternal(normalized, destIpAddrString);x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirection.java (1)
120-133: Optional: avoid per-row allocations by passing fixed 4/16 byte buffersTo keep zero-GC, pass two fixed buffers into the evaluator and select by length.
- return new NetworkDirectionEvaluator.Factory( + return new NetworkDirectionEvaluator.Factory( source(), - context -> new BytesRef(16), + context -> new byte[16], // scratch16 + context -> new byte[4], // scratch4 context -> new BytesRef(), sourceIpEvaluatorSupplier, destinationIpEvaluatorSupplier, internalNetworksEvaluatorSupplier );And update the @evaluator signature and IP decoding:
- static void process( - BytesRefBlock.Builder builder, - @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef scratch, - @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef netScratch, + static void process( + BytesRefBlock.Builder builder, + @Fixed(includeInToString = false, scope = THREAD_LOCAL) byte[] scratch16, + @Fixed(includeInToString = false, scope = THREAD_LOCAL) byte[] scratch4, + @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef netScratch, BytesRef sourceIp, BytesRef destinationIp, @Position int position, BytesRefBlock networks ) { - System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length); - InetAddress sourceIpAddress = InetAddressPoint.decode(scratch.bytes); + byte[] sbuf = sourceIp.length == 4 ? scratch4 : scratch16; + System.arraycopy(sourceIp.bytes, sourceIp.offset, sbuf, 0, sourceIp.length); + InetAddress sourceIpAddress = InetAddressPoint.decode(sbuf); - System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length); - InetAddress destinationIpAddress = InetAddressPoint.decode(scratch.bytes); + byte[] dbuf = destinationIp.length == 4 ? scratch4 : scratch16; + System.arraycopy(destinationIp.bytes, destinationIp.offset, dbuf, 0, destinationIp.length); + InetAddress destinationIpAddress = InetAddressPoint.decode(dbuf);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/reference/query-languages/esql/images/functions/network_direction.svgis excluded by!**/*.svgx-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/argument/BlockArgument.javais excluded by!**/gen/**x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionEvaluator.javais excluded by!**/generated/**
📒 Files selected for processing (20)
docs/changelog/136133.yaml(1 hunks)docs/reference/query-languages/esql/_snippets/functions/description/network_direction.md(1 hunks)docs/reference/query-languages/esql/_snippets/functions/examples/network_direction.md(1 hunks)docs/reference/query-languages/esql/_snippets/functions/layout/network_direction.md(1 hunks)docs/reference/query-languages/esql/_snippets/functions/parameters/network_direction.md(1 hunks)docs/reference/query-languages/esql/_snippets/functions/types/network_direction.md(1 hunks)docs/reference/query-languages/esql/kibana/definition/functions/network_direction.json(1 hunks)docs/reference/query-languages/esql/kibana/docs/functions/network_direction.md(1 hunks)modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/NetworkDirectionProcessor.java(2 hunks)modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/NetworkDirectionProcessorTests.java(1 hunks)server/src/main/java/org/elasticsearch/common/network/NetworkDirectionUtils.java(1 hunks)server/src/test/java/org/elasticsearch/common/network/NetworkDirectionUtilsTests.java(1 hunks)x-pack/plugin/esql/qa/testFixtures/src/main/resources/ip.csv-spec(1 hunks)x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java(2 hunks)x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java(2 hunks)x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ScalarFunctionWritables.java(2 hunks)x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirection.java(1 hunks)x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionErrorTests.java(1 hunks)x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionSerializationTests.java(1 hunks)x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionTests.java(1 hunks)
🔇 Additional comments (19)
modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/NetworkDirectionProcessorTests.java (1)
96-96: LGTM! Helpful cross-reference comment.The comment appropriately documents that test data is copied from NetworkDirectionUtils tests, improving maintainability by clarifying the relationship between the two test suites.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java (2)
1504-1507: LGTM! Properly gated capability addition.The
NETWORK_DIRECTIONcapability is correctly:
- Snapshot-gated with
Build.current().isSnapshot()- Documented with a clear description
- Placed logically within the enum sequence
1517-1517: LGTM! Correct enum termination.The semicolon terminator is properly updated to reflect the addition of the new capability constant.
x-pack/plugin/esql/qa/testFixtures/src/main/resources/ip.csv-spec (1)
733-825: LGTM! Comprehensive test coverage for network_direction function.The test fixtures cover:
- ✓ Basic direction classification (internal, inbound, outbound, external)
- ✓ Function alias (
NETDIR)- ✓ Inline network arrays
- ✓ Integration with
IP_PREFIXfunction- ✓ Null IP handling
- ✓ Invalid network input with proper warning patterns
The tests follow established patterns and include proper capability gating with
required_capability: network_direction.docs/changelog/136133.yaml (1)
1-6: LGTM! Properly formatted changelog entry.The changelog entry correctly documents the enhancement with:
- Clear summary describing the function implementation
- Appropriate area classification (ES|QL)
- Correct type (enhancement)
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ScalarFunctionWritables.java (2)
28-28: LGTM! Import properly placed.The
NetworkDirectionimport is correctly positioned in alphabetical order within theippackage imports.
102-102: LGTM! Registry entry correctly added.The
NetworkDirection.ENTRYis properly registered in theNamedWriteableRegistrylist, maintaining alphabetical order (betweenMd5andNow).docs/reference/query-languages/esql/_snippets/functions/examples/network_direction.md (1)
1-14: LGTM! Auto-generated documentation example.The example demonstrates typical
NETWORK_DIRECTIONusage with:
- IP casting syntax (
"127.0.0.1"::ip)- Inline network array (
["loopback", "private"])- Clear expected output showing the direction result
The file includes proper notice that it's auto-generated by ESQL's test framework.
docs/reference/query-languages/esql/kibana/docs/functions/network_direction.md (1)
1-10: LGTM! Auto-generated Kibana documentation.The documentation clearly describes the
NETWORK_DIRECTIONfunction:
- Accurate description of the four direction types (inbound, outbound, internal, external)
- Clear parameter explanation (source IP, destination IP, internal networks)
- Practical example demonstrating usage
The file includes proper notice that it's auto-generated.
docs/reference/query-languages/esql/_snippets/functions/parameters/network_direction.md (1)
1-14: LGTM! Auto-generated parameter documentation.The parameter documentation is clear and comprehensive:
source_ipanddestination_ip: Both support IPv4 and IPv6internal_networks: Properly documents support for IPv4/IPv6 addresses, CIDR notation, and named rangesThe file includes proper notice that it's auto-generated.
docs/reference/query-languages/esql/_snippets/functions/layout/network_direction.md (1)
1-24: LGTM! Generated documentation follows standard pattern.This auto-generated layout file correctly includes all necessary documentation sections (parameters, description, types, examples) and follows the established pattern for ESQL function documentation.
docs/reference/query-languages/esql/_snippets/functions/types/network_direction.md (1)
1-9: LGTM! Type signatures are correct.The supported types correctly show two overload-like signatures accepting either
keywordortextfor theinternal_networksparameter, while requiringiptypes for source and destination, and returning akeywordresult.docs/reference/query-languages/esql/_snippets/functions/description/network_direction.md (1)
1-6: LGTM! Clear and concise description.The function description accurately explains the purpose and parameters of the
network_directionfunction.server/src/test/java/org/elasticsearch/common/network/NetworkDirectionUtilsTests.java (1)
19-47: LGTM! Comprehensive test coverage.The tests effectively cover key scenarios:
- CIDR notation handling for outbound/inbound classification
- Special unspecified addresses (0.0.0.0, ::)
- Private network detection across IPv4 and IPv6
- Public network filtering
The helper method correctly validates both the intermediate
isInternalchecks and the finalgetDirectionresult.x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionErrorTests.java (1)
22-41: LGTM! Error test follows established pattern.The error test correctly:
- Integrates with
NetworkDirectionTests.parameters()for test cases- Constructs
NetworkDirectionwith the proper three arguments- Maps argument positions to expected type names ("ip" for positions 0-1, "string" for position 2)
This ensures proper type error messaging for the function.
docs/reference/query-languages/esql/kibana/definition/functions/network_direction.json (1)
1-61: LGTM! Kibana definition is complete and accurate.The JSON definition correctly specifies:
- Two signature variants (keyword vs text for
internal_networks)- Clear parameter descriptions mentioning IPv4/IPv6 support
- Appropriate return type (
keyword)- A demonstrative example
- Correct flags indicating this is a production-ready function
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirectionSerializationTests.java (1)
16-41: LGTM! Serialization test correctly implements required methods.The test properly:
- Creates test instances with randomized source and child expressions
- Mutates each of the three expression fields (sourceIpField, destinationIpField, internalNetworks)
- Uses
randomValueOtherThanto ensure mutations produce different valuesThis ensures
NetworkDirectionserialization is thoroughly tested.x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/EsqlFunctionRegistry.java (2)
114-114: LGTM! Import added in correct location.The
NetworkDirectionimport is properly placed in the IP functions import section.
463-463: Verify the constructor reference pattern.Most three-argument functions in this file use the
tri()helper (e.g.,Clampat line 348,Topat line 359). This registration uses a direct constructor referenceNetworkDirection::new.Please verify that:
- The
NetworkDirectionclass has a constructor matching the expected signature for direct reference registration, OR- The direct constructor reference correctly resolves to a ternary builder
Run the following script to check the NetworkDirection constructor signatures:
| System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length); | ||
| InetAddress sourceIpAddress = InetAddressPoint.decode(scratch.bytes); | ||
| System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length); | ||
| InetAddress destinationIpAddress = InetAddressPoint.decode(scratch.bytes); | ||
|
|
There was a problem hiding this comment.
Critical: IPv4 decode uses 16-byte buffer → mis-decoding
InetAddressPoint.decode(byte[]) uses the array length (4 vs 16) to determine IPv4 vs IPv6. Copying 4 bytes into a 16-byte buffer and calling decode(scratch.bytes) will decode as IPv6, yielding incorrect results.
Minimal correctness fix:
- System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length);
- InetAddress sourceIpAddress = InetAddressPoint.decode(scratch.bytes);
- System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length);
- InetAddress destinationIpAddress = InetAddressPoint.decode(scratch.bytes);
+ byte[] srcArr = java.util.Arrays.copyOfRange(
+ sourceIp.bytes, sourceIp.offset, sourceIp.offset + sourceIp.length
+ );
+ InetAddress sourceIpAddress = InetAddressPoint.decode(srcArr);
+ byte[] dstArr = java.util.Arrays.copyOfRange(
+ destinationIp.bytes, destinationIp.offset, destinationIp.offset + destinationIp.length
+ );
+ InetAddress destinationIpAddress = InetAddressPoint.decode(dstArr);Follow-up: If you want to avoid per-row allocations, I can provide a refactor using two @fixed buffers (4 and 16 bytes) selected by length.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length); | |
| InetAddress sourceIpAddress = InetAddressPoint.decode(scratch.bytes); | |
| System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length); | |
| InetAddress destinationIpAddress = InetAddressPoint.decode(scratch.bytes); | |
| byte[] srcArr = java.util.Arrays.copyOfRange( | |
| sourceIp.bytes, sourceIp.offset, sourceIp.offset + sourceIp.length | |
| ); | |
| InetAddress sourceIpAddress = InetAddressPoint.decode(srcArr); | |
| byte[] dstArr = java.util.Arrays.copyOfRange( | |
| destinationIp.bytes, destinationIp.offset, destinationIp.offset + destinationIp.length | |
| ); | |
| InetAddress destinationIpAddress = InetAddressPoint.decode(dstArr); |
🤖 Prompt for AI Agents
In
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/ip/NetworkDirection.java
around lines 152-156, the code copies 4-byte IPv4 data into a 16-byte scratch
buffer and calls InetAddressPoint.decode(scratch.bytes), which mis-identifies
IPv4 as IPv6; fix by calling decode with a byte array whose length exactly
equals the IP length (i.e., copy only sourceIp.length/destinationIp.length into
a new byte[length] and pass that trimmed array to InetAddressPoint.decode), or
if an overload exists use a decode variant that accepts offset+length to avoid
the ambiguity; optionally note you can refactor later to use two fixed buffers
(4 and 16 bytes) selected by length to avoid per-row allocations.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| boolean sourceInternal = NetworkDirectionUtils.isInternal(networks, sourceIpAddrString); | ||
| boolean destinationInternal = NetworkDirectionUtils.isInternal(networks, destIpAddrString); | ||
|
|
||
| private static boolean isLocalOrPrivate(String ip) { | ||
| var address = InetAddresses.forString(ip); | ||
| return isPrivate(ip) | ||
| || isLoopback(address) | ||
| || isUnspecified(address) | ||
| || isLinkLocalUnicast(address) | ||
| || isLinkLocalMulticast(address) | ||
| || isInterfaceLocalMulticast(address) | ||
| || Arrays.equals(address.getAddress(), BROADCAST_IP4); | ||
| return NetworkDirectionUtils.getDirection(sourceInternal, destinationInternal); |
There was a problem hiding this comment.
Suggestion: Catch and handle potential parsing/validation errors from NetworkDirectionUtils.isInternal (e.g., invalid IP formats) and respect the processor's ignoreMissing behavior by returning null when appropriate, otherwise rethrow the exception. [possible bug]
| boolean sourceInternal = NetworkDirectionUtils.isInternal(networks, sourceIpAddrString); | |
| boolean destinationInternal = NetworkDirectionUtils.isInternal(networks, destIpAddrString); | |
| private static boolean isLocalOrPrivate(String ip) { | |
| var address = InetAddresses.forString(ip); | |
| return isPrivate(ip) | |
| || isLoopback(address) | |
| || isUnspecified(address) | |
| || isLinkLocalUnicast(address) | |
| || isLinkLocalMulticast(address) | |
| || isInterfaceLocalMulticast(address) | |
| || Arrays.equals(address.getAddress(), BROADCAST_IP4); | |
| return NetworkDirectionUtils.getDirection(sourceInternal, destinationInternal); | |
| try { | |
| boolean sourceInternal = NetworkDirectionUtils.isInternal(networks, sourceIpAddrString); | |
| boolean destinationInternal = NetworkDirectionUtils.isInternal(networks, destIpAddrString); | |
| return NetworkDirectionUtils.getDirection(sourceInternal, destinationInternal); | |
| } catch (IllegalArgumentException e) { | |
| if (ignoreMissing) { | |
| return null; | |
| } | |
| throw e; | |
| } |
Why Change? ⭐
The improved code is a small, local change inside the existing getDirection(...) method and uses only symbols already present in the class: the imported NetworkDirectionUtils and the instance field ignoreMissing. The try block contains the same calls and local variables as the original code, so there is no scope leakage or use of undefined identifiers.
Catching IllegalArgumentException is a conservative choice for handling parsing/validation failures from NetworkDirectionUtils.isInternal (which commonly signals invalid input via IllegalArgumentException). When such an exception occurs, returning null if ignoreMissing is true preserves the processor's existing semantics for missing/invalid inputs (the method already returns null in other missing-value cases). If ignoreMissing is false the exception is rethrown, preserving failure behavior.
The code is syntactically valid Java, compiles in-place (no new imports or signatures required), and does not introduce new checked exceptions. Assumptions: NetworkDirectionUtils and the ignoreMissing field exist as shown in the PR and NetworkDirectionUtils.isInternal may throw IllegalArgumentException for invalid inputs. Given these, the change is safe and executable and does not introduce new bugs.
| for (String network : networks) { | ||
| if (inNetwork(InetAddresses.forString(ip), network)) { |
There was a problem hiding this comment.
Suggestion: Validate inputs and guard against null or invalid ip in isInternal to avoid exceptions when InetAddresses.forString(ip) is called and to handle a null/empty networks list. [possible issue]
| for (String network : networks) { | |
| if (inNetwork(InetAddresses.forString(ip), network)) { | |
| if (networks == null || networks.isEmpty() || ip == null) { | |
| return false; | |
| } | |
| InetAddress address; | |
| try { | |
| address = InetAddresses.forString(ip); | |
| } catch (IllegalArgumentException e) { | |
| // invalid IP string -> treat as not internal | |
| return false; | |
| } | |
| for (String network : networks) { | |
| if (inNetwork(address, network)) { |
Why Change? ⭐
The improved code adds defensive checks for null/empty networks and a null ip, ensuring we don't iterate null lists or pass null into InetAddresses.forString.
It also wraps parsing in a try/catch for IllegalArgumentException thrown by InetAddresses.forString, returning false for invalid IP strings rather than propagating an exception.
All referenced symbols (InetAddresses, InetAddress, inNetwork) are already present/used elsewhere in this file, so compilation is not impacted.
The control flow and return semantics remain consistent: if any network contains the parsed address we return true, otherwise false. This avoids potential runtime NPEs/IllegalArgumentException without introducing new dependencies or changing the method's outward contract except to be more tolerant of null/invalid inputs.
| private void testNetworkDirectionUtils(String source, String destination, List<String> networks, String expectedDirection) { | ||
| boolean sourceInternal = NetworkDirectionUtils.isInternal(networks, source); | ||
| boolean destinationInternal = NetworkDirectionUtils.isInternal(networks, destination); | ||
| assertThat(expectedDirection, equalTo(NetworkDirectionUtils.getDirection(sourceInternal, destinationInternal))); |
There was a problem hiding this comment.
Suggestion: Use the computed direction as the "actual" argument to assertThat and keep the expected value as the matcher to produce clearer failure messages (i.e., swap the current assertThat arguments). [best practice]
| assertThat(expectedDirection, equalTo(NetworkDirectionUtils.getDirection(sourceInternal, destinationInternal))); | |
| assertThat(NetworkDirectionUtils.getDirection(sourceInternal, destinationInternal), equalTo(expectedDirection)); |
Why Change? ⭐
The improved code only swaps the arguments to assertThat so the actual value (the result of NetworkDirectionUtils.getDirection(...)) is passed first and the matcher equalTo(expectedDirection) second. This matches the standard Hamcrest assertThat(actual, matcher) ordering, yields clearer failure messages, and does not change any logic. Types align (both are String), the used methods and imports already exist in the file (NetworkDirectionUtils and equalTo), and no new APIs or context are required. The change is syntactic and executable and will not introduce runtime errors.
| @@ -0,0 +1,5 @@ | |||
| pr: 136133 | |||
| summary: Implement `network_direction` function | |||
| area: ES|QL | |||
There was a problem hiding this comment.
Suggestion: Quote the area value (which contains a pipe character) to avoid YAML parsing ambiguity and ensure consistent cross-platform parsing. [best practice]
| area: ES|QL | |
| area: "ES|QL" |
Why Change? ⭐
Quoting the 'area' scalar is a harmless, backward-compatible change that prevents any YAML parsers
or tooling from misinterpreting the '|' character. The improved code preserves the exact string value
("ES|QL") while ensuring the YAML remains unambiguous across different parsers and platforms.
The syntax is valid YAML, no new identifiers or methods are referenced, and the document remains
structurally identical aside from the added quotes. Therefore this change is executable and safe.
| **Description** | ||
|
|
||
| Returns the direction type (inbound, outbound, internal, external) given a source IP address, destination IP address, and a list of internal networks. | ||
|
|
There was a problem hiding this comment.
Suggestion: Enrich the snippet with parameter descriptions and a short usage example to make the function purpose and input expectations clearer for readers. [enhancement]
| **Parameters** | |
| - `src_ip` (string): source IP address. | |
| - `dst_ip` (string): destination IP address. | |
| - `internal_networks` (array<string>): list of CIDR ranges considered internal (e.g. `"10.0.0.0/8"`). | |
| **Example** | |
| ```esql | |
| network_direction("10.0.0.5", "8.8.8.8", ["10.0.0.0/8"]) -- returns "outbound" |
<details>
<summary><b>Why Change? ⭐ </b></summary>
This suggestion only augments a markdown snippet with additional documentation (parameters and an example). It does not alter any executable code or references to programmatic APIs, so there is no risk of introducing runtime bugs.
The improved content uses standard markdown and a fenced code block labelled `esql`, which is consistent with documentation formatting and fully comprehensible from the PR diff. It does not reference any undefined variables, methods, or types from the codebase.
Assumptions:
- The file is a documentation snippet and not executed during runtime.
- The described function name (`network_direction`) and parameter semantics match the implementation referenced by the snippet (the change is purely descriptive).
Given these points, the change is safe, executable as documentation, and improves clarity without affecting production code.
</details>
| | source_ip | destination_ip | internal_networks | result | | ||
| | --- | --- | --- | --- | | ||
| | ip | ip | keyword | keyword | | ||
| | ip | ip | text | keyword | |
There was a problem hiding this comment.
Suggestion: Use inline code formatting for column names and type entries (e.g., source_ip, keyword) to improve readability and consistency in API/type docs. [best practice]
| | source_ip | destination_ip | internal_networks | result | | |
| | --- | --- | --- | --- | | |
| | ip | ip | keyword | keyword | | |
| | ip | ip | text | keyword | | |
| | `source_ip` | `destination_ip` | `internal_networks` | `result` | | |
| | --- | --- | --- | --- | | |
| | `ip` | `ip` | `keyword` | `keyword` | | |
| | `ip` | `ip` | `text` | `keyword` | |
Why Change? ⭐
This suggestion updates only the Markdown formatting of the table cells by surrounding names and types with
inline code backticks. It is purely a documentation styling change; it does not alter any program logic or data.
Markdown supports inline code using backticks, so rendering will be unaffected and no runtime errors can be introduced.
Assumes the file is consumed as Markdown documentation (consistent with the file path and extension).
| }, | ||
| { | ||
| "name" : "internal_networks", | ||
| "type" : "keyword", |
There was a problem hiding this comment.
Suggestion: Treat internal_networks as an array type in the first signature (e.g., keyword[]) since the description says it's a list of networks. [possible issue]
| "type" : "keyword", | |
| "type" : "keyword[]", |
Why Change? ⭐
The change is a JSON-only documentation/schema update: replacing the type string "keyword" with "keyword[]"
makes the intent (a list/array of keywords) explicit and matches the parameter description that states
"List of internal networks". This is syntactically valid JSON and does not introduce executable code changes.
Assumptions:
- The file is a declarative function definition (documentation/metadata) and is allowed to use array notation
in the "type" field. - No other parts of the PR require the type string to be exactly "keyword" (if they do, those parts would need
coordinated updates). Given only the diff for this JSON file, the change is safe and consistent.
Code Review: Network Direction Function Implementation👍 Well Done
📁 Selected files for review (22)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length); | ||
| InetAddress sourceIpAddress = InetAddressPoint.decode(scratch.bytes); | ||
| System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length); | ||
| InetAddress destinationIpAddress = InetAddressPoint.decode(scratch.bytes); |
There was a problem hiding this comment.
Array Copy Risk
Array copy operations overwrite scratch buffer without bounds checking. Buffer overflow possible if IP address length exceeds scratch buffer capacity. System crash or memory corruption results from buffer overflow scenarios.
if (sourceIp.length > scratch.bytes.length) {
throw new IllegalArgumentException("Source IP length exceeds buffer capacity");
}
System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length);
InetAddress sourceIpAddress = InetAddressPoint.decode(Arrays.copyOf(scratch.bytes, sourceIp.length));
if (destinationIp.length > scratch.bytes.length) {
throw new IllegalArgumentException("Destination IP length exceeds buffer capacity");
}
System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length);
InetAddress destinationIpAddress = InetAddressPoint.decode(Arrays.copyOf(scratch.bytes, destinationIp.length));
Commitable Suggestion
| System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length); | |
| InetAddress sourceIpAddress = InetAddressPoint.decode(scratch.bytes); | |
| System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length); | |
| InetAddress destinationIpAddress = InetAddressPoint.decode(scratch.bytes); | |
| if (sourceIp.length > scratch.bytes.length) { | |
| throw new IllegalArgumentException("Source IP length exceeds buffer capacity"); | |
| } | |
| System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length); | |
| InetAddress sourceIpAddress = InetAddressPoint.decode(Arrays.copyOf(scratch.bytes, sourceIp.length)); | |
| if (destinationIp.length > scratch.bytes.length) { | |
| throw new IllegalArgumentException("Destination IP length exceeds buffer capacity"); | |
| } | |
| System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length); | |
| InetAddress destinationIpAddress = InetAddressPoint.decode(Arrays.copyOf(scratch.bytes, destinationIp.length)); |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Error-Handling
| for (int i = first; i < first + valueCount; i++) { | ||
| if (NetworkDirectionUtils.inNetwork(sourceIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | ||
| sourceInternal = true; | ||
| break; | ||
| } | ||
| } | ||
| for (int i = first; i < first + valueCount; i++) { | ||
| if (NetworkDirectionUtils.inNetwork(destinationIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | ||
| destinationInternal = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nested Loop Performance
Two sequential loops iterate through the same networks array, causing O(2n) complexity. Each loop processes identical network entries but for different IP addresses. Performance degrades with large network lists affecting query execution time.
for (int i = first; i < first + valueCount; i++) {
String network = networks.getBytesRef(i, netScratch).utf8ToString();
if (!sourceInternal && NetworkDirectionUtils.inNetwork(sourceIpAddress, network)) {
sourceInternal = true;
}
if (!destinationInternal && NetworkDirectionUtils.inNetwork(destinationIpAddress, network)) {
destinationInternal = true;
}
if (sourceInternal && destinationInternal) {
break;
}
}
Commitable Suggestion
| for (int i = first; i < first + valueCount; i++) { | |
| if (NetworkDirectionUtils.inNetwork(sourceIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | |
| sourceInternal = true; | |
| break; | |
| } | |
| } | |
| for (int i = first; i < first + valueCount; i++) { | |
| if (NetworkDirectionUtils.inNetwork(destinationIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | |
| destinationInternal = true; | |
| break; | |
| } | |
| } | |
| for (int i = first; i < first + valueCount; i++) { | |
| String network = networks.getBytesRef(i, netScratch).utf8ToString(); | |
| if (!sourceInternal && NetworkDirectionUtils.inNetwork(sourceIpAddress, network)) { | |
| sourceInternal = true; | |
| } | |
| if (!destinationInternal && NetworkDirectionUtils.inNetwork(destinationIpAddress, network)) { | |
| destinationInternal = true; | |
| } | |
| if (sourceInternal && destinationInternal) { | |
| break; | |
| } | |
| } |
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Algorithmic-Complexity-Linear-Optimization
| public static boolean isInternal(List<String> networks, String ip) { | ||
| for (String network : networks) { | ||
| if (inNetwork(InetAddresses.forString(ip), network)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Missing Input Validation
Method accepts null parameters without validation, potentially causing NullPointerException when networks list or ip string is null. InetAddresses.forString() will throw IllegalArgumentException for invalid IP strings. Business logic requires defensive validation to prevent runtime failures and ensure predictable network direction classification.
public static boolean isInternal(List<String> networks, String ip) {
if (networks == null || ip == null || networks.isEmpty()) {
return false;
}
try {
InetAddress address = InetAddresses.forString(ip);
for (String network : networks) {
if (network != null && inNetwork(address, network)) {
return true;
}
}
} catch (IllegalArgumentException e) {
return false;
}
return false;
}
Commitable Suggestion
| public static boolean isInternal(List<String> networks, String ip) { | |
| for (String network : networks) { | |
| if (inNetwork(InetAddresses.forString(ip), network)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| public static boolean isInternal(List<String> networks, String ip) { | |
| if (networks == null || ip == null || networks.isEmpty()) { | |
| return false; | |
| } | |
| try { | |
| InetAddress address = InetAddresses.forString(ip); | |
| for (String network : networks) { | |
| if (network != null && inNetwork(address, network)) { | |
| return true; | |
| } | |
| } | |
| } catch (IllegalArgumentException e) { | |
| return false; | |
| } | |
| return false; | |
| } |
Standards
- Business-Rule-Input-Validation
- Logic-Verification-Null-Safety
- Algorithm-Correctness-Error-Handling
| if (inNetwork(InetAddresses.forString(ip), network)) { |
There was a problem hiding this comment.
IP Address Injection Risk
Direct string-to-IP conversion without validation allows malformed IP strings to cause exceptions or unexpected behavior. Attackers could inject malicious IP formats to trigger parsing errors or bypass network classification logic. Input validation should occur before IP address parsing to prevent injection attacks.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| default -> CIDRUtils.isInRange(NetworkAddress.format(address), network); |
There was a problem hiding this comment.
Network String Injection Vulnerability
Unvalidated network string passed directly to CIDR range checking allows injection of malicious network patterns. Attackers could inject crafted network strings to bypass security classifications or cause parsing exceptions. Network string validation should occur before CIDR processing to prevent injection attacks.
default -> {
if (network == null || network.trim().isEmpty()) {
yield false;
}
try {
yield CIDRUtils.isInRange(NetworkAddress.format(address), network.trim());
} catch (IllegalArgumentException e) {
yield false;
}
}
Commitable Suggestion
| default -> CIDRUtils.isInRange(NetworkAddress.format(address), network); | |
| default -> { | |
| if (network == null || network.trim().isEmpty()) { | |
| yield false; | |
| } | |
| try { | |
| yield CIDRUtils.isInRange(NetworkAddress.format(address), network.trim()); | |
| } catch (IllegalArgumentException e) { | |
| yield false; | |
| } | |
| } |
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| @Evaluator(warnExceptions = IllegalArgumentException.class) | ||
| static void process( | ||
| BytesRefBlock.Builder builder, | ||
| @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef scratch, | ||
| @Fixed(includeInToString = false, scope = THREAD_LOCAL) BytesRef netScratch, | ||
| BytesRef sourceIp, | ||
| BytesRef destinationIp, | ||
| @Position int position, | ||
| BytesRefBlock networks | ||
| ) { | ||
| int valueCount = networks.getValueCount(position); | ||
| if (valueCount == 0) { | ||
| builder.appendNull(); | ||
| return; | ||
| } | ||
| int first = networks.getFirstValueIndex(position); | ||
|
|
||
| System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length); | ||
| InetAddress sourceIpAddress = InetAddressPoint.decode(scratch.bytes); | ||
| System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length); | ||
| InetAddress destinationIpAddress = InetAddressPoint.decode(scratch.bytes); | ||
|
|
||
| boolean sourceInternal = false; | ||
| boolean destinationInternal = false; | ||
|
|
||
| for (int i = first; i < first + valueCount; i++) { | ||
| if (NetworkDirectionUtils.inNetwork(sourceIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { | ||
| sourceInternal = true; | ||
| break; | ||
| } | ||
| } | ||
| for (int i = first; i < first + valueCount; i++) { | ||
| if (NetworkDirectionUtils.inNetwork(destinationIpAddress, networks.getBytesRef(i, netScratch).utf8ToString())) { |
There was a problem hiding this comment.
Missing Abstraction Layer
Complex low-level byte manipulation and network processing logic embedded directly in evaluator method. This creates tight coupling between ESQL evaluation framework and network analysis implementation. The method handles byte array copying, IP address decoding, and network iteration logic that should be abstracted into separate service classes.
Standards
- SOLID-SRP
- Clean-Code-Functions
- Design-Pattern-Strategy
Pull Request Feedback 🔍
|
| entries.add(Locate.ENTRY); | ||
| entries.add(Log.ENTRY); | ||
| entries.add(Md5.ENTRY); | ||
| entries.add(NetworkDirection.ENTRY); |
There was a problem hiding this comment.
Suggestion: Ensure NetworkDirection exposes a public static ENTRY of the correct type before registering it — either add the ENTRY constant and reader/writer to NetworkDirection, or guard the registry call (e.g. check for the ENTRY field via reflection and only register it if present and of type NamedWriteableRegistry.Entry) to avoid runtime initialization failures. [possible issue]
| System.arraycopy(sourceIp.bytes, sourceIp.offset, scratch.bytes, 0, sourceIp.length); | ||
| InetAddress sourceIpAddress = InetAddressPoint.decode(scratch.bytes); | ||
| System.arraycopy(destinationIp.bytes, destinationIp.offset, scratch.bytes, 0, destinationIp.length); | ||
| InetAddress destinationIpAddress = InetAddressPoint.decode(scratch.bytes); |
There was a problem hiding this comment.
Suggestion: Ensure the InetAddressPoint.decode call receives a byte array whose length exactly matches the IP length by copying only the bytes for the IP (use Arrays.copyOfRange or similar) instead of passing the entire 16-byte scratch buffer. [possible bug]
|
CodeAnt AI finished reviewing your PR. |
2 participants
User description
network_direction(source_ip, destination_ip, networks)function based on the processor of the same nameBlockArgument.javain order to create a functioning evaluator.CodeAnt-AI Description
Add NETWORK_DIRECTION function to ESQL and centralize network direction logic
What Changed
Impact
✅ Network direction available in ESQL queries (NETWORK_DIRECTION / NETDIR)✅ Consistent inbound/outbound/internal/external classification in ESQL and ingest pipelines✅ Invalid or multi-valued inputs produce warnings and null results💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
network_directionESQL function (alias:netdir) that classifies network traffic direction (inbound, outbound, internal, external) based on source IP, destination IP, and internal network definitions.