Skip to content

Commit

Permalink
Merge #98153 #98718
Browse files Browse the repository at this point in the history
98153: sqlproxyccl: change denylist into an access control list r=pjtatlow a=pjtatlow

To support an IP Allowlist in the sqlproxy, this change extends
the denylist code to make the Watcher support multiple AccessControllers.
Each AccessController is consulted before allowing a connection through,
and rechecked on any changes to the underlying files.

Part of: [CC-8136](https://cockroachlabs.atlassian.net/browse/CC-8136)

98718: storage: batch pebbleResults allocation r=sumeerbhola a=jbowens

Embed a `pebbleResults` struct on the `pebbleMVCCScanner` to avoid an allocation.

```
                                                                     │  23.1.txt   │           23.1-wip.txt            │
                                                                     │   sec/op    │   sec/op     vs base              │
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24   109.3µ ± 1%   105.2µ ± 3%  -3.73% (p=0.009 n=6)

                                                                     │   23.1.txt   │          23.1-wip.txt          │
                                                                     │     B/s      │      B/s       vs base         │
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24   68.36Ki ± 0%   78.12Ki ± 12%  ~ (p=0.061 n=6)

                                                                     │  23.1.txt   │            23.1-wip.txt            │
                                                                     │    B/op     │    B/op      vs base               │
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24   354.0 ± 22%   228.5 ± 28%  -35.45% (p=0.002 n=6)

                                                                     │  23.1.txt  │           23.1-wip.txt            │
                                                                     │ allocs/op  │ allocs/op   vs base               │
MVCCGet_Pebble/batch=true/versions=100/valueSize=8/numRangeKeys=0-24   3.000 ± 0%   2.000 ± 0%  -33.33% (p=0.002 n=6)
```

Informs #96960.
Epic: None
Release note: None

Co-authored-by: PJ Tatlow <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
  • Loading branch information
3 people committed Mar 16, 2023
3 parents 898a32a + a20ff9b + d93a851 commit 82fd797
Show file tree
Hide file tree
Showing 20 changed files with 1,303 additions and 778 deletions.
8 changes: 4 additions & 4 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ ALL_TESTS = [
"//pkg/ccl/spanconfigccl/spanconfigsqlwatcherccl:spanconfigsqlwatcherccl_test",
"//pkg/ccl/sqlitelogictestccl/tests/3node-tenant:3node-tenant_test",
"//pkg/ccl/sqlitelogictestccl:sqlitelogictestccl_test",
"//pkg/ccl/sqlproxyccl/acl:acl_test",
"//pkg/ccl/sqlproxyccl/balancer:balancer_test",
"//pkg/ccl/sqlproxyccl/denylist:denylist_test",
"//pkg/ccl/sqlproxyccl/interceptor:interceptor_test",
"//pkg/ccl/sqlproxyccl/tenant:tenant_test",
"//pkg/ccl/sqlproxyccl/throttler:throttler_test",
Expand Down Expand Up @@ -820,10 +820,10 @@ GO_TARGETS = [
"//pkg/ccl/sqlitelogictestccl/tests/3node-tenant:3node-tenant_test",
"//pkg/ccl/sqlitelogictestccl:sqlitelogictestccl",
"//pkg/ccl/sqlitelogictestccl:sqlitelogictestccl_test",
"//pkg/ccl/sqlproxyccl/acl:acl",
"//pkg/ccl/sqlproxyccl/acl:acl_test",
"//pkg/ccl/sqlproxyccl/balancer:balancer",
"//pkg/ccl/sqlproxyccl/balancer:balancer_test",
"//pkg/ccl/sqlproxyccl/denylist:denylist",
"//pkg/ccl/sqlproxyccl/denylist:denylist_test",
"//pkg/ccl/sqlproxyccl/interceptor:interceptor",
"//pkg/ccl/sqlproxyccl/interceptor:interceptor_test",
"//pkg/ccl/sqlproxyccl/tenant:tenant",
Expand Down Expand Up @@ -2438,8 +2438,8 @@ GET_X_DATA_TARGETS = [
"//pkg/ccl/sqlitelogictestccl:get_x_data",
"//pkg/ccl/sqlitelogictestccl/tests/3node-tenant:get_x_data",
"//pkg/ccl/sqlproxyccl:get_x_data",
"//pkg/ccl/sqlproxyccl/acl:get_x_data",
"//pkg/ccl/sqlproxyccl/balancer:get_x_data",
"//pkg/ccl/sqlproxyccl/denylist:get_x_data",
"//pkg/ccl/sqlproxyccl/interceptor:get_x_data",
"//pkg/ccl/sqlproxyccl/tenant:get_x_data",
"//pkg/ccl/sqlproxyccl/tenantdirsvr:get_x_data",
Expand Down
5 changes: 3 additions & 2 deletions pkg/ccl/sqlproxyccl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/base",
"//pkg/ccl/sqlproxyccl/acl",
"//pkg/ccl/sqlproxyccl/balancer",
"//pkg/ccl/sqlproxyccl/denylist",
"//pkg/ccl/sqlproxyccl/interceptor",
"//pkg/ccl/sqlproxyccl/tenant",
"//pkg/ccl/sqlproxyccl/tenantdirsvr",
Expand Down Expand Up @@ -80,8 +80,8 @@ go_test(
"//pkg/base",
"//pkg/ccl",
"//pkg/ccl/kvccl/kvtenantccl",
"//pkg/ccl/sqlproxyccl/acl",
"//pkg/ccl/sqlproxyccl/balancer",
"//pkg/ccl/sqlproxyccl/denylist",
"//pkg/ccl/sqlproxyccl/interceptor",
"//pkg/ccl/sqlproxyccl/tenant",
"//pkg/ccl/sqlproxyccl/tenantdirsvr",
Expand Down Expand Up @@ -119,6 +119,7 @@ go_test(
"@com_github_jackc_pgx_v4//:pgx",
"@com_github_stretchr_testify//assert",
"@com_github_stretchr_testify//require",
"@in_gopkg_yaml_v3//:yaml_v3",
"@org_golang_google_grpc//codes",
"@org_golang_google_grpc//status",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,19 @@ load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "denylist",
name = "acl",
srcs = [
"access_control.go",
"allowlist.go",
"denylist.go",
"file.go",
"watcher.go",
],
importpath = "github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl/denylist",
importpath = "github.com/cockroachdb/cockroach/pkg/ccl/sqlproxyccl/acl",
visibility = ["//visibility:public"],
deps = [
"//pkg/util/log",
"//pkg/util/metric",
"//pkg/util/syncutil",
"//pkg/util/timeutil",
"@com_github_cockroachdb_errors//:errors",
Expand All @@ -21,15 +24,16 @@ go_library(
)

go_test(
name = "denylist_test",
name = "acl_test",
srcs = [
"file_test.go",
"watcher_test.go",
],
args = ["-test.timeout=295s"],
embed = [":denylist"],
embed = [":acl"],
deps = [
"//pkg/util/leaktest",
"//pkg/util/metric",
"//pkg/util/timeutil",
"@com_github_google_btree//:btree",
"@com_github_stretchr_testify//require",
Expand Down
21 changes: 21 additions & 0 deletions pkg/ccl/sqlproxyccl/acl/access_control.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright 2023 The Cockroach Authors.
//
// Licensed as a CockroachDB Enterprise file under the Cockroach Community
// License (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt

package acl

import "github.com/cockroachdb/cockroach/pkg/util/timeutil"

// ConnectionTags contains connection properties to match against the denylist.
type ConnectionTags struct {
IP string
Cluster string
}

type AccessController interface {
CheckConnection(ConnectionTags, timeutil.TimeSource) error
}
85 changes: 85 additions & 0 deletions pkg/ccl/sqlproxyccl/acl/allowlist.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright 2023 The Cockroach Authors.
//
// Licensed as a CockroachDB Enterprise file under the Cockroach Community
// License (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt

package acl

import (
"net"

"github.com/cockroachdb/cockroach/pkg/util/timeutil"
"github.com/cockroachdb/errors"
)

type AllowlistFile struct {
Seq int64 `yaml:"SequenceNumber"`
Allowlist map[string]AllowEntry `yaml:"allowlist"`
}

// Allowlist represents the current IP Allowlist,
// which maps cluster IDs to a list of allowed IP ranges.
type Allowlist struct {
entries map[string]AllowEntry
}

func (al *Allowlist) UnmarshalYAML(unmarshal func(interface{}) error) error {
var f AllowlistFile
if err := unmarshal(&f); err != nil {
return err
}
al.entries = f.Allowlist
return nil
}

func (al *Allowlist) CheckConnection(
connection ConnectionTags, timeSource timeutil.TimeSource,
) error {
entry, ok := al.entries[connection.Cluster]
if !ok {
// No allowlist entry, allow all traffic
return nil
}
ip := net.ParseIP(connection.IP)
if ip == nil {
return errors.Newf("could not parse ip address: '%s'", ip)
}
// Check all ips for this cluster.
// If one of them contains the current IP then it's allowed.
for _, allowedIP := range entry.ips {
if allowedIP.Contains(ip) {
return nil
}
}

return errors.Newf("connection ip '%s' denied: ip address not allowed", connection.IP)
}

type AllowEntry struct {
ips []*net.IPNet
}

// This custom unmarshal code converts each string IP address into a *net.IPNet.
// If it cannot be parsed, it is currently ignored and not added to the AllowEntry.
func (e *AllowEntry) UnmarshalYAML(unmarshal func(interface{}) error) error {
var raw struct {
IPs []string `yaml:"ips"`
}

if err := unmarshal(&raw); err != nil {
return err
}
e.ips = make([]*net.IPNet, 0)

for _, ip := range raw.IPs {
_, ipNet, _ := net.ParseCIDR(ip)
if ipNet != nil {
e.ips = append(e.ips, ipNet)
}
}

return nil
}
136 changes: 136 additions & 0 deletions pkg/ccl/sqlproxyccl/acl/denylist.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
// Copyright 2021 The Cockroach Authors.
//
// Licensed as a CockroachDB Enterprise file under the Cockroach Community
// License (the "License"); you may not use this file except in compliance with
// the License. You may obtain a copy of the License at
//
// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt

package acl

import (
"strings"
"time"

"github.com/cockroachdb/cockroach/pkg/util/timeutil"
"github.com/cockroachdb/errors"
)

// File represents a on-disk version of the denylist config.
// This also serves as a spec of expected yaml file format.
type DenylistFile struct {
Seq int64 `yaml:"SequenceNumber"`
Denylist []*DenyEntry `yaml:"denylist"`
}

// Denylist represents an in-memory cache for the current denylist.
// It also handles the logic of deciding what to be denied.
type Denylist struct {
entries map[DenyEntity]*DenyEntry
}

func (dl *Denylist) UnmarshalYAML(unmarshal func(interface{}) error) error {
var f DenylistFile
if err := unmarshal(&f); err != nil {
return err
}
dl.entries = make(map[DenyEntity]*DenyEntry)
for _, entry := range f.Denylist {
dl.entries[entry.Entity] = entry
}

return nil
}

func (dl *Denylist) CheckConnection(
connection ConnectionTags, timeSource timeutil.TimeSource,
) error {
ip := DenyEntity{Item: connection.IP, Type: IPAddrType}
if err := dl.denied(ip, timeSource); err != nil {
return errors.Wrapf(err, "connection ip '%v' denied", connection.IP)
}
cluster := DenyEntity{Item: connection.Cluster, Type: ClusterType}
if err := dl.denied(cluster, timeSource); err != nil {
return errors.Wrapf(err, "connection cluster '%v' denied", connection.Cluster)
}
return nil
}

// denied returns an error if the entity is denied access. The error message
// describes the reason for the denial.
func (dl *Denylist) denied(entity DenyEntity, timeSource timeutil.TimeSource) error {
if ent, ok := dl.entries[entity]; ok &&
(ent.Expiration.IsZero() || !ent.Expiration.Before(timeSource.Now())) {
return errors.Newf("%s", ent.Reason)
}
return nil
}

// DenyEntry records info about one denied entity,
// the reason and the expiration time.
// This also serves as spec for the yaml config format.
type DenyEntry struct {
Entity DenyEntity `yaml:"entity"`
Expiration time.Time `yaml:"expiration"`
Reason string `yaml:"reason"`
}

// DenyEntity represent one denied entity.
// This also serves as the spec for the config format.
type DenyEntity struct {
Item string `yaml:"item"`
Type DenyType `yaml:"type"`
}

// DenyType is the type of the denied entity.
type DenyType int

// Enum values for DenyType.
const (
IPAddrType DenyType = iota + 1
ClusterType
UnknownType
)

var strToTypeMap = map[string]DenyType{
"ip": IPAddrType,
"cluster": ClusterType,
}

var typeToStrMap = map[DenyType]string{
IPAddrType: "ip",
ClusterType: "cluster",
}

// UnmarshalYAML implements yaml.Unmarshaler interface for type.
func (typ *DenyType) UnmarshalYAML(unmarshal func(interface{}) error) error {
var raw string
err := unmarshal(&raw)
if err != nil {
return err
}

normalized := strings.ToLower(raw)
t, ok := strToTypeMap[normalized]
if !ok {
*typ = UnknownType
} else {
*typ = t
}

return nil
}

// MarshalYAML implements yaml.Marshaler interface for type.
func (typ DenyType) MarshalYAML() (interface{}, error) {
return typ.String(), nil
}

// String implements Stringer interface for type.
func (typ DenyType) String() string {
s, ok := typeToStrMap[typ]
if !ok {
return "UNKNOWN"
}
return s
}
Loading

0 comments on commit 82fd797

Please sign in to comment.