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

Implement HTTP proxy for primary redirection & consistency #271

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

benbjohnson
Copy link
Collaborator

@benbjohnson benbjohnson commented Feb 1, 2023

This pull request implements a thin HTTP proxy that can run in front of an application and automatically handle primary redirection & TXID consistency tracking. This works for applications that use read-only GET requests and only use a single SQLite database.

It implements the following:

  • For non-GET requests on a replica, a fly-replay is returned to automatically forward to the primary.
  • For non-GET requests on the primary, a __txid cookie is set with the current database TXID.
  • For GET requests on a replica, it will wait until the database catches up to the TXID stored in the cookie.

Fixes #269

Configuration

To enable the proxy, you need to set several fields in the proxy section of the config:

proxy:
  # Bind address for the proxy to listen on.
  # This should match your "services.internal_port" in your fly.toml
  addr: ":8080"

  # Hostport of your application. You'll need to change the port on your
  # application so Fly only talks to it through the proxy.
  target: "localhost:8081"

  # Filename of the SQLite database you want to use for TXID tracking.
  db: "my.db"

  # If true, this enables verbose debug logging for the proxy.
  debug: false

@benbjohnson
Copy link
Collaborator Author

@kentcdodds I have tests around the primary redirection and that works but I'm going to fire it up on a Fly.io test app and make sure the TXID consistency tracking is working on replicas. It's hard to test that locally since it happens really fast in tests.

This is still very much a work in progress! Please don't deploy to production! ;)

@benbjohnson benbjohnson marked this pull request as ready for review February 2, 2023 16:54
@benbjohnson benbjohnson merged commit ec7ccd5 into main Feb 2, 2023
@benbjohnson benbjohnson deleted the proxy branch February 2, 2023 17:14
@benbjohnson
Copy link
Collaborator Author

@kentcdodds I did some more testing on this proxy this morning and it seems to be working pretty well. I'm going to merge this in and if you have any issues then we can address those separately.

Also, note that if you upgrade to this PR then you'll get compression (#132) as well and you won't be able to easily rollback to a v0.3.0 version.


// No TXID or we couldn't parse it. Just send to the target.
if txid == 0 {
s.logf("proxy: %s %s: no client txid, proxying to target", r.Method, r.URL.Path)

Choose a reason for hiding this comment

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

Does this log show up all the time or can I turn it on/off? I have a feeling we'd see this a lot 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logf() messages only show up if you set proxy.debug. Otherwise they're no-ops.

Choose a reason for hiding this comment

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

Excellent. Thank you! I'm going to test this out today.

@kentcdodds
Copy link

This is great!

All that is good to know!

To be clear, I can have multiple sqlite databases, but I can only specify one of them as the one for txid tracking right? That's fine for my use cases, I just wanted to be sure.

This is a pretty elegant solution I think.

@benbjohnson
Copy link
Collaborator Author

Yes, that’s correct. Multiple databases is fine. I had thought about allowing a list of databases to use for TXID tracking but that seemed overly complicated.

@kentcdodds
Copy link

Worked in a simple case, now I'm trying it on my own site to see if it works for a complex case.

One thing I want to double check is that it doesn't override the set-cookie header that the target server already set.

if db := s.store.DB(s.DBName); db != nil {
pos := db.Pos()
s.logf("proxy: %s %s: setting txid cookie to %s", r.Method, r.URL.Path, ltx.FormatTXID(pos.TXID))
http.SetCookie(w, &http.Cookie{

Choose a reason for hiding this comment

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

Does this override an existing header on the response or does it append? We'll need it to append.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appends the cookie so it shouldn't affect application cookies (unless they're also named __txid): https://cs.opensource.google/go/go/+/refs/tags/go1.20:src/net/http/cookie.go;l=171

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to clarify, Header().Add() appends. If it overrode it then it would be Header().Set().

Choose a reason for hiding this comment

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

I don't see a Header().Add() or a Header().Set() with regard to this cookie 🤔 But if you say we're good then I'll believe you 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The http.SetCookie() calls w.Header().Add(). Sorry, I should have been more clear.

func SetCookie(w ResponseWriter, cookie *Cookie) {
	if v := cookie.String(); v != "" {
		w.Header().Add("Set-Cookie", v)
	}
}

@kentcdodds
Copy link

Should we ever delete the txid cookie? Seems reasonable to do that just in case they get to a high cookie and then we deploy a new instance of our app where the txid starts over. If we never delete the cookie then users would always get directed to primary until the new db caught up.

@benbjohnson
Copy link
Collaborator Author

Should we ever delete the txid cookie?

I added the expiration in case someone needed to revert their database snapshot to an older version. For example, if you revert to a snapshot from yesterday then clients that saw a TXID just before the reversion could be waiting 24 hours for enough writes to come through to catch up.

I arbitrarily chose 5 minutes since that's a decently long time for a node to be disconnected from the primary.

@kentcdodds
Copy link

That makes perfect sense 👍 Thanks!

@benbjohnson benbjohnson added this to the v0.4.0 milestone Mar 3, 2023
@ccll
Copy link

ccll commented Apr 7, 2023

Hello, the proxy feature is very promising, but I got some issue when trying it out with 30X redirections.

Currently the 'set-cookie' header seems not setting the 'Path=' attribute, so it defaults to the path segment in current URL.
Say first I issue a write request with POST /write/to/database, the response is a redirection to another read request URL GET /redirect/to/here, I can see that __txid cookie is correctly set in the first response, but it'll never send in the second request cause the URL path is different.

I'm not an expert on this, I think the proxy should set __txid cookie with Path=/ in this case, so any GET requests after the non-GET request will correctly send the cookie, right?

@benbjohnson
Copy link
Collaborator Author

@ccll Thanks for reporting that. Yes, it should be setting the cookie path. I added a fix on #304.

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.

Built-in Proxy
3 participants