Initialize project

[robertodauria]

This PR imports some initial code for the switch-monitoring daemon.

The only switch where NETCONF is enabled already is s1.lga0t.

Implemented:

• Retrieving the switches list from siteinfo (the project can be specified with -project)
• Getting the configuration, cleaning it from comments/blank lines, replacing the actual encrypted password with dummy and generating a SHA256 hash

Not implemented yet:

• Retrieving the expected configuration for a switch from GCS
• Comparing expected vs actual hashes
• Actually looping over the switches list

---

This change is 

[stephen-soltesz]

No tests? :)

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nkinkade, @robertodauria, and @stephen-soltesz)

---

.gitignore, line 17 at r1 (raw file):

# vendor/

.vscode/

nit: weird eol spaces?

---

go.mod, line 1 at r1 (raw file):

module github.com/m-lab/switch-monitoring

Do we need go mod for this repo ? (no wrong answer, but could eliminate go.sum)

---

cmd/switch-monitoring/main.go, line 13 at r1 (raw file):

	"github.com/m-lab/go/flagx"
	"github.com/m-lab/go/rtx"
	"github.com/m-lab/switch-monitoring/cmd/switch-monitoring/client"

The location of these support packages seem out of the ordinary. I would expect to find them at the top level directory. Maybe in an internal/ toplevel subdirectory. Why subdirectories of cmd ?

---

cmd/switch-monitoring/main.go, line 35 at r1 (raw file):

	log.SetHandler(text.New(os.Stdout))

	flag.Parse()

flagDebug is referenced above before flags are parsed here.

---

cmd/switch-monitoring/main.go, line 45 at r1 (raw file):

	// Initialize Siteinfo provider and the NETCONF client.
	siteinfo := &siteinfo.Siteinfo{ProjectID: *flagProject}

That's a lot of "siteinfo". I suggest calling the struct siteinfo.Client{}. And, please use a different variable and package name.

---

cmd/switch-monitoring/switch-monitoring, line 0 at r1 (raw file):
This looks like a mistake? Please remove.

---

cmd/switch-monitoring/client/netconf.go, line 1 at r1 (raw file):

package client

I suggest naming this package netconf. Then, usage would look like netconf.Client or netconf.New.

---

cmd/switch-monitoring/client/netconf.go, line 54 at r1 (raw file):

	// Replace all password fields with "dummy".
	re = regexp.MustCompile("encrypted-password.+")
	config = re.ReplaceAllString(config, "encrypted-password \"dummy\";")

Can we identify the default password hash? (eventually?)

---

cmd/switch-monitoring/siteinfo/siteinfo.go, line 1 at r1 (raw file):

package siteinfo

This package makes me happy. Please consider making it part of m-lab/go. It's not impossible for jsonnet to generate Go structs for the output formats supported by m-lab/siteinfo.

---

cmd/switch-monitoring/siteinfo/siteinfo.go, line 32 at r1 (raw file):

}

func (s Siteinfo) Sites() ([]string, error) {

If you want to generalize this package for m-lab/go/siteinfo, I suggest naming the functions after the output format.

---

cmd/switch-monitoring/siteinfo/siteinfo.go, line 62 at r1 (raw file):

	url := fmt.Sprintf(baseURLFormat+"sites/switches.json", s.ProjectID)

	resp, err := http.Get(url)

If this package will be part of a long-lived daemon, then I suggest using http requests with timeouts. This could be by using a custom http.DefaultClient (no timeouts) or using http.NewRequestWithContext with a context timeout.

[nkinkade]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nkinkade and @robertodauria)

---

netconf/client.go, line 44 at r2 (raw file):

	// Remove comments (lines starting with '#') and trim the result.
	re := regexp.MustCompile("(?m)^#.*$")

Is this only intended to remove the first comment line in the file, inserted dynamically by JunOS?

[robertodauria]

@stephen-soltesz PTAL again?

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nkinkade and @stephen-soltesz)

---

go.mod, line 1 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Do we need go mod for this repo ? (no wrong answer, but could eliminate go.sum)

