Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds/resolver: fix panic when two LDS updates are receives without RDS in between #4327

Merged
merged 1 commit into from Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion xds/internal/resolver/watch_service.go
Expand Up @@ -132,7 +132,15 @@ func (w *serviceUpdateWatcher) handleLDSResp(update xdsclient.ListenerUpdate, er
//
// If the route name did change, then we must wait until the first RDS
// update before reporting this LDS config.
w.serviceCb(w.lastUpdate, nil)
if w.lastUpdate.virtualHost != nil {
// We want to send an update with the new fields from the new LDS
// (e.g. max stream duration), and old fields from the the previous
// RDS.
//
// But note that this should only happen when virtual host is set,
// which means an RDS was received.
w.serviceCb(w.lastUpdate, nil)
}
return
}
w.rdsName = update.RouteConfigName
Expand Down
42 changes: 42 additions & 0 deletions xds/internal/resolver/xds_resolver_test.go
Expand Up @@ -1030,6 +1030,48 @@ func (s) TestXDSResolverResourceNotFoundError(t *testing.T) {
}
}

// TestXDSResolverMultipleLDSUpdates tests the case where two LDS updates with
// the same RDS name to watch are received without an RDS in between. Those LDS
// updates shouldn't trigger service config update.
//
// This test case also makes sure the resolver doesn't panic.
func (s) TestXDSResolverMultipleLDSUpdates(t *testing.T) {
xdsC := fakeclient.NewClient()
xdsR, tcc, cancel := testSetup(t, setupOpts{
xdsClientFunc: func() (xdsClientInterface, error) { return xdsC, nil },
})
defer func() {
cancel()
xdsR.Close()
}()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
waitForWatchListener(ctx, t, xdsC, targetStr)
xdsC.InvokeWatchListenerCallback(xdsclient.ListenerUpdate{RouteConfigName: routeStr, HTTPFilters: routerFilterList}, nil)
waitForWatchRouteConfig(ctx, t, xdsC, routeStr)
defer replaceRandNumGenerator(0)()

// Send a new LDS update, with the same fields.
xdsC.InvokeWatchListenerCallback(xdsclient.ListenerUpdate{RouteConfigName: routeStr, HTTPFilters: routerFilterList}, nil)
ctx, cancel = context.WithTimeout(context.Background(), defaultTestShortTimeout)
defer cancel()
// Should NOT trigger a state update.
gotState, err := tcc.stateCh.Receive(ctx)
if err == nil {
t.Fatalf("ClientConn.UpdateState received %v, want timeout error", gotState)
}

// Send a new LDS update, with the same RDS name, but different fields.
xdsC.InvokeWatchListenerCallback(xdsclient.ListenerUpdate{RouteConfigName: routeStr, MaxStreamDuration: time.Second, HTTPFilters: routerFilterList}, nil)
ctx, cancel = context.WithTimeout(context.Background(), defaultTestShortTimeout)
defer cancel()
gotState, err = tcc.stateCh.Receive(ctx)
if err == nil {
t.Fatalf("ClientConn.UpdateState received %v, want timeout error", gotState)
}
}

type filterBuilder struct {
httpfilter.Filter // embedded as we do not need to implement registry / parsing in this test.
path *[]string
Expand Down