Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

add ipv6 listening for API / WebGateway #84

Closed
wants to merge 0 commits into from

Conversation

RubenKelevra
Copy link
Contributor

No description provided.

@welcome
Copy link

welcome bot commented May 8, 2020

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

@RubenKelevra RubenKelevra marked this pull request as ready for review May 8, 2020 15:02
@rpodgorny
Copy link

thumbs up for ipv6!

addresses.go Outdated
API Strings // address for the local API (RPC)
Gateway Strings // address to listen on for IPFS HTTP object gateway
API []string // addresses for the local API (RPC)
Gateway []string // addresses to listen on for IPFS HTTP object gateway
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing these types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, Swarm, Announce and NoAnnounce are arrays, because they hold multiple addresses.

API and Gateway now need to hold multiple addresses too. So they need to be an array.

Or is there's something wrong with my assumption?

Copy link
Member

Choose a reason for hiding this comment

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

We created the Strings type to allow either a single string or an array of strings. Changing this to a simple array would be a backwards incompatible change.

@Stebalien
Copy link
Member

What's the motivation for listening on IPv6 for localhost? IPv4 should always work for localhost regardless.

@rpodgorny
Copy link

What's the motivation for listening on IPv6 for localhost? IPv4 should always work for localhost regardless.

...because [::1] is shorter to type than 127.0.0.1? ;-)

now, kidding aside, ipv6 simply supersedes ipv4 and will be the the default protocol in the future. actually, it could be the default right now if people cared a bit more. ...imagine the feeling when you're dropping all this nat-traversing code and shit! ;-)

i agree the localhost is the least case where ipv6 is important but i look at it from a different point of view. i feel this as a part of the "ipv6 by default everywhere" mindset that will drag the general ipv6 adoption with it...

anyway, at least for this very patch, i don't see any negative (increased code complexity, security issues, ...) with it so in my opinion it should be a "go" even if you personally don't really care about ipv6 that much.

@Stebalien
Copy link
Member

Fair enough. I'm just always wary of changes without reasons because https://xkcd.com/1172/.

@RubenKelevra
Copy link
Contributor Author

What's the motivation for listening on IPv6 for localhost? IPv4 should always work for localhost regardless.

Well, there's no reason why we shouldn't use IPv6. I dislike using legacy IP. :)

@RubenKelevra
Copy link
Contributor Author

Sorry, messed that up. I'm not sure you can change the remote branch of a PR, so I filed a new one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants