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

According to the [docs](http://v3-apidocs.cloudfoundry.org/version/3.86.0/index.html#the-manifest-schema),
the default-route flag should be ignored if routes are also provided in
the manifest. However if the app name was an invalid route host, the
request would error out early in the message class.

We pushed validation back into the action right before a default route
would actually be created.

[finishes #173550835]

Author: monamohebbi

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 * 100) }
+              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