New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

`skywire-cli vpn` subcommands + separate-systray #1317

Merged

0pcom merged 16 commits into develop from feature/cli-flags

Aug 14, 2022

Collaborator

0pcom commented Jul 16, 2022

We need these flags to avoid users needing to edit the .json by hand, and to integrate with package / installer configuration scripts.

Did you run make format && make check?
yes

Changes:

add skywire cli config gen -y --autoconn
add skywire cli config gen -z --ispublic

test:

go run cmd/skywire-cli/skywire-cli.go config gen --all
go run cmd/skywire-cli/skywire-cli.go config gen -yn
go run cmd/skywire-cli/skywire-cli.go config gen -zn

0pcom added 5 commits

July 18, 2022 13:54


          add flags for skywire-cli vpn list

8dde57a


          remove accidentally included files

405d731


          establish transport before starting VPN

0f83f14


          update vpn list cli command logic

b202365


          add skywire-cli vpn stop after successful tests with skywire-cli vpn …

70dfbb6

…start ; fix make check errors

0pcom changed the title ~~is_public and public_autoconnect skywire-cli config gen flags~~ skywire-cli config gen flags + skywire-cli vpn subcommands

Collaborator Author

0pcom commented Jul 19, 2022

skywire-cli vpn subcommands now on this PR:

skywire-cli vpn list
skywire-cli vpn start <public-key>
skywiire-cli vpn stop

list has flags to filter country & version
start will automatically establish transports

working when tested with the test deployment

full test ; run a visor as public on test deployment and disable the public autoconnect and use package config path

sudo go run cmd/skywire-cli/skywire-cli.go config gen -biprtvyz
sudo go run cmd/skywire-visor/skywire-visor.go -p

In another terminal:

skywire-cli vpn list

If running on test deployment, the only result is likely to be the locally running visor itself.

If it's possible to test with two visors, after the second visor is running, start the vpn server to its public key

skywire-cli vpn start <public-key>

to complete the test

skywire-cli vpn stop

Next for this PR I will add my systray rework.

The revised systray can co-exist with the current systray and should not affect mergability of this PR

jdknives requested changes

View reviewed changes

cmd/skywire-cli/commands/vpn/vvpn.go Outdated

		"github.com/skycoin/skywire/pkg/visor"
		"github.com/skycoin/skywire/cmd/skywire-cli/commands/visor"

Member

jdknives Jul 20, 2022

Doesnt appear to be your issue, but please remove the commented import.

cmd/skywire-cli/commands/vpn/vvpn.go Outdated

               	"github.com/spf13/cobra"
               	"github.com/toqueteos/webbrowser"
+              	utilenv "github.com/skycoin/skywire-utilities/pkg/skyenv"
+              	skyenv "github.com/skycoin/skywire/pkg/skyenv"

Member

jdknives Jul 20, 2022

May be a good move to get this to skywire-utilities repo as well.

Collaborator Author

0pcom Jul 20, 2022

the skyenv is here just getting the default name of the app binary "vpn-client"

that could be redundantly defined in skywire-utilities to avoid the import in this instance, but considering that the vpn-client app itself s defined in skycoin/skywire I think it prudent to keep it here.

cmd/skywire-cli/commands/vpn/vvpn.go Outdated

               	//"github.com/skycoin/skywire-utilities/pkg/cipher"
               )
+              var  timeout	time.Duration

Member

jdknives Jul 20, 2022

Format your code please.

cmd/skywire-cli/commands/vpn/vvpn.go Outdated

@@ @@ -2,33 +2,47 @@ package clivpn @@
               import (
               	"encoding/json"
+              	"io/ioutil"

Member

jdknives Jul 20, 2022

Is the file name intentional?

Collaborator Author

0pcom Jul 20, 2022

my reasoning behind this naming scheme is to avoid making generic names for files or imports like "vpn.go" or "package vpn" as surely these exist somewhere else in the source already, and I would not want to pollute search results for such files and package names.

Is it your suggestion to change it?

cmd/skywire-cli/commands/vpn/vvpn.go

               	"github.com/spf13/cobra"
               	"github.com/toqueteos/webbrowser"
+              	utilenv "github.com/skycoin/skywire-utilities/pkg/skyenv"
+              	skyenv "github.com/skycoin/skywire/pkg/skyenv"
               	"github.com/skycoin/skywire/pkg/visor/visorconfig"
               	clirpc "github.com/skycoin/skywire/cmd/skywire-cli/commands/rpc"

Member

jdknives Jul 20, 2022

Any reason we are naming this import here? Is RPC not fine? I dont see us importing the stdlib rpc package here.

Member

jdknives Jul 20, 2022

Also, is the file name intentional?

Collaborator Author

0pcom Jul 20, 2022

it was an attempt to avoid confusion with any other package named rpc and specify that this is the one that is associated with or provided by the cli subcommands

that same rpc client function was in a few different files

cmd/skywire-cli/commands/vpn/vvpn.go Outdated

+              			network.SUDPH,
+              			network.DMSG,
+              		}
+              		for _, transportType := range transportTypes {

Member

jdknives Jul 20, 2022

This will almost necessarily fail for most transports here. Why are we trying to establish 4 transports? In my opinion, we should leave auto transport establishment to the router module as it has a substantially better view of current transports etc.

Collaborator Author

0pcom Jul 20, 2022

From the currently available cli interface; the method I have used is the same as is used by skywire-cli tp add - sans the for loop

Is what you described provided by some other cli command currently? if so, what?

cmd/skywire-visor/commands/root.go Outdated

@@ @@ -262,6 +262,7 @@ func runVisor(conf *visorconfig.V1) { @@
               			conf.Hypervisors = append(conf.Hypervisors, pubkey)
               		}
               	}
