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

[WIP] [IGNORE] proxy: conrad is messing around #6283

Closed
wants to merge 1 commit into from
Closed

Conversation

conradludgate
Copy link
Contributor

@conradludgate conradludgate commented Jan 7, 2024

Problem

Error handling has always been a pain in the ass (#5599). I think much of that is due to errors being so deep in the stack.

Additionally, the full handle_client future is 8KiB, which seems wasteful once we get to proxy_pass.

This has been an idea I wanted to do for ages now, this weekend I got the courage and motivation to try it out.

Summary of changes

Refactor proxy to use a state machine paradigm. Proxy processes requests in a very linear way, so these state machines are straightforward.

The main benefit here is that each stage in the state machine is only 1 step away from the error handling for that session. There are no complicated error chains to worry about. All errors are now ReportableErrors. These have a direct blame associated with them (as introduced in my RequestMonitoring PR).

Current state machine flows:

---
title: Standard Proxy
---
stateDiagram-v2
    [*] --> NeedsHandshake
    NeedsHandshake --> NeedsAuthentication

    NeedsAuthentication --> NeedsPasswordHack: if no SNI or endpoint option
    NeedsPasswordHack --> NeedsAuthSecret

    NeedsAuthentication --> NeedsAuthSecret
    NeedsAuthSecret --> NeedsWakeCompute
    NeedsWakeCompute --> NeedsCompute
    NeedsCompute --> ProxyPass
    ProxyPass --> [*]
Loading
---
title: Passwordless Proxy
---
stateDiagram-v2
    [*] --> NeedsHandshake
    NeedsHandshake --> NeedsAuthentication
    NeedsAuthentication --> NeedsLinkAuthentication
    NeedsLinkAuthentication --> NeedsLinkAuthenticationResponse
    NeedsLinkAuthenticationResponse --> NeedsCompute
    NeedsCompute --> ProxyPass
    ProxyPass --> [*]
Loading

The core of this setup is in state_machine.rs with the Stage trait. Stage essentially describes Stage -> Option<Result<Stage>> as a step in the state machine.

In this setup, the largest future (NeedsCompute) is 904 bytes, whereas ProxyPass, the final stage, is only 224 bytes. This means that 10000 active database connections go down from 85.4MiB to 2.13MiB (this is not counting additional allocations each connection might hold)

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

Copy link

github-actions bot commented Jan 7, 2024

2280 tests run: 2189 passed, 0 failed, 91 skipped (full report)


Flaky tests (1)

Postgres 16

  • test_statvfs_pressure_usage: debug

Code coverage (full report)

  • functions: 54.5% (10663 of 19562 functions)
  • lines: 81.5% (61118 of 74983 lines)

The comment gets automatically updated with the latest test results
7fed0ba at 2024-01-23T14:09:38.196Z :recycle:

Base automatically changed from request-context to main January 8, 2024 11:42
@conradludgate conradludgate force-pushed the actorsssss branch 2 times, most recently from 37635a3 to a10b382 Compare January 12, 2024 09:26
@conradludgate conradludgate mentioned this pull request Jan 19, 2024
5 tasks
@conradludgate conradludgate changed the base branch from main to proxy-provider-tweaks January 19, 2024 11:16
Base automatically changed from proxy-provider-tweaks to main January 21, 2024 08:58
conradludgate added a commit that referenced this pull request Jan 21, 2024
## Problem

In #6283 I did a couple changes
that weren't directly related to the goal of extracting the state
machine, so I'm putting them here

## Summary of changes

- move postgres vs console provider into another enum
- reduce error cases for link auth
- slightly refactor link flow
This was referenced Jan 23, 2024
abhigets pushed a commit that referenced this pull request Jan 24, 2024
## Problem

In #6283 I did a couple changes
that weren't directly related to the goal of extracting the state
machine, so I'm putting them here

## Summary of changes

- move postgres vs console provider into another enum
- reduce error cases for link auth
- slightly refactor link flow
conradludgate added a commit that referenced this pull request Jan 29, 2024
## Problem

Taking my ideas from #6283 and
doing a bit less radical changes. smaller commits.

Proxy flow was quite deeply nested, which makes adding more interesting
error handling quite tricky.

## Summary of changes

I recommend reviewing commit by commit.

1. move handshake logic into a separate file
2. move passthrough logic into a separate file
3. no longer accept a closure in CancelMap session logic
4. Remove connect_to_db, copy logic into handle_client
5. flatten auth_and_wake_compute in authenticate
6. record info for link auth
conradludgate added a commit that referenced this pull request Feb 9, 2024
## Problem

Taking my ideas from #6283 and
doing a bit less radical changes. smaller commits.

We currently don't report error classifications in proxy as the current
error handling made it hard to do so.

## Summary of changes

1. Add a `ReportableError` trait that all errors will implement. This
provides the error classification functionality.
2. Handle Client requests a strongly typed error
    * this error is a `ReportableError` and is logged appropriately
3. The handle client error only has a few possible error types, to
account for the fact that at this point errors should be returned to the
user.
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.

1 participant