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

feat: transport config #7479

Merged
merged 4 commits into from
Jun 17, 2020
Merged

feat: transport config #7479

merged 4 commits into from
Jun 17, 2020

Conversation

Stebalien
Copy link
Member

Add a config section for configuring transports. This way, users can disable transports (especially QUIC), and set muxer/security transport priorities.

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

A few typos, and added some questions/comments, but looks good. ❤️ docs

core/node/libp2p/relay.go Show resolved Hide resolved
core/node/libp2p/sec.go Outdated Show resolved Hide resolved
core/node/libp2p/smux.go Outdated Show resolved Hide resolved
core/node/libp2p/transport.go Show resolved Hide resolved
Comment on lines 122 to 123
iptb run -- ipfs config --json Swarm.Transports.Security TLS false &&
iptb run -- ipfs config --json Swarm.Transports.Security Secio false
Copy link
Contributor

Choose a reason for hiding this comment

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

Any recommendations on how to keep the sharness tests up to date as the default configs change? Is it just grepping through the sharness tests for the configuration options related to the ones you're changing (e.g. Swarm.Transports.Security if any of the security transports change)?

I suspect we're not going to be adding new security transports anytime soon and so this is mostly hypothetical, but the prior "override" semantics made it easier to keep these tests correct when new security protocols were added. IMO the new one (this PR) is much nicer to interact with in code and in the config file editing though.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a problem, but I think we can revisit it later. We may want a second DisableDefaults flag.

test/sharness/t0125-twonode.sh Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
docs/config.md Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Show resolved Hide resolved
@Stebalien
Copy link
Member Author

Ok, so I've "fixed" the config commands to mostly do what we need. They're still buggy but that's an issue for a future release.

@Stebalien
Copy link
Member Author

Digging into the type checking, it looks like it was a failed attempt to auto-decode the input into the correct type (e.g., detect if something should be a bool, etc.). However:

  1. It looks like it never worked correctly.
  2. In the original PR, Assert and convert config value into predefined struct #1440, the exact bugs we're running into here were pointed out but, apparently, weren't a problem? I have no idea how that got merged.

@Stebalien Stebalien force-pushed the feat/transport-conf branch 2 times, most recently from 13291a9 to 8644485 Compare June 16, 2020 22:02
This way, users can disable transports (especially QUIC), and set muxer/security
transport priorities.
This bug was pointed out in
#1440 (comment) but was ignored
for some reason.
@Stebalien Stebalien merged commit 99fa027 into release-v0.6.0 Jun 17, 2020
@Stebalien Stebalien deleted the feat/transport-conf branch June 17, 2020 01:59
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.

2 participants