Skip to content

implement net.cidr_is_valid builtin - #422

Merged
anakrish merged 5 commits into
microsoft:mainfrom
tjons:tjons/net-cidr-is-valid
Aug 6, 2025
Merged

implement net.cidr_is_valid builtin#422
anakrish merged 5 commits into
microsoft:mainfrom
tjons:tjons/net-cidr-is-valid

Conversation

@tjons

@tjons tjons commented Jun 30, 2025

Copy link
Copy Markdown
Contributor

@anakrish I've been working on implementing the net.cidr_is_valid builtin. It currently is complete for IPv4, and so I wanted to try to get some early feedback while I finish support for IPv6.

I'm still quite new to Rust, and so please feel free to give any pointers or suggestions you think I might be missing 🦀

@tjons

tjons commented Jun 30, 2025

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@anakrish anakrish left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @tjons,
Thanks for the contribution. Overall looks good to me. I've provided some suggestions.

Comment thread src/builtins/net.rs Outdated
Comment thread src/builtins/net.rs Outdated
Comment thread src/builtins/net.rs Outdated
Comment thread src/builtins/net.rs Outdated
Comment thread src/builtins/net.rs Outdated
@tjons

tjons commented Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

@anakrish thanks for the feedback! I'll update this and finish ipv6 this week. Do let me know your thoughts on #422 (comment), which will either dramatically simplify the implementation or take a bit longer :)

tjons added 2 commits July 28, 2025 13:31
Signed-off-by: tjons <tylerschade99@gmail.com>
Signed-off-by: tjons <tylerschade99@gmail.com>
@tjons tjons changed the title WIP: implement net.cidr_is_valid builtin implement net.cidr_is_valid builtin Jul 28, 2025
Signed-off-by: tjons <tylerschade99@gmail.com>
@tjons
tjons requested a review from anakrish July 28, 2025 18:52
@tjons

tjons commented Jul 28, 2025

Copy link
Copy Markdown
Contributor Author

@anakrish sorry for the wait, addressed all the feedback! Let me know if anything else is needed for this builtin, I'll get started on the rest this week.

@tjons
tjons force-pushed the tjons/net-cidr-is-valid branch from c49417d to 5797da6 Compare July 28, 2025 19:00

@anakrish anakrish left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjons Overall looks good to me. Some suggestions on propagating the errors.

Comment thread src/builtins/net.rs Outdated
Comment thread src/builtins/net.rs Outdated
Comment thread src/builtins/net.rs Outdated
Signed-off-by: tjons <tylerschade99@gmail.com>
@tjons
tjons requested a review from anakrish August 4, 2025 21:39
Comment thread src/builtins/net.rs Fixed
@anakrish

anakrish commented Aug 5, 2025

Copy link
Copy Markdown
Collaborator

Hey @tjons, Overall looks good to me.

There are a bunch of format string related failures. https://github.com/microsoft/regorus/actions/runs/16734832282/job/47424635159?pr=422

Can you also add v0/netcidrisvalid and v1/netcidrisvalid to https://github.com/microsoft/regorus/blob/main/tests/opa.passing to ensure that tests from OPA testsuite are run?

@tjons

tjons commented Aug 5, 2025

Copy link
Copy Markdown
Contributor Author

@anakrish done! I promise future contributions will be smoother, thanks for bearing with me, sorry this has taken a while as I'm learning the ropes.

While enabling the builtins in the tests, I had to refactor the code to not use Err returns, because the OPA built-in doesn't propagate errors when an invalid CIDR is passed and it was causing unsafe variable references. See https://github.com/open-policy-agent/opa/blob/main/v1/topdown/cidr.go#L220.

@anakrish

anakrish commented Aug 6, 2025

Copy link
Copy Markdown
Collaborator

@tjons No worries. I truly glad that you are contributing.

@anakrish
anakrish merged commit a8384da into microsoft:main Aug 6, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants