HomeMesosphereNo notifications. 4 unresolved issues.
  • Search
Differential D703

Added strict mode to RamlTypeGenerator to be able to throw an validation error if unknown properties submitted via ValidationResource
Needs RevisionAll Users
Actions

Authored by unterstein on Apr 18 2017, 5:23 PM.

Details

Reviewers
jdef
• jasongilanfarr
aquamatthias
zen-dog
jenkins
JIRA Issues
JIRA MARATHON-3000 Don't allow unknown fields though the json deserializer
Summary

If unknown properties are submitted in a json posted to marathon, this should be validated at validation endpoint.

Depends on D698

Test Plan

sbt test

Diff Detail

Repository
rMARATHON marathon
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2210
Build 4174: Marathon (deprecated)Velocity Results
Build 4173: arc lint + arc unit
unterstein created this revision.Apr 18 2017, 5:23 PM
unterstein updated this revision to Diff 2715.Apr 18 2017, 5:39 PM
  • removed scalastyle:off
  • fixed typo and test fixture
unterstein edited the test plan for this revision. (Show Details)Apr 18 2017, 5:39 PM
unterstein edited the test plan for this revision. (Show Details)
unterstein edited the summary of this revision. (Show Details)
unterstein edited the test plan for this revision. (Show Details)
unterstein added a dependency: D698: Added basic functionality to have a dedicated validation endpoint which do only validation..
jenkins requested changes to this revision.Apr 18 2017, 5:41 PM
This revision now requires changes to proceed.Apr 18 2017, 5:41 PM
unterstein updated this revision to Diff 2716.Apr 18 2017, 5:48 PM
  • added failing test to also test embedded unknown properties
unterstein updated this revision to Diff 2717.Apr 18 2017, 5:53 PM
  • added additional test for unknown properties in pods
jenkins requested changes to this revision.Apr 18 2017, 5:57 PM
This revision now requires changes to proceed.Apr 18 2017, 5:57 PM
• jasongilanfarr requested changes to this revision.Apr 18 2017, 7:23 PM

I'm not sure why we would want non-strict mode... so with that in mind, if we only use strict, can we remove the stack/dependency too?

changelog.md
65

I feel like this should be a separate change item.

project/RamlTypeGenerator.scala
14

I didn't link the comments from the previous review. Why is the valuable information?

488

We should _just_ have strict mode no?!

492

Why do we want non-strict mode ever?

unterstein added a subscriber: meichstedt.Apr 18 2017, 8:02 PM
unterstein added inline comments.
project/RamlTypeGenerator.scala
14

It took me around 30 minutes to figure out that I need to do sbt reload and I wanted to prevent others from this loss of time. If this is not needed I can remove it.

On current master you need more then reload, right? There were some kind of caching added. What do I need to do correctly? I did clean, but probably this is too much, right?

492

