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

make remote host a real url #16

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CosmicToast
Copy link

this means you can do things like prefix-based reverse-proxying and reverse-proxying in general, actually
it also knows about proxying to subfolders and thus uses a url-base, meaning you can do something like kvass config remote https://my.normal.website/secret/kvass/

example setup with caddy
server: kvass --bind=localhost:8081 (you can also bind to 0.0.0.0 if the reverse proxy is off-system)
proxy:

my.domain {
	// ... the rest of the domain
	handle_path /secret/prefix/kvass/* {
		reverse_proxy http://localhost:8081 # match to above
	}
}

clients: kvass config key ... && kvass config remote https://my.domain/secret/prefix/kvass/

Note that the trailing / is important.

this means you can do things like prefix-based reverse-proxying
and reverse-proxying in general, actually
@maxmunzel
Copy link
Owner

maxmunzel commented Nov 27, 2023

Hi Cosmic!

Thanks for your work, making kvass work properly with reverse proxies was on the todo list for (too) long and I'm happy you made it happen :)

I'm too tired for a proper review today, but will look at it tomorrow. Some quick questions in the meantime:

  1. How does the migration look for existing users? I thinks it's important that current users can upgrade without having to change the config. I'd image NewSqlitePersistance() checks/updates the SchemaVersion field and take care of it.
  2. There are some places with // this should never fail comments. Could you add if err != nil {panic(err)} checks around them to be sure? Feel free to pack them into a convenience function if you find the explicit version to be ugly to read.
  3. Are there cases, where we would not want a trailing /? If not, lets add a warning or even an automatic conversion so users avoid the foot gun.

unsetting the remote is much more trivial now, for instance
@CosmicToast
Copy link
Author

re: migrations, I don't typically work with/on databases (I put points into shell instead), so I'm not entirely sure what that would look like
the rest is all done, as well as slightly improved URL parsing

@maxmunzel
Copy link
Owner

maxmunzel commented Nov 28, 2023

Thanks for the quick adjustments. The migration would actually have little db logic in it and would be at the end of NewSqlitePersistance(). Basically:

  1. Check if state.SchemaVersion == 1
  2. do some magic to make a RemoteURL from a RemoteHostname
  3. increment state.SchemaVersion
  4. write the new state struct to the db

There's already some code for the last schema migration, but this one is even simpler.
If you want, you can just give me a snippet for step 2 and I'll do the reset.

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.

2 participants