Skip to content

Commit ecf8daa

Browse files
craig[bot]jaylim-crlnvbrafiss
committed
107729: ccl/sqlproxyccl: remove ConnectivityType from tenants r=jaylim-crl a=jaylim-crl 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 108151: kv: deflake TestTxnCoordSenderRetries r=arulajmani a=nvanbenschoten Fixes cockroachdb#107847. This commit deflakes `TestTxnCoordSenderRetries`, which was observed to fail if a load-based split was performed at key "ab". A split at this key had the effect of turning the "write too old in staging commit" subtest into something closer to "multi-range batch commit with write too old (err on first range)", where the transaction record and write-write conflict were on different ranges. This commit deflakes the test by disabling load-based splits. While diagnosing the issue, I found two easy ways to reproduce the failure which demonstrate that load-based splitting was the source of the flakiness: - manually split at key "ab" - sleep for 10ms between each subtest, giving load-based splitting more time With load-based splitting disabled, the test passes under race+stress even with the 10ms pauses. Release note: None 108205: logictest: increase lease transfer timeout for cockroach-go/testserver r=rafiss a=rafiss Since the test stops and starts nodes, giving more time to transfer leases during shutdown should make it less flaky. fixes cockroachdb#107778 Release note: None Co-authored-by: Jay <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
4 parents 7a597b6 + e332c5f + 0aee1bb + 892ea7b commit ecf8daa

File tree

10 files changed

+55
-142
lines changed

10 files changed

+55
-142
lines changed

‎pkg/ccl/sqlproxyccl/acl/cidr_ranges.go‎

Lines changed: 18 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,8 @@ import (
1616
)
1717

1818
// CIDRRanges represents the controller used to manage ACL rules for public
19-
// connections. It rejects connections if:
20-
// 1. the cluster does not allow public connections, or
21-
// 2. none of the allowed_cidr_ranges entries match the incoming connection's
22-
// IP.
19+
// connections. It rejects connections if none of the AllowedCIDRRanges entries
20+
// match the incoming connection's IP.
2321
type CIDRRanges struct {
2422
LookupTenantFn lookupTenantFunc
2523
}
@@ -33,33 +31,30 @@ func (p *CIDRRanges) CheckConnection(ctx context.Context, conn ConnectionTags) e
3331
return nil
3432
}
3533

34+
ip := net.ParseIP(conn.IP)
35+
if ip == nil {
36+
return errors.Newf("could not parse IP address: '%s'", conn.IP)
37+
}
38+
3639
tenantObj, err := p.LookupTenantFn(ctx, conn.TenantID)
3740
if err != nil {
3841
return err
3942
}
40-
41-
// Cluster allows public connections, so we'll check allowed CIDR ranges.
42-
if tenantObj.AllowPublicConn() {
43-
ip := net.ParseIP(conn.IP)
44-
if ip == nil {
45-
return errors.Newf("could not parse IP address: '%s'", conn.IP)
43+
for _, cidrRange := range tenantObj.AllowedCIDRRanges {
44+
// It is assumed that all public CIDR ranges are valid, so the
45+
// tenant directory server will have to enforce that.
46+
_, ipNetwork, err := net.ParseCIDR(cidrRange)
47+
if err != nil {
48+
return err
4649
}
47-
for _, cidrRange := range tenantObj.AllowedCIDRRanges {
48-
// It is assumed that all public CIDR ranges are valid, so the
49-
// tenant directory server will have to enforce that.
50-
_, ipNetwork, err := net.ParseCIDR(cidrRange)
51-
if err != nil {
52-
return err
53-
}
54-
// A matching CIDR range was found.
55-
if ipNetwork.Contains(ip) {
56-
return nil
57-
}
50+
// A matching CIDR range was found.
51+
if ipNetwork.Contains(ip) {
52+
return nil
5853
}
5954
}
6055

61-
// By default, connections are rejected if the cluster does not allow public
62-
// connections, or if no ranges match the connection's IP.
56+
// By default, connections are rejected if no ranges match the connection's
57+
// IP.
6358
return errors.Newf(
6459
"connection to '%s' denied: cluster does not allow public connections from IP %s",
6560
conn.TenantID.String(),

‎pkg/ccl/sqlproxyccl/acl/cidr_ranges_test.go‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,11 @@ func TestCIDRRanges(t *testing.T) {
4343
require.EqualError(t, err, "foo")
4444
})
4545

46-
// Private connection should allow, despite not having any CIDR ranges.
46+
// Private connection should be allowed, despite not having any CIDR ranges.
4747
t.Run("private connection", func(t *testing.T) {
4848
p := &acl.CIDRRanges{
4949
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
50-
return &tenant.Tenant{
51-
ConnectivityType: tenant.ALLOW_PUBLIC_ONLY,
52-
}, nil
50+
return &tenant.Tenant{}, nil
5351
},
5452
}
5553
err := p.CheckConnection(ctx, makeConn("foo"))
@@ -61,7 +59,6 @@ func TestCIDRRanges(t *testing.T) {
6159
p := &acl.CIDRRanges{
6260
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
6361
return &tenant.Tenant{
64-
ConnectivityType: tenant.ALLOW_ALL,
6562
AllowedCIDRRanges: []string{"127.0.0.0/32", "10.0.0.8/16"},
6663
}, nil
6764
},
@@ -74,7 +71,6 @@ func TestCIDRRanges(t *testing.T) {
7471
p := &acl.CIDRRanges{
7572
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
7673
return &tenant.Tenant{
77-
ConnectivityType: tenant.ALLOW_ALL,
7874
AllowedCIDRRanges: []string{},
7975
}, nil
8076
},
@@ -87,7 +83,6 @@ func TestCIDRRanges(t *testing.T) {
8783
p := &acl.CIDRRanges{
8884
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
8985
return &tenant.Tenant{
90-
ConnectivityType: tenant.ALLOW_ALL,
9186
AllowedCIDRRanges: []string{"0.0.0.0/0"},
9287
}, nil
9388
},
@@ -96,24 +91,10 @@ func TestCIDRRanges(t *testing.T) {
9691
require.NoError(t, err)
9792
})
9893

99-
t.Run("disallow public connections", func(t *testing.T) {
100-
p := &acl.CIDRRanges{
101-
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
102-
return &tenant.Tenant{
103-
ConnectivityType: tenant.ALLOW_PRIVATE_ONLY,
104-
AllowedCIDRRanges: []string{"127.0.0.1/32"},
105-
}, nil
106-
},
107-
}
108-
err := p.CheckConnection(ctx, makeConn(""))
109-
require.EqualError(t, err, "connection to '42' denied: cluster does not allow public connections from IP 127.0.0.1")
110-
})
111-
11294
t.Run("could not parse connection IP", func(t *testing.T) {
11395
p := &acl.CIDRRanges{
11496
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
11597
return &tenant.Tenant{
116-
ConnectivityType: tenant.ALLOW_ALL,
11798
AllowedCIDRRanges: []string{"127.0.0.1/32"},
11899
}, nil
119100
},
@@ -129,7 +110,6 @@ func TestCIDRRanges(t *testing.T) {
129110
p := &acl.CIDRRanges{
130111
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
131112
return &tenant.Tenant{
132-
ConnectivityType: tenant.ALLOW_ALL,
133113
AllowedCIDRRanges: []string{"127.0.0.1"},
134114
}, nil
135115
},

‎pkg/ccl/sqlproxyccl/acl/private_endpoints.go‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,8 @@ type lookupTenantFunc func(ctx context.Context, tenantID roachpb.TenantID) (*ten
2525
// PrivateEndpoints represents the controller used to manage ACL rules for
2626
// private connections. A connection is assumed to be private if it includes
2727
// the EndpointID field, which gets populated through the PROXY headers. The
28-
// controller rejects connections if:
29-
// 1. the cluster does not allow private connections, or
30-
// 2. none of the allowed_private_endpoints entries match the incoming
31-
// connection's endpoint identifier.
28+
// controller rejects connections if none of the AllowedPrivateEndpoints
29+
// entries match the incoming connection's endpoint identifier.
3230
type PrivateEndpoints struct {
3331
LookupTenantFn lookupTenantFunc
3432
}
@@ -46,20 +44,15 @@ func (p *PrivateEndpoints) CheckConnection(ctx context.Context, conn ConnectionT
4644
if err != nil {
4745
return err
4846
}
49-
50-
// Cluster allows private connections, so we'll check allowed endpoints.
51-
if tenantObj.AllowPrivateConn() {
52-
for _, endpoints := range tenantObj.AllowedPrivateEndpoints {
53-
// A matching endpointID was found.
54-
if endpoints == conn.EndpointID {
55-
return nil
56-
}
47+
for _, endpoints := range tenantObj.AllowedPrivateEndpoints {
48+
// A matching endpointID was found.
49+
if endpoints == conn.EndpointID {
50+
return nil
5751
}
5852
}
5953

60-
// By default, connections are rejected if the cluster does not allow
61-
// private connections, or if no endpoints match the connection's endpoint
62-
// ID.
54+
// By default, connections are rejected if no endpoints match the
55+
// connection's endpoint ID.
6356
return errors.Newf(
6457
"connection to '%s' denied: cluster does not allow private connections from endpoint '%s'",
6558
conn.TenantID.String(),

‎pkg/ccl/sqlproxyccl/acl/private_endpoints_test.go‎

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,12 @@ func TestPrivateEndpoints(t *testing.T) {
4545
require.EqualError(t, err, "foo")
4646
})
4747

48-
// Public connection should allow, despite not having any private endpoints.
48+
// Public connection should be allowed, despite not having any private
49+
// endpoints.
4950
t.Run("public connection", func(t *testing.T) {
5051
p := &acl.PrivateEndpoints{
5152
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
52-
return &tenant.Tenant{
53-
ConnectivityType: tenant.ALLOW_PRIVATE_ONLY,
54-
}, nil
53+
return &tenant.Tenant{}, nil
5554
},
5655
}
5756
err := p.CheckConnection(ctx, makeConn(""))
@@ -63,7 +62,6 @@ func TestPrivateEndpoints(t *testing.T) {
6362
p := &acl.PrivateEndpoints{
6463
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
6564
return &tenant.Tenant{
66-
ConnectivityType: tenant.ALLOW_ALL,
6765
AllowedPrivateEndpoints: []string{"foo", "baz"},
6866
}, nil
6967
},
@@ -76,7 +74,6 @@ func TestPrivateEndpoints(t *testing.T) {
7674
p := &acl.PrivateEndpoints{
7775
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
7876
return &tenant.Tenant{
79-
ConnectivityType: tenant.ALLOW_ALL,
8077
AllowedPrivateEndpoints: []string{},
8178
}, nil
8279
},
@@ -89,27 +86,13 @@ func TestPrivateEndpoints(t *testing.T) {
8986
p := &acl.PrivateEndpoints{
9087
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
9188
return &tenant.Tenant{
92-
ConnectivityType: tenant.ALLOW_ALL,
9389
AllowedPrivateEndpoints: []string{"foo"},
9490
}, nil
9591
},
9692
}
9793
err := p.CheckConnection(ctx, makeConn("foo"))
9894
require.NoError(t, err)
9995
})
100-
101-
t.Run("disallow private connections", func(t *testing.T) {
102-
p := &acl.PrivateEndpoints{
103-
LookupTenantFn: func(ctx context.Context, tenantID roachpb.TenantID) (*tenant.Tenant, error) {
104-
return &tenant.Tenant{
105-
ConnectivityType: tenant.ALLOW_PUBLIC_ONLY,
106-
AllowedPrivateEndpoints: []string{"foo"},
107-
}, nil
108-
},
109-
}
110-
err := p.CheckConnection(ctx, makeConn("foo"))
111-
require.EqualError(t, err, "connection to '42' denied: cluster does not allow private connections from endpoint 'foo'")
112-
})
11396
}
11497

11598
func TestFindPrivateEndpointID(t *testing.T) {

‎pkg/ccl/sqlproxyccl/acl/watcher_test.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,6 @@ func TestACLWatcher(t *testing.T) {
8888
Version: "001",
8989
TenantID: tenantID.ToUint64(),
9090
ClusterName: "my-tenant",
91-
ConnectivityType: tenant.ALLOW_ALL,
9291
AllowedCIDRRanges: []string{"1.1.0.0/16"},
9392
AllowedPrivateEndpoints: []string{"foo-bar-baz", "cockroachdb"},
9493
})
@@ -266,7 +265,6 @@ func TestACLWatcher(t *testing.T) {
266265
Version: "002",
267266
TenantID: tenantID.ToUint64(),
268267
ClusterName: "my-tenant",
269-
ConnectivityType: tenant.ALLOW_ALL,
270268
AllowedCIDRRanges: []string{"1.1.0.0/16"},
271269
AllowedPrivateEndpoints: []string{"foo-bar-baz"},
272270
})
@@ -319,7 +317,6 @@ func TestACLWatcher(t *testing.T) {
319317
Version: "003",
320318
TenantID: tenantID.ToUint64(),
321319
ClusterName: "my-tenant",
322-
ConnectivityType: tenant.ALLOW_ALL,
323320
AllowedCIDRRanges: []string{"127.0.0.1/32"},
324321
})
325322

‎pkg/ccl/sqlproxyccl/proxy_handler_test.go‎

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -263,22 +263,18 @@ func TestPrivateEndpointsACL(t *testing.T) {
263263
Version: "001",
264264
TenantID: tenant10.ToUint64(),
265265
ClusterName: "my-tenant",
266-
ConnectivityType: tenant.ALLOW_ALL,
267266
AllowedPrivateEndpoints: []string{"vpce-abc123"},
268267
})
269268
tds.CreateTenant(tenant20, &tenant.Tenant{
270269
Version: "002",
271270
TenantID: tenant20.ToUint64(),
272271
ClusterName: "other-tenant",
273-
ConnectivityType: tenant.ALLOW_ALL,
274272
AllowedPrivateEndpoints: []string{"vpce-some-other-vpc"},
275273
})
276274
tds.CreateTenant(tenant30, &tenant.Tenant{
277-
Version: "003",
278-
TenantID: tenant30.ToUint64(),
279-
ClusterName: "public-tenant",
280-
ConnectivityType: tenant.ALLOW_PUBLIC_ONLY,
281-
AllowedPrivateEndpoints: []string{},
275+
Version: "003",
276+
TenantID: tenant30.ToUint64(),
277+
ClusterName: "public-tenant",
282278
})
283279
// All tenants map to the same pod.
284280
for _, tenID := range []roachpb.TenantID{tenant10, tenant20, tenant30} {
@@ -350,7 +346,6 @@ func TestPrivateEndpointsACL(t *testing.T) {
350346
Version: "010",
351347
TenantID: tenant10.ToUint64(),
352348
ClusterName: "my-tenant",
353-
ConnectivityType: tenant.ALLOW_ALL,
354349
AllowedPrivateEndpoints: []string{},
355350
})
356351

@@ -436,22 +431,18 @@ func TestAllowedCIDRRangesACL(t *testing.T) {
436431
Version: "001",
437432
TenantID: tenant10.ToUint64(),
438433
ClusterName: "my-tenant",
439-
ConnectivityType: tenant.ALLOW_ALL,
440434
AllowedCIDRRanges: []string{"127.0.0.1/32"},
441435
})
442436
tds.CreateTenant(tenant20, &tenant.Tenant{
443437
Version: "002",
444438
TenantID: tenant20.ToUint64(),
445439
ClusterName: "other-tenant",
446-
ConnectivityType: tenant.ALLOW_ALL,
447440
AllowedCIDRRanges: []string{"10.0.0.8/32"},
448441
})
449442
tds.CreateTenant(tenant30, &tenant.Tenant{
450-
Version: "003",
451-
TenantID: tenant30.ToUint64(),
452-
ClusterName: "private-tenant",
453-
ConnectivityType: tenant.ALLOW_PRIVATE_ONLY,
454-
AllowedCIDRRanges: []string{"0.0.0.0/0"},
443+
Version: "003",
444+
TenantID: tenant30.ToUint64(),
445+
ClusterName: "private-tenant",
455446
})
456447
// All tenants map to the same pod.
457448
for _, tenID := range []roachpb.TenantID{tenant10, tenant20, tenant30} {
@@ -483,7 +474,6 @@ func TestAllowedCIDRRangesACL(t *testing.T) {
483474
Version: "010",
484475
TenantID: tenant10.ToUint64(),
485476
ClusterName: "my-tenant",
486-
ConnectivityType: tenant.ALLOW_ALL,
487477
AllowedCIDRRanges: []string{},
488478
})
489479

‎pkg/ccl/sqlproxyccl/tenant/directory.proto‎

Lines changed: 5 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -94,29 +94,6 @@ message EnsurePodRequest {
9494
message EnsurePodResponse {
9595
}
9696

97-
// ConnectivityType indicates the extent to which the tenant can be accessed
98-
// (e.g. private only, public only, or both).
99-
enum ConnectivityType {
100-
option (gogoproto.goproto_enum_prefix) = false;
101-
102-
// ALLOW_ALL indicates that the cluster allows both public and private
103-
// connections. In this case, both IP allowlist and private endpoint rules
104-
// will apply.
105-
//
106-
// The proxy will block incoming connections if:
107-
// 1. the connection is public, and there are no IP allowlist entries, or
108-
// 2. the connection is private, and there are no private endpoint entries.
109-
ALLOW_ALL = 0;
110-
// ALLOW_PUBLIC_ONLY indicates that the cluster is exposed to the public
111-
// internet, and IP allowlist rules will apply. By default, if there are no
112-
// rules, the proxy will block all public connections.
113-
ALLOW_PUBLIC_ONLY = 1;
114-
// ALLOW_PRIVATE_ONLY indicates that the cluster allows private endpoints, and
115-
// private endpoint rules will apply. By default, if there are no rules, the
116-
// proxy will block all private connections.
117-
ALLOW_PRIVATE_ONLY = 2;
118-
}
119-
12097
// Tenant contains information about a tenant, such as its name and ID.
12198
// Fields are optional except TenantID and Version.
12299
message Tenant {
@@ -127,15 +104,14 @@ message Tenant {
127104
string version = 2;
128105
// ClusterName is the name of the tenant's cluster.
129106
string cluster_name = 3;
130-
// ConnectivityType indicates the extent to which the tenant can be accessed.
131-
ConnectivityType connectivity_type = 4;
107+
reserved 4;
132108
// AllowedCIDRRanges corresponds to the list of source CIDR ranges that are
133-
// allowed to access the tenant. This will only apply if the tenant allows
134-
// public connections.
109+
// allowed to access the tenant. By default, if there are no rules, the proxy
110+
// will block all public connections.
135111
repeated string allowed_cidr_ranges = 5 [(gogoproto.customname) = "AllowedCIDRRanges"];
136112
// AllowedPrivateEndpoints corresponds to the list of endpoint identifiers
137-
// that are allowed to access the tenant. This will only apply if the tenant
138-
// allows private connections.
113+
// that are allowed to access the tenant. By default, if there are no rules,
114+
// the proxy will block all private connections.
139115
repeated string allowed_private_endpoints = 6;
140116
}
141117

‎pkg/ccl/sqlproxyccl/tenant/entry.go‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -334,15 +334,3 @@ func hasRunningPod(pods []*Pod) bool {
334334
}
335335
return false
336336
}
337-
338-
// AllowPrivateConn returns true if the tenant allows private connections to it,
339-
// or false otherwise.
340-
func (t *Tenant) AllowPrivateConn() bool {
341-
return t.ConnectivityType == ALLOW_ALL || t.ConnectivityType == ALLOW_PRIVATE_ONLY
342-
}
343-
344-
// AllowPublicConn returns true if the tenant allows public connections to it,
345-
// or false otherwise.
346-
func (t *Tenant) AllowPublicConn() bool {
347-
return t.ConnectivityType == ALLOW_ALL || t.ConnectivityType == ALLOW_PUBLIC_ONLY
348-
}

0 commit comments

Comments
 (0)