Skip to content
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

[Feature Request] CIDR Validation #66

Closed
Kryan90 opened this issue Aug 10, 2023 · 3 comments · Fixed by #99
Closed

[Feature Request] CIDR Validation #66

Kryan90 opened this issue Aug 10, 2023 · 3 comments · Fixed by #99
Labels
Feature New feature or request

Comments

@Kryan90
Copy link

Kryan90 commented Aug 10, 2023

Feature description:
Add field validation for CIDR ranges, ie. support string cidr = 1 [(buf.validate.field).string.cidr = true];

I found a similar request from the protoc-gen-validate repo that had been closed bufbuild/protoc-gen-validate#461

Problem it solves or use case:
Currently to get validation behavior I am using this pattern

message CidrRange {
  string address_prefix = 1 [(buf.validate.field).string.ipv4 = true];
  google.protobuf.UInt32Value prefix_len = 2 [(buf.validate.field).uint32 = {lte: 32}];
}

however, this isn't particularly ergonomic when used with google.api.http routes since you end up having to do:

?cidr.address_prefix=1.2.3.4&cidr.prefix_len=28

vs

?cidr=1.2.3.4/28

Proposed implementation or solution:
I think doing a similar implementation as IP validators would make sense

//`ip` specifies that the field value must be a valid IP
//(v4 or v6) address, without surrounding square brackets for IPv6 addresses.
//If the field value isn't a valid IP address, an error message will be
//generated.
//
//```proto
//message MyString {
// // value must be a valid IP address
// string value = 1 [(buf.validate.field).string.ip = true];
//}
//```
bool ip = 14 [(priv.field).cel = {
id: "string.ip",
message: "value must be a valid IP address",
expression: "this.isIp()",
}];
//`ipv4` specifies that the field value must be a valid IPv4
//address. If the field value isn't a valid IPv4 address, an error message
//will be generated.
//
//```proto
//message MyString {
// // value must be a valid IPv4 address
// string value = 1 [(buf.validate.field).string.ipv4 = true];
//}
//```
bool ipv4 = 15 [(priv.field).cel = {
id: "string.ipv4",
message: "value must be a valid IPv4 address",
expression: "this.isIp(4)"
}];
//`ipv6` specifies that the field value must be a valid
//IPv6 address, without surrounding square brackets. If the field value is
//not a valid IPv6 address, an error message will be generated.
//
//```proto
//message MyString {
// // value must be a valid IPv6 address
// string value = 1 [(buf.validate.field).string.ipv6 = true];
//}
//```
bool ipv6 = 16 [(priv.field).cel = {
id: "string.ipv6",
message: "value must be a valid IPv6 address",
expression: "this.isIp(6)",
}];

  1. (buf.validate.field).string.cidr
  2. (buf.validate.field).string.cidrv4
  3. (buf.validate.field).string.cidrv6

Contribution:
I would be happy to try and implement this myself. I'm not sure how implementations in the language specific repos is done, but I could also help with the Go and Python implementations.

Examples or references:
https://github.com/go-playground/validator/blob/5bf55dc757cad229e8297d42640ec036e2360df7/baked_in.go#L375-L393

@Kryan90 Kryan90 added the Feature New feature or request label Aug 10, 2023
@elliotmjackson
Copy link
Contributor

Thank you for the comprehensive feature request! The proposed implementations, (buf.validate.field).string.cidr, .string.cidrv4, and .string.cidrv6 seem in line with the existing structure and should provide a clear and consistent validation approach.

Your willingness to contribute to the implementation is much appreciated. We'd be happy to guide you on how implementations are done. Leveraging the existing functions to make this a pure CEL constraint is a compelling idea. If we can achieve that, it'd be ideal as it would remove the need to complete language-specific implementations.

@rodaine rodaine linked a pull request Oct 2, 2023 that will close this issue
4 tasks
@higebu
Copy link
Contributor

higebu commented Oct 5, 2023

Hi @Kryan90,
I've created a pull request (#99). I hadn't seen this issue when I made the PR. Having now reviewed the request, I believe my PR might align with what you're looking for. Could you please take a look and let me know if it matches the PR you had in mind? I'm open to feedback and any adjustments needed.
My proposed spec is here: https://github.com/higebu/protovalidate/blob/9f4f80cc1119ecedf5214af4aec3c189acd52902/proto/protovalidate/buf/validate/validate.proto#L2821-L2918
Thank you!

@Kryan90
Copy link
Author

Kryan90 commented Oct 6, 2023

Hi @Kryan90, I've created a pull request (#99). I hadn't seen this issue when I made the PR. Having now reviewed the request, I believe my PR might align with what you're looking for. Could you please take a look and let me know if it matches the PR you had in mind? I'm open to feedback and any adjustments needed. My proposed spec is here: https://github.com/higebu/protovalidate/blob/9f4f80cc1119ecedf5214af4aec3c189acd52902/proto/protovalidate/buf/validate/validate.proto#L2821-L2918 Thank you!

This is functionally exactly what I had been looking for, thank you! The naming is slightly different, but I think ip_with_prefixlen is probably a more accurate descriptor anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants