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: add keysize through an option to spawn #203

Merged
merged 15 commits into from
Feb 21, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .aegir.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
const createServer = require('./src').createServer

const server = createServer() // using defaults

module.exports = {
karma: {
files: [{
Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@daviddias daviddias Feb 16, 2018

Choose a reason for hiding this comment

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

read: #188 (comment)

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

Copy link
Member

Choose a reason for hiding this comment

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

README needs to be updated to include initOptions

- `start` bool (default true) - should the node be started
- `repoPath` string - the repository path to use for this node, ignored if node is disposable
- `disposable` bool (default true) - a new repo is created and initialized for each invocation, as well as cleaned up automatically once the process exits
Expand Down
13 changes: 8 additions & 5 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
"lint": "aegir lint",
"docs": "aegir docs",
"build": "aegir build",
"test": "aegir test -t node -t browser --no-cors",
"test:node": "aegir test -t node",
"test:browser": "aegir test -t browser --no-cors",
"test": "cross-env IPFS_KEYSIZE=1024 aegir test -t node -t browser --no-cors",
"test:node": "cross-env IPFS_KEYSIZE=1024 aegir test -t node",
"test:browser": "cross-env IPFS_KEYSIZE=1024 aegir test -t browser --no-cors",
"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",
Copy link
Member

Choose a reason for hiding this comment

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

@dryajov please set these values on tests themselves.

"coverage-publish": "aegir coverage -u"
},
"browser": {
Expand Down Expand Up @@ -78,17 +78,19 @@
"detect-node": "^2.0.3",
"hapi": "^16.6.2",
"hat": "0.0.3",
"ipfs-api": "^17.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

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

"ipfs-repo": "^0.18.5",
"joi": "^13.0.2",
"lodash.clone": "^4.5.0",
"lodash.defaults": "^4.2.0",
"lodash.defaultsdeep": "^4.6.0",
"multiaddr": "^3.0.2",
"once": "^1.4.0",
"readable-stream": "^2.3.3",
"rimraf": "^2.6.2",
"safe-json-parse": "^4.0.0",
"safe-json-stringify": "^1.0.4",
"shutdown": "^0.3.0",
"ipfs-api": "^17.3.0",
"stream-http": "^2.7.2",
"subcomandante": "^1.0.5",
"superagent": "^3.8.2",
Expand All @@ -97,6 +99,7 @@
"devDependencies": {
"aegir": "^12.4.0",
"chai": "^4.1.2",
"cross-env": "^5.1.3",
"detect-port": "^1.2.2",
"dirty-chai": "^2.0.1",
"go-ipfs-dep": "0.4.13",
Expand Down
1 change: 1 addition & 0 deletions src/factory-daemon.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class FactoryDaemon {
*
* Options are:
* - `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
Copy link
Member

Choose a reason for hiding this comment

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

Update the README as well

* - `start` bool - should the node be started
* - `repoPath` string - the repository path to use for this node, ignored if node is disposable
* - `disposable` bool - a new repo is created and initialized for each invocation
Expand Down
3 changes: 2 additions & 1 deletion src/factory-in-proc.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ class FactoryInProc {
* Spawn JSIPFS instances
*
* Options are:
* - `init` bool - should the node be initialized
* - `init` {bool|Object} - should the node be initialized
* - `initOpts` Object, it is expected to be of the form `{bits: <size>}`, which sets the desired key size
* - `start` bool - should the node be started
* - `repoPath` string - the repository path to use for this node, ignored if node is disposable
* - `disposable` bool - a new repo is created and initialized for each invocation
Expand Down
63 changes: 50 additions & 13 deletions src/ipfsd-daemon.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,26 @@

const fs = require('fs')
const waterfall = require('async/waterfall')
const series = require('async/series')
const ipfs = require('ipfs-api')
const multiaddr = require('multiaddr')
const rimraf = require('rimraf')
const path = require('path')
const once = require('once')
const truthy = require('truthy')
const flatten = require('./utils/flatten')
const defaults = require('lodash.defaults')
const debug = require('debug')
const os = require('os')
const hat = require('hat')
const log = debug('ipfsd-ctl:daemon')

const safeParse = require('safe-json-parse/callback')
const safeStringify = require('safe-json-stringify')

const parseConfig = require('./utils/parse-config')
const tmpDir = require('./utils/tmp-dir')
const findIpfsExecutable = require('./utils/find-ipfs-executable')
const setConfigValue = require('./utils/set-config-value')
const configureNode = require('./utils/configure-node')
const run = require('./utils/run')

const GRACE_PERIOD = 10500 // amount of ms to wait before sigkill
Expand All @@ -42,8 +45,6 @@ class Daemon {
const type = truthy(process.env.IPFS_TYPE)

this.opts = opts || { type: type || 'go' }
this.opts.config = flatten(this.opts.config)

const td = tmpDir(opts.type === 'js')
this.path = this.opts.disposable
? td
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

L60 is redundant

Copy link
Member

Choose a reason for hiding this comment

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

s/initOpts/initOptions/g


if (this.opts.env) {
Object.assign(this.env, this.opts.env)
Expand Down Expand Up @@ -112,7 +114,7 @@ class Daemon {
* Initialize a repo.
*
* @param {Object} [initOpts={}]
* @param {number} [initOpts.keysize=2048] - The bit size of the identiy key.
* @param {number} [initOpts.bits=2048] - The bit size of the identiy key.
* @param {string} [initOpts.directory=IPFS_PATH] - The location of the repo.
* @param {string} [initOpts.pass] - The passphrase of the keychain.
* @param {function (Error, Node)} callback
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this repeating part of line 61?

Copy link
Member Author

Choose a reason for hiding this comment

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

init also accepts a keysize

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (bits) {
args.concat(['-b', bits])
}
if (initOpts.pass) {
args.push('--pass')
args.push('"' + initOpts.pass + '"')
}
log(`initializing with keysize: ${bits}`)
run(this, args, { env: this.env }, (err, result) => {
if (err) {
return callback(err)
}

configureNode(this, this.opts.config, (err) => {
if (err) {
return callback(err)
}

waterfall([
(cb) => this.getConfig(cb),
(conf, cb) => this.replaceConfig(defaults({}, this.opts.config, conf), cb)
], (err) => {
if (err) { return callback }
Copy link
Member

Choose a reason for hiding this comment

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

the callback needs to be called

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

this.clean = false
this.initialized = true
callback(null, this)
return callback(null, this)
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

please remove.

})
})
}
Expand Down Expand Up @@ -328,7 +338,7 @@ class Daemon {
cb
),
(config, cb) => {
if (!key) {
if (key === 'show') {
return safeParse(config, cb)
}
cb(null, config.trim())
Expand All @@ -348,6 +358,33 @@ class Daemon {
setConfigValue(this, key, value, callback)
}

/**
* Replace the current config with the provided one
*
* @param {object} config
* @param {function(Error)} callback
* @return {undefined}
*/
replaceConfig (config, callback) {
const tmpFile = path.join(os.tmpdir(), hat())
// 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...
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

series([
(cb) => fs.writeFile(tmpFile, safeStringify(config), cb),
(cb) => run(
this,
['config', 'replace', `${tmpFile}`],
{ env: this.env },
cb
)
], (err) => {
if (err) { return callback(err) }
fs.unlink(tmpFile, callback)
})
}

/**
* Get the version of ipfs
*
Expand Down
48 changes: 32 additions & 16 deletions src/ipfsd-in-proc.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
'use strict'

const eachOf = require('async/eachOf')
const multiaddr = require('multiaddr')
const defaults = require('lodash.defaultsdeep')
const defaultsDeep = require('lodash.defaultsdeep')
const createRepo = require('./utils/repo/create-nodejs')
const flatten = require('./utils/flatten')
const defaults = require('lodash.defaults')
const waterfall = require('async/waterfall')

/**
* ipfsd for a js-ipfs instance (aka in-process IPFS node)
Expand Down Expand Up @@ -32,8 +32,9 @@ class Node {
this._started = false
this.initialized = false
this.api = null
this.bits = this.opts.initOpts ? this.opts.initOpts.bits : process.env.IPFS_KEYSIZE

this.opts.EXPERIMENTAL = defaults({}, opts.EXPERIMENTAL, {
this.opts.EXPERIMENTAL = defaultsDeep({}, opts.EXPERIMENTAL, {
pubsub: false,
sharding: false,
relay: {
Expand All @@ -55,6 +56,7 @@ class Node {
throw new Error('Unkown argument ' + arg)
}
})

this.exec = new IPFS({
repo: this.repo,
init: false,
Expand Down Expand Up @@ -114,7 +116,7 @@ class Node {
* Initialize a repo.
*
* @param {Object} [initOpts={}]
* @param {number} [initOpts.keysize=2048] - The bit size of the identiy key.
* @param {number} [initOpts.bits=2048] - The bit size of the identiy key.
* @param {string} [initOpts.directory=IPFS_PATH] - The location of the repo.
* @param {string} [initOpts.pass] - The passphrase of the keychain.
* @param {function (Error, Node)} callback
Expand All @@ -126,23 +128,26 @@ class Node {
initOpts = {}
}

initOpts.bits = initOpts.keysize || 2048
const bits = initOpts.keysize ? initOpts.bits : this.bits
// do not just set a default keysize,
// in case we decide to change it at
// the daemon level in the future
if (bits) {
initOpts.bits = bits
}
this.exec.init(initOpts, (err) => {
if (err) {
return callback(err)
}

const conf = flatten(this.opts.config)
eachOf(conf, (val, key, cb) => {
this.setConfig(key, val, cb)
}, (err) => {
if (err) {
return callback(err)
}

this.initialized = true
waterfall([
(cb) => this.getConfig(cb),
(conf, cb) => this.replaceConfig(defaults({}, this.opts.config, conf), cb)
], (err) => {
if (err) { return callback }
this.clean = false
callback(null, this)
this.initialized = true
return callback(null, this)
})
})
}
Expand Down Expand Up @@ -278,6 +283,17 @@ class Node {
this.exec.config.set(key, value, callback)
}

/**
* Replace the current config with the provided one
*
* @param {object} config
* @param {function(Error)} callback
* @return {undefined}
*/
replaceConfig (config, callback) {
this.exec.config.replace(config, callback)
}

/**
* Get the version of ipfs
*
Expand Down
3 changes: 3 additions & 0 deletions src/utils/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,8 @@ module.exports = (node, args, opts, callback) => {
executable = process.execPath
}

// Don't pass on arguments that were passed into the node executable
opts.execArgv = []

return exec(executable, args, opts, callback)
}
12 changes: 10 additions & 2 deletions test/spawn-options.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ const isNode = require('detect-node')
const hat = require('hat')
const IPFSFactory = require('../src')
const JSIPFS = require('ipfs')
const os = require('os')

const isWindows = os.platform() === 'win32'

const tests = [
{ type: 'go' },
Expand Down Expand Up @@ -112,6 +115,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 }
Copy link
Member

Choose a reason for hiding this comment

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

Please use a this.skip()

Copy link
Member Author

Choose a reason for hiding this comment

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

Skip doesn't work inside describes only it blocks.

Copy link
Member

Choose a reason for hiding this comment

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


// TODO: figure out why `proc` IPFS refuses
// to start with a provided repo
// `Error: Not able to start from state: uninitalized`
Expand Down Expand Up @@ -183,7 +189,7 @@ describe('Spawn options', () => {
})
})

describe('custom init options', () => {
describe('custom config options', () => {
it('custom config', function (done) {
this.timeout(40 * 1000)

Expand Down Expand Up @@ -215,6 +221,7 @@ describe('Spawn options', () => {
config = JSON.parse(config)
}
expect(config).to.eql([swarmAddr1])
// expect(config).to.include(swarmAddr1)
cb(null, ipfsd)
})
], (err, ipfsd) => {
Expand Down Expand Up @@ -344,7 +351,8 @@ describe('Spawn options', () => {
})
})

after((done) => {
after(function (done) {
this.timeout(50 * 1000)
ipfsd.stop(done)
})

Expand Down
Loading