From e1655cc18cf1baa6bce0d9b319f4535ae40161d7 Mon Sep 17 00:00:00 2001 From: Alena Varkockova Date: Wed, 28 Jun 2017 16:08:46 +0200 Subject: [PATCH 1/6] App and pod validation errors for missing network name MARATHON-7398 --- .../api/akkahttp/v2/AppsController.scala | 4 ++-- .../marathon/api/v2/AppsResource.scala | 6 ++--- .../marathon/api/v2/PodsResource.scala | 2 +- .../api/v2/validation/AppValidation.scala | 13 +++++++++-- .../api/v2/validation/PodsValidation.scala | 6 ++++- .../marathon/api/v2/AppsResourceTest.scala | 23 ++++++++++++++++--- .../marathon/api/v2/PodsResourceTest.scala | 7 +++--- .../v2/json/AppDefinitionFormatsTest.scala | 5 ++-- .../marathon/api/v2/json/AppUpdateTest.scala | 5 ++-- .../api/v2/validation/AppValidationTest.scala | 19 +++++++++++++-- .../v2/validation/PodsValidationTest.scala | 10 ++++---- .../validation/AppUpdateValidatorTest.scala | 2 +- .../api/validation/RunSpecValidatorTest.scala | 3 ++- 13 files changed, 76 insertions(+), 29 deletions(-) diff --git a/src/main/scala/mesosphere/marathon/api/akkahttp/v2/AppsController.scala b/src/main/scala/mesosphere/marathon/api/akkahttp/v2/AppsController.scala index 4ff7004c595..8eb933c1201 100644 --- a/src/main/scala/mesosphere/marathon/api/akkahttp/v2/AppsController.scala +++ b/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) } } diff --git a/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala b/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala index 2f280fdc08b..fe46db55545 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala +++ b/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) } diff --git a/src/main/scala/mesosphere/marathon/api/v2/PodsResource.scala b/src/main/scala/mesosphere/marathon/api/v2/PodsResource.scala index 0d4459b138c..6bfadc4f06e 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/PodsResource.scala +++ b/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 diff --git a/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala index 6c661c5a812..511f8ed6584 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala +++ b/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." } diff --git a/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala index b0c476b6cb8..b902433c908 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala +++ b/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 diff --git a/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala b/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala index a7718cb06b3..68b8fc9473d 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala +++ b/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")) { diff --git a/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala b/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala index 4e4e5470691..ae46a97df65 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala +++ b/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 { diff --git a/src/test/scala/mesosphere/marathon/api/v2/json/AppDefinitionFormatsTest.scala b/src/test/scala/mesosphere/marathon/api/v2/json/AppDefinitionFormatsTest.scala index 670b2a4afb6..31571e82b52 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/json/AppDefinitionFormatsTest.scala +++ b/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,6 +682,7 @@ 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", @@ -689,7 +690,7 @@ class AppDefinitionFormatsTest extends UnitTest | "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") ) } diff --git a/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala b/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala index 1c5ac76f6a9..6dee4f5cb03 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala +++ b/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 { diff --git a/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala b/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala index 65b13c46e21..79a192684b0 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala @@ -3,6 +3,7 @@ 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 } @@ -10,8 +11,11 @@ class AppValidationTest extends UnitTest with ResultMatchers with ValidationTest 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( diff --git a/src/test/scala/mesosphere/marathon/api/v2/validation/PodsValidationTest.scala b/src/test/scala/mesosphere/marathon/api/v2/validation/PodsValidationTest.scala index 6f3d062c257..9e083c7fe5d 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/validation/PodsValidationTest.scala +++ b/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( diff --git a/src/test/scala/mesosphere/marathon/api/validation/AppUpdateValidatorTest.scala b/src/test/scala/mesosphere/marathon/api/validation/AppUpdateValidatorTest.scala index 5cbffa36497..f698c34792f 100644 --- a/src/test/scala/mesosphere/marathon/api/validation/AppUpdateValidatorTest.scala +++ b/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 { diff --git a/src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala b/src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala index 9c3f01cbbaf..fb9b1ab426c 100644 --- a/src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala +++ b/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()) From 6e6306984fac487162b36a9140fb3c47af44f953 Mon Sep 17 00:00:00 2001 From: Alena Varkockova Date: Tue, 1 Aug 2017 21:53:49 +0200 Subject: [PATCH 2/6] Remove redundant implicits from AppValidationTest --- .../marathon/api/v2/validation/AppValidationTest.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala b/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala index 79a192684b0..c9a14dfa880 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala @@ -13,9 +13,9 @@ class AppValidationTest extends UnitTest with ResultMatchers with ValidationTest 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) + val basicValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set.empty, config) + val withSecretsValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set("secrets"), config) + 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 { From a53dd269d391f48aba03391917425d99fcdf206e Mon Sep 17 00:00:00 2001 From: Alena Varkockova Date: Tue, 1 Aug 2017 22:21:40 +0200 Subject: [PATCH 3/6] Do not pass whole config, use just func returning default network name --- .../marathon/api/akkahttp/v2/AppsController.scala | 4 ++-- .../scala/mesosphere/marathon/api/v2/AppsResource.scala | 6 +++--- .../marathon/api/v2/validation/AppValidation.scala | 8 ++++---- .../marathon/api/v2/json/AppDefinitionFormatsTest.scala | 4 ++-- .../mesosphere/marathon/api/v2/json/AppUpdateTest.scala | 2 +- .../marathon/api/v2/validation/AppValidationTest.scala | 6 +++--- .../marathon/api/validation/AppUpdateValidatorTest.scala | 2 +- .../marathon/api/validation/RunSpecValidatorTest.scala | 2 +- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/main/scala/mesosphere/marathon/api/akkahttp/v2/AppsController.scala b/src/main/scala/mesosphere/marathon/api/akkahttp/v2/AppsController.scala index 8eb933c1201..88457e7479c 100644 --- a/src/main/scala/mesosphere/marathon/api/akkahttp/v2/AppsController.scala +++ b/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, normalizationConfig) + private implicit lazy val updateValidator = AppValidation.validateCanonicalAppUpdateAPI(config.availableFeatures, () => normalizationConfig.defaultNetworkName) 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, config.config)) + validateOrThrow(migrated)(AppValidation.validateCanonicalAppAPI(config.enabledFeatures, () => config.config.defaultNetworkName)) AppNormalization(config.config).normalized(migrated) } } diff --git a/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala b/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala index fe46db55545..a5044f4a2b2 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala +++ b/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, normalizationConfig) + private implicit lazy val validateCanonicalAppUpdateAPI = AppValidation.validateCanonicalAppUpdateAPI(config.availableFeatures, () => normalizationConfig.defaultNetworkName) 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, config.config)) + 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, config.config)) + validateOrThrow(app)(AppValidation.validateCanonicalAppUpdateAPI(config.enabledFeatures, () => config.config.defaultNetworkName)) AppNormalization.forUpdates(config.config).normalized(migrated) } diff --git a/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala index 511f8ed6584..5cb59d711de 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala @@ -305,7 +305,7 @@ trait AppValidation { } ) - def validateCanonicalAppUpdateAPI(enabledFeatures: Set[String], normalizationConfig: AppNormalization.Config): 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)) @@ -326,7 +326,7 @@ trait AppValidation { !(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)) + update.networks.forall(n => n.forall(c => c.mode != NetworkMode.Container || c.name.isDefined || defaultNetworkName().isDefined)) }, isTrue("The 'version' field may only be combined with the 'id' field.") { update => def onlyVersionOrIdSet: Boolean = update.productIterator.forall { @@ -371,7 +371,7 @@ trait AppValidation { } ) - def validateCanonicalAppAPI(enabledFeatures: Set[String], normalizationConfig: AppNormalization.Config): 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) @@ -386,7 +386,7 @@ trait AppValidation { !(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) + app.networks.forall(c => c.mode != NetworkMode.Container || c.name.isDefined || defaultNetworkName().isDefined) } ) diff --git a/src/test/scala/mesosphere/marathon/api/v2/json/AppDefinitionFormatsTest.scala b/src/test/scala/mesosphere/marathon/api/v2/json/AppDefinitionFormatsTest.scala index 31571e82b52..b81107a5935 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/json/AppDefinitionFormatsTest.scala +++ b/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, config) + AppValidation.validateCanonicalAppAPI(Set.empty, () => None) ) ) } @@ -690,7 +690,7 @@ class AppDefinitionFormatsTest extends UnitTest | "container": {} |}""".stripMargin) val ramlApp = json.as[raml.App] - AppValidation.validateCanonicalAppAPI(Set.empty, config)(ramlApp) should failWith( + AppValidation.validateCanonicalAppAPI(Set.empty, () => config.defaultNetworkName)(ramlApp) should failWith( group("container", "is invalid", "docker" -> "not defined") ) } diff --git a/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala b/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala index 6dee4f5cb03..292057f31e5 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala @@ -18,7 +18,7 @@ import scala.collection.immutable.Seq class AppUpdateTest extends UnitTest { - implicit val appUpdateValidator: Validator[AppUpdate] = AppValidation.validateCanonicalAppUpdateAPI(Set("secrets"), AppNormalization.Configuration(None, "mesos-bridge-name")) + implicit val appUpdateValidator: Validator[AppUpdate] = AppValidation.validateCanonicalAppUpdateAPI(Set("secrets"), () => AppNormalization.Configuration(None, "mesos-bridge-name").defaultNetworkName) val runSpecId = PathId("/test") diff --git a/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala b/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala index c9a14dfa880..87c5bf4b9d2 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/validation/AppValidationTest.scala @@ -13,9 +13,9 @@ class AppValidationTest extends UnitTest with ResultMatchers with ValidationTest 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) - val withSecretsValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set("secrets"), config) - val withDefaultNetworkNameValidator: Validator[App] = AppValidation.validateCanonicalAppAPI(Set.empty, configWithDefaultNetworkName) + 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 { diff --git a/src/test/scala/mesosphere/marathon/api/validation/AppUpdateValidatorTest.scala b/src/test/scala/mesosphere/marathon/api/validation/AppUpdateValidatorTest.scala index f698c34792f..79e0bf0263b 100644 --- a/src/test/scala/mesosphere/marathon/api/validation/AppUpdateValidatorTest.scala +++ b/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, AppNormalization.Configuration(None, "mesos-bridge-name")) + 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 { diff --git a/src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala b/src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala index fb9b1ab426c..845926133c1 100644 --- a/src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala +++ b/src/test/scala/mesosphere/marathon/api/validation/RunSpecValidatorTest.scala @@ -22,7 +22,7 @@ import scala.reflect.ClassTag class RunSpecValidatorTest extends UnitTest { val config = AppNormalization.Configuration(None, "mesos-bridge-name") - private implicit lazy val validApp = AppValidation.validateCanonicalAppAPI(Set(), config) + 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()) From 981812c999cf57e9fd0194b18fe41a8f42e040a9 Mon Sep 17 00:00:00 2001 From: Alena Varkockova Date: Tue, 1 Aug 2017 22:33:06 +0200 Subject: [PATCH 4/6] Do not use scallopoption - use regular option instead --- .../mesosphere/marathon/api/v2/PodsResource.scala | 2 +- .../marathon/api/v2/validation/PodsValidation.scala | 3 +-- .../api/v2/validation/PodsValidationTest.scala | 12 ++++++------ 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/main/scala/mesosphere/marathon/api/v2/PodsResource.scala b/src/main/scala/mesosphere/marathon/api/v2/PodsResource.scala index 6bfadc4f06e..df797f19092 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/PodsResource.scala +++ b/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)), config.defaultNetworkName) + 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 diff --git a/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala index b902433c908..5d98790a50e 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala @@ -10,7 +10,6 @@ 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 /** @@ -195,7 +194,7 @@ trait PodsValidation { hostPorts.distinct.size == hostPorts.size } - def podValidator(enabledFeatures: Set[String], mesosMasterVersion: SemanticVersion, defaultNetworkName: ScallopOption[String]): 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) diff --git a/src/test/scala/mesosphere/marathon/api/v2/validation/PodsValidationTest.scala b/src/test/scala/mesosphere/marathon/api/v2/validation/PodsValidationTest.scala index 9e083c7fe5d..19d24dd603f 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/validation/PodsValidationTest.scala +++ b/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.{ ScallopStub, SemanticVersion } +import mesosphere.marathon.util.SemanticVersion +import mesosphere.{ UnitTest, ValidationTestLike } class PodsValidationTest extends UnitTest with ResultMatchers with PodsValidation with SchedulingValidation with ValidationTestLike { @@ -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, ScallopStub(None)) - val secretValidator: Validator[Pod] = podValidator(Set(Features.SECRETS), SemanticVersion.zero, ScallopStub(None)) + 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, ScallopStub(Some("default-network-name"))) + 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( From 360e4e5ac8807f02ad683d63f53bde4a8e8b411b Mon Sep 17 00:00:00 2001 From: Alena Varkockova Date: Wed, 2 Aug 2017 21:56:56 +0200 Subject: [PATCH 5/6] Extract common logic to NetworkValidation trait --- .../marathon/api/v2/validation/AppValidation.scala | 11 ++--------- .../api/v2/validation/NetworkValidation.scala | 9 +++++++++ .../marathon/api/v2/validation/PodsValidation.scala | 6 ++---- .../mesosphere/marathon/api/v2/AppsResourceTest.scala | 6 +++--- .../mesosphere/marathon/api/v2/PodsResourceTest.scala | 2 +- .../marathon/api/v2/json/AppUpdateTest.scala | 4 ++-- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala index 5cb59d711de..662fe1b5d6e 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/validation/AppValidation.scala @@ -5,7 +5,6 @@ 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._ @@ -316,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 }), @@ -325,9 +325,6 @@ 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 || 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 @@ -376,6 +373,7 @@ trait AppValidation { 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 }), @@ -384,9 +382,6 @@ 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 || defaultNetworkName().isDefined) } ) @@ -537,6 +532,4 @@ 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." } diff --git a/src/main/scala/mesosphere/marathon/api/v2/validation/NetworkValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/validation/NetworkValidation.scala index eb2ab37a74e..262f8794cc3 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/validation/NetworkValidation.scala +++ b/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." +} \ No newline at end of file diff --git a/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala b/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala index 5d98790a50e..f630a85ddea 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/validation/PodsValidation.scala @@ -8,8 +8,8 @@ import com.wix.accord.dsl._ import mesosphere.marathon.api.v2.Validation import mesosphere.marathon.raml._ import mesosphere.marathon.state.PathId -import mesosphere.marathon.util.SemanticVersion import mesosphere.marathon.stream.Implicits._ +import mesosphere.marathon.util.SemanticVersion // scalastyle:on /** @@ -211,9 +211,7 @@ 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.networks is defaultNetworkNameValidator(() => defaultNetworkName) pod.scheduling is optional(schedulingValidator) pod.scaling is optional(scalingValidator) pod is endpointNamesUnique and endpointContainerPortsUnique and endpointHostPortsUnique diff --git a/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala b/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala index 68b8fc9473d..6cae30bc263 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala @@ -7,7 +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.api.v2.validation.{ AppValidationMessages, NetworkValidationMessages } import mesosphere.marathon.core.appinfo.AppInfo.Embed import mesosphere.marathon.core.appinfo._ import mesosphere.marathon.core.base.ConstantClock @@ -287,7 +287,7 @@ class AppsResourceTest extends AkkaUnitTest with GroupCreation { Then("Validation fails") response.getStatus should be(422) - response.getEntity.toString should include(AppValidationMessages.NetworkNameMustBeSpecified) + 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 { @@ -398,7 +398,7 @@ class AppsResourceTest extends AkkaUnitTest with GroupCreation { Then("the update should fail") response.getStatus should be(422) - response.getEntity.toString should include(AppValidationMessages.NetworkNameMustBeSpecified) + 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")) { diff --git a/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala b/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala index ae46a97df65..f8e13debb00 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala @@ -286,7 +286,7 @@ class PodsResourceTest extends AkkaUnitTest with Mockito { 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") + response.getEntity.toString should include("Network name must be specified") } "create a pod with custom executor resource declaration" in { diff --git a/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala b/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala index 292057f31e5..59c47801f9f 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/json/AppUpdateTest.scala +++ b/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, AppValidationMessages } +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 } @@ -83,7 +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) + shouldViolate(update.copy(networks = Some(Seq(Network(mode = NetworkMode.Container)))), "/networks", NetworkValidationMessages.NetworkNameMustBeSpecified) } "Validate secrets" in { From 07cf3ac8ed380f4cceb28d0403b9fe9be1e6e21a Mon Sep 17 00:00:00 2001 From: Alena Varkockova Date: Wed, 16 Aug 2017 20:13:41 +0200 Subject: [PATCH 6/6] Use NetworkValidationMessages also in PodsResourceTest --- .../scala/mesosphere/marathon/api/v2/PodsResourceTest.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala b/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala index f8e13debb00..0df53c394c8 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/PodsResourceTest.scala +++ b/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 @@ -286,7 +287,7 @@ class PodsResourceTest extends AkkaUnitTest with Mockito { 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") + response.getEntity.toString should include(NetworkValidationMessages.NetworkNameMustBeSpecified) } "create a pod with custom executor resource declaration" in {