itopo: Add svc validator

[oncilla]

Add validator for infra services according to itopo/doc.go

---

This change is 

[scrye]

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

---

go/lib/infra/modules/itopo/validate.go, line 130 at r1 (raw file):

// It checks that the service is present, and only permissible updates are done.
type svcValidator struct {
	generalValidator

Do you care about embedding here (i.e., is generalValidator implementing some vanilla aspect that you can use without changing it at all)?

If you never call methods on svcValidator that go directly to generalValidator, changing generalValidator to a normal field is a bit safer because it makes code easier to refactor in the future (it means future changes to generalValidator won't inadvertently enrich the API of svcValidator, and no accidental calls can be made go generalValidators methods).

---

go/lib/topology/topology.go, line 287 at r1 (raw file):

}

func (t *Topo) GetTopoAddrs(id string, svc proto.ServiceType) (*TopoAddr, error) {

I decided not to comment on the last PR, but I think it's clearer if this were named GetTopoAddr, as it only returns one TopoAddr type object.

[oncilla]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)

---

go/lib/infra/modules/itopo/validate.go, line 130 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Do you care about embedding here (i.e., is generalValidator implementing some vanilla aspect that you can use without changing it at all)?

If you never call methods on svcValidator that go directly to generalValidator, changing generalValidator to a normal field is a bit safer because it makes code easier to refactor in the future (it means future changes to generalValidator won't inadvertently enrich the API of svcValidator, and no accidental calls can be made go generalValidators methods).

We directly reuse SemiMutable and MustDropDynamic.
I can also re-implement, if you want.

(I have no strong opinion whether it should be an embedding or field.)

---

go/lib/topology/topology.go, line 287 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

I decided not to comment on the last PR, but I think it's clearer if this were named GetTopoAddr, as it only returns one TopoAddr type object.

Done.

[scrye]

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

---

go/lib/infra/modules/itopo/validate.go, line 130 at r1 (raw file):

Previously, Oncilla wrote…

We directly reuse SemiMutable and MustDropDynamic.
I can also re-implement, if you want.

(I have no strong opinion whether it should be an embedding or field.)

Nah, it's fine, generalValidator is so basic that refactoring probably won't be a problem, so if the embedding saves a few lines of code it's probably a gain.

[scrye]

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @oncilla)

---

acceptance/topo_invalid_reloads_acceptance/test, line 57 at r3 (raw file):

    ./tools/dc scion kill -s HUP scion_"$1"
    sleep 1
    grep -q "Reloaded topology" "logs/$1.log" || local failed=$?

It's safer to test for the presence of the Unable to set topology message, because there are multiple branches that can exit ReloadTopo without the topology getting successfully reloaded.

[oncilla]

Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @scrye)

---

acceptance/topo_invalid_reloads_acceptance/test, line 57 at r3 (raw file):

Previously, scrye (Sergiu Costea) wrote…

It's safer to test for the presence of the Unable to set topology message, because there are multiple branches that can exit ReloadTopo without the topology getting successfully reloaded.

Done.

[scrye]

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