Skip to content
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.

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

Restore segments to path graph & bug fixes #44

Merged
merged 7 commits into from
Apr 3, 2019

Conversation

mwfarb
Copy link
Contributor

@mwfarb mwfarb commented Mar 1, 2019

The major work of this PR is to restore segments to the path graph:

  • copy in pace and read path database for segment data
  • render segment data in UI list of interfaces and paths graph
  • filter "dirty" list of segments from paths db against known paths
  • use filtered segments to mark proper IAs core/not core

Additional minor fixes and improvements included:

  • sort "enp" interfaces first, then "lo", then alphabetically (prevents docker0 loading as default when testing locally)
  • reduce logging debug output for webapp.log
  • fixed several small runtime errors
  • prevented accidental db downgrade
  • updated AS Topology graph to consider each unique ip/port as a single border router, which may have multiple interfaces
  • use a testing IA when testing on local topology and $SC is undefined

This change is Reviewable

- Adding path database lookup for segments
- Using segments to derive core IAs in path graph
- Prevent accidental db downgrade
- Ensure localhost interface when not in VM
- Default to port 8000, improved localhost defaults
- Load default IA when not scionlab $SC not defined
- combine ia topology BRs using same ip:port
- handle error case when src==dst
- add filter for unused up, down, expired segments
@mwfarb mwfarb changed the title Restore segments to path graph & bug fixes (WIP) Restore segments to path graph & bug fixes Apr 3, 2019
Copy link
Member

@juagargi juagargi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Not from this PR, but I've noticed we have entries such as `ffaa:1:f` and other `ffaa:1:XXXX` in the `webapp/config/servers_default.json` file. Maybe we can remove those user ASes from the list? Or what was the reason to have them there?

Reviewed 8 of 10 files at r1, 1 of 2 files at r2, 3 of 3 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor Author

@mwfarb mwfarb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default server addresses come from the addresses we encourage people to test scionlab apps with in our tutorials:
https://netsec-ethz.github.io/scion-tutorials/sample_projects/bwtester/
https://netsec-ethz.github.io/scion-tutorials/sample_projects/access_camera/
https://netsec-ethz.github.io/scion-tutorials/sample_projects/fetch_sensor_readings/

We use servers_default.json to populate the server dropdown list on the webapp Apps tab to make it easier to use out of the box..

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

Copy link
Contributor Author

@mwfarb mwfarb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a suggestion to keep them out of the code base, I can add a future PR to migrate these locations to our Visualization GAE tool storing API keys. I probably need to move it anyway so that it moves from scion-viz to scion-apps.

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

@mwfarb mwfarb merged commit 30c6df0 into netsec-ethz:master Apr 3, 2019
@mwfarb mwfarb deleted the bug_fixes_feb branch April 3, 2019 14:40
cneukom pushed a commit to cneukom/scion-apps that referenced this pull request Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants