feat: add keysize through an option to spawn #203

dryajov · 2018-02-16T01:36:59Z

wip, tests are on the way

dryajov · 2018-02-16T01:37:31Z

victorb · 2018-02-16T16:15:32Z

README.md

@@ -115,7 +115,8 @@ Install one or both of the following modules:
 Spawn the daemon

 - `options` is an optional object the following properties:
-  - `init` bool (default true) - should the node be initialized
+  - `init` bool (default true) or Object - should the node be initialized
+    - if `init` is an Object, it is expected to be of the form `{keySize: <bits>}`, this will set the `keysize` to initialize the daemon with


Not a fan of conditional types in a parameter. Either it's a Boolean or it's a Object. Rather, we should add a new option. If we're only adding keySize, why not make keySize a key of the options object?

I also think we call the option bits here, OR, change the option in go-ipfs as well. It's bits there currently, but keySize is probably a more descriptive name.

Thanks @victorbjelkholm, I thought about that too and you're right, I'll add another options initOpts and leave init as a bool.

For the second comment, keysize is used because historically that is what ipfsd-ctl used and it's the same name we use when passing that flag to init, and yes I think keysize is more descriptive.

read: #188 (comment)

I'm fine with init being boolean or object, but I'm fine with initOptions (not initOpts) too.

README needs to be updated to include initOptions

victorb · 2018-02-16T16:16:17Z

src/ipfsd-daemon.js

+
+    this.keySize = process.env.IPFS_KEYSIZE
+
+    // option takes precedence over env variable


I don't think this is right. First it should use the default value. Second it should use the passed in value and lastly, if there is a environment variable, use that.

This way we can override application configs when deploying etc.

👍 for what @victorbjelkholm said.

Wait, what @victorbjelkholm said reads as - default take precedence over env, which takes precedence over option?

Seems backwards, unless I'm reading it wrong. In any case, this is what I'm doing now - option takes precedence over ENV variable, which takes precedence over default. I think that is the canonical way of doing it and allows passing defaults with the ENV variable, but override them on a case by case basis in tests cases and elsewhere?

Default -> Options trumps Default -> Env trumps Options and Default

@dryajov I believe you are using precedence as "it comes before === less value" but the english definiton says "precedence: the condition of being considered more important than someone or something else; priority in importance, order, or rank." and so, if default took precedence over env, default values would always win.

option takes precedence over ENV variable, which takes precedence over default.

Which uses precedence in the english definition of the term.

I think the contention is whether ENV take precedence over option or vice versa. The case I'm making is that an ENV variable will define a global that should be overridable through an option when calling spawn. This allows to override the default globally and locally on a case by case basis.

So in my case it is -

Default -> Env trumps Default -> Options trumps Default and Env

But I'm not dead set on it, but it feels more natural to me.

It also follows well established scoping rules - IMO.

Updated with last line

@dryajov use #203 (comment), which is pretty much how every system works.

I wonder which system are you familiar with that uses it the other way around.

Any programming language with correct scoping rules :)

daviddias · 2018-02-16T17:29:34Z

@dryajov while at this, update the tests to use smaller keysizes as well so that they run faster :)

dryajov · 2018-02-16T17:55:17Z

@diasdavid @victorbjelkholm btw, any quick way of verifying that the keys were generated with the correct size?

daviddias · 2018-02-17T08:01:55Z

@dryajov not a deterministic one, other than a smaller key in bit size should be smaller in storage size. see https://github.com/libp2p/js-peer-id/blob/master/test/peer-id.spec.js#L124-L134

When running a script that involves `ipfsd-ctl` and you pass on additional parameters it used to pass on those parameters to the subprocesses it spaws. This lead to issues especially with `--inspect-brk` as it would spawn another inspector which then would fail because there is already one running on the same port. Fixes #202.

codecov · 2018-02-17T18:49:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cf794cd). Click here to learn what that means.
The diff coverage is 93.75%.

@@            Coverage Diff            @@
##             master     #203   +/-   ##
=========================================
  Coverage          ?   86.71%           
=========================================
  Files             ?       17           
  Lines             ?      670           
  Branches          ?        0           
=========================================
  Hits              ?      581           
  Misses            ?       89           
  Partials          ?        0

