Updating webapp to latest scionlab dist #147

mwfarb · 2020-05-15T16:50:19Z

New location of beacon logging.
Calling all sciond connected methods by tcp address.
Updated service names, IA error check
Loading sciond address from sd.toml development/production.
Updating AS Topology calls
Updated parsing of paths
Closes [webapp] Adapt to updated SCIONLab #132
Created default relative paths (example:-sroot /git/scion -srvroot /git/scion-apps/webapp/web) to allow skipping other paths in development.
Migrated known fixes from other PRs.
Fixed continuous traceroute.

This change is

- New location of beacon logging - removed scion-apps args for local address - rework default args - updated service names, IA error check - loading sciond address from sd.toml - updating asTopo calls - Updated parsing of paths

FR4NK-W

Reviewed 20 of 20 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mwfarb)

webapp/lib/bwcont.go, line 197 at r1 (raw file):

// ensure % if a

Comment at wrong place

webapp/lib/config.go, line 113 at r1 (raw file):

compute

I think you can drop the compute

webapp/models/traceroute.go, line 356 at r1 (raw file):

inserted = hop.Inserted

These variables don't seem necessary anymore, directly assign the values.

webapp/web/static/js/asviz.js, line 402 at r1 (raw file):

var svc_pre = {
    0 : "unset", // Not set
    1 : "bs", // Beacon service

Those services have been unified into the Control Service

webapp/web/static/js/tab-paths.js, line 366 at r1 (raw file):

ent

ent -> entry

webapp/web/static/js/tab-paths.js, line 395 at r1 (raw file):

"<li seg-type='PATH' seg-num=" + p + "><a href='#'><span "

I see you are cleaning this up.
This might even be cleaner: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

webapp/web/tests/health/testSCIONRunning.sh, line 63 at r1 (raw file):

if [ $isd -ge 16 ]; then
    # not used for localhost testing
    check_presence /run/shm/sciond default.sock

You still might want to check that the TCP sciond socket is properly running, e.g. by retrieving the config:
curl -m 1 127.0.0.1:30455/config

mwfarb

Reviewable status: 12 of 21 files reviewed, 7 unresolved discussions (waiting on @FR4NK-W and @matzf)

webapp/lib/bwcont.go, line 197 at r1 (raw file):

Previously, FR4NK-W wrote…

// ensure % if a
Comment at wrong place

Done.

webapp/lib/config.go, line 113 at r1 (raw file):

Previously, FR4NK-W wrote…

compute
I think you can drop the compute

Done.

webapp/models/traceroute.go, line 356 at r1 (raw file):

Previously, FR4NK-W wrote…

inserted = hop.Inserted
These variables don't seem necessary anymore, directly assign the values.

Done.

webapp/web/static/js/asviz.js, line 402 at r1 (raw file):

Previously, FR4NK-W wrote…

Those services have been unified into the Control Service

True, but the sciond query for services info still responds with the old microservice names, which this list is meant to match.

webapp/web/static/js/tab-paths.js, line 366 at r1 (raw file):

Previously, FR4NK-W wrote…

ent
ent -> entry

Done.

webapp/web/static/js/tab-paths.js, line 395 at r1 (raw file):

Previously, FR4NK-W wrote…

"<li seg-type='PATH' seg-num=" + p + "><a href='#'><span "
I see you are cleaning this up.
This might even be cleaner: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

Thank you! That is so much cleaner.

webapp/web/tests/health/testSCIONRunning.sh, line 63 at r1 (raw file):

Previously, FR4NK-W wrote…

You still might want to check that the TCP sciond socket is properly running, e.g. by retrieving the config:
curl -m 1 127.0.0.1:30455/config

Done.

matzf

Reviewed 9 of 9 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @FR4NK-W and @mwfarb)

a discussion (no related file):
It seems that the names of the scion-apps binaries need to be updated to work on scion-apps master (recent changes to Makefile). The names of the binaries are now in line with the names used in the packages, so this should also fix packaged installation.
Additionally, running the apps fails, because the commandline passes the -sciond flag which we've removed and replaced with an environment variable.
I have a small patch to fix these issues. If it's ok for you, I can push this to this branch directly.

webapp/lib/config.go, line 134 at r2 (raw file):

	port := flag.Int(CMD_PRT, listenPortDef, "Port of server host.")
	appsRoot := flag.String(CMD_ART, path.Join(GOPATH, "bin"),
		"Path to execute the installed scionlab apps binaries")

With the apps now all built in the same directory, the simplest default would be the location of the webapp binary. (See os.Executable).

webapp/lib/config.go, line 182 at r2 (raw file):

	// writing myIA means we have to retrieve sciond's tcp address too
	// since sciond's address may be autognerated
	config, err := LoadSciondConfig(options, settings.MyIA)

I don't follow here; which problem with autogenerated addresses is prevented by writing out the SD address(es) to disk? At least some places seem to ignore the value from the settings anyway (AsTopoHandler and PathTopoHandler).

webapp/lib/sciond.go, line 219 at r2 (raw file):

sd*

This now changes the behaviour to show the scion daemon path DB. To keep the old behaviour, this could show the cs*.path.db.

