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

feat: add keysize through an option to spawn #203

merged 15 commits into from
Feb 21, 2018

Conversation

dryajov
Copy link
Member

@dryajov dryajov commented Feb 16, 2018

wip, tests are on the way

@ghost ghost assigned dryajov Feb 16, 2018
@ghost ghost added the status/in-progress In progress label Feb 16, 2018
@dryajov
Copy link
Member Author

dryajov commented Feb 16, 2018

ref #188

README.md Outdated
@@ -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


this.keySize = process.env.IPFS_KEYSIZE

// option takes precedence over env variable
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍 for what @victorbjelkholm said.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

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.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@dryajov dryajov Feb 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Any programming language with correct scoping rules :)

@daviddias
Copy link
Member

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

@dryajov
Copy link
Member Author

dryajov commented Feb 16, 2018

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

@daviddias
Copy link
Member

@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

vmx and others added 3 commits February 17, 2018 12:49
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
Copy link

codecov bot commented Feb 17, 2018

Codecov Report

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

Impacted file tree graph

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

It's now working, see PR #205

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

waiting for a review by @diasdavid

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

package.json Outdated
"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.

* - `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
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

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

README.md Outdated
@@ -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.

README needs to be updated to include initOptions

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

s/initOpts/initOptions/g

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

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

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

@daviddias
Copy link
Member

What's missing in this PR? @dryajov ?

@dryajov
Copy link
Member Author

dryajov commented Feb 20, 2018

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
Copy link
Member

@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
Copy link
Member Author

dryajov commented Feb 20, 2018

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

@daviddias
Copy link
Member

@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
Copy link
Member Author

dryajov commented Feb 21, 2018

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

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

reply({ initialized: node.initialized })
reply({ initialized: true })
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

leftover from moving the env vars to a separate PR.

callback = initOpts
initOpts = {}
callback = initOptions
initOptions = {}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

leftovers.

@@ -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 }
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

})
})

describe('start and stop multiple times', () => {
// 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 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.

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

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

it('daemon should not be running', (done) => {
it('daemon should be running', (done) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@daviddias 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
{ type: 'go' },
{ type: 'js' },
{ type: 'go', bits: 1024 },
{ type: 'js', bits: 512 },
{ type: 'proc', exec: JSIPFS }
]
Copy link
Member

Choose a reason for hiding this comment

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

The proc node also should receive the bits.

@daviddias
Copy link
Member

Is there any blockers from getting Jenkins to pass?

@dryajov
Copy link
Member Author

dryajov commented Feb 21, 2018

no, I know what it is, pushing shortly.

@daviddias
Copy link
Member

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

image

{ type: 'go' },
{ type: 'js' },
{ type: 'go', bits: 1024 },
{ type: 'js', bits: 512 },
{ type: 'proc', exec: JSIPFS }
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget about his one.

@dryajov
Copy link
Member Author

dryajov commented Feb 21, 2018

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.

package.json Outdated
@@ -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

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

dryajov commented Feb 21, 2018

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

@daviddias
Copy link
Member

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
Copy link
Member

Seems that Travis is also not Happy:

image

@dryajov
Copy link
Member Author

dryajov commented Feb 21, 2018

yep, looking into it now.

@dryajov
Copy link
Member Author

dryajov commented Feb 21, 2018

@dryajov
Copy link
Member Author

dryajov commented Feb 21, 2018

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

@daviddias
Copy link
Member

@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
Copy link
Member Author

dryajov commented Feb 21, 2018

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

@daviddias daviddias merged commit 151303c into master Feb 21, 2018
@ghost ghost removed the status/in-progress In progress label Feb 21, 2018
@daviddias daviddias deleted the feat/keysize branch February 21, 2018 16:57
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.

5 participants