Initial commit for cidr function #706

salyh · 2024-09-26T18:50:01Z

Description

Implement cidr function to check wether an ip address is with a given cidr

Issues Resolved

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

salyh · 2024-09-26T18:58:49Z

@YANG-DB @vamshin @anirudha

I pushed the scaffold for the CIDR function.
The real implementation which validates the ip address and check if its within the cidr is missing because we need to discuss the introduction of a new 3rd party library:

I suggest to add a dependency to https://github.com/seancfoley/IPAddress because this library seems to provide ip validation and in range checking for ipv4 and ipv6 addresses/cidrs. The library is Apache licensed.

Other options are the following libraries or code snippets (or of course manual implementation with java standard library which is error prone in my view):

https://www.baeldung.com/java-check-ip-address-range
https://github.com/apache/commons-net (ipv4 only)
https://github.com/edazdarevic/CIDRUtils/blob/master/CIDRUtils.java (MIT license)

Let me know what you think and what the general process is to discuss new dependencies

YANG-DB · 2024-09-26T21:38:30Z

Hi @salyh
I agree - let's use https://github.com/seancfoley/IPAddress library since it's in use in our SQL repo already

LantaoJin · 2024-09-27T00:14:40Z

ppl-spark-integration/src/main/java/org/opensearch/sql/ast/expression/Cidr.java