It`s a breaking change. I can not make the decision to change the default. If we want to have it, @meichstedt or someone from the product team need to make this decision. I asked @meichstedt to discuss it.

A plan could be to announce this with 1.5 and the validation endpoint and then remove the non-strict way.

• jasongilanfarr added inline comments.Apr 18 2017, 8:06 PM
project/RamlTypeGenerator.scala
14

I guess I just thought this was totally obvious since its part of the project itself and all of those files need a reload. I don't believe the caching needs to change, but we could record the last modified time of this file to see if the cache is invalid anyways.

492

I thought the JIRA for this already said that our current behavior is actually a bug and I feel like I recently saw an issue come up where the user was confused. I don't think its actually breaking since we don't preserve the information anyways... @jdef ?

If anything, I'd love to see "allowDeprecated" as an option so we can warn in the validation endpoint.

jdef added inline comments.Apr 18 2017, 8:38 PM
changelog.md
65

the summary and title of this changeset mentioned the new validation endpoint. This comment mentions /v2/apps. Confusing at best, likely something is wrong somewhere

project/RamlTypeGenerator.scala
14

FWIW I've never had to sbt reload to pick up changes to this file

487

please provide some sample scala output generated by these changes

492

(a) it's potentially "breaking" if you try to install a universe package that has a marathon.json template w/ bogus fields. or if Alice or Dan has some custom tooling/scripting that's been getting away with submitting bad JSON to marathon -- that tooling will now break if we always enforce strict mode. I think that I can live with that as long as we thoroughly call this out as a potentially breaking change in the changelog. since marathon 1.5 should only ever emit valid JSON fields .. I think I'm ok with this kind of breaking change.

the only edge case that I can think of is that Alice or Dan queries a groups endpoint, and tries to feed the json back into Marathon. that's going to break w/ strict field validation because the groups endpoint potentially embeds lots of fields that are really part of, for example, an AppInfo type, and not App. should we be worried about this? maybe... or maybe not: users that want to do this are already faced with the prospect that marathon 1.4 and prior output JSON that can't be fed back into Marathon without cleanup because in 1.4 and prior Marathon actually populates deprecated fields upon serialization -- a practice that has already stopped with Marathon 1.5

(b) yes, i'd also rather see a flag to toggle on/off support for deprecated fields. in a separate PR, probably tracked by a separate JIRA

• jasongilanfarr added inline comments.Apr 20 2017, 8:50 PM
project/RamlTypeGenerator.scala
14

Well that's some black magic. all of *.sbt and project/*.scala have to be reloaded to get their changes picked up.

492

+1 on (b) - JIRA and PR separately.

Revision Contents

Diff 2717

View Options

changelog.md

Show First 20 Lines • Show All 56 Lines • ▼ Show 20 Line(s)
5757
5858
5959### Overview
6060
6161#### New validation endpoint
6262It is now possible to validate a given app or pod definition against a dedicated validation endpoint `/v2/validations`.
6363You can `POST` an app definition to `/v2/validations/app` and you receive a `HTTP 200` if validation succeeds or a `HTTP 422` plus a error json if validation fails.
6464Same for pod definitions on `/v2/validations/pod`.
65
65In addition to `POST` on `/v2/apps` for example, this validation will raise a validation error in case of unknown properties.
jasongilanfarrUnsubmitted
Not Done

I feel like this should be a separate change item.

jdefUnsubmitted
Not Done

the summary and title of this changeset mentioned the new validation endpoint. This comment mentions /v2/apps. Confusing at best, likely something is wrong somewhere

6666
6767## Changes from 1.4.1 to 1.4.2
6868Bugfix release
6969
7070### Fixed issues
7171- [MARATHON-4570](https://jira.mesosphere.com/browse/MARATHON-4570) Don't destroy persistent volumes when killing unreachable tasks
7272- [MARATHON-4390](https://jira.mesosphere.com/browse/MARATHON-4390) Lazy event parsing
7373- Improve health checks to log less warnings.
▲ Show 20 Lines • Show All 1929 Lines • Show Last 20 Lines
View Options

project/RamlTypeGenerator.scala

11package mesosphere.raml
22
33import treehugger.forest._
44import definitions._
55import org.raml.v2.api.RamlModelResult
66import org.raml.v2.api.model.v10.api.Library
77import org.raml.v2.api.model.v10.datamodel._
88import treehuggerDSL._
99
1010import scala.annotation.tailrec
1111import scala.collection.JavaConversions._
1212import scala.collection.immutable.Seq
1313
14// scalastyle:off
14/**
jasongilanfarrUnsubmitted
Not Done

I didn't link the comments from the previous review. Why is the valuable information?

untersteinAuthorUnsubmitted
Not Done

It took me around 30 minutes to figure out that I need to do sbt reload and I wanted to prevent others from this loss of time. If this is not needed I can remove it.

On current master you need more then reload, right? There were some kind of caching added. What do I need to do correctly? I did clean, but probably this is too much, right?

jasongilanfarrUnsubmitted
Not Done

I guess I just thought this was totally obvious since its part of the project itself and all of those files need a reload. I don't believe the caching needs to change, but we could record the last modified time of this file to see if the cache is invalid anyways.

jdefUnsubmitted
Not Done

FWIW I've never had to sbt reload to pick up changes to this file

jasongilanfarrUnsubmitted
Not Done

Well that's some black magic. all of *.sbt and project/*.scala have to be reloaded to get their changes picked up.

15 * Attention: This class is only re-compiled during `sbt reload`
16 */
1517object RamlTypeGenerator {
1618 val AdditionalProperties = "additionalProperties"
1719 val baseTypeTable: Map[String, Symbol] =
1820 Map(
1921 "string" -> StringClass,
2022 "int8" -> ByteClass,
2123 "int16" -> ShortClass,
2224 "int32" -> IntClass,
▲ Show 20 Lines • Show All 454 Lines • ▼ Show 20 Line(s)
477479 REF("playJsonWriter") DOT "writes" APPLY REF("o")
478480 )
479481 )
480482 )
481483 } else if (actualFields.size > 22 || actualFields.exists(f => f.repeated || f.omitEmpty || f.constraints.nonEmpty) ||
482484 actualFields.map(_.toString).exists(t => t.toString.startsWith(name) || t.toString.contains(s"[$name]"))) {
483485 actualFields.map(_.constraints).requiredImports ++ Seq(
484486 OBJECTDEF("playJsonFormat") withParents PLAY_JSON_FORMAT(name) withFlags Flags.IMPLICIT := BLOCK(
485 DEF("reads", PLAY_JSON_RESULT(name)) withParams PARAM("json", PlayJsValue) := BLOCK(
487 DEF("reads", PLAY_JSON_RESULT(name)) withParams PARAM("json", PlayJsValue) := REF("reads") APPLY (REF("json"), FALSE),
jdefUnsubmitted
Not Done

please provide some sample scala output generated by these changes

488 DEF("reads", PLAY_JSON_RESULT(name)) withParams Seq(PARAM("json", PlayJsValue).tree, (PARAM("strict", BooleanClass)).tree) := BLOCK(
jasongilanfarrUnsubmitted
Not Done

We should _just_ have strict mode no?!

486489 actualFields.map { field =>
487490 VAL(field.name) := field.playValidator
488491 } ++ Seq(
489 VAL("_errors") := SEQ(actualFields.map(f => TUPLE(LIT(f.name), REF(f.name)))) DOT "collect" APPLY BLOCK(
492 VAL("jsonFields") withType ("Seq[String]") := REF("json") MATCH(
jasongilanfarrUnsubmitted
Not Done

Why do we want non-strict mode ever?

untersteinAuthorUnsubmitted
Not Done

It`s a breaking change. I can not make the decision to change the default. If we want to have it, @meichstedt or someone from the product team need to make this decision. I asked @meichstedt to discuss it.

A plan could be to announce this with 1.5 and the validation endpoint and then remove the non-strict way.

jasongilanfarrUnsubmitted
Not Done

I thought the JIRA for this already said that our current behavior is actually a bug and I feel like I recently saw an issue come up where the user was confused. I don't think its actually breaking since we don't preserve the information anyways... @jdef ?

If anything, I'd love to see "allowDeprecated" as an option so we can warn in the validation endpoint.

jdefUnsubmitted
Not Done

(a) it's potentially "breaking" if you try to install a universe package that has a marathon.json template w/ bogus fields. or if Alice or Dan has some custom tooling/scripting that's been getting away with submitting bad JSON to marathon -- that tooling will now break if we always enforce strict mode. I think that I can live with that as long as we thoroughly call this out as a potentially breaking change in the changelog. since marathon 1.5 should only ever emit valid JSON fields .. I think I'm ok with this kind of breaking change.

the only edge case that I can think of is that Alice or Dan queries a groups endpoint, and tries to feed the json back into Marathon. that's going to break w/ strict field validation because the groups endpoint potentially embeds lots of fields that are really part of, for example, an AppInfo type, and not App. should we be worried about this? maybe... or maybe not: users that want to do this are already faced with the prospect that marathon 1.4 and prior output JSON that can't be fed back into Marathon without cleanup because in 1.4 and prior Marathon actually populates deprecated fields upon serialization -- a practice that has already stopped with Marathon 1.5