webapp/web/tests/health/testSCIONRunning.sh, line 63 at r1 (raw file):

Previously, mwfarb (mwfarb) wrote…

Done.

Not blocking; with the sciond itself running over TCP, wouldn't be easier to check for the actual SD port, instead of the config webserver thing? I'm sure there are dozens of ways, but one might be netcat: nc -z <ip> <port>

mwfarb

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @FR4NK-W, @matzf, and @mwfarb)

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

It seems that the names of the scion-apps binaries need to be updated to work on scion-apps master (recent changes to Makefile). The names of the binaries are now in line with the names used in the packages, so this should also fix packaged installation.
Additionally, running the apps fails, because the commandline passes the -sciond flag which we've removed and replaced with an environment variable.
I have a small patch to fix these issues. If it's ok for you, I can push this to this branch directly.

Yes please, that would be great. Please merge it in. :-)

matzf

Reviewable status: 20 of 22 files reviewed, 8 unresolved discussions (waiting on @FR4NK-W, @matzf, and @mwfarb)

a discussion (no related file):

Previously, mwfarb (mwfarb) wrote…

Yes please, that would be great. Please merge it in. :-)

Done.

matzf

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @FR4NK-W and @mwfarb)

matzf

Reviewable status: 17 of 22 files reviewed, 7 unresolved discussions (waiting on @FR4NK-W and @matzf)

webapp/lib/sciond.go, line 219 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

sd*
This now changes the behaviour to show the scion daemon path DB. To keep the old behaviour, this could show the cs*.path.db.

On more careful reading, the sd path DB seems more appropriate for what this is used (retrieve segment information just after paths have been queried ).

webapp/web/tests/health/testSCIONRunning.sh, line 63 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Not blocking; with the sciond itself running over TCP, wouldn't be easier to check for the actual SD port, instead of the config webserver thing? I'm sure there are dozens of ways, but one might be netcat: nc -z <ip> <port>

Done.

matzf

Reviewable status: 17 of 22 files reviewed, 7 unresolved discussions (waiting on @FR4NK-W and @matzf)

webapp/lib/config.go, line 134 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

With the apps now all built in the same directory, the simplest default would be the location of the webapp binary. (See os.Executable).

Done

matzf

Reviewed 5 of 5 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FR4NK-W)

FR4NK-W

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

mwfarb added 6 commits May 15, 2020 12:48

Updating webapp to latest scionlab dist v0.1.0

885ba90

- New location of beacon logging - removed scion-apps args for local address - rework default args - updated service names, IA error check - loading sciond address from sd.toml - updating asTopo calls - Updated parsing of paths

Merge branch 'master' into webapp_sl_v0.1.0

4b36837

migrating all known fixes/refactoring/performant updates

7f67715

improved command line experience

ca21676

url

fc66d4d

more code cleanup

6dd8567

mwfarb changed the title ~~Updating webapp to latest scionlab dist v0.1.0~~ Updating webapp to latest scionlab dist May 22, 2020

mwfarb marked this pull request as ready for review May 22, 2020 07:07

FR4NK-W requested changes May 25, 2020

View reviewed changes

FR4NK-W requested a review from matzf May 25, 2020 13:29

mwfarb added 3 commits May 28, 2020 02:11

Addressed r1 comments

3bbf032

use proper metrics

2e4c2a2

fixed sciond checks

a730918

mwfarb commented May 28, 2020

View reviewed changes

mwfarb requested a review from FR4NK-W May 28, 2020 17:58

matzf suggested changes Jun 2, 2020

View reviewed changes

Update app binary names, set sciond env

eada73a

mwfarb commented Jun 3, 2020

View reviewed changes

matzf reviewed Jun 3, 2020

View reviewed changes

matzf added 2 commits June 22, 2020 11:24

Simplify sciond health check, avoid using metrics port

6effa28

Simplify default directory paths used for webapp

7698b2e

matzf reviewed Jun 22, 2020

View reviewed changes

matzf added 2 commits June 22, 2020 11:54

Fix lint

d942149

Merge branch 'master' into webapp_sl_v0.1.0

dcfac31

matzf approved these changes Jun 22, 2020

View reviewed changes

FR4NK-W approved these changes Jun 22, 2020

View reviewed changes

matzf merged commit bacb201 into netsec-ethz:master Jun 22, 2020

mwfarb deleted the webapp_sl_v0.1.0 branch September 5, 2020 04:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating webapp to latest scionlab dist #147

Updating webapp to latest scionlab dist #147

mwfarb commented May 15, 2020 •

edited

Loading

FR4NK-W left a comment

mwfarb left a comment

matzf left a comment

mwfarb left a comment

matzf left a comment

matzf left a comment

matzf left a comment

matzf left a comment

matzf left a comment

FR4NK-W left a comment

Updating webapp to latest scionlab dist #147

Updating webapp to latest scionlab dist #147

Conversation

mwfarb commented May 15, 2020 • edited Loading

FR4NK-W left a comment

Choose a reason for hiding this comment

mwfarb left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

mwfarb left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

FR4NK-W left a comment

Choose a reason for hiding this comment

mwfarb commented May 15, 2020 •

edited

Loading