rules fix for pod level portmapping

1. fix the iptables rules
2. improve the rollback mechanisms
3. don't merge continuous port mapping rules in spec
4. fix the API server

Signed-off-by: Wang Xu <[email protected]>

Author: gnawux

Makefile.am

@@ -26,7 +26,7 @@ if ON_DARWIN
 SUBDIRS=mac_installer
 endif
 
-VERSION_PARAM=-ldflags "-X github.com/hyperhq/hyperd/utils.VERSION=$(VERSION) -X github.com/hyperhq/hyperd/utils.GITCOMMIT=`git describe`"
+VERSION_PARAM=-ldflags "-X github.com/hyperhq/hyperd/utils.VERSION=$(VERSION) -X github.com/hyperhq/hyperd/utils.GITCOMMIT=`git describe --dirty --always --tags 2> /dev/null || true`"
 
 all-local: build-hyperd build-hyperctl build-vmlogd
 clean-local:

networking/portmapping/host_portmapping_linux.go

@@ -78,6 +78,7 @@ func generateIptablesArgs(containerip string, m *PortMapping) ([]string, []strin
 		proto string
 		from  string
 		to    string
+		dport string
 	)
 
 	if strings.EqualFold(m.Protocol, "udp") {
@@ -90,16 +91,18 @@ func generateIptablesArgs(containerip string, m *PortMapping) ([]string, []strin
 		from = strconv.Itoa(m.FromPorts.Begin)
 		m.FromPorts.End = m.FromPorts.Begin
 	} else if m.FromPorts.End > m.FromPorts.Begin {
-		from = fmt.Sprintf("%d:%d", m.FromPorts, m.FromPorts.End)
+		from = fmt.Sprintf("%d:%d", m.FromPorts.Begin, m.FromPorts.End)
 	} else {
 		return []string{}, []string{}, fmt.Errorf("invalid from port range %d-%d", m.FromPorts.Begin, m.FromPorts.End)
 	}
 
 	if m.ToPorts.End == 0 || m.ToPorts.End == m.ToPorts.Begin {
-		to = net.JoinHostPort(containerip, strconv.Itoa(m.ToPorts.Begin))
+		dport = strconv.Itoa(m.ToPorts.Begin)
+		to = net.JoinHostPort(containerip, dport)
 		m.ToPorts.End = m.ToPorts.Begin
 	} else if m.ToPorts.End > m.ToPorts.Begin {
-		to = net.JoinHostPort(containerip, fmt.Sprintf("%d-%d", m.ToPorts, m.ToPorts.End))
+		dport = fmt.Sprintf("%d:%d", m.ToPorts.Begin, m.ToPorts.End)
+		to = net.JoinHostPort(containerip, fmt.Sprintf("%d-%d", m.ToPorts.Begin, m.ToPorts.End))
 	} else {
 		return []string{}, []string{}, fmt.Errorf("invalid to port range %d-%d", m.ToPorts.Begin, m.ToPorts.End)
 	}
@@ -112,42 +115,74 @@ func generateIptablesArgs(containerip string, m *PortMapping) ([]string, []strin
 	}
 
 	natArgs := []string{"-p", proto, "-m", proto, "--dport", from, "-j", "DNAT", "--to-destination", to}
-	filterArgs := []string{"-d", containerip, "-p", proto, "-m", proto, "--dport", to, "-j", "ACCEPT"}
+	filterArgs := []string{"-d", containerip, "-p", proto, "-m", proto, "--dport", dport, "-j", "ACCEPT"}
 
 	return natArgs, filterArgs, nil
 }
 
