From 7ea07a6405f886a2f18f7497cb5416de1439e61d Mon Sep 17 00:00:00 2001
From: Stefan Junker <mail@stefanjunker.de>
Date: Tue, 21 Jun 2016 16:28:38 -0700
Subject: [PATCH] pkg/ipam{,test}: improve error handling and tests

* _suite.go and _test.go file should be in the same package, using the
  _test package for that, which requires some fields and methods to be
  exported
* Introduce error type for cleaner error handling
* test adaptions for error type checking
---
 ipam/ipam.go      | 39 +++++++++++++++++++++++++++++++--------
 ipam/ipam_test.go | 23 +++++++++++++++--------
 2 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/ipam/ipam.go b/ipam/ipam.go
index 7777e6f6..74f937d7 100644
--- a/ipam/ipam.go
+++ b/ipam/ipam.go
@@ -30,7 +30,7 @@ import (
 
 const (
 	// private mac prefix safe to use
-	privateMACPrefix = "0a:58"
+	PrivateMACPrefix = "0a:58"
 
 	// veth link dev type
 	vethLinkType = "veth"
@@ -58,7 +58,7 @@ func ConfigureIface(ifName string, res *types.Result) error {
 
 	// only set hardware address to veth when using ipv4
 	if link.Type() == vethLinkType && res.IP4 != nil {
-		hwAddr, err := generateHardwareAddr(res.IP4.IP.IP)
+		hwAddr, err := GenerateHardwareAddr4(res.IP4.IP.IP, PrivateMACPrefix)
 		if err != nil {
 			return fmt.Errorf("failed to generate hardware addr: %v", err)
 		}
@@ -89,20 +89,43 @@ func ConfigureIface(ifName string, res *types.Result) error {
 	return nil
 }
 
-// generateHardwareAddr generates 48 bit virtual mac addresses based on the IP input.
-func generateHardwareAddr(ip net.IP) (net.HardwareAddr, error) {
-	if ip.To4() == nil {
-		return nil, fmt.Errorf("generateHardwareAddr only support valid ipv4 address as input")
+type SupportIp4OnlyErr struct{ msg string }
+
+func (e SupportIp4OnlyErr) Error() string { return e.msg }
+
+type MacParseErr struct{ msg string }
+
+func (e MacParseErr) Error() string { return e.msg }
+
+type InvalidPrefixLengthErr struct{ msg string }
+
+func (e InvalidPrefixLengthErr) Error() string { return e.msg }
+
+// GenerateHardwareAddr4 generates 48 bit virtual mac addresses based on the IP4 input.
+func GenerateHardwareAddr4(ip net.IP, prefix string) (net.HardwareAddr, error) {
+	switch {
+
+	case ip.To4() == nil:
+		return nil, SupportIp4OnlyErr{msg: "GenerateHardwareAddr4 only supports valid IPv4 address as input"}
+
+	case len(prefix) != len(PrivateMACPrefix):
+		return nil, InvalidPrefixLengthErr{msg: fmt.Sprintf(
+			"Prefix has length %d instead  of %d", len(prefix), len(PrivateMACPrefix)),
+		}
 	}
-	mac := privateMACPrefix
+
+	mac := prefix
 	sections := strings.Split(ip.String(), ".")
 	for _, s := range sections {
 		i, _ := strconv.Atoi(s)
 		mac = mac + ":" + fmt.Sprintf("%02x", i)
 	}
+
 	hwAddr, err := net.ParseMAC(mac)
 	if err != nil {
-		return nil, fmt.Errorf("Failed to parse mac address %s generated based on ip %s due to: %v", mac, ip, err)
+		return nil, MacParseErr{msg: fmt.Sprintf(
+			"Failed to parse mac address %q generated based on IP %q due to: %v", mac, ip, err),
+		}
 	}
 	return hwAddr, nil
 }
diff --git a/ipam/ipam_test.go b/ipam/ipam_test.go
index 0cacd08f..1390ccec 100644
--- a/ipam/ipam_test.go
+++ b/ipam/ipam_test.go
@@ -12,16 +12,18 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package ipam
+package ipam_test
 
 import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"net"
+
+	"github.com/containernetworking/cni/pkg/ipam"
 )
 
 var _ = Describe("ipam utils", func() {
-	Context("Generate Hardware Addrress", func() {
+	Context("Generate Hardware Address", func() {
 		It("generate hardware address based on ipv4 address", func() {
 			testCases := []struct {
 				ip          net.IP
@@ -29,20 +31,20 @@ var _ = Describe("ipam utils", func() {
 			}{
 				{
 					ip:          net.ParseIP("10.0.0.2"),
-					expectedMAC: privateMACPrefix + ":0a:00:00:02",
+					expectedMAC: ipam.PrivateMACPrefix + ":0a:00:00:02",
 				},
 				{
 					ip:          net.ParseIP("10.250.0.244"),
-					expectedMAC: privateMACPrefix + ":0a:fa:00:f4",
+					expectedMAC: ipam.PrivateMACPrefix + ":0a:fa:00:f4",
 				},
 				{
 					ip:          net.ParseIP("172.17.0.2"),
-					expectedMAC: privateMACPrefix + ":ac:11:00:02",
+					expectedMAC: ipam.PrivateMACPrefix + ":ac:11:00:02",
 				},
 			}
 
 			for _, tc := range testCases {
-				mac, err := generateHardwareAddr(tc.ip)
+				mac, err := ipam.GenerateHardwareAddr4(tc.ip, ipam.PrivateMACPrefix)
 				Expect(err).NotTo(HaveOccurred())
 				Expect(mac.String()).To(Equal(tc.expectedMAC))
 			}
@@ -54,9 +56,14 @@ var _ = Describe("ipam utils", func() {
 				net.ParseIP("2001:db8:0:1:1:1:1:1"),
 			}
 			for _, tc := range testCases {
-				_, err := generateHardwareAddr(tc)
-				Expect(err.Error()).To(Equal("generateHardwareAddr only support valid ipv4 address as input"))
+				_, err := ipam.GenerateHardwareAddr4(tc, ipam.PrivateMACPrefix)
+				Expect(err).To(BeAssignableToTypeOf(ipam.SupportIp4OnlyErr{}))
 			}
 		})
+
+		It("return error if prefix is invalid", func() {
+			_, err := ipam.GenerateHardwareAddr4(net.ParseIP("10.0.0.2"), "")
+			Expect(err).To(BeAssignableToTypeOf(ipam.InvalidPrefixLengthErr{}))
+		})
 	})
 })