From ba0d68ad02e9c578eb625474fd75b26b4f697685 Mon Sep 17 00:00:00 2001 From: Jovan Kostovski Date: Tue, 31 Oct 2023 16:28:15 +0100 Subject: [PATCH] Deprecated Syslog Binding URL API removal Code clean up after the deprecation of the `/v4` API for the Syslog Binding URL. --- .../internal/syslog_drain_urls_controller.rb | 45 +- docs/internal/README.md | 9 +- .../syslog_drain_urls_controller_spec.rb | 587 +++++++++--------- 3 files changed, 311 insertions(+), 330 deletions(-) diff --git a/app/controllers/internal/syslog_drain_urls_controller.rb b/app/controllers/internal/syslog_drain_urls_controller.rb index 00e70a5944c..1e02c829899 100644 --- a/app/controllers/internal/syslog_drain_urls_controller.rb +++ b/app/controllers/internal/syslog_drain_urls_controller.rb @@ -3,53 +3,10 @@ class SyslogDrainUrlsInternalController < RestController::BaseController # Endpoint uses mutual tls for auth, handled by nginx allow_unauthenticated_access - get '/internal/v4/syslog_drain_urls', :list + get '/internal/v5/syslog_drain_urls', :list def list prepare_aggregate_function - guid_to_drain_maps = AppModel. - join(ServiceBinding.table_name, app_guid: :guid). - join(Space.table_name, guid: :apps__space_guid). - join(Organization.table_name, id: :spaces__organization_id). - where(Sequel.lit('syslog_drain_url IS NOT NULL')). - where(Sequel.lit("syslog_drain_url != ''")). - group( - :"#{AppModel.table_name}__guid", - :"#{AppModel.table_name}__name", - :"#{Space.table_name}__name", - :"#{Organization.table_name}__name" - ). - select( - :"#{AppModel.table_name}__guid", - :"#{AppModel.table_name}__name", - aggregate_function(:"#{ServiceBinding.table_name}__syslog_drain_url").as(:syslog_drain_urls) - ). - select_append(:"#{Space.table_name}__name___space_name"). - select_append(:"#{Organization.table_name}__name___organization_name"). - order(:guid). - limit(batch_size). - offset(last_id). - all - - next_page_token = nil - drain_urls = {} - - guid_to_drain_maps.each do |guid_and_drains| - drain_urls[guid_and_drains[:guid]] = { - drains: guid_and_drains[:syslog_drain_urls].split(','), - hostname: hostname_from_app_name(guid_and_drains[:organization_name], guid_and_drains[:space_name], guid_and_drains[:name]) - } - end - - next_page_token = last_id + batch_size unless guid_to_drain_maps.empty? - - [HTTP::OK, MultiJson.dump({ results: drain_urls, next_id: next_page_token, v5_available: true }, pretty: true)] - end - - get '/internal/v5/syslog_drain_urls', :listv5 - - def listv5 - prepare_aggregate_function bindings = ServiceBinding. join(:apps, guid: :app_guid). diff --git a/docs/internal/README.md b/docs/internal/README.md index 174aec74e48..940396e8bd2 100644 --- a/docs/internal/README.md +++ b/docs/internal/README.md @@ -91,15 +91,8 @@ We do not recommend using internal API endpoints for anything other than their i **Auth Mechanism:** mTLS -### GET /internal/v4/syslog_drain_urls -**Description:** Return list of syslog drain urls from logging services - -**Intended Consumer:** Loggregator - -**Auth Mechanism:** mTLS - ### GET /internal/v5/syslog_drain_urls -**Description:** Return list of syslog drain urls from logging services. Intends to replace v4 version. +**Description:** Return list of syslog drain urls from logging services. **Intended Consumer:** Loggregator diff --git a/spec/unit/controllers/internal/syslog_drain_urls_controller_spec.rb b/spec/unit/controllers/internal/syslog_drain_urls_controller_spec.rb index 5d44bd32ccc..98296415229 100644 --- a/spec/unit/controllers/internal/syslog_drain_urls_controller_spec.rb +++ b/spec/unit/controllers/internal/syslog_drain_urls_controller_spec.rb @@ -12,280 +12,6 @@ module VCAP::CloudController let!(:binding_with_drain1) { ServiceBinding.make(syslog_drain_url: 'fish,finger', app: app_obj, service_instance: instance1) } let!(:binding_with_drain2) { ServiceBinding.make(syslog_drain_url: 'foobar', app: app_obj, service_instance: instance2) } - describe 'GET /internal/v4/syslog_drain_urls' do - it 'returns a list of syslog drain urls' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_results.count).to eq(1) - expect(decoded_v5_available).to be(true) - expect(decoded_results).to include( - { - app_obj.guid => { 'drains' => contain_exactly('fish%2cfinger', 'foobar'), - 'hostname' => 'org-1.space-1.app-1' } - } - ) - end - - context 'rfc-1034-compliance: whitespace converted to hyphens' do - let(:org) { Organization.make(name: 'org 2') } - let(:space) { Space.make(name: 'space 2', organization: org) } - let(:app_obj) { AppModel.make(name: 'app 2', space: space) } - - it 'truncates trailing hyphens' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_results.count).to eq(1) - expect(decoded_v5_available).to be(true) - expect(decoded_results).to include( - { - app_obj.guid => { 'drains' => contain_exactly('fish%2cfinger', 'foobar'), - 'hostname' => 'org-2.space-2.app-2' } - } - ) - end - end - - context 'rfc-1034-compliance: named end with hyphens' do - let(:org) { Organization.make(name: 'org-3-') } - let(:space) { Space.make(name: 'space-3--', organization: org) } - let(:app_obj) { AppModel.make(name: 'app-3---', space: space) } - - it 'truncates trailing hyphens' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_results.count).to eq(1) - expect(decoded_v5_available).to be(true) - expect(decoded_results).to include( - { - app_obj.guid => { 'drains' => contain_exactly('fish%2cfinger', 'foobar'), - 'hostname' => 'org-3.space-3.app-3' } - } - ) - end - end - - context 'rfc-1034-compliance: remove disallowed characters' do - let(:org) { Organization.make(name: '!org@-4#' + [233].pack('U')) } - let(:space) { Space.make(name: '$space%-^4--&', organization: org) } - let(:app_obj) { AppModel.make(name: '";*app(-)4_-=-+-[]{}\\|;:,.<>/?`~', space: space) } - - it 'truncates trailing hyphens' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_results.count).to eq(1) - expect(decoded_v5_available).to be(true) - expect(decoded_results).to include( - { - app_obj.guid => { 'drains' => contain_exactly('fish%2cfinger', 'foobar'), - 'hostname' => 'org-4.space-4.app-4' } - } - ) - end - end - - context 'rfc-1034-compliance: truncate overlong name components to first 63' do - let(:orgName) { 'org-5-' + ('x' * (63 - 6)) } - let(:orgNamePlus) { orgName + 'y' } - let(:org) { Organization.make(name: orgNamePlus) } - let(:spaceName) { 'space-5-' + ('x' * (63 - 8)) } - let(:spaceNamePlus) { spaceName + 'y' } - let(:space) { Space.make(name: spaceNamePlus, organization: org) } - let(:appName) { 'app-5-' + ('x' * (63 - 6)) } - let(:appNamePlus) { appName + 'y' } - let(:app_obj) { AppModel.make(name: appNamePlus, space: space) } - - it 'truncates trailing hyphens' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_results.count).to eq(1) - expect(decoded_v5_available).to be(true) - expect(decoded_results).to include( - { - app_obj.guid => { 'drains' => contain_exactly('fish%2cfinger', 'foobar'), - 'hostname' => "#{orgName}.#{spaceName}.#{appName}" } - } - ) - end - end - - context 'rfc-1034-compliance: keep 63-char names' do - let(:orgName) { 'org-5-' + ('x' * (63 - 6)) } - let(:org) { Organization.make(name: orgName) } - let(:spaceName) { 'space-5-' + ('x' * (63 - 8)) } - let(:space) { Space.make(name: spaceName, organization: org) } - let(:appName) { 'app-5-' + ('x' * (63 - 6)) } - let(:app_obj) { AppModel.make(name: appName, space: space) } - - it 'retains length-compliant names' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_results.count).to eq(1) - expect(decoded_v5_available).to be(true) - expect(decoded_results).to include( - { - app_obj.guid => { 'drains' => contain_exactly('fish%2cfinger', 'foobar'), - 'hostname' => "#{orgName}.#{spaceName}.#{appName}" } - } - ) - end - end - - context 'when an app has no service binding' do - let!(:app_no_binding) { AppModel.make } - - it 'does not include that app' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_v5_available).to be(true) - expect(decoded_results).not_to have_key(app_no_binding.guid) - end - end - - context "when an app's bindings have no syslog_drain_url" do - let!(:app_no_drain) { ServiceBinding.make.app } - - it 'does not include that app' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_v5_available).to be(true) - expect(decoded_results).not_to have_key(app_no_drain.guid) - end - end - - context "when an app's binding has blank syslog_drain_urls" do - let!(:app_empty_drain) { ServiceBinding.make(syslog_drain_url: '').app } - - it 'includes the app without the empty syslog_drain_urls' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_v5_available).to be(true) - expect(decoded_results).not_to have_key(app_empty_drain.guid) - end - end - - context 'when there are many service bindings on a single app' do - before do - 50.times do |i| - ServiceBinding.make( - app: app_obj, - syslog_drain_url: "syslog://example.com/#{i}", - service_instance: UserProvidedServiceInstance.make(space: app_obj.space) - ) - end - end - - it 'includes all of the syslog_drain_urls for that app' do - get '/internal/v4/syslog_drain_urls', '{}' - expect(last_response).to be_successful - expect(decoded_v5_available).to be(true) - expect(decoded_results[app_obj.guid]['drains'].length).to eq(52) - end - end - - describe 'paging' do - before do - 3.times do - app_obj = AppModel.make - instance = UserProvidedServiceInstance.make(space: app_obj.space) - ServiceBinding.make(syslog_drain_url: 'fish,finger', app: app_obj, service_instance: instance) - end - end - - it 'respects the batch_size parameter' do - [1, 3].each do |size| - get '/internal/v4/syslog_drain_urls', { 'batch_size' => size } - expect(last_response).to be_successful - expect(decoded_v5_available).to be(true) - expect(decoded_results.size).to eq(size) - end - end - - it 'returns non-intersecting results when token is supplied' do - get '/internal/v4/syslog_drain_urls', { - 'batch_size' => 2, - 'next_id' => 0 - } - - saved_results = decoded_results.dup - expect(saved_results.size).to eq(2) - - get '/internal/v4/syslog_drain_urls', { - 'batch_size' => 2, - 'next_id' => decoded_response['next_id'] - } - - new_results = decoded_results.dup - - expect(new_results.size).to eq(2) - saved_results.each_key do |guid| - expect(new_results).not_to have_key(guid) - end - end - - it 'eventuallies return entire collection, batch after batch' do - apps = {} - total_size = AppModel.count - - token = 0 - while apps.size < total_size - get '/internal/v4/syslog_drain_urls', { - 'batch_size' => 2, - 'next_id' => token - } - - expect(last_response.status).to eq(200) - token = decoded_response['next_id'] - apps.merge!(decoded_results) - end - - expect(apps.size).to eq(total_size) - get '/internal/v4/syslog_drain_urls', { - 'batch_size' => 2, - 'next_id' => token - } - expect(decoded_results.size).to eq(0) - expect(decoded_v5_available).to be(true) - expect(decoded_response['next_id']).to be_nil - end - - context 'when an app has no service_bindings' do - before do - AppModel.make(guid: '00000') - end - - it 'does not affect the paging results' do - get '/internal/v4/syslog_drain_urls', { - 'batch_size' => 2, - 'next_id' => 0 - } - - saved_results = decoded_results.dup - expect(saved_results.size).to eq(2) - expect(decoded_v5_available).to be(true) - end - end - - context 'when an app has no syslog_drain_urls' do - let(:app_with_first_ordered_guid) { AppModel.make(guid: '000', space: instance1.space) } - - before do - ServiceBinding.make(syslog_drain_url: nil, app: app_with_first_ordered_guid, service_instance: instance1) - end - - it 'does not affect the paging results' do - get '/internal/v4/syslog_drain_urls', { - 'batch_size' => 2, - 'next_id' => 0 - } - - saved_results = decoded_results.dup - expect(saved_results.size).to eq(2) - expect(decoded_v5_available).to be(true) - end - end - end - end - describe 'GET /internal/v5/syslog_drain_urls' do let(:app_obj2) { AppModel.make(name: 'app-2', space: space) } let(:app_obj3) { AppModel.make(name: 'app-3', space: space) } @@ -531,6 +257,315 @@ module VCAP::CloudController expect(decoded_next_id).to be_nil expect(decoded_results.length).to be(0) end + + context 'rfc-1034-compliance: whitespace converted to hyphens' do + let(:org) { Organization.make(name: 'org 2') } + let(:space) { Space.make(name: 'space 2', organization: org) } + let(:app_obj) { AppModel.make(name: 'app 2', space: space) } + + it 'truncates trailing hyphens' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results.count).to eq(8) + expect(decoded_results).to include( + { + 'credentials' => + [{ 'apps' => + [{ 'app_id' => app_obj.guid, + 'hostname' => 'org-2.space-2.app-2' }], + 'ca' => '', + 'cert' => '', + 'key' => '' }], + 'url' => 'fish%2cfinger' + } + ) + end + end + + context 'rfc-1034-compliance: named end with hyphens' do + let(:org) { Organization.make(name: 'org-3-') } + let(:space) { Space.make(name: 'space-3--', organization: org) } + let(:app_obj) { AppModel.make(name: 'app-3---', space: space) } + + it 'truncates trailing hyphens' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results.count).to eq(8) + expect(decoded_results).to include( + { + 'credentials' => + [{ 'apps' => + [{ 'app_id' => app_obj.guid, + 'hostname' => 'org-3.space-3.app-3' }], + 'ca' => '', + 'cert' => '', + 'key' => '' }], + 'url' => 'fish%2cfinger' + } + ) + end + end + + context 'rfc-1034-compliance: remove disallowed characters' do + let(:org) { Organization.make(name: '!org@-4#' + [233].pack('U')) } + let(:space) { Space.make(name: '$space%-^4--&', organization: org) } + let(:app_obj) { AppModel.make(name: '";*app(-)4_-=-+-[]{}\\|;:,.<>/?`~', space: space) } + + it 'truncates trailing hyphens' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results.count).to eq(8) + expect(decoded_results).to include( + { + 'credentials' => + [{ 'apps' => + [{ 'app_id' => app_obj.guid, + 'hostname' => 'org-4.space-4.app-4' }], + 'ca' => '', + 'cert' => '', + 'key' => '' }], + 'url' => 'fish%2cfinger' + } + ) + end + end + + # rubocop:disable RSpec/MultipleMemoizedHelpers + context 'rfc-1034-compliance: truncate overlong name components to first 63' do + let(:orgName) { 'org-5-' + ('x' * (63 - 6)) } + let(:orgNamePlus) { orgName + 'y' } + let(:org) { Organization.make(name: orgNamePlus) } + let(:spaceName) { 'space-5-' + ('x' * (63 - 8)) } + let(:spaceNamePlus) { spaceName + 'y' } + let(:space) { Space.make(name: spaceNamePlus, organization: org) } + let(:appName) { 'app-5-' + ('x' * (63 - 6)) } + let(:appNamePlus) { appName + 'y' } + let(:app_obj) { AppModel.make(name: appNamePlus, space: space) } + + it 'truncates trailing hyphens' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results.count).to eq(8) + expect(decoded_results).to include( + { + 'credentials' => + [{ 'apps' => + [{ 'app_id' => app_obj.guid, + 'hostname' => "#{orgName}.#{spaceName}.#{appName}" }], + 'ca' => '', + 'cert' => '', + 'key' => '' }], + 'url' => 'fish%2cfinger' + } + ) + end + end + + context 'rfc-1034-compliance: keep 63-char names' do + let(:orgName) { 'org-5-' + ('x' * (63 - 6)) } + let(:org) { Organization.make(name: orgName) } + let(:spaceName) { 'space-5-' + ('x' * (63 - 8)) } + let(:space) { Space.make(name: spaceName, organization: org) } + let(:appName) { 'app-5-' + ('x' * (63 - 6)) } + let(:app_obj) { AppModel.make(name: appName, space: space) } + + it 'retains length-compliant names' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results.count).to eq(8) + expect(decoded_results).to include( + { + 'credentials' => + [{ 'apps' => + [{ 'app_id' => app_obj.guid, + 'hostname' => "#{orgName}.#{spaceName}.#{appName}" }], + 'ca' => '', + 'cert' => '', + 'key' => '' }], + 'url' => 'fish%2cfinger' + } + ) + end + end + + context 'when an app has no service binding' do + let!(:app_no_binding) { AppModel.make } + + it 'does not include that app' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results).not_to include( + { + 'credentials' => + [{ 'apps' => + [{ 'app_id' => app_no_binding.guid }] }] + } + ) + end + end + + context "when an app's bindings have no syslog_drain_url" do + it 'does not include that app' do + app_no_drain = ServiceBinding.make.app + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results.count).to eq(8) + expect(decoded_results).not_to include( + { + 'credentials' => + [{ 'apps' => + [{ 'app_id' => app_no_drain.guid }] }] + } + ) + end + end + + context "when an app's binding has blank syslog_drain_urls" do + let!(:app_empty_drain) { ServiceBinding.make(syslog_drain_url: '').app } + + it 'includes the app without the empty syslog_drain_urls' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results).not_to include( + { + 'credentials' => + [{ 'apps' => + [{ 'app_id' => app_empty_drain.guid }] }] + } + ) + end + end + # rubocop:enable RSpec/MultipleMemoizedHelpers + + context 'when there are many service bindings on a single app' do + before do + 50.times do |i| + ServiceBinding.make( + app: app_obj, + syslog_drain_url: "syslog://example.com/#{i}", + service_instance: UserProvidedServiceInstance.make(space: app_obj.space) + ) + end + end + + it 'includes all of the syslog_drain_urls for that app' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + expect(decoded_results.length).to eq(50) + end + end + + describe 'paging' do + before do + 3.times do + app_obj = AppModel.make + instance = UserProvidedServiceInstance.make(space: app_obj.space) + ServiceBinding.make(syslog_drain_url: 'fish,finger', app: app_obj, service_instance: instance) + end + end + + it 'respects the batch_size parameter' do + [1, 3].each do |size| + get '/internal/v5/syslog_drain_urls', { 'batch_size' => size } + expect(last_response).to be_successful + expect(decoded_results.size).to eq(size) + end + end + + it 'returns non-intersecting results when token is supplied' do + get '/internal/v5/syslog_drain_urls', { + 'batch_size' => 2, + 'next_id' => 0 + } + + saved_results = decoded_results.dup + expect(saved_results.size).to eq(2) + + get '/internal/v5/syslog_drain_urls', { + 'batch_size' => 2, + 'next_id' => decoded_response['next_id'] + } + + new_results = decoded_results.dup + expect(new_results.size).to eq(2) + saved_urls = [] + saved_results.each do |cred| + saved_urls.append(cred['url']) + end + + new_urls = [] + new_results.each do |cred| + new_urls.append(cred['url']) + end + + saved_urls.each do |url| + expect(new_urls).not_to include(url) + end + end + + it 'eventually return entire collection, batch after batch' do + get '/internal/v5/syslog_drain_urls', '{}' + expect(last_response).to be_successful + total_size = decoded_results.length + + token = 0 + drains = [] + while drains.size < total_size + get '/internal/v5/syslog_drain_urls', { + 'batch_size' => 2, + 'next_id' => token + } + + expect(last_response.status).to eq(200) + token = decoded_response['next_id'] + drains.push(*decoded_results) + end + expect(drains.size).to eq(total_size) + + get '/internal/v5/syslog_drain_urls', { + 'batch_size' => 2, + 'next_id' => token + } + expect(decoded_results.size).to eq(0) + expect(decoded_response['next_id']).to be_nil + end + + context 'when an app has no service_bindings' do + before do + AppModel.make(guid: '00000') + end + + it 'does not affect the paging results' do + get '/internal/v5/syslog_drain_urls', { + 'batch_size' => 2, + 'next_id' => 0 + } + + saved_results = decoded_results.dup + expect(saved_results.size).to eq(2) + end + end + + # rubocop:disable RSpec/MultipleMemoizedHelpers + context 'when an app has no syslog_drain_urls' do + let(:app_with_first_ordered_guid) { AppModel.make(guid: '000', space: instance1.space) } + + before do + ServiceBinding.make(syslog_drain_url: nil, app: app_with_first_ordered_guid, service_instance: instance1) + end + + it 'does not affect the paging results' do + get '/internal/v5/syslog_drain_urls', { + 'batch_size' => 2, + 'next_id' => 0 + } + + saved_results = decoded_results.dup + expect(saved_results.size).to eq(2) + end + end + # rubocop:enable RSpec/MultipleMemoizedHelpers + end end def decoded_results @@ -540,9 +575,5 @@ def decoded_results def decoded_next_id decoded_response.fetch('next_id') end - - def decoded_v5_available - decoded_response.fetch('v5_available') - end end end