Reorganize code in preparation for scamper2 datatype

[SaiedKazemi]

This commit does not change the functionality of traceroute-caller. It only reorganizes its code so that regular traceroutes can be easily added in a separate commit.

Changes were tested locally via go test and also docker-compose.

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 423

• 124 of 128 (96.88%) changed or added relevant lines in 7 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.4%) to 96.181%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
caller.go	14	16	87.5%
tracer/scamper.go	51	53	96.23%
<!--	Total:	124	128	96.88%	-->

Totals
Change from base Build 415:	-0.4%
Covered Lines:	554
Relevant Lines:	576

---

💛 - Coveralls

[SaiedKazemi]

---

caller.go, line 33 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

In context, if this were "tracelb" it would be obvious to a reader that the "*Tracelb*" variables are related to this mode. It's not clear how a reader would otherwise know to associate "mda" with these other options. Also, please consider using the https://pkg.go.dev/github.com/m-lab/go@v0.1.45/flagx#Enum to limit options to pre-defined values.

Changed the message to show "tracelb" corresponds to the "mda" traceroute type.

Used flagx's Enum.

[SaiedKazemi]

---

caller.go, line 73 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

This feels weird. Is the issue that the logic later may start two processes that both try to create this directory and one may fail? I would rather see the directly created explicitly or from the Dockerfile before starting this process. `main` does not feel like the right place.

I agree that this is an environment issue that doesn't belong to traceroute-caller. However, because traceroute-caller can be invoked directly, Dockerfile won't help. That's why I put the workaround here until scamper is fixed but I have removed it now.

[SaiedKazemi]

---

caller.go, line 90 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

As discussed, it would be great if we could add a bit more context to these errors, similar to line 113.

Done.

[SaiedKazemi]

---

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

Previously, cristinaleonr (Cristina Leon) wrote…

Nitpick: missing period.

Done.

[SaiedKazemi]

---

parser/parser.go, line 33 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Saied, You mentioned this a few times, but I thought it was in reference to the old parser schema. For a new schema, I would recommend treating all fields consistently for the schema. Because the v2 pipeline writes JSONL to GCS, and BigQuery loads the data from there, I *think* we want to add `bigquery` tags everywhere that mirror the JSON tag name. Otherwise, when we create the schema using `update-schema` command line tool from m-lab/etl it will create the CamelCase field names for those without a bigquery tag, and the BigQuery load will fail to find all the data.. I *think*.

Struct fields tags in parser.go now include bigquery:"xxx".

[SaiedKazemi]

---

parser/parser.go, line 40 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Same as above. Also, looks like`Hostname` should be capitalized.

Done.

[SaiedKazemi]

---

tracer/scamper.go, line 54 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I didn't realize until I read the code that this writes a file.

The comment was from before. I have now updated it to explicitly mention that it writes a file.

[SaiedKazemi]

---

tracer/scamper.go, line 111 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

If this were me, rather than adding additional logic for the function under test, I might create a super simple shell script that "does the right thing" if "echo, yes, and false` all do the wrong thing. Then this condition is not needed.

Done.

[SaiedKazemi]

I have added extra tests for higher coverage. The only lines that are not covered now are the lines inside if conditions for errors that cannot be easily forced via package testing code.

ok github.com/m-lab/traceroute-caller 1.058s coverage: 87.5% of statements ok github.com/m-lab/traceroute-caller/hopannotation 0.646s coverage: 93.6% of statements ok github.com/m-lab/traceroute-caller/ipcache 3.020s coverage: 100.0% of statements ok github.com/m-lab/traceroute-caller/parser 0.034s coverage: 100.0% of statements ok github.com/m-lab/traceroute-caller/tracer 1.046s coverage: 98.9% of statements ok github.com/m-lab/traceroute-caller/triggertrace 0.088s coverage: 96.3% of statements

[SaiedKazemi]

---

tracer/scamper.go, line 34 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please add a unit test where `s.Binary` is a plain file that is not executable.

Done.

[SaiedKazemi]

---

tracer/scamper.go, line 39 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

What is the code path that makes this option possible? Why do we want to support this as valid input?

While TRC recognizes "mda" and "regular" in lower case as valid types, the tracer package can be used by other programs. too. And because MDA is an acronym, the intention was for the package to allow both "mda" and "MDA". But I removed the case for "MDA".

[SaiedKazemi]

---

tracer/scamper.go, line 36 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

What if this is a named pipe or a device file? There exists a simpler and complete condition.

Done.

[SaiedKazemi]

---

tracer/scamper.go, line 40 at r4 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

You're clearly in my wheel house. :) You don't have to solve this in general, but just to point out that this is more complex still. What would this return for a non-owner user? e.g. binary is owned by root, but not executable by group or users, e.g. 0700. This should not impact M-Lab's use-case b/c we run the containers as uid:0.

Sorry Stephen, I am not sure if I understand.

If mode is 0700, all is OK because fileMore&0100 will be 1 and the if condition isn't true. Isn't this what we want?

[SaiedKazemi]

Thanks for LGTM but I am not merging because, sorry, I don't understand your comment on fileMode&0100.

[SaiedKazemi]

---

tracer/scamper.go, line 40 at r4 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Could non-root, non-owner user running this command execute a binary with mode 0700? (I read this as a false-positive for a `Validate` method).

The non-root, non-owner user will get a permission denied error and, in that sense, you're right that Validate() can be seen as a false-positive method.

I should have commented that, in its current form, Validate() is meant to validate the traceroute type and, although it checks that the scamper binary exists,Validate() is not meant to be a full-fledged validator. If some parameters are invalid (e.g., OutputPath), they will cause an error (which will be handled appropriately) elsewhere in the code.

In light of this feedback, I will add complete checks in a separate PR.

[SaiedKazemi]

Thank you both for your reviews.