Mattermost-server: Automatically retry on failed plugin installation from marketplace

[lieut-data]

When installing plugins in bulk – i.e. quickly click Install next to multiple plugins in the marketplace – some plugins fail installing in the first try but can get installed successfully after subsequent tries.

Let’s automatically retry failed plugin installations server side when using the marketplace before giving up and reporting the error back to the client.

---

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA: https://mattermost.atlassian.net/browse/MM-21216

[RajatVaryani]

@lieut-data I would like to work on it.

[RajatVaryani]

@lieut-data Few thoughts below,

I have used heimdall to solve this problem before. Here is the an example from the library. The problem with using it is that it does not give capability to override the round tripper(the way we have done in our code) implementation. I can reach out to the maintainers if we are using this route and I will raise the PR.

The other approach is that I can tweak our http service to provide retry with a linear backoff if this is a common requirement in our code.

Lastly, we can go the crude way to implement this retry mechanism only to be used in install plugin via marketplace call.

Thanks, Thoughts?

[lieut-data]

Thanks for teaching me about heimdall! (Love the name ;P) I'd propose we isolate this to the code installing the plugin from the marketplace, and separately evaluate making this more generic at the http service level if this proves useful in a different context. (I'd also suggest checking to see if anyone has already done this somewhere in our source, in case we can reuse same.)

[RajatVaryani]

Hello @lieut-data. This is entirely unrelated 👆🏽 but I attempted to modify InstallMarketplacePlugin function. I felt my changes have made code a bit more cleaner to read because

1. It gets rid of unnecessary mutation of `pluginFile, signatureFile at the cost of duplicating some part of code.
2. Removed nilness check for pluginFile, signatureFile because the methods if not returning any error will honour the values.

Thoughts? cc: @marianunez

[lieut-data]

@RajatVaryani, just a brief review, but superficially I'd agree with you that that makes the code simpler.

[RajatVaryani]

@marianunez Can you please review as well? 👆🏽

[marianunez]

@RajatVaryani I agree that some changes can be done there so the code is simpler and more understandable. However, I believe the check for signature file must still be done before calling InstallPluginWithSignature given that if null is passed it will skip verification.

@ali-farooq0 can you confirm my statement is correct?

[ali-farooq0]

@marianunez did you mean check the signature file before calling InstallMarketplacePlugin? if so then yes, i believe we need to check the signature file before invoking InstallPluginWithSignature. However if InstallPluginWithSignature does return an error if the signature file is nil(which i think it does) then the new changes that @RajatVaryani made should work just fine.

[marianunez]

However if InstallPluginWithSignature does return an error if the signature file is nil(which i think it does)

@ali-farooq0 can you double check this? I believe it's not returning an error if signature is nil, see https://github.com/mattermost/mattermost-server/blob/master/app/plugin_install.go#L143-L148

Is it intended to skip the signature verification if non is passed (which I believe was the case on that function) or should we add a check there to avoid this kind of confusion?

[ali-farooq0]

@marianunez ah yes, but since https://github.com/RajatVaryani/mattermost-server/commit/3e2d18f4b5b08adce30dd701654b2fb5348845c5#diff-ccbdc948bde42de2905077fcf0d55817R189 might be wrapping a nil []byte, that signature expression should be truthy and result in verifying the signature, as it's the wrapper that's being passed in.

However, i still agree with you that it might be risky to allow nils through, so i propose to add a nil check in InstallPluginWithSignature and return an error if signature is nil in that function.

[RajatVaryani]

Let me make the change as part of this issue.

[RajatVaryani]

Folks, I will finish the PR in this week.

[RajatVaryani]

Folks, I won't be able to contribute. Please make it open to the public.

[lieut-data]

Appreciate your efforts, @RajatVaryani!

[iamsayantan]

I would like to work on this.

[ali-farooq0]

You got it @iamsayantan!

[waqasraz]

also related to this, If I click on a plugin install and refresh the page it does not show which one is already installing, once the plugin is installed it shows up as installed.

[ali-farooq0]

also related to this, If I click on a plugin install and refresh the page it does not show which one is already installing, once the plugin is installed it shows up as installed.

@waqasraz That is working as designed, as we only store plugin install-ing state locally and not on the server, which is cleared as soon as you refresh the page.

[waqasraz]

@ali-farooq0 hmm maybe it does not work locally. I've tested it before adding a comment. I clicked on the install plugin then reloaded the page and click on Install again and again until it shows configured.

[waqasraz]

Also, I am unable to replicate the issue on a local machine. I clicked on all the market place plugins it worked fine. I did it multiple times.

[ali-farooq0]

Clicking install, refreshing page and clicking install again may result in unexpected behaviour as it's possible that the server is installing a plugin while you attempt to re-install. That is working as designed, as the server doesn't return any installing state to notify the webapp which plugins are currently being installed.

If you're not seeing installing when you click install the first time that is a bug and should be fixed.

This is the expected behaviour.

[waqasraz]

now I'm getting this

{"level":"error","ts":1585330609.6284842,"caller":"mlog/log.go:184","msg":"Failed to get plugins from the marketplace server.","path":"/api/v4/plugins/marketplace","request_id":"z7hyymnrebg37ksj865yrwoaya","ip_addr":"127.0.0.1","user_id":"38zfzcf6bbybup9ar81f3ouogh","method":"GET","err_where":"getRemotePlugins","http_code":500,"err_details":"Get https://api.integrations.mattermost.com/api/v1/plugins?filter=&local_only=false&page=0&server_version=5.22.0: net/http: TLS handshake timeout"}

[waqasraz]

This is what I mean, it's a bit long gif but give you the idea.

[ali-farooq0]

Ah most interesting. In your gif I do see an error as soon as you click Install, but not sure why when you refresh the 2nd time it shows as Configure, unless you have local code that retries to install the plugin? Or the other possibility is that you're seeing a client side failure while the server is installing and once the installation is done you see the Configure button on refresh.

[waqasraz]

handshake error disappear for some reason.