-
Notifications
You must be signed in to change notification settings - Fork 30
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
Initial commit for cidr function #706
base: main
Are you sure you want to change the base?
Conversation
I pushed the scaffold for the CIDR function. 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):
Let me know what you think and what the general process is to discuss new dependencies |
Hi @salyh |
private UnresolvedExpression ipAddress; | ||
private UnresolvedExpression cidrBlock; | ||
|
||
public Cidr(UnresolvedExpression ipAddress, UnresolvedExpression cidrBlock) { | ||
this.ipAddress = ipAddress; | ||
this.cidrBlock = cidrBlock; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge the latest code base and refactor this class with Lombok annotations. Ref 38ca314
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Hi @salyh |
I checked but it does not support cidr checks. https://github.com/seancfoley/IPAddress is also already used in the sql project. |
done |
@salyh plz resolve conflict |
done |
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:
Then the remaining question is wether we should enforce other syntax checks like which the library is not enforcing:
My suggestion is to only
and leave everything else to the library. |
@salyh |
@salyh
In addition plz document the additional validations as future work:
Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the needed documentation in the following locations:
Once we approve we'll also add it onto the other docs:
@Override | ||
public Boolean apply(String ipAddress, String cidrBlock) { | ||
|
||
IPAddressString parsedIpAddress = new IPAddressString(ipAddress, valOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we leverage InetAddresses in opensearch-common package to parse IpAddress and CIDR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's anything wrong with it. It is a well-maintained package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not concerning the package is well-maintained or not.
My point is to be consistent with OpenSearch's parsing and validation behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@penghuo Can you provide a link to code or docs where we can see the OpenSearch's parsing and validation behavior.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@penghuo Thanks. But please see #706 (comment)
IPAddressString parsedCidrBlock = new IPAddressString(cidrBlock, valOptions); | ||
|
||
try { | ||
parsedCidrBlock.validate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we align the validation with OpenSearch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dr-lilienthal I agree with @penghuo
lets consolidate for using the same functionality
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@penghuo please assist...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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"
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@dr-lilienthal can u plz resolve the conflicts and also signOff the commits ?( DCO failure ) |
Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>
Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>
Signed-off-by: Hendrik Saly <hendrik.saly@eliatra.com>
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: Jens Schmidt <jens.schmidt@eliatra.com>
Description
Implement cidr function to check wether an ip address is with a given cidr
Issues Resolved
#671
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.