diff --git a/changelog.md b/changelog.md index 86e89438271..0589397ad21 100644 --- a/changelog.md +++ b/changelog.md @@ -151,6 +151,14 @@ The artifact store was introduced as an easy solution to store and retrieve arti There is a variety of tools that can handle this functionality better then Marathon. We will remove this functionality from Marathon without replacement. +#### Deprecate PATCH semantic for PUT on /v2/apps + +A PUT on /v2/apps has a PATCH like semantic: +All values that are not defined in the json, will not update existing values. +This was always the default behaviour in Marathon versions. +For backward compatibility, we will not change this behaviour, but let users opt in for a proper PUT. +The next version of Marathon will use PATCH and PUT as two separate actions. + #### Forcefully stop a deployment Deployments in Marathon can be stopped with force. diff --git a/docs/docs/rest-api/public/api/v2/apps.raml b/docs/docs/rest-api/public/api/v2/apps.raml index 76140df7867..5f1b3211307 100644 --- a/docs/docs/rest-api/public/api/v2/apps.raml +++ b/docs/docs/rest-api/public/api/v2/apps.raml @@ -90,6 +90,16 @@ Note: This operation will create a deployment. The operation finishes, if the deployment succeeds. You can query the deployments endoint to see the status of the deployment. is: [ secured, deployable ] + queryParameters: + partialUpdate: + required: false + default: true + description: | + Without specifying this parameter, this method has a patch like semantic: + All values that are not defined in the json, will not change existing values. + This was the default behaviour in previous Marathon versions. + For backward compatibility, we will not change this behaviour, but let users opt in for a proper PUT. + Note: We will change the default behaviour in the next Marathon version to support PATCH and PUT as HTTP methods. body: application/json: example: !include examples/apps_create.json @@ -127,9 +137,6 @@ ] } - - - ### # Create application # @@ -182,7 +189,6 @@ ] } - /{app_id}: ### @@ -251,6 +257,16 @@ Note: This operation will create a deployment. The operation finishes, if the deployment succeeds. You can query the deployments endoint to see the status of the deployment. is: [ secured, deployable ] + queryParameters: + partialUpdate: + required: false + default: true + description: | + Without specifying this parameter, this method has a patch like semantic: + All values that are not defined in the json, will not change existing values. + This was the default behaviour in previous Marathon versions. + For backward compatibility, we will not change this behaviour, but let users opt in for a proper PUT. + Note: We will change the default behaviour in the next Marathon version to support PATCH and PUT as HTTP methods. body: application/json: example: !include examples/app.json @@ -293,8 +309,6 @@ ] } - - ### # Delete a specific app # diff --git a/src/main/scala/mesosphere/marathon/api/RestResource.scala b/src/main/scala/mesosphere/marathon/api/RestResource.scala index a9118259dc2..6c747c4cc74 100644 --- a/src/main/scala/mesosphere/marathon/api/RestResource.scala +++ b/src/main/scala/mesosphere/marathon/api/RestResource.scala @@ -3,13 +3,14 @@ package mesosphere.marathon.api import java.net.URI import javax.ws.rs.core.Response import javax.ws.rs.core.Response.{ ResponseBuilder, Status } + +import akka.http.scaladsl.model.StatusCodes import com.wix.accord._ -import mesosphere.marathon.MarathonConf +import mesosphere.marathon.{ MarathonConf, ValidationFailedException } import mesosphere.marathon.api.v2.Validation._ import mesosphere.marathon.api.v2.json.Formats._ import mesosphere.marathon.state.{ PathId, Timestamp } import mesosphere.marathon.upgrade.DeploymentPlan - import play.api.libs.json.Json.JsValueWrapper import play.api.libs.json.{ Json, Writes } @@ -74,6 +75,19 @@ trait RestResource { case Success => fn(t) } } + + /** + * Execute the given function and if any validation errors crop up, generate an UnprocessableEntity + * HTTP status code and send the validation error as the response body (in JSON form). + */ + protected def assumeValid(f: => Response): Response = + try { + f + } catch { + case vfe: ValidationFailedException => + val entity = Json.toJson(vfe.failure).toString + Response.status(StatusCodes.UnprocessableEntity.intValue).entity(entity).build() + } } object RestResource { diff --git a/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala b/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala index b481a69b9e4..53912412154 100644 --- a/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala +++ b/src/main/scala/mesosphere/marathon/api/v2/AppsResource.scala @@ -22,7 +22,7 @@ import mesosphere.marathon.plugin.auth._ import mesosphere.marathon.state.PathId._ import mesosphere.marathon.state._ import mesosphere.marathon.stream._ -import play.api.libs.json.Json +import play.api.libs.json.{ JsObject, Json } @Path("v2/apps") @Consumes(Array(MediaType.APPLICATION_JSON)) @@ -61,6 +61,18 @@ class AppsResource @Inject() ( Response.ok(jsonObjString("apps" -> mapped)).build() } + def normalizedApp(appDef: AppDefinition, now: Timestamp): AppDefinition = { + appDef.copy( + ipAddress = appDef.ipAddress.map { ipAddress => + config.defaultNetworkName.get.collect { + case (defaultName: String) if defaultName.nonEmpty && ipAddress.networkName.isEmpty => + ipAddress.copy(networkName = Some(defaultName)) + }.getOrElse(ipAddress) + }, + versionInfo = VersionInfo.OnlyVersion(now) + ) + } + @POST @Timed def create( @@ -68,17 +80,7 @@ class AppsResource @Inject() ( @DefaultValue("false")@QueryParam("force") force: Boolean, @Context req: HttpServletRequest): Response = authenticated(req) { implicit identity => withValid(Json.parse(body).as[AppDefinition].withCanonizedIds()) { appDef => - val now = clock.now() - val app = appDef.copy( - ipAddress = appDef.ipAddress.map { ipAddress => - config.defaultNetworkName.get.collect { - case (defaultName: String) if defaultName.nonEmpty && ipAddress.networkName.isEmpty => - ipAddress.copy(networkName = Some(defaultName)) - }.getOrElse(ipAddress) - }, - versionInfo = VersionInfo.OnlyVersion(now) - ) - + val app = normalizedApp(appDef, clock.now()) checkAuthorization(CreateRunSpec, app) def createOrThrow(opt: Option[AppDefinition]) = opt @@ -141,6 +143,50 @@ class AppsResource @Inject() ( } } + /** + * Validate and normalize a single application update submitted via the REST API. Validation exceptions are not + * handled here, that's left as an exercise for the caller. + * + * @param appId used as the id of the generated app update (vs. whatever might be in the JSON body) + * @param body is the raw, unparsed JSON + * @param partialUpdate true if the JSON should be parsed as a partial application update (all fields optional) + * or as a wholesale replacement (parsed like an app definition would be) + */ + def canonicalAppUpdateFromJson(appId: PathId, body: Array[Byte], partialUpdate: Boolean): AppUpdate = { + if (partialUpdate) { + validateOrThrow(Json.parse(body).as[AppUpdate].copy(id = Some(appId)).withCanonizedIds()) + } else { + // this is a complete replacement of the app as we know it, so parse and normalize as if we're dealing + // with a brand new app because the rules are different (for example, many fields are non-optional with brand-new apps). + // however since this is an update, the user isn't required to specify an ID as part of the definition so we do + // some hackery here to pass initial JSON parsing. + val jsObj = Json.parse(body).as[JsObject] + ("id" -> Json.toJson(appId.toString)) + // the version is thrown away in toUpdate so just pass `zero` for now + normalizedApp(validateOrThrow(jsObj.as[AppDefinition].withCanonizedIds()), Timestamp.zero).toUpdate + } + } + + /** + * Validate and normalize an array of application updates submitted via the REST API. Validation exceptions are not + * handled here, that's left as an exercise for the caller. + * + * @param body is the raw, unparsed JSON + * @param partialUpdate true if the JSON should be parsed as a partial application update (all fields optional) + * or as a wholesale replacement (parsed like an app definition would be) + */ + def canonicalAppUpdatesFromJson(body: Array[Byte], partialUpdate: Boolean): Seq[AppUpdate] = { + if (partialUpdate) { + validateOrThrow(Json.parse(body).as[Seq[AppUpdate]].map(_.withCanonizedIds())) + } else { + // this is a complete replacement of the app as we know it, so parse and normalize as if we're dealing + // with a brand new app because the rules are different (for example, many fields are non-optional with brand-new apps). + // the version is thrown away in toUpdate so just pass `zero` for now. + validateOrThrow(Json.parse(body).as[Seq[AppDefinition]].map(_.withCanonizedIds())).map { app => + normalizedApp(app, Timestamp.zero).toUpdate + } + } + } + @PUT @Path("""{id:.+}""") @Timed @@ -148,13 +194,15 @@ class AppsResource @Inject() ( @PathParam("id") id: String, body: Array[Byte], @DefaultValue("false")@QueryParam("force") force: Boolean, + @DefaultValue("true")@QueryParam("partialUpdate") partialUpdate: Boolean, @Context req: HttpServletRequest): Response = authenticated(req) { implicit identity => - val appId = id.toRootPath - val now = clock.now() - withValid(Json.parse(body).as[AppUpdate].copy(id = Some(appId))) { appUpdate => - val plan = result(groupManager.updateApp(appId, updateOrCreate(appId, _, appUpdate), now, force)) + val appId = id.toRootPath + assumeValid { + val appUpdate = canonicalAppUpdateFromJson(appId, body, partialUpdate) + val version = clock.now() + val plan = result(groupManager.updateApp(appId, updateOrCreate(appId, _, appUpdate, partialUpdate), version, force)) val response = plan.original.app(appId) .map(_ => Response.ok()) .getOrElse(Response.created(new URI(appId.toString))) @@ -169,14 +217,17 @@ class AppsResource @Inject() ( @Timed def replaceMultiple( @DefaultValue("false")@QueryParam("force") force: Boolean, + @DefaultValue("true")@QueryParam("partialUpdate") partialUpdate: Boolean, body: Array[Byte], @Context req: HttpServletRequest): Response = authenticated(req) { implicit identity => - withValid(Json.parse(body).as[Seq[AppUpdate]].map(_.withCanonizedIds())) { updates => + + assumeValid { val version = clock.now() + val updates = canonicalAppUpdatesFromJson(body, partialUpdate) def updateGroup(rootGroup: RootGroup): RootGroup = updates.foldLeft(rootGroup) { (group, update) => update.id match { - case Some(id) => group.updateApp(id, updateOrCreate(id, _, update), version) + case Some(id) => group.updateApp(id, updateOrCreate(id, _, update, partialUpdate), version) case None => group } } @@ -232,18 +283,20 @@ class AppsResource @Inject() ( deploymentResult(restartDeployment) } - private def updateOrCreate( + private[v2] def updateOrCreate( appId: PathId, existing: Option[AppDefinition], - appUpdate: AppUpdate)(implicit identity: Identity): AppDefinition = { + appUpdate: AppUpdate, + partialUpdate: Boolean)(implicit identity: Identity): AppDefinition = { def createApp(): AppDefinition = { val app = validateOrThrow(appUpdate.empty(appId)) checkAuthorization(CreateRunSpec, app) } def updateApp(current: AppDefinition): AppDefinition = { - val app = validateOrThrow(appUpdate(current)) - checkAuthorization(UpdateRunSpec, app) + val updatedApp = if (partialUpdate) appUpdate(current) else appUpdate.empty(appId) + val validatedApp = validateOrThrow(updatedApp) + checkAuthorization(UpdateRunSpec, validatedApp) } def rollback(current: AppDefinition, version: Timestamp): AppDefinition = { diff --git a/src/main/scala/mesosphere/marathon/state/AppDefinition.scala b/src/main/scala/mesosphere/marathon/state/AppDefinition.scala index 2cc2d326d12..02b46a1d118 100644 --- a/src/main/scala/mesosphere/marathon/state/AppDefinition.scala +++ b/src/main/scala/mesosphere/marathon/state/AppDefinition.scala @@ -9,6 +9,7 @@ import com.wix.accord.dsl._ import mesosphere.marathon.Protos.Constraint import mesosphere.marathon.api.serialization._ import mesosphere.marathon.api.v2.Validation._ +import mesosphere.marathon.api.v2.json.AppUpdate import mesosphere.marathon.core.externalvolume.ExternalVolumes import mesosphere.marathon.core.health._ import mesosphere.marathon.core.plugin.PluginManager @@ -356,6 +357,47 @@ case class AppDefinition( else if (ipAddress.isDefined) fromDiscoveryInfo else fromPortDefinitions } + + /** + * @return an app update generated from this app definition, but without the `version` field (since that can only + * be combined with `id` for app updates) + */ + def toUpdate: AppUpdate = + AppUpdate( + id = Some(id), + cmd = cmd, + args = Some(args), + user = user, + env = Some(env), + instances = Some(instances), + cpus = Some(resources.cpus), + mem = Some(resources.mem), + disk = Some(resources.disk), + gpus = Some(resources.gpus), + executor = Some(executor), + constraints = Some(constraints), + fetch = Some(fetch), + storeUrls = Some(storeUrls), + portDefinitions = Some(portDefinitions), + requirePorts = Some(requirePorts), + backoff = Some(backoffStrategy.backoff), + backoffFactor = Some(backoffStrategy.factor), + maxLaunchDelay = Some(backoffStrategy.maxLaunchDelay), + container = container, + healthChecks = Some(healthChecks), + readinessChecks = Some(readinessChecks), + taskKillGracePeriod = taskKillGracePeriod, + dependencies = Some(dependencies), + upgradeStrategy = Some(upgradeStrategy), + labels = Some(labels), + acceptedResourceRoles = Some(acceptedResourceRoles), + ipAddress = ipAddress, + residency = residency, + secrets = Some(secrets), + unreachableStrategy = Some(unreachableStrategy), + killSelection = Some(killSelection) + // purposefully ignore version since it can only be combined with the `id` field + ) } @SuppressWarnings(Array("IsInstanceOf")) // doesn't work well in the validation macros?! diff --git a/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala b/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala index b04c085eced..7fd66f7855a 100644 --- a/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala +++ b/src/test/scala/mesosphere/marathon/api/v2/AppsResourceTest.scala @@ -5,7 +5,6 @@ import java.util import javax.ws.rs.core.Response import akka.event.EventStream -import mesosphere.marathon._ import mesosphere.marathon.api._ import mesosphere.marathon.core.appinfo.AppInfo.Embed import mesosphere.marathon.core.appinfo._ @@ -110,7 +109,7 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match val updatedApp = app.copy(ipAddress = Some(IpAddress(networkName = Some("bar")))) val updatedJson = Json.toJson(updatedApp).as[JsObject] - "uris" - "version" val updatedBody = Json.stringify(updatedJson).getBytes("UTF-8") - val response = appsResource.replace(updatedApp.id.toString, updatedBody, false, auth.request) + val response = appsResource.replace(updatedApp.id.toString, updatedBody, force = false, partialUpdate = true, auth.request) Then("It is successful") response.getStatus should be(200) @@ -188,7 +187,7 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match val updatedApp = app.copy(ipAddress = Some(IpAddress(networkName = Some("bar")))) val updatedJson = Json.toJson(updatedApp).as[JsObject] - "uris" - "version" val updatedBody = Json.stringify(updatedJson).getBytes("UTF-8") - val response = appsResource.replace(updatedApp.id.toString, updatedBody, false, auth.request) + val response = appsResource.replace(updatedApp.id.toString, updatedBody, force = false, partialUpdate = true, auth.request) Then("It is successful") response.getStatus should be(200) @@ -208,7 +207,7 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match val updatedApp = app.copy(ipAddress = Some(IpAddress())) val updatedJson = Json.toJson(updatedApp).as[JsObject] - "uris" - "version" val updatedBody = Json.stringify(updatedJson).getBytes("UTF-8") - val response = appsResource.replace(updatedApp.id.toString, updatedBody, false, auth.request) + val response = appsResource.replace(updatedApp.id.toString, updatedBody, force = false, partialUpdate = true, auth.request) Then("It is successful") response.getStatus should be(200) @@ -635,7 +634,7 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match groupManager.app(PathId("/app")) returns Future.successful(Some(app)) When("The application is updated") - val response = appsResource.replace(app.id.toString, body, false, auth.request) + val response = appsResource.replace(app.id.toString, body, force = false, partialUpdate = true, auth.request) Then("The application is updated") response.getStatus should be(200) @@ -653,7 +652,7 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match val body = Json.stringify(appJsonWithOnlyPorts).getBytes("UTF-8") When("The application is updated") - val response = appsResource.replace(app.id.toString, body, false, auth.request) + val response = appsResource.replace(app.id.toString, body, force = false, partialUpdate = true, auth.request) Then("The application is updated") response.getStatus should be(200) @@ -674,7 +673,7 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match |}""".stripMargin.getBytes("UTF-8") Then("A serialization exception is thrown") - intercept[SerializationFailedException] { appsResource.replace(app.id.toString, body, force = false, auth.request) } + intercept[SerializationFailedException] { appsResource.replace(app.id.toString, body, force = false, partialUpdate = true, auth.request) } } def createAppWithVolumes(`type`: String, volumes: String): Response = { @@ -1023,13 +1022,48 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match |}""".stripMargin.getBytes("UTF-8") When("The application is updated") - val response = appsResource.replace(app.id.toString, body, force = false, auth.request) + val response = appsResource.replace(app.id.toString, body, force = false, partialUpdate = true, auth.request) Then("The return code indicates success") response.getStatus should be(200) response.getMetadata.containsKey(RestResource.DeploymentHeader) should be(true) } + test("Replacing an existing docker application, upgrading from host to user networking") { + Given("a docker app using host networking and non-empty port definitions") + val app = AppDefinition( + id = PathId("/app"), container = Some(Container.Docker(image = "foo")), portDefinitions = PortDefinitions(0)) + + When("upgraded to user networking using full-replacement semantics (no port definitions)") + val body = + """{ + | "cmd": "sleep 1", + | "container": { + | "type": "DOCKER", + | "docker": { + | "image": "/test:latest", + | "network": "USER" + | } + | }, + | "ipAddress": { "name": "dcos" } + |}""".stripMargin.getBytes("UTF-8") + val appUpdate = appsResource.canonicalAppUpdateFromJson(app.id, body, partialUpdate = false) + + Then("the application is updated") + appsResource.updateOrCreate( + app.id, Some(app), appUpdate, partialUpdate = false)(auth.identity) + + And("fails when the update operation uses partial-update semantics") + val caught = intercept[IllegalArgumentException] { + val partUpdate = appsResource.canonicalAppUpdateFromJson(app.id, body, partialUpdate = true) + appsResource.updateOrCreate( + app.id, Some(app), partUpdate, partialUpdate = true)(auth.identity) + } + assert(caught.getMessage.indexOf( + s"IP address (${Option(IpAddress())}) and ports (${PortDefinitions(0)}) are not allowed at the same time" + ) > -1) + } + test("Restart an existing app") { val app = AppDefinition(id = PathId("/app")) val rootGroup = createRootGroup(Map(app.id -> app)) @@ -1142,12 +1176,12 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match show.getStatus should be(auth.NotAuthenticatedStatus) When("we try to update an app") - val replace = appsResource.replace("", app.getBytes("UTF-8"), false, req) + val replace = appsResource.replace("", app.getBytes("UTF-8"), force = false, partialUpdate = true, req) Then("we receive a NotAuthenticated response") replace.getStatus should be(auth.NotAuthenticatedStatus) When("we try to update multiple apps") - val replaceMultiple = appsResource.replaceMultiple(false, s"[$app]".getBytes("UTF-8"), req) + val replaceMultiple = appsResource.replaceMultiple(force = false, partialUpdate = true, s"[$app]".getBytes("UTF-8"), req) Then("we receive a NotAuthenticated response") replaceMultiple.getStatus should be(auth.NotAuthenticatedStatus) @@ -1187,12 +1221,12 @@ class AppsResourceTest extends MarathonSpec with MarathonActorSupport with Match show.getStatus should be(auth.UnauthorizedStatus) When("we try to update an app") - val replace = appsResource.replace("/a", app.getBytes("UTF-8"), false, req) + val replace = appsResource.replace("/a", app.getBytes("UTF-8"), force = false, partialUpdate = true, req) Then("we receive a NotAuthorized response") replace.getStatus should be(auth.UnauthorizedStatus) When("we try to update multiple apps") - val replaceMultiple = appsResource.replaceMultiple(false, s"[$app]".getBytes("UTF-8"), req) + val replaceMultiple = appsResource.replaceMultiple(force = false, partialUpdate = true, s"[$app]".getBytes("UTF-8"), req) Then("we receive a NotAuthorized response") replaceMultiple.getStatus should be(auth.UnauthorizedStatus)