(b) yes, i'd also rather see a flag to toggle on/off support for deprecated fields. in a separate PR, probably tracked by a separate JIRA

jasongilanfarrUnsubmitted
Not Done

+1 on (b) - JIRA and PR separately.

490 CASE(REF(s"(field, e:$PlayJsError)")) ==> (REF("e") DOT "repath" APPLY(REF(PlayPath) DOT "\\" APPLY REF("field"))) DOT s"asInstanceOf[$PlayJsError]"),
493 CASE(ID("obj") withType (PlayJsObject)) ==> (REF("obj") DOT "value" DOT "keys" DOT "toSeq"),
494 CASE(ID("obj")) ==> REF("Seq.empty[String]")
495 ),
496 VAL("ramlFields") := SEQ(actualFields.map(f => TUPLE(LIT(f.name), REF(f.name)))),
497 VAL("ramlIds") := REF("ramlFields") DOT "map" APPLY REF("_._1"),
498 VAL("_errors") := REF("ramlFields") DOT "collect" APPLY BLOCK(
499 CASE(REF(s"(field, e:$PlayJsError)")) ==> (REF("e") DOT "repath" APPLY(REF(PlayPath) DOT "\\" APPLY REF("field"))) DOT s"asInstanceOf[$PlayJsError]")
500 DOT "++" APPLY(
501 IF(REF("strict")) THEN (
502 (REF("jsonFields") DOT "filterNot" APPLY(REF("ramlIds") DOT "contains") DOT "map" APPLY (REF("a => ") APPLY
503 (REF(PlayJsError) APPLY (REF(PlayPath) DOT "\\" APPLY REF("a"), REF(PlayValidationError) APPLY(LIT("error.unknown.property"))))
504 ))) ELSE SEQ()),
491505 IF(REF("_errors") DOT "nonEmpty") THEN (
492506 REF("_errors") DOT "reduceOption" APPLYTYPE PlayJsError APPLY (REF("_") DOT "++" APPLY REF("_")) DOT "getOrElse" APPLY (REF("_errors") DOT "head")
493507 ) ELSE (
494508 REF(PlayJsSuccess) APPLY (REF(name) APPLY
495509 actualFields.map { field =>
496510 REF(field.name) := (REF(field.name) DOT "get")
497511 }))
498512 )
▲ Show 20 Lines • Show All 429 Lines • Show Last 20 Lines
View Options

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

Show First 20 Lines • Show All 48 Lines • ▼ Show 20 Line(s)
4949
5050 @POST
5151 @Path("app")
5252 def validateApp(
5353 body: Array[Byte],
5454 @Context req: HttpServletRequest): Response = authenticated(req) { implicit identity =>
5555
5656 assumeValid {
57 val deserializedApp = raml.App.playJsonFormat.reads(Json.parse(body))
57 val deserializedApp = raml.App.playJsonFormat.reads(Json.parse(body), strict = true)
5858 val rawApp: AppDefinition = deserializedApp match {
5959 case success: JsSuccess[raml.App] => Raml.fromRaml(normalizeApp(success.get))
6060 case error: JsError => throw JsResultException(error.errors)
6161 }
6262 val now = clock.now()
6363 val app = validateOrThrow(rawApp).copy(versionInfo = VersionInfo.OnlyVersion(now))
6464 checkAuthorization(CreateRunSpec, app)
6565
6666 Response
6767 .ok()
6868 .build()
6969 }
7070 }
7171
7272 @POST
7373 @Path("pod")
7474 def validatePod(
7575 body: Array[Byte],
7676 @Context req: HttpServletRequest): Response = {
7777 authenticated(req) { implicit identity =>
78 val deserializedPod = raml.Pod.playJsonFormat.reads(Json.parse(body))
78 val deserializedPod = raml.Pod.playJsonFormat.reads(Json.parse(body), strict = true)
7979 val rawPod: Pod = deserializedPod match {
8080 case success: JsSuccess[raml.Pod] => normalizePod(success.value)
8181 case error: JsError => throw JsResultException(error.errors)
8282 }
8383 withValid(rawPod) { podDef =>
8484 val pod = normalizePod(Raml.fromRaml(normalizePod(podDef)))
8585 withAuthorization(CreateRunSpec, pod) {
8686
8787 Response
8888 .ok()
8989 .build()
9090 }
9191 }
9292 }
9393 }
9494}
View Options

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

