Fixes #5016 by validating, if the app id is non empty.

Summary: Started a PodsValidationTest which was not available before.

Test Plan: sbt test

Reviewers: unterstein, jeschkies

Reviewed By: unterstein, jeschkies

Subscribers: jenkins, marathon-team

Differential Revision: https://phabricator.mesosphere.com/D447

src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala

@@ -294,7 +294,7 @@ trait PodsValidation {
   }
 
   def podDefValidator(enabledFeatures: Set[String], mesosMasterVersion: SemanticVersion): Validator[Pod] = validator[Pod] { pod =>
-    PathId(pod.id) as "id" is valid and valid(PathId.absolutePathValidator)
+    PathId(pod.id) as "id" is valid and PathId.absolutePathValidator and PathId.nonEmptyPath
     pod.user is optional(notEmpty)
     pod.environment is envValidator(pod, enabledFeatures)
     pod.volumes is every(volumeValidator(pod.containers)) and isTrue("volume names are unique") { volumes: Seq[Volume] =>

src/main/scala/mesosphere/marathon/state/AppDefinition.scala

@@ -456,8 +456,7 @@ object AppDefinition extends GeneralPurposeCombinators {
   def validAppDefinition(
     enabledFeatures: Set[String])(implicit pluginManager: PluginManager): Validator[AppDefinition] =
     validator[AppDefinition] { app =>
-      app.id is valid
-      app.id is PathId.absolutePathValidator
+      app.id is valid and PathId.absolutePathValidator and PathId.nonEmptyPath
       app.dependencies is every(PathId.validPathWithBase(app.id.parent))
     } and validBasicAppDefinition(enabledFeatures) and pluginValidators
 

src/main/scala/mesosphere/marathon/state/PathId.scala

@@ -137,17 +137,23 @@ object PathId {
     path is validPathChars
   }
 
+  /**
+    * Make sure that the given path is a child of the defined parent path.
+    * Every root and every relative path can be ignored.
+    */
   private def childOf(parent: PathId): Validator[PathId] = {
     isTrue[PathId](s"Identifier is not child of $parent. Hint: use relative paths.") { child =>
-      parent == PathId.empty || !parent.absolute ||
-        (parent.absolute && child.canonicalPath(parent).parent == parent)
+      parent == PathId.empty || !parent.absolute || (parent.absolute && child.canonicalPath(parent).parent == parent)
     }
   }
 
+  /**
+    * Makes sure, the path is not only the root path and is not empty.
+    */
+  val nonEmptyPath = isTrue[PathId]("Path must contain at least one path element") { _.path.nonEmpty }
+
   /**
     * Needed for AppDefinitionValidatorTest.testSchemaLessStrictForId.
     */
-  val absolutePathValidator = isTrue[PathId]("Path needs to be absolute") { path =>
-    path.absolute
-  }
+  val absolutePathValidator = isTrue[PathId]("Path needs to be absolute") { _.absolute }
 }

src/main/scala/mesosphere/marathon/util/SemanticVersion.scala

@@ -11,6 +11,8 @@ case class SemanticVersion(major: Int, minor: Int, patch: Int) {
 object SemanticVersion {
   val Pattern: Regex = """(\d+)\.(\d+)\.(\d+).*""".r
 
+  val zero = SemanticVersion(0, 0, 0)
+
   // If we can not match the version pattern we prefer to throw an
   // error rather than silently construct a dummy version.
   def apply(version: String): Option[SemanticVersion] = version match {

src/test/scala/mesosphere/marathon/api/v2/validation/PodsValidationTest.scala

@@ -0,0 +1,68 @@
+package mesosphere.marathon
+package v2.validation
+
+import mesosphere.UnitTest
+import com.wix.accord.scalatest.ResultMatchers
+import mesosphere.marathon.api.v2.validation.PodsValidation
+import mesosphere.marathon.raml.{ Endpoint, Network, NetworkMode, Pod, PodContainer, Resources, Volume, VolumeMount }
+import mesosphere.marathon.util.SemanticVersion
+
+class PodsValidationTest extends UnitTest with ResultMatchers with PodsValidation {
+
+  "A pod definition" should {
+
+    "be rejected if the id is empty" in new Fixture {
+      private val invalid = validPod.copy(id = "/")
+      validator(invalid) should failWith("id" -> "Path must contain at least one path element")
+    }
+
+    "be rejected if the id is not absolute" in new Fixture {
+      private val invalid = validPod.copy(id = "some/foo")
+      validator(invalid) should failWith("id" -> "Path needs to be absolute")
+    }
+
+    "be rejected if a defined user is empty" in new Fixture {
+      private val invalid = validPod.copy(user = Some(""))
+      validator(invalid) should failWith("user" -> "must not be empty")
+    }
+
+    "be rejected if no container is defined" in new Fixture {
+      private val invalid = validPod.copy(containers = Seq.empty)
+      validator(invalid) should failWith("containers" -> "must not be empty")
+    }
+
+    "be rejected if container names are not unique" in new Fixture {
+      private val invalid = validPod.copy(containers = Seq(validContainer, validContainer))
+      validator(invalid) should failWith("containers" -> "container names are unique")
+    }
+
+    "be rejected if endpoint names are not unique" in new Fixture {
+      val endpoint = Endpoint("endpoint", hostPort = Some(123))
+      private val invalid = validPod.copy(containers = Seq(validContainer.copy(endpoints = Seq(endpoint, endpoint))))
+      validator(invalid) should failWith("value" -> "Endpoint names are unique")
+    }
+
+    "be rejected if volume names are not unique" in new Fixture {
+      val volume = Volume("volume", host = Some("/foo"))
+      val volumeMount = VolumeMount(volume.name, "/bla")
+      private val invalid = validPod.copy(
+        volumes = Seq(volume, volume),
+        containers = Seq(validContainer.copy(volumeMounts = Seq(volumeMount)))
+      )
+      validator(invalid) should failWith("volumes" -> "volume names are unique")
+    }
+  }
+
+  class Fixture {
+    val validContainer = PodContainer(
+      name = "ct1",
+      resources = Resources()
+    )
+    val validPod = Pod(
+      id = "/some/pod",
+      containers = Seq(validContainer),
+      networks = Seq(Network(mode = NetworkMode.Host))
+    )
+    val validator = podDefValidator(Set.empty, SemanticVersion.zero)
+  }
+}