core: kgw cookie on multi providers #691
Conversation
Yaiba
force-pushed
the
feature/kgw-client-multi-provider
branch
from
11d0430 to
a9a902c
Compare
Yaiba
changed the title
|
@KwilLuke I couldn't find how kwil-js handle this in nodejs, can check that? |
Yaiba
force-pushed
the
feature/kgw-client-multi-provider
branch
from
a9a902c to
8110b3f
Compare
Yaiba
changed the title
Yaiba
force-pushed
the
feature/kgw-client-multi-provider
branch
from
8110b3f to
8aa3934
Compare
| @@ -98,14 +98,19 @@ func DialClient(ctx context.Context, cmd *cobra.Command, flags uint8, fn RoundTr | |||
| return fn(ctx, client, conf) | |||
| } | |||
|
|
|||
| cookie, err := LoadPersistedCookie(KGWAuthTokenFilePath(), clientConfig.Signer.Identity()) | |||
| providerDomain, err := getDomain(conf.Provider) | |||
There was a problem hiding this comment.
When backport, conf.Provider should be conf.GrpcUrl
There was a problem hiding this comment.
All looks, fine, but I'm not certain why we have to use this computed domain (actually scheme://domain[:port]) when doing SaveCookie. Does http.Cookie.Domain not persist in the cookie jar? Or would that not be the exactly right key, missing scheme and probably port?
| return "", fmt.Errorf("target is empty") | ||
| } | ||
|
|
||
| parsedTarget, err := url.Parse(target) |
There was a problem hiding this comment.
One quirk of url.Parse is how it handles a string like localhost:8080. It does not error. It returns a URL with no host and "Opaque":
&url.URL{Scheme:"localhost", Opaque:"8080", User:(*url.Userinfo)(nil), Host:"", Path:"", RawPath:"", OmitHost:false, ForceQuery:false, RawQuery:"", Fragment:"", RawFragment:""}
We'll probably be rejecting or erroring on such a target before we reach getDomain, but it's still probably an unintended result for getDomain:
- input
localhost:8080 - output
localhost://
There was a problem hiding this comment.
ah I got screwed every time I use url.Parse 🤦♂️
|
| @@ -84,6 +85,10 @@ func getDomain(target string) (string, error) { | |||
| return "", fmt.Errorf("target is empty") | |||
| } | |||
|
|
|||
| if !(strings.HasPrefix(target, "http://") || strings.HasPrefix(target, "https://")) { | |||
There was a problem hiding this comment.
For 0.7 or v0.6, this should work, since we require scheme https://docs.kwil.com/docs/kwil-cli/configuration#available-configs.
For v0.8, seems this could break?
There was a problem hiding this comment.
you got a pretty handy util function here https://github.com/kwilteam/kwil-db/pull/703/files#diff-7a8b6c70656b28a8bba3d014ad27d1a48efe27f72f184939a4b9d7b5d2313a84R72, I think it will help.
There was a problem hiding this comment.
With latest update to that PR, cfg.Provider must be a proper URL now. Another reason to keep that convention I figure.
There was a problem hiding this comment.
The persistened cfg.Provider could still be without scheme, am I right?
There was a problem hiding this comment.
Not any more. It's an error:
$ ./kwil-cli database list
Error: parse url: parse "127.0.0.1:8484": first path segment in URL cannot contain colon
parse url: parse "127.0.0.1:8484": first path segment in URL cannot contain colon
$ cat ~/.kwil-cli/config.json
{"provider":"127.0.0.1:8484"}
I think we had that same constraint in v0.7?
There was a problem hiding this comment.
We have some unpleasant constraint, like this :
.build/kwil-cli database list --kwil-provider localhost:8080
Error: wrap client: chain_info: Get "localhost:8080/api/v1/chain_info": unsupported protocol scheme "localhost"
wrap client: chain_info: Get "localhost:8080/api/v1/chain_info": unsupported protocol scheme "localhost"
It'll get error when create client, that's before getDomain, so I guess we're fine
Yaiba
added
backport-to-release-v0.7
|
eh, backport workflow didn't run |
* core: kgw cookie on multi providers
* core: kgw cookie on multi providers
* core: kgw cookie on multi providers (#691) * fix: use conf.GrpcURL
* core: kgw cookie on multi providers (#691) * fix: use conf.GrpcURL
This pr change the saved cookie
keyfor kgw client, to support multi providers.The behavior before is shown here
This pr is supposed to backported to v0.7 and v0.6.
I'll come back to this pr once #689 is merged