From b5236c3ca4bff1a0c5c706c91f150372ece0987e Mon Sep 17 00:00:00 2001
From: Simon Mayer <Simon.Mayer@x-cellent.com>
Date: Fri, 8 Nov 2024 08:29:10 +0100
Subject: [PATCH] Implement review

---
 .../internal/service/machine-service.go       | 55 +++++++++++--------
 .../internal/service/partition-service.go     | 18 ++++--
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/cmd/metal-api/internal/service/machine-service.go b/cmd/metal-api/internal/service/machine-service.go
index 4c86f966..78e098b7 100644
--- a/cmd/metal-api/internal/service/machine-service.go
+++ b/cmd/metal-api/internal/service/machine-service.go
@@ -1152,22 +1152,18 @@ func createMachineAllocationSpec(ds *datastore.RethinkStore, machineRequest v1.M
 		return nil, fmt.Errorf("partition:%s not found err:%w", partitionID, err)
 	}
 	var (
-		dnsServers metal.DNSServers
-		ntpServers metal.NTPServers
+		dnsServers = partition.DNSServers
+		ntpServers = partition.NTPServers
 	)
 	if len(machineRequest.DNSServers) != 0 {
 		if len(machineRequest.DNSServers) > 3 {
 			return nil, errors.New("please specify a maximum of three dns servers")
 		}
 		dnsServers = machineRequest.DNSServers
-	} else {
-		dnsServers = partition.DNSServers
 	}
-	for _, dnsServer := range dnsServers {
-		_, err := netip.ParseAddr(dnsServer.IP)
-		if err != nil {
-			return nil, fmt.Errorf("ip: %s for dns server not correct err: %w", dnsServer, err)
-		}
+
+	if err := ValidateDNSServers(dnsServers); err != nil {
+		return nil, err
 	}
 
 	if len(machineRequest.NTPServers) != 0 {
@@ -1175,21 +1171,10 @@ func createMachineAllocationSpec(ds *datastore.RethinkStore, machineRequest v1.M
 			return nil, errors.New("please specify a minimum of 3 and a maximum of 5 ntp servers")
 		}
 		ntpServers = machineRequest.NTPServers
-	} else {
-		ntpServers = partition.NTPServers
 	}
 
-	for _, ntpserver := range ntpServers {
-		if net.ParseIP(ntpserver.Address) != nil {
-			_, err := netip.ParseAddr(ntpserver.Address)
-			if err != nil {
-				return nil, fmt.Errorf("ip: %s for ntp server not correct err: %w", ntpserver, err)
-			}
-		} else {
-			if !govalidator.IsDNSName(ntpserver.Address) {
-				return nil, fmt.Errorf("dns name: %s for ntp server not correct err: %w", ntpserver, err)
-			}
-		}
+	if err := ValidateNTPServers(ntpServers); err != nil {
+		return nil, err
 	}
 
 	return &machineAllocationSpec{
@@ -2527,3 +2512,29 @@ func (s machineAllocationSpec) noautoNetworkN() int {
 func (s machineAllocationSpec) autoNetworkN() int {
 	return len(s.Networks) - s.noautoNetworkN()
 }
+
+func ValidateDNSServers(d metal.DNSServers) error {
+	for _, dnsServer := range d {
+		_, err := netip.ParseAddr(dnsServer.IP)
+		if err != nil {
+			return fmt.Errorf("ip: %s for dns server not correct err: %w", dnsServer, err)
+		}
+	}
+	return nil
+}
+
+func ValidateNTPServers(dnsServers metal.NTPServers) error {
+	for _, ntpserver := range dnsServers {
+		if net.ParseIP(ntpserver.Address) != nil {
+			_, err := netip.ParseAddr(ntpserver.Address)
+			if err != nil {
+				return fmt.Errorf("ip: %s for ntp server not correct err: %w", ntpserver, err)
+			}
+		} else {
+			if !govalidator.IsDNSName(ntpserver.Address) {
+				return fmt.Errorf("dns name: %s for ntp server not correct", ntpserver)
+			}
+		}
+	}
+	return nil
+}
diff --git a/cmd/metal-api/internal/service/partition-service.go b/cmd/metal-api/internal/service/partition-service.go
index 5172490e..d31e8e61 100644
--- a/cmd/metal-api/internal/service/partition-service.go
+++ b/cmd/metal-api/internal/service/partition-service.go
@@ -206,13 +206,23 @@ func (r *partitionResource) createPartition(request *restful.Request, response *
 	}
 
 	var dnsServers metal.DNSServers
-	if len(requestPayload.DNSServers) > 0 {
-		dnsServers = requestPayload.DNSServers
+	reqDNSServers := requestPayload.DNSServers
+	if len(reqDNSServers) > 0 {
+		if err := ValidateDNSServers(reqDNSServers); err != nil {
+			r.sendError(request, response, httperrors.BadRequest(err))
+			return
+		}
+
+		dnsServers = reqDNSServers
 	}
 
 	var ntpServers metal.NTPServers
-	if len(requestPayload.NTPServers) > 0 {
-		ntpServers = requestPayload.NTPServers
+	reqNtpServers := requestPayload.NTPServers
+	if len(ntpServers) > 0 {
+		if err := ValidateNTPServers(reqNtpServers); err != nil {
+			r.sendError(request, response, httperrors.BadRequest(err))
+		}
+		ntpServers = reqNtpServers
 	}
 
 	p := &metal.Partition{