v3 🐞: default route in manifest no longer errors when meant to be ignored

Originally reverted (07b60b1) because
unit tests were failing when running with mysql. Error message:

``` Error: expected VCAP::CloudController::AppApplyManifest::Error with
"Failed to create default route from app name: Host cannot exceed 63
characters", got #<Sequel::DatabaseError: Mysql2::Error: Data too long
for column 'name' at row 1> ```

The original commit (805220c) used an
app name which was 10,000 characters long, well exceeding the 63
character limit. Reducing the name to 100 characters gave us the
expected error message rather than the Sequel error.

[finishes #173550835]

Co-authored-by: Sarah Weinstein <sweinstein@pivotal.io>
Co-authored-by: Merric De Launey <mdelauney@pivotal.io>

Author: sweinstein22

app/actions/app_apply_manifest.rb

@@ -9,6 +9,7 @@
 
 module VCAP::CloudController
   class AppApplyManifest
+    class Error < StandardError; end
     class NoDefaultDomain < StandardError; end
     class ServiceBindingError < StandardError; end
 
@@ -105,6 +106,7 @@ def update_routes(app, message)
       end
 
       if update_message.default_route && existing_routes.empty?
+        validate_name_dns_compliant!(app.name)
         domain_name = get_default_domain_name(app)
 
         route = "#{app.name}.#{domain_name}"
@@ -121,6 +123,18 @@ def get_default_domain_name(app)
       domain_name
     end
 
+    def validate_name_dns_compliant!(name)
+      prefix = 'Failed to create default route from app name:'
+
+      if name.present? && name.length > 63
+        error!(prefix + ' Host cannot exceed 63 characters')
+      end
+
+      unless name&.match(/\A[\w\-]+\z/)
+        error!(prefix + ' Host must be either "*" or contain only alphanumeric characters, "_", or "-"')
+      end
+    end
+
     def create_service_bindings(manifest_service_bindings_message, app)
       action = ServiceBindingCreate.new(@user_audit_info, manifest_triggered: true)
       manifest_service_bindings_message.manifest_service_bindings.each do |manifest_service_binding|
@@ -162,5 +176,9 @@ def volume_services_enabled?
     def logger
       @logger ||= Steno.logger('cc.action.app_apply_manifest')
     end
+
+    def error!(message)
+      raise Error.new(message)
+    end
   end
 end

app/jobs/app_apply_manifest_action_job.rb

@@ -23,6 +23,7 @@ def perform
              SidecarUpdate::InvalidSidecar,
              ManifestRouteUpdate::InvalidRoute,
              Route::InvalidOrganizationRelation,
+             AppApplyManifest::Error,
              AppApplyManifest::ServiceBindingError => e
 
         error = CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', e.message)

app/jobs/space_apply_manifest_action_job.rb

@@ -22,6 +22,7 @@ def perform
                ProcessUpdate::InvalidProcess,
                ManifestRouteUpdate::InvalidRoute,
                Route::InvalidOrganizationRelation,
+               AppApplyManifest::Error,
                AppApplyManifest::ServiceBindingError => e
 
           app_name = AppModel.find(guid: app_guid)&.name

app/messages/named_app_manifest_message.rb

@@ -6,18 +6,5 @@ class NamedAppManifestMessage < AppManifestMessage
     register_allowed_keys [:name]
 
     validates :name, presence: { message: 'must not be empty' }, string: true
-    validate :validate_name_dns_compliant!, if: -> { default_route }
-
-    def validate_name_dns_compliant!
-      prefix = 'Failed to create default route from app name:'
-
-      if name.present? && name.length > 63
-        errors.add(prefix, 'Host cannot exceed 63 characters')
-      end
-
-      unless name&.match(/\A[\w\-]+\z/)
-        errors.add(prefix, 'Host must be either "*" or contain only alphanumeric characters, "_", or "-"')
-      end
-    end
   end
 end

spec/request/space_manifests_spec.rb

@@ -309,7 +309,7 @@
     end
 
     context 'when the app name is not a valid host name and the default-route flag is set to true' do
-      let(:app1_model) { VCAP::CloudController::AppModel.make(name: '!' * 64, space: space) }
+      let(:app1_model) { VCAP::CloudController::AppModel.make(name: 'a' * 64, space: space) }
       let(:yml_manifest) do
         {
           'applications' => [
@@ -319,16 +319,51 @@
         }.to_yaml
       end
 
-      it 'returns a 422 with an informative message' do
+      it 'returns a 202 but fails on the job' do
         expect {
           post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
         }.to change { VCAP::CloudController::AppModel.count }.by(0)
 
-        expect(last_response).to have_status_code(422)
-        expect(last_response).to include_error_message(
-          /Failed to create default route from app name: Host cannot exceed 63 characters/)
-        expect(last_response).to include_error_message(
-          /Failed to create default route from app name: Host must be either "\*" or contain only alphanumeric characters, "_", or "-"/)
+        expect(last_response).to have_status_code(202)
+        expect(last_response.status).to eq(202), last_response.body
+
+        job_guid = VCAP::CloudController::PollableJobModel.last.guid
+        expect(last_response.headers['Location']).to match(%r(/v3/jobs/#{job_guid}))
+
+        Delayed::Worker.new.work_off
+        job = VCAP::CloudController::PollableJobModel.find(guid: job_guid)
+        errors = YAML.safe_load(job.cf_api_error)['errors']
+        expect(errors.length).to eq 1
+        expect(errors[0]['detail']).to include('Host cannot exceed 63 characters')
+      end
+
+      context 'and routes are provided in the manifest' do
+        let(:yml_manifest) do
+          {
+            'applications' => [
+              { 'name' => app1_model.name,
+                'default-route' => true,
+                'routes' => [{ 'route' => "http://#{route.host}.#{shared_domain.name}" }] },
+            ]
+          }.to_yaml
+        end
+
+        it 'returns a 202 and succeeds' do
+          expect {
+            post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
+          }.to change { VCAP::CloudController::AppModel.count }.by(0)
+
+          expect(last_response).to have_status_code(202)
+          expect(last_response.status).to eq(202), last_response.body
+
+          job_guid = VCAP::CloudController::PollableJobModel.last.guid
+          expect(last_response.headers['Location']).to match(%r(/v3/jobs/#{job_guid}))
+
+          Delayed::Worker.new.work_off
+          job = VCAP::CloudController::PollableJobModel.find(guid: job_guid)
+          expect(job.complete?).to be true
+          expect(job.cf_api_error).to be nil
+        end
       end
     end
 

spec/unit/actions/app_apply_manifest_spec.rb

@@ -553,6 +553,28 @@ module VCAP::CloudController
               end
             end
 
+            context 'when the app name has special characters' do
+              let(:message) { AppManifestMessage.create_from_yml({ name: 'blah!@#', default_route: true }) }
+              let(:app_model) { AppModel.make(name: 'blah!@#') }
+              it 'fails with a useful error message' do
+                expect {
+                  app_apply_manifest.apply(app_model.guid, message)
+                }.to raise_error(AppApplyManifest::Error,
+                  /Failed to create default route from app name: Host must be either "\*" or contain only alphanumeric characters, "_", or "-"/)
+              end
+            end
+
+            context 'when the app name is too long' do
+              let(:app_name) { 'a' * 100 }
+              let(:message) { AppManifestMessage.create_from_yml({ name: app_name, default_route: true }) }
+              let(:app_model) { AppModel.make(name: app_name) }
+              it 'fails with a useful error' do
+                expect {
+                  app_apply_manifest.apply(app_model.guid, message)
+                }.to raise_error(AppApplyManifest::Error, 'Failed to create default route from app name: Host cannot exceed 63 characters')
+              end
+            end
+
             context 'when there is no shared domain' do
               let(:domain) { PrivateDomain.make(owning_organization: app.organization) }
 

spec/unit/messages/named_app_manifest_message_spec.rb

@@ -24,76 +24,6 @@ module VCAP::CloudController
           expect(message.errors.full_messages[0]).to match(/^Name must not be empty/)
         end
       end
-
-      context 'when the name is too long' do
-        let(:params) { { name: 'x' * 64, 'default-route': default_route } }
-
-        context 'when default-route is true' do
-          let(:default_route) { true }
-
-          it 'is not valid' do
-            message = NamedAppManifestMessage.create_from_yml(params)
-
-            expect(message).to_not be_valid
-            expect(message.errors.full_messages[0]).to match(/Host cannot exceed 63 characters/)
-          end
-        end
-
-        context 'when default-route is false' do
-          let(:default_route) { false }
-
-          it 'is valid' do
-            message = NamedAppManifestMessage.create_from_yml(params)
-
-            expect(message).to be_valid
-          end
-        end
-
-        context 'when default-route is not set' do
-          let(:default_route) { nil }
-
-          it 'is valid' do
-            message = NamedAppManifestMessage.create_from_yml(params)
-
-            expect(message).to be_valid
-          end
-        end
-      end
-
-      context 'when the name contains special characters' do
-        let(:params) { { name: '%%%', 'default-route': default_route } }
-
-        context 'when default-route is true' do
-          let(:default_route) { true }
-
-          it 'is not valid' do
-            message = NamedAppManifestMessage.create_from_yml(params)
-
-            expect(message).to_not be_valid
-            expect(message.errors.full_messages[0]).to match(/Host must be either "\*" or contain only alphanumeric characters, "_", or "-"/)
-          end
-        end
-
-        context 'when default-route is false' do
-          let(:default_route) { false }
-
-          it 'is valid' do
-            message = NamedAppManifestMessage.create_from_yml(params)
-
-            expect(message).to be_valid
-          end
-        end
-
-        context 'when default-route is not set' do
-          let(:default_route) { nil }
-
-          it 'is valid' do
-            message = NamedAppManifestMessage.create_from_yml(params)
-
-            expect(message).to be_valid
-          end
-        end
-      end
     end
   end
 end