+func parseRawResultOnHyper(output []byte, err error) error {
+	if err != nil {
+		return err
+	} else if len(output) != 0 {
+		return &iptables.ChainError{Chain: "HYPER", Output: output}
+
+	}
+	return nil
+}
+
 func SetupPortMaps(containerip string, maps []*PortMapping) error {
 	if disableIptables || len(maps) == 0 {
 		return nil
 	}
 
+	var (
+		revert      bool
+		revertRules = [][]string{}
+	)
+	defer func() {
+		if revert {
+			hlog.Log(hlog.WARNING, "revert portmapping rules...")
+			for _, r := range revertRules {
+				hlog.Log(hlog.INFO, "revert rule: %v", r)
+				err := parseRawResultOnHyper(iptables.Raw(r...))
+				if err != nil {
+					hlog.Log(hlog.ERROR, "failed to revert rule: %v", err)
+					err = nil //just ignore
+				}
+			}
+		}
+	}()
+
 	for _, m := range maps {
 
 		natArgs, filterArgs, err := generateIptablesArgs(containerip, m)
 		if err != nil {
+			revert = true
 			return err
 		}
 
 		//check if this rule has already existed
 		if iptables.PortMapExists("HYPER", natArgs) {
-			return nil
+			continue
 		}
 
 		if iptables.PortMapUsed("HYPER", m.Protocol, m.FromPorts.Begin, m.FromPorts.End) {
+			revert = true
 			return fmt.Errorf("Host port %v has aleady been used", m.FromPorts)
 		}
 
-		err = iptables.OperatePortMap(iptables.Insert, "HYPER", natArgs)
+		err = parseRawResultOnHyper(iptables.Raw(append([]string{"-t", "nat", "-I", "HYPER"}, natArgs...)...))
 		if err != nil {
-			return err
+			revert = true
+			return fmt.Errorf("Unable to setup NAT rule in HYPER chain: %s", err)
 		}
+		revertRules = append(revertRules, append([]string{"-t", "nat", "-D", "HYPER"}, natArgs...))
 
-		if output, err := iptables.Raw(append([]string{"-I", "HYPER"}, filterArgs...)...); err != nil {
-			return fmt.Errorf("Unable to setup forward rule in HYPER chain: %s", err)
-		} else if len(output) != 0 {
-			return &iptables.ChainError{Chain: "HYPER", Output: output}
+		if err = parseRawResultOnHyper(iptables.Raw(append([]string{"-I", "HYPER"}, filterArgs...)...)); err != nil {
+			revert = true
+			return fmt.Errorf("Unable to setup FILTER rule in HYPER chain: %s", err)
 		}
+		revertRules = append(revertRules, append([]string{"-D", "HYPER"}, filterArgs...))
 
 		i := m.FromPorts.Begin
 		j := m.ToPorts.Begin
@@ -163,6 +198,7 @@ func SetupPortMaps(containerip string, maps []*PortMapping) error {
 
 		for i <= m.FromPorts.End {
 			if err = PortMapper.AllocateMap(m.Protocol, i, containerip, j); err != nil {
+				revert = true
 				return err
 			}
 			i++
@@ -178,6 +214,7 @@ func ReleasePortMaps(containerip string, maps []*PortMapping) error {
 		return nil
 	}
 
+release_loop:
 	for _, m := range maps {
 		if !strings.EqualFold(m.Protocol, "udp") {
 			m.Protocol = "tcp"
@@ -191,7 +228,7 @@ func ReleasePortMaps(containerip string, maps []*PortMapping) error {
 		for i := m.FromPorts.Begin; i <= m.FromPorts.End; i++ {
 			err := PortMapper.ReleaseMap(m.Protocol, i)
 			if err != nil {
-				continue
+				continue release_loop
 			}
 		}
 

server/router/pod/pod.go

@@ -32,7 +32,7 @@ func NewRouter(b Backend) router.Router {
 		local.NewPostRoute("/pod/pause", r.postPodPause),
 		local.NewPostRoute("/pod/unpause", r.postPodUnpause),
 		// PUT
-		local.NewPostRoute("/pod/{id}/portmappings/{action}", r.putPortMappings),
+		local.NewPutRoute("/pod/{id}/portmappings/{action}", r.putPortMappings),
 		// DELETE
 		local.NewDeleteRoute("/pod", r.deletePod),
 	}

types/spec_test.go

@@ -217,7 +217,7 @@ func TestMergePorts(t *testing.T) {
 
 	t.Log("> testing empty port mapping list")
 	pms = []*_PortMapping{}
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err != nil {
 		t.Fatalf("failed with emport pm list: %v", err)
 	}
@@ -241,7 +241,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err != nil {
 		t.Fatalf("error with normal pm list: %v", err)
 	}
@@ -266,7 +266,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err != nil {
 		t.Fatalf("error with normal pm list: %v", err)
 	}
@@ -290,7 +290,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err != nil {
 		t.Fatalf("error with normal pm list: %v", err)
 	}
@@ -315,7 +315,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err == nil {
 		t.Fatalf("failed with overlapped pm list: %v", res)
 	}
@@ -336,7 +336,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err == nil {
 		t.Fatalf("failed with overlapped pm list: %v", res)
 	}
@@ -357,7 +357,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err == nil {
 		t.Fatalf("failed with overlapped pm list: %v", res)
 	}
@@ -379,7 +379,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err != nil {
 		t.Fatalf("error with random pm list: %v", err)
 	}
@@ -403,7 +403,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err == nil {
 		t.Fatalf("didn't found collision in random pm list: %v", res)
 	}
@@ -428,7 +428,7 @@ func TestMergePorts(t *testing.T) {
 		},
 	}
 	t.Logf("---[debug] input items %v", pms)
-	res, err = mergePorts(pms)
+	res, err = mergeContinuousPorts(pms)
 	if err != nil {
 		t.Logf("got an error with random pm list: %v", err)
 		t.Log("it's acceptable to fail in this case")

types/utils.go

@@ -250,7 +250,7 @@ func (pm *_PortMapping) notDetermined() bool {
 	return !pm.container.isRange() && pm.host.isRange()
 }
 
-func mergePorts(pms []*_PortMapping) ([]*_PortMapping, error) {
+func mergeContinuousPorts(pms []*_PortMapping) ([]*_PortMapping, error) {
 	var (
 		results = []*_PortMapping{}
 		occupy  = map[int]bool{}
@@ -364,18 +364,6 @@ func (p *UserPod) MergePortmappings() error {
 		}
 	}
 
-	hlog.Log(hlog.TRACE, "TCP ports to be merged: %v", tcpPorts)
-	tcpPorts, err = mergePorts(tcpPorts)
-	if err != nil {
-		return err
-	}
-
-	hlog.Log(hlog.TRACE, "UDP ports to be merged: %v", udpPorts)
-	udpPorts, err = mergePorts(udpPorts)
-	if err != nil {
-		return err
-	}
-
 	pms := []*PortMapping{}
 	for _, pm := range tcpPorts {
 		pms = append(pms, pm.toSpec())