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.

Sign up for GitHub

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

Exit hubot on Unauthorized errors #178

Merged
merged 8 commits into from
Jun 26, 2019
Merged

Exit hubot on Unauthorized errors #178

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9980b85
Exit hubot on unauthorized errors
Jun 20, 2019
6322ac7
Add a changelog record for Unauthorized fix
Jun 20, 2019
b9735a9
Exit bot on stream unauthorized errors as well
Jun 20, 2019
6904108
Merge branch 'master' into fix/exit-on-unauthorized
armab Jun 22, 2019
2fa40ac
Rely on fixed st2client v1.1.3
Jun 22, 2019
16a5946
Add a test for hubot to shutdown on 401 unauthorized error from API
Jun 25, 2019
92e4776
Bump version to 0.9.5
Jun 25, 2019
8b8504e
Use the updated COPYRIGHT header
Jun 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Changelog
in development
--------------
* Exit hubot on unhandled promise rejections and uncaught exceptions, usually caused by unrecoverable errors (bug fix)
* Exit hubot on invalid, expired `ST2_API_KEY` / `ST2_AUTH_TOKEN` or any other Unauthorized response from st2 server (bug fix)

0.9.3
-----
Expand Down
9 changes: 7 additions & 2 deletions scripts/stackstorm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,10 @@ module.exports = function(robot) {
robot.logger.info(command_factory.st2_hubot_commands.length + ' commands are loaded');
})
.catch(function (err) {
var error_msg = 'Failed to retrieve commands from "%s": %s';
robot.logger.error(util.format(error_msg, env.ST2_API_URL, err.message));
robot.logger.error(util.format('Failed to retrieve commands from "%s": %s', env.ST2_API_URL, err.message));
if (err.status === 401 || err.message.includes('Unauthorized')) {
throw err;
Copy link
Member Author

@arm4b arm4b Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

After more testing, not happy with this change either right now in case when st2 username & password used. This throw breaks any attempt of hubot-stackstorm to recover from failure and re-request st2 token as it happens normally (see #157 (comment)).

Here is how it works with current PR:

INFO Requesting a token...
INFO Loading commands....
ERROR Failed to retrieve commands from "https://localhost/api": Unauthorized - Token has expired.
ERROR {"name":"APIError","status":401,"message":"Unauthorized - Token has expired."}
INFO Hubot will shut down ...
INFO Disconnected from Slack RTM
INFO Exiting...

In this case request-token loop clashes in timing with the reload commands loop and so token was requested, but not received yet. At that exact moment reload commands loop happens that fails with 401 Token has expired because we hadn't enough time to receive the updated token.

To address that I think we need to modify the loop in st2client.js here: https://github.com/StackStorm/st2client.js/blob/46562a2b4959308d82998cc309b076f6c42d0ec2/index.js#L183-L201
and re-request token 1-3s earlier in advance. That will give enough timing window.

I think that might be one of the issues behind #157

Copy link
Member Author

@arm4b arm4b Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

StackStorm/st2client.js#69 should fix it which emits st2 client expiry token 10s earlier, giving some time to re-authenticate that should fix some race condition cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

@armab I think this is resolved with StackStorm/st2client.js#69 merged.

}
});
};

Expand Down Expand Up @@ -397,6 +399,9 @@ module.exports = function(robot) {
source.onerror = function (err) {
// TODO: squeeze a little bit more info out of evensource.js
robot.logger.error('Stream error:', err);
if (err.status === 401) {
throw err;
}
};
source.addEventListener('st2.announcement__chatops', function (e) {
var data;
Expand Down