easwars review 2

xds/internal/balancer/cdsbalancer/cdsbalancer.go

@@ -526,6 +526,8 @@ func (b *cdsBalancer) onClusterAmbientError(name string, err error) {
 //
 // Only executed in the context of a serializer callback.
 func (b *cdsBalancer) onClusterResourceChangedError(name string, err error) {
+	b.logger.Errorf("Cluster resource %q received on-resource-changed error update: %v", name, err)
+
 	if b.childLB != nil {
 		b.childLB.ResolverError(err)
 	} else {

xds/internal/balancer/clusterresolver/resource_resolver_eds.go

@@ -84,11 +84,7 @@ func (er *edsDiscoveryMechanism) OnResourceChanged(update *xdsresource.ResourceD
 
 	if update.Err != nil {
 		if er.logger.V(2) {
-			if xdsresource.ErrType(update.Err) == xdsresource.ErrorTypeResourceNotFound {
-				er.logger.Infof("EDS discovery mechanism for resource %q reported resource-does-not-exist error", er.nameToWatch)
-			} else {
-				er.logger.Infof("EDS discovery mechanism for resource %q reported on resource changed error: %v", er.nameToWatch, update.Err)
-			}
+			er.logger.Infof("EDS discovery mechanism for resource %q reported on resource changed error: %v", er.nameToWatch, update.Err)
 		}
 		// Report an empty update that would result in no priority child being
 		// created for this discovery mechanism. This would result in the priority

xds/internal/resolver/xds_resolver.go

@@ -528,11 +528,7 @@ func (r *xdsResolver) onListenerResourceAmbientError(err error) {
 // Only executed in the context of a serializer callback.
 func (r *xdsResolver) onListenerResourceChangedError(err error) {
 	if r.logger.V(2) {
-		if xdsresource.ErrType(err) == xdsresource.ErrorTypeResourceNotFound {
-			r.logger.Infof("Received resource-not-found-error for Listener resource %q", r.ldsResourceName)
-		} else {
-			r.logger.Infof("Received on-resource-changed error for Listener resource %q: %v", r.ldsResourceName, err)
-		}
+		r.logger.Infof("Received on-resource-changed error for Listener resource %q: %v", r.ldsResourceName, err)
 	}
 
 	r.listenerUpdateRecvd = false
@@ -573,11 +569,7 @@ func (r *xdsResolver) onRouteConfigResourceAmbientError(name string, err error)
 // Only executed in the context of a serializer callback.
 func (r *xdsResolver) onRouteConfigResourceChangedError(name string, err error) {
 	if r.logger.V(2) {
-		if xdsresource.ErrType(err) == xdsresource.ErrorTypeResourceNotFound {
-			r.logger.Infof("Received resource-not-found-error for RouteConfiguration resource %q", name)
-		} else {
-			r.logger.Infof("Received on-resource-changed error for RouteConfiguration resource %q: %v", name, err)
-		}
+		r.logger.Infof("Received on-resource-changed error for RouteConfiguration resource %q: %v", name, err)
 	}
 
 	if r.rdsResourceName != name {

xds/internal/server/listener_wrapper.go

@@ -424,19 +424,17 @@ func (lw *ldsWatcher) OnResourceChanged(update *xdsresource.ResourceDataOrError,
 		}
 		return
 	}
-	if lw.logger.V(2) {
-		if update.Err != nil {
-			lw.logger.Infof("LDS watch for resource %q received error: %#v", lw.name, update.Err)
-		} else {
-			u := update.Data.(*xdsresource.ListenerResourceData)
-			lw.logger.Infof("LDS watch for resource %q received update: %#v", lw.name, u.Resource)
-		}
-	}
 	if update.Err != nil {
+		if lw.logger.V(2) {
+			lw.logger.Infof("LDS watch for resource %q received error: %v", lw.name, update.Err)
+		}
 		lw.parent.onLDSResourceChangedError(update.Err)
 		return
 	}
 	u := update.Data.(*xdsresource.ListenerResourceData)
+	if update.Err != nil {
+		lw.logger.Infof("LDS watch for resource %q received update: %v", lw.name, u.Resource)
+	}
 	lw.parent.handleLDSUpdate(u.Resource)
 }
 

xds/internal/server/rds_handler.go

@@ -155,19 +155,17 @@ func (rw *rdsWatcher) OnResourceChanged(update *xdsresource.ResourceDataOrError,
 		return
 	}
 	rw.mu.Unlock()
-	if rw.logger.V(2) {
-		if update.Err != nil {
-			rw.logger.Infof("RDS watch for resource %q received error: %#v", rw.routeName, update.Err)
-		} else {
-			u := update.Data.(*xdsresource.RouteConfigResourceData)
-			rw.logger.Infof("RDS watch for resource %q received update: %#v", rw.routeName, u.Resource)
-		}
-	}
 	if update.Err != nil {
+		if rw.logger.V(2) {
+			rw.logger.Infof("RDS watch for resource %q received error: %v", rw.routeName, update.Err)
+		}
 		rw.parent.handleRouteUpdate(rw.routeName, rdsWatcherUpdate{err: update.Err})
 		return
 	}
 	u := update.Data.(*xdsresource.RouteConfigResourceData)
+	if rw.logger.V(2) {
+		rw.logger.Infof("RDS watch for resource %q received update: %#v", rw.routeName, u.Resource)
+	}
 	rw.parent.handleRouteUpdate(rw.routeName, rdsWatcherUpdate{data: &u.Resource})
 }
 

xds/internal/testutils/resource_watcher.go

@@ -54,7 +54,6 @@ func (w *TestResourceWatcher) OnResourceChanged(update xdsresource.ResourceDataO
 		}
 		w.ErrorCh <- update.Err
 		return
-
 	}
 	select {
 	case <-w.UpdateCh:

xds/internal/xdsclient/authority.go

@@ -466,7 +466,7 @@ func (a *authority) handleADSResourceUpdate(serverConfig *bootstrap.ServerConfig
 			watcher := watcher
 			watcherCnt.Add(1)
 			funcsToSchedule = append(funcsToSchedule, func(context.Context) {
-				watcher.OnResourceChanged(xdsresource.ResourceDataOrError{Err: xdsresource.NewErrorf(xdsresource.ErrorTypeResourceNotFound, "xds: resource has been removed")}, done)
+				watcher.OnResourceChanged(xdsresource.ResourceDataOrError{Err: xdsresource.NewErrorf(xdsresource.ErrorTypeResourceNotFound, "xds: resource %q of type %q has been removed", name, rType.TypeName())}, done)
 			})
 		}
 	}
@@ -510,7 +510,7 @@ func (a *authority) handleADSResourceDoesNotExist(rType xdsresource.Type, resour
 	for watcher := range state.watchers {
 		watcher := watcher
 		a.watcherCallbackSerializer.TrySchedule(func(context.Context) {
-			watcher.OnResourceChanged(xdsresource.ResourceDataOrError{Err: xdsresource.NewErrorf(xdsresource.ErrorTypeResourceNotFound, "xds: resource %s does not exist", rType.TypeName())}, func() {})
+			watcher.OnResourceChanged(xdsresource.ResourceDataOrError{Err: xdsresource.NewErrorf(xdsresource.ErrorTypeResourceNotFound, "xds: resource %q of type %q does not exist", resourceName, rType.TypeName())}, func() {})
 		})
 	}
 }
@@ -667,7 +667,7 @@ func (a *authority) watchResource(rType xdsresource.Type, resourceName string, w
 		// server does not have this resource, notify the new watcher.
 		if state.md.Status == xdsresource.ServiceStatusNotExist {
 			a.watcherCallbackSerializer.TrySchedule(func(context.Context) {
-				watcher.OnResourceChanged(xdsresource.ResourceDataOrError{Err: xdsresource.NewErrorf(xdsresource.ErrorTypeResourceNotFound, "xds: resource %s does not exist", rType.TypeName())}, func() {})
+				watcher.OnResourceChanged(xdsresource.ResourceDataOrError{Err: xdsresource.NewErrorf(xdsresource.ErrorTypeResourceNotFound, "xds: resource %q of type %q does not exist", resourceName, rType.TypeName())}, func() {})
 			})
 		}
 		cleanup = a.unwatchResource(rType, resourceName, watcher)

xds/internal/xdsclient/tests/resource_update_test.go

@@ -161,7 +161,7 @@ func (s) TestHandleListenerResponseFromManagementServer(t *testing.T) {
 					Value:   []byte{1, 2, 3, 4},
 				}},
 			},
-			wantErr: "xds: resource ListenerResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"ListenerResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.listener.v3.Listener",
@@ -177,7 +177,7 @@ func (s) TestHandleListenerResponseFromManagementServer(t *testing.T) {
 				TypeUrl:     "type.googleapis.com/envoy.config.listener.v3.Listener",
 				VersionInfo: "1",
 			},
-			wantErr: "xds: resource ListenerResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"ListenerResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.listener.v3.Listener",
@@ -194,7 +194,7 @@ func (s) TestHandleListenerResponseFromManagementServer(t *testing.T) {
 				VersionInfo: "1",
 				Resources:   []*anypb.Any{testutils.MarshalAny(t, &v3routepb.RouteConfiguration{})},
 			},
-			wantErr: "xds: resource ListenerResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"ListenerResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.listener.v3.Listener",
@@ -418,7 +418,7 @@ func (s) TestHandleRouteConfigResponseFromManagementServer(t *testing.T) {
 					Value:   []byte{1, 2, 3, 4},
 				}},
 			},
-			wantErr: "xds: resource RouteConfigResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"RouteConfigResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
@@ -434,7 +434,7 @@ func (s) TestHandleRouteConfigResponseFromManagementServer(t *testing.T) {
 				TypeUrl:     "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
 				VersionInfo: "1",
 			},
-			wantErr: "xds: resource RouteConfigResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"RouteConfigResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
@@ -451,7 +451,7 @@ func (s) TestHandleRouteConfigResponseFromManagementServer(t *testing.T) {
 				VersionInfo: "1",
 				Resources:   []*anypb.Any{testutils.MarshalAny(t, &v3clusterpb.Cluster{})},
 			},
-			wantErr: "xds: resource RouteConfigResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"RouteConfigResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
@@ -667,7 +667,7 @@ func (s) TestHandleClusterResponseFromManagementServer(t *testing.T) {
 					Value:   []byte{1, 2, 3, 4},
 				}},
 			},