Show First 20 Lines • Show All 89 Lines • ▼ Show 20 Line(s)
9090 val body = Json.stringify(Json.toJson(pod)).getBytes
9191 val ex = intercept[JsResultException](validationsResource.validatePod(body, auth.request))
9292
9393 Then("correct error")
9494 val error = ex.errors.head
9595 error._1 should be(JsPath(List(KeyPathNode("containers"))))
9696 error._2.head should be(ValidationError("error.minLength", 1))
9797 }
98
99 "validate an app with unknown properties" in new Fixture {
100 Given("app definition")
101
102 val body =
103 """{
104 | "id": "/test",
105 | "cmd": "sleep 10",
106 | "foo" : "bar"
107 |}""".stripMargin.getBytes("UTF-8")
108
109 val result = validationsResource.validateApp(body, auth.request)
110
111 Then("http result is 422 and correct error")
112 result.getStatus shouldEqual 422
113
114 val json = Json.parse(result.getEntity.toString)
115 (json \ "message") should be(JsDefined(JsString("Invalid JSON")))
116 (json \ "details" \ 0 \ "path") should be(JsDefined(JsString("/foo")))
117 }
118 "validate a pod with unknown properties" in new Fixture {
119 Given("app definition")
120
121 val body = """
122 | { "id": "/mypod", "networks": [ { "mode": "host" } ],
123 | "foo": "bar",
124 | "containers": [
125 | { "name": "webapp",
126 | "resources": { "cpus": 0.03, "mem": 64 },
127 | "image": { "kind": "DOCKER", "id": "busybox" },
128 | "exec": { "command": { "shell": "sleep 1" } } } ] }
129 """.stripMargin.getBytes("UTF-8")
130
131 val ex = intercept[JsResultException](validationsResource.validatePod(body, auth.request))
132
133 Then("correct error")
134 val error = ex.errors.head
135 error._1 should be(JsPath(List(KeyPathNode("foo"))))
136 error._2.head should be(ValidationError("error.unknown.property"))
137 }
138
139 "validate an app with unknown and nested properties" in new Fixture {
140 Given("app definition")
141
142 val body =
143 """{
144 | "id": "/test",
145 | "cmd": "sleep 10",
146 | "upgradeStrategy":{
147 | "minimumHealthCapacity":0.85,
148 | "maximumOverCapacity":0.15,
149 | "foo": "bar"
150 | }
151 |}""".stripMargin.getBytes("UTF-8")
152
153 val result = validationsResource.validateApp(body, auth.request)
154
155 Then("http result is 422 and correct error")
156 result.getStatus shouldEqual 422
157
158 val json = Json.parse(result.getEntity.toString)
159 (json \ "message") should be(JsDefined(JsString("Invalid JSON")))
160 (json \ "details" \ 0 \ "path") should be(JsDefined(JsString("/upgradeStrategy/foo")))
161 }
98162 }
99163}
These changes have not finished building yet and may have build failures.