Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: disable provide over HTTP with Routing.Type=auto #9511

Merged

Conversation

ajnavarro
Copy link
Member

@ajnavarro ajnavarro commented Dec 16, 2022

Applying A solution from #9504

Closes #9504 cc #9417

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@ajnavarro ajnavarro requested review from lidel and guseggert December 16, 2022 09:21
@ajnavarro ajnavarro self-assigned this Dec 16, 2022
@lidel lidel changed the title Feat: Disable Provide on HTTP delegated routing. fix: disable provide over HTTP with Routing.Type=auto Dec 16, 2022
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

lgtm, but test/sharness/t0240-republisher.sh needs to be fixed.

Interesting case: it was flaky before, but now it fails every time :(

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@ajnavarro ajnavarro force-pushed the ajnavarro/feat/option-a-disable-provide-on-http-routing branch from be0cf82 to ca078dd Compare January 3, 2023 11:30
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@ajnavarro ajnavarro requested review from lidel and galargh January 3, 2023 12:08
@ajnavarro
Copy link
Member Author

ajnavarro commented Jan 3, 2023

@lidel I already fixed the problem! PTAL when you have time.

@lidel lidel self-assigned this Jan 3, 2023
@BigLep BigLep mentioned this pull request Jan 3, 2023
Adds IPFS_HTTP_ROUTERS and uses it in basic regression test that
confirms the default mode (Routing.Type=auto) sends lookups over HTTP
for unknown CIDs but no requests are sent with announcements.
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you @ajnavarro, works as expected.

Quick note:

  • Added basic regression test in 858dc78 that confirms we send GETs but no PUTs
    • It introduces IPFS_HTTP_ROUTERS override for use in tests (similar to IPFS_NS_MAP), which allows for tests that work reliably, even without WAN (won't be flaky due to cid.contact).

@lidel lidel merged commit d6069b9 into release-v0.18 Jan 3, 2023
@lidel lidel deleted the ajnavarro/feat/option-a-disable-provide-on-http-routing branch January 3, 2023 20:10
lidel pushed a commit that referenced this pull request Jan 4, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants