improve internal route error message on Kubernetes

- return a meaningful error for now when a user attempts to create an internal route when running on Kubernetes -- eventually this can be removed once internal routes are supported - no longer allocate VIPs when creating an internal route and running on Kubernetes -- these were only used for Istio on BOSH and have no purpose on Kubernetes - related issue cloudfoundry#1928 [#175420211](https://www.pivotaltracker.com/story/show/175420211) Co-authored-by: Tim Downey <[email protected]> Co-authored-by: Piyali Banerjee <[email protected]>
univ0298 · Dec 2, 2020 · fed6547 · fed6547
1 parent 8c07baf
commit fed6547
Show file tree

Hide file tree

Showing 7 changed files with 139 additions and 48 deletions.
diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb
@@ -9,6 +9,7 @@ def initialize(user_audit_info)
 
     def create(message:, space:, domain:, manifest_triggered: false)
       validate_tcp_route!(domain, message)
+      validate_internal_kubernetes_domain!(domain)
 
       route = Route.new(
         host: message.host || '',
@@ -51,6 +52,12 @@ def validate_tcp_route!(domain, message)
       end
     end
 
+    def validate_internal_kubernetes_domain!(domain)
+      if domain.internal? && VCAP::CloudController::Config.config.kubernetes_api_configured?
+        error!('Internal domains are currently not supported on Kubernetes')
+      end
+    end
+
     def port(message, domain)
       generated_port = if !message.requested?(:port) && domain.protocols.include?('tcp')
                          PortGenerator.generate_port(domain.guid, router_group(domain).reservable_ports)

diff --git a/app/models/runtime/domain.rb b/app/models/runtime/domain.rb
@@ -147,6 +147,10 @@ def protocols
       ['http']
     end
 
+    def internal?
+      !!internal
+    end
+
     private
 
     def k8s_enabled?

diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb
@@ -218,6 +218,8 @@ def find_next_vip_offset
     end
 
     def before_save
+      return if VCAP::CloudController::Config.kubernetes_api_configured?
+
       if internal? && vip_offset.nil?
         len = internal_route_vip_range_len
         raise OutOfVIPException.new('out of vip_offset slots') if self.class.exclude(vip_offset: nil).count >= len

diff --git a/app/models/runtime/shared_domain.rb b/app/models/runtime/shared_domain.rb
@@ -92,10 +92,6 @@ def transient_attrs
       router_group_type.blank? ? [] : [:router_group_type]
     end
 
-    def internal?
-      !!internal
-    end
-
     def routing_api_client
       @routing_api_client ||= CloudController::DependencyLocator.instance.routing_api_client
     end

diff --git a/spec/unit/actions/route_create_spec.rb b/spec/unit/actions/route_create_spec.rb
@@ -529,6 +529,52 @@ module VCAP::CloudController
             subject.create(message: message, space: space, domain: internal_domain)
           }.to raise_error(RouteCreate::Error, 'Paths are not supported for internal domains.')
         end
+
+        context 'when the Kubernetes API is configured' do
+          before do
+            TestConfig.override(
+              kubernetes: { host_url: 'https://api.default.svc.cluster-domain.example' }
+            )
+          end
+
+          it 'raises an error' do
+            message = RouteCreateMessage.new({
+              host: 'a',
+              relationships: {
+                space: {
+                  data: { guid: space.guid }
+                },
+                domain: {
+                  data: { guid: internal_domain.guid }
+                },
+              },
+            })
+
+            expect {
+              subject.create(message: message, space: space, domain: internal_domain)
+            }.to raise_error(RouteCreate::Error, 'Internal domains are currently not supported on Kubernetes')
+          end
+        end
+
+        context 'when the Kubernetes API is not configured' do
+          it 'does not raise an error' do
+            message = RouteCreateMessage.new({
+              host: 'a',
+              relationships: {
+                space: {
+                  data: { guid: space.guid }
+                },
+                domain: {
+                  data: { guid: internal_domain.guid }
+                },
+              },
+            })
+
+            expect {
+              subject.create(message: message, space: space, domain: internal_domain)
+            }.not_to raise_error
+          end
+        end
       end
 
       context 'when using a reserved system hostname' do

diff --git a/spec/unit/lib/cloud_controller/copilot/adapter_spec.rb b/spec/unit/lib/cloud_controller/copilot/adapter_spec.rb
@@ -15,7 +15,7 @@ module VCAP::CloudController
       allow(CloudController::DependencyLocator.instance).to receive(:copilot_client).and_return(copilot_client)
       allow(Steno).to receive(:logger).and_return(fake_logger)
       allow(fake_logger).to receive(:debug)
-      TestConfig.override(copilot: { enabled: true, temporary_istio_domains: [istio_domain.name, internal_istio_domain.name] })
+      TestConfig.override(copilot: { enabled: true, temporary_istio_domains: [istio_domain.name, internal_istio_domain.name] }, kubernetes: {})
     end
 
     describe '#create_route' do

diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb
@@ -1331,6 +1331,12 @@ def assert_invalid_path(path)
     end
 
     context '#internal?' do
+      before do
+        TestConfig.override(
+          kubernetes: {}
+        )
+      end
+
       let(:internal_domain) { SharedDomain.make(name: 'apps.internal', internal: true) }
       let(:internal_route) { Route.make(host: 'meow', domain: internal_domain) }
       let(:external_private_route) { Route.make }
@@ -1350,7 +1356,7 @@ def assert_invalid_path(path)
 
     describe 'vip_offset' do
       before do
-        TestConfig.override( # 8 theoretical available ips, 6 actual
+        TestConfig.override(
           internal_route_vip_range: '127.128.99.0/29',
           kubernetes: {}
         )
@@ -1363,62 +1369,86 @@ def assert_invalid_path(path)
         let!(:internal_route_3) { Route.make(host: 'quack', domain: internal_domain) }
         let(:external_private_route) { Route.make }
 
-        it 'auto-assigns vip_offset to internal routes only' do
-          expect(internal_route_1.vip_offset).not_to be_nil
-          expect(external_private_route.vip_offset).to be_nil
-        end
-
-        it 'assigns multiple vips in ascending order without duplicates' do
-          expect(internal_route_1.vip_offset).to eq(1)
-          expect(internal_route_2.vip_offset).to eq(2)
-        end
-
-        it 'never assigns the same vip_offset to multiple internal routes' do
-          expect {
-            Route.make(host: 'ants', vip_offset: 1)
-          }.to raise_error(Sequel::UniqueConstraintViolation, /duplicate.*routes_vip_offset_index/i)
-        end
+        context 'when the kubernetes API is configured' do
+          before do
+            TestConfig.override(
+              internal_route_vip_range: '',
+              kubernetes: { host_url: 'api.k8s.example.com' }
+            )
+          end
 
-        it 'finds an available offset' do
-          Route.make(host: 'gulp', domain: internal_domain)
-          expect(Route.all.map(&:vip_offset)).to match_array((1..4).to_a)
+          it 'does not assign vip_offset' do
+            internal_route_4 = Route.make(host: 'moo', domain: internal_domain)
+            expect(internal_route_4.vip_offset).to be_nil
+            expect(internal_route_4.vip).to be_nil
+          end
         end
 
-        context 'when the taken offset is not the first' do
+        context 'when the Kubernetes API is not configured' do
           before do
-            TestConfig.override(
+            TestConfig.override( # 8 theoretical available ips, 6 actual
+              internal_route_vip_range: '127.128.99.0/29',
               kubernetes: {}
             )
           end
-          it 'finds the first offset' do
-            internal_route_1.destroy
-            expect(Route.make(host: 'gulp', domain: internal_domain).vip_offset).to eq(1)
+
+          it 'auto-assigns vip_offset to internal routes only' do
+            expect(internal_route_1.vip_offset).not_to be_nil
+            expect(external_private_route.vip_offset).to be_nil
+          end
+
+          it 'assigns multiple vips in ascending order without duplicates' do
+            expect(internal_route_1.vip_offset).to eq(1)
+            expect(internal_route_2.vip_offset).to eq(2)
+          end
+
+          it 'never assigns the same vip_offset to multiple internal routes' do
+            expect {
+              Route.make(host: 'ants', vip_offset: 1)
+            }.to raise_error(Sequel::UniqueConstraintViolation, /duplicate.*routes_vip_offset_index/i)
           end
-        end
 
-        context 'when the taken offsets include first and not second' do
           it 'finds an available offset' do
-            internal_route_2.destroy
-            expect(Route.make(host: 'gulp', domain: internal_domain).vip_offset).to eq(2)
+            Route.make(host: 'gulp', domain: internal_domain)
+            expect(Route.all.map(&:vip_offset)).to match_array((1..4).to_a)
+          end
+
+          context 'when the taken offset is not the first' do
+            before do
+              TestConfig.override(
+                kubernetes: {}
+              )
+            end
+            it 'finds the first offset' do
+              internal_route_1.destroy
+              expect(Route.make(host: 'gulp', domain: internal_domain).vip_offset).to eq(1)
+            end
           end
-        end
 
-        context 'when filling the vip range' do
-          it 'can make 3 more new routes only' do
-            expect { Route.make(host: 'route4', domain: internal_domain) }.not_to raise_error
-            expect { Route.make(host: 'route5', domain: internal_domain) }.not_to raise_error
-            expect { Route.make(host: 'route6', domain: internal_domain) }.not_to raise_error
-            expect { Route.make(host: 'route7', domain: internal_domain) }.to raise_error(Route::OutOfVIPException)
+          context 'when the taken offsets include first and not second' do
+            it 'finds an available offset' do
+              internal_route_2.destroy
+              expect(Route.make(host: 'gulp', domain: internal_domain).vip_offset).to eq(2)
+            end
           end
 
-          it 'can reclaim lost vips' do
-            expect { Route.make(host: 'route4', domain: internal_domain) }.not_to raise_error
-            expect { Route.make(host: 'route5', domain: internal_domain) }.not_to raise_error
-            expect { Route.make(host: 'route6', domain: internal_domain) }.not_to raise_error
-            Route.last.destroy
-            internal_route_2.destroy
-            expect(Route.make(host: 'new2', domain: internal_domain).vip_offset).to eq(2)
-            expect(Route.make(host: 'new6', domain: internal_domain).vip_offset).to eq(6)
+          context 'when filling the vip range' do
+            it 'can make 3 more new routes only' do
+              expect { Route.make(host: 'route4', domain: internal_domain) }.not_to raise_error
+              expect { Route.make(host: 'route5', domain: internal_domain) }.not_to raise_error
+              expect { Route.make(host: 'route6', domain: internal_domain) }.not_to raise_error
+              expect { Route.make(host: 'route7', domain: internal_domain) }.to raise_error(Route::OutOfVIPException)
+            end
+
+            it 'can reclaim lost vips' do
+              expect { Route.make(host: 'route4', domain: internal_domain) }.not_to raise_error
+              expect { Route.make(host: 'route5', domain: internal_domain) }.not_to raise_error
+              expect { Route.make(host: 'route6', domain: internal_domain) }.not_to raise_error
+              Route.last.destroy
+              internal_route_2.destroy
+              expect(Route.make(host: 'new2', domain: internal_domain).vip_offset).to eq(2)
+              expect(Route.make(host: 'new6', domain: internal_domain).vip_offset).to eq(6)
+            end
           end
         end
       end
@@ -1476,6 +1506,12 @@ def assert_invalid_path(path)
     end
 
     describe 'vip' do
+      before do
+        TestConfig.override(
+          kubernetes: {}
+        )
+      end
+
       let(:internal_domain) { SharedDomain.make(name: 'apps.internal', internal: true) }
       let!(:internal_route_1) { Route.make(host: 'meow', domain: internal_domain, vip_offset: 1) }
       let!(:internal_route_2) { Route.make(host: 'woof', domain: internal_domain, vip_offset: 2) }