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

SetDoesNotUseRepo(true) is not being used correctly #8375

Open
aschmahmann opened this issue Aug 24, 2021 · 5 comments
Open

SetDoesNotUseRepo(true) is not being used correctly #8375

aschmahmann opened this issue Aug 24, 2021 · 5 comments
Assignees
Labels
effort/days Estimated to take multiple days, but less than a week exp/intermediate Prior experience is likely helpful kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up topic/commands Topic commands

Comments

@aschmahmann
Copy link
Contributor

aschmahmann commented Aug 24, 2021

Description

Both ipfs cid and ipfs multibase have SetDoesNotUseRepo(true) defined on the root command. However, this option doesn't propagate down to the child commands.

If we actually want this behavior then we should propagate to the children, otherwise we should drop it.

#8180 (comment)

@aschmahmann aschmahmann added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Aug 24, 2021
@BigLep BigLep added this to the go-ipfs 0.10 milestone Aug 24, 2021
@BigLep BigLep removed the need/triage Needs initial labeling and prioritization label Aug 26, 2021
@BigLep
Copy link
Contributor

BigLep commented Aug 26, 2021

@lidel: your input here is needed - thanks.

@schomatis
Copy link
Contributor

Assigning myself along lidel just to flag this is a good candidate for me to work on after lidel's input.

@schomatis schomatis self-assigned this Dec 23, 2021
@lidel
Copy link
Member

lidel commented Jan 13, 2022

On how SetDoesNotUseRepo works

Looked into this and GetDoesNotUseRepo is used only in one place, at the beginning of every CLI execution:

https://github.com/ipfs/go-ipfs/blob/14046d000276c5f1c8b1aed50634f18cd2ef23d7/cmd/ipfs/main.go#L210-L213

If flag was set via SetDoesNotUseRepo, then ipfs command will initialize and execute locally, skipping every external dependency and mutable states that could cause an error:

  1. ignores remote API addr in $IPFS_PATH/api or --api CLI param (will always run locally, never over HTTP client, even if daemon is running)
  2. will not do any daemon / repo checks (eg. will not attempt to read API addr from $IPFS_PATH/config, or see if repo is initialized – will work even when IPFS_PATH=/dev/null)

I believe this is a valuable feature: we want ipfs cid and ipfs multibase to be useful as standalone utilities.
Presence or lack of API addr or IPFS repo, config should have no impact on them.

What is missing in cid and multibase commands

Commands start and run locally thanks to SetDoesNotUseRepo(true), however their internals refer to $IPFS_PATH/config, which IMO is a bug and should be fixed.

All cid and multibase should work without repo, right now they produce errors like this:

$ echo test | IPFS_PATH=/dev/null ipfs multibase encode
Error: error loading plugins: open /dev/null/config: not a directory

$ IPFS_PATH=/dev/null ipfs cid base32 QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMn
Error: error loading plugins: open /dev/null/config: not a directory

(use IPFS_PATH=/dev/null to ensure command execution is local and independent of external state)

TODO

@schomatis iiuc the tasks here are:

  • fix cid and multibase to work with IPFS_PATH=/dev/null
  • make sure other commands that have SetDoesNotUseRepo(true) set work with IPFS_PATH=/dev/null, fix if necessary

@lidel lidel added effort/hours Estimated to take one or several hours P1 High: Likely tackled by core team if no one steps up exp/novice Someone with a little familiarity can pick up topic/commands Topic commands labels Jan 13, 2022
@schomatis
Copy link
Contributor

@lidel tl;dr the error here is that the plugins live in the repo and those are always loaded; I'm not sure the implications of not loading them (or even how to do it), so someone else should spend some time designing a solution (I'll be un-assigning myself to signal that I can be of little use at the moment)

Other loose thoughts:

  • This DoesNotUseRepo flag is very misleading and should be removed altogether.
  • I very much agree that we should delineate which commands modify state and which don't.
  • There is a tension around using the ipfs umbrella for these "standalone/independent" commands (and maybe those should be decoupled, e.g., just have a cid command that consumes go-ipfs, or some of its dependencies, as a library).
  • The ipfs commands depend on the go-ipfs-cmds library, and its entire architecture and implied design throughout the codebase is that ipfs needs a repo because you will always be creating an IPFS node and interacting with it (this is my interpretation, not formalized anywhere, but AFAICT the code follows this).
  • Investing time in go-ipfs-cmds to accommodate this third category of "I don't need a repo/node" (in contrast with: I need a node, I might crate it myself locally or access it remotely through the daemon) doesn't seem worthwhile given the lack of maintenance of that library.
  • Our best bet would be to craft some bypass mechanism in go-ipfs here but I'm not the appropriate person to design that.

@schomatis schomatis added exp/intermediate Prior experience is likely helpful effort/days Estimated to take multiple days, but less than a week and removed exp/novice Someone with a little familiarity can pick up effort/hours Estimated to take one or several hours labels Jan 13, 2022
@schomatis schomatis removed their assignment Jan 13, 2022
@BigLep BigLep modified the milestones: go-ipfs 0.13, Best Effort Track Mar 3, 2022
@BigLep
Copy link
Contributor

BigLep commented Mar 3, 2022

@lidel : can you describe what the next steps here are?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/days Estimated to take multiple days, but less than a week exp/intermediate Prior experience is likely helpful kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up topic/commands Topic commands
Projects
No open projects
Status: 🥞 Todo
Development

No branches or pull requests

4 participants