Impacted Files	Coverage Δ
src/factory-in-proc.js	`86.04% <ø> (ø)`
src/factory-daemon.js	`88.09% <100%> (ø)`
src/endpoint/routes.js	`80.89% <100%> (ø)`
src/utils/run.js	`72.72% <100%> (ø)`
src/ipfsd-client.js	`97.26% <100%> (ø)`
src/ipfsd-in-proc.js	`78.26% <91.66%> (ø)`
src/ipfsd-daemon.js	`91.83% <93.93%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf794cd...174e9dc. Read the comment docs.

richardschneider · 2018-02-17T19:13:35Z

test/start-stop.node.js

@@ -212,6 +211,9 @@ types.forEach((type) => {
    })

    describe('start and stop multiple times', () => {
+      // TODO: wont work on windows until we get /shutdown implemented in js-ipfs


It's now working, see PR #205

Nice... is it ready to review and merge?

do we need to wait for js-ipfs to get released?

nvm - ipfs/js-ipfs#1224

waiting for a review by @diasdavid

My PR has a link to js-ipfs/master (not using npm) so it can be used immediately.

daviddias · 2018-02-17T21:29:29Z

package.json

    "release": "aegir release",
    "release-minor": "aegir release --type minor",
    "release-major": "aegir release --type major",
-    "coverage": "COVERAGE=true aegir coverage --timeout 50000",
+    "coverage": "IPFS_KEYSIZE=1024 COVERAGE=true aegir coverage --timeout 50000",


@dryajov please set these values on tests themselves.

daviddias · 2018-02-17T21:29:48Z

src/factory-daemon.js

-   * - `init` {bool|Object} - should the node be initialized
-   *  - if `init` is an Object, it is expected to be of the form `{keySize: <bits>}`
+   * - `init` bool - should the node be initialized
+   * - `initOpts` Object, it is expected to be of the form `{bits: <size>}`, which sets the desired key size


Update the README as well

daviddias · 2018-02-17T21:30:06Z

src/ipfsd-daemon.js

-    if (typeof this.opts.init === 'object') {
-      this.keySize = this.opts.init.keySize
-    }
+    this.bits = this.opts.initOpts ? this.opts.initOpts.bits : process.env.IPFS_KEYSIZE


L60 is redundant

daviddias · 2018-02-17T22:04:46Z

README.md

@@ -115,7 +115,8 @@ Install one or both of the following modules:
 Spawn the daemon

 - `options` is an optional object the following properties:
-  - `init` bool (default true) - should the node be initialized
+  - `init` bool (default true) or Object - should the node be initialized
+    - if `init` is an Object, it is expected to be of the form `{keySize: <bits>}`, this will set the `keysize` to initialize the daemon with


README needs to be updated to include initOptions

daviddias · 2018-02-17T22:05:33Z

src/ipfsd-daemon.js

@@ -57,6 +58,7 @@ class Daemon {
    this._gatewayAddr = null
    this._started = false
    this.api = null
+    this.bits = this.opts.initOpts ? this.opts.initOpts.bits : process.env.IPFS_KEYSIZE


s/initOpts/initOptions/g

daviddias · 2018-02-17T22:06:14Z

src/ipfsd-daemon.js

@@ -128,24 +130,32 @@ class Daemon {
      this.path = initOpts.directory
    }

-    const args = ['init', '-b', initOpts.keysize || 2048]
+    const bits = initOpts.bits ? initOpts.bits : this.bits


Isn't this repeating part of line 61?

init also accepts a keysize

daviddias · 2018-02-17T22:07:25Z

src/ipfsd-daemon.js

+    const args = ['init']
+    // do not just set a default keysize,
+    // in case we decide to change it at
+    // the daemon level in the future


This sentence doesn't read right. Do you mean "Skip if default daemon keysize"?

Currently, ipfs daemons have a default keysize when using ipfs init - if we pass a value with ipfs init -b <keysize>, we're overriding that default (we're currently doing that in latest master). I don't think that is correct and we should rely on the default used by the daemon internally.

daviddias · 2018-02-17T22:07:48Z

src/ipfsd-daemon.js

+        (cb) => this.getConfig(cb),
+        (conf, cb) => this.replaceConfig(defaults({}, this.opts.config, conf), cb)
+      ], (err) => {
+        if (err) { return callback }


the callback needs to be called

daviddias · 2018-02-17T22:08:21Z

src/ipfsd-daemon.js

        this.clean = false
        this.initialized = true
-        callback(null, this)
+        return callback(null, this)


Init should not need to return a reference to the instance.

this is done for backwards compatibility, that is how we used to do it previous versions, plus it helps with chaining in async/waterfall type constructs - but not a real contention point for me, if noone objects, I'll remove.

please remove.

daviddias · 2018-02-17T22:09:36Z

src/ipfsd-daemon.js

+    // I wanted to use streams here, but js-ipfs doesn't
+    // read from stdin when providing '-' (or piping) like
+    // go-ipfs, and adding it right now seems like a fair
+    // bit of work, so we're using tmp file for now - not ideal...


PR with a WIP here ipfs/js-ipfs#785 really close to finish.

Can you update this comment to refer the WIP PR ipfs/js-ipfs#785 and add a proper TODO to come back here once that is done.

daviddias · 2018-02-20T07:45:17Z

What's missing in this PR? @dryajov ?

dryajov · 2018-02-20T20:44:26Z

We had this discussion in already, but I wanted to document why I believe that the approach of having ENV variables taking precedence over method arguments is wrong. First, to clarify one more time what I'm advocating for:

(top to bottom, top entry has highest precedence)

command line options/method arguments
env variables
default values

I don't want to talk specifically about the keysize, but in general about "environment setup" and precedence overall. I believe this is a subtle but very important issue, that can hinder flexibility and behave in a counter intuitive way for the user (this last one is subjective)

I did some research and while there don't seem to be any official standard for this, it is a widely used convention, a pretty representative one is gcc, from the official gcc docs:

https://gcc.gnu.org/onlinedocs/gcc/Environment-Variables.html (second paragraph)

Note that you can also specify places to search using options such as -B, -I and -L (see Directory Options). These take precedence over places specified using environment variables, which in turn take precedence over those specified by the configuration of GCC. See Controlling the Compilation Driver gcc in GNU Compiler Collection (GCC) Internals.

The second sentence lays out the rules of precedence that I listed above.

Why I think this is important.

Doing it this way allows us to have control over the application defaults, but don't prevent the user from overriding those when it is required. A few use cases that come to mind.

We want to override the default repo location (~/.ipfs) for disposable: false daemons, we can set the IPFS_PATH variable to an alternative path, but still allow all of our repo specific tests to run with the provided repo to spawn.
We want to set the default keysize to something other than 4096 (the default) system wide, we can use the IPFS_KEYSIZE variable, but still have specific tests that require running with different keysizes to continue doing so.

There are probably other cases that we would want to handle through environment variables that we haven't yet come across.

Another issue with having ENV taking precedence is unexpected behavior for the user (again this is subjective, but working with posix utils that follow this convention has made me expect this and so many other people, IMO). If an ENV var is set to some value, but I am explicitly passing a different value in my application, I expect that value to take precedence over the env - anything else is confusing.

A benefit of going with what i'm proposing here, is that it allows us to enable this across all of the pertinent repos (js-ipfs, ipfs-api, etc...) without modifying any of the tests - just pass IPFS_KEYSIZE=<size> to aegir in the package.json as I'm already doing it here.

Sorry for the long post and possibly "beating on a dead horse", and please do let me know where and if I'm missing the point.

@diasdavid let me know if you just want me to make the change in #203 (review) regardless.

daviddias · 2018-02-20T20:51:46Z

@dryajov thank you for the very well thought through proposal and literature check. For the sake of moving things along, the reality is that we don't need env vars to solve the slow tests problem. Can we take that feature out into a separate PR with that same proposal you made?

dryajov · 2018-02-20T20:57:04Z

@diasdavid I'll go ahead and make the changes in - #203 (review)

daviddias · 2018-02-21T08:11:33Z

@dryajov can I get a status update here? Is this PR ready? Have you tested it with the dependents to ensure everything works well? Is there PRs to update the keysize for those repos?

dryajov · 2018-02-21T08:23:15Z

This PR is ready. I'll open up PRs for the remaining repos.

daviddias · 2018-02-21T08:46:46Z

src/endpoint/routes.js

        if (err) {
          return reply(boom.badRequest(err))
        }

-        reply({ initialized: node.initialized })
+        reply({ initialized: true })


Why did this get removed? nodes[id] is still available within scope.

daviddias · 2018-02-21T08:47:45Z

src/ipfsd-daemon.js

@@ -57,6 +58,7 @@ class Daemon {
    this._gatewayAddr = null
    this._started = false
    this.api = null
+    this.bits = process.env.IPFS_KEYSIZE || (this.opts.initOptions ? this.opts.initOptions.bits : null)


leftover from moving the env vars to a separate PR.

daviddias · 2018-02-21T08:48:32Z

src/ipfsd-daemon.js

-      callback = initOpts
-      initOpts = {}
+      callback = initOptions
+      initOptions = {}


Does this mean that is mandatory to always pass a initOptions object? There should be a check to see if initOptions is a func.

we're checking for !callback which is not consistent with how do it elsewhere (leftover from prev codebase), I'll change it. Good catch.

daviddias · 2018-02-21T08:49:22Z

src/ipfsd-daemon.js

+    // I wanted to use streams here, but js-ipfs doesn't
+    // read from stdin when providing '-' (or piping) like
+    // go-ipfs, and adding it right now seems like a fair
+    // bit of work, so we're using tmp file for now - not ideal...


Can you update this comment to refer the WIP PR ipfs/js-ipfs#785 and add a proper TODO to come back here once that is done.

daviddias · 2018-02-21T08:49:31Z

src/ipfsd-in-proc.js

@@ -32,8 +35,9 @@ class Node {
    this._started = false
    this.initialized = false
    this.api = null
+    this.bits = process.env.IPFS_KEYSIZE || (this.opts.initOptions ? this.opts.initOptions.bits : null)


daviddias · 2018-02-21T08:50:24Z

test/add-retrieve.spec.js

@@ -25,7 +25,9 @@ describe('data can be put and fetched', () => {

      const f = IPFSFactory.create(dfOpts)

-      f.spawn((err, _ipfsd) => {
+      f.spawn({
+        initOptions: { bits: 1024 }


Put the initOptions as part of the tests array. This way you can do 512 for js-ipfs and 1024 for go-ipfs

daviddias · 2018-02-21T08:51:00Z

test/api.spec.js

@@ -110,7 +114,10 @@ describe('ipfsd.api for Daemons', () => {
    it('check if API and Gateway addrs are correct', function (done) {
      this.timeout(30 * 1000)


These timeouts are probably no longer needed (or less time) with shorter keysize. Please check all of them.

daviddias · 2018-02-21T08:51:25Z

test/spawn-options.spec.js

@@ -112,6 +116,9 @@ describe('Spawn options', () => {
      })

      describe('spawn from a initialized repo', () => {
+        // TODO: wont work on windows until we get `/shutdown` implemented in js-ipfs
+        if (isWindows) { return }


Please use a this.skip()

Skip doesn't work inside describes only it blocks.

#203 (comment)

daviddias · 2018-02-21T08:52:39Z

test/start-stop.node.js

      })
    })

    describe('start and stop multiple times', () => {
+      // TODO: wont work on windows until we get `/shutdown` implemented in js-ipfs
+      if (isWindows) { return }


Please use this.skip()

Skip doesn't work inside describes only it blocks.

@dryajov can we put that inside every it block then? It signals the viewer that something is not complete.

daviddias · 2018-02-21T08:53:01Z

test/start-stop.node.js

@@ -244,7 +264,7 @@ types.forEach((type) => {
        expect(ipfsd).to.exist()
      })

-      it('daemon should not be running', (done) => {
+      it('daemon should be running', (done) => {


Why this change?

It was misnamed, we are explicitly initing and starting the node.

daviddias · 2018-02-21T10:26:21Z

test/add-retrieve.spec.js

-  { type: 'go' },
-  { type: 'js' },
+  { type: 'go', bits: 1024 },
+  { type: 'js', bits: 512 },
  { type: 'proc', exec: JSIPFS }
 ]


The proc node also should receive the bits.

daviddias · 2018-02-21T10:36:06Z

Is there any blockers from getting Jenkins to pass?

dryajov · 2018-02-21T10:36:39Z

no, I know what it is, pushing shortly.

daviddias · 2018-02-21T10:37:28Z

Seems that -- ipfs/js-ipfs#1103 (comment) -- is also captured here:

daviddias · 2018-02-21T10:41:01Z

test/add-retrieve.spec.js

-  { type: 'go' },
-  { type: 'js' },
+  { type: 'go', bits: 1024 },
+  { type: 'js', bits: 512 },
  { type: 'proc', exec: JSIPFS }


Don't forget about his one.

dryajov · 2018-02-21T10:41:36Z

Yes, at this point I'm not sure if there is something running on those ports on mac os already, or something in ipfs or elsewhere is holding on to those ports.

daviddias · 2018-02-21T10:41:56Z

package.json

@@ -78,17 +78,19 @@
    "detect-node": "^2.0.3",
    "hapi": "^16.6.2",
    "hat": "0.0.3",
+    "ipfs-api": "^17.3.0",


While you are at it, mind upgrading ipfs-api to latest?

daviddias

Now we just need to get CI green and see those 3 PRs to ipfs/interop, js-ipfs-api and js-ipfs.

It would be rad to get the stats on how much time got shaved :)

dryajov · 2018-02-21T10:48:52Z

my simplistic time measurements have yielded ~2 mins :)!

daviddias · 2018-02-21T10:51:23Z

Last missing issue is: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfsd-ctl/detail/PR-203/17/pipeline/13#step-97-log-272 #203 (comment).

Let's handle that one in a separate PR so that we reduce the amount of time spending in testing in the other projects today and we figure out why Jenkins doesn't like how the factory-endpoint terminates.

daviddias · 2018-02-21T11:24:10Z

Seems that Travis is also not Happy:

dryajov · 2018-02-21T11:30:30Z

yep, looking into it now.

dryajov · 2018-02-21T11:57:38Z

PRs to use the new reduced keysizes

dryajov · 2018-02-21T12:03:37Z

ok, CIs are gree, I think this is ready?

daviddias · 2018-02-21T12:22:47Z

@dryajov you are missing the ipfs/interop tests, those daemons can have their keys set to 1024 bits.

Also, did you test everything locally? Everything 100% passing flawlessly?

dryajov · 2018-02-21T16:54:21Z

Updated interop as well. Looking into why ipfs-api is failing.

feat: add keysize through env or as an option to spawn

2595fc4

ghost assigned dryajov Feb 16, 2018

ghost added the status/in-progress In progress label Feb 16, 2018

dryajov mentioned this pull request Feb 16, 2018

Make testing fast again - Bring init options back #188

Closed

fix: lint

718e0ce

victorb reviewed Feb 16, 2018

View reviewed changes

daviddias mentioned this pull request Feb 16, 2018

Smaller key size when testing #198

Closed

vmx and others added 3 commits February 17, 2018 12:49

use initOpts istead of reusing init

c3fca62

feat: replace config on init instead of setting it once for each option

55142c5

use cross-env to set env vars and not break windows

17d578e

dryajov force-pushed the feat/keysize branch from 05c9d20 to 17d578e Compare February 17, 2018 18:55

test: skip multiple start on windows

f5f400e

richardschneider reviewed Feb 17, 2018

View reviewed changes

test: skip spawn from a initialized repo on windows

ee74427

daviddias requested changes Feb 17, 2018

View reviewed changes

daviddias mentioned this pull request Feb 19, 2018

⚡️ v0.28.0 RELEASE 🚀 ipfs/js-ipfs#1228

Closed

20 tasks

feat: small changes from review

b167a3c

fix: setting keysize on each tests

ac70cda

daviddias requested changes Feb 21, 2018

View reviewed changes

fix: several changes from review

6c7a028

dryajov force-pushed the feat/keysize branch from eace5bc to 6c7a028 Compare February 21, 2018 10:14

daviddias changed the title ~~feat: add keysize through env or as an option to spawn~~ feat: add keysize through an option to spawn Feb 21, 2018

daviddias requested changes Feb 21, 2018

View reviewed changes

feat: proc nodes should run whit bits

2ca9aa1

daviddias requested changes Feb 21, 2018

View reviewed changes

daviddias reviewed Feb 21, 2018

View reviewed changes

dryajov added 2 commits February 21, 2018 04:42

fix: dont forget about bits in add retrieve

1351dde

chore: update ipfs-api

fb30e0e

daviddias approved these changes Feb 21, 2018

View reviewed changes

timeouts

174e9dc

daviddias merged commit 151303c into master Feb 21, 2018

ghost removed the status/in-progress In progress label Feb 21, 2018

daviddias deleted the feat/keysize branch February 21, 2018 16:57

dryajov mentioned this pull request Feb 22, 2018

Implement correct ENV precedence handling #210

Closed


		this.keySize = process.env.IPFS_KEYSIZE

		// option takes precedence over env variable

		@@ -110,7 +114,10 @@ describe('ipfsd.api for Daemons', () => {
		it('check if API and Gateway addrs are correct', function (done) {
		this.timeout(30 * 1000)

feat: add keysize through an option to spawn #203

feat: add keysize through an option to spawn #203

Conversation

dryajov commented Feb 16, 2018

dryajov commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Feb 16, 2018

dryajov commented Feb 16, 2018

daviddias commented Feb 17, 2018

codecov bot commented Feb 17, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Feb 20, 2018

dryajov commented Feb 20, 2018

Why I think this is important.

daviddias commented Feb 20, 2018

dryajov commented Feb 20, 2018

daviddias commented Feb 21, 2018

dryajov commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Feb 21, 2018

dryajov commented Feb 21, 2018

daviddias commented Feb 21, 2018

Choose a reason for hiding this comment

dryajov commented Feb 21, 2018

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

daviddias Feb 16, 2018 •

edited

Loading

daviddias Feb 16, 2018 •

edited

Loading

dryajov Feb 16, 2018 •

edited

Loading

codecov bot commented Feb 17, 2018 •

edited

Loading

dryajov commented Feb 21, 2018 •

edited

Loading