Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqlproxyccl: change denylist into an access control list #98153

Merged
merged 2 commits into from
Mar 16, 2023

Conversation

pjtatlow
Copy link
Contributor

@pjtatlow pjtatlow commented Mar 7, 2023

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

@pjtatlow pjtatlow requested review from a team as code owners March 7, 2023 17:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pjtatlow pjtatlow changed the title sqlproxyccl: change denylist into an access control list CC-8136: sqlproxyccl: change denylist into an access control list Mar 7, 2023
@pjtatlow pjtatlow changed the title CC-8136: sqlproxyccl: change denylist into an access control list [CC-8136] sqlproxyccl: change denylist into an access control list Mar 7, 2023
@pjtatlow pjtatlow force-pushed the sqlproxy-access-control-list branch 2 times, most recently from 205ecd1 to c78fd0c Compare March 7, 2023 20:06
@pjtatlow pjtatlow changed the title [CC-8136] sqlproxyccl: change denylist into an access control list sqlproxyccl: change denylist into an access control list Mar 7, 2023
@pjtatlow pjtatlow force-pushed the sqlproxy-access-control-list branch from c78fd0c to 865224d Compare March 7, 2023 21:05
pkg/ccl/sqlproxyccl/acl/file.go Outdated Show resolved Hide resolved
pkg/ccl/sqlproxyccl/proxy_handler.go Outdated Show resolved Hide resolved
pkg/ccl/sqlproxyccl/denylist/file_test.go Show resolved Hide resolved
pkg/ccl/sqlproxyccl/acl/watcher.go Outdated Show resolved Hide resolved
pkg/ccl/sqlproxyccl/acl/watcher.go Outdated Show resolved Hide resolved
pkg/ccl/sqlproxyccl/acl/watcher.go Outdated Show resolved Hide resolved
pkg/ccl/sqlproxyccl/acl/watcher.go Show resolved Hide resolved
@pjtatlow pjtatlow force-pushed the sqlproxy-access-control-list branch 3 times, most recently from 72a1fb5 to 202bb09 Compare March 9, 2023 17:10
@pjtatlow pjtatlow requested a review from jeffswenson March 9, 2023 17:16
@pjtatlow pjtatlow force-pushed the sqlproxy-access-control-list branch from 202bb09 to b73d22c Compare March 9, 2023 17:31
@pjtatlow pjtatlow force-pushed the sqlproxy-access-control-list branch 3 times, most recently from 2b23fa3 to ad0a74f Compare March 9, 2023 19:28
@pjtatlow pjtatlow requested a review from jaylim-crl March 9, 2023 21:17
Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! Once the race is fixed I'll approve the change.

pkg/ccl/sqlproxyccl/acl/access_control.go Outdated Show resolved Hide resolved
pkg/ccl/sqlproxyccl/acl/watcher.go Show resolved Hide resolved
@pjtatlow pjtatlow force-pushed the sqlproxy-access-control-list branch from ad0a74f to 4862c96 Compare March 16, 2023 15:35
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.

The sqlproxy will also fail to start if it begins with an invalid allow
or deny list, but if invalid files are written later then it increments
a new error metric so we can be alerted and take action to fix it.

Part of: https://cockroachlabs.atlassian.net/browse/CC-8136

Release note: None
@pjtatlow pjtatlow force-pushed the sqlproxy-access-control-list branch from 4862c96 to a20ff9b Compare March 16, 2023 15:40
Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pjtatlow
Copy link
Contributor Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 16, 2023

Build succeeded:

@craig craig bot merged commit 82fd797 into cockroachdb:master Mar 16, 2023
@pjtatlow pjtatlow deleted the sqlproxy-access-control-list branch March 16, 2023 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants