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.

Sign up for GitHub

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

Jump to bottom

release-22.2: Backport SQL Proxy Access Control List #99830

Closed
wants to merge 4 commits into from
Closed

release-22.2: Backport SQL Proxy Access Control List #99830

wants to merge 4 commits into from

Conversation

pjtatlow
Copy link
Contributor

Backport:

Please see individual PRs for details.

/cc @cockroachdb/release

PJ Tatlow added 4 commits March 28, 2023 11:01
sqlproxyccl: move denylist files to acl package
3f49adb 
Part of: https://cockroachlabs.atlassian.net/browse/CC-8136

Release note: None
sqlproxyccl: change denylist into an access control list
610c758 
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
ccl/sqlproxyccl/acl: fix TestParsingErrorHandling flake
ef0583c 
TestParsingErrorHandling asserts that the error count metric is updated
correctly when reading a file fails or succeeds, and the files are checked
at a regular interval. For the tests that interval is set to 100ms, and we waited
200ms to ensure the metric would be updated, but that seems to not be reliable.
This change increases the wait to 500ms which should ensure the file is
re-read before we check the value of the error metics.
sqlproxyccl/acl: fix the TestParsingErrorHandling flake
5d8ff12 
This is attempt number two to fix this test flake. This time, rather
than waiting a fixed amount of time before checking the metric, we use
testutils.SucceedsSoon to recheck the metric until it's the value we expect.

Release note: None
@pjtatlow pjtatlow requested review from a team as code owners March 28, 2023 17:21
@blathers-crl
Copy link

blathers-crl bot commented Mar 28, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pjtatlow pjtatlow closed this Mar 29, 2023
@pjtatlow pjtatlow deleted the backport22.2-98153-99050-99385 branch March 29, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pjtatlow @cockroach-teamcity