From 756e0cd6427e5bc5c149d9ef57b9da15dca5bfe5 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Thu, 16 Aug 2018 13:13:49 -0400 Subject: [PATCH 1/5] feat(discovery-client): Add CLI commands to find and SSH into a robot --- discovery-client/README.md | 85 +++++++-- discovery-client/bin/discovery-ssh | 18 ++ discovery-client/package.json | 6 +- discovery-client/src/__tests__/client.test.js | 70 +++++--- discovery-client/src/cli.js | 165 ++++++++++++------ discovery-client/src/index.js | 37 ++-- 6 files changed, 272 insertions(+), 109 deletions(-) create mode 100755 discovery-client/bin/discovery-ssh diff --git a/discovery-client/README.md b/discovery-client/README.md index 97ea972ce62..a36fb4a0568 100644 --- a/discovery-client/README.md +++ b/discovery-client/README.md @@ -24,7 +24,7 @@ const options = { nameFilter: /^opentrons/i, allowedPorts: [31950], pollInterval: 5000, - candidates: [{ip: '[fd00:0:cafe:fefe::1]', port: 31950}, 'localhost'] + candidates: [{ ip: '[fd00:0:cafe:fefe::1]', port: 31950 }, 'localhost'] } const client = DiscoveryClientFactory(options) @@ -127,7 +127,8 @@ type Option = { /** * list of extra IP addresses to add to the search list - * default: {} + ip/port from all Robots in options.discovered + * note that candidates will bypass any name and IP filters + * default: [] */ candidates?: Array, @@ -138,9 +139,15 @@ type Option = { nameFilter?: string | RegExp, /** - * regexp or string (passed to `new RegExp`) to filter mDNS service names + * starting substring to filter mDNS service IPs * default: '' */ + ipFilter?: string, + + /** + * regexp or string (passed to `new RegExp`) to filter mDNS service names + * default: [] + */ allowedPorts?: Array, /** optional logger */ @@ -153,7 +160,7 @@ type Option = { If you need access to the `DiscoveryClient` class itself for some reason: ```js -import {DiscoveryClient} from '@opentrons/discovery-client' +import { DiscoveryClient } from '@opentrons/discovery-client' const client = new DiscoveryClient({}) ``` @@ -185,19 +192,65 @@ client.on('error', (error) => console.error(error) make -C discovery-client # run via yarn run -yarn run discovery [options] -# or run from node_modules directly -node_modules/.bin/discovery [options] +yarn run discovery [command] [options] + +# run via npx +npx discovery [command] [options] + +# run from node_modules directly +node_modules/.bin/discovery [command] [options] ``` -It will print out robots as it discovers them. It has the same options the API: +### global options -| api option | cli flag | example | -| -------------- | -------------------- | ------------------------------------ | -| `pollInterval` | `-p, --pollInterval` | `--pollInterval 1000` | -| `services` | `-s, --services` | `-s.0.name=foo -s.0.ip=192.168.1.42` | -| `candidates` | `-c, --candidates` | `-c localhost 192.168.1.42` | -| `nameFilter` | `-n, --nameFilter` | `-n opentrons` | -| `allowedPorts` | `-a, --allowedPorts` | `-a 31951 31952 31953` | +The CLI's global options are almost completely the same as the API's options, with the addition of `logLevel`: + +| flag | description | default | example | +| -------------------- | ------------------------- | -------- | ---------------- | +| `-p, --pollInterval` | see `pollInterval` option | `1000` | `-p 500` | +| `-c, --candidates` | see `candidates` option | `[]` | `-c localhost` | +| `-n, --nameFilter` | see `nameFilter` option | `''` | `-n opentrons` | +| `-i, --ipFilter` | see `ipFilter` option | `''` | `-i 169.254` | +| `-a, --allowedPorts` | see `allowedPorts` option | `[]` | `-a 31951 31952` | +| `-l, --logLevel` | log level for printout | `'info'` | `-l debug` | + +### `discovery (browse) [options]` + +Print out robots as it discovers them. + +```shell +# example: browse for robots, including at localhost +discovery browse -c localhost + +# browse is the default command, so you can leave the "browse" out +discovery --nameFilter moon +``` -`--services` and `--candidates` may be passed multiple times to add more than one service or candidate. +### `discovery find [name] [options]` + +Find the first robot you can, optionally specifying name or any other global options, and print out the IP address to `stdout` (bypassing any log level settings). + +```shell +# example: find a specific robot +discovery find opentrons-moon-moon + +# example: find the IP address of a link-local wired robot +discovery find --ipFilter 169.254 +``` + +#### command specific options + +| flag | description | default | example | +| --------------- | ---------------------------- | ------- | ---------- | +| `-t, --timeout` | How long to wait for a robot | `5000` | `-t 10000` | + +### `discovery-ssh [name] [options]` + +Calls `discovery find` and using the output to SSH into the robot it finds. Takes all the same arguments and options as `discovery find`. + +`discovery-ssh` is a Bash script, so it must be called from a command line with Bash available. + +```shell +# example: SSH into a link-local wired robot +discovery-ssh --ipFilter 169.254 +``` diff --git a/discovery-client/bin/discovery-ssh b/discovery-client/bin/discovery-ssh new file mode 100755 index 00000000000..d515b3586dd --- /dev/null +++ b/discovery-client/bin/discovery-ssh @@ -0,0 +1,18 @@ +#!/usr/bin/env bash +# usage: +# discovery-ssh # ssh into first robot we can find +# discovery-ssh name # ssh into the named robot +# discovery-ssh --nameFilter moon # ssh into first robot that matches filter +# +# all arguments are passed directly to `discovery find` + +dir=`dirname "$0"` +host=`$dir/discovery find $*` + +if [[ -z "$host" ]]; then + echo "No robot found" + exit 1 +fi; + +echo "SSH'ing into $host" +ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no "root@$host" diff --git a/discovery-client/package.json b/discovery-client/package.json index 45de3097b8f..9206d7ae170 100644 --- a/discovery-client/package.json +++ b/discovery-client/package.json @@ -4,7 +4,8 @@ "description": "Node.js client for discovering Opentrons robots on the network", "main": "lib/index.js", "bin": { - "discovery": "bin/index.js" + "discovery": "bin/index.js", + "discovery-ssh": "bin/discovery-ssh" }, "repository": { "type": "git", @@ -22,6 +23,7 @@ }, "dependencies": { "mdns-js": "^1.0.1", - "node-fetch": "^2.1.2" + "node-fetch": "^2.1.2", + "yargs": "^12.0.1" } } diff --git a/discovery-client/src/__tests__/client.test.js b/discovery-client/src/__tests__/client.test.js index 88e059a173a..f2e33d4f8a4 100644 --- a/discovery-client/src/__tests__/client.test.js +++ b/discovery-client/src/__tests__/client.test.js @@ -205,7 +205,7 @@ describe('discovery client', () => { ok: true } ], - candidates: [{ip: '192.168.1.43', port: 31950}] + candidates: [{ ip: '192.168.1.43', port: 31950 }] }) expect(client.services).toEqual([ @@ -217,7 +217,7 @@ describe('discovery client', () => { ok: null } ]) - expect(client.candidates).toEqual([{ip: '192.168.1.43', port: 31950}]) + expect(client.candidates).toEqual([{ ip: '192.168.1.43', port: 31950 }]) }) test('candidates should be deduped by services', () => { @@ -230,7 +230,7 @@ describe('discovery client', () => { ok: true } ], - candidates: [{ip: '192.168.1.42', port: 31950}] + candidates: [{ ip: '192.168.1.42', port: 31950 }] }) expect(client.candidates).toEqual([]) @@ -245,12 +245,12 @@ describe('discovery client', () => { port: 1 } ], - candidates: [{ip: 'bar', port: 2}, {ip: 'baz', port: 3}] + candidates: [{ ip: 'bar', port: 2 }, { ip: 'baz', port: 3 }] }) client.start() expect(poller.poll).toHaveBeenCalledWith( - [{ip: 'foo', port: 1}, {ip: 'bar', port: 2}, {ip: 'baz', port: 3}], + [{ ip: 'foo', port: 1 }, { ip: 'bar', port: 2 }, { ip: 'baz', port: 3 }], 5000, expect.any(Function), client._logger @@ -260,7 +260,7 @@ describe('discovery client', () => { test('client should have configurable poll interval', () => { const client = DiscoveryClient({ pollInterval: 1000, - candidates: [{ip: 'foo', port: 31950}] + candidates: [{ ip: 'foo', port: 31950 }] }) client.start() @@ -275,10 +275,10 @@ describe('discovery client', () => { test('client.stop should stop polling', () => { const client = DiscoveryClient() - poller.poll.mockReturnValueOnce({id: 'foobar'}) + poller.poll.mockReturnValueOnce({ id: 'foobar' }) client.start() client.stop() - expect(poller.stop).toHaveBeenCalledWith({id: 'foobar'}, client._logger) + expect(poller.stop).toHaveBeenCalledWith({ id: 'foobar' }, client._logger) }) test('if poll comes back bad, ok should be flagged false', () => { @@ -295,7 +295,7 @@ describe('discovery client', () => { client.start() const onHealth = poller.poll.mock.calls[0][2] - onHealth({ip: '192.168.1.42', port: 31950}, null) + onHealth({ ip: '192.168.1.42', port: 31950 }, null) expect(client.services[0].ok).toBe(false) expect(client.services).toHaveLength(1) }) @@ -322,7 +322,7 @@ describe('discovery client', () => { client.start() const onHealth = poller.poll.mock.calls[0][2] - onHealth({ip: '192.168.1.42', port: 31950}, {name: 'opentrons-dev'}) + onHealth({ ip: '192.168.1.42', port: 31950 }, { name: 'opentrons-dev' }) }, 10 ) @@ -330,7 +330,9 @@ describe('discovery client', () => { test( 'if health comes back for a candidate, it should be promoted', done => { - const client = DiscoveryClient({candidates: [{ip: 'foo', port: 31950}]}) + const client = DiscoveryClient({ + candidates: [{ ip: 'foo', port: 31950 }] + }) client.once('service', () => { expect(client.candidates).toEqual([]) @@ -347,7 +349,7 @@ describe('discovery client', () => { client.start() const onHealth = poller.poll.mock.calls[0][2] - onHealth({ip: 'foo', port: 31950}, {name: 'opentrons-dev'}) + onHealth({ ip: 'foo', port: 31950 }, { name: 'opentrons-dev' }) }, 10 ) @@ -356,12 +358,12 @@ describe('discovery client', () => { 'if health comes back with IP conflict, null out old service', done => { const client = DiscoveryClient({ - services: [{name: 'bar', ip: 'foo', port: 31950}] + services: [{ name: 'bar', ip: 'foo', port: 31950 }] }) client.once('service', () => { expect(client.services).toEqual([ - {name: 'bar', ip: null, port: 31950, ok: null}, + { name: 'bar', ip: null, port: 31950, ok: null }, { name: 'opentrons-dev', ip: 'foo', @@ -374,7 +376,7 @@ describe('discovery client', () => { client.start() const onHealth = poller.poll.mock.calls[0][2] - onHealth({ip: 'foo', port: 31950}, {name: 'opentrons-dev'}) + onHealth({ ip: 'foo', port: 31950 }, { name: 'opentrons-dev' }) }, 10 ) @@ -382,7 +384,7 @@ describe('discovery client', () => { test('if new service is added, poller is restarted', () => { const client = DiscoveryClient() - poller.poll.mockReturnValueOnce({id: 1234}) + poller.poll.mockReturnValueOnce({ id: 1234 }) client.start() expect(poller.poll).toHaveBeenLastCalledWith( @@ -393,9 +395,9 @@ describe('discovery client', () => { ) client.once('service', robot => { - expect(poller.stop).toHaveBeenLastCalledWith({id: 1234}, client._logger) + expect(poller.stop).toHaveBeenLastCalledWith({ id: 1234 }, client._logger) expect(poller.poll).toHaveBeenLastCalledWith( - [{ip: '192.168.1.42', port: 31950}], + [{ ip: '192.168.1.42', port: 31950 }], expect.anything(), expect.anything(), client._logger @@ -439,10 +441,10 @@ describe('discovery client', () => { ] }) - poller.poll.mockReturnValueOnce({id: 1234}) + poller.poll.mockReturnValueOnce({ id: 1234 }) client.start() client.remove('opentrons-dev') - expect(poller.stop).toHaveBeenLastCalledWith({id: 1234}, client._logger) + expect(poller.stop).toHaveBeenLastCalledWith({ id: 1234 }, client._logger) expect(poller.poll).toHaveBeenLastCalledWith( [], expect.anything(), @@ -469,7 +471,7 @@ describe('discovery client', () => { } ] - const client = DiscoveryClient({services}) + const client = DiscoveryClient({ services }) client.on('serviceRemoved', service => { expect(services).toContainEqual(service) @@ -498,7 +500,7 @@ describe('discovery client', () => { }) test('can filter services by name', () => { - const client = DiscoveryClient({nameFilter: /^OPENTRONS/i}) + const client = DiscoveryClient({ nameFilter: /^OPENTRONS/i }) client.start() mdns.__mockBrowser.emit('update', BROWSER_SERVICE) @@ -517,8 +519,24 @@ describe('discovery client', () => { ]) }) + test('can filter services by ip', () => { + const client = DiscoveryClient({ ipFilter: '169.254' }) + + client.start() + mdns.__mockBrowser.emit('update', { + ...BROWSER_SERVICE, + addresses: ['169.254.1.2'] + }) + mdns.__mockBrowser.emit('update', { + ...BROWSER_SERVICE, + addresses: ['192.168.3.4'] + }) + + expect(client.services.map(s => s.ip)).toEqual(['169.254.1.2']) + }) + test('can filter services by port', () => { - const client = DiscoveryClient({allowedPorts: [31950, 31951]}) + const client = DiscoveryClient({ allowedPorts: [31950, 31951] }) client.start() mdns.__mockBrowser.emit('update', BROWSER_SERVICE) @@ -540,7 +558,7 @@ describe('discovery client', () => { const client = DiscoveryClient() const result = client.add('localhost').add('localhost') - const expectedCandidates = [{ip: 'localhost', port: null}] + const expectedCandidates = [{ ip: 'localhost', port: null }] expect(result).toBe(client) expect(client.candidates).toEqual(expectedCandidates) expect(poller.poll).toHaveBeenLastCalledWith( @@ -552,8 +570,8 @@ describe('discovery client', () => { }) test('can change polling interval on the fly', () => { - const client = DiscoveryClient({candidates: ['localhost']}) - const expectedCandidates = [{ip: 'localhost', port: null}] + const client = DiscoveryClient({ candidates: ['localhost'] }) + const expectedCandidates = [{ ip: 'localhost', port: null }] let result = client.setPollInterval(1000) expect(result).toBe(client) diff --git a/discovery-client/src/cli.js b/discovery-client/src/cli.js index 267b1c5c845..c789ad29f5f 100755 --- a/discovery-client/src/cli.js +++ b/discovery-client/src/cli.js @@ -1,56 +1,123 @@ // @flow -import DiscoveryClient, {DEFAULT_POLL_INTERVAL} from '.' - -const LOG_LVL = ['error', 'warn', 'info', 'http', 'verbose', 'debug', 'silly'] - .indexOf(process.env.OT_LOG_LEVEL || 'info') - -const noop = () => {} - -const argv = require('yargs-parser')(process.argv.slice(2), { - number: ['pollInterval'], - string: ['nameFilter'], - array: ['services', 'candidates', 'allowedPorts'], - default: { - pollInterval: DEFAULT_POLL_INTERVAL, - services: [], - candidates: [], - allowedPorts: [], - nameFilter: '' - }, - alias: { - p: 'pollInterval', - s: 'services', - c: 'candidates', - n: 'nameFilter', - a: 'allowedPorts' - }, - coerce: { - services: objectNotationToArray, - candidates: objectNotationToArray - } -}) - -const logger = { - error: LOG_LVL >= 0 ? console.error : noop, - warn: LOG_LVL >= 1 ? console.warn : noop, - info: LOG_LVL >= 2 ? console.info : noop, - http: LOG_LVL >= 3 ? console.debug : noop, - verbose: LOG_LVL >= 4 ? console.debug : noop, - debug: LOG_LVL >= 5 ? console.debug : noop, - silly: LOG_LVL >= 6 ? console.debug : noop +import DiscoveryClient from '.' + +const LOG_LVLS = ['error', 'warn', 'info', 'http', 'verbose', 'debug', 'silly'] +const noop = (...args: Array<*>) => {} + +require('yargs') + .options({ + pollInterval: { + describe: 'How often the health poller hits every IP address', + alias: 'p', + default: 1000, + type: 'number' + }, + nameFilter: { + describe: 'Filter mDNS advertisements by name', + alias: 'n', + default: '', + type: 'string' + }, + ipFilter: { + describe: 'Filter mDNS advertisements by subnet', + alias: 'i', + default: '', + type: 'string' + }, + allowedPorts: { + describe: 'Filter mDNS advertisements by port', + alias: 'a', + default: [], + type: 'array' + }, + candidates: { + describe: 'Extra IP addresses to poll outside of mDNS', + alias: 'c', + default: [], + type: 'array' + }, + logLevel: { + describe: 'Log level', + alias: 'l', + choices: [...LOG_LVLS, 'off'], + default: 'info' + } + }) + .env('OT_DC') + .middleware([addLogger, addHandleError, logArgv]) + .command(['$0', 'browse'], 'Browse for robots on the network', noop, browse) + .command( + 'find [name]', + 'Find the IP of a robot by its name', + yargs => { + yargs.positional('name', { + describe: 'Name of robot to find; if omitted will find first robot', + type: 'string' + }) + yargs.option('timeout', { + describe: 'How long to wait for a robot', + alias: 't', + default: 5000, + type: 'number' + }) + }, + find + ) + .version() + .help() + .parse() + +function browse (argv) { + DiscoveryClient(argv) + .on('service', s => argv.logger.info('service added or updated:', s)) + .on('serviceRemoved', s => argv.logger.info('service removed:', s)) + .once('error', argv.handleError) + .start() + + argv.logger.warn('Browsing for services') } -const client = DiscoveryClient(Object.assign({}, argv, {logger})) +function find (argv) { + setTimeout( + () => argv.handleError('Timed out waiting for robot'), + argv.timeout + ) + + DiscoveryClient(argv) + .on('service', s => { + if (!argv.name || s.name === argv.name) { + process.stdout.write(`${s.ip}\n`) + process.exit(0) + } + }) + .once('error', argv.handleError) + .start() -client - .on('service', s => console.info('service added or updated:', s)) - .on('serviceRemoved', s => console.info('service removed:', s)) - .on('error', console.error) - .start() + argv.logger.warn(`Finding robot with name: "${argv.name || ''}"`) +} -console.info('searching for services') +function addLogger (argv) { + const level = LOG_LVLS.indexOf(argv.logLevel) + + argv.logger = { + error: level >= 0 ? console.error : noop, + warn: level >= 1 ? console.warn : noop, + info: level >= 2 ? console.info : noop, + http: level >= 3 ? console.debug : noop, + verbose: level >= 4 ? console.debug : noop, + debug: level >= 5 ? console.debug : noop, + silly: level >= 6 ? console.debug : noop + } +} + +function addHandleError (argv) { + argv.handleError = error => { + argv.logger.error(error) + process.exit(1) + } +} -function objectNotationToArray (obj) { - if (Array.isArray(obj)) return obj - return Array.from(Object.assign({}, obj, {length: Object.keys(obj).length})) +function logArgv (argv) { + argv.logger.debug(`Calling ${argv.$0} with argv:`, argv) + return argv } diff --git a/discovery-client/src/index.js b/discovery-client/src/index.js index fcdde59049a..ac8fd958a70 100644 --- a/discovery-client/src/index.js +++ b/discovery-client/src/index.js @@ -5,7 +5,7 @@ import EventEmitter from 'events' import mdns from 'mdns-js' -import {poll, stop, type PollRequest} from './poller' +import { poll, stop, type PollRequest } from './poller' import { DEFAULT_PORT, fromMdnsBrowser, @@ -18,7 +18,7 @@ import { rejectCandidate } from './service' -import type {Browser, BrowserService} from 'mdns-js' +import type { Browser, BrowserService } from 'mdns-js' import type { Candidate, Service, @@ -34,6 +34,7 @@ type Options = { services?: Array, candidates?: Array, nameFilter?: string | RegExp, + ipFilter?: string, allowedPorts?: Array, logger?: Logger } @@ -48,7 +49,7 @@ export default function DiscoveryClientFactory (options?: Options) { export const SERVICE_EVENT: 'service' = 'service' export const SERVICE_REMOVED_EVENT: 'serviceRemoved' = 'serviceRemoved' export const DEFAULT_POLL_INTERVAL = 5000 -export {DEFAULT_PORT} +export { DEFAULT_PORT } export class DiscoveryClient extends EventEmitter { services: Array @@ -64,15 +65,16 @@ export class DiscoveryClient extends EventEmitter { super() // null out ok flag for pre-populated services - this.services = (options.services || []).map(s => ({...s, ok: null})) + this.services = (options.services || []).map(s => ({ ...s, ok: null })) // allow strings instead of full {ip: string, port: ?number} object this.candidates = (options.candidates || []) - .map(c => (typeof c === 'string' ? {ip: c, port: null} : c)) + .map(c => (typeof c === 'string' ? { ip: c, port: null } : c)) .filter(c => this.services.every(s => s.ip !== c.ip)) this._pollInterval = options.pollInterval || DEFAULT_POLL_INTERVAL this._nameFilter = new RegExp(options.nameFilter || '') + this._ipFilter = options.ipFilter || '' this._allowedPorts = [DEFAULT_PORT].concat(options.allowedPorts || []) this._logger = options.logger this._browser = null @@ -96,8 +98,8 @@ export class DiscoveryClient extends EventEmitter { add (ip: string, port?: number): DiscoveryClient { if (!this.candidates.some(c => c.ip === ip)) { - const candidate = {ip, port: port || null} - log(this._logger, 'debug', 'adding new unique candidate', {candidate}) + const candidate = { ip, port: port || null } + log(this._logger, 'debug', 'adding new unique candidate', { candidate }) this.candidates = this.candidates.concat(candidate) this._poll() } @@ -113,7 +115,7 @@ export class DiscoveryClient extends EventEmitter { removals.every(s => s.ip !== c.ip) ) - log(this._logger, 'debug', 'removed services from discovery', {removals}) + log(this._logger, 'debug', 'removed services from discovery', { removals }) this._poll() removals.forEach(s => this.emit(SERVICE_REMOVED_EVENT, s)) @@ -171,13 +173,16 @@ export class DiscoveryClient extends EventEmitter { } _handleUp (browserService: BrowserService): void { - log(this._logger, 'debug', 'mdns service detected', {browserService}) + log(this._logger, 'debug', 'mdns service detected', { browserService }) + + const service = fromMdnsBrowser(browserService) if ( - this._nameFilter.test(browserService.fullname) && + this._nameFilter.test(service.name) && + (service.ip || '').startsWith(this._ipFilter) && this._allowedPorts.includes(browserService.port) ) { - this._handleService(fromMdnsBrowser(browserService)) + this._handleService(service) } } @@ -187,16 +192,16 @@ export class DiscoveryClient extends EventEmitter { if (service) return this._handleService(service) // else, response was not ok, so unset ok flag in all matching ips - const {ip} = candidate + const { ip } = candidate const nextServices = this.services.map( - s => (s.ip === ip && s.ok !== false ? {...s, ok: false} : s) + s => (s.ip === ip && s.ok !== false ? { ...s, ok: false } : s) ) this._updateServiceList(nextServices) } _handleService (service: Service): mixed { - const {ok} = service + const { ok } = service const candidateExists = this.candidates.some(matchCandidate(service)) const serviceConflicts = this.services.filter(matchConflict(service)) const prevService = @@ -211,7 +216,7 @@ export class DiscoveryClient extends EventEmitter { nextServices = nextServices.map(s => { // if we have a service already, make sure not to reset ok to null if (s === prevService && s.ok !== ok && ok !== null) return service - if (serviceConflicts.includes(s)) return {...s, ip: null, ok: null} + if (serviceConflicts.includes(s)) return { ...s, ip: null, ok: null } return s }) @@ -230,7 +235,7 @@ export class DiscoveryClient extends EventEmitter { if (poll) this._poll() if (updated.length) { updated.forEach(s => this.emit(SERVICE_EVENT, s)) - log(this._logger, 'debug', 'updated services', {updated}) + log(this._logger, 'debug', 'updated services', { updated }) } } } From 85c81ae024149db90dc25fd8ceac9d8bccf8fc03 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Thu, 16 Aug 2018 14:29:16 -0400 Subject: [PATCH 2/5] fixup: fix name/ip/port filtering --- discovery-client/README.md | 8 ++++---- discovery-client/src/index.js | 22 +++++++++++----------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/discovery-client/README.md b/discovery-client/README.md index a36fb4a0568..edb895eb49e 100644 --- a/discovery-client/README.md +++ b/discovery-client/README.md @@ -127,25 +127,25 @@ type Option = { /** * list of extra IP addresses to add to the search list - * note that candidates will bypass any name and IP filters * default: [] */ candidates?: Array, /** - * regexp or string (passed to `new RegExp`) to filter mDNS service names + * regexp or string (passed to `new RegExp`) to filter services by name * default: '' */ nameFilter?: string | RegExp, /** - * starting substring to filter mDNS service IPs + * starting substring to filter services by IP * default: '' */ ipFilter?: string, /** - * regexp or string (passed to `new RegExp`) to filter mDNS service names + * array of numbers to filter services by port + * the default port of 31950 is always included * default: [] */ allowedPorts?: Array, diff --git a/discovery-client/src/index.js b/discovery-client/src/index.js index ac8fd958a70..42cb5270f96 100644 --- a/discovery-client/src/index.js +++ b/discovery-client/src/index.js @@ -174,16 +174,7 @@ export class DiscoveryClient extends EventEmitter { _handleUp (browserService: BrowserService): void { log(this._logger, 'debug', 'mdns service detected', { browserService }) - - const service = fromMdnsBrowser(browserService) - - if ( - this._nameFilter.test(service.name) && - (service.ip || '').startsWith(this._ipFilter) && - this._allowedPorts.includes(browserService.port) - ) { - this._handleService(service) - } + this._handleService(fromMdnsBrowser(browserService)) } _handleHealth (candidate: Candidate, response: ?HealthResponse): mixed { @@ -201,7 +192,16 @@ export class DiscoveryClient extends EventEmitter { } _handleService (service: Service): mixed { - const { ok } = service + const { name, ip, port, ok } = service + + if ( + !this._nameFilter.test(name) || + !(ip || '').startsWith(this._ipFilter) || + !this._allowedPorts.includes(port) + ) { + return + } + const candidateExists = this.candidates.some(matchCandidate(service)) const serviceConflicts = this.services.filter(matchConflict(service)) const prevService = From 80b846fe8ace2db38ed7510db1893425788612f3 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Fri, 17 Aug 2018 12:12:02 -0400 Subject: [PATCH 3/5] fixup: correct mdns types and make port non-optional --- app-shell/src/discovery.js | 3 +-- discovery-client/src/__tests__/client.test.js | 4 ++-- discovery-client/src/cli.js | 10 ++++++++- discovery-client/src/index.js | 10 ++++++--- discovery-client/src/poller.js | 3 +-- discovery-client/src/service.js | 21 ++++++++++++------- discovery-client/src/types.js | 4 ++-- flow-typed/npm/mdns-js_v1.0.x.js | 10 ++++----- 8 files changed, 41 insertions(+), 24 deletions(-) diff --git a/app-shell/src/discovery.js b/app-shell/src/discovery.js index 48c59bc2cb8..53e5feff223 100644 --- a/app-shell/src/discovery.js +++ b/app-shell/src/discovery.js @@ -8,7 +8,6 @@ import uniqBy from 'lodash/uniqBy' import Store from 'electron-store' import DiscoveryClient, { - DEFAULT_PORT, SERVICE_EVENT, SERVICE_REMOVED_EVENT } from '@opentrons/discovery-client' @@ -109,7 +108,7 @@ function serviceToConnection (service: Service): ?Connection { return { ip: service.ip, ok: service.ok, - port: service.port || DEFAULT_PORT, + port: service.port, local: isLocal(service.ip) } } diff --git a/discovery-client/src/__tests__/client.test.js b/discovery-client/src/__tests__/client.test.js index f2e33d4f8a4..b414bec23a8 100644 --- a/discovery-client/src/__tests__/client.test.js +++ b/discovery-client/src/__tests__/client.test.js @@ -558,7 +558,7 @@ describe('discovery client', () => { const client = DiscoveryClient() const result = client.add('localhost').add('localhost') - const expectedCandidates = [{ ip: 'localhost', port: null }] + const expectedCandidates = [{ ip: 'localhost', port: 31950 }] expect(result).toBe(client) expect(client.candidates).toEqual(expectedCandidates) expect(poller.poll).toHaveBeenLastCalledWith( @@ -571,7 +571,7 @@ describe('discovery client', () => { test('can change polling interval on the fly', () => { const client = DiscoveryClient({ candidates: ['localhost'] }) - const expectedCandidates = [{ ip: 'localhost', port: null }] + const expectedCandidates = [{ ip: 'localhost', port: 31950 }] let result = client.setPollInterval(1000) expect(result).toBe(client) diff --git a/discovery-client/src/cli.js b/discovery-client/src/cli.js index c789ad29f5f..24927902761 100755 --- a/discovery-client/src/cli.js +++ b/discovery-client/src/cli.js @@ -4,6 +4,8 @@ import DiscoveryClient from '.' const LOG_LVLS = ['error', 'warn', 'info', 'http', 'verbose', 'debug', 'silly'] const noop = (...args: Array<*>) => {} +const NORMALIZE_IP_RE = /\[?([a-f0-9.:]+)]?/i + require('yargs') .options({ pollInterval: { @@ -86,7 +88,7 @@ function find (argv) { DiscoveryClient(argv) .on('service', s => { if (!argv.name || s.name === argv.name) { - process.stdout.write(`${s.ip}\n`) + process.stdout.write(`${normalizeIp(s.ip)}\n`) process.exit(0) } }) @@ -96,6 +98,12 @@ function find (argv) { argv.logger.warn(`Finding robot with name: "${argv.name || ''}"`) } +// remove brackets from IPv6 +function normalizeIp (ip: string): string { + const match = ip.match(NORMALIZE_IP_RE) + return (match && match[1]) || '' +} + function addLogger (argv) { const level = LOG_LVLS.indexOf(argv.logLevel) diff --git a/discovery-client/src/index.js b/discovery-client/src/index.js index 42cb5270f96..db3f7ae39a6 100644 --- a/discovery-client/src/index.js +++ b/discovery-client/src/index.js @@ -10,6 +10,7 @@ import { DEFAULT_PORT, fromMdnsBrowser, fromResponse, + makeCandidate, toCandidate, matchService, matchUnassigned, @@ -69,7 +70,7 @@ export class DiscoveryClient extends EventEmitter { // allow strings instead of full {ip: string, port: ?number} object this.candidates = (options.candidates || []) - .map(c => (typeof c === 'string' ? { ip: c, port: null } : c)) + .map(c => (typeof c === 'string' ? makeCandidate(c) : c)) .filter(c => this.services.every(s => s.ip !== c.ip)) this._pollInterval = options.pollInterval || DEFAULT_POLL_INTERVAL @@ -98,7 +99,7 @@ export class DiscoveryClient extends EventEmitter { add (ip: string, port?: number): DiscoveryClient { if (!this.candidates.some(c => c.ip === ip)) { - const candidate = { ip, port: port || null } + const candidate = makeCandidate(ip, port) log(this._logger, 'debug', 'adding new unique candidate', { candidate }) this.candidates = this.candidates.concat(candidate) this._poll() @@ -174,7 +175,9 @@ export class DiscoveryClient extends EventEmitter { _handleUp (browserService: BrowserService): void { log(this._logger, 'debug', 'mdns service detected', { browserService }) - this._handleService(fromMdnsBrowser(browserService)) + const service = fromMdnsBrowser(browserService) + + if (service) this._handleService(service) } _handleHealth (candidate: Candidate, response: ?HealthResponse): mixed { @@ -199,6 +202,7 @@ export class DiscoveryClient extends EventEmitter { !(ip || '').startsWith(this._ipFilter) || !this._allowedPorts.includes(port) ) { + log(this._logger, 'debug', 'Ignoring service', service) return } diff --git a/discovery-client/src/poller.js b/discovery-client/src/poller.js index 5f4e96ab5cc..eccac10f66e 100644 --- a/discovery-client/src/poller.js +++ b/discovery-client/src/poller.js @@ -4,7 +4,6 @@ import fetch from 'node-fetch' import type {Candidate, Logger, HealthResponse} from './types' -import {DEFAULT_PORT} from './service' export type PollRequest = { id: ?IntervalID @@ -29,7 +28,7 @@ export function poll ( function pollIp () { const next = getNextCandidate() - const url = `http://${next.ip}:${next.port || DEFAULT_PORT}/health` + const url = `http://${next.ip}:${next.port}/health` fetch(url) .then(response => { diff --git a/discovery-client/src/service.js b/discovery-client/src/service.js index 58fecfa8d3a..1fc7ccd14de 100644 --- a/discovery-client/src/service.js +++ b/discovery-client/src/service.js @@ -1,9 +1,9 @@ // @flow // create Services from different sources import net from 'net' -import type {BrowserService, ServiceType} from 'mdns-js' +import type { BrowserService, ServiceType } from 'mdns-js' -import type {Service, Candidate, HealthResponse} from './types' +import type { Service, Candidate, HealthResponse } from './types' const nameExtractor = (st: ServiceType) => new RegExp(`^(.+)\\._${st.name}\\._${st.protocol}`) @@ -16,15 +16,22 @@ export function makeService ( port: ?number, ok: ?boolean ): Service { - return {name, ip, port, ok} + return { name, ip: ip || null, port: port || DEFAULT_PORT, ok: ok || null } } -export function fromMdnsBrowser (browserService: BrowserService): Service { - const {addresses, type, port, fullname} = browserService +export function makeCandidate (ip: string, port: ?number): Candidate { + return { ip, port: port || DEFAULT_PORT } +} + +export function fromMdnsBrowser (browserService: BrowserService): ?Service { + const { addresses, type, port, fullname } = browserService + + if (!type || !fullname) return null + let ip = addresses.find(address => net.isIPv4(address)) if (!ip) ip = addresses.find(address => net.isIP(address)) if (!ip) ip = browserService.host - if (net.isIPv6(ip)) ip = `[${ip}]` + if (ip && net.isIPv6(ip)) ip = `[${ip}]` const nameMatch = type[0] && fullname.match(nameExtractor(type[0])) const name = (nameMatch && nameMatch[1]) || fullname @@ -44,7 +51,7 @@ export function fromResponse ( export function toCandidate (service: Service): ?Candidate { if (!service.ip) return null - return {ip: service.ip, port: service.port} + return makeCandidate(service.ip, service.port) } export const matchService = (source: Service) => (target: Service) => diff --git a/discovery-client/src/types.js b/discovery-client/src/types.js index 4cd586f367f..650eec1196b 100644 --- a/discovery-client/src/types.js +++ b/discovery-client/src/types.js @@ -2,13 +2,13 @@ export type Candidate = { ip: string, - port: ?number + port: number } export type Service = { name: string, ip: ?string, - port: ?number, + port: number, ok: ?boolean } diff --git a/flow-typed/npm/mdns-js_v1.0.x.js b/flow-typed/npm/mdns-js_v1.0.x.js index ed9f10d9bc0..19b7d20e7c3 100644 --- a/flow-typed/npm/mdns-js_v1.0.x.js +++ b/flow-typed/npm/mdns-js_v1.0.x.js @@ -28,11 +28,11 @@ declare module 'mdns-js' { declare type BrowserService = { addresses: Array, query: Array, - type: Array, - txt: Array, - port: number, - fullname: string, - host: string, + type?: Array, + txt?: Array, + port?: number, + fullname?: string, + host?: string, interfaceIndex: number, networkInterface: string } From 03d548da5343608c1573eab034e48e13ce586d9f Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Mon, 20 Aug 2018 11:21:59 -0400 Subject: [PATCH 4/5] fixup: Add tarball build to makefile --- discovery-client/.npmignore | 5 +++++ discovery-client/Makefile | 10 ++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 discovery-client/.npmignore diff --git a/discovery-client/.npmignore b/discovery-client/.npmignore new file mode 100644 index 00000000000..6027969edba --- /dev/null +++ b/discovery-client/.npmignore @@ -0,0 +1,5 @@ +src +dist +__mocks__ +.babelrc +*.tgz diff --git a/discovery-client/Makefile b/discovery-client/Makefile index 63e134b9766..05edbf7b54d 100644 --- a/discovery-client/Makefile +++ b/discovery-client/Makefile @@ -19,7 +19,7 @@ env := cross-env NODE_ENV ##################################################################### .PHONY: all -all: clean lib +all: dist .PHONY: install install: @@ -27,7 +27,7 @@ install: .PHONY: clean clean: - shx rm -rf lib + shx rm -rf lib dist # artifacts ##################################################################### @@ -36,3 +36,9 @@ clean: lib: $(env)=production $(babel) $(flow_copy) + +.PHONY: dist +dist: clean lib + @mkdir -p dist + npm pack + shx mv '*.tgz' dist From 590963ba1cc092f1889385fcf0609edd4c9c225a Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Mon, 20 Aug 2018 13:25:38 -0400 Subject: [PATCH 5/5] fixup: Per feedback, allow filters to be arrays --- discovery-client/README.md | 29 +++++++++++------- discovery-client/package.json | 2 ++ discovery-client/src/__tests__/client.test.js | 8 ++--- discovery-client/src/cli.js | 14 ++++----- discovery-client/src/index.js | 30 +++++++++++++------ flow-typed/npm/escape-string-regexp_v1.x.x.js | 6 ++++ flow-typed/npm/to-regex_v3.0.x.js | 16 ++++++++++ 7 files changed, 74 insertions(+), 31 deletions(-) create mode 100644 flow-typed/npm/escape-string-regexp_v1.x.x.js create mode 100644 flow-typed/npm/to-regex_v3.0.x.js diff --git a/discovery-client/README.md b/discovery-client/README.md index edb895eb49e..f2dae9224c6 100644 --- a/discovery-client/README.md +++ b/discovery-client/README.md @@ -21,8 +21,8 @@ Creates a new `DiscoveryClient`. ```js const options = { - nameFilter: /^opentrons/i, - allowedPorts: [31950], + nameFilter: ['opentrons'], + portFilter: [31950], pollInterval: 5000, candidates: [{ ip: '[fd00:0:cafe:fefe::1]', port: 31950 }, 'localhost'] } @@ -132,23 +132,23 @@ type Option = { candidates?: Array, /** - * regexp or string (passed to `new RegExp`) to filter services by name + * RegExps or strings to filter services by name * default: '' */ - nameFilter?: string | RegExp, + nameFilter?: Array, /** - * starting substring to filter services by IP + * RegExps or strings to filter services by IP address * default: '' */ - ipFilter?: string, + ipFilter?: Array, /** * array of numbers to filter services by port * the default port of 31950 is always included * default: [] */ - allowedPorts?: Array, + portFilter?: Array, /** optional logger */ logger?: { @@ -209,9 +209,9 @@ The CLI's global options are almost completely the same as the API's options, wi | -------------------- | ------------------------- | -------- | ---------------- | | `-p, --pollInterval` | see `pollInterval` option | `1000` | `-p 500` | | `-c, --candidates` | see `candidates` option | `[]` | `-c localhost` | -| `-n, --nameFilter` | see `nameFilter` option | `''` | `-n opentrons` | -| `-i, --ipFilter` | see `ipFilter` option | `''` | `-i 169.254` | -| `-a, --allowedPorts` | see `allowedPorts` option | `[]` | `-a 31951 31952` | +| `-n, --nameFilter` | see `nameFilter` option | `[]` | `-n opentrons` | +| `-i, --ipFilter` | see `ipFilter` option | `[]` | `-i 169.254` | +| `-a, --portFilter` | see `portFilter` option | `[]` | `-a 31951 31952` | | `-l, --logLevel` | log level for printout | `'info'` | `-l debug` | ### `discovery (browse) [options]` @@ -236,6 +236,10 @@ discovery find opentrons-moon-moon # example: find the IP address of a link-local wired robot discovery find --ipFilter 169.254 + +# example: find the IP address of a wired robot that may be IPv4 or IPv6 +# (IPv6 means legacy non-mDNS wired configuration) +discovery find -i "169.254" "fd00" -c "[fd00:0:cafe:fefe::1]" ``` #### command specific options @@ -246,11 +250,14 @@ discovery find --ipFilter 169.254 ### `discovery-ssh [name] [options]` -Calls `discovery find` and using the output to SSH into the robot it finds. Takes all the same arguments and options as `discovery find`. +Calls `discovery find` and using the output to SSH into the robot it finds. **Takes all the same arguments and options as `discovery find`.** `discovery-ssh` is a Bash script, so it must be called from a command line with Bash available. ```shell # example: SSH into a link-local wired robot discovery-ssh --ipFilter 169.254 + +# example: SSH into any wired robot, including legacy wired configuration +discovery-ssh -i "169.254" "fd00" -c "[fd00:0:cafe:fefe::1]" ``` diff --git a/discovery-client/package.json b/discovery-client/package.json index 9206d7ae170..a40637d0249 100644 --- a/discovery-client/package.json +++ b/discovery-client/package.json @@ -22,8 +22,10 @@ "flow-typed": "^2.5.1" }, "dependencies": { + "escape-string-regexp": "^1.0.5", "mdns-js": "^1.0.1", "node-fetch": "^2.1.2", + "to-regex": "^3.0.2", "yargs": "^12.0.1" } } diff --git a/discovery-client/src/__tests__/client.test.js b/discovery-client/src/__tests__/client.test.js index b414bec23a8..8d8f0375f13 100644 --- a/discovery-client/src/__tests__/client.test.js +++ b/discovery-client/src/__tests__/client.test.js @@ -500,7 +500,7 @@ describe('discovery client', () => { }) test('can filter services by name', () => { - const client = DiscoveryClient({ nameFilter: /^OPENTRONS/i }) + const client = DiscoveryClient({ nameFilter: ['OPENTRONS'] }) client.start() mdns.__mockBrowser.emit('update', BROWSER_SERVICE) @@ -510,7 +510,7 @@ describe('discovery client', () => { }) mdns.__mockBrowser.emit('update', { ...BROWSER_SERVICE, - fullname: 'fopentrons._http._tcp.local' + fullname: 'apentrons._http._tcp.local' }) expect(client.services.map(s => s.name)).toEqual([ @@ -520,7 +520,7 @@ describe('discovery client', () => { }) test('can filter services by ip', () => { - const client = DiscoveryClient({ ipFilter: '169.254' }) + const client = DiscoveryClient({ ipFilter: ['169.254'] }) client.start() mdns.__mockBrowser.emit('update', { @@ -536,7 +536,7 @@ describe('discovery client', () => { }) test('can filter services by port', () => { - const client = DiscoveryClient({ allowedPorts: [31950, 31951] }) + const client = DiscoveryClient({ portFilter: [31950, 31951] }) client.start() mdns.__mockBrowser.emit('update', BROWSER_SERVICE) diff --git a/discovery-client/src/cli.js b/discovery-client/src/cli.js index 24927902761..e827025a7e5 100755 --- a/discovery-client/src/cli.js +++ b/discovery-client/src/cli.js @@ -15,18 +15,18 @@ require('yargs') type: 'number' }, nameFilter: { - describe: 'Filter mDNS advertisements by name', + describe: 'Filter found robots by name', alias: 'n', - default: '', - type: 'string' + default: [], + type: 'array' }, ipFilter: { - describe: 'Filter mDNS advertisements by subnet', + describe: 'Filter found robots by IP address', alias: 'i', - default: '', - type: 'string' + default: [], + type: 'array' }, - allowedPorts: { + portFilter: { describe: 'Filter mDNS advertisements by port', alias: 'a', default: [], diff --git a/discovery-client/src/index.js b/discovery-client/src/index.js index db3f7ae39a6..b56adf9ca96 100644 --- a/discovery-client/src/index.js +++ b/discovery-client/src/index.js @@ -4,6 +4,8 @@ import EventEmitter from 'events' import mdns from 'mdns-js' +import escape from 'escape-string-regexp' +import toRegex from 'to-regex' import { poll, stop, type PollRequest } from './poller' import { @@ -34,15 +36,20 @@ type Options = { pollInterval?: number, services?: Array, candidates?: Array, - nameFilter?: string | RegExp, - ipFilter?: string, - allowedPorts?: Array, + nameFilter?: Array, + ipFilter?: Array, + portFilter?: Array, logger?: Logger } const log = (logger: ?Logger, level: LogLevel, msg: string, meta?: {}) => logger && typeof logger[level] === 'function' && logger[level](msg, meta) +const santizeRe = (patterns: ?(Array)) => { + if (!patterns) return [] + return patterns.map(p => typeof p === 'string' ? escape(p) : p) +} + export default function DiscoveryClientFactory (options?: Options) { return new DiscoveryClient(options || {}) } @@ -52,6 +59,8 @@ export const SERVICE_REMOVED_EVENT: 'serviceRemoved' = 'serviceRemoved' export const DEFAULT_POLL_INTERVAL = 5000 export { DEFAULT_PORT } +const TO_REGEX_OPTS = {contains: true, nocase: true, safe: true} + export class DiscoveryClient extends EventEmitter { services: Array candidates: Array @@ -59,7 +68,8 @@ export class DiscoveryClient extends EventEmitter { _pollRequest: ?PollRequest _pollInterval: number _nameFilter: RegExp - _allowedPorts: Array + _ipFilter: RegExp + _portFilter: Array _logger: ?Logger constructor (options: Options) { @@ -74,11 +84,13 @@ export class DiscoveryClient extends EventEmitter { .filter(c => this.services.every(s => s.ip !== c.ip)) this._pollInterval = options.pollInterval || DEFAULT_POLL_INTERVAL - this._nameFilter = new RegExp(options.nameFilter || '') - this._ipFilter = options.ipFilter || '' - this._allowedPorts = [DEFAULT_PORT].concat(options.allowedPorts || []) + this._nameFilter = toRegex(santizeRe(options.nameFilter), TO_REGEX_OPTS) + this._ipFilter = toRegex(santizeRe(options.ipFilter), TO_REGEX_OPTS) + this._portFilter = [DEFAULT_PORT].concat(options.portFilter || []) this._logger = options.logger this._browser = null + + log(this._logger, 'silly', 'Created', this) } start (): DiscoveryClient { @@ -199,8 +211,8 @@ export class DiscoveryClient extends EventEmitter { if ( !this._nameFilter.test(name) || - !(ip || '').startsWith(this._ipFilter) || - !this._allowedPorts.includes(port) + !this._ipFilter.test(ip || '') || + !this._portFilter.includes(port) ) { log(this._logger, 'debug', 'Ignoring service', service) return diff --git a/flow-typed/npm/escape-string-regexp_v1.x.x.js b/flow-typed/npm/escape-string-regexp_v1.x.x.js new file mode 100644 index 00000000000..b2842c27b38 --- /dev/null +++ b/flow-typed/npm/escape-string-regexp_v1.x.x.js @@ -0,0 +1,6 @@ +// flow-typed signature: ed810fce1a8248c9f857e4b34ac68c14 +// flow-typed version: 94e9f7e0a4/escape-string-regexp_v1.x.x/flow_>=v0.28.x + +declare module 'escape-string-regexp' { + declare module.exports: (input: string) => string; +} diff --git a/flow-typed/npm/to-regex_v3.0.x.js b/flow-typed/npm/to-regex_v3.0.x.js new file mode 100644 index 00000000000..f151d16f6a6 --- /dev/null +++ b/flow-typed/npm/to-regex_v3.0.x.js @@ -0,0 +1,16 @@ +// flow-typed signature: 75a00bfb6d88f0eeab26b7cf7ea7f237 +// flow-typed version: <>/to-regex_v3.0.2/flow_v0.76.0 + +declare module 'to-regex' { + declare module.exports: ( + patterns: string | RegExp | Array, + options: { + contains?: boolean, + negate?: boolean, + nocase?: boolean, + flags?: string, + cache?: boolean, + safe?: boolean, + } + ) => RegExp; +};