-			wantErr: "xds: resource ClusterResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"ClusterResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.cluster.v3.Cluster",
@@ -683,7 +683,7 @@ func (s) TestHandleClusterResponseFromManagementServer(t *testing.T) {
 				TypeUrl:     "type.googleapis.com/envoy.config.cluster.v3.Cluster",
 				VersionInfo: "1",
 			},
-			wantErr: "xds: resource ClusterResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"ClusterResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.cluster.v3.Cluster",
@@ -700,7 +700,7 @@ func (s) TestHandleClusterResponseFromManagementServer(t *testing.T) {
 				VersionInfo: "1",
 				Resources:   []*anypb.Any{testutils.MarshalAny(t, &v3endpointpb.ClusterLoadAssignment{})},
 			},
-			wantErr: "xds: resource ClusterResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"ClusterResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.cluster.v3.Cluster",
@@ -974,7 +974,7 @@ func (s) TestHandleEndpointsResponseFromManagementServer(t *testing.T) {
 					Value:   []byte{1, 2, 3, 4},
 				}},
 			},
-			wantErr: "xds: resource EndpointsResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"EndpointsResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",
@@ -990,7 +990,7 @@ func (s) TestHandleEndpointsResponseFromManagementServer(t *testing.T) {
 				TypeUrl:     "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",
 				VersionInfo: "1",
 			},
