At a high level, this is what I've done and why:
- I'm moving the protobuf generation for the `protocol`, `discovery` and
`db` packages to the modern alternatives, and using `buf` to generate
because it's nice and simple.
- After trying various approaches on how to integrate the new types with
the existing code, I opted for splitting off our own data model types
from the on-the-wire generated types. This means we can have a
`FileInfo` type with nicer ergonomics and lots of methods, while the
protobuf generated type stays clean and close to the wire protocol. It
does mean copying between the two when required, which certainly adds a
small amount of inefficiency. If we want to walk this back in the future
and use the raw generated type throughout, that's possible, this however
makes the refactor smaller (!) as it doesn't change everything about the
type for everyone at the same time.
- I have simply removed in cold blood a significant number of old
database migrations. These depended on previous generations of generated
messages of various kinds and were annoying to support in the new
fashion. The oldest supported database version now is the one from
Syncthing 1.9.0 from Sep 7, 2020.
- I changed config structs to be regular manually defined structs.
For the sake of discussion, some things I tried that turned out not to
work...
### Embedding / wrapping
Embedding the protobuf generated structs in our existing types as a data
container and keeping our methods and stuff:
```
package protocol
type FileInfo struct {
*generated.FileInfo
}
```
This generates a lot of problems because the internal shape of the
generated struct is quite different (different names, different types,
more pointers), because initializing it doesn't work like you'd expect
(i.e., you end up with an embedded nil pointer and a panic), and because
the types of child types don't get wrapped. That is, even if we also
have a similar wrapper around a `Vector`, that's not the type you get
when accessing `someFileInfo.Version`, you get the `*generated.Vector`
that doesn't have methods, etc.
### Aliasing
```
package protocol
type FileInfo = generated.FileInfo
```
Doesn't help because you can't attach methods to it, plus all the above.
### Generating the types into the target package like we do now and
attaching methods
This fails because of the different shape of the generated type (as in
the embedding case above) plus the generated struct already has a bunch
of methods that we can't necessarily override properly (like `String()`
and a bunch of getters).
### Methods to functions
I considered just moving all the methods we attach to functions in a
specific package, so that for example
```
package protocol
func (f FileInfo) Equal(other FileInfo) bool
```
would become
```
package fileinfos
func Equal(a, b *generated.FileInfo) bool
```
and this would mostly work, but becomes quite verbose and cumbersome,
and somewhat limits discoverability (you can't see what methods are
available on the type in auto completions, etc). In the end I did this
in some cases, like in the database layer where a lot of things like
`func (fv *FileVersion) IsEmpty() bool` becomes `func fvIsEmpty(fv
*generated.FileVersion)` because they were anyway just internal methods.
Fixes#8247
I came accross this in another context and didn't investigate fully, but
literally ten lines above this code, in another method, we say that
filesets _must_ be created under the lock. It's either one or the other
and I'm taking the safer route here.
---------
Co-authored-by: Simon Frei <freisim93@gmail.com>
Encrypted-to-encrypted connections (i.e., ones where both sides set a
password) used to work but were broken in the 1.28.0 release. The
culprit is the 5342bec1b refactor which slightly changed how the request
was constructed, resulting in a bad block hash field.
Co-authored-by: Simon Frei <freisim93@gmail.com>
This adds `allow_contributor: true` which allows approvals by
contributors to the PR (but still not the author themself, which is a
different thing). This allows things like pushing minor fixups while
also approving.
The `ignore_update_merges: true` option makes it so that someone is not
considered a "contributor" just because they push the merge button to
update the branch. In principle this is not needed given the above, but
I like it for clarity.
Skipping these makes the sequence numbering inconcistent; we've received
a file and suppsedly added it to the database, but if you check the
sequence number afterwards it didn't increase, i.e., we trigger [this
failure
condition](https://github.com/syncthing/syncthing/blob/47f48faed7331b7ba4ad3d6775d5cffacf8931b5/lib/model/indexhandler.go#L447-L459)
and, similarly, a future update will look like there was a hole in the
numbering.
I propose to at least temporarily remove this optimisation in order for
things to make more sense. Is there a reason to keep this beyond saving
some database operations?
There was a bug that the unique ID was not set when reporting was
enabled, and thus the reports where rejected by the server. The unique
ID got set only on startup, so next time Syncthing restarted.
This makes sure to set the unique ID when blank.
I can see already in our Sentry data that there are a fair amount of
these warnings, and mostly the shape of it. Asking users to report them
will likely cause a lot of reporting effort to fairly little additional
value. We can do that when/if we have something more targeted to ask
for.
This codifies a review policy which is closer to what I always
envisioned, but which isn't expressible using the normal checks in the
GitHub GUI. It would move the commit approval check from GitHub into the
policy-bot check which is already present to enforce the
conventional-commits standard. Approvals in general would still work the
same -- it's just that the bot picks it up and toggles the status
accordingly. From a GitHub side when this is enabled we'd remove the
requires-review check from there and let the bot decide that part. We
would still require builds and tests to pass of course.
There are a couple of relexations from the current policy, details in
the code but briefly:
- Changes to translations or dependencies by a trusted person don't
require review
- Trivial changes by a trusted person, explicitly marked as such, don't
require review
This enables less bureaucracy for things like adding new translated
languages and updating dependencies, and enables the trivial-change
workflow to a larger audience than, like, me, who could always just
bypass the rules by way of being admin.
Move infrastructure related commands to under `cmd/infra` and
development stuff to `cmd/dev`. The default build command builds the
regular user facing binaries: syncthing, stdiscosrv, and strelaysrv.
* infrastructure:
chore(stdiscosrv): ensure incoming addresses are sorted and unique
chore(stdiscosrv): use zero-allocation merge in the common case
chore(stdiscosrv): properly clean out old addresses from memory
chore(stdiscosrv): calculate IPv6 GUA
This is to add the generation of `compat.json` as a release artifact. It
describes the runtime requirements of the release in question. The next
step is to have the upgrade server use this information to filter
releases provided to clients. This is per the discussion in #9656
---------
Co-authored-by: Ross Smith II <ross@smithii.com>
This changes the two remaining instances where we use insecure HTTPS to
use standard HTTPS certificate verification.
When we introduced these things, almost a decade ago, HTTPS certificates
were expensive and annoying to get, much of the web was still HTTP, and
many devices seemed to not have up-to-date CA bundles.
Nowadays _all_ of the web is HTTPS and I'm skeptical that any device can
work well without understanding LetsEncrypt certificates in particular.
Our current discovery servers use hardcoded certificates which has
several issues:
- Not great for security if it leaks as there is no way to rotate it
- Not great for infrastructure flexibility as we can't use many load
balancer or TLS termination services
- The certificate is a very oddball ECDSA-SHA384 type certificate which
has higher CPU cost than a more regular certificate, which has real
effects on our infrastructure
Using normal TLS certificates here improves these things.
I expect there will be some very few devices out there for which this
doesn't work. For the foreseeable future they can simply change the
config to use the old URLs and parameters -- it'll be years before we
can retire those entirely.
For the upgrade client this simply seems like better hygiene. While our
releases are signed anyway, protecting the metadata exchange is _better_
and, again, I doubt many clients will fail this today.
This adds a header with the operating system version, verbatim in
whatever format the operating system reports it, to the upgrade check.
The intention is that the upgrade server can use this information to
filter out (or maybe just mark) potentially unsupported upgrades.
This adds a header with the operating system version, verbatim in
whatever format the operating system reports it, to the upgrade check.
The intention is that the upgrade server can use this information to
filter out (or maybe just mark) potentially unsupported upgrades.
### Purpose
Avoid the issue where the folder marker is deleted by overzealous
cleanup tools because it's just a useless, empty directory.
We create a small file containing a an admonishment to not delete the
directory, and some metadata that is just for human consumption at the
moment. (But it would parse as a valid yaml file if we wanted to read
this, at some point.)
This will only apply when _creating_ a folder marker, that is, existing
setups will not gain the file automatically. Obviously, when using a
custom folder marker none of this applies.
Also, slightly adjust the permission bits for the folder marker directory and file on Unixes, making sure the group & write bits are unset.
### Testing
I've created and deleted a few folders and it appears to behave as I
expect.
### Screenshots
```
jb@ok:~/somefolder % ls -la
total 0
drwxr-xr-x 3 jb staff 96 May 1 08:52 ./
drwx------ 12 jb staff 384 May 1 08:52 ../
drwxr-xr-x 3 jb staff 96 May 1 08:52 .stfolder/
jb@ok:~/somefolder % ls -l .stfolder
total 8
-rw-r--r-- 1 jb staff 122 May 1 08:52 syncthing-folder-39a4b0.txt
jb@ok:~/somefolder % cat .stfolder/syncthing-folder-39a4b0.txt
# This directory is a Syncthing folder marker.
# Do not delete.
folderID: xtdca-cudyf
created: 2024-05-01T08:52:49+02:00
```
Currently the maximum delay is always derived automatically from the
initial delay. This is fine in most cases, but for some use cases (large
files that take a long time to write) we need to be able to set a longer
max delay than the computed value (e.g., 15s delay with 10min timeout).
This adds a small package `geoip` which knows how to download and manage
the Maxmind GeoLite2 database we use. This removes the need for various
scripts to download and manage the geoip database, something that today
happens on Docker startup for the relay pool server and using various
hand written hacks for the usage reporting server.
The database is downloaded when needed and then refreshed on a
best-effort basis weekly.
### Purpose
Resend our indexes since we fixed that index-sending issue.
I made a new thing to only drop the non-local-device index IDs, i.e.,
those for other devices. This means we will see a mismatch and resend
all indexes, but they will not. This is somewhat cleaner as it avoids
resending everything twice when two devices are upgraded, and in any
case, we have no reason to force a resend of incoming indexes here.
### Testing
It happens on my computer...
The changes to go.mod in latest Go 1.21/1.22 are not fully understood by
older Go that might be pre-installed on builds, so make sure we always
have a modern one in place even for running small release scripts etc.
Somewhere along the way, the non-parallel test became parallel, and at
that point, timeouts occurred. Parallel is better, so increase the
timeout on the offending call a bit...
The new test has a flakiness factor on slow platforms, where the close
on the sending connection races with the last index message, potentially
messing up the count. This adds a wait to ensure that all sent messages
are received, or the test will eventually fail with a timeout.
This makes a couple of small improvements to the folder summary
mechanism:
- The folder summary includes the local and remote sequence numbers in
clear text, rather than some odd sum that I'm not sure what it was
intended to represent.
- The folder summary event is generated when appropriate, regardless of
whether there is an event listener. We did this before because
generating it was expensive, and we wanted to avoid doing it
unnecessarily. Nowadays, however, it's mostly just reading out
pre-calculated metadata, and anyway, it's nice if it shows up reliably
when running with -verbose.
The point of all this is to make it easier to use these events to judge
when devices are, in fact, in sync. As-is, if I'm looking at two
devices, it's very difficult to reliably determine if they are in sync
or not. The reason is that while we can ask device A if it thinks it's
in sync, we can't see if the answer is "yes" because it has processed
all changes from B, or if it just doesn't know about the changes from B
yet. With proper sequence numbers in the event we can compare the two
and determine the truth. This makes testing a lot easier.
This is a refactor of the protocol/model interface to take the actual
message as the parameter, instead of the broken-out fields:
```diff
type Model interface {
// An index was received from the peer device
- Index(conn Connection, folder string, files []FileInfo) error
+ Index(conn Connection, idx *Index) error
// An index update was received from the peer device
- IndexUpdate(conn Connection, folder string, files []FileInfo) error
+ IndexUpdate(conn Connection, idxUp *IndexUpdate) error
// A request was made by the peer device
- Request(conn Connection, folder, name string, blockNo, size int32, offset int64, hash []byte, weakHash uint32, fromTemporary bool) (RequestResponse, error)
+ Request(conn Connection, req *Request) (RequestResponse, error)
// A cluster configuration message was received
- ClusterConfig(conn Connection, config ClusterConfig) error
+ ClusterConfig(conn Connection, config *ClusterConfig) error
// The peer device closed the connection or an error occurred
Closed(conn Connection, err error)
// The peer device sent progress updates for the files it is currently downloading
- DownloadProgress(conn Connection, folder string, updates []FileDownloadProgressUpdate) error
+ DownloadProgress(conn Connection, p *DownloadProgress) error
}
```
(and changing the `ClusterConfig` to `*ClusterConfig` for symmetry;
we'll be forced to use all pointers everywhere at some point anyway...)
The reason for this is that I have another thing cooking which is a
small troubleshooting change to check index consistency during transfer.
This required adding a field or two to the index/indexupdate messages,
and plumbing the extra parameters in umpteen changes is almost as big a
diff as this is. I figured let's do it once and avoid having to do that
in the future again...
The rest of the diff falls out of the change above, much of it being in
test code where we run these methods manually...
Jakob Borg