Done. Discussed on Slack, now it's apparently the recommended way to organize a new Go project (https://golang.org/doc/code.html#Organization)

---

cmd/switch-monitoring/main.go, line 13 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

The location of these support packages seem out of the ordinary. I would expect to find them at the top level directory. Maybe in an internal/ toplevel subdirectory. Why subdirectories of cmd ?

Done. I've moved them to an /internal folder. siteinfo could be moved to m-lab/go in future.

---

cmd/switch-monitoring/main.go, line 35 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

flagDebug is referenced above before flags are parsed here.

Done. That's embarassing. 🤕

---

cmd/switch-monitoring/main.go, line 45 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

That's a lot of "siteinfo". I suggest calling the struct siteinfo.Client{}. And, please use a different variable and package name.

Done. Now it's client and I've moved it to the switches() func since the logic to extract switch hostnames from the switches.json siteinfo format has been moved here to keep the siteinfo package cleaner.

---

cmd/switch-monitoring/switch-monitoring, line at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

This looks like a mistake? Please remove.

Done.

---

cmd/switch-monitoring/client/netconf.go, line 1 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I suggest naming this package netconf. Then, usage would look like netconf.Client or netconf.New.

Done.

---

cmd/switch-monitoring/client/netconf.go, line 54 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Can we identify the default password hash? (eventually?)

In the end we should be able to generate the encrypted password in genconfig.py, so the .conf we store in GCS is exactly the same as the one we get from the switch and we just need to check them for equality.

---

cmd/switch-monitoring/siteinfo/siteinfo.go, line 62 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

If this package will be part of a long-lived daemon, then I suggest using http requests with timeouts. This could be by using a custom http.DefaultClient (no timeouts) or using http.NewRequestWithContext with a context timeout.

Done. Now the HTTP client is passed when constructing a siteinfo.Client and configured in main.go with a default of 15 seconds.

---

netconf/client.go, line 44 at r2 (raw file):

Previously, nkinkade wrote…

Is this only intended to remove the first comment line in the file, inserted dynamically by JunOS?

@nkinkade Yes. I would compare everything else line-by-line, including comments. I've noticed that manual changes to a section that contains a comment may delete the comment. The idea is that switch configs are driven by the templates in switch-config (plus genconfig.py, or its future replacement) and manual changes should always trigger some sort of warning. At the same time, re-applying the standard configuration should be trivial.

[stephen-soltesz]

This PR is getting pretty big. Looks like ~1000 lines of Go.

I've added a few comments. My general sense is that the unit tests could be a little simpler.

Reviewed 3 of 8 files at r2, 6 of 14 files at r3.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nkinkade, @robertodauria, and @stephen-soltesz)

---

.travis.yml, line 10 at r3 (raw file):

script:
- go test -covermode=count -coverprofile=profile.cov ./...

Please add:

- go vet ./...
- go test ./... -race

These are becoming the norm among new projects.

---

internal/siteinfo/client_test.go, line 24 at r3 (raw file):

}

func (prov fileReaderProvider) Get(string) (*http.Response, error) {

Tests must be correct by inspection. There's a lot of machinery here. Can this be simpler?

[robertodauria]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nkinkade and @stephen-soltesz)

---

.travis.yml, line 10 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please add:

- go vet ./...
- go test ./... -race

These are becoming the norm among new projects.

Done.

---

internal/siteinfo/client_test.go, line 24 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Tests must be correct by inspection. There's a lot of machinery here. Can this be simpler?

What do you think of it now? I've removed all the conditionals by making different mock objects to test the various scenarios. Each mock object is (I believe) correct by inspection.

[stephen-soltesz]

- thank you for the test simplifications. maybe a general guideline for how to achieve simpler tests is that fewer branches in unit tests is better when possible by using literal, single-purpose objects.

Reviewed 5 of 14 files at r3, 1 of 2 files at r4.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @nkinkade and @stephen-soltesz)

---

internal/siteinfo/client_test.go, line 24 at r3 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

What do you think of it now? I've removed all the conditionals by making different mock objects to test the various scenarios. Each mock object is (I believe) correct by inspection.

I like it.