-			wantErr: "xds: resource EndpointsResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"EndpointsResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",
@@ -1007,7 +1007,7 @@ func (s) TestHandleEndpointsResponseFromManagementServer(t *testing.T) {
 				VersionInfo: "1",
 				Resources:   []*anypb.Any{testutils.MarshalAny(t, &v3listenerpb.Listener{})},
 			},
-			wantErr: "xds: resource EndpointsResource does not exist",
+			wantErr: fmt.Sprintf("xds: resource \"%v\" of type \"EndpointsResource\" does not exist", resourceName1),
 			wantGenericXDSConfig: []*v3statuspb.ClientConfig_GenericXdsConfig{
 				{
 					TypeUrl:      "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",

xds/internal/xdsclient/xdsresource/cluster_resource_type.go

@@ -123,8 +123,8 @@ type ClusterWatcher interface {
 	//      - connection failure (if resource is not cached)
 	OnResourceChanged(*ResourceDataOrError, OnDoneFunc)
 
-	// If resource is already cached, it is invoked under different error
-	// conditions including but not limited to the following:
+	// OnAmbientError is invoked if resource is already cached under different
+	// error conditions including but not limited to the following:
 	//      - resource validation error
 	//      - ADS stream failure
 	//      - connection failure

xds/internal/xdsclient/xdsresource/endpoints_resource_type.go

@@ -119,8 +119,8 @@ type EndpointsWatcher interface {
 	//      - connection failure (if resource is not cached)
 	OnResourceChanged(*ResourceDataOrError, OnDoneFunc)
 
-	// If resource is already cached, it is invoked under different error
-	// conditions including but not limited to the following:
+	// OnAmbientError is invoked if resource is already cached under different
+	// error conditions including but not limited to the following:
 	//      - resource validation error
 	//      - ADS stream failure
 	//      - connection failure

xds/internal/xdsclient/xdsresource/listener_resource_type.go

@@ -156,8 +156,8 @@ type ListenerWatcher interface {
 	//      - connection failure (if resource is not cached)
 	OnResourceChanged(*ResourceDataOrError, OnDoneFunc)
 
-	// If resource is already cached, it is invoked under different error
-	// conditions including but not limited to the following:
+	// OnAmbientError is invoked if resource is already cached under different
+	// error conditions including but not limited to the following:
 	//      - resource validation error
 	//      - ADS stream failure
 	//      - connection failure

xds/internal/xdsclient/xdsresource/resource_type.go

@@ -73,15 +73,18 @@ type ResourceWatcher interface {
 	// the resource received from the xDS server or an error indicating the
 	// reason why the resource could not be obtained.
 	//
-	// The ResourceData of the ResourceDataOrError needs to be type asserted to
-	// the appropriate type for the resource being watched. In case of error,
-	// the ResourceData is nil otherwise its not nil and error is nil but both
-	// will never be nil together.
+	// In the former case, this callback will be invoked with a non-nil
+	// ResourceData in ResourceDataOrError. The ResourceData of the
+	// ResourceDataOrError needs to be type asserted to the appropriate type
+	// for the resource being watched.
+	//
+	// In the latter case, this callback will be invoked with a non-nil error
+	// value in ResourceDataOrError.
 	//
 	// Watcher is expected to use the most recent value passed to
-	// OnResourceChanged(), regardless of whether that's a resource or an error
-	// i.e., if the watcher is given an error via OnResourceChanged(), that
-	// means it should stop using any previously delivered resource.
+	// OnResourceChanged(), regardless of whether that's a ResourceData or an
+	// error i.e., if the watcher is given an error via OnResourceChanged(),
+	// that means it should stop using any previously delivered resource.
 	//
 	// It is invoked under different error conditions including but not
 	// limited to the following:

xds/internal/xdsclient/xdsresource/route_config_resource_type.go

@@ -120,8 +120,8 @@ type RouteConfigWatcher interface {
 	//      - connection failure (if resource is not cached)
 	OnResourceChanged(*ResourceDataOrError, OnDoneFunc)
 
-	// If resource is already cached, it is invoked under different error
-	// conditions including but not limited to the following:
+	// OnAmbientError is invoked if resource is already cached under different
+	// error conditions including but not limited to the following:
 	//      - resource validation error
 	//      - ADS stream failure
 	//      - connection failure