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 Socket::IPAddress#link_local? #13204

Merged

Conversation

GeopJr
Copy link
Contributor

@GeopJr GeopJr commented Mar 20, 2023

This PR adds Socket::IPAddress#link_local?like Ruby's

(the spec examples are also from Ruby)

fix partially: #13198

Co-authored-by: Mike Robbins <1057385+compumike@users.noreply.github.com>
@Fryguy
Copy link
Contributor

Fryguy commented Mar 20, 2023

Does private? have to be changed to use the new link_local? method? Or is that for another PR? (asking because you said this fixes #13198)

EDIT: Actually it's not that way on the Ruby side, either, so I don't understand where/how this method gets used.

@GeopJr
Copy link
Contributor Author

GeopJr commented Mar 20, 2023

The thread that the issue author links (who is also the author of) is about detecting non-public ip addresses, mainly 169.254.0.0/16. #link_local? should allow them to detect it as non-public.

I don't think it should be under #private? as rfc 6890 mentions 169.254.0.0/16 as Link Local

@Blacksmoke16
Copy link
Member

I don't think it should be under #private? as rfc 6890 mentions 169.254.0.0/16 as Link Local

I think it makes sense to have a dedicated method for it, but my understanding is while 169.254.0.0/16 is Link Local, link local is also a private IP, so it would make sense to do something like return false if self.link_local? within #private such that you do not have to || a bunch of separate calls together. E.g. from the OP #private is missing a few others in additional to this link local range.

@GeopJr
Copy link
Contributor Author

GeopJr commented Mar 20, 2023

I'd personally vote for #public? being something like !link_local? && !private? && !loopback? && !unspecified? instead of breaking compatibility with ruby:

irb(main):002:0> IPAddr.new('169.254.169.254').link_local?
=> true
irb(main):003:0> IPAddr.new('169.254.169.254').private?
=> false

edit:
I re-read the comment, I'll unlink the issue for now since this requires more thought

@straight-shoota
Copy link
Member

I commented some clarification in #13198 (comment)
Please open a new issue for a #public? method (although I'm pretty convinced we need a better name because public? not being the inverse of #private? would be confusing).

@GeopJr
Copy link
Contributor Author

GeopJr commented Mar 20, 2023

I don't really have any strong arguments in favor of a #public?-like method instead of !private? && ... so I'll pass on creating an issue

@straight-shoota straight-shoota added this to the 1.8.0 milestone Mar 27, 2023
@straight-shoota straight-shoota changed the title Add Socket::IPAddress#link_local? Add Socket::IPAddress#link_local? Mar 29, 2023
@straight-shoota straight-shoota merged commit 09e0fb1 into crystal-lang:master Mar 29, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants