From 5b6f59e36cc68bf5bb2e0bd363fe005b55bf0c8f Mon Sep 17 00:00:00 2001
From: Matthew Heon <matthew.heon@gmail.com>
Date: Tue, 27 Mar 2018 15:00:16 -0400
Subject: Prevent ctrs not in pods from depending on pod ctrs

Containers in pods cannot depend on containers outside of the
same pod. Make the reverse true as well - containers not in pods
cannot depend on containers in pods. This greatly simplifies our
dependency handling, as we can guarantee that removing a pod will
not encounter dependency issues.

Signed-off-by: Matthew Heon <matthew.heon@gmail.com>

Closes: #558
Approved by: rhatdan
---
 libpod/boltdb_state_internal.go | 11 +++++--
 libpod/in_memory_state.go       |  9 ++++--
 libpod/state_test.go            | 64 ++++++++++++++++++++---------------------
 3 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/libpod/boltdb_state_internal.go b/libpod/boltdb_state_internal.go
index 9c05dcc3a..fc175fc1f 100644
--- a/libpod/boltdb_state_internal.go
+++ b/libpod/boltdb_state_internal.go
@@ -407,11 +407,16 @@ func (s *BoltState) addContainer(ctr *Container, pod *Pod) error {
 				return errors.Wrapf(ErrNoSuchCtr, "container %s depends on container %s, but it does not exist in the DB", ctr.ID(), dependsCtr)
 			}
 
-			// If we're part of a pod, make sure the dependency is part of the same pod
+			depCtrPod := depCtrBkt.Get(podIDKey)
 			if pod != nil {
-				depCtrPod := depCtrBkt.Get(podIDKey)
+				// If we're part of a pod, make sure the dependency is part of the same pod
 				if depCtrPod == nil {
-					return errors.Wrapf(ErrInvalidArg, "container %s depends on container%s which is not in pod %s", ctr.ID(), dependsCtr, pod.ID())
+					return errors.Wrapf(ErrInvalidArg, "container %s depends on container %s which is not in pod %s", ctr.ID(), dependsCtr, pod.ID())
+				}
+			} else {
+				// If we're not part of a pod, we cannot depend on containets in a pod
+				if depCtrPod != nil {
+					return errors.Wrapf(ErrInvalidArg, "container %s depends on container %s which is in a pod - containers not in pods cannot depend on containers in pods", ctr.ID(), dependsCtr)
 				}
 			}
 
diff --git a/libpod/in_memory_state.go b/libpod/in_memory_state.go
index a38e080b7..386ace5b6 100644
--- a/libpod/in_memory_state.go
+++ b/libpod/in_memory_state.go
@@ -126,9 +126,12 @@ func (s *InMemoryState) AddContainer(ctr *Container) error {
 	// But in-memory state is intended purely for testing and not production
 	// use, so this should be fine.
 	depCtrs := ctr.Dependencies()
-	for _, depCtr := range depCtrs {
-		if _, ok := s.containers[depCtr]; !ok {
-			return errors.Wrapf(ErrNoSuchCtr, "cannot depend on nonexistent container %s", depCtr)
+	for _, depID := range depCtrs {
+		depCtr, ok := s.containers[depID]
+		if !ok {
+			return errors.Wrapf(ErrNoSuchCtr, "cannot depend on nonexistent container %s", depID)
+		} else if depCtr.config.Pod != "" {
+			return errors.Wrapf(ErrInvalidArg, "cannot depend on container in a pod if not part of same pod")
 		}
 	}
 
diff --git a/libpod/state_test.go b/libpod/state_test.go
index 634280c99..8b6abdb8d 100644
--- a/libpod/state_test.go
+++ b/libpod/state_test.go
@@ -273,6 +273,38 @@ func TestAddCtrInPodFails(t *testing.T) {
 	})
 }
 
+func TestAddCtrDepInPodFails(t *testing.T) {
+	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
+		testPod, err := getTestPod1(lockPath)
+		assert.NoError(t, err)
+
+		testCtr1, err := getTestCtr2(lockPath)
+		assert.NoError(t, err)
+
+		testCtr1.config.Pod = testPod.ID()
+
+		testCtr2, err := getTestCtrN("3", lockPath)
+		assert.NoError(t, err)
+
+		testCtr2.config.UserNsCtr = testCtr1.ID()
+
+		err = state.AddPod(testPod)
+		assert.NoError(t, err)
+
+		err = state.AddContainerToPod(testPod, testCtr1)
+		assert.NoError(t, err)
+
+		err = state.AddContainer(testCtr2)
+		assert.Error(t, err)
+
+		ctrs, err := state.AllContainers()
+		assert.NoError(t, err)
+		assert.Equal(t, 1, len(ctrs))
+
+		testContainersEqual(t, testCtr1, ctrs[0])
+	})
+}
+
 func TestGetNonexistentContainerFails(t *testing.T) {
 	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
 		_, err := state.Container("does not exist")
@@ -1749,38 +1781,6 @@ func TestRemovePodContainerDependencyInPod(t *testing.T) {
 	})
 }
 
-func TestRemovePodContainerDependencyNotInPod(t *testing.T) {
-	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
-		testPod, err := getTestPod1(lockPath)
-		assert.NoError(t, err)
-
-		testCtr1, err := getTestCtr2(lockPath)
-		assert.NoError(t, err)
-		testCtr1.config.Pod = testPod.ID()
-
-		testCtr2, err := getTestCtrN("3", lockPath)
-		assert.NoError(t, err)
-		testCtr2.config.IPCNsCtr = testCtr1.ID()
-
-		err = state.AddPod(testPod)
-		assert.NoError(t, err)
-
-		err = state.AddContainerToPod(testPod, testCtr1)
-		assert.NoError(t, err)
-
-		err = state.AddContainer(testCtr2)
-		assert.NoError(t, err)
-
-		err = state.RemovePodContainers(testPod)
-		t.Logf("Err %v", err)
-		assert.Error(t, err)
-
-		ctrs, err := state.PodContainersByID(testPod)
-		assert.NoError(t, err)
-		assert.Equal(t, 1, len(ctrs))
-	})
-}
-
 func TestAddContainerToPodInvalidPod(t *testing.T) {
 	runForAllStates(t, func(t *testing.T, state State, lockPath string) {
 		testCtr, err := getTestCtr1(lockPath)
-- 
cgit v1.2.3-54-g00ecf