router: refactor phase 2

[jiceatscion]

• Moved the sender and receiver tasks to the underlay. Metrics and a a few other things had to change a bit as a result.
• Got sick of maintaining the fiction that a zero-valued Dataplane makes sense. so created constructors and remove the bug-prone and merit-less lazy initializations. While at it, unexported the dataPlane struct altogether so there cannot be any ctor bypassing in the future.
• Moved the udpip provider to its own subdirectory.
• Tests adjusted as needed.

contributes to: #4593

[jiceatscion]

This change is 

[jiceatscion]

Reviewable status: 0 of 16 files reviewed, 1 unresolved discussion (waiting on @romshark)

[marcfrei]

Reviewed 7 of 16 files at r1, 1 of 7 files at r2, 2 of 3 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @romshark)

---

router/dataplane.go line 246 at r4 (raw file):

	zeroBuffer = make([]byte, 16)

	theMetrics = NewMetrics() // There can be only one.

Just metrics

Code quote:

	theMetrics

---

router/dataplane.go line 265 at r4 (raw file):

// struct for anything. Support for lazy initialization has been removed. It was much too
// bug-friendly.
func NewDataPlane(runConfig RunConfig, authSCMP bool) *dataPlane {

newDataPlane

Code quote:

NewDataPlane

---

router/dataplane.go line 268 at r4 (raw file):

	x := makeDataPlane(runConfig, authSCMP)
	return &x
}

Make it a one-liner?

Code quote:

	x := makeDataPlane(runConfig, authSCMP)
	return &x
}

---

router/underlayproviders/udpip/udpip.go line 182 at r4 (raw file):

}

func (u *udpConnection) receiver(

runReceiver?

Code quote:

receiver

---

router/underlayproviders/udpip/udpip.go line 309 at r4 (raw file):

// For now, we do the first option. Whether that is good enough is still TBD.

func (u *udpConnection) sender(batchSize int, pool chan *router.Packet) {

runSender?

Code quote:

sender

---

router/metrics.go line 391 at r4 (raw file):

// UpdateOutputMetrics updates the given InterfaceMetrics in bulk according
// to the given set of just sent packets. This much faster than looking up

"This is"?

Code quote:

This much

---

router/export_test.go line 112 at r4 (raw file):

	}
	for i, addr := range internalNextHops {
		dp.addForwardingMetrics(i, External)

Is this correct?

Code quote:

External

---

router/export_test.go line 143 at r4 (raw file):

	dp := makeDP(external, linkTypes, internal, internalNextHops, svc, local, neighbors, key)
	return &dp

Could be one line, right?

Maybe also in the following functions.

Code quote:

	dp := makeDP(external, linkTypes, internal, internalNextHops, svc, local, neighbors, key)
	return &dp

---

router/export_test.go line 173 at r4 (raw file):

// to non-internal tests that do not want any dataplane configuration beyond the strictly necessary.
// This is equivalent to router.NewDataPlane, but returns an exported type.
func NewDPraw(runConfig RunConfig, authSCMP bool) *DataPlane {

NewDPRaw

Code quote:

NewDPraw

[jiceatscion]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @marcfrei and @romshark)

---

router/dataplane.go line 268 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Make it a one-liner?

As stupid as it seems, you can take the address of a composite litteralk, but you cannot take the address of a returned value. I may be mistaken, but this is the case here and in the other places that you pointed out.

---

router/underlayproviders/udpip/udpip.go line 309 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

runSender?

Is there some idiomatic tradition about that? That seems unusually verbose by go standards.

[marcfrei]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jiceatscion and @romshark)

---

router/underlayproviders/udpip/udpip.go line 309 at r4 (raw file):

Previously, jiceatscion wrote…

Is there some idiomatic tradition about that? That seems unusually verbose by go standards.

Just as a counterpart to the old runReceiver. Other than that, I don't know, there's "Functions that do something are given verb-like names"(https://google.github.io/styleguide/go/best-practices.html#naming-conventions) and maybe things like https://pkg.go.dev/testing#RunBenchmarks

[jiceatscion]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @marcfrei and @romshark)

---

router/dataplane.go line 246 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Just metrics

done

---

router/dataplane.go line 265 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

newDataPlane

We can do that, but only because we instantiate the router via the Connector wrapper. Which should logically be optional. I think it should be possible to instantiate the router from a different package without the Connector wrapper.

---

router/export_test.go line 112 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Is this correct?

Done. Thanks, it wasn't.

---

router/export_test.go line 143 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Could be one line, right?

Maybe also in the following functions.

cannot.

---

router/export_test.go line 173 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

NewDPRaw

Done.

---

router/metrics.go line 391 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

"This is"?

Done.

---

router/underlayproviders/udpip/udpip.go line 182 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

runReceiver?

See next question.

---

router/underlayproviders/udpip/udpip.go line 309 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Just as a counterpart to the old runReceiver. Other than that, I don't know, there's "Functions that do something are given verb-like names"(https://google.github.io/styleguide/go/best-practices.html#naming-conventions) and maybe things like https://pkg.go.dev/testing#RunBenchmarks

Ah, I see. OK for the verb naming. I am proposing "receive" and "send"... why complicate matters?

[jiceatscion]

Reviewable status: 11 of 17 files reviewed, 11 unresolved discussions (waiting on @marcfrei and @romshark)

[marcfrei]

Reviewed 5 of 6 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jiceatscion and @romshark)

---

router/dataplane.go line 265 at r4 (raw file):

Previously, jiceatscion wrote…

We can do that, but only because we instantiate the router via the Connector wrapper. Which should logically be optional. I think it should be possible to instantiate the router from a different package without the Connector wrapper.

I suggest to make it consistent. Either export the function and the data type it returns or keep both private.

[jiceatscion]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @marcfrei and @romshark)

---

router/dataplane.go line 265 at r4 (raw file):

Previously, marcfrei (Marc Frei) wrote…

I suggest to make it consistent. Either export the function and the data type it returns or keep both private.

Ah, yes Go's special relationship with opaque types. I guess the correct thing to do would be to create a DataPlane interface (one that is not the connector API). Since I don't want to do that, I'll grant you your wish.

[jiceatscion]

Reviewable status: 14 of 17 files reviewed, 8 unresolved discussions (waiting on @marcfrei and @romshark)

---

router/dataplane.go line 265 at r4 (raw file):

Previously, jiceatscion wrote…

Ah, yes Go's special relationship with opaque types. I guess the correct thing to do would be to create a DataPlane interface (one that is not the connector API). Since I don't want to do that, I'll grant you your wish.

done

[marcfrei]

Reviewable status: 14 of 17 files reviewed, 7 unresolved discussions (waiting on @romshark)

---

router/dataplane.go line 265 at r4 (raw file):

Previously, jiceatscion wrote…

done

:-)

[romshark]

LGTM

[romshark]

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)