Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uri shortener for long host label endpoints #2083

Merged
merged 2 commits into from
Jul 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/models/proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -216,11 +216,13 @@ def config
def generate(name)
template = config.fetch(name.try(:to_sym)) { return }

format template, {
uri = format template, {
system_name: service.parameterized_system_name, account_id: service.account_id,
tenant_name: provider_subdomain,
env: proxy.proxy_env, port: proxy.proxy_port
}

UriShortener.call(uri).to_s
end
end

Expand Down
4 changes: 0 additions & 4 deletions app/services/service_creator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ def call!(params = {})
def call(params = {})
call!(params)
rescue ActiveRecord::RecordInvalid
return if !proxy || proxy.errors[:endpoint].none? { |message| message =~ /is too long/ }

@service.errors.add(:system_name, :invalid_for_proxy_endpoints)

false
end

Expand Down
47 changes: 47 additions & 0 deletions app/services/uri_shortener.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# frozen_string_literal: true

class UriShortener
def self.call(uri)
new(uri).call
end

def initialize(uri)
@uri = URI.parse(uri)
rescue URI::InvalidURIError
end

attr_reader :uri

def call
return unless uri
labels = uri.host.split('.')
uri.host = labels.map(&UriLabelShortner.method(:call)).join('.')
uri
end

class UriLabelShortner
def self.call(label)
new(label).call
end

def initialize(label)
@label = label
end

attr_reader :label
delegate :size, to: :label

MAX_LABEL_SIZE = UriValidator::UriThreeScaleComplianceChecker::MAX_LABEL_SIZE
HASH_SIZE = 7
KEEP_SIZE = (MAX_LABEL_SIZE - HASH_SIZE - 1).freeze

def call
return label if size <= MAX_LABEL_SIZE
[*label.slice(0, KEEP_SIZE).split('-'), hash].join('-')
end

def hash
Digest::SHA1.hexdigest(label).slice(0, HASH_SIZE)
end
end
end
1 change: 0 additions & 1 deletion config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -898,7 +898,6 @@ en:
attributes:
system_name:
invalid: invalid. Only ASCII letters, numbers, dashes and underscores are allowed.
invalid_for_proxy_endpoints: must be shorter.

cinstance:
attributes:
Expand Down
11 changes: 0 additions & 11 deletions test/integration/api/services_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -236,16 +236,5 @@ class ServiceCreateTest < Api::ServicesControllerTest
post admin_services_path, service: { name: 'example-service', system_name: '###' }
assert_equal 'System name invalid. Only ASCII letters, numbers, dashes and underscores are allowed.', flash[:error]
end

test 'chosen system name affects proxy endpoint validation' do
@provider.settings.allow_multiple_services!

post admin_services_path, service: { name: 'My New Product', system_name: 'this-hostname-label-is-longer-than-63-chars-which-is-not-allowed-according-to-rfc-1035' }
assert_equal 'System name must be shorter.', flash[:error]

post admin_services_path, service: { name: 'My New Product', system_name: 'short-labels-are-ok' }
refute flash[:error].presence
assert_response :redirect
end
end
end
13 changes: 13 additions & 0 deletions test/unit/proxy_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,19 @@ def test_endpoints_on_create
assert proxy.production_endpoint
end

test 'generates shortened URI label endpoints' do
service = FactoryBot.create(:simple_service, deployment_option: 'hosted', system_name: 'long-system-name-that-will-cause-invalid-host', proxy: nil)

proxy_config = System::Application.config.three_scale.sandbox_proxy
proxy_config.expects(:fetch).with(:port).at_least_once.returns('8080')
proxy_config.expects(:fetch).with(:apicast_staging_endpoint).at_least_once.returns('http://%{system_name}-needs-to-be-63-chars-or-shorter.staging.apicast.dev:8080')
proxy_config.expects(:fetch).with(:apicast_production_endpoint).at_least_once.returns('http://%{system_name}-needs-to-be-63-chars-or-shorter.production.apicast.dev:8080')

proxy = FactoryBot.create(:proxy, service: service, apicast_configuration_driven: true)
assert_equal 'http://long-system-name-that-will-cause-invalid-host-needs-to-4207987.staging.apicast.dev:8080', proxy.sandbox_endpoint
assert_equal 'http://long-system-name-that-will-cause-invalid-host-needs-to-4207987.production.apicast.dev:8080', proxy.endpoint
end

def test_deployable
assert_predicate Service.new(deployment_option: 'hosted').build_proxy, :deployable?
assert_predicate Service.new(deployment_option: 'self_managed').build_proxy, :deployable?
Expand Down
7 changes: 3 additions & 4 deletions test/unit/services/service_creator_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,10 @@ class ServiceCreatorTest < ActiveSupport::TestCase
assert backend_api_config.persisted?
end

test 'service with a too long system_name for its proxy' do
test 'service with a long system_name used in its proxy endpoint' do
system_name = 'this-hostname-label-is-longer-than-63-chars-which-is-not-allowed-according-to-rfc-1035'
service = FactoryBot.build(:service, system_name: system_name)
creator = ServiceCreator.new(service: service)
creator.call
assert_includes service.errors[:system_name], 'must be shorter.'
ServiceCreator.new(service: service).call
assert_match /this-hostname-label-is-longer-than-63-chars-which-is-no-/, service.proxy.endpoint
end
end
31 changes: 31 additions & 0 deletions test/unit/services/uri_shortener_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# frozen_string_literal: true

require 'test_helper'

class UriShortenerTest < ActiveSupport::TestCase
test 'uri with long label' do
shortener = UriShortener.new('http://this-hostname-label-is-longer-than-63-chars-which-is-not-allowed-according-to-rfc-1035.example.com')
assert_equal 'http://this-hostname-label-is-longer-than-63-chars-which-is-no-a6ca0fb.example.com', shortener.call.to_s
end

test 'uri with multiple long labels' do
shortener = UriShortener.new('http://this-hostname-label-is-longer-than-63-chars-which-is-not-allowed-according-to-rfc-1035.this-other-label-is-as-well-longer-than-63-chars-also-violating-the-rfc-1035.example.com')
assert_equal 'http://this-hostname-label-is-longer-than-63-chars-which-is-no-a6ca0fb.this-other-label-is-as-well-longer-than-63-chars-also-v-57fa2c4.example.com', shortener.call.to_s
end

test 'does not duplicate leading dash' do
shortener = UriShortener.new('http://long-system-name-that-will-cause-invalid-host-needs-to-be-63-chars-or-shorter.example.com')
assert_not_equal 'http://long-system-name-that-will-cause-invalid-host-needs-to--4207987.example.com', shortener.call.to_s
assert_equal 'http://long-system-name-that-will-cause-invalid-host-needs-to-4207987.example.com', shortener.call.to_s
end

test 'uri with no long labels' do
shortener = UriShortener.new('http://this-label-is-ok.example.com')
assert_equal 'http://this-label-is-ok.example.com', shortener.call.to_s
end

test 'invalid uri' do
shortener = UriShortener.new('not a uri')
refute shortener.call
end
end