Merge pull request #67 from jacobweinstock/fix-relay-interactions

Unicast to the giaddr address when non zero:

## Description

<!--- Please describe what this PR is going to change -->
When the DHCP header `giaddr` is set, we must unicast responses to that address. [DHCP RFC](https://www.ietf.org/rfc/rfc2131.txt) section 4.1, page 22:

> "If the 'giaddr' field in a DHCP message from a client is non-zero, the server sends any return messages to the 'DHCP server' port on the BOOTP relay agent whose address appears in 'giaddr'."

In addition to not being to spec, sending responses to the direct peer causes that a relay agent that sent a unicast request doesn't receive the expected unicast response if there is more than one relay in the mix. This is observed with the v0.4.2 Tink stack helm chart as we deploy a DHCP relay as part of the stack.

Current Traffic flow for the Helm chart deployment. Traffic from the relay container (inside the tink stack deployment) doesn't unicast responses to the network's relay agent.

```bash
                                                                                ▲                                              
                                                                                │                                              
                                                                                │                                              
                                                                            broadcast                                          
                                                                                │                                              
                                                                                │                                              
┌───────────────┐                   ┌───────────────┐                   ┌───────────────┐                     ┌───────────────┐
│    machine    │◀─────broadcast───▶│     relay     │──────unicast─────▶│relay container│◀──────unicast──────▶│     Smee      │
└───────────────┘                   └───────────────┘                   └───────────────┘                     └───────────────┘
```

## Why is this needed

<!--- Link to issue you have raised -->

Fixes: #tinkerbell/smee#382

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
Manually testing using the v0.4.2 helm chart.

Traffic will now make it back to the network's relay agent:

```ascii
                                                          ┌───────────────┐                 
                                             ┌────────────│     Smee      │◀───────┐        
                                             │            └───────────────┘        │        
                                          unicast                               unicast     
                                             │                                     │        
                                             ▼                                     │        
┌───────────────┐                    ┌───────────────┐                     ┌───────────────┐
│    machine    │◀─────broadcast────▶│     relay     │───────unicast──────▶│relay container│
└───────────────┘                    └───────────────┘                     └───────────────┘
```


## How are existing users impacted? What migration steps/scripts do we need?

<!--- Fixes a bug, unblocks installation, removes a component of the stack etc -->
<!--- Requires a DB migration script, etc. -->


## Checklist:

I have:

- [ ] updated the documentation and/or roadmap (if required)
- [ ] added unit or e2e tests
- [ ] provided instructions on how to upgrade

handler/reservation/handler.go

@@ -124,12 +124,15 @@ func (h *Handler) Handle(ctx context.Context, conn *ipv4.PacketConn, p data.Pack
 	if ns := reply.ServerIPAddr; ns != nil {
 		log = log.WithValues("nextServer", ns.String())
 	}
-	log = log.WithValues("ipAddress", reply.YourIPAddr.String(), "destination", p.Peer.String())
+
+	dst := replyDestination(p.Peer, p.Pkt.GatewayIPAddr)
+	log = log.WithValues("ipAddress", reply.YourIPAddr.String(), "destination", dst.String())
 	cm := &ipv4.ControlMessage{}
 	if p.Md != nil {
 		cm.IfIndex = p.Md.IfIndex
 	}
-	if _, err := conn.WriteTo(reply.ToBytes(), cm, p.Peer); err != nil {
+
+	if _, err := conn.WriteTo(reply.ToBytes(), cm, dst); err != nil {
 		log.Error(err, "failed to send DHCP")
 		span.SetStatus(codes.Error, err.Error())
 
@@ -141,6 +144,22 @@ func (h *Handler) Handle(ctx context.Context, conn *ipv4.PacketConn, p data.Pack
 	span.SetStatus(codes.Ok, "sent DHCP response")
 }
 
+// replyDestination determines the destination address for the DHCP reply.
+// If the giaddr is set, then the reply should be sent to the giaddr.
+// Otherwise, the reply should be sent to the direct peer.
+//
+// From page 22 of https://www.ietf.org/rfc/rfc2131.txt:
+// "If the 'giaddr' field in a DHCP message from a client is non-zero,
+// the server sends any return messages to the 'DHCP server' port on
+// the BOOTP relay agent whose address appears in 'giaddr'.".
+func replyDestination(directPeer net.Addr, giaddr net.IP) net.Addr {
+	if !giaddr.IsUnspecified() && giaddr != nil {
+		return &net.UDPAddr{IP: giaddr, Port: dhcpv4.ServerPort}
+	}
+
+	return directPeer
+}
+
 // readBackend encapsulates the backend read and opentelemetry handling.
 func (h *Handler) readBackend(ctx context.Context, mac net.HardwareAddr) (*data.DHCP, *data.Netboot, error) {
 	h.setDefaults()

handler/reservation/handler_test.go

@@ -275,10 +275,6 @@ func TestHandle(t *testing.T) {
 			want:    nil,
 			wantErr: errBadBackend,
 		},
-		/*"nil incoming packet": {
-			want:    nil,
-			wantErr: errBadBackend,
-		},*/
 		"failure no hardware found discover": {
 			server: Handler{
 				Backend: &mockBackend{hardwareNotFound: true},
@@ -601,3 +597,33 @@ func TestEncodeToAttributes(t *testing.T) {
 		})
 	}
 }
+
+func TestReplyDestination(t *testing.T) {
+	tests := map[string]struct {
+		directPeer net.Addr
+		giaddr     net.IP
+		want       net.Addr
+	}{
+		"direct peer": {
+			directPeer: &net.UDPAddr{IP: net.IP{192, 168, 1, 100}, Port: 68},
+			want:       &net.UDPAddr{IP: net.IP{192, 168, 1, 100}, Port: 68},
+		},
+		"direct peer with unspecified giaddr": {
+			directPeer: &net.UDPAddr{IP: net.IP{192, 168, 1, 99}, Port: 68},
+			giaddr:     net.IPv4zero,
+			want:       &net.UDPAddr{IP: net.IP{192, 168, 1, 99}, Port: 68},
+		},
+		"giaddr": {
+			giaddr: net.IP{192, 168, 2, 1},
+			want:   &net.UDPAddr{IP: net.IP{192, 168, 2, 1}, Port: 67},
+		},
+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := replyDestination(tt.directPeer, tt.giaddr)
+			if diff := cmp.Diff(got, tt.want); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+	}
+}