ccl/sqlproxyccl: remove ConnectivityType from tenants

Previously, we added a ConnectivityType to tenants with the intention of
restricting ACLs to only public or private connections. After some internal
discussions, this field seems to be redundant. Callers should just supply an
empty AllowedCIDRRanges or AllowedPrivateEndpoints field if they wanted to
restrict specific types of connections. This commit removes the unnecessary
ConnectivityType field from the tenant object.

Release note: None

Epic: none

Author: jaylim-crl

pkg/ccl/sqlproxyccl/acl/cidr_ranges.go

@@ -16,10 +16,8 @@ import (
 )
 
 // CIDRRanges represents the controller used to manage ACL rules for public
-// connections. It rejects connections if:
-//  1. the cluster does not allow public connections, or
-//  2. none of the allowed_cidr_ranges entries match the incoming connection's
-//     IP.
+// connections. It rejects connections if none of the AllowedCIDRRanges entries
+// match the incoming connection's IP.
 type CIDRRanges struct {
 	LookupTenantFn lookupTenantFunc
 }
@@ -33,33 +31,30 @@ func (p *CIDRRanges) CheckConnection(ctx context.Context, conn ConnectionTags) e
 		return nil
 	}
 
+	ip := net.ParseIP(conn.IP)
+	if ip == nil {
+		return errors.Newf("could not parse IP address: '%s'", conn.IP)
+	}
+
 	tenantObj, err := p.LookupTenantFn(ctx, conn.TenantID)
 	if err != nil {
 		return err
 	}
