-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor fetch instance resource from API for error message generation #8
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
Changes from all commits
4c9bcd8
9e50eb7
74837e6
2bc920d
af96a83
f2555a7
5e5c9c1
e8c3e28
ab191aa
db767af
ab64255
b140690
c815c1d
5e78b79
0b64376
8b78d96
2d42b54
8972fe4
25b8f10
fc953de
b1ed80b
a8f0098
b1b83dd
68764a4
9e39139
b45b1e1
4e91d82
b9a3035
e7e34bf
730e20d
d08f22b
a4ea96a
7b45982
eb33f53
55a5e25
a3283df
9591b25
83a6ad7
ae7769f
181f42e
cf117d1
4c6656c
1606b54
326844a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,63 +1,231 @@ | ||
| /** | ||
| * @module app | ||
| */ | ||
| 'use strict'; | ||
|
|
||
| var ErrorCat = require('error-cat'); | ||
| var Runnable = require('runnable'); | ||
| var assign = require('101/assign'); | ||
| var bodyParser = require('body-parser'); | ||
| var express = require('express'); | ||
| var keypather = require('keypather')(); | ||
| var path = require('path'); | ||
| var bodyParser = require('body-parser'); | ||
| var put = require('101/put'); | ||
|
|
||
| var version = require('./package.json').version; | ||
| var app = express(); | ||
| var log = app.log = require('./logger')(__filename); | ||
| var version = require('./package.json').version; | ||
|
|
||
| // valid Detention request types (val for req validation) | ||
| var validDetentionTypes = [ | ||
| 'not_running', | ||
| 'ports', | ||
| 'signin', | ||
| 'unresponsive' | ||
| ]; | ||
|
|
||
| // view engine setup | ||
| app.set('views', path.join(__dirname, 'views')); | ||
| app.set('view engine', 'jade'); | ||
|
|
||
| var locals = { | ||
| version: version | ||
| assign(app.locals, { | ||
| localVersion: version, | ||
| absoluteUrl: process.env.ABSOLUTE_URL | ||
| }); | ||
|
|
||
| // this is used for hello runnable user so we only have to login once | ||
| var superUser = app.superUser = new Runnable(process.env.API_HOSTNAME, { | ||
| requestDefaults: { | ||
| headers: { | ||
| 'user-agent': 'detention-root' | ||
| }, | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * Authenticate with API as super user | ||
| * Must invoke before server begins listening | ||
| */ | ||
| app.loginSuperUser = function (cb) { | ||
| var logData = { | ||
| tx: true | ||
| }; | ||
| log.info(logData, 'api.loginSuperUser'); | ||
| superUser.githubLogin(process.env.HELLO_RUNNABLE_GITHUB_TOKEN, function (err) { | ||
| if (err) { | ||
| log.error(put({ | ||
| err: err | ||
| }, logData), 'loginSuperUser error'); | ||
| } else { | ||
| log.trace(logData, 'loginSuperUser success'); | ||
| } | ||
| cb(err); | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Validate query parameters | ||
| */ | ||
| app._validateRequest = function (req, res, next) { | ||
| log.info('app._validateRequest'); | ||
| if (!req.query.shortHash && req.query.type !== 'signin') { | ||
| log.trace('_validateRequest !shortHash'); | ||
| // TODO?: switch to createAndReport | ||
| return next(ErrorCat.create(500, 'instance shortHash required')); | ||
| } | ||
| if (!~validDetentionTypes.indexOf(req.query.type)) { | ||
| // only valid occurance if login error | ||
| log.trace('_validateRequest !type'); | ||
| // TODO?: switch to createAndReport | ||
| return next(ErrorCat.create(500, 'invalid request type')); | ||
| } | ||
| log.trace('_validateRequest success'); | ||
| next(); | ||
| }; | ||
|
|
||
| /** | ||
| * Fetch Instance resource from API | ||
| */ | ||
| app._fetchInstance = function (req, res, next) { | ||
| log.info({ | ||
| shortHash: req.query.shortHash | ||
| }, 'api._fetchInstance'); | ||
| if (req.query.type === 'signin') { | ||
| log.trace('_fetchInstance signin bypass'); | ||
| return next(); | ||
| } | ||
| req.instance = superUser.fetchInstance(req.query.shortHash, function (err) { | ||
| if (err) { | ||
| log.error({ | ||
| err: err | ||
| }, '_fetchInstance superUser.fetchInstance error'); | ||
| // TODO?: switch to createAndReport | ||
| return next(ErrorCat.create(404, 'instance not found')); | ||
| } | ||
| log.trace('_fetchInstance superUser.fetchInstance success'); | ||
| next(); | ||
| }); | ||
| }; | ||
| // uncomment after placing your favicon in /public | ||
| app.use(bodyParser.json()); | ||
| app.use(bodyParser.urlencoded({ extended: false })); | ||
| app.use(express.static(path.join(__dirname, 'public'))); | ||
|
|
||
| app.route('/*').get(function (req, res, next) { | ||
| var options = { | ||
| localVersion: version, | ||
| absoluteUrl: process.env.ABSOLUTE_URL || 'detention.runnable.io' | ||
| }; | ||
| if (req.query.type) { | ||
| var page = req.query.type; | ||
| [ | ||
| 'status', | ||
| 'branchName', | ||
| 'redirectUrl', | ||
| 'containerUrl', | ||
| 'ownerName', | ||
| 'instanceName' | ||
| ].forEach(function (option) { | ||
| options[option] = req.query[option]; | ||
| }); | ||
| if (req.query.ports) { | ||
| var value = req.query.ports; | ||
| if (!Array.isArray(value)) { | ||
| value = [value]; | ||
| } | ||
| options.ports = value; | ||
| /** | ||
| * Resolve instance status and render + return relevant error message html page | ||
| */ | ||
| app._processNaviError = function (req, res, next) { | ||
| log.info({ | ||
| query: req.query | ||
| }, 'processNaviError'); | ||
| var options = {}; | ||
|
|
||
| [ | ||
| 'redirectUrl', | ||
| 'shortHash' | ||
| ].forEach(function (option) { | ||
| options[option] = req.query[option]; | ||
| }); | ||
|
|
||
| if (req.instance) { | ||
| options.branchName = req.instance.getBranchName(); | ||
| // Temp missing pending resolution of SAN-3018 | ||
| // https://runnable.atlassian.net/browse/SAN-3018 | ||
| options.instanceName = keypather.get(req.instance, 'attrs.lowerName'); | ||
| options.ownerName = keypather.get(req.instance, 'attrs.owner.username'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is null, which makes the links (view logs, go to container) not work
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you see this? #8 (comment)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's weird that the comment wasn't on the full change... |
||
| var ports = keypather.get(req.instance, 'attrs.container.ports'); | ||
| if (ports) { | ||
| options.ports = Object.keys(ports).map(function (portKey) { | ||
| // Ex: '3000/tcp' --> '3000' | ||
| return portKey.replace(/\/tcp$/, ''); | ||
| }); | ||
| } else { | ||
| log.warn({ | ||
| instance: req.instance | ||
| }, '_processNaviError instance !ports'); | ||
| } | ||
| options.headerText = options.status; | ||
| if (options.status) { | ||
| if (options.status === 'buildFailed') { | ||
| options.headerText = 'build failed.'; | ||
| options.status = 'failed to build'; | ||
| } else { | ||
| options.status = 'is ' + options.status; | ||
| } | ||
| } | ||
|
|
||
| if (req.query.type === 'signin') { | ||
| log.trace('processNaviError type signin'); | ||
| return res.render('pages/signin', options); | ||
| } | ||
| if (req.query.type === 'not_running') { | ||
| log.trace('processNaviError type not_running'); | ||
|
|
||
| // container state error pages. | ||
| // - Not running (building, starting, crashed) | ||
| // - Running, but unresponsive | ||
| var status = req.instance.status(); | ||
| log.trace({ | ||
| status: status, | ||
| options: options | ||
| }, 'processNaviError instance status'); | ||
|
|
||
| options.status = status; | ||
| switch(status) { | ||
| case 'stopped': | ||
| case 'crashed': | ||
| case 'stopping': | ||
| options.headerText = 'is ' + status; | ||
| res.render('pages/dead', options); | ||
| break; | ||
| case 'running': | ||
| // The instance could have started after Navi fetched it and proxied to detention. | ||
| // Might not be the best idea to trigger a refresh, could easily result in user-unfriendly | ||
| // infinite redirect loops. Better to display an error page prompting user to refresh? | ||
| options.headerText = 'is running'; | ||
| res.render('pages/dead', options); | ||
| break; | ||
| case 'buildFailed': | ||
| options.headerText = 'build failed'; | ||
| res.render('pages/dead', options); | ||
| break; | ||
| case 'building': | ||
| options.headerText = 'is building'; | ||
| res.render('pages/dead', options); | ||
| break; | ||
| case 'neverStarted': | ||
| case 'starting': | ||
| options.headerText = 'is starting'; | ||
| res.render('pages/dead', options); | ||
| break; | ||
| case 'unknown': | ||
| options.headerText = 'unknown'; | ||
| res.render('pages/dead', options); | ||
| break; | ||
| } | ||
| res.render('pages/' + page, options); | ||
| } else { | ||
| res.render('pages/invalid', options); | ||
| return; | ||
| } | ||
| }); | ||
| if (req.query.type === 'ports'){ | ||
| log.trace('processNaviError type ports'); | ||
| /* | ||
| * Currently not implemented, might be bundled into 'unresponsive' | ||
| * | ||
| * Userland hipache will only route to navi if a request is made to an elastic url on a port | ||
| * that's explicitly set on the instance (we set hipache redis entries when ports are exposed) | ||
| * otherwise userland-hipache will return an error page due to a lack of a redis entry. | ||
| * | ||
| * Probably could fix by patching Hipache or perhaps reading the manual to see if there's a | ||
| * some kind of forward-for-all-ports functionality | ||
| * | ||
| * Anand if you read this Monday morning lets chat about it at 3pm | ||
| */ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't ports work right now? Or have I only been seeing them due to errors?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The unresponsive page will show you a list of ports. That's probably what you've been seeing. We don't have anything yet that can display a message if you try to access a port that isn't set as exposed on the instance document. We basically need to hack hipache to get that. |
||
| return; | ||
| } | ||
| if (req.query.type === 'unresponsive'){ | ||
| log.trace('processNaviError type unresponsive'); | ||
| res.render('pages/unresponsive', options); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| app.use(bodyParser.json()); | ||
| app.use(bodyParser.urlencoded({ extended: false })); | ||
| app.use(express.static(path.join(__dirname, 'public'))); | ||
|
|
||
| app.route('/*').get( | ||
| app._validateRequest, | ||
| app._fetchInstance, | ||
| app._processNaviError | ||
| ); | ||
|
|
||
| // catch 404 and forward to error handler | ||
| app.use(function(req, res, next) { | ||
|
|
@@ -66,15 +234,10 @@ app.use(function(req, res, next) { | |
| next(err); | ||
| }); | ||
|
|
||
| // error handlers | ||
|
|
||
| // production error handler | ||
| // no stacktraces leaked to user | ||
| app.use(function(err, req, res, next) { | ||
| res.render('pages/invalid', { | ||
| localVersion: version | ||
| }); | ||
| res.render('pages/invalid', {}); | ||
| }); | ||
|
|
||
|
|
||
| module.exports = app; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,10 +4,29 @@ | |
| * Module dependencies. | ||
| */ | ||
|
|
||
| var app = require('../app'); | ||
| var ErrorCat = require('error-cat'); | ||
| var debug = require('debug')('detention:server'); | ||
| var hasKeypaths = require('101/has-keypaths'); | ||
| var http = require('http'); | ||
|
|
||
| var app = require('../app'); | ||
| var log = require('../logger')(__filename); | ||
|
|
||
| var requiredEnvKeys = [ | ||
| 'HELLO_RUNNABLE_GITHUB_TOKEN', | ||
| 'API_HOSTNAME' | ||
| ]; | ||
| if (!hasKeypaths(process.env, requiredEnvKeys)) { | ||
| log.error({ | ||
| requiredEnvKeys: requiredEnvKeys, | ||
| env: process.env | ||
| }, 'Missing required ENV keys'); | ||
| // send to rollbar | ||
| ErrorCat.report(new Error('Detention missing required ENV values'), null, function () { | ||
| process.exit(1); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Get port from environment and store in Express. | ||
| */ | ||
|
|
@@ -25,7 +44,9 @@ var server = http.createServer(app); | |
| * Listen on provided port, on all network interfaces. | ||
| */ | ||
|
|
||
| server.listen(port); | ||
| app.loginSuperUser(function () { | ||
| server.listen(port); | ||
| }); | ||
| server.on('error', onError); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move this above app.login, was a bug before, if listen error sync it will not get called.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also where is
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahh nm, git diff messed me up
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was confused by what you thought you were seeing.... |
||
| server.on('listening', onListening); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 20 lines down
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahh nm, git diff messed me up |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| machine: | ||
| environment: | ||
| NODE_ENV: test | ||
| LOG_LEVEL_STDOUT: error | ||
| dependencies: | ||
| override: | ||
| - nvm install 4.2.0 | ||
| - nvm alias default 4.2.0 | ||
| - npm install -g npm@2.8.3 | ||
| - npm install | ||
| test: | ||
| pre: | ||
| - ulimit -n 10240 | ||
| override: | ||
| - npm run test |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /** | ||
| * @module lib/logger | ||
| */ | ||
| 'use strict'; | ||
|
|
||
| var bunyan = require('bunyan'); | ||
| var envIs = require('101/env-is'); | ||
| var path = require('path'); | ||
| var put = require('101/put'); | ||
|
|
||
| var logger = bunyan.createLogger({ | ||
| name: 'detention', | ||
| streams: [{ | ||
| level: process.env.LOG_LEVEL_STDOUT || 'trace', | ||
| stream: process.stdout | ||
| }], | ||
| serializers: bunyan.stdSerializers, | ||
| // DO NOT use src in prod, slow | ||
| src: !envIs('production'), | ||
| environment: process.env.NODE_ENV | ||
| }); | ||
|
|
||
| /** | ||
| * Return a new child bunyan instance | ||
| * @param {String} namespace | ||
| */ | ||
| module.exports = function (namespace) { | ||
| return logger.child({ | ||
| module: path.relative(process.cwd(), namespace) | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found in my testing that this coming back null (using prod api). I only get the githubId back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @Nathan219 this JIRA ticket is tracking that.
https://runnable.atlassian.net/browse/SAN-3018
Not a blocker for deploying the refactored detention as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye tho finding that.