Pull the lock-bearing code into a closure, and use a clone rather than a
shallow copy of the hostinfo record.
Updates #11649
Change-Id: I4f1d42c42ce45e493b204baae0d50b1cbf82b102
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
The early unlock on this branch was required because the "send" method goes on
to acquire the mutex itself. Rather than release the lock just to acquire it
again, call the underlying locked helper directly.
Updates #11649
Change-Id: I50d81864a00150fc41460b7486a9c65655f282f5
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
In places where we are locking the LocakBackend and immediately deferring an
unlock, and where there is no shortcut path in the control flow below the
deferral, we do not need the unlockOnce helper. Replace all these with use of
the lock directly.
Updates #11649
Change-Id: I3e6a7110dfc9ec6c1d38d2585c5367a0d4e76514
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
Instead of referring to groups, which is a term of art for a different entity,
update the doc comments to more accurately describe what tags are in reference
to the policy document.
Updates #cleanup
Change-Id: Iefff6f84981985f834bae7c6a6c34044f53f2ea2
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
There are several methods within the LocalBackend that used an unusual and
error-prone lock discipline whereby they require the caller to hold the backend
mutex on entry, but release it on the way out.
In #11650 we added some support code to make this pattern more visible.
Now it is time to eliminate the pattern (at least within this package).
This is intended to produce no semantic changes, though I am relying on
integration tests and careful inspection to achieve that.
To the extent possible I preserved the existing control flow. In a few places,
however, I replaced this with an unlock/lock closure. This means we will
sometimes reacquire a lock only to release it again one frame up the stack, but
these operations are not performance sensitive and the legibility gain seems
worthwhile.
We can probably also pull some of these out into separate methods, but I did
not do that here so as to avoid other variable scope changes that might be hard
to see. I would like to do some more cleanup separately.
As a follow-up, we could also remove the unlockOnce helper, but I did not do
that here either.
Updates #11649
Change-Id: I4c92d4536eca629cfcd6187528381c33f4d64e20
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
Some of the operations of the local API need an event bus to correctly
instantiate other components (notably including the portmapper).
This commit adds that, and as the parameter list is starting to get a bit long
and hard to read, I took the opportunity to move the arguments to a config
type. Only a few call sites needed to be updated and this API is not intended
for general use, so I did not bother to stage the change.
Updates #15160
Updates #16842
Change-Id: I7b057d71161bd859f5acb96e2f878a34c85be0ef
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
This affects the 1.87.33 unstable release.
Updates #16842
Updates #15160
Change-Id: Ie6d1b2c094d1a6059fbd1023760567900f06e0ad
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
In the components where an event bus is already plumbed through, remove the
exceptions that allow it to be omitted, and update all the tests that relied on
those workarounds execute properly.
This change applies only to the places where we're already using the bus; it
does not enforce the existence of a bus in other components (yet),
Updates #15160
Change-Id: Iebb92243caba82b5eb420c49fc3e089a77454f65
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
Ideally when we attempt to create a new port mapping, we should not return
without error when no mapping is available. We already log these cases as
unexpected, so this change is just to avoiding panicking dispatch on the
invalid result in those cases. We still separately need to fix the underlying
control flow.
Updates #16662
Change-Id: I51e8a116b922b49eda45e31cd27f6b89dd51abc8
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
This is a follow-up to #15351, which fixed the test for Linux but not for
Darwin, which stores its "true" executable in /usr/bin instead of /bin.
Try both paths when not running on Windows.
In addition, disable CGo in the integration test build, which was causing the
linker to fail. These tests do not need CGo, and it appears we had some version
skew with the base image on the runners.
In addition, in error cases the recover step of the permissions check was
spuriously panicking and masking the "real" failure reason. Don't do that check
when a command was not produced.
Updates #15350
Change-Id: Icd91517f45c90f7554310ebf1c888cdfd109f43a
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
This means the caller does not have to remember to close the reader, and avoids
having to duplicate the logic to decode JSON into events.
Updates #15160
Change-Id: I20186fabb02f72522f61d5908c4cc80b86b8936b
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
As note in the comment, it now being more than six months since this was
deprecated and there being no (further) uses of the old pattern in our internal
services, let's drop the migrator.
Updates #cleanup
Change-Id: Ie4fb9518b2ca04a9b361e09c51cbbacf1e2633a8
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
When an event bus is plumbed in, use it to subscribe and react to port mapping
updates instead of using the client's callback mechanism. For now, the callback
remains available as a fallback when an event bus is not provided.
Updates #15160
Change-Id: I026adca44bf6187692ee87ae8ec02641c12f7774
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
When an event bus is configured publish an event each time a new port mapping
is updated. Publication is unconditional and occurs prior to calling any
callback that is registered. For now, the callback is still fired in a separate
goroutine as before -- later, those callbacks should become subscriptions to
the published event.
For now, the event type is defined as a new type here in the package. We will
want to move it to a more central package when there are subscribers. The event
wrapper is effectively a subset of the data exported by the internal mapping
interface, but on a concrete struct so the bus plumbing can inspect it.
Updates #15160
Change-Id: I951f212429ac791223af8d75b6eb39a0d2a0053a
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
In preparation for adding more parameters (and later, moving some away), rework
the portmapper constructor to accept its arguments on a Config struct rather
than positionally.
This is a breaking change to the function signature, but one that is very easy
to update, and a search of GitHub reveals only six instances of usage outside
clones and forks of Tailscale itself, that are not direct copies of the code
fixed up here.
While we could stub in another constructor, I think it is safe to let those
folks do the update in-place, since their usage is already affected by other
changes we can't test for anyway.
Updates #15160
Change-Id: I9f8a5e12b38885074c98894b7376039261b43f43
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
Although, at the moment, we do not yet require an event bus to be present, as
we start to add more pieces we will want to ensure it is always available. Add
a new constructor and replace existing uses of new(tsd.System) throughout.
Update generated files for import changes.
Updates #15160
Change-Id: Ie5460985571ade87b8eac8b416948c7f49f0f64b
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
Shovel small events through the pipeine as fast as possible in a few basic
configurations, to establish some baseline performance numbers.
Updates #15160
Change-Id: I1dcbbd1109abb7b93aa4dcb70da57f183eb0e60e
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
The read of the synced field for logging takes place outside the lock, and
races with other (locked) writes of this field, including for example the one
at current line 556 in mapRoutine.
Updates tailscale/corp#13856
Change-Id: I056b36d7a93025aafdf73528dd7645f10b791af6
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
The main motivation for this change is to stop using the deprecated
set-output function which triggers deprecation warnings in the action.
Change-Id: I80496c44ea1166b9c40d5cd9e450129778ad4aaf
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
This package handles cases where we need to truncate human-readable text to fit
a length constraint without leaving "ragged" multi-byte rune fragments at the
end of the truncated value.
Change-Id: Id972135d1880485f41b1fedfb65c2b8cc012d416
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
