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

Exit hubot on Unauthorized errors #178

Merged
merged 8 commits into from
Jun 26, 2019
Merged

Exit hubot on Unauthorized errors #178

merged 8 commits into from
Jun 26, 2019

Conversation

arm4b
Copy link
Member

@arm4b arm4b commented Jun 20, 2019

Addresses StackStorm/st2chatops#124
Accidentally Closes #157
Accidentally Closes StackStorm/st2#4119

This bugfix solves particular problem when invalid, expired ST2_API_KEY / ST2_AUTH_TOKEN or any other Unauthorized response from st2 server leads to bot exit.

Previously the bot was running forever with no exit, trying to re-auth in a loop every ST2_COMMANDS_RELOAD_INTERVAL (2mins). Now we deem such errors as fatal.

@arm4b arm4b added the bug label Jun 20, 2019
@arm4b arm4b requested review from blag and jinpingh June 20, 2019 19:30
@nmaludy
Copy link
Member

nmaludy commented Jun 20, 2019

This might also address #157

At least if the service was dying we would know there was an error instead of having it logged and then continue on like there was no problem, even though the connection failed and the service isn't working.

@arm4b
Copy link
Member Author

arm4b commented Jun 20, 2019

Thanks @nmaludy
Let me try to reproduce it from the instructions StackStorm/st2#4119

Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

Is it possible to add tests for ST2 returning an HTTP 401 when loading commands?

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

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

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.

arm4b pushed a commit to StackStorm/st2client.js that referenced this pull request Jun 20, 2019
This allows re-request st2 token in advance, instead of trying to get it when it's _already_ expired.
Add here HTTP request/response time to communicate token update and it leads to issues and races like StackStorm/st2#4119 StackStorm/hubot-stackstorm#178 (comment) StackStorm/hubot-stackstorm#157
@arm4b arm4b added the tests label Jun 25, 2019
@arm4b arm4b requested review from blag and jinpingh and removed request for jinpingh June 25, 2019 23:42
@arm4b
Copy link
Member Author

arm4b commented Jun 25, 2019

@blag added the tests, per your request.

Copy link
Contributor

@jinpingh jinpingh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

👍

@arm4b arm4b merged commit c692c6e into master Jun 26, 2019
@arm4b arm4b deleted the fix/exit-on-unauthorized branch June 26, 2019 11:03
arm4b pushed a commit to StackStorm/st2chatops that referenced this pull request Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

st2chatops is not reconnecting when auth token expires st2chatops not reconnecting after token expires
5 participants