From fed65473643307183f3342e98d556cde8ce3a1d5 Mon Sep 17 00:00:00 2001 From: Tim Downey Date: Tue, 1 Dec 2020 10:11:37 -0800 Subject: [PATCH] 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 https://github.com/cloudfoundry/cloud_controller_ng/issues/1928 [#175420211](https://www.pivotaltracker.com/story/show/175420211) Co-authored-by: Tim Downey Co-authored-by: Piyali Banerjee --- app/actions/route_create.rb | 7 + app/models/runtime/domain.rb | 4 + app/models/runtime/route.rb | 2 + app/models/runtime/shared_domain.rb | 4 - spec/unit/actions/route_create_spec.rb | 46 +++++++ .../cloud_controller/copilot/adapter_spec.rb | 2 +- spec/unit/models/runtime/route_spec.rb | 122 ++++++++++++------ 7 files changed, 139 insertions(+), 48 deletions(-) diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index 594e147b903..d00adbe56a9 100644 --- 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 index 3b355acd916..13221e58213 100644 --- 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 index 6e25d52ff68..1aa30998414 100644 --- 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 index 6c2896edce5..7eb310b0a5a 100644 --- 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 index 3ebc58fd0b8..6c0f63f5180 100644 --- 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 index af58b3059b8..0ceb52a8804 100644 --- 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 index ff87e16a25d..8641dc707fd 100644 --- 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) }