Implement naive manifest diffing

- add new `/v3/spaces/:guid/manifest-diff` endpoint
- does not handle array manifest fields with push style logic
  - buildpacks are coincidentally right
- uses JSONpatch convention
- only reports diff for apps in the provided manifest, ignoring any
other apps in the space
- endpoint only accessible to space writer roles

[#173688915]
[finishes #173478913]

Co-authored-by: James Palmer <jpalmer@pivotal.io>
Co-authored-by: Teal Stannard <tstannard@pivotal.io>
Co-authored-by: Reid Mitchell <rmitchell@pivotal.io>
Co-authored-by: Sarah Weinstein <sweinstein@pivotal.io>

Author: 4 people

Gemfile

@@ -11,6 +11,7 @@ gem 'fluent-logger'
 gem 'googleapis-common-protos'
 gem 'hashdiff'
 gem 'httpclient'
+gem 'json-diff'
 gem 'json-schema'
 gem 'json_pure'
 gem 'kubeclient'

Gemfile.lock

@@ -239,6 +239,7 @@ GEM
     ipaddress (0.8.3)
     jaro_winkler (1.5.4)
     json (2.3.0)
+    json-diff (0.4.1)
     json-schema (2.8.0)
       addressable (>= 2.4)
     json_pure (2.1.0)
@@ -512,6 +513,7 @@ DEPENDENCIES
   googleapis-common-protos
   hashdiff
   httpclient
+  json-diff
   json-schema
   json_pure
   kubeclient

app/controllers/v3/space_manifests_controller.rb

@@ -3,6 +3,7 @@
 require 'messages/named_app_manifest_message'
 require 'actions/app_find_or_create_skeleton'
 require 'actions/app_create'
+require 'json-diff'
 
 class SpaceManifestsController < ApplicationController
   wrap_parameters :body, format: [:yaml]
@@ -41,6 +42,43 @@ def apply_manifest
     head HTTP::ACCEPTED, 'Location' => url_builder.build_url(path: "/v3/jobs/#{job.guid}")
   end
 
+  def diff_manifest
+    space = Space.find(guid: hashed_params[:guid])
+    space_not_found! unless space && permission_queryer.can_read_from_space?(space.guid, space.organization.guid)
+    unauthorized! unless permission_queryer.can_write_to_space?(space.guid)
+
+    json_diff = []
+
+    parsed_app_manifests.each_with_index do |manifest_app_hash, index|
+      existing_app = space.app_models.find { |app| app.name == manifest_app_hash['name'] } || {}
+
+      if existing_app == {}
+        existing_app_hash = {}
+      else
+        manifest_presenter = Presenters::V3::AppManifestPresenter.new(
+          existing_app,
+          existing_app.service_bindings,
+          existing_app.routes,
+        )
+        existing_app_hash = manifest_presenter.to_hash.deep_stringify_keys['applications'][0]
+      end
+
+      curr_diff = JsonDiff.diff(
+        existing_app_hash,
+        manifest_app_hash.to_hash,
+        include_was: true,
+      )
+
+      curr_diff.each do |diff|
+        diff['path'] = "/applications/#{index}" + diff['path']
+      end
+
+      json_diff += curr_diff
+    end
+
+    render status: :created, json: { diff: json_diff }
+  end
+
   private
 
   def errors_for_message(message, index)

config/routes.rb

@@ -228,6 +228,7 @@
 
   # space_manifests
   post '/spaces/:guid/actions/apply_manifest', to: 'space_manifests#apply_manifest'
+  post '/spaces/:guid/manifest_diff', to: 'space_manifests#diff_manifest'
 
   # space_quotas
   post '/space_quotas', to: 'space_quotas#create'

docs/v3/source/includes/resources/manifests/_create_diff.md

@@ -0,0 +1,53 @@
+### Create a manifest diff for apps
+
+```
+Example Request
+```
+
+```shell
+curl "https://api.example.org/v3/spaces/[guid]/manifest_diff" \
+  -X POST \
+  -H "Authorization: bearer [token]" \
+  -d @/path/to/manifest.yml
+```
+
+```
+Example Response
+```
+
+```http
+HTTP/1.1 202 OK
+Content-Type: application/json
+
+{
+  "diff": [
+    {
+      "op": "remove",
+      "path": "/applications/0/routes/1",
+      "was": {"route": "route.example.com"}
+    },
+    {
+      "op": "add",
+      "path": "/applications/1/buildpacks/2",
+      "value": "java_buildpack"
+    },
+    {
+      "op": "replace",
+      "path": "/applications/2/processes/1/memory",
+      "was": "256M",
+      "value": "512M"
+    }
+  ]
+}
+```
+
+Create a manifest diff for apps in the provided manifest and their underlying processes.
+
+#### Definition
+`GET /v3/spaces/:guid/manifest_diff`
+
+#### Permitted Roles
+ |
+--- | ---
+Admin |
+Space Developer |

docs/v3/source/index.md

@@ -168,6 +168,7 @@ includes:
   - resources/manifests/object
   - resources/manifests/apply
   - resources/manifests/get
+  - resources/manifests/create_diff
   - resources/organizations/header
   - resources/organizations/object
   - resources/organizations/create

spec/request/space_manifests_spec.rb

@@ -1,4 +1,5 @@
 require 'spec_helper'
+require 'request_spec_shared_examples'
 
 RSpec.describe 'Space Manifests' do
   let(:user) { VCAP::CloudController::User.make }
@@ -10,12 +11,6 @@
     VCAP::CloudController::Route.make(domain: shared_domain, space: space, path: '/path', host: 'b_host')
   }
 
-  before do
-    space.organization.add_user(user)
-    space.add_developer(user)
-    TestConfig.override(kubernetes: {})
-  end
-
   describe 'POST /v3/spaces/:guid/actions/apply_manifest' do
     let(:buildpack) { VCAP::CloudController::Buildpack.make }
     let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: space) }