-
-	// Cluster allows public connections, so we'll check allowed CIDR ranges.
-	if tenantObj.AllowPublicConn() {
-		ip := net.ParseIP(conn.IP)
-		if ip == nil {
-			return errors.Newf("could not parse IP address: '%s'", conn.IP)
+	for _, cidrRange := range tenantObj.AllowedCIDRRanges {
+		// It is assumed that all public CIDR ranges are valid, so the
+		// tenant directory server will have to enforce that.
+		_, ipNetwork, err := net.ParseCIDR(cidrRange)
+		if err != nil {
+			return err
 		}
-		for _, cidrRange := range tenantObj.AllowedCIDRRanges {
-			// It is assumed that all public CIDR ranges are valid, so the
-			// tenant directory server will have to enforce that.
-			_, ipNetwork, err := net.ParseCIDR(cidrRange)
-			if err != nil {
-				return err
-			}
-			// A matching CIDR range was found.
-			if ipNetwork.Contains(ip) {
-				return nil
-			}
+		// A matching CIDR range was found.
+		if ipNetwork.Contains(ip) {
+			return nil
 		}
 	}
 
-	// By default, connections are rejected if the cluster does not allow public
-	// connections, or if no ranges match the connection's IP.
+	// By default, connections are rejected if no ranges match the connection's
+	// IP.
 	return errors.Newf(
 		"connection to '%s' denied: cluster does not allow public connections from IP %s",
 		conn.TenantID.String(),

pkg/ccl/sqlproxyccl/acl/cidr_ranges_test.go

@@ -43,13 +43,11 @@ func TestCIDRRanges(t *testing.T) {
 		require.EqualError(t, err, "foo")
 	})
 
-	// Private connection should allow, despite not having any CIDR ranges.
+	// Private connection should be allowed, despite not having any CIDR ranges.
 	t.Run("private connection", func(t *testing.T) {
 		p := &acl.CIDRRanges{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
-				return &tenant.Tenant{
-					ConnectivityType: tenant.ALLOW_PUBLIC_ONLY,
-				}, nil
+				return &tenant.Tenant{}, nil
 			},
 		}
 		err := p.CheckConnection(ctx, makeConn("foo"))
@@ -61,7 +59,6 @@ func TestCIDRRanges(t *testing.T) {
 		p := &acl.CIDRRanges{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
 				return &tenant.Tenant{
-					ConnectivityType:  tenant.ALLOW_ALL,
 					AllowedCIDRRanges: []string{"127.0.0.0/32", "10.0.0.8/16"},
 				}, nil
 			},
@@ -74,7 +71,6 @@ func TestCIDRRanges(t *testing.T) {
 		p := &acl.CIDRRanges{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
 				return &tenant.Tenant{
-					ConnectivityType:  tenant.ALLOW_ALL,
 					AllowedCIDRRanges: []string{},
 				}, nil
 			},
@@ -87,7 +83,6 @@ func TestCIDRRanges(t *testing.T) {
 		p := &acl.CIDRRanges{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
 				return &tenant.Tenant{
-					ConnectivityType:  tenant.ALLOW_ALL,
 					AllowedCIDRRanges: []string{"0.0.0.0/0"},
 				}, nil
 			},
@@ -96,24 +91,10 @@ func TestCIDRRanges(t *testing.T) {
 		require.NoError(t, err)
 	})
 
-	t.Run("disallow public connections", func(t *testing.T) {
-		p := &acl.CIDRRanges{
-			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
-				return &tenant.Tenant{
-					ConnectivityType:  tenant.ALLOW_PRIVATE_ONLY,
-					AllowedCIDRRanges: []string{"127.0.0.1/32"},
-				}, nil
-			},
-		}
-		err := p.CheckConnection(ctx, makeConn(""))
-		require.EqualError(t, err, "connection to '42' denied: cluster does not allow public connections from IP 127.0.0.1")
-	})
-
 	t.Run("could not parse connection IP", func(t *testing.T) {
 		p := &acl.CIDRRanges{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
 				return &tenant.Tenant{
-					ConnectivityType:  tenant.ALLOW_ALL,
 					AllowedCIDRRanges: []string{"127.0.0.1/32"},
 				}, nil
 			},
@@ -129,7 +110,6 @@ func TestCIDRRanges(t *testing.T) {
 		p := &acl.CIDRRanges{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
 				return &tenant.Tenant{
-					ConnectivityType:  tenant.ALLOW_ALL,
 					AllowedCIDRRanges: []string{"127.0.0.1"},
 				}, nil
 			},

pkg/ccl/sqlproxyccl/acl/private_endpoints.go

@@ -25,10 +25,8 @@ type lookupTenantFunc func(ctx context.Context, tenantID roachpb.TenantID) (*ten
 // PrivateEndpoints represents the controller used to manage ACL rules for
 // private connections. A connection is assumed to be private if it includes
 // the EndpointID field, which gets populated through the PROXY headers. The
-// controller rejects connections if:
-//  1. the cluster does not allow private connections, or
-//  2. none of the allowed_private_endpoints entries match the incoming
-//     connection's endpoint identifier.
+// controller rejects connections if none of the AllowedPrivateEndpoints
+// entries match the incoming connection's endpoint identifier.
 type PrivateEndpoints struct {
 	LookupTenantFn lookupTenantFunc
 }
@@ -46,20 +44,15 @@ func (p *PrivateEndpoints) CheckConnection(ctx context.Context, conn ConnectionT
 	if err != nil {
 		return err
 	}
-
-	// Cluster allows private connections, so we'll check allowed endpoints.
-	if tenantObj.AllowPrivateConn() {
-		for _, endpoints := range tenantObj.AllowedPrivateEndpoints {
-			// A matching endpointID was found.
-			if endpoints == conn.EndpointID {
-				return nil
-			}
+	for _, endpoints := range tenantObj.AllowedPrivateEndpoints {
+		// A matching endpointID was found.
+		if endpoints == conn.EndpointID {
+			return nil
 		}
 	}
 
-	// By default, connections are rejected if the cluster does not allow
-	// private connections, or if no endpoints match the connection's endpoint
-	// ID.
+	// By default, connections are rejected if no endpoints match the
+	// connection's endpoint ID.
 	return errors.Newf(
 		"connection to '%s' denied: cluster does not allow private connections from endpoint '%s'",
 		conn.TenantID.String(),

pkg/ccl/sqlproxyccl/acl/private_endpoints_test.go

@@ -45,13 +45,12 @@ func TestPrivateEndpoints(t *testing.T) {
 		require.EqualError(t, err, "foo")
 	})
 
-	// Public connection should allow, despite not having any private endpoints.
+	// Public connection should be allowed, despite not having any private
+	// endpoints.
 	t.Run("public connection", func(t *testing.T) {
 		p := &acl.PrivateEndpoints{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
-				return &tenant.Tenant{
-					ConnectivityType: tenant.ALLOW_PRIVATE_ONLY,
-				}, nil
+				return &tenant.Tenant{}, nil
 			},
 		}
 		err := p.CheckConnection(ctx, makeConn(""))
@@ -63,7 +62,6 @@ func TestPrivateEndpoints(t *testing.T) {
 		p := &acl.PrivateEndpoints{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
 				return &tenant.Tenant{
-					ConnectivityType:        tenant.ALLOW_ALL,
 					AllowedPrivateEndpoints: []string{"foo", "baz"},
 				}, nil
 			},
@@ -76,7 +74,6 @@ func TestPrivateEndpoints(t *testing.T) {
 		p := &acl.PrivateEndpoints{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
 				return &tenant.Tenant{
-					ConnectivityType:        tenant.ALLOW_ALL,
 					AllowedPrivateEndpoints: []string{},
 				}, nil
 			},
@@ -89,27 +86,13 @@ func TestPrivateEndpoints(t *testing.T) {
 		p := &acl.PrivateEndpoints{
 			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
 				return &tenant.Tenant{
-					ConnectivityType:        tenant.ALLOW_ALL,
 					AllowedPrivateEndpoints: []string{"foo"},
 				}, nil
 			},
 		}
 		err := p.CheckConnection(ctx, makeConn("foo"))
 		require.NoError(t, err)
 	})
-
-	t.Run("disallow private connections", func(t *testing.T) {
-		p := &acl.PrivateEndpoints{
-			LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
-				return &tenant.Tenant{
-					ConnectivityType:        tenant.ALLOW_PUBLIC_ONLY,
-					AllowedPrivateEndpoints: []string{"foo"},
-				}, nil
-			},
-		}
-		err := p.CheckConnection(ctx, makeConn("foo"))
-		require.EqualError(t, err, "connection to '42' denied: cluster does not allow private connections from endpoint 'foo'")
-	})
 }
 
 func TestFindPrivateEndpointID(t *testing.T) {

pkg/ccl/sqlproxyccl/acl/watcher_test.go

@@ -88,7 +88,6 @@ func TestACLWatcher(t *testing.T) {
 		Version:                 "001",
 		TenantID:                tenantID.ToUint64(),
 		ClusterName:             "my-tenant",
-		ConnectivityType:        tenant.ALLOW_ALL,
 		AllowedCIDRRanges:       []string{"1.1.0.0/16"},
 		AllowedPrivateEndpoints: []string{"foo-bar-baz", "cockroachdb"},
 	})
@@ -266,7 +265,6 @@ func TestACLWatcher(t *testing.T) {
 			Version:                 "002",
 			TenantID:                tenantID.ToUint64(),
 			ClusterName:             "my-tenant",
-			ConnectivityType:        tenant.ALLOW_ALL,
 			AllowedCIDRRanges:       []string{"1.1.0.0/16"},
 			AllowedPrivateEndpoints: []string{"foo-bar-baz"},
 		})
@@ -319,7 +317,6 @@ func TestACLWatcher(t *testing.T) {
 			Version:           "003",
 			TenantID:          tenantID.ToUint64(),
 			ClusterName:       "my-tenant",
-			ConnectivityType:  tenant.ALLOW_ALL,
 			AllowedCIDRRanges: []string{"127.0.0.1/32"},
 		})
 

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -263,22 +263,18 @@ func TestPrivateEndpointsACL(t *testing.T) {
 		Version:                 "001",
 		TenantID:                tenant10.ToUint64(),
 		ClusterName:             "my-tenant",
-		ConnectivityType:        tenant.ALLOW_ALL,
 		AllowedPrivateEndpoints: []string{"vpce-abc123"},
 	})
 	tds.CreateTenant(tenant20, &tenant.Tenant{
 		Version:                 "002",
 		TenantID:                tenant20.ToUint64(),
 		ClusterName:             "other-tenant",
-		ConnectivityType:        tenant.ALLOW_ALL,
 		AllowedPrivateEndpoints: []string{"vpce-some-other-vpc"},
 	})
 	tds.CreateTenant(tenant30, &tenant.Tenant{
-		Version:                 "003",
-		TenantID:                tenant30.ToUint64(),
-		ClusterName:             "public-tenant",
-		ConnectivityType:        tenant.ALLOW_PUBLIC_ONLY,
-		AllowedPrivateEndpoints: []string{},
+		Version:     "003",
+		TenantID:    tenant30.ToUint64(),
+		ClusterName: "public-tenant",
 	})
 	// All tenants map to the same pod.
 	for _, tenID := range []roachpb.TenantID{tenant10, tenant20, tenant30} {
@@ -350,7 +346,6 @@ func TestPrivateEndpointsACL(t *testing.T) {
 					Version:                 "010",
 					TenantID:                tenant10.ToUint64(),
 					ClusterName:             "my-tenant",
-					ConnectivityType:        tenant.ALLOW_ALL,
 					AllowedPrivateEndpoints: []string{},
 				})
 
@@ -436,22 +431,18 @@ func TestAllowedCIDRRangesACL(t *testing.T) {
 		Version:           "001",
 		TenantID:          tenant10.ToUint64(),
 		ClusterName:       "my-tenant",
-		ConnectivityType:  tenant.ALLOW_ALL,
 		AllowedCIDRRanges: []string{"127.0.0.1/32"},
 	})
 	tds.CreateTenant(tenant20, &tenant.Tenant{
 		Version:           "002",
 		TenantID:          tenant20.ToUint64(),
 		ClusterName:       "other-tenant",
-		ConnectivityType:  tenant.ALLOW_ALL,
 		AllowedCIDRRanges: []string{"10.0.0.8/32"},
 	})
 	tds.CreateTenant(tenant30, &tenant.Tenant{
-		Version:           "003",
-		TenantID:          tenant30.ToUint64(),
-		ClusterName:       "private-tenant",
-		ConnectivityType:  tenant.ALLOW_PRIVATE_ONLY,
-		AllowedCIDRRanges: []string{"0.0.0.0/0"},
+		Version:     "003",
+		TenantID:    tenant30.ToUint64(),
+		ClusterName: "private-tenant",
 	})
 	// All tenants map to the same pod.
 	for _, tenID := range []roachpb.TenantID{tenant10, tenant20, tenant30} {
@@ -483,7 +474,6 @@ func TestAllowedCIDRRangesACL(t *testing.T) {
 				Version:           "010",
 				TenantID:          tenant10.ToUint64(),
 				ClusterName:       "my-tenant",
-				ConnectivityType:  tenant.ALLOW_ALL,
 				AllowedCIDRRanges: []string{},
 			})
 

pkg/ccl/sqlproxyccl/tenant/directory.proto

@@ -94,29 +94,6 @@ message EnsurePodRequest {
 message EnsurePodResponse {
 }
 
-// ConnectivityType indicates the extent to which the tenant can be accessed
-// (e.g. private only, public only, or both).
-enum ConnectivityType {
-  option (gogoproto.goproto_enum_prefix) = false;
-
-  // ALLOW_ALL indicates that the cluster allows both public and private
-  // connections. In this case, both IP allowlist and private endpoint rules
-  // will apply.
-  //
-  // The proxy will block incoming connections if:
-  //   1. the connection is public, and there are no IP allowlist entries, or
-  //   2. the connection is private, and there are no private endpoint entries.
-  ALLOW_ALL = 0;
-  // ALLOW_PUBLIC_ONLY indicates that the cluster is exposed to the public
-  // internet, and IP allowlist rules will apply. By default, if there are no
-  // rules, the proxy will block all public connections.
-  ALLOW_PUBLIC_ONLY = 1;
-  // ALLOW_PRIVATE_ONLY indicates that the cluster allows private endpoints, and
-  // private endpoint rules will apply. By default, if there are no rules, the
-  // proxy will block all private connections.
-  ALLOW_PRIVATE_ONLY = 2;
-}
-
 // Tenant contains information about a tenant, such as its name and ID.
 // Fields are optional except TenantID and Version.
 message Tenant {
@@ -127,15 +104,14 @@ message Tenant {
   string version = 2;
   // ClusterName is the name of the tenant's cluster.
   string cluster_name = 3;
-  // ConnectivityType indicates the extent to which the tenant can be accessed.
-  ConnectivityType connectivity_type = 4;
+  reserved 4;
   // AllowedCIDRRanges corresponds to the list of source CIDR ranges that are
-  // allowed to access the tenant. This will only apply if the tenant allows
-  // public connections.
+  // allowed to access the tenant. By default, if there are no rules, the proxy
+  // will block all public connections.
   repeated string allowed_cidr_ranges = 5 [(gogoproto.customname) = "AllowedCIDRRanges"];
   // AllowedPrivateEndpoints corresponds to the list of endpoint identifiers
-  // that are allowed to access the tenant. This will only apply if the tenant
-  // allows private connections.
+  // that are allowed to access the tenant. By default, if there are no rules,
+  // the proxy will block all private connections.
   repeated string allowed_private_endpoints = 6;
 }
 

pkg/ccl/sqlproxyccl/tenant/entry.go

@@ -334,15 +334,3 @@ func hasRunningPod(pods []*Pod) bool {
 	}
 	return false
 }
-
-// AllowPrivateConn returns true if the tenant allows private connections to it,
-// or false otherwise.
-func (t *Tenant) AllowPrivateConn() bool {
-	return t.ConnectivityType == ALLOW_ALL || t.ConnectivityType == ALLOW_PRIVATE_ONLY
-}
-
-// AllowPublicConn returns true if the tenant allows public connections to it,
-// or false otherwise.
-func (t *Tenant) AllowPublicConn() bool {
-	return t.ConnectivityType == ALLOW_ALL || t.ConnectivityType == ALLOW_PUBLIC_ONLY
-}