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

alenkacz · 2017-07-02T06:21:13Z

Throws validation exception instead of just NormalizationException when default network name is not specified

unterstein

Hey Alena,
great PR! Thank you for taking the time to complete fix this issue. Just a really minor comment from my side, but no hurdle to merge.

Thanks
Johannes

unterstein · 2017-07-03T15:31:47Z

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

+  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)


I dont know why basicValidator and withSecretsValidator are declared as implicit, but I think we should change all three validators to regular vars.

agree, redundant implicit types are strange here. it's not the OPs job to fix, but I'd like to suggest that we at least refrain from continuing the pattern: withDefaultNetworkNameValidator should not be implicit.

unterstein · 2017-07-03T16:13:44Z

Hey @jdef,

when your back after US holidays, could you have a look, this would be awesome!

Thanks =)

jdef

thanks for the PR, left some feedback

jdef · 2017-07-06T14:18:07Z

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

@@ -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(


please use a defaultNetworkName: Option[String] here. let's not mix normalization APIs into the validation code

Due to the migration code here https://github.com/mesosphere/marathon/blob/master/src/main/scala/mesosphere/marathon/storage/migration/MigrationTo15.scala#L63 it was not able to pass the Option[String] directly. Any ideas here?

sure, refactor some bits in the migration code: extract the network name logic into a separate def that's reused by the normalization init code

We already had an discussion regarding this issue. There is an explicit override

override def defaultNetworkName: Option[String] = | env.vars.get(DefaultNetworkNameForMigratedApps).orElse(networkName).orElse(throw SerializationFailedException( | MigrationFailedMissingNetworkEnvVar))

which throws an exception if this is is None. But None is a valid state in some cases afterwards. Therefore we decided to pass this not directly and go for a lazy implementation.

What about adding a trait ValidationConfig which holds the enabledFeatures and defaultNetworkName ?

Trait works. Or else a func that produces an Option[String] (which would be even more minimal than a trait)

jdef · 2017-07-06T14:18:29Z

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

@@ -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(


ditto, Option[String]

jdef · 2017-07-06T14:19:16Z

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

@@ -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 =>


please don't use scallop here, it's not needed. use a simple Option[String] type instead. pretty sure that you can invoke .get on a ScallopOption[T] to get an Option[T] (to pass in here)

jdef · 2017-07-06T14:23:57Z

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

@@ -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 =>


this logic is the same for both apps and pods. perhaps extract into a NetworkValidation class and NetworkValidationMessages class (used by both app and pod validation), and use the static message field (instead of a literal string) for the validation error

jdef · 2017-07-06T14:25:53Z

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

+  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)


agree, redundant implicit types are strange here. it's not the OPs job to fix, but I'd like to suggest that we at least refrain from continuing the pattern: withDefaultNetworkNameValidator should not be implicit.

alenkacz · 2017-07-07T06:43:03Z

Thanks for the review! I'll fix those comments over the weekend :-)

alenkacz · 2017-08-02T19:59:08Z

@jdef @unterstein can you re-review please? I fixed all your comments

PS: Sorry for the delay - holidays :-)

alenkacz · 2017-08-14T06:20:10Z

@jdef @unterstein anything I can do to get this merged? :-)

jdef

nit, otherwise lgtm

jdef · 2017-08-14T14:44:48Z

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

-      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")


nit: reference NetworkValidationMessages.NetworkNameMustBeSpecified instead of hard coding the text message

alenkacz · 2017-08-16T18:14:43Z

@jdef good point - fixed

unterstein · 2017-08-21T10:49:45Z

Thanks @alenkacz !

    .  o ..                  
     o . o o.o                
          ...oo               
            __[]__          Shippin'  
         __|_o_o_o\__         it!
         \""""""""""/         
          \. ..  . /          
     ^^^^^^^^^^^^^^^^^^^^

unterstein approved these changes Jul 3, 2017

View reviewed changes

unterstein requested a review from jdef July 3, 2017 16:12

unterstein self-assigned this Jul 3, 2017

jdef suggested changes Jul 6, 2017

View reviewed changes

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

e1655cc

alenkacz force-pushed the alena/fix_MARATHON-7398 branch from 10d17d6 to e1655cc Compare August 1, 2017 19:45

alenkacz added 4 commits August 1, 2017 21:53

Remove redundant implicits from AppValidationTest

6e63069

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

a53dd26

Do not use scallopoption - use regular option instead

981812c

Extract common logic to NetworkValidation trait

360e4e5

unterstein approved these changes Aug 4, 2017

View reviewed changes

jdef suggested changes Aug 14, 2017

View reviewed changes

Use NetworkValidationMessages also in PodsResourceTest

07cf3ac

jdef approved these changes Aug 16, 2017

View reviewed changes

unterstein merged commit 6e13c9d into d2iq-archive:master Aug 21, 2017

kensipe mentioned this pull request Sep 26, 2017

bump to marathon 1.5.1 dcos/dcos#1967

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alenkacz commented Jul 2, 2017

unterstein left a comment

unterstein Jul 3, 2017

jdef Jul 6, 2017

unterstein commented Jul 3, 2017

jdef left a comment

jdef Jul 6, 2017

unterstein Jul 6, 2017

jdef Jul 6, 2017

unterstein Jul 6, 2017

jdef Jul 6, 2017

jdef Jul 6, 2017

jdef Jul 6, 2017

jdef Jul 6, 2017

jdef Jul 6, 2017

alenkacz commented Jul 7, 2017

alenkacz commented Aug 2, 2017

alenkacz commented Aug 14, 2017

jdef left a comment

jdef Aug 14, 2017

alenkacz commented Aug 16, 2017

unterstein commented Aug 21, 2017

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

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

Conversation

alenkacz commented Jul 2, 2017

unterstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unterstein commented Jul 3, 2017

jdef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz commented Jul 7, 2017

alenkacz commented Aug 2, 2017

alenkacz commented Aug 14, 2017

jdef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz commented Aug 16, 2017

unterstein commented Aug 21, 2017