@@ -100,9 +95,13 @@
     end
 
     before do
+      space.organization.add_user(user)
+      space.add_developer(user)
+      TestConfig.override(kubernetes: {})
+
       stub_bind(service_instance)
       VCAP::CloudController::LabelsUpdate.update(app1_model, { 'potato' => 'french',
-        'downton' => 'abbey road', }, VCAP::CloudController::AppLabelModel)
+                                                               'downton' => 'abbey road', }, VCAP::CloudController::AppLabelModel)
       VCAP::CloudController::AnnotationsUpdate.update(app1_model, { 'potato' => 'baked',
         'berry' => 'white', }, VCAP::CloudController::AppAnnotationModel)
     end
@@ -461,4 +460,159 @@
       end
     end
   end
+
+  describe 'POST /v3/spaces/:guid/manifest_diff' do
+    let(:org) { space.organization }
+    let(:app1_model) { VCAP::CloudController::AppModel.make(name: 'app-1', space: space) }
+    let!(:process1) { VCAP::CloudController::ProcessModel.make(app: app1_model) }
+
+    let(:api_call) { lambda { |user_headers| post "/v3/spaces/#{space.guid}/manifest_diff", yml_manifest, yml_headers(user_headers) } }
+
+    let(:expected_codes_and_responses) do
+      h = Hash.new(code: 403)
+      h['org_auditor'] = { code: 404 }
+      h['org_billing_manager'] = { code: 404 }
+      h['no_role'] = { code: 404 }
+
+      h['admin'] = {
+        code: 201,
+        response_object: diff_json
+      }
+
+      h['space_developer'] = {
+        code: 201,
+        response_object: diff_json
+      }
+
+      h.freeze
+    end
+
+    context 'when there are no changes in the manifest' do
+      let(:diff_json) do
+        {
+          diff: []
+        }
+      end
+
+      let(:yml_manifest) do
+        {
+          'applications' => [
+            {
+              'name' => app1_model.name,
+              'stack' => process1.stack.name,
+              'processes' => [
+                {
+                  'type' => process1.type,
+                  'instances' => process1.instances,
+                  'memory' => '1024M',
+                  'disk_quota' => '1024M',
+                  'health-check-type' =>  process1.health_check_type
+                }
+              ]
+            },
+          ]
+        }.to_yaml
+      end
+      it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
+    end
+
+    context 'when there are changes in the manifest' do
+      let(:diff_json) do
+        {
+          diff: [
+            { op: 'add', path: '/applications/0/comp', value: 'hoh' },
+            { op: 'replace', path: '/applications/0/stack', was: process1.stack.name, value: 'big brother' },
+            { op: 'remove', path: '/applications/0/processes/0/memory', was: '1024M' },
+          ]
+        }
+      end
+
+      let(:yml_manifest) do
+        {
+          'applications' => [
+            {
+              'name' => app1_model.name,
+              'stack' => 'big brother',
+              'comp' => 'hoh',
+              'processes' => [
+                {
+                  'type' => process1.type,
+                  'instances' => process1.instances,
+                  'disk_quota' => '1024M',
+                  'health-check-type' =>  process1.health_check_type
+                }
+              ]
+            },
+          ]
+        }.to_yaml
+      end
+      it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
+    end
+    context 'when there is a new app' do
+      let(:diff_json) do
+        {
+          diff: [
+            { op: 'add', path: '/applications/0/name', value: 'new-app' },
+            { op: 'add', path: '/applications/1/name', value: 'newer-app' },
+          ]
+        }
+      end
+
+      let(:yml_manifest) do
+        {
+          'applications' => [
+            {
+              'name' => 'new-app',
+            },
+            {
+              'name' => 'newer-app',
+            }
+          ]
+        }.to_yaml
+      end
+      it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
+    end
+
+    context 'the manifest is unparseable' do
+      let(:yml_manifest) do
+        {
+          89 => [
+            {
+              'name' => 'new-app',
+            },
+          ]
+        }.to_yaml
+      end
+
+      before do
+        space.organization.add_user(user)
+        space.add_developer(user)
+      end
+
+      it 'returns an appropriate error' do
+        post "/v3/spaces/#{space.guid}/manifest_diff", yml_manifest, yml_headers(user_header)
+        parsed_response = MultiJson.load(last_response.body)
+
+        expect(last_response).to have_status_code(400)
+        expect(parsed_response['errors'].first['detail']).to eq('The request is invalid')
+      end
+    end
+    context 'the space does not exist' do
+      let(:yml_manifest) do
+        {
+          'applications' => [
+            {
+              'name' => 'new-app',
+            },
+          ]
+        }.to_yaml
+      end
+
+      it 'returns an appropriate error' do
+        post '/v3/spaces/not-space-guid/manifest_diff', yml_manifest, yml_headers(user_header)
+
+        expect(last_response).to have_status_code(404)
+      end
+    end
+  end
 end