+              	fmt.Printf("%+v\n", conf.Launcher.Apps[0].Args)

Member

jdknives Jul 20, 2022

Whats this doing here?

Member

jdknives Jul 20, 2022

Could this panic?

Collaborator Author

0pcom Jul 20, 2022

that was for some debugging i was doing. supposed to have been removed.

perhaps it could panic if there were no apps in the config

pkg/visor/api.go Outdated

@@ @@ -36,11 +36,13 @@ type API interface { @@
               	Health() (*HealthInfo, error)
               	Uptime() (float64, error)
+              	//	QueryUptime(pubkeys []string) (*Uptime, error)

Member

jdknives Jul 20, 2022

Comment?

Collaborator Author

0pcom Jul 20, 2022

This was something I had mosty implemented - querying the uptime from the uptime tracker for arbitrary (remote visor) keys

there was just one small bug in it that i wanted mohammad to look at.

I think this is worthwhile to have, but my original reason for needing it should be addressed by the fixes to the production deployment. So it is no longer critical, but it is something I would make a cli command specifically for...

pkg/visor/api.go Outdated

+              	if v.tpM == nil {
+              		return ErrTrpMangerNotAvailable
+              	}
+              	v.conf.Launcher.Apps[0].Args = []string{"-srv", pubkey}

Member

jdknives Jul 20, 2022

How do we know the first app is the VPN?

Member

jdknives Jul 20, 2022

Is this code equivalent to what we do when we call the VPN start cli command typically?

Collaborator Author

0pcom Jul 20, 2022

this was mostly adapted from the adjacent functions and the skywire cli visor app start / stop commands

I know it's the first app for myself but there are instances where it may not even exist and then this would cause panic. Will solve that.

pkg/visor/api.go Outdated

-              	}
-              	return serverAddrs, nil
+              	//	serverAddrs := make([]string, len(vpnServers))
+              	//	for idx, server := range vpnServers {

Member

jdknives Jul 20, 2022

Comment

0pcom changed the title ~~skywire-cli config gen flags + skywire-cli vpn subcommands~~ skywire-cli vpn subcommands

0pcom added 4 commits

July 20, 2022 15:50


          improve detection of vpn-client app config in api with range loop ins…

c7bbb0e

…tead of hardcoding index ; make format check


          remove extra logging from cmd/skywire-visor/commands/root.go

498aa61


          make separate-systray work with vpn cli commands

a2b1a5c


          update cmd/skywire-cli/commands/vpn/vvpn.go

8ddf64d

0pcom mentioned this pull request

separate systray [WIP] #1308

Closed

0pcom changed the title ~~skywire-cli vpn subcommands~~ skywire-cli vpn subcommands + separate-systray

This was referenced Aug 1, 2022

Create separate Systray UI package #1140

Closed

Create skywire-cli vpn command #1277

Closed

0pcom added the v1.1.0 label

jdknives removed the v1.1.0 label

0pcom added 7 commits

August 8, 2022 11:17


          add version flag for skywire-cli ; filter results of skywire-cli vpn …

ab9bf63

…list by buildinfo.Version() instead of hardcoded version by default


          update cmd/skywire-systray/skywire-systray.go to match restructured s…

37b22e1

…kywire-cli subcommands


          Merge branch 'develop' into feature/cli-flags

f5d0e06


          move skywire-cli hv subcommand to skywire-cli visor hvui

73d4dbf


          make format check

d5e68d6


          skywire-cli visor vpn list query filtering

67bb78c


          comment out query filtering ; make format check

b42e900

Collaborator Author

0pcom commented Aug 14, 2022

Merging & will open a new PR with uncommented query filtering

0pcom merged commit 9961f74 into develop

0pcom deleted the feature/cli-flags branch

August 14, 2022 16:08

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment