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

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

@@ -43,7 +43,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)
 
   import AppsController._
   import EntityMarshallers._
@@ -148,7 +148,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))
     AppNormalization(config.config).normalized(migrated)
   }
 }

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

@@ -44,7 +44,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)
 
   private val normalizationConfig = AppNormalization.Configuration(
     config.defaultNetworkName.get,
@@ -408,14 +408,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))
     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))
     AppNormalization.forUpdates(config.config).normalized(migrated)
   }
 

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

@@ -46,7 +46,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)
 
   // 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

@@ -5,6 +5,7 @@ import java.util.regex.Pattern
 
 import com.wix.accord._
 import com.wix.accord.dsl._
+import mesosphere.marathon.api.v2.AppNormalization
 import mesosphere.marathon.api.v2.Validation.{ featureEnabled, _ }
 import mesosphere.marathon.core.externalvolume.ExternalVolumes
 import mesosphere.marathon.raml._
@@ -304,7 +305,7 @@ trait AppValidation {
     }
   )
 
-  def validateCanonicalAppUpdateAPI(enabledFeatures: Set[String]): Validator[AppUpdate] = forAll(
+  def validateCanonicalAppUpdateAPI(enabledFeatures: Set[String], normalizationConfig: AppNormalization.Config): 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))
@@ -324,6 +325,9 @@ trait AppValidation {
     isTrue("portDefinitions are only allowed with host-networking") { update =>
       !(update.networks.exists(_.exists(_.mode != NetworkMode.Host)) && update.portDefinitions.exists(_.nonEmpty))
     },
+    isTrue(AppValidationMessages.NetworkNameMustBeSpecified) { update =>
+      update.networks.forall(n => n.forall(c => c.mode != NetworkMode.Container || c.name.isDefined || normalizationConfig.defaultNetworkName.isDefined))
+    },
     isTrue("The 'version' field may only be combined with the 'id' field.") { update =>
       def onlyVersionOrIdSet: Boolean = update.productIterator.forall {
         case x: Some[Any] => x == update.version || x == update.id // linter:ignore UnlikelyEquality
@@ -367,7 +371,7 @@ trait AppValidation {
     }
   )
 
-  def validateCanonicalAppAPI(enabledFeatures: Set[String]): Validator[App] = forAll(
+  def validateCanonicalAppAPI(enabledFeatures: Set[String], normalizationConfig: AppNormalization.Config): Validator[App] = forAll(
     validBasicAppDefinition(enabledFeatures),
     validator[App] { app =>
       PathId(app.id) as "id" is (PathId.pathIdValidator and PathId.absolutePathValidator and PathId.nonEmptyPath)
@@ -380,6 +384,9 @@ trait AppValidation {
     },
     isTrue("portDefinitions are only allowed with host-networking") { app =>
       !(app.networks.exists(_.mode != NetworkMode.Host) && app.portDefinitions.exists(_.nonEmpty))
+    },
+    isTrue(AppValidationMessages.NetworkNameMustBeSpecified) { app =>
+      app.networks.forall(c => c.mode != NetworkMode.Container || c.name.isDefined || normalizationConfig.defaultNetworkName.isDefined)
     }
   )
 
@@ -530,4 +537,6 @@ object AppValidationMessages {
 
   val DockerEngineLimitedToSingleContainerNetwork =
     "may only specify a single container network when using the Docker container engine"
+
+  val NetworkNameMustBeSpecified = "Network name must be specified when container network is selected."
 }

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

@@ -10,6 +10,7 @@ import mesosphere.marathon.raml._
 import mesosphere.marathon.state.PathId
 import mesosphere.marathon.util.SemanticVersion
 import mesosphere.marathon.stream.Implicits._
+import org.rogach.scallop.ScallopOption
 // scalastyle:on
 
 /**
@@ -194,7 +195,7 @@ trait PodsValidation {
     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: ScallopOption[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)
@@ -211,6 +212,9 @@ trait PodsValidation {
     }
     pod.secrets is empty or (valid(secretValidator) and featureEnabled(enabledFeatures, Features.SECRETS))
     pod.networks is valid(ramlNetworksValidator)
+    pod.networks is isTrue[Seq[Network]]("network name must be specified when container network is selected") { nets =>
+      nets.forall(c => c.mode != NetworkMode.Container || c.name.isDefined || defaultNetworkName.isDefined)
+    }
     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
 import mesosphere.marathon.core.appinfo.AppInfo.Embed
 import mesosphere.marathon.core.appinfo._
 import mesosphere.marathon.core.base.ConstantClock
@@ -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(AppValidationMessages.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(AppValidationMessages.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

@@ -284,10 +284,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("network name must be specified")
     }
 
     "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, config)
         )
     )
   }
@@ -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)(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 }
 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"))
 
   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)))), "/", AppValidationMessages.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")
+  implicit val basicValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set.empty, config)
+  implicit val withSecretsValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set("secrets"), config)
+  implicit val withDefaultNetworkNameValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set.empty, configWithDefaultNetworkName)
 
   "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

@@ -5,7 +5,7 @@ import com.wix.accord.{ Result, Validator }
 import com.wix.accord.scalatest.ResultMatchers
 import mesosphere.{ UnitTest, ValidationTestLike }
 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.marathon.util.{ ScallopStub, SemanticVersion }
 
 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, ScallopStub(None))
+    val secretValidator: Validator[Pod] = podValidator(Set(Features.SECRETS), SemanticVersion.zero, ScallopStub(None))
   }
 
   "network validation" when {
-    val validator: Validator[Pod] = podValidator(Set.empty, SemanticVersion.zero)
+    val validator: Validator[Pod] = podValidator(Set.empty, SemanticVersion.zero, ScallopStub(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"))
   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)
   private implicit lazy val validAppDefinition = AppDefinition.validAppDefinition(Set())(PluginManager.None)
   private def validContainer(networks: Seq[Network] = Nil) = Container.validContainer(networks, Set())