Reload Server configuration when ConfigMap is updated

[agilgur5]

Summary

Currently, when the argo-workflows-workflow-controller-configmap is updated, the configuration is not reloaded, and so I have to restart some Deployments to get them to have the updated configuration.

Use Cases

For instance, when I was working on #10927, I had to change the scopes field, and this did not automatically apply in the Argo Server. For it to apply, I had to restart the Server Deployment. While this works, it can lead to a gotcha for some operators.

Automatically restarting Deployments when a ConfigMap changes is one of the primary purposes of a tool like Reloader, but it feels like maybe this should be built into Argo in some way instead of requiring a different third-party tool that is not mentioned in any docs.

Let me know what you think. This could be considered a bug instead of a feature as well, depending on how you look at it.

Root cause analysis

The config is generally reloaded when Get is called, but I see for SSO for the Server, it is only configured once during instantiation. As a result it is never reloaded if there is an update.

I'd be open to contributing this as well but may need a bit more guidance as this requires a bit of refactoring. And there are potentially multiple ways of achieving this with different trade-offs (e.g. goroutine to poll on configmap changes vs. always reading the configmap vs. caching etc) which would be for a maintainer to decide on the approach.

---

Message from the maintainers:

Love this enhancement proposal? Give it a 👍. We prioritise the proposals with the most 👍.

[tico24]

What workflows version are you running? I'm sure this was fixed in 3.4.6.

[agilgur5]

I'm on 3.3.8. #10660 looks like it's for the controller, not the server. The latest server code I linked to above does not seem to ever refresh the config (e.g. for SSO config).

[tico24]

Apologies, I mis-read.

[terrytangyuan]

@agilgur5 Would you like to submit a PR?

[agilgur5]

@terrytangyuan per the issue description, I'd be open to it, but this needs a bit of maintainer refinement in terms of what approach would be preferred to resolve this:

I'd be open to contributing this as well but may need a bit more guidance as this requires a bit of refactoring. And there are potentially multiple ways of achieving this with different trade-offs (e.g. goroutine to poll on configmap changes vs. always reading the configmap vs. caching etc) which would be for a maintainer to decide on the approach.

If I'm interpreting the code correctly, the configmap mainly (only?) impacts SSO / Auth, so that keeps the scope smaller, but there are different trade-offs to each approach (two of which could be out-of-scope: "leave that to Reloader" or "let Helm handle that"), so usually I'd defer a decision like that to a maintainer. That's also why I filed an issue instead of going straight to PR -- didn't want to do some refactoring without first getting input (or assume that a PR would even be wanted).

[agilgur5]

@terrytangyuan any suggestions on the above?

[agilgur5]

Now that I'm more familiar with the codebase (#11426 in particular), it seems like the current convention would be to use a watch / get on this configmap (potentially with an informer) and reconfigure on change. Would be similar to the Controller logic.
Some of the initialization logic would be good to refactor out so that it runs on both initialization and config change (this is (much) easier said than done). It also might make sense to have some shared logic between the Server and Controller in the config controller package (heavy on the might, that would be a decent sized refactor).

It looks like the Server already has configmap RBAC (and unconditional in Argo Helm), seemingly only for this exact configmap, so at least no permission changes needed for that.

Volume mounting has been mentioned before too and that could be done as an alternative (watch on the FS), but is not one of the current conventions.