We switch everything to use QueryContext/QueryRead/etc instead
of Qcx/Tx.
We drop the short_txkey subpackage (it's now handled by either
keys or querycontext).
We drop all the dbshard stuff, and all the tx/txfactory stuff.
We remove all the things that related to the old "Block" concept,
which was mostly used by the anti-entropy code, but had one
fragmentary usage left in the ImportRoaringOverwrite case of
ImportRoaring. That's replaced by using a rewriter that deletes
all bits (not just bits in specific columns) from an existing
thing, but writes in new bits. Actually we could probably do that
better with a custom "eradicate-rewriter" that doesn't try to
be clever, and just eliminates things.
This includes a number of minor bug fixes that were
exposed by getting the testing to work. For example:
* When checking whether an operation "requires write", we
now consider a Delete a kind of a Write, because it is.
* Several tests were relying on the fact that writes through
Qcx were being committed whether or not the Qcx was ever
told to finish. With QueryContext, you actually have to
reach a Commit() or the writes don't happen (except for
special cases in Delete).
* Replaced a lot of panics with t.Fatalf in tests.
There's also some minor staticcheck fixes, like deleting the
unused "db" member of a boltdb transaction wrapper.
implements an fb_exec_requests system table. The purpose of this table is to allow access to internal state to see what queries are running and have been run.
Co-authored-by: Travis Turner <travis@molecula.com>
FB-1771
In api_test.go, it was being used to make sure that incorrect input
produced the right errors; this is now handled by making sure the
error isn't nil and then checking its string against the expected
error string.
In executor_test.go and internal_client_test.go, it was being used
to compare QueryResponse structures, which contain an error.
handler.go now has a function specifically for comparing them,
which can provide additional detail if necessary.
Added a test for SameAs to handler_test.go.
This fixes the OriginalIP and RequestUserID in the main featurebase
package, and the Access and Refresh tokens, the UserInfo, and the
[]string of Indexes passed with context.Context(s) in the authn package.
An empty struct was used for all of these keys (and relevant helper
functions we added) to avoid allocations where possible while still
using the context functionality.
Some of the logic in the server.GetIndexes function was fixed.
The anti-entropy feature has never actually worked. We've been
talking about removing it or replacing it for ages, but haven't
had a concrete motivation.
But the anti-entropy interface is the sole user of several components
of the Tx interface, and now that we're trying to replace that
interface, being able to drop those components has some appeal, so
let's remove the one thing that used them, in the hopes that this
will simplify life.
This also lets us drop ForEach and ForEachRange, which were
barely used at all. The one surviving usage (CSV export) can be
handled by using the container iterator we already have, and
making ContainerCallback exported so we can use it to just call
things for every bit.
enriched metadata for tables
added support for the concept of a table and field owners in metadata; mechanism to derive owner from http request metadata; metadata for table description
When doing tests, we create a ton of one-off clusters. This
turns out to be expensive and slow. Fixing it is surprisingly hard.
Fundamentally: If we're sharing clusters, we need to use different
indexes for each test, to avoid clashes. This changes index names.
As a side-effect, this reorders many partition-based things, like
the order keys are returned in. Thus, to fix this, we change a lot
of tests to no longer depend on the *order* in which strings are
returned.
Having done that, we can also discard the ModHasher behavior, since
that only existed to allow us to reliably predict partitioning.
The basic design is as follows: Instead of a cluster being a
[]*Command, a "shareable" cluster is now a []*Command plus some
flags, and a "cluster" is a pointer to a possibly-shared cluster,
plus a link to the specific test using this specific cluster,
and correspondingly, its test name suitably coerced to be a valid
index name prefix.
The "test.Cluster" object now has methods to allow retrieving an
index name, and also implemnts fmt.Formatter to let you use,
e.g., `%i` with it in Sprintf to get "the index name, plus an i".
(This works for everything but %p and %T.)
This allows us to consistently rework all the many things that
use index names in a persistent way.
We also have `MustUnshared` and `MustRunUnsharedCluster` methods
which allow us to specify that a given test needs its own cluster
for some reason. For instance, the tests that want to run backups
need their own isolated cluster, and the tests that want to close
or reopen nodes need their own cluster because a reopened cluster
won't have working GRPC for some reason.
On "closing" a shared cluster (actually the test-specific wrapper
that reflects a given sharing), we delete any indexes starting with
that test's index name prefix. Otherwise, the huge pile of open
indexes prevents `go test -race` from working on MacOS, where we
run out of address space too quickly.
This is fairly enormous but most of the individual changes are
fairly trivial things like replacing the string "i" with "c.Idx()".
We also tweaked a test that failed for me a couple of times to
not depend on sort order.
We make clients pass in a context which has an associated
data structure which can be used to annotate errors we've encountered
inside retryablehttp. You might ask why we do this instead of
just giving retryablehttp a logger that isn't nopLogger; the answer
is that retryablehttp's logging is too spammy to use.
So we create a context, and then pass that in for requests, so that
when we check to see whether we should be retrying, we can log
the errors we encounter, so if we encounter errors we can log
something specific instead of "gave up after 5 retries".
We also make a second retryable client that would forward
authentication, and reuse that, rather than making a new client
for every call that needs to forward authentication. This allows
us to avoid the problem where that inner client wasn't picking
up the timeout settings we'd applied to the parent.
We log errors with Errorf if we actually failed entirely to do the
request, we log them with Infof if there's errors but the request
ultimately succeeds. We also try to log part of the response body
when there is one, but there wasn't an error.
If there's no response, we produce a spurious 500 and a message
saying that we produced it.
compares free space in output directory to
the usage of either the data directory or
index depending on what is being backed up.
- adds an http_handler endpoint to get usage
of a particular index
- adds InternalClient methods to get DiskUsage and
IndexUsage
The root problem this is attempting to address is sporadic
weird cases in which etcd mistakenly thinks it's down even when
it's up. I am not confident that this is addressed, but there's
a reasonable chance that it is, and I can't trigger it at the
moment, but it was always sporadic, so that doesn't prove much.
There's a lot going on here, and it comes into roughly three
categories.
First: Dropping unused/unneeded code. There's a lot of leftover
bits from the initial development and refactoring of this.
Second: Unifying and shuffling some of the design. We had
multiple interfaces which are functionally impossible to
usefully implement separately, so they're combined together,
and in some cases, moved.
Third: Streamlining logic and simplifying design choices.
This is combined into one commit because the changes are
thoroughly entertwined with each other and you can't usefully
break most of them out.
Also, a bunch of test coverage for most of these changes.
Big changes:
We merge the topology and disco packages. The topology and disco
packages being separate creates a complicated tangle of problems
and dependencies. The fundamental problem, approximately, is that
topology.Node has to track disco.NodeState.
There's three core interfaces interacting here:
topology.Noder (maintains list of nodes)
disco.Stator (maintains the state of a node)
disco.Metadator (stores, possibly retrieves, node metadata)
But the node state mantained by the Noder *is* the set of node
metadata, plus state updates produced by Stators. The only actual
non-trivial and usable implementation of these interfaces is a single
thing which implements all three, and in which the implementations
share a single backend data source which they are all modifying.
But you can't move Noder into disco, because Noder has to refer
to topology.Node, but topology.Node refers to disco.
Solution: First, merge these two packages. Second, merge these
three interfaces, to provide a single interface which is more
clear about the fact that (metadator.)SetMetadata() and
(stator.)Started() are both changing the output we'll get from
(noder.)Nodes().
We rework the node state tracking.
We have this nodeStates map which is almost unused. Really, we
don't need it at all. Every node's state is either its last heartbeat
state or "Unknown", so we simplify this a bit. Also, we ensure that
the populateNodeStates function itself is yielding the sorted nodes
list, so we don't have to be as worried about possible later lookups
of sortedNodes happening outside a lock. We also add diagnostics
for deleting nodes from the metadata list (this should never happen),
and try to track heartbeat state more closely.
This is *probably* what fixes the underlying reported problem,
if anything did.
Still an open issue: Make heartbeat state changes aware of when
they're talking about *this* node and possibly not try to
mark it down? Except this may have a flaw: That would result in
each node disagreeing with other nodes in etcd about the state
of that node in the failure cases, and undermine the point of
using etcd to keep these states consistent.
We reduce the number of contexts and cancelfuncs in the etcd wrapper.
We create a shared context for the non-etcd.embed children of our
etcd wrapper, the heartbeat/keepalive and the node watcher, so we
can cancel that one context and cancel all of those at once, so
we don't need to separately track a function to call to cancel
the watch, AND be closing another channel. Also, our shutdown
now propagates automatically to the various etcd API calls we've
made for things like the node watcher and keepalive calls.
We still need to watch that channel in watchNodesOnce, though,
because apparently the watch doesn't yield an error even if the
context calling it is canceled. Whee.
This should reduce the risk of ending up in an inconsistent state,
and also the Close() function is probably idempotent now.
Smaller changes:
* Remove config-generators that existed to generate etcd
configs but were used only for tests that no longer exist
or make sense.
* Move the logic to generate etcd configs into the etcd
package, instead of the "testing" subpackage. This allows
us to write a self-contained config generator for
clusters where the nodes know about each other, but do
this just with etcd, not with full featurebase servers.
* Move the thing generating `fake:%d` socket names into
the etcd package, which is the only place we use it.
Also simplify it slightly.
* Don't panic on invalid URLs, report errors from them.
* At least try to use etcd's config.Validate functionality.
It's underdocumented, so we're not sure what it will report,
but at least if it does we'll get reports from it and
know what they are?
* Try to handle CompactRevision errors from watches more
correctly -- after a CompactRevision, any future attempt
to watch from a lower revision will necessarily fail, so
we adjust our target revision up. We don't have good
testing for this.
* Drop the Metadata() method (that used to be in Metadator)
because nothing ever used it and it didn't make much sense
to try.
* Convert SetMetadata from taking an arbitrary json blob
to taking the only data that would ever be valid since
we always use it to extract node information anyway.
* Drop several unused functions, unexport things only used
internally.
* Replace Started() with SetState("STARTED"), allowing us
to write tests that mess with states. We weren't really thinking
carefully about state transitions sometimes and now it's much
easier to do that thinking.
* Stop leaving stray localhost:2380 and localhost:2379 in
our embed config. We still sometimes see peer requests from
those and I honestly don't know why, but at least it should
be rarer.
* Add refresh token header/cookie
As part of work on automatic refreshing of access tokens in the grafana plugin
(FB-1377), we will now accept a refresh token in the "X-Molecula-Refresh-Token"
header or the "refresh-molecula-chip" cookie.
This refresh token will be used if the access token is expired. To achieve this,
there was a lot of plumbing that had to be done. Here is a list of some of it:
* Added lots of constants for the new values.
* Removed token cache, since we will be keeping state on the clients.
* We now only refresh tokens when they are expired, which is more inline with the
OAuth spec.
* Refactored SetGRPCMetadata to be simpler to read.
* Refactored AddAuthToken.
* Update failing tests.
* We now don't split GRPC cookies on ";". Not sure why we did that before tbh.
I also added TODOs to add the refresh token to other subcommands. This is out of
scope for my current ticket, but it would be nice to have in the future.
* remove unnecessary context from Authenticate
* Add comments on why we check both cases for headers
It's because some GRPC clients lowercase metadata names. I've run into issues with
this enough that I think it's worth the extra checks. We prefer lowercase though,
because that's "standard".
* Fix test that broke during rebase
* Use IP whitelisting for ingest
For ingest, use configured IPs to authenticate the requests.
Auth-token will no longer be used for requests from ingest consumers.
If IP in request is in configured IPs, authenticate and authorize as an admin.
If IP in request is not in configured IPs, proceed with the standard authentication/authorization using ADD.
* need to remove port from client IP
* addressed review comments
* Change Ttl to TTL
Following go convention, acronyms should have a consistent case.
See
[Initialisms](https://github.com/golang/go/wiki/CodeReviewComments#initialisms)
This commit changes some public-facing methods, so any code importing
this package and using these methods will need to be updated.
* rewrite Ttl -> TTL
Co-authored-by: reesporte <reesedporter@gmail.com>
This commit addresses a bug in https://molecula.atlassian.net/browse/SUP-200 where
the Authorization header was not being set correctly when the token was passed via
"userinfo" in the context and not "token".
Now, we prefix the token with "Bearer " when the token comes from userinfo.
This commit also adds a unit test for this function, and simplifies logic. It also
fixes a test that didn't quite test the behavior we wanted.
