App and pod validation errors for missing network name MARATHON-7398 (#…

…5432)

* App and pod validation errors for missing network name MARATHON-7398

* Remove redundant implicits from AppValidationTest

* Do not pass whole config, use just func returning default network name

* Do not use scallopoption - use regular option instead

* Extract common logic to NetworkValidation trait

* Use NetworkValidationMessages also in PodsResourceTest

src/main/scala/mesosphere/marathon/api/akkahttp/v2/AppsController.scala

@@ -44,7 +44,7 @@ class AppsController(
   import Directives._
 
   private implicit lazy val validateApp = AppDefinition.validAppDefinition(config.availableFeatures)(pluginManager)
-  private implicit lazy val updateValidator = AppValidation.validateCanonicalAppUpdateAPI(config.availableFeatures)
+  private implicit lazy val updateValidator = AppValidation.validateCanonicalAppUpdateAPI(config.availableFeatures, () => normalizationConfig.defaultNetworkName)
 
   import AppsController._
   import EntityMarshallers._
@@ -149,7 +149,7 @@ object AppsController {
   def appNormalization(config: NormalizationConfig): Normalization[raml.App] = Normalization { app =>
     validateOrThrow(app)(AppValidation.validateOldAppAPI)
     val migrated = AppNormalization.forDeprecated(config.config).normalized(app)
-    validateOrThrow(migrated)(AppValidation.validateCanonicalAppAPI(config.enabledFeatures))
+    validateOrThrow(migrated)(AppValidation.validateCanonicalAppAPI(config.enabledFeatures, () => config.config.defaultNetworkName))
     AppNormalization(config.config).normalized(migrated)
   }
 }

src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala

@@ -45,7 +45,7 @@ class AppsResource @Inject() (
 
   private[this] val ListApps = """^((?:.+/)|)\*$""".r
   private implicit lazy val appDefinitionValidator = AppDefinition.validAppDefinition(config.availableFeatures)(pluginManager)
-  private implicit lazy val validateCanonicalAppUpdateAPI = AppValidation.validateCanonicalAppUpdateAPI(config.availableFeatures)
+  private implicit lazy val validateCanonicalAppUpdateAPI = AppValidation.validateCanonicalAppUpdateAPI(config.availableFeatures, () => normalizationConfig.defaultNetworkName)
 
   private val normalizationConfig = AppNormalization.Configuration(
     config.defaultNetworkName.get,
@@ -414,14 +414,14 @@ object AppsResource {
   def appNormalization(config: NormalizationConfig): Normalization[raml.App] = Normalization { app =>
     validateOrThrow(app)(AppValidation.validateOldAppAPI)
     val migrated = AppNormalization.forDeprecated(config.config).normalized(app)
-    validateOrThrow(migrated)(AppValidation.validateCanonicalAppAPI(config.enabledFeatures))
+    validateOrThrow(migrated)(AppValidation.validateCanonicalAppAPI(config.enabledFeatures, () => config.config.defaultNetworkName))
     AppNormalization(config.config).normalized(migrated)
   }
 
   def appUpdateNormalization(config: NormalizationConfig): Normalization[raml.AppUpdate] = Normalization { app =>
     validateOrThrow(app)(AppValidation.validateOldAppUpdateAPI)
     val migrated = AppNormalization.forDeprecatedUpdates(config.config).normalized(app)
-    validateOrThrow(app)(AppValidation.validateCanonicalAppUpdateAPI(config.enabledFeatures))
+    validateOrThrow(app)(AppValidation.validateCanonicalAppUpdateAPI(config.enabledFeatures, () => config.config.defaultNetworkName))
     AppNormalization.forUpdates(config.config).normalized(migrated)
   }
 

src/main/scala/mesosphere/marathon/api/v2/PodsResource.scala

@@ -49,7 +49,7 @@ class PodsResource @Inject() (
   implicit def podDefValidator: Validator[Pod] =
     PodsValidation.podValidator(
       config.availableFeatures,
-      scheduler.mesosMasterVersion().getOrElse(SemanticVersion(0, 0, 0)))
+      scheduler.mesosMasterVersion().getOrElse(SemanticVersion(0, 0, 0)), config.defaultNetworkName.get)
 
   // If we change/add/upgrade the notion of a Pod and can't do it purely in the internal model,
   // update the json first

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

@@ -304,7 +304,7 @@ trait AppValidation {
     }
   )
 
-  def validateCanonicalAppUpdateAPI(enabledFeatures: Set[String]): Validator[AppUpdate] = forAll(
+  def validateCanonicalAppUpdateAPI(enabledFeatures: Set[String], defaultNetworkName: () => Option[String]): Validator[AppUpdate] = forAll(
     validator[AppUpdate] { update =>
       update.id.map(PathId(_)) as "id" is optional(valid)
       update.dependencies.map(_.map(PathId(_))) as "dependencies" is optional(every(valid))
@@ -315,6 +315,7 @@ trait AppValidation {
       update.portDefinitions is optional(portDefinitionsValidator)
       update.container is optional(valid(validContainer(enabledFeatures, update.networks.getOrElse(Nil), update.secrets.getOrElse(Map.empty))))
       update.acceptedResourceRoles is valid(optional(ResourceRole.validAcceptedResourceRoles(update.residency.isDefined) and notEmpty))
+      update.networks is optional(NetworkValidation.defaultNetworkNameValidator(defaultNetworkName))
     },
     isTrue("must not be root")(!_.id.fold(false)(PathId(_).isRoot)),
     isTrue("must not be an empty string")(_.cmd.forall { s => s.length() > 1 }),
@@ -367,11 +368,12 @@ trait AppValidation {
     }
   )
 
-  def validateCanonicalAppAPI(enabledFeatures: Set[String]): Validator[App] = forAll(
+  def validateCanonicalAppAPI(enabledFeatures: Set[String], defaultNetworkName: () => Option[String]): Validator[App] = forAll(
     validBasicAppDefinition(enabledFeatures),
     validator[App] { app =>
       PathId(app.id) as "id" is (PathId.pathIdValidator and PathId.absolutePathValidator and PathId.nonEmptyPath)
       app.dependencies.map(PathId(_)) as "dependencies" is every(valid)
+      app.networks is defaultNetworkNameValidator(defaultNetworkName)
     },
     isTrue("must not be root")(!_.id.toPath.isRoot),
     isTrue("must not be an empty string")(_.cmd.forall { s => s.length() > 1 }),

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

@@ -71,6 +71,15 @@ trait NetworkValidation {
         (hostNetworks == 0 && bridgeNetworks == 1 && containerNetworks == 0) ||
         (hostNetworks == 0 && bridgeNetworks == 0 && containerNetworks > 0)
     }
+
+  def defaultNetworkNameValidator(defaultNetworkName: () => Option[String]): Validator[Seq[Network]] =
+    isTrue(NetworkValidationMessages.NetworkNameMustBeSpecified) { n =>
+      n.forall(c => c.mode != NetworkMode.Container || c.name.isDefined || defaultNetworkName().isDefined)
+    }
 }
 
 object NetworkValidation extends NetworkValidation
+
+object NetworkValidationMessages {
+  val NetworkNameMustBeSpecified = "Network name must be specified when container network is selected."
+}

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

@@ -198,7 +198,7 @@ trait PodsValidation extends GeneralPurposeCombinators {
     hostPorts.distinct.size == hostPorts.size
   }
 
-  def podValidator(enabledFeatures: Set[String], mesosMasterVersion: SemanticVersion): Validator[Pod] = validator[Pod] { pod =>
+  def podValidator(enabledFeatures: Set[String], mesosMasterVersion: SemanticVersion, defaultNetworkName: Option[String]): Validator[Pod] = validator[Pod] { pod =>
     PathId(pod.id) as "id" is valid and PathId.absolutePathValidator and PathId.nonEmptyPath
     pod.user is optional(notEmpty)
     pod.environment is envValidator(strictNameValidation = false, pod.secrets, enabledFeatures)
@@ -215,6 +215,7 @@ trait PodsValidation extends GeneralPurposeCombinators {
     }
     pod.secrets is empty or (valid(secretValidator) and featureEnabled(enabledFeatures, Features.SECRETS))
     pod.networks is valid(ramlNetworksValidator)
+    pod.networks is defaultNetworkNameValidator(() => defaultNetworkName)
     pod.scheduling is optional(schedulingValidator)
     pod.scaling is optional(scalingValidator)
     pod is endpointNamesUnique and endpointContainerPortsUnique and endpointHostPortsUnique

src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala

@@ -7,6 +7,7 @@ import javax.ws.rs.core.Response
 import akka.Done
 import mesosphere.AkkaUnitTest
 import mesosphere.marathon.api._
+import mesosphere.marathon.api.v2.validation.{ AppValidationMessages, NetworkValidationMessages }
 import mesosphere.marathon.core.appinfo.AppInfo.Embed
 import mesosphere.marathon.core.appinfo._
 import mesosphere.marathon.test.SettableClock
@@ -273,6 +274,22 @@ class AppsResourceTest extends AkkaUnitTest with GroupCreation {
       response.getMetadata.containsKey(RestResource.DeploymentHeader) should be(true)
     }
 
+    "Fail creating application when network name is missing" in new Fixture {
+      Given("An app and group")
+      val app = App(
+        id = "/app",
+        cmd = Some("cmd"),
+        networks = Seq(Network(mode = NetworkMode.Container))
+      )
+
+      When("The create request is made")
+      val response = appsResource.create(Json.stringify(Json.toJson(app)).getBytes("UTF-8"), force = false, auth.request)
+
+      Then("Validation fails")
+      response.getStatus should be(422)
+      response.getEntity.toString should include(NetworkValidationMessages.NetworkNameMustBeSpecified)
+    }
+
     "Create a new app with IP/CT, no default network name, Alice does not specify a network" in new Fixture {
       Given("An app and group")
       val app = App(
@@ -377,11 +394,11 @@ class AppsResourceTest extends AkkaUnitTest with GroupCreation {
       When("The application is updated")
       val updatedJson = Json.toJson(updatedApp).as[JsObject]
       val updatedBody = Json.stringify(updatedJson).getBytes("UTF-8")
+      val response = appsResource.replace(updatedApp.id, updatedBody, force = false, partialUpdate = false, auth.request)
 
       Then("the update should fail")
-      the[NormalizationException] thrownBy {
-        appsResource.replace(updatedApp.id, updatedBody, force = false, partialUpdate = false, auth.request)
-      } should have message NetworkNormalizationMessages.ContainerNetworkNameUnresolved
+      response.getStatus should be(422)
+      response.getEntity.toString should include(NetworkValidationMessages.NetworkNameMustBeSpecified)
     }
 
     "Create a new app without IP/CT when default virtual network is bar" in new Fixture(configArgs = Seq("--default_network_name", "bar")) {

src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala

@@ -8,6 +8,7 @@ import akka.stream.Materializer
 import akka.stream.scaladsl.Source
 import mesosphere.AkkaUnitTest
 import mesosphere.marathon.api.v2.json.Formats.TimestampFormat
+import mesosphere.marathon.api.v2.validation.NetworkValidationMessages
 import mesosphere.marathon.api.{ RestResource, TaskKiller, TestAuthFixture }
 import mesosphere.marathon.core.appinfo.PodStatusService
 import mesosphere.marathon.core.async.ExecutionContexts
@@ -284,10 +285,9 @@ class PodsResourceTest extends AkkaUnitTest with Mockito {
 
       podSystem.create(any, eq(false)).returns(Future.successful(DeploymentPlan.empty))
 
-      val ex = intercept[NormalizationException] {
-        f.podsResource.create(podSpecJsonWithContainerNetworking.getBytes(), force = false, f.auth.request)
-      }
-      ex.msg shouldBe NetworkNormalizationMessages.ContainerNetworkNameUnresolved
+      val response = f.podsResource.create(podSpecJsonWithContainerNetworking.getBytes(), force = false, f.auth.request)
+      response.getStatus shouldBe 422
+      response.getEntity.toString should include(NetworkValidationMessages.NetworkNameMustBeSpecified)
     }
 
     "create a pod with custom executor resource declaration" in {

src/test/scala/mesosphere/marathon/api/v2/json/AppDefinitionFormatsTest.scala

@@ -51,7 +51,7 @@ class AppDefinitionFormatsTest extends UnitTest
         AppNormalization.apply(config)
           .normalized(Validation.validateOrThrow(
             AppNormalization.forDeprecated(config).normalized(app))(AppValidation.validateOldAppAPI)))(
-          AppValidation.validateCanonicalAppAPI(Set.empty)
+          AppValidation.validateCanonicalAppAPI(Set.empty, () => None)
         )
     )
   }
@@ -682,14 +682,15 @@ class AppDefinitionFormatsTest extends UnitTest
     }
 
     "FromJSON should fail for empty container (#4978)" in {
+      val config = AppNormalization.Configuration(None, "mesos-bridge-name")
       val json = Json.parse(
         """{
           |  "id": "/docker-compose-demo",
           |  "cmd": "echo hello world",
           |  "container": {}
           |}""".stripMargin)
       val ramlApp = json.as[raml.App]
-      AppValidation.validateCanonicalAppAPI(Set.empty)(ramlApp) should failWith(
+      AppValidation.validateCanonicalAppAPI(Set.empty, () => config.defaultNetworkName)(ramlApp) should failWith(
         group("container", "is invalid", "docker" -> "not defined")
       )
     }

src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala

@@ -5,7 +5,7 @@ import com.wix.accord._
 import mesosphere.UnitTest
 import mesosphere.marathon.api.JsonTestHelper
 import mesosphere.marathon.api.v2.Validation.validateOrThrow
-import mesosphere.marathon.api.v2.validation.AppValidation
+import mesosphere.marathon.api.v2.validation.{ AppValidation, AppValidationMessages, NetworkValidationMessages }
 import mesosphere.marathon.api.v2.{ AppNormalization, AppsResource, ValidationHelper }
 import mesosphere.marathon.core.readiness.ReadinessCheckTestHelper
 import mesosphere.marathon.raml.{ AppCContainer, AppUpdate, Artifact, Container, ContainerPortMapping, DockerContainer, EngineType, Environment, Network, NetworkMode, PortDefinition, PortDefinitions, Raml, SecretDef, UpgradeStrategy }
@@ -18,7 +18,7 @@ import scala.collection.immutable.Seq
 
 class AppUpdateTest extends UnitTest {
 
-  implicit val appUpdateValidator: Validator[AppUpdate] = AppValidation.validateCanonicalAppUpdateAPI(Set("secrets"))
+  implicit val appUpdateValidator: Validator[AppUpdate] = AppValidation.validateCanonicalAppUpdateAPI(Set("secrets"), () => AppNormalization.Configuration(None, "mesos-bridge-name").defaultNetworkName)
 
   val runSpecId = PathId("/test")
 
@@ -83,6 +83,7 @@ class AppUpdateTest extends UnitTest {
       shouldViolate(update.copy(cpus = Some(-3.0)), "/cpus", "error.min")
       shouldViolate(update.copy(disk = Some(-3.0)), "/disk", "error.min")
       shouldViolate(update.copy(instances = Some(-3)), "/instances", "error.min")
+      shouldViolate(update.copy(networks = Some(Seq(Network(mode = NetworkMode.Container)))), "/networks", NetworkValidationMessages.NetworkNameMustBeSpecified)
     }
 
     "Validate secrets" in {

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

@@ -3,15 +3,19 @@ package api.v2.validation
 
 import com.wix.accord.Validator
 import com.wix.accord.scalatest.ResultMatchers
+import mesosphere.marathon.api.v2.AppNormalization
 import mesosphere.marathon.raml._
 import mesosphere.{ UnitTest, ValidationTestLike }
 
 class AppValidationTest extends UnitTest with ResultMatchers with ValidationTestLike {
 
   import Normalization._
 
-  implicit val basicValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set.empty)
-  implicit val withSecretsValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set("secrets"))
+  val config = AppNormalization.Configuration(None, "mesos-bridge-name")
+  val configWithDefaultNetworkName = AppNormalization.Configuration(Some("defaultNetworkName"), "mesos-bridge-name")
+  val basicValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set.empty, () => config.defaultNetworkName)
+  val withSecretsValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set("secrets"), () => config.defaultNetworkName)
+  val withDefaultNetworkNameValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set.empty, () => configWithDefaultNetworkName.defaultNetworkName)
 
   "File based secrets validation" when {
     "file based secret is used when secret feature is not enabled" should {
@@ -184,6 +188,17 @@ class AppValidationTest extends UnitTest with ResultMatchers with ValidationTest
                 networkNames = List("1"))))) shouldBe (aSuccess)
       }
 
+      "consider a container network without name to be invalid" in {
+        val result = basicValidator(
+          networkedApp(Seq.empty, networks = Seq(Network(mode = NetworkMode.Container)), false))
+        result.isFailure shouldBe true
+      }
+
+      "consider a container network without name but with a default name from config valid" in {
+        withDefaultNetworkNameValidator(
+          networkedApp(Seq.empty, networks = Seq(Network(mode = NetworkMode.Container)), false)) shouldBe (aSuccess)
+      }
+
       "consider a portMapping with a hostPort and two valid networkNames as invalid" in {
         basicValidator(
           containerNetworkedApp(

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

@@ -1,11 +1,11 @@
 package mesosphere.marathon
 package api.v2.validation
 
-import com.wix.accord.{ Result, Validator }
 import com.wix.accord.scalatest.ResultMatchers
-import mesosphere.{ UnitTest, ValidationTestLike }
+import com.wix.accord.{ Result, Validator }
 import mesosphere.marathon.raml.{ Constraint, ConstraintOperator, DockerPullConfig, Endpoint, EnvVarSecret, EphemeralVolume, Image, ImageType, Network, NetworkMode, Pod, PodContainer, PodSecretVolume, Resources, SecretDef, VolumeMount }
 import mesosphere.marathon.util.SemanticVersion
+import mesosphere.{ UnitTest, ValidationTestLike }
 
 class PodsValidationTest extends UnitTest with ResultMatchers with PodsValidation with SchedulingValidation with ValidationTestLike {
 
@@ -59,7 +59,7 @@ class PodsValidationTest extends UnitTest with ResultMatchers with PodsValidatio
       val endpoint1 = Endpoint("endpoint1", containerPort = Some(123))
       val endpoint2 = Endpoint("endpoint2", containerPort = Some(123))
       private val invalid = validPod.copy(
-        networks = Seq(Network(mode = NetworkMode.Container)),
+        networks = Seq(Network(mode = NetworkMode.Container, name = Some("default-network-name"))),
         containers = Seq(validContainer.copy(endpoints = Seq(endpoint1, endpoint2)))
       )
       validator(invalid) should failWith("value" -> PodsValidationMessages.ContainerPortsMustBeUnique)
@@ -155,12 +155,12 @@ class PodsValidationTest extends UnitTest with ResultMatchers with PodsValidatio
       secrets = Map("aSecret" -> SecretDef("/pull/config"))
     )
 
-    val validator: Validator[Pod] = podValidator(Set.empty, SemanticVersion.zero)
-    val secretValidator: Validator[Pod] = podValidator(Set(Features.SECRETS), SemanticVersion.zero)
+    val validator: Validator[Pod] = podValidator(Set.empty, SemanticVersion.zero, None)
+    val secretValidator: Validator[Pod] = podValidator(Set(Features.SECRETS), SemanticVersion.zero, None)
   }
 
   "network validation" when {
-    val validator: Validator[Pod] = podValidator(Set.empty, SemanticVersion.zero)
+    val validator: Validator[Pod] = podValidator(Set.empty, SemanticVersion.zero, Some("default-network-name"))
 
     def podContainer(name: String = "ct1", resources: Resources = Resources(), endpoints: Seq[Endpoint] = Nil) =
       PodContainer(

src/test/scala/mesosphere/marathon/api/validation/AppUpdateValidatorTest.scala

@@ -12,7 +12,7 @@ import org.scalatest.Matchers
 import play.api.libs.json.Json
 
 class AppUpdateValidatorTest extends UnitTest with Matchers {
-  implicit val appUpdateValidator = AppValidation.validateCanonicalAppUpdateAPI(Set.empty)
+  implicit val appUpdateValidator = AppValidation.validateCanonicalAppUpdateAPI(Set.empty, () => AppNormalization.Configuration(None, "mesos-bridge-name").defaultNetworkName)
   implicit val validAppDefinition = AppDefinition.validAppDefinition(Set.empty)(PluginManager.None)
 
   "validation that considers container types" should {

src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala

@@ -21,7 +21,8 @@ import scala.reflect.ClassTag
 
 class RunSpecValidatorTest extends UnitTest {
 
-  private implicit lazy val validApp = AppValidation.validateCanonicalAppAPI(Set())
+  val config = AppNormalization.Configuration(None, "mesos-bridge-name")
+  private implicit lazy val validApp = AppValidation.validateCanonicalAppAPI(Set(), () => config.defaultNetworkName)
   private implicit lazy val validAppDefinition = AppDefinition.validAppDefinition(Set())(PluginManager.None)
   private def validContainer(networks: Seq[Network] = Nil) = Container.validContainer(networks, Set())