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

add TrieAddr mruby library #1038

Merged
merged 8 commits into from Aug 26, 2016
Merged

add TrieAddr mruby library #1038

merged 8 commits into from Aug 26, 2016

Conversation

i110
Copy link
Contributor

@i110 i110 commented Aug 23, 2016

@kazuho @hirose31 Adds TrieAddr module which can handle CIDR based IP address groups.

SYNOPSYS:

paths:
  /:
    mruby.handler: |
      require "trie_addr.rb"
      trie = TrieAddr.new.add(["192.168.0.0/16", "172.16.0.0/12"])
      acl {
        allow { trie.match?(addr) }
        deny
      }
    file.dir: examples/doc_root

This library is mainly built for the use-case in acl block, but this is actually independent of ACL feature, so we can use this in other handlers or anywhere else.

@kazuho
Copy link
Member

kazuho commented Aug 23, 2016

Thank you for the PR.

The only concern I have before merging this is how it interacts with IPv6. I do not think it would be necessary to support IPv6 in trie_addr.rb, but I think the code and the doc should be clear on what happens if a v6 address is passed in.

@i110
Copy link
Contributor Author

i110 commented Aug 24, 2016

@kazuho Thank you for your comment.
Considering about ipv6 address,

  • I modified add method to raise an ArgumentError if a user provide IPv6 CIDR or not valid IPv4 address. By this, a user who mistakenly believes that this library supports IPv6 can realize the mistake.
  • match? method doesn't do any check because of performance, and it always returns false if invalid ipv4 address are passed in. We have to describe this behavior in the document

BTW, where should I add a document about this library? In Access Control ?

@i110
Copy link
Contributor Author

i110 commented Aug 24, 2016

Changed to use hashes internally instead of arrays. Thanks to it,

  • Performance improved because of removing to_i calls
  • Below edge-case bug is fixed
    • TrieAddr.new.add("0.0.0.0/8").match?("hoge") # true
  • Got cleaner code by using Hash#default

@kazuho
Copy link
Member

kazuho commented Aug 24, 2016

@i110 Thank you for the updates.

Considering about ipv6 address,

  • I modified add method to raise an ArgumentError if a user provide IPv6 CIDR or not valid IPv4 address. By this, a user who mistakenly believes that this library supports IPv6 can realize the mistake.
  • match? method doesn't do any check because of performance, and it always returns false if invalid ipv4 address are passed in. We have to describe this behavior in the document

SGTM

BTW, where should I add a document about this library? In Access Control ?

Let's discuss. Maybe we should create a section dedicated mruby, but I'd like to think more before proceeding.

@i110
Copy link
Contributor Author

i110 commented Aug 26, 2016

@kazuho I added a document in Configure > Access-Control.

@@ -241,4 +241,33 @@ paths:
EOT
?>

<h2 id="how-to" class="section-head">How-To</h2>

<h3 id="cidr">IP Address Matching Against CIDR list</h3>
Copy link
Member

Choose a reason for hiding this comment

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

Possibly change the title to "Matching IP Address Blocks?".

Note that CIDR is an abbreviation of Classless Inter-Domain Routing. Are we routing packets?

@kazuho
Copy link
Member

kazuho commented Aug 26, 2016

Thank you for the doc! Left comments in-line.

I generally believe that using the phrase CIDR is not the way to go since it's not precise in our context (and since the word is not so common). I have left some suggestions on how they should be changed, but please go though and change those I have missed.

@i110
Copy link
Contributor Author

i110 commented Aug 26, 2016

@kazuho thank you for your comments. I addressed the points you mentioned.

@kazuho kazuho merged commit d57be15 into h2o:master Aug 26, 2016
@kazuho
Copy link
Member

kazuho commented Aug 26, 2016

Thank you for writing this nifty library.

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.

None yet

2 participants