Retry unauthorized HTTP request with a renewed JWT token

[didier-wenzek]

Signed-off-by: Didier Wenzek didier.wenzek@free.fr

Proposed changes

Address the two root causes of Unauthorized request:i

• Re-open the MQTT connection for each JWT token request.
• When an HTTP request to Cumulocity is rejected as unauthorized, then renew the JWT token and retry.
  Do this only once, since there is no point to get yet another fresh token if rejected twice in a row.

For the first point, the alternative is to keep the MQTT connection open and to have a background task that consumes all the JWT token - requested by the process or not, and to cache the latest one. However, this alternative introduces more complexity and opening an MQTT connection before each HTTP request should not be a performance issue for the current use-cases.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

See #1562

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s)
• I ran cargo fmt as mentioned in CODING_GUIDELINES
• I used cargo clippy as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[albinsuresh]

The try_get_internal_id function also needs to be updated with this retry logic.

[albinsuresh]

Isn't it better to have a dedicated test for this retry feature?

[didier-wenzek]

I agree that I have been a bit lazy here ;-) Will see if I find some time for that.

[didier-wenzek]

Added with 73d114f

[reubenmiller]

Looks good.

Moving forward we probably need a more generic retry handler to also handle failed requests due to network interruptions etc. But it is not for this ticket's scope as there would be additional aspects like backing off before retrying etc.

[reubenmiller]

(Deleted duplicate post)

[didier-wenzek]

The try_get_internal_id function also needs to be updated with this retry logic.

Done with 626d934

[albinsuresh]

Neat trick using a fresh connection for every token request. The fix wasn't obvious to me on a first look, as I was searching for the logic that iterates over the message stream for the most recent token.

[reubenmiller]

One of the checks is failing due to a fixup! commit, so just so we following the correct process could you please squash the changes to remove the fixup! commit?

[didier-wenzek]

Neat trick using a fresh connection for every token request. The fix wasn't obvious to me on a first look, as I was searching for the logic that iterates over the message stream for the most recent token.

I found no crystal clear way to get the most recent token. The main issue is that consuming messages from a subscription there is no way to know that we have read all the available messages.

[didier-wenzek]

One of the checks is failing due to a fixup! commit, so just so we following the correct process could you please squash the changes to remove the fixup! commit?

Sure, I will git rebase -i --autosquash main.
We use to do that only once the PR approved, the goal being to avoid push --force during the review process.

[reubenmiller]

One of the checks is failing due to a fixup! commit, so just so we following the correct process could you please squash the changes to remove the fixup! commit?

Sure, I will git rebase -i --autosquash main. We use to do that only once the PR approved, the goal being to avoid push --force during the review process.

Ok, thanks for the clarification. Approved

[reubenmiller]

Looks good

[albinsuresh]

Neat trick using a fresh connection for every token request. The fix wasn't obvious to me on a first look, as I was searching for the logic that iterates over the message stream for the most recent token.

I found no crystal clear way to get the most recent token. The main issue is that consuming messages from a subscription there is no way to know that we have read all the available messages.

This may have been an option: https://docs.rs/futures/0.3.25/futures/future/trait.FutureExt.html#method.now_or_never by repeatedly calling it until it returns None.

[didier-wenzek]

Neat trick using a fresh connection for every token request. The fix wasn't obvious to me on a first look, as I was searching for the logic that iterates over the message stream for the most recent token.

I found no crystal clear way to get the most recent token. The main issue is that consuming messages from a subscription there is no way to know that we have read all the available messages.

This may have been an option: https://docs.rs/futures/0.3.25/futures/future/trait.FutureExt.html#method.now_or_never by repeatedly calling it until it returns None.

Thanks for the hint. Indeed this might be useful. However, in that specific case, complexity arises due to a race condition between this call and the reception of a token. One might get None not because all the published token have been read, but because the latest taken as not been received yet.