Skip to content

make more conf.Watch usages async

Warren Gifford requested to merge sg/async into master

Created by: slimsag

conf.Watch should not be invoked in a blocker manner inside of init (per its docs), this change fixes that.

Prior to this change, these instances were bad code (both for violating the conf.Watch docs and because doing heavy work inside of init is not good), but these instances were not causing any user facing issues. However, after an upcoming change these instances would result in a deadlock (which was automatically detected by pkg/conf's deadlock detector).

My goal is to make these changes such that:

  1. It is easily verifiable there are zero regressions in both behavior and security guarantees.
  2. The changes are merged quickly, because this is blocking my management-console work.

A note about imperfection

The changes here are not perfect. I have no doubt in my mind that some of these conf.Watch instances can be written more clearly. In specific:

  • The auth config_watch.go files feel very duplicative and can probably be unified somehow (not a problem introduced by this PR, but a more general problem with the existing code).
  • New constraints are introduced on auth.UpdateProviders, authz.GetProviders which long term are likely not the correct solutions. I would like someone more familiar with this code to address this because:
    1. It is hard for me to verify when auth/authz provider getter methods are invoked relative to other service operations. I could do it, but it would take me a while to do it and feel confident about it.
    2. There are higher level simplifications that I think could be made, but it is hard to say for certain without a good global overview of the code setup.

Reviewing

If this change did not touch auth code, I would merge it now (I am very confident about it, and this is a big blocker for the management-console to land). Since this does, I will instead block on review by @beyang, @sqs, or @nicksnyder for authority sake.

Hopefully review comments fall into one of two categories:

  1. Whether or not this change causes any regressions in functionality or security (merge blockers).
  2. Whether or not we can improve the code here in various ways: I would prefer these be filed as separate issues if possible (issues me or someone else can follow up on in the future).

Merge request reports