-
-
Notifications
You must be signed in to change notification settings - Fork 38
Log errors and exit on authentication issues on Hubot plugin initialization #168
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 39 commits
8f7c74e
c2770bb
6613a3d
fe44e13
c3b451f
d44be18
3bf6fe2
51b0262
969c084
98cf5f2
233915c
0cf0e26
28d2e4c
76e7179
f588845
8518e92
52a4d0f
862a331
b112859
63abae1
97f2ef9
b07c28f
189b2d0
2ccc85e
e6c5e01
df5a0db
1765c47
7605f3c
0dcba8a
f81eca6
965ffb1
95a4434
a5311e9
c39ba0f
6a39dfb
9ac9aa7
9903e19
5c86bfa
6165f1e
6d11ae2
de756aa
59147a6
47639de
954ec13
1432afd
e7e9d93
f346581
82b8fe6
27523a0
7ee397f
0b75e5b
68d3b9d
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 |
---|---|---|
|
@@ -99,22 +99,20 @@ var TWOFACTOR_MESSAGE = "This action requires two-factor auth! Waiting for your | |
module.exports = function(robot) { | ||
slack_monkey_patch.patchSendMessage(robot); | ||
|
||
var self = this; | ||
|
||
var promise = Promise.resolve(); | ||
|
||
if (env.ST2_API) { | ||
robot.logger.warning("ST2_API is deprecated and will be removed in a future releases. Instead, please use the ST2_API_URL environment variable."); | ||
} | ||
var url = utils.parseUrl(env.ST2_API_URL); | ||
|
||
var opts = { | ||
protocol: url.protocol, | ||
host: url.hostname, | ||
port: url.port, | ||
prefix: url.path, | ||
rejectUnauthorized: false | ||
}; | ||
var _stream = null, | ||
self = this, | ||
promise = Promise.resolve(), | ||
url = utils.parseUrl(env.ST2_API_URL), | ||
opts = { | ||
protocol: url.protocol, | ||
host: url.hostname, | ||
port: url.port, | ||
prefix: url.path, | ||
rejectUnauthorized: false | ||
}; | ||
|
||
if (env.ST2_STREAM_URL) { | ||
var stream_url = utils.parseUrl(env.ST2_STREAM_URL); | ||
|
@@ -126,16 +124,30 @@ module.exports = function(robot) { | |
}; | ||
} | ||
|
||
var api = st2client(opts); | ||
var api_client = st2client(opts); | ||
|
||
if (env.ST2_API_KEY) { | ||
api.setKey({ key: env.ST2_API_KEY }); | ||
api_client.setKey({ key: env.ST2_API_KEY }); | ||
} else if (env.ST2_AUTH_TOKEN) { | ||
api.setToken({ token: env.ST2_AUTH_TOKEN }); | ||
api_client.setToken({ token: env.ST2_AUTH_TOKEN }); | ||
} | ||
|
||
function logErrorAndExit(err, res) { | ||
if (err && err.stack) { | ||
robot.logger.error(err.stack); | ||
} | ||
if (res) { | ||
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. I found that second param Does it make sense to move entire By throwing when we want to end an app (which will then call @blag What do you think? 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. I considered this and ultimately decided against it. We use 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. @jinpingh What do you think? 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.
Since we changed how 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. I'm actually talking about reverting initial design where we previously Here is the diff: - robot.error(logErrorAndExit);
- function logErrorAndExit(err, res) {
+ robot.error(function(err, res) {
if (err) {
robot.logger.error(err.stack);
}
if (res) {
res.send(JSON.stringify({
"status": "failed",
"msg": "An error occurred trying to post the message:\n" + err
}));
}
stop({shutdown: true});
- };
+ });
- logErrorAndExit('Failed to authenticate: ' + err.message);
+ throw 'Failed to authenticate: ' + err.message; By technical reasons I think that raising Add here cases when users want to re-use 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 isn't quite how the new code works. Calling If our users want to change or remove any of the error callbacks, they can still do so, they just have to manipulate 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. Making the hubot error handler an unnamed lambda function, and then simply re- |
||
res.send(JSON.stringify({ | ||
"status": "failed", | ||
"msg": "An error occurred trying to post the message:\n" + err | ||
})); | ||
} | ||
|
||
stop({shutdown: true}); | ||
} | ||
|
||
function authenticate() { | ||
api.removeListener('expiry', authenticate); | ||
api_client.removeListener('expiry', authenticate); | ||
|
||
// API key gets precedence 1 | ||
if (env.ST2_API_KEY) { | ||
|
@@ -152,7 +164,7 @@ module.exports = function(robot) { | |
|
||
var url = utils.parseUrl(env.ST2_AUTH_URL); | ||
|
||
var client = st2client({ | ||
var auth_client = st2client({ | ||
auth: { | ||
protocol: url.protocol, | ||
host: url.hostname, | ||
|
@@ -161,26 +173,28 @@ module.exports = function(robot) { | |
} | ||
}); | ||
|
||
return client.authenticate(env.ST2_AUTH_USERNAME, env.ST2_AUTH_PASSWORD) | ||
return auth_client.authenticate(env.ST2_AUTH_USERNAME, env.ST2_AUTH_PASSWORD) | ||
.then(function (token) { | ||
robot.logger.info('Token received. Expiring ' + token.expiry); | ||
api.setToken(token); | ||
client.on('expiry', authenticate); | ||
api_client.setToken(token); | ||
auth_client.on('expiry', authenticate); | ||
}) | ||
.catch(function (err) { | ||
robot.logger.error('Failed to authenticate: ' + err.message); | ||
arm4b marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
throw err; | ||
// Exit from invalid ST2_AUTH_USERNAME or ST2_AUTH_PASSWORD. | ||
robot.logger.error('Failed to authenticate with st2 username and password: ' + err.message); | ||
logErrorAndExit(err); | ||
}); | ||
} | ||
|
||
if (env.ST2_API_KEY || env.ST2_AUTH_TOKEN || env.ST2_AUTH_USERNAME || env.ST2_AUTH_PASSWORD) { | ||
// If using username and password then all are required. | ||
if ((env.ST2_AUTH_USERNAME || env.ST2_AUTH_PASSWORD) && | ||
!(env.ST2_AUTH_USERNAME && env.ST2_AUTH_PASSWORD && env.ST2_AUTH_URL)) { | ||
throw new Error('Env variables ST2_AUTH_USERNAME, ST2_AUTH_PASSWORD and ST2_AUTH_URL should only be used together.'); | ||
robot.logger.error('Environment variables ST2_AUTH_USERNAME, ST2_AUTH_PASSWORD and ST2_AUTH_URL should only be used together.'); | ||
stop({shutdown: true}); | ||
} else { | ||
promise = authenticate(); | ||
} | ||
promise = authenticate(); | ||
} | ||
|
||
// Pending 2-factor auth commands | ||
|
@@ -199,9 +213,9 @@ module.exports = function(robot) { | |
var postDataHandler = postData.getDataPostHandler(robot.adapterName, robot, formatter); | ||
|
||
var loadCommands = function() { | ||
robot.logger.info('Loading commands....'); | ||
robot.logger.info('Loading commands...'); | ||
|
||
api.actionAlias.list() | ||
api_client.actionAlias.list() | ||
.then(function (aliases) { | ||
// Remove all the existing commands | ||
command_factory.removeCommands(); | ||
|
@@ -237,6 +251,9 @@ module.exports = function(robot) { | |
.catch(function (err) { | ||
var error_msg = 'Failed to retrieve commands from "%s": %s'; | ||
jinpingh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
robot.logger.error(util.format(error_msg, env.ST2_API_URL, err.message)); | ||
if (err.status === 401 || err.message.includes("Unauthorized")) { | ||
blag marked this conversation as resolved.
Show resolved
Hide resolved
|
||
logErrorAndExit(err); | ||
jinpingh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}); | ||
}; | ||
|
||
|
@@ -263,7 +280,7 @@ module.exports = function(robot) { | |
var sendAliasExecutionRequest = function (msg, payload) { | ||
robot.logger.debug('Sending command payload:', JSON.stringify(payload)); | ||
|
||
api.aliasExecution.create(payload) | ||
api_client.aliasExecution.create(payload) | ||
.then(function (res) { sendAck(msg, res); }) | ||
.catch(function (err) { | ||
// Compatibility with older StackStorm versions | ||
|
@@ -307,7 +324,7 @@ module.exports = function(robot) { | |
var twofactor_id = uuid.v4(); | ||
robot.logger.debug('Requested an action that requires 2FA. Guid: ' + twofactor_id); | ||
msg.send(TWOFACTOR_MESSAGE); | ||
api.executions.create({ | ||
api_client.executions.create({ | ||
'action': env.HUBOT_2FA, | ||
'parameters': { | ||
'uuid': twofactor_id, | ||
|
@@ -326,7 +343,7 @@ module.exports = function(robot) { | |
}; | ||
|
||
robot.respond(/([\s\S]+?)$/i, function(msg) { | ||
var command, result, command_name, format_string, action_alias; | ||
var command, result; | ||
|
||
// Normalize the command and remove special handling provided by the chat service. | ||
// e.g. slack replace quote marks with left double quote which would break behavior. | ||
|
@@ -339,9 +356,7 @@ module.exports = function(robot) { | |
return; | ||
} | ||
|
||
command_name = result[0]; | ||
format_string = result[1]; | ||
action_alias = result[2]; | ||
var [command_name, format_string, action_alias] = result; | ||
|
||
executeCommand(msg, command_name, format_string, command, action_alias); | ||
}); | ||
|
@@ -357,25 +372,30 @@ module.exports = function(robot) { | |
} | ||
postDataHandler.postData(data); | ||
|
||
res.send('{"status": "completed", "msg": "Message posted successfully"}'); | ||
} catch (e) { | ||
robot.logger.error("Unable to decode JSON: " + e); | ||
robot.logger.error(e.stack); | ||
res.send('{"status": "failed", "msg": "An error occurred trying to post the message: ' + e + '"}'); | ||
res.send(JSON.stringify({ | ||
"status": "completed", | ||
"msg": "Message posted successfully" | ||
})); | ||
} catch (err) { | ||
robot.logger.error('Encountered an error when attempting to post ' + | ||
arm4b marked this conversation as resolved.
Show resolved
Hide resolved
|
||
'message to chat provider, execution results for ' + | ||
'chatops.post_message may not be accurate.'); | ||
robot.logger.error(err); | ||
} | ||
}); | ||
|
||
var commands_load_interval; | ||
|
||
function start() { | ||
api.stream.listen().catch(function (err) { | ||
robot.logger.error('Unable to connect to stream:', err); | ||
}).then(function (source) { | ||
source.onerror = function (err) { | ||
// TODO: squeeze a little bit more info out of evensource.js | ||
robot.logger.error('Stream error:', err); | ||
}; | ||
source.addEventListener('st2.announcement__chatops', function (e) { | ||
robot.error(logErrorAndExit); | ||
|
||
api_client.stream.listen().then(function (stream) { | ||
_stream = stream; // save stream for use in stop() | ||
stream.on('error', function (err) { | ||
robot.logger.error('StackStorm event stream error:', err); | ||
robot.logger.error('Implicitly attempting to reconnect to StackStorm event stream.'); | ||
}); | ||
stream.addEventListener('st2.announcement__chatops', function (e) { | ||
var data; | ||
|
||
robot.logger.debug('Chatops message received:', e.data); | ||
|
@@ -390,7 +410,7 @@ module.exports = function(robot) { | |
}); | ||
|
||
if (env.HUBOT_2FA) { | ||
source.addEventListener('st2.announcement__2fa', function (e) { | ||
stream.addEventListener('st2.announcement__2fa', function (e) { | ||
var data; | ||
|
||
robot.logger.debug('Successfull two-factor auth:', e.data); | ||
|
@@ -408,22 +428,29 @@ module.exports = function(robot) { | |
} | ||
}); | ||
|
||
// Add an interval which tries to re-load the commands | ||
commands_load_interval = setInterval(loadCommands.bind(self), (env.ST2_COMMANDS_RELOAD_INTERVAL * 1000)); | ||
|
||
// Initial command loading | ||
loadCommands(); | ||
|
||
// Add an interval which tries to re-load the commands | ||
commands_load_interval = setInterval(loadCommands.bind(self), (env.ST2_COMMANDS_RELOAD_INTERVAL * 1000)); | ||
|
||
// Install SIGUSR2 handler which reloads the command | ||
install_sigusr2_handler(); | ||
} | ||
|
||
function stop() { | ||
function stop(opts) { | ||
var default_opts = {shutdown: false}, | ||
opts = Object.assign(default_opts, (opts || {})); | ||
clearInterval(commands_load_interval); | ||
api.stream.listen().then(function (source) { | ||
source.removeAllListeners(); | ||
source.close(); | ||
}); | ||
|
||
if (opts.shutdown) { | ||
if (_stream) { | ||
_stream.removeAllListeners(); | ||
_stream.close(); | ||
} | ||
|
||
robot.shutdown(); | ||
} | ||
} | ||
|
||
function install_sigusr2_handler() { | ||
|
@@ -433,7 +460,7 @@ module.exports = function(robot) { | |
} | ||
|
||
// Authenticate with StackStorm backend and then call start. | ||
// On a failure to authenticate log the error but do not quit. | ||
// On a failure to authenticate log the error and quit. | ||
return promise.then(function () { | ||
start(); | ||
return stop; | ||
|
Uh oh!
There was an error while loading. Please reload this page.