Clients overhaul #403
Clients overhaul #403
Conversation
|
For |
|
Ok, then I can go ahead and make a client this morning. |
Yaiba
force-pushed
the
clients-overhaul
branch
from
a3e94db
to
a4392b2
Compare
brennanjl
force-pushed
the
clients-overhaul
branch
from
a4392b2
to
d8cbcc4
Compare
Yaiba
force-pushed
the
clients-overhaul
branch
from
d8cbcc4
to
a4392b2
Compare
|
|
||
| // convertErr will convert the error to a known type, if possible. | ||
| // It is expected that the error is from a gRPC call. | ||
| func ConvertGRPCErr(err error) error { |
There was a problem hiding this comment.
Had to do similar for different reasons (txquery and broadcast), with handling in both http and grpc. I had found a bunch of breakage when client switched to http by default.
Same general approach, and not conflicting in concept. Also wrapping our error with join, but also combining with our own structured type.
I'll adapt things if this PR goes in first (probably should), so no need to change anything. Just noting.
Yaiba
force-pushed
the
clients-overhaul
branch
from
be969b2
to
0bc1af9
Compare
brennanjl
force-pushed
the
clients-overhaul
branch
from
0bc1af9
to
e7dde7d
Compare
|
We can fix @@ -348,10 +349,15 @@ func incrementPort(incoming string, amt int) (string, error) {
return "", err
}
+ if res.Scheme == "unix" { // no port to increment
+ return incoming, nil
+ }
+
// Split the URL into two parts: host (and possibly scheme) and port
host, portStr, err := net.SplitHostPort(res.Host)
if err != nil {
// Handle the case where there is no scheme (like "localhost:50051")
+ // NOTE: we could also detect this before SplitHostPort by inspecting res.Opaque
if strings.Contains(err.Error(), "missing port in address") {
parts := strings.Split(incoming, ":")
if len(parts) != 2 {with a test case like: {
input: "unix:///tmp/kwil_admin.sock",
amount: 99,
want: "unix:///tmp/kwil_admin.sock",
}, |
|
Just a heads up on a modification I made tonight: All flags now use hyphens, instead of underscores. Apparently, this is specified in the GNU conventions: https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html I was hesitant to make this change, but my rationale was that, after this release, we likely will not be able to change these since they are so critical to running kwild. Changing them also required me to change how we loaded configs (Viper forces all names to be the same, and since ENV vars and TOMLs use underscores, it wasn't possible to have both). We could actually get rid of Viper now; we simply use it for converting ENVs, and we could use https://github.com/kelseyhightower/envconfig to get the same effect with nesting. |
brennanjl
force-pushed
the
clients-overhaul
branch
from
03a50f9
to
503cf11
Compare
|
I got a panic running acceptance tests via |
|
That appears to be from |
I can't understand why that would be happening... The tests pass fine on Github. Did you rebuild? |
Yup. It's also happening in CI: https://github.com/kwilteam/kwil-db/actions/runs/7093407153/job/19306714183?pr=403#step:15:110 |
brennanjl
force-pushed
the
clients-overhaul
branch
from
503cf11
to
5b418f3
Compare
| // Merge merges b onto a, overwriting any fields in a that are also set in b. | ||
| func (a *KwildConfig) Merge(b *KwildConfig) error { | ||
| return merge.MergeWithOverwrite(a, b) |
There was a problem hiding this comment.
How is this different from *a = *b? The docs for this merge package are confusing around the "empty" concept. What does empty mean for bool or int?
There was a problem hiding this comment.
MergeWithOverwrite will merge take any non-zero values in b and set them in a.
There was a problem hiding this comment.
Empty for bool or int means it is the zero value, and therefore does not merge: darccio/mergo#178
There was a problem hiding this comment.
That's not correct in all cases then. What if the override is to set something to zero or false?
There was a problem hiding this comment.
Hmm I see. I guess we need some other way of checking that fields have been modified?
There was a problem hiding this comment.
We can also go back to the old way, however I may need some help. I spent almost 2 hours trying to get viper to support underscore in toml/envs and hyphens in flags, and could not figure out why it was not working. That's why I ended up just doing this
There was a problem hiding this comment.
Yeah I see. Ooof. Lemme see if there are any fields at present where this would be a problem in practice.
Changing some fields to pointers is also a soln, but not pretty.
There was a problem hiding this comment.
AppConfig.EnableRPCTLS, SnapshotConfig.Enabled, P2PConfig.AddrBookStrict, P2PConfig.MaxNumInboundPeers, P2PConfig.MaxNumOutboundPeers, P2PConfig.PexReactor, and P2PConfig.AllowDuplicateIP, and StateSyncConfig.Enable all seems like ones where a zero value could be sensible. Mempool also has some int values, but I don't think 0 values would make sense / be usable.
There was a problem hiding this comment.
The only one there where the default is non-zero-valued (and thus might need to be overridden with the zero value) is AllowDuplicateIP:
kwil-db/cmd/kwild/config/config.go
Line 420 in b32e76c
I think we can punt on this, but seems like a real issue to solve at some point.
| // unmarshal toml to maps | ||
| var cfg map[string]interface{} | ||
| err = toml.Unmarshal(bts, &cfg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse config file: %v", err) | ||
| } | ||
| return nil | ||
|
|
||
| // convert mapstructure toml to KwilConfig | ||
| var kwilCfg KwildConfig | ||
|
|
||
| mapDecoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ |
There was a problem hiding this comment.
I'm probably being dense, but I thought the idea of the pelletier/go-toml package was to unmarshal directly into your toml tagged struct. How come we have to go through the map[string]interface{} followed by mapstructure? I would expect to see the tags change to e.g. toml:"persistent_peers" and directly unmarshal into a *KwildConfig using toml.Unmarshal.
There was a problem hiding this comment.
This was simply for compatibility with the existing struct tags. We still use viper for parsing the ENVs, so I just kept it
| func (c *GatewayClient) GetAuthCookie() (cookie *http.Cookie, found bool) { | ||
| cookies := c.httpClient.Jar.Cookies(c.target) |
There was a problem hiding this comment.
I'm pretty sure using c.target here with Jar.Cookies() restricts kgw to setting the cookie path to /
cmd/kwild/config/config.go
Outdated
| @@ -265,15 +427,15 @@ func DefaultConfig() *KwildConfig { | |||
| }, | |||
| ChainCfg: &ChainConfig{ | |||
| P2P: &P2PConfig{ | |||
| ListenAddress: "tcp://0.0.0.0:26656", | |||
| ListenAddress: "tcp://127.0.0.1:26656", | |||
There was a problem hiding this comment.
Oh, this is why you had to update the docker-compose.yml to get blocks mining. I think this should say at 0.0.0.0 not a loopback. The p2p should be listening on all interfaces I believe.
Depending on where we go with #408, validators might want to be less promiscuous, but in our current model even the validators must be listening.
|
I'd be inclined to merge it, simply so that we can get everything else merged and ready for him. Easier to go back and do a last minute PR if something needs changed. He likely won't be working for 4 more hours. |
|
Sounds good, let's rebase, update proto, and merge. But actually, I hit a 401 on an authenticated call even after success on The {
"3Bj0mT6TtQSG4+VOJ9kdV87h2gc=": {
"name": "kgw_session",
"value": "OG8hhQpCkEiwM6XP0CyNpSCZTwSrjQqH8MVtCFFSuwE5ZjUyMDEwYjE1NjdlMjVhMzNjYQ==",
"expires": "0001-01-01T00:00:00Z"
}
}That expires is surely incorrect? Did I do something incorrect? |
Yeah, you have to pass the |
|
Seems to not be an issue with cookie persistence. Seems to actually be an issue with cookie issuance |
|
Yeah SaveCookie is working fine. |
auto kwil cli docs mostly finished with migrating kwil admin, still cleaning uo mostly finished with migrating kwil admin, still cleaning up refactored kwil admin into a standard cobra app minor changes to doc generation registering txsvc in the admin svc as well made an example admin grpc client added admin client that includes kwild client added documentation for tls added protobuf for expanded admin svc various changes and cleanup for rpcs http codegen added http client using generated stubs, fixed several bugs in current proto definitions refactoring the clients gateway client concept added gateway client to kwil-cli everything building, still some issues various bug fixes final cleanup, tweaks fixed bug causing failures in integration tests update ci check swagger generated code added url parsing util since stdlib is very finnicky when managing grpc, http, and unix update linter skip dirs update assert import tidy enable swagger check added integration test mounting for volumes fixed docker compose issue in acceptance test debugging integration test unix sockets improved output formatting for kwil-admin various upgrades to tests for validators fixed all integration tests updated run permissions of swagger check script addressed Jons feedback, trying new permission with failing script fixed failing unit tests added kwil-admin to main dockerfile retrying due to transient ci error trying to fix failing CI build fix dockerfile copy command added a node info utility, and also added integration tests changed all kwild flags to use hyphens, since it is far more common figured bug caused by updating flags to hyphens from underscores changed flags to use hyphens instead of underscores in flags, to be in line with GNU standards fixed typo that linter was complaining about added functionsvc client removed node info, due to security issues added setup peer command, which makes it easy to join a running network. Also changed genesis to exist in root dir added list pending joins output and formatting upgrades
…needed for kwild, and renamed client constructors
…d avoid confusion with metadata
…ies (previously only persisted manual auth)
brennanjl
force-pushed
the
clients-overhaul
branch
from
9f9bc1f
to
b0bee6d
Compare
I think it's the default cookiejar issue, And the outout: |
This PR depends on this proto PR: kwilteam/proto#26
Apologies for the mega branch. I've been trying to get better about this, but sort've got swept up in the tight deadline and the interconnectedness of these problems.
There are still a few minor things I am working through (particularly with validator integration tests, CI workflows, as well as any other edge cases I run into as I finish the guide docs), but the rest of it is in place.
This PR includes:
kwil-cliandkwil-admin.kwil-adminis now a cobra appkwil-adminnow supports remote signing for validator commandskwil-adminnow supports unix as well as grpckwil-cliandkwil-adminfunctionservice, which containsVerifySignature. It was previously incore/client, but this was not the right place for it. I assume the gateway relies on this?)adminclientandgatewayclientpackages, that handle the usecase specific nuances (transport layers, cookies, auth, etc) using composed clients and transport layerscore/gatewayclient), that supports both automatic and manual auth