scion: one command to rule them all

[oncilla]

Cobra has shown to be a very useful framework for writing CLI applications.
It makes it easy to write applications with many sub-commands and high configurability.

We currently have many binaries in the code base, that do not really warrant to be their own application. With the approach that we put most application logic in go/pkg/..., we can easily create standalone binaries (when needed/desired) and also bundle them in an all-in-one scion command.

This PR is a exploration how this could look like.
At the base is the scion command that bundles all the other commands.

For this exploration, I moved the showpaths logic to go/pkgs/showpaths such that it can be imported as a library. The old showpaths binary is not affected by the change, and if we continue to support the standalone binary, this will be easy to do.

Under go/scion the all-in-on binary is set up. Currently, it only has 3 commands:

• completion : generate auto-completion
• version: display the scion version
• showpaths: run the showpaths application

The nice thing is, that we gradually can add sub-commands that make sense to be bundled.
E.g., I envision scion echo, scion traceroute, scion pingpong, and the soon to be added scion pathdbdump (#3707) tools to be part of scion eventually.

---

This change is 

[karampok]

Reviewed 7 of 11 files at r1.
Reviewable status: 7 of 11 files reviewed, 12 unresolved discussions (waiting on @karampok and @oncilla)

a discussion (no related file):
Is that binary go get? would be nice someone to use without building everything.

---

a discussion (no related file):
Personally I like to consolidate our stuff, but there should a direct effort to remove the old binaries
(not keep the in parallel).

---

---

go/pkg/showpaths/config.go, line 15 at r1 (raw file):

// limitations under the License.

package showpaths

if we have this pkg, who will longterm use it apart from the scion cli?

---

go/pkg/showpaths/config.go, line 47 at r1 (raw file):

// InitDefaults initializes the default values.
func (cfg *Config) InitDefaults() {

about default values, I will add them on the flag only (when each config is flag in the command line, the default value will be stated only there).
Single source of truth that is visible to the user will be good idea

---

go/scion/completion.go, line 34 at r1 (raw file):

For example, you can add autocompletion for your current bash session using:

    . <( scion completion )

usually, I see eval "$( scion completion bash)"
I do not remember why, but there was a difference

---

go/scion/completion.go, line 38 at r1 (raw file):

To permanently add bash autocompletion, run:

    scion completion > /etc/bash_completion.d/scion

this requires sudo access, and there more ways to that (for example, I just do eval in bashrc).
I suggest we can just remove the documentation about it. It is a well-known task.

---

go/scion/completion.go, line 47 at r1 (raw file):

			return rootCmd.GenZshCompletion(os.Stdout)
		default:
			return serrors.New("unknown shell", "input", completionShell)

no fish shell?!

---

go/scion/scion.go, line 41 at r1 (raw file):

https://github.com/spf13/cobra/issues/340#issuecomment-374617413

the ugliest thing about cobra

---

go/scion/showpaths.go, line 30 at r1 (raw file):

)

var showpathsFlags struct {

this struct and the config struct is so similar, can we somehow use only one?

---

go/scion/showpaths.go, line 42 at r1 (raw file):

var showpathsCmd = &cobra.Command{
	Use:   "showpaths",

I always like a short version scion sp because lines are super long already

---

go/scion/showpaths.go, line 89 at r1 (raw file):

func init() {
	rootCmd.AddCommand(showpathsCmd)
	showpathsCmd.Flags().StringVar(&showpathsFlags.sciond, "sciond", sciond.DefaultSCIONDAddress,

Is there a framework to do that base on structs and tags? It would be super nice.

for example like https://github.com/octago/sflags

---

go/tools/showpaths/paths.go, line 72 at r1 (raw file):

	ctx, cancelF := context.WithTimeout(context.Background(), *timeout)
	defer cancelF()
	sdConn, err := sciond.NewService(*sciondAddr).Connect(ctx)

given that there are no tests here, any refactor on that code makes me uncomfortable.
My suggestion is duplication and hard switch.
Also to make the code re-usable from two parts might lead to not optimal decisions.

[oncilla]

Reviewable status: 3 of 9 files reviewed, 12 unresolved discussions (waiting on @karampok and @oncilla)

a discussion (no related file):

Previously, karampok (Konstantinos) wrote…

Is that binary go get? would be nice someone to use without building everything.

It should be. But I don't know how to test unless it is in master. Do you know a way?

---

a discussion (no related file):

Previously, karampok (Konstantinos) wrote…

Personally I like to consolidate our stuff, but there should a direct effort to remove the old binaries
(not keep the in parallel).

I would prefer having scmp etc in place before getting rid of the old binaries.

---

---

go/pkg/showpaths/config.go, line 15 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

if we have this pkg, who will longterm use it apart from the scion cli?

I don't think so. But I want our main packages to be as small as possible. I only want flag, env-parsing and a bit of initialization in there. All real logic should be outside of the main package.
Every time where we let stuff live in the main package, it is riddled with globals. And then a use case comes up where we do want to import it for the outside, and boom unhappy days.

---

go/pkg/showpaths/config.go, line 47 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

about default values, I will add them on the flag only (when each config is flag in the command line, the default value will be stated only there).
Single source of truth that is visible to the user will be good idea

Done.

---

go/scion/completion.go, line 34 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

usually, I see eval "$( scion completion bash)"
I do not remember why, but there was a difference

I think . is correct. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#dot

but I'm not a bash guru. 🤷

---

go/scion/completion.go, line 38 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

this requires sudo access, and there more ways to that (for example, I just do eval in bashrc).
I suggest we can just remove the documentation about it. It is a well-known task.

It is well-known to you. But some users will not know how this works.

Putting a sudo here makes unnecessary assumptions about how a user will run the application.
When they run into a permission error, they likely know to add a sudo in front.

I still think this is useful information, and I want to keep it.

---

go/scion/completion.go, line 47 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

no fish shell?!

cobra has no support. I'm sure they would accept a PR upstream 😉

---

go/scion/scion.go, line 41 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

https://github.com/spf13/cobra/issues/340#issuecomment-374617413

the ugliest thing about cobra

meh, this seems to work pretty okay.

---

go/scion/showpaths.go, line 30 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

this struct and the config struct is so similar, can we somehow use only one?

Done.

---

go/scion/showpaths.go, line 42 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I always like a short version scion sp because lines are super long already

Done.

---

go/scion/showpaths.go, line 89 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Is there a framework to do that base on structs and tags? It would be super nice.

for example like https://github.com/octago/sflags

TBH, I don't want to pull in a dependency for something so simple.

---

go/tools/showpaths/paths.go, line 72 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

given that there are no tests here, any refactor on that code makes me uncomfortable.
My suggestion is duplication and hard switch.
Also to make the code re-usable from two parts might lead to not optimal decisions.

Done.

[oncilla]

Reviewable status: 3 of 9 files reviewed, 12 unresolved discussions (waiting on @karampok and @oncilla)

---

go/scion/completion.go, line 47 at r1 (raw file):

Previously, Oncilla wrote…

cobra has no support. I'm sure they would accept a PR upstream 😉

spf13/cobra#1048 oh wow, it was added one week ago 🎉🎉🎉🎉

[karampok]

Reviewed 2 of 11 files at r1, 1 of 6 files at r2, 7 of 7 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)

---

go/pkg/showpaths/showpaths.go, line 84 at r3 (raw file):

// Run lists the paths to the specified ISD-AS to stdout.
func Run(ctx context.Context, dst addr.IA, cfg Config) (*Result, error) {

If a such named function is inside cobra, it does make sense.
Outside it is a bit "weird" name. If you want a new pkg, then maybe rename that to something else
(and ideally, an interface with the Public API calls this pkg should expose)

---

go/pkg/showpaths/showpaths.go, line 145 at r3 (raw file):

}

// TODO(matzf): this is a simple, hopefully temporary, workaround to not having

Is that comment valid, is there a better approach? (just asking no changes)

---

go/scion/scion.go, line 41 at r1 (raw file):

Previously, Oncilla wrote…

meh, this seems to work pretty okay.

Is there a need for comment then?

[oncilla]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @karampok)

---

go/pkg/showpaths/showpaths.go, line 84 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

If a such named function is inside cobra, it does make sense.
Outside it is a bit "weird" name. If you want a new pkg, then maybe rename that to something else
(and ideally, an interface with the Public API calls this pkg should expose)

what is weird about this. showpaths.Run ?

---

go/pkg/showpaths/showpaths.go, line 145 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

Is that comment valid, is there a better approach? (just asking no changes)

Yes, but we don't have an agreement on what the better approach is.

To me, select for every path the local IP based on the next hop border router.
To matzf, bake this into snet.

---

go/scion/scion.go, line 41 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Is there a need for comment then?

The comment is here for people that wonder why we dot his.

[karampok]

Reviewable status: complete! all files reviewed, all discussions resolved

[oncilla]

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved