Use backoff to initialize connectors in the background

[nabokihms]

Overview

This pull request introduces a crucial enhancement to the project by implementing queue-based initialization for connectors with backoff. The primary motivation behind this update is to mitigate the risk associated with misconfigured connectors, preventing a single connector from disrupting the entire functionality of the Dex.

What this PR does / why we need it

Related #2789 (there is more info)

This update significantly reduces the impact of misconfigured connectors, safeguarding the stability of the Dex by:

• Introducing queue-based connector initialization.
• Implementing a backoff strategy for enhanced resilience.

Special notes for your reviewer

[sagikazarmark]

Should we maybe also parallelize connector initialization?

https://github.com/sourcegraph/conc

[nabokihms]

Didn't invest a lot into parallelizing because for me this is the next problem. The liner code is easier to understand and to apply the backoff pattern. If you think this is worth investing more into concurrent initialization, we can:

1. Add a separate queue for each connector.
2. Run each initialization in a separate goroutine.

The code then will look like this:

func (s *Server) InitializeConnectors(connectors []storage.Connector) {
	b := backoff.NewExponentialBackOff()
	b.MaxElapsedTime = 0
	b.InitialInterval = 100 * time.Millisecond
	b.MaxInterval = 30 * time.Second

	s.logger.Info("start initializing connectors")

	for _, c := range connectors {
		go func(conn storage.Connector) {
			limiter := backoff.NewTicker(b)

			for {
				s.logger.Debugf("initializing %q connector", conn.ID)

				_, err := s.OpenConnector(conn)
				if err == nil {
					break
				}

				s.logger.Error(err)
				<-limiter.C // Wait for the next retry only on fails
			}

			s.logger.Debugf("connector %q has been initialized successfully", conn.ID)
		}(c)
	}
}

[sagikazarmark]

Well, if the goal is to make sure a failing connector doesn't block other connectors from initialization, then parallelization is certainly worth considering.

There is another potential side effect though: Dex may start and report ready before connectors are initialized. I'm not sure if that's the case or if it's a problem, but even in your PR there is an extra goroutine now.

Last, but not least: even adding a backoff may delay initialization. We might want to consider adding some sort of retry queue where failing connectors are added back at the end of the queue so we can try initializing other connectors first.

In any case, this may be a more sophisticated problem that it looks at first.

I'm slightly concerned about the added goroutine causing trouble on initialization.

If you think this is a serious issue though and this solves it or at least serves as a temporary solution, I'm not gonna block it. Maybe hide it behind a feature flag?

[nabokihms]

There is another potential side effect though: Dex may start and report ready before connectors are initialized. I'm not sure if that's the case or if it's a problem, but even in your PR there is an extra goroutine now.

Good point, there is an additional check for the readiness probe that validates that at least one connector was initialized

Last, but not least: even adding a backoff may delay initialization. We might want to consider adding some sort of retry queue where failing connectors are added back at the end of the queue so we can try initializing other connectors first.

In the first commit of the PR, there were a single backoff queue for all connectors, but because we decided to switch to parallel, there is now a separate queue for each connector.

In any case, this may be a more sophisticated problem that it looks at first.

Never expected it to be easy breezy 🙂

If you think this is a serious issue though and this solves it or at least serves as a temporary solution, I'm not gonna block it. Maybe hide it behind a feature flag?

Good point, I have another PR in my pocket. Let's merge it first before introducing a new feature flag.
#3278