From a38fb1629508c300a51603d71fe9ed9c0a88796f Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 11 Jul 2023 10:57:11 -0700 Subject: [PATCH] Load FederationDomain endpoints before updating its status - Avoid a possible race condition where the status says "Ready" but the endpoints take another moment to become available, potentially casing a fast client to get a 404 after observing that the status is "Ready" and then immediately trying to use the endpoints. Co-authored-by: Benjamin A. Petersen --- .../federation_domain_watcher.go | 20 +++++++++++++++---- .../federation_domain_watcher_test.go | 2 +- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 939de443..3cf2becb 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -134,6 +134,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro var errs []error federationDomainIssuers := make([]*federationdomainproviders.FederationDomainIssuer, 0) crossDomainConfigValidator := newCrossFederationDomainConfigValidator(federationDomains) + fdToConditionsMap := map[*configv1alpha1.FederationDomain][]*configv1alpha1.Condition{} for _, federationDomain := range federationDomains { conditions := make([]*configv1alpha1.Condition, 0, 4) @@ -425,10 +426,10 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro }) } - if err = c.updateStatus(ctx.Context, federationDomain, conditions); err != nil { - errs = append(errs, fmt.Errorf("could not update status: %w", err)) - continue - } + // Now that we have determined the conditions, save them for after the loop. + // For a valid FederationDomain, want to update the conditions after we have + // made the FederationDomain's endpoints available. + fdToConditionsMap[federationDomain] = conditions if !hadErrorCondition(conditions) { // Successfully validated the FederationDomain, so allow it to be loaded. @@ -436,8 +437,19 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro } } + // Load the endpoints of every valid FederationDomain. Removes the endpoints of any + // previous FederationDomains which no longer exist or are no longer valid. c.federationDomainsSetter.SetFederationDomains(federationDomainIssuers...) + // Now that the endpoints of every valid FederationDomain are available, update the + // statuses. This allows clients to wait for Ready without any race conditions in the + // endpoints being available. + for federationDomain, conditions := range fdToConditionsMap { + if err = c.updateStatus(ctx.Context, federationDomain, conditions); err != nil { + errs = append(errs, fmt.Errorf("could not update status: %w", err)) + } + } + return errorsutil.NewAggregate(errs) } diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index 6b30818e..92e20c11 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go @@ -485,7 +485,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, wantErr: "could not update status: some update error", wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{ - // federationDomain1 is not included because it encountered an error + federationDomainIssuerWithDefaultIDP(t, federationDomain1.Spec.Issuer, oidcIdentityProvider.ObjectMeta), federationDomainIssuerWithDefaultIDP(t, federationDomain2.Spec.Issuer, oidcIdentityProvider.ObjectMeta), }, wantStatusUpdates: []*configv1alpha1.FederationDomain{