+    private UnresolvedExpression ipAddress;
+    private UnresolvedExpression cidrBlock;
+
+    public Cidr(UnresolvedExpression ipAddress, UnresolvedExpression cidrBlock) {
+        this.ipAddress = ipAddress;
+        this.cidrBlock = cidrBlock;


Please merge the latest code base and refactor this class with Lombok annotations. Ref 38ca314

LantaoJin · 2024-09-27T00:20:01Z

Hi @salyh
Can you check if the Guava library can meet our needs?

salyh · 2024-09-27T09:51:37Z

Hi @salyh Can you check if the Guava library can meet our needs?

I checked but it does not support cidr checks. https://github.com/seancfoley/IPAddress is also already used in the sql project.

salyh · 2024-09-27T09:51:53Z

Hi @salyh I agree - let's use https://github.com/seancfoley/IPAddress library since it's in use in our SQL repo already

done

YANG-DB · 2024-09-30T04:49:02Z

@salyh plz resolve conflict
thanks

salyh · 2024-09-30T13:03:21Z

@salyh plz resolve conflict thanks

done

salyh · 2024-09-30T13:27:14Z

@YANG-DB @vamshin @anirudha

Regarding validation of the CIDR function parameters:

The IPAddress library is very lenient regarding validation of ip addresss and cidr blocks.

I suggest we at least disallow partial ip address segements and empty or null adresses:

  IPAddressStringParameters valOptions = new IPAddressStringParameters.Builder()
                .allowEmpty(false)
                .setEmptyAsLoopback(false)
                .allow_inet_aton(false)
                .allowSingleSegment(false)
                .toParams();

Then the remaining question is wether we should enforce other syntax checks like which the library is not enforcing:

check that cidr block is a really a in cicdr notation (containing a '/')
check that ip address and cidr are either both ipv6 or both ipv4
define the behaviour for edge cases like 0.0.0.0 (or just rely on the api)
define handling of special ip adresses like multicast, anycast, 255...* etc

My suggestion is to only

disallow partial ip address segements and empty or null adresses (like already implemented) and leave everything else
disallow mixed ipv4/ipv6 adresses and cidr blocks

and leave everything else to the library.

YANG-DB · 2024-10-07T16:06:29Z

@salyh
please add the needed documentation in the following locations:

YANG-DB · 2024-10-07T18:24:46Z

@YANG-DB @vamshin @anirudha

Regarding validation of the CIDR function parameters:

The IPAddress library is very lenient regarding validation of ip addresss and cidr blocks.

I suggest we at least disallow partial ip address segements and empty or null adresses:
  IPAddressStringParameters valOptions = new IPAddressStringParameters.Builder()
                .allowEmpty(false)
                .setEmptyAsLoopback(false)
                .allow_inet_aton(false)
                .allowSingleSegment(false)
                .toParams();
Then the remaining question is wether we should enforce other syntax checks like which the library is not enforcing:

check that cidr block is a really a in cicdr notation (containing a '/')

check that ip address and cidr are either both ipv6 or both ipv4

define the behaviour for edge cases like 0.0.0.0 (or just rely on the api)

define handling of special ip adresses like multicast, anycast, 255...* etc

My suggestion is to only

disallow partial ip address segements and empty or null adresses (like already implemented) and leave everything else

disallow mixed ipv4/ipv6 adresses and cidr blocks

and leave everything else to the library.

@salyh
Thanks for this review - I support your suggestion

disallow partial ip address segements and empty or null adresses (like already implemented) and leave everything else
disallow mixed ipv4/ipv6 adresses and cidr blocks

In addition plz document the additional validations as future work:

check that cidr block is a really a in cicdr notation (containing a '/')
check that ip address and cidr are either both ipv6 or both ipv4
define the behaviour for edge cases like 0.0.0.0 (or just rely on the api)
define handling of special ip adresses like multicast, anycast, 255...* etc

Thanks

YANG-DB

please add the needed documentation in the following locations:

ppl planning list

Once we approve we'll also add it onto the other docs:

ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4

penghuo · 2024-10-10T04:00:55Z

ppl-spark-integration/src/main/java/org/opensearch/sql/expression/function/SerializableUdf.java

+        @Override
+        public Boolean apply(String ipAddress, String cidrBlock) {
+
+            IPAddressString parsedIpAddress = new IPAddressString(ipAddress, valOptions);


Should we leverage InetAddresses in opensearch-common package to parse IpAddress and CIDR?

I don't think there's anything wrong with it. It is a well-maintained package

I am not concerning the package is well-maintained or not.
My point is to be consistent with OpenSearch's parsing and validation behavior.

@penghuo Can you provide a link to code or docs where we can see the OpenSearch's parsing and validation behavior.?

https://github.com/opensearch-project/OpenSearch/blob/main/libs/common/src/main/java/org/opensearch/common/network/InetAddresses.java#L358

public static InetAddress forString(String ipString) { byte[] addr = ipStringToBytes(ipString); // The argument was malformed, i.e. not an IP string literal. if (addr == null) { throw new IllegalArgumentException(String.format(Locale.ROOT, "'%s' is not an IP string literal.", ipString)); } return bytesToInetAddress(addr); }

@penghuo Thanks. But please see #706 (comment)

penghuo · 2024-10-10T04:06:03Z

ppl-spark-integration/src/main/java/org/opensearch/sql/expression/function/SerializableUdf.java

+            IPAddressString parsedCidrBlock = new IPAddressString(cidrBlock, valOptions);
+
+            try {
+                parsedCidrBlock.validate();


Could we align the validation with OpenSearch?

@dr-lilienthal I agree with @penghuo
lets consolidate for using the same functionality
thanks

When I read this and the above thread correct the idea is to remove the https://github.com/seancfoley/IPAddress lib dependency and instead use classes of the org.opensearch.common.network, right?

@penghuo @YANG-DB Got it. But I dont see any methods to check if the ipaddress is within the cidr block or not. Did I overlooked something or should we implement it ourself? See the discussions above: 1 and 2

@penghuo please assist...

@salyh for cidr match, I want to confirm if the behavior of the term query is as expected. If it is, I can help locate the relevant code.

GET my-index-000001/_search { "query": { "term": { "ip_addr": "192.168.0.0/16" } } }

@penghuo yes it is. Is the relevant code this?: https://github.com/opensearch-project/OpenSearch/blob/119abaff95a7f54affea6c844fad7ce3c8360155/server/src/main/java/org/opensearch/index/mapper/IpFieldMapper.java#L248

If so I try to re-implement based on org.apache.lucene.document.InetAddressPoint.newPrefixQuery() which evalutes to something like

byte[] lower = value.getAddress(); byte[] upper = value.getAddress(); for (int i = prefixLength; i < 8 * lower.length; i++) { int m = 1 << (7 - (i & 7)); lower[i >> 3] &= ~m; upper[i >> 3] |= m; }

IpField term query is rewritten as PointRangeQuery in lucene. I can think 2 approach

read value from child operator, index into Lucene, and run range query.

leverage parseCidr logic, and perform match query using https://github.com/seancfoley/IPAddress.

YANG-DB · 2024-10-11T16:24:49Z

@dr-lilienthal can u plz resolve the conflicts and also signOff the commits ?( DCO failure )
thanks

Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>

…h exception runtime exception in case of a mixed adress type Signed-off-by: Jens Schmidt <jens.schmidt@eliatra.com>

Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>

Signed-off-by: Jens Schmidt <jens.schmidt@eliatra.com>

LantaoJin reviewed Sep 27, 2024

View reviewed changes

YANG-DB added Lang:PPL Pipe Processing Language support 0.6 labels Sep 27, 2024

salyh force-pushed the cidr branch from 62681d3 to ab51739 Compare September 27, 2024 07:36

salyh force-pushed the cidr branch from ec67564 to 2e90eed Compare September 30, 2024 12:22

salyh marked this pull request as ready for review October 1, 2024 12:50

salyh requested review from dai-chen, rupal-bq, vamsi-amazon, penghuo, seankao-az, anirudha, kaituo and YANG-DB as code owners October 1, 2024 12:50

salyh force-pushed the cidr branch from 2e90eed to d1f3dc2 Compare October 5, 2024 16:33

salyh force-pushed the cidr branch from d1f3dc2 to 3ad3d5d Compare October 8, 2024 13:17

YANG-DB reviewed Oct 8, 2024

View reviewed changes

salyh force-pushed the cidr branch from 3ad3d5d to 0c02ec4 Compare October 9, 2024 12:06

penghuo reviewed Oct 10, 2024

View reviewed changes

salyh force-pushed the cidr branch from 3fb01de to 3cddc42 Compare October 14, 2024 09:51

dr-lilienthal force-pushed the cidr branch from bfcd162 to cf05c3e Compare October 15, 2024 10:19

salyh force-pushed the cidr branch from cf05c3e to a4d1e3c Compare October 24, 2024 12:18

salyh and others added 7 commits October 25, 2024 10:49

Initial commit for cidr function

9bd9ec8

Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>

Add inet.ipaddr library and implement basic cidr check

83730e0

Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>

Refactor for using lombok

77beed4

Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>

Add unittests

da15ca4

Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>

add check for mixed ip address types like ipv4 and ipv6; add test wit…

eaeaa09

…h exception runtime exception in case of a mixed adress type Signed-off-by: Jens Schmidt <jens.schmidt@eliatra.com>

Fix antlr

ff5ba68

Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>

rename cird function cirdmatch

cf34bb0

Signed-off-by: Jens Schmidt <jens.schmidt@eliatra.com>

salyh force-pushed the cidr branch from a4d1e3c to cf34bb0 Compare October 25, 2024 08:49

salyh requested review from mengweieric, noCharger and ykmr1224 as code owners October 25, 2024 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit for cidr function #706

Initial commit for cidr function #706

salyh commented Sep 26, 2024

salyh commented Sep 26, 2024

YANG-DB commented Sep 26, 2024

LantaoJin Sep 27, 2024

salyh Sep 27, 2024

LantaoJin commented Sep 27, 2024 •

edited

Loading

salyh commented Sep 27, 2024

salyh commented Sep 27, 2024

YANG-DB commented Sep 30, 2024

salyh commented Sep 30, 2024

salyh commented Sep 30, 2024

YANG-DB commented Oct 7, 2024

YANG-DB commented Oct 7, 2024

YANG-DB left a comment

penghuo Oct 10, 2024

dr-lilienthal Oct 10, 2024

penghuo Oct 11, 2024

salyh Oct 14, 2024

penghuo Oct 14, 2024

salyh Oct 15, 2024

penghuo Oct 10, 2024

YANG-DB Oct 11, 2024

salyh Oct 14, 2024

penghuo Oct 14, 2024

salyh Oct 15, 2024 •

edited

Loading

YANG-DB Oct 24, 2024

penghuo Oct 24, 2024

salyh Oct 24, 2024

penghuo Oct 25, 2024

YANG-DB commented Oct 11, 2024

Initial commit for cidr function #706

Are you sure you want to change the base?

Initial commit for cidr function #706

Conversation

salyh commented Sep 26, 2024

Description

Issues Resolved

salyh commented Sep 26, 2024

YANG-DB commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LantaoJin commented Sep 27, 2024 • edited Loading

salyh commented Sep 27, 2024

salyh commented Sep 27, 2024

YANG-DB commented Sep 30, 2024

salyh commented Sep 30, 2024

salyh commented Sep 30, 2024

YANG-DB commented Oct 7, 2024

YANG-DB commented Oct 7, 2024

YANG-DB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salyh Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YANG-DB commented Oct 11, 2024

LantaoJin commented Sep 27, 2024 •

edited

Loading

salyh Oct 15, 2024 •

edited

Loading