Added ENV VAR Protection for RBAC, RBAC Test is failing because not b…

…eing registered

internal/xds/env/env.go

@@ -43,6 +43,7 @@ const (
 	clientSideSecuritySupportEnv = "GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT"
 	aggregateAndDNSSupportEnv    = "GRPC_XDS_EXPERIMENTAL_ENABLE_AGGREGATE_AND_LOGICAL_DNS_CLUSTER"
 	retrySupportEnv              = "GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"
+	rbacSupportEnv               = "GRPC_XDS_EXPERIMENTAL_ENABLE_RBAC"
 
 	c2pResolverSupportEnv                    = "GRPC_EXPERIMENTAL_GOOGLE_C2P_RESOLVER"
 	c2pResolverTestOnlyTrafficDirectorURIEnv = "GRPC_TEST_ONLY_GOOGLE_C2P_RESOLVER_TRAFFIC_DIRECTOR_URI"
@@ -82,6 +83,9 @@ var (
 	// RetrySupport indicates whether xDS retry is enabled.
 	RetrySupport = strings.EqualFold(os.Getenv(retrySupportEnv), "true")
 
+	// RBACSupport indicates whether xDS configured RBAC HTTP Filter is enabled.
+	RBACSupport = strings.EqualFold(os.Getenv(rbacSupportEnv), "true")
+
 	// C2PResolverSupport indicates whether support for C2P resolver is enabled.
 	// This can be enabled by setting the environment variable
 	// "GRPC_EXPERIMENTAL_GOOGLE_C2P_RESOLVER" to "true".

xds/internal/httpfilter/rbac/rbac.go

@@ -28,6 +28,7 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/golang/protobuf/ptypes"
 	"google.golang.org/grpc/internal/resolver"
+	"google.golang.org/grpc/internal/xds/env"
 	"google.golang.org/grpc/internal/xds/rbac"
 	"google.golang.org/grpc/xds/internal/httpfilter"
 	"google.golang.org/protobuf/types/known/anypb"
@@ -37,7 +38,9 @@ import (
 )
 
 func init() {
-	httpfilter.Register(builder{})
+	if env.RBACSupport {
+		httpfilter.Register(builder{})
+	}
 }
 
 type builder struct {

xds/internal/server/listener_wrapper.go

@@ -34,6 +34,7 @@ import (
 	internalbackoff "google.golang.org/grpc/internal/backoff"
 	internalgrpclog "google.golang.org/grpc/internal/grpclog"
 	"google.golang.org/grpc/internal/grpcsync"
+	"google.golang.org/grpc/internal/xds/env"
 	"google.golang.org/grpc/xds/internal/xdsclient"
 	"google.golang.org/grpc/xds/internal/xdsclient/bootstrap"
 )
@@ -301,34 +302,37 @@ func (l *listenerWrapper) Accept() (net.Conn, error) {
 			conn.Close()
 			continue
 		}
-		var rc xdsclient.RouteConfigUpdate
-		if fc.InlineRouteConfig != nil {
-			rc = *fc.InlineRouteConfig
-		} else {
-			rcPtr := atomic.LoadPointer(&l.rdsUpdates)
-			rcuPtr := (*map[string]xdsclient.RouteConfigUpdate)(rcPtr)
-			// This shouldn't happen, but this error protects against a panic.
-			if rcuPtr == nil {
-				return nil, errors.New("route configuration pointer is nil")
+		var vhswi []xdsclient.VirtualHostWithInterceptors
+		if env.RBACSupport {
+			var rc xdsclient.RouteConfigUpdate
+			if fc.InlineRouteConfig != nil {
+				rc = *fc.InlineRouteConfig
+			} else {
+				rcPtr := atomic.LoadPointer(&l.rdsUpdates)
+				rcuPtr := (*map[string]xdsclient.RouteConfigUpdate)(rcPtr)
+				// This shouldn't happen, but this error protects against a panic.
+				if rcuPtr == nil {
+					return nil, errors.New("route configuration pointer is nil")
+				}
+				rcu := *rcuPtr
+				rc = rcu[fc.RouteConfigName]
+			}
+			// The filter chain will construct a usuable route table on each
+			// connection accept. This is done because preinstantiating every route
+			// table before it is needed for a connection would potentially lead to
+			// a lot of cpu time and memory allocated for route tables that will
+			// never be used. There was also a thought to cache this configuration,
+			// and reuse it for the next accepted connection. However, this would
+			// lead to a lot of code complexity (RDS Updates for a given route name
+			// can come it at any time), and connections aren't accepted too often,
+			// so this reinstantation of the Route Configuration is an acceptable
+			// tradeoff for simplicity.
+			vhswi, err = fc.ConstructUsableRouteConfiguration(rc)
+			if err != nil {
+				l.logger.Warningf("route configuration construction: %v", err)
+				conn.Close()
+				continue
 			}
-			rcu := *rcuPtr
-			rc = rcu[fc.RouteConfigName]
-		}
-		// The filter chain will construct a usuable route table on each
-		// connection accept. This is done because preinstantiating every route
-		// table before it is needed for a connection would potentially lead to
-		// a lot of cpu time and memory allocated for route tables that will
-		// never be used. There was also a thought to cache this configuration,
-		// and reuse it for the next accepted connection. However, this would
-		// lead to a lot of code complexity (RDS Updates for a given route name
-		// can come it at any time), and connections aren't accepted too often,
-		// so this reinstantation of the Route Configuration is an acceptable
-		// tradeoff for simplicity.
-		vhswi, err := fc.ConstructUsableRouteConfiguration(rc)
-		if err != nil {
-			l.logger.Warningf("route configuration construction: %v", err)
-			conn.Close()
-			continue
 		}
 		return &connWrapper{Conn: conn, filterChain: fc, parent: l, virtualHosts: vhswi}, nil
 	}

xds/internal/server/listener_wrapper_test.go

@@ -34,6 +34,7 @@ import (
 	wrapperspb "github.com/golang/protobuf/ptypes/wrappers"
 	"google.golang.org/grpc/internal/grpctest"
 	"google.golang.org/grpc/internal/testutils"
+	"google.golang.org/grpc/internal/xds/env"
 	_ "google.golang.org/grpc/xds/internal/httpfilter/router"
 	"google.golang.org/grpc/xds/internal/testutils/e2e"
 	"google.golang.org/grpc/xds/internal/testutils/fakeclient"
@@ -325,6 +326,11 @@ func (s) TestNewListenerWrapper(t *testing.T) {
 // the update from the rds handler should it move the server to
 // ServingModeServing.
 func (s) TestNewListenerWrapperWithRouteUpdate(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	_, readyCh, xdsC, _, cleanup := newListenerWrapper(t)
 	defer cleanup()
 

xds/internal/test/xds_server_integration_test.go

@@ -34,6 +34,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
 	"google.golang.org/grpc/internal/testutils"
+	"google.golang.org/grpc/internal/xds/env"
 	"google.golang.org/grpc/status"
 	"google.golang.org/grpc/xds"
 	_ "google.golang.org/grpc/xds/internal/httpfilter/rbac"
@@ -374,6 +375,11 @@ func (s) TestServerSideXDS_SecurityConfigChange(t *testing.T) {
 // (NonForwardingAction), and the RPC's matching those routes should proceed as
 // normal.
 func (s) TestServerSideXDS_RouteConfiguration(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	managementServer, nodeID, bootstrapContents, resolver, cleanup1 := setupManagementServer(t)
 	defer cleanup1()
 
@@ -711,6 +717,11 @@ func serverListenerWithRBACHTTPFilters(host string, port uint32, rbacCfg *rpb.RB
 // as normal and certain RPC's are denied by the RBAC HTTP Filter which gets
 // called by hooked xds interceptors.
 func (s) TestRBACHTTPFilter(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	tests := []struct {
 		name                string
 		rbacCfg             *rpb.RBAC

xds/internal/xdsclient/filter_chain.go

@@ -29,6 +29,7 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/golang/protobuf/ptypes"
 	"google.golang.org/grpc/internal/resolver"
+	"google.golang.org/grpc/internal/xds/env"
 	"google.golang.org/grpc/xds/internal/httpfilter"
 	"google.golang.org/grpc/xds/internal/version"
 )
@@ -587,33 +588,35 @@ func processNetworkFilters(filters []*v3listenerpb.Filter) (*FilterChain, error)
 				// TODO: Implement terminal filter logic, as per A36.
 				filterChain.HTTPFilters = filters
 				seenHCM = true
-				switch hcm.RouteSpecifier.(type) {
-				case *v3httppb.HttpConnectionManager_Rds:
-					if hcm.GetRds().GetConfigSource().GetAds() == nil {
-						return nil, fmt.Errorf("ConfigSource is not ADS: %+v", hcm)
+				if env.RBACSupport {
+					switch hcm.RouteSpecifier.(type) { // Guard this routing parsing...what does this trigger in filter chain and more downstream (no, nil is completly valid all the way...)? Also guard routeAndProcess, this will prevent the logic of a nil route configuration)
+					case *v3httppb.HttpConnectionManager_Rds:
+						if hcm.GetRds().GetConfigSource().GetAds() == nil {
+							return nil, fmt.Errorf("ConfigSource is not ADS: %+v", hcm)
+						}
+						name := hcm.GetRds().GetRouteConfigName()
+						if name == "" {
+							return nil, fmt.Errorf("empty route_config_name: %+v", hcm)
+						}
+						filterChain.RouteConfigName = name
+					case *v3httppb.HttpConnectionManager_RouteConfig:
+						// "RouteConfiguration validation logic inherits all
+						// previous validations made for client-side usage as RDS
+						// does not distinguish between client-side and
+						// server-side." - A36
+						// Can specify v3 here, as will never get to this function
+						// if v2.
+						routeU, err := generateRDSUpdateFromRouteConfiguration(hcm.GetRouteConfig(), nil, false)
+						if err != nil {
+							return nil, fmt.Errorf("failed to parse inline RDS resp: %v", err)
+						}
+						filterChain.InlineRouteConfig = &routeU
+					case nil:
+						// No-op, as no route specifier is a valid configuration on
+						// the server side.
+					default:
+						return nil, fmt.Errorf("unsupported type %T for RouteSpecifier", hcm.RouteSpecifier)
 					}
-					name := hcm.GetRds().GetRouteConfigName()
-					if name == "" {
-						return nil, fmt.Errorf("empty route_config_name: %+v", hcm)
-					}
-					filterChain.RouteConfigName = name
-				case *v3httppb.HttpConnectionManager_RouteConfig:
-					// "RouteConfiguration validation logic inherits all
-					// previous validations made for client-side usage as RDS
-					// does not distinguish between client-side and
-					// server-side." - A36
-					// Can specify v3 here, as will never get to this function
-					// if v2.
-					routeU, err := generateRDSUpdateFromRouteConfiguration(hcm.GetRouteConfig(), nil, false)
-					if err != nil {
-						return nil, fmt.Errorf("failed to parse inline RDS resp: %v", err)
-					}
-					filterChain.InlineRouteConfig = &routeU
-				case nil:
-					// No-op, as no route specifier is a valid configuration on
-					// the server side.
-				default:
-					return nil, fmt.Errorf("unsupported type %T for RouteSpecifier", hcm.RouteSpecifier)
 				}
 			}
 		default:

xds/internal/xdsclient/filter_chain_test.go

@@ -40,6 +40,7 @@ import (
 
 	iresolver "google.golang.org/grpc/internal/resolver"
 	"google.golang.org/grpc/internal/testutils"
+	"google.golang.org/grpc/internal/xds/env"
 	"google.golang.org/grpc/xds/internal/httpfilter"
 	"google.golang.org/grpc/xds/internal/httpfilter/router"
 	"google.golang.org/grpc/xds/internal/testutils/e2e"
@@ -456,6 +457,11 @@ func TestNewFilterChainImpl_Failure_BadSecurityConfig(t *testing.T) {
 // TestNewFilterChainImpl_Success_RouteUpdate tests the construction of the
 // filter chain with valid HTTP Filters present.
 func TestNewFilterChainImpl_Success_RouteUpdate(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	tests := []struct {
 		name   string
 		lis    *v3listenerpb.Listener
@@ -691,6 +697,11 @@ func TestNewFilterChainImpl_Success_RouteUpdate(t *testing.T) {
 // TestNewFilterChainImpl_Failure_BadRouteUpdate verifies cases where the Route
 // Update in the filter chain are invalid.
 func TestNewFilterChainImpl_Failure_BadRouteUpdate(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	tests := []struct {
 		name    string
 		lis     *v3listenerpb.Listener
@@ -864,6 +875,11 @@ func TestNewFilterChainImpl_Failure_BadHTTPFilters(t *testing.T) {
 // TestNewFilterChainImpl_Success_HTTPFilters tests the construction of the
 // filter chain with valid HTTP Filters present.
 func TestNewFilterChainImpl_Success_HTTPFilters(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	tests := []struct {
 		name   string
 		lis    *v3listenerpb.Listener
@@ -1182,6 +1198,11 @@ func TestNewFilterChainImpl_Success_HTTPFilters(t *testing.T) {
 // TestNewFilterChainImpl_Success_SecurityConfig verifies cases where the
 // security configuration in the filter chain contains valid data.
 func TestNewFilterChainImpl_Success_SecurityConfig(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	tests := []struct {
 		desc   string
 		lis    *v3listenerpb.Listener
@@ -1409,6 +1430,11 @@ func TestNewFilterChainImpl_Success_SecurityConfig(t *testing.T) {
 // success at config validation time and the filter chains which contains
 // unsupported match fields will be skipped at lookup time.
 func TestNewFilterChainImpl_Success_UnsupportedMatchFields(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	unspecifiedEntry := &destPrefixEntry{
 		srcTypeArr: [3]*sourcePrefixes{
 			{
@@ -1574,6 +1600,11 @@ func TestNewFilterChainImpl_Success_UnsupportedMatchFields(t *testing.T) {
 // TestNewFilterChainImpl_Success_AllCombinations verifies different
 // combinations of the supported match criteria.
 func TestNewFilterChainImpl_Success_AllCombinations(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	tests := []struct {
 		desc   string
 		lis    *v3listenerpb.Listener
@@ -2220,6 +2251,11 @@ func TestLookup_Failures(t *testing.T) {
 }
 
 func TestLookup_Successes(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	lisWithDefaultChain := &v3listenerpb.Listener{
 		FilterChains: []*v3listenerpb.FilterChain{
 			{

xds/internal/xdsclient/lds_test.go

@@ -33,6 +33,7 @@ import (
 	"google.golang.org/protobuf/types/known/durationpb"
 
 	"google.golang.org/grpc/internal/testutils"
+	"google.golang.org/grpc/internal/xds/env"
 	"google.golang.org/grpc/xds/internal/httpfilter"
 	_ "google.golang.org/grpc/xds/internal/httpfilter/router"
 	"google.golang.org/grpc/xds/internal/testutils/e2e"
@@ -601,6 +602,11 @@ func (s) TestUnmarshalListener_ClientSide(t *testing.T) {
 }
 
 func (s) TestUnmarshalListener_ServerSide(t *testing.T) {
+	oldRBAC := env.RBACSupport
+	env.RBACSupport = true
+	defer func() {
+		env.RBACSupport = oldRBAC
+	}()
 	const (
 		v3LDSTarget = "grpc/server?xds.resource.listening_address=0.0.0.0:9999"
 		testVersion = "test-version-lds-server"

xds/server.go

@@ -36,6 +36,7 @@ import (
 	"google.golang.org/grpc/internal/grpcsync"
 	iresolver "google.golang.org/grpc/internal/resolver"
 	"google.golang.org/grpc/internal/transport"
+	"google.golang.org/grpc/internal/xds/env"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/status"
 	"google.golang.org/grpc/xds/internal/server"
@@ -380,17 +381,21 @@ func routeAndProcess(ctx context.Context) error {
 // xdsUnaryInterceptor is the unary interceptor added to the gRPC server to
 // perform any xDS specific functionality on unary RPCs.
 func xdsUnaryInterceptor(ctx context.Context, req interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
-	if err := routeAndProcess(ctx); err != nil {
-		return nil, err
+	if env.RBACSupport {
+		if err := routeAndProcess(ctx); err != nil {
+			return nil, err
+		}
 	}
 	return handler(ctx, req)
 }
 
 // xdsStreamInterceptor is the stream interceptor added to the gRPC server to
 // perform any xDS specific functionality on streaming RPCs.
 func xdsStreamInterceptor(srv interface{}, ss grpc.ServerStream, _ *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
-	if err := routeAndProcess(ss.Context()); err != nil {
-		return err
+	if env.RBACSupport {
+		if err := routeAndProcess(ss.Context()); err != nil {
+			return err
+		}
 	}
 	return handler(srv, ss)
 }