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 <ben@benjaminapetersen.me>
This commit is contained in:
Ryan Richard 2023-07-11 10:57:11 -07:00
parent e334ad6f7e
commit a38fb16295
2 changed files with 17 additions and 5 deletions

View File

@ -134,6 +134,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro
var errs []error var errs []error
federationDomainIssuers := make([]*federationdomainproviders.FederationDomainIssuer, 0) federationDomainIssuers := make([]*federationdomainproviders.FederationDomainIssuer, 0)
crossDomainConfigValidator := newCrossFederationDomainConfigValidator(federationDomains) crossDomainConfigValidator := newCrossFederationDomainConfigValidator(federationDomains)
fdToConditionsMap := map[*configv1alpha1.FederationDomain][]*configv1alpha1.Condition{}
for _, federationDomain := range federationDomains { for _, federationDomain := range federationDomains {
conditions := make([]*configv1alpha1.Condition, 0, 4) 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 { // Now that we have determined the conditions, save them for after the loop.
errs = append(errs, fmt.Errorf("could not update status: %w", err)) // For a valid FederationDomain, want to update the conditions after we have
continue // made the FederationDomain's endpoints available.
} fdToConditionsMap[federationDomain] = conditions
if !hadErrorCondition(conditions) { if !hadErrorCondition(conditions) {
// Successfully validated the FederationDomain, so allow it to be loaded. // 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...) 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) return errorsutil.NewAggregate(errs)
} }

View File

@ -485,7 +485,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
}, },
wantErr: "could not update status: some update error", wantErr: "could not update status: some update error",
wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{ 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), federationDomainIssuerWithDefaultIDP(t, federationDomain2.Spec.Issuer, oidcIdentityProvider.ObjectMeta),
}, },
wantStatusUpdates: []*configv1alpha1.FederationDomain{ wantStatusUpdates: []*configv1alpha1.FederationDomain{