xds/outlierdetection: fix config handling

internal/balancer/nop/nop.go

@@ -0,0 +1,60 @@
+/*
+ *
+ * Copyright 2023 gRPC authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+// Package nop implements a balancer with all of it's balancer operations as
+// no-ops, other than returning a Transient Failure Picker on a Client Conn
+// update.
+package nop
+
+import (
+	"errors"
+
+	"google.golang.org/grpc/balancer"
+	"google.golang.org/grpc/balancer/base"
+	"google.golang.org/grpc/connectivity"
+)
+
+// Balancer is a balancer with all of it's balancer operations as no-ops, other
+// than returning a Transient Failure Picker on a Client Conn update.
+type Balancer struct {
+	cc balancer.ClientConn
+}
+
+// NewNOPBalancer returns a no-op balancer.
+func NewNOPBalancer(cc balancer.ClientConn) *Balancer {
+	return &Balancer{cc: cc}
+}
+
+// UpdateClientConnState updates the Balancer's Client Conn with an Error Picker
+// and a Connectivity State of TRANSIENT_FAILURE.
+func (b *Balancer) UpdateClientConnState(_ balancer.ClientConnState) error {
+	b.cc.UpdateState(balancer.State{
+		Picker:            base.NewErrPicker(errors.New("no-op balancer invoked")),
+		ConnectivityState: connectivity.TransientFailure,
+	})
+	return nil
+}
+
+// ResolverError is a no-op.
+func (b *Balancer) ResolverError(_ error) {}
+
+// UpdateSubConnState is a no-op.
+func (b *Balancer) UpdateSubConnState(_ balancer.SubConn, _ balancer.SubConnState) {}
+
+// Close is a no-op.
+func (b *Balancer) Close() {}

test/xds/xds_client_outlier_detection_test.go

@@ -267,7 +267,7 @@ func (s) TestOutlierDetectionXDSDefaultOn(t *testing.T) {
 	// Configure CDS resources with Outlier Detection set but
 	// EnforcingSuccessRate unset. This should cause Outlier Detection to be
 	// configured with SuccessRateEjection present in configuration, which will
-	// eventually be populated with it's default values along with the knobs set
+	// eventually be populated with its default values along with the knobs set
 	// as SuccessRate fields in the proto, and thus Outlier Detection should be
 	// on and actively eject upstreams.
 	const serviceName = "my-service-client-side-xds"

xds/internal/balancer/cdsbalancer/cdsbalancer.go

@@ -27,6 +27,7 @@ import (
 	"google.golang.org/grpc/connectivity"
 	"google.golang.org/grpc/credentials"
 	"google.golang.org/grpc/credentials/tls/certprovider"
+	"google.golang.org/grpc/internal/balancer/nop"
 	"google.golang.org/grpc/internal/buffer"
 	xdsinternal "google.golang.org/grpc/internal/credentials/xds"
 	"google.golang.org/grpc/internal/envconfig"
@@ -78,13 +79,13 @@ func (bb) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Bal
 		// Shouldn't happen, registered through imported Cluster Resolver,
 		// defensive programming.
 		logger.Errorf("%q LB policy is needed but not registered", clusterresolver.Name)
-		return nil
+		return nop.NewNOPBalancer(cc)
 	}
 	crParser, ok := builder.(balancer.ConfigParser)
 	if !ok {
 		// Shouldn't happen, imported Cluster Resolver builder has this method.
 		logger.Errorf("%q LB policy does not implement a config parser", clusterresolver.Name)
-		return nil
+		return nop.NewNOPBalancer(cc)
 	}
 	b := &cdsBalancer{
 		bOpts:    opts,

xds/internal/balancer/clusterresolver/clusterresolver.go

@@ -30,6 +30,7 @@ import (
 	"google.golang.org/grpc/balancer"
 	"google.golang.org/grpc/balancer/base"
 	"google.golang.org/grpc/connectivity"
+	"google.golang.org/grpc/internal/balancer/nop"
 	"google.golang.org/grpc/internal/buffer"
 	"google.golang.org/grpc/internal/envconfig"
 	"google.golang.org/grpc/internal/grpclog"
@@ -64,12 +65,12 @@ func (bb) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Bal
 	priorityBuilder := balancer.Get(priority.Name)
 	if priorityBuilder == nil {
 		logger.Errorf("%q LB policy is needed but not registered", priority.Name)
-		return nil
+		return nop.NewNOPBalancer(cc)
 	}
 	priorityConfigParser, ok := priorityBuilder.(balancer.ConfigParser)
 	if !ok {
 		logger.Errorf("%q LB policy does not implement a config parser", priority.Name)
-		return nil
+		return nop.NewNOPBalancer(cc)
 	}
 
 	b := &clusterResolverBalancer{
@@ -116,23 +117,19 @@ func (bb) ParseConfig(j json.RawMessage) (serviceconfig.LoadBalancingConfig, err
 		return nil, fmt.Errorf("unable to unmarshal balancer config %s into cluster-resolver config, error: %v", string(j), err)
 	}
 
-	odCfgs := make([]outlierdetection.LBConfig, len(cfg.DiscoveryMechanisms))
-	for i, dm := range cfg.DiscoveryMechanisms {
-		lbCfg, err := odParser.ParseConfig(dm.OutlierDetection)
-		if err != nil {
-			return nil, fmt.Errorf("error parsing Outlier Detection config: %v", dm.OutlierDetection)
-		}
-		odCfg, ok := lbCfg.(*outlierdetection.LBConfig)
-		if !ok {
-			// Shouldn't happen, Parser built at build time with Outlier Detection
-			// builder pulled from gRPC LB Registry.
-			return nil, fmt.Errorf("odParser returned config with unexpected type %T: %v", lbCfg, lbCfg)
-		}
-		odCfgs[i] = *odCfg
-	}
 	if envconfig.XDSOutlierDetection {
-		for i, odCfg := range odCfgs {
-			cfg.DiscoveryMechanisms[i].outlierDetection = odCfg
+		for i, dm := range cfg.DiscoveryMechanisms {
+			lbCfg, err := odParser.ParseConfig(dm.OutlierDetection)
+			if err != nil {
+				return nil, fmt.Errorf("error parsing Outlier Detection config %v: %v", dm.OutlierDetection, err)
+			}
+			odCfg, ok := lbCfg.(*outlierdetection.LBConfig)
+			if !ok {
+				// Shouldn't happen, Parser built at build time with Outlier Detection
+				// builder pulled from gRPC LB Registry.
+				return nil, fmt.Errorf("odParser returned config with unexpected type %T: %v", lbCfg, lbCfg)
+			}
+			cfg.DiscoveryMechanisms[i].outlierDetection = *odCfg
 		}
 	}
 	if err := json.Unmarshal(cfg.XDSLBPolicy, &cfg.xdsLBPolicy); err != nil {

xds/internal/balancer/outlierdetection/config.go

@@ -19,6 +19,7 @@ package outlierdetection
 
 import (
 	"encoding/json"
+	"time"
 
 	iserviceconfig "google.golang.org/grpc/internal/serviceconfig"
 	"google.golang.org/grpc/serviceconfig"
@@ -59,7 +60,7 @@ type SuccessRateEjection struct {
 type successRateEjection SuccessRateEjection
 
 // UnmarshalJSON unmarshals JSON into SuccessRateEjection. If a
-// SuccessRateEjection field is not set, that field will get it's default value.
+// SuccessRateEjection field is not set, that field will get its default value.
 func (sre *SuccessRateEjection) UnmarshalJSON(j []byte) error {
 	sre.StdevFactor = 1900
 	sre.EnforcementPercentage = 100
@@ -124,7 +125,7 @@ type FailurePercentageEjection struct {
 type failurePercentageEjection FailurePercentageEjection
 
 // UnmarshalJSON unmarshals JSON into FailurePercentageEjection. If a
-// FailurePercentageEjection field is not set, that field will get it's default
+// FailurePercentageEjection field is not set, that field will get its default
 // value.
 func (fpe *FailurePercentageEjection) UnmarshalJSON(j []byte) error {
 	fpe.Threshold = 85
@@ -188,6 +189,28 @@ type LBConfig struct {
 	ChildPolicy *iserviceconfig.BalancerConfig `json:"childPolicy,omitempty"`
 }
 
+// For UnmarshalJSON to work correctly and set defaults without infinite
+// recursion.
+type lbConfig LBConfig
+
+// UnmarshalJSON unmarshals JSON into LBConfig. If a top level LBConfig field
+// (i.e. not next layer sre or fpe) is not set, that field will get its default
+// value. If sre or fpe is not set, it will stay unset, otherwise it will
+// unmarshal on those types populating with default values for their fields if
+// needed.
+func (lbc *LBConfig) UnmarshalJSON(j []byte) error {
+	// Default top layer values as documented in A50.
+	lbc.Interval = iserviceconfig.Duration(10 * time.Second)
+	lbc.BaseEjectionTime = iserviceconfig.Duration(30 * time.Second)
+	lbc.MaxEjectionTime = iserviceconfig.Duration(300 * time.Second)
+	lbc.MaxEjectionPercent = 10
+	// Unmarshal JSON on a type with zero values for methods, including
+	// UnmarshalJSON. Overwrites defaults, leaves alone if not. typecast to
+	// avoid infinite recursion by not recalling this function and causing stack
+	// overflow.
+	return json.Unmarshal(j, (*lbConfig)(lbc))
+}
+
 // EqualIgnoringChildPolicy returns whether the LBConfig is same with the
 // parameter outside of the child policy, only comparing the Outlier Detection
 // specific configuration.

xds/internal/xdsclient/xdsresource/unmarshal_cds.go

@@ -664,9 +664,5 @@ func outlierConfigFromCluster(cluster *v3clusterpb.Cluster) (json.RawMessage, er
 		SuccessRateEjection:       sre,
 		FailurePercentageEjection: fpe,
 	}
-	odLBCfgJSON, err := json.Marshal(odLBCfg)
-	if err != nil {
-		return nil, err
-	}
-	return odLBCfgJSON, nil
+	return json.Marshal(odLBCfg)
 }