From c1584096c100dc115da4f616da1cb12cfd78c197 Mon Sep 17 00:00:00 2001 From: Alexander Nicke Date: Mon, 4 Nov 2024 14:29:06 +0100 Subject: [PATCH 1/7] Introduce per-route options This commit adds a configurable load-balancing algorithm as a first example of per-route options. It adds the 'options' field to the route object in the V3 API, and the app manifest. The options field is an object storing key-value pairs, with 'lb_algo' being the only supported key for now. The supported load-balancing algorithms are 'round-robin' and 'least-connections'. The options field is introduced as a column in the database route table, and forwarded to the Diego backend. Co-authored-by: Alexander Nicke See: https://github.com/cloudfoundry/capi-release/issues/482 See: https://github.com/cloudfoundry/community/blob/main/toc/rfc/rfc-0027-generic-per-route-features.md --- app/actions/manifest_route_update.rb | 10 +- app/actions/route_create.rb | 3 +- app/actions/route_update.rb | 10 ++ .../manifest_routes_update_message.rb | 42 +++++- app/messages/route_create_message.rb | 7 + app/messages/route_options_message.rb | 16 +++ app/messages/route_update_message.rb | 13 +- app/messages/validators.rb | 20 +++ app/models/runtime/route.rb | 21 ++- .../route_properties_presenter.rb | 9 +- app/presenters/v3/route_presenter.rb | 1 + .../20241105000000_add_options_to_route.rb | 14 ++ .../app_manifest/manifest_route.rb | 20 ++- .../diego/app_recipe_builder.rb | 4 +- .../diego/protocol/routing_info.rb | 1 + lib/cloud_controller/route_validator.rb | 1 + spec/request/space_manifests_spec.rb | 130 ++++++++++++++++++ spec/support/legacy_api_dsl.rb | 2 +- .../app_manifest/manifest_route_spec.rb | 46 ++++++- .../manifest_routes_update_message_spec.rb | 50 +++++++ .../messages/route_create_message_spec.rb | 108 +++++++++++++++ .../messages/route_update_message_spec.rb | 32 +++++ spec/unit/messages/validators_spec.rb | 45 ++++++ spec/unit/models/runtime/route_spec.rb | 4 +- .../presenters/v3/route_presenter_spec.rb | 20 ++- 25 files changed, 606 insertions(+), 23 deletions(-) create mode 100644 app/messages/route_options_message.rb create mode 100644 db/migrations/20241105000000_add_options_to_route.rb diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index d5fef841e5d..50830da16d3 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -1,5 +1,6 @@ require 'cloud_controller/app_manifest/manifest_route' require 'actions/route_create' +require 'actions/route_update' module VCAP::CloudController class ManifestRouteUpdate @@ -81,7 +82,8 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) message = RouteCreateMessage.new({ 'host' => host, 'path' => manifest_route[:path], - 'port' => manifest_route[:port] + 'port' => manifest_route[:port], + 'options' => manifest_route[:options] }) route = RouteCreate.new(user_audit_info).create( @@ -90,6 +92,12 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) domain: existing_domain, manifest_triggered: true ) + elsif route[:options] != manifest_route[:options] + message = RouteUpdateMessage.new({ + 'options' => manifest_route[:options] + }) + route = RouteUpdate.new.update(route:, message:) + elsif route.space.guid != app.space_guid raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces') end diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index 21d08fd8fdd..bac1c489739 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -15,7 +15,8 @@ def create(message:, space:, domain:, manifest_triggered: false) path: message.path || '', port: port(message, domain), space: space, - domain: domain + domain: domain, + options: message.options ) Route.db.transaction do diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index d5254a824b7..91a1821a4a2 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -2,6 +2,16 @@ module VCAP::CloudController class RouteUpdate def update(route:, message:) Route.db.transaction do + if message.requested?(:options) + route.options = if message.options.nil? + nil + elsif route.options.nil? + message.options + else + route.options.merge(message.options) + end + end + route.save MetadataUpdate.update(route, message) end diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 74fa2a31e8d..00923bee92d 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -1,4 +1,5 @@ require 'messages/base_message' +require 'messages/route_options_message' require 'cloud_controller/app_manifest/manifest_route' module VCAP::CloudController @@ -7,9 +8,20 @@ class ManifestRoutesUpdateMessage < BaseMessage class ManifestRoutesYAMLValidator < ActiveModel::Validator def validate(record) - return unless is_not_array?(record.routes) || contains_non_route_hash_values?(record.routes) + if is_not_array?(record.routes) || contains_non_route_hash_values?(record.routes) + record.errors.add(:routes, message: 'must be a list of route objects') + return + end - record.errors.add(:routes, message: 'must be a list of route objects') + if contains_invalid_route_options?(record.routes) + record.errors.add(:routes, message: 'contains invalid route options') + return + end + + return unless contains_invalid_lb_algo?(record.routes) + + record.errors.add(:routes, message: 'contains an invalid loadbalancing-algorithm option') + nil end def is_not_array?(routes) @@ -19,6 +31,26 @@ def is_not_array?(routes) def contains_non_route_hash_values?(routes) routes.any? { |r| !(r.is_a?(Hash) && r[:route].present?) } end + + def contains_invalid_route_options?(routes) + routes.any? do |r| + next unless r[:options] + + return true unless r[:options].is_a?(Hash) + + return false if r[:options].empty? + + return r[:options].keys.all? { |key| RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) } + end + end + + def contains_invalid_lb_algo?(routes) + routes.any? do |r| + next unless r[:options] && r[:options][:'loadbalancing-algorithm'] + + return true if r[:options][:'loadbalancing-algorithm'] && RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(r[:options][:'loadbalancing-algorithm']) + end + end end validates_with NoAdditionalKeysValidator @@ -32,10 +64,12 @@ def contains_non_route_hash_values?(routes) def manifest_route_mappings @manifest_route_mappings ||= routes.map do |route| - { - route: ManifestRoute.parse(route[:route]), + r = { + route: ManifestRoute.parse(route[:route], route[:options]), protocol: route[:protocol] } + r[:options] = route[:options] unless route[:options].nil? + r end end diff --git a/app/messages/route_create_message.rb b/app/messages/route_create_message.rb index 1eeac2c2e46..12a1eda8581 100644 --- a/app/messages/route_create_message.rb +++ b/app/messages/route_create_message.rb @@ -1,4 +1,5 @@ require 'messages/metadata_base_message' +require 'messages/route_options_message' module VCAP::CloudController class RouteCreateMessage < MetadataBaseMessage @@ -10,6 +11,7 @@ class RouteCreateMessage < MetadataBaseMessage path port relationships + options ] validates :host, @@ -56,6 +58,7 @@ class RouteCreateMessage < MetadataBaseMessage validates_with NoAdditionalKeysValidator validates_with RelationshipValidator + validates_with OptionsValidator delegate :space_guid, to: :relationships_message delegate :domain_guid, to: :relationships_message @@ -65,6 +68,10 @@ def relationships_message @relationships_message ||= Relationships.new(relationships&.deep_symbolize_keys) end + def options_message + @options_message ||= RouteOptionsMessage.new(options&.deep_symbolize_keys) + end + def wildcard? host == '*' end diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb new file mode 100644 index 00000000000..614eccba7f7 --- /dev/null +++ b/app/messages/route_options_message.rb @@ -0,0 +1,16 @@ +require 'messages/metadata_base_message' + +module VCAP::CloudController + class RouteOptionsMessage < BaseMessage + VALID_MANIFEST_ROUTE_OPTIONS = %i[loadbalancing-algorithm].freeze + VALID_ROUTE_OPTIONS = %i[lb_algo].freeze + VALID_LOADBALANCING_ALGORITHMS = %w[round-robin least-connections].freeze + + register_allowed_keys %i[lb_algo] + validates_with NoAdditionalKeysValidator + validates :lb_algo, + inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "'%s' is not a supported load-balancing algorithm" }, + presence: true, + allow_nil: true + end +end diff --git a/app/messages/route_update_message.rb b/app/messages/route_update_message.rb index 84365bdb3d2..3e567404690 100644 --- a/app/messages/route_update_message.rb +++ b/app/messages/route_update_message.rb @@ -1,8 +1,19 @@ require 'messages/metadata_base_message' +require 'messages/route_options_message' module VCAP::CloudController class RouteUpdateMessage < MetadataBaseMessage - register_allowed_keys [] + register_allowed_keys %i[options] + + def self.options_requested? + @options_requested ||= proc { |a| a.requested?(:options) } + end + + def options_message + @options_message ||= RouteOptionsMessage.new(options&.deep_symbolize_keys) + end + + validates_with OptionsValidator, if: options_requested? validates_with NoAdditionalKeysValidator end diff --git a/app/messages/validators.rb b/app/messages/validators.rb index c5555856c33..c9baf7e83a2 100644 --- a/app/messages/validators.rb +++ b/app/messages/validators.rb @@ -240,6 +240,26 @@ def validate(record) end end + class OptionsValidator < ActiveModel::Validator + def validate(record) + # Empty option hashes are allowed, so we skip further validation + record.options.blank? && return + + unless record.options.is_a?(Hash) + record.errors.add(:options, message: "'options' is not a valid object") + return + end + + opt = record.options_message + + return if opt.valid? + + opt.errors.full_messages.each do |message| + record.errors.add(:options, message:) + end + end + end + class ToOneRelationshipValidator < ActiveModel::EachValidator def validate_each(record, attribute, relationship) if has_correct_structure?(relationship) diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index 3f10c89dd67..7ec7a0b2264 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -40,8 +40,8 @@ class OutOfVIPException < CloudController::Errors::InvalidRelation; end add_association_dependencies route_mappings: :destroy - export_attributes :host, :path, :domain_guid, :space_guid, :service_instance_guid, :port - import_attributes :host, :path, :domain_guid, :space_guid, :app_guids, :port + export_attributes :host, :path, :domain_guid, :space_guid, :service_instance_guid, :port, :options + import_attributes :host, :path, :domain_guid, :space_guid, :app_guids, :port, :options add_association_dependencies labels: :destroy add_association_dependencies annotations: :destroy @@ -71,6 +71,23 @@ def as_summary_json } end + def options_with_serialization=(opts) + self.options_without_serialization = Oj.dump(opts) + end + + alias_method :options_without_serialization=, :options= + alias_method :options=, :options_with_serialization= + + def options_with_serialization + string = options_without_serialization + return nil if string.blank? + + Oj.load(string) + end + + alias_method :options_without_serialization, :options + alias_method :options, :options_with_serialization + alias_method :old_path, :path def path old_path.nil? ? '' : old_path diff --git a/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb b/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb index d3f43f40d31..f4cef6ab901 100644 --- a/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb +++ b/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb @@ -5,10 +5,17 @@ module AppManifestPresenters class RoutePropertiesPresenter def to_hash(route_mappings:, app:, **_) route_hashes = route_mappings.map do |route_mapping| - { + route_hash = { route: route_mapping.route.uri, protocol: route_mapping.protocol } + + if route_mapping.route.options + route_hash[:options] = {} + route_hash[:options][:'loadbalancing-algorithm'] = route_mapping.route.options[:lb_algo] if route_mapping.route.options[:lb_algo] + end + + route_hash end { routes: alphabetize(route_hashes).presence } diff --git a/app/presenters/v3/route_presenter.rb b/app/presenters/v3/route_presenter.rb index 9715e3c09c9..c090fafae5b 100644 --- a/app/presenters/v3/route_presenter.rb +++ b/app/presenters/v3/route_presenter.rb @@ -48,6 +48,7 @@ def to_hash }, links: build_links } + hash.merge!(options: route.options) unless route.options.nil? @decorators.reduce(hash) { |memo, d| d.decorate(memo, [route]) } end diff --git a/db/migrations/20241105000000_add_options_to_route.rb b/db/migrations/20241105000000_add_options_to_route.rb new file mode 100644 index 00000000000..94b75933ced --- /dev/null +++ b/db/migrations/20241105000000_add_options_to_route.rb @@ -0,0 +1,14 @@ +Sequel.migration do + up do + alter_table(:routes) do + # rubocop:disable Migration/IncludeStringSize + add_column :options, String, text: true, default: nil + # rubocop:enable Migration/IncludeStringSize + end + end + down do + alter_table(:routes) do + drop_column :options + end + end +end diff --git a/lib/cloud_controller/app_manifest/manifest_route.rb b/lib/cloud_controller/app_manifest/manifest_route.rb index 74ad1493e72..6a3cab14c90 100644 --- a/lib/cloud_controller/app_manifest/manifest_route.rb +++ b/lib/cloud_controller/app_manifest/manifest_route.rb @@ -6,8 +6,8 @@ class ManifestRoute SUPPORTED_TCP_SCHEMES = %w[tcp unspecified].freeze WILDCARD_HOST = '*'.freeze - def self.parse(string) - parsed_uri = Addressable::URI.heuristic_parse(string, scheme: 'unspecified') + def self.parse(route, options=nil) + parsed_uri = Addressable::URI.heuristic_parse(route, scheme: 'unspecified') attrs = if parsed_uri.nil? {} @@ -15,11 +15,22 @@ def self.parse(string) parsed_uri.to_hash end - attrs[:full_route] = string + attrs[:full_route] = route + + if options + attrs[:options] = {} + attrs[:options][:lb_algo] = options[:'loadbalancing-algorithm'] if options[:'loadbalancing-algorithm'] + end + ManifestRoute.new(attrs) end def valid? + if @attrs[:options] && !@attrs[:options].empty? + return false if @attrs[:options].keys.any? { |key| RouteOptionsMessage::VALID_ROUTE_OPTIONS.exclude?(key) } + return false if @attrs[:options][:lb_algo] && RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(@attrs[:options][:lb_algo]) + end + return false if @attrs[:host].blank? return SUPPORTED_TCP_SCHEMES.include?(@attrs[:scheme]) if @attrs[:port] @@ -43,7 +54,8 @@ def to_hash { candidate_host_domain_pairs: pairs, port: @attrs[:port], - path: @attrs[:path] + path: @attrs[:path], + options: @attrs[:options] } end diff --git a/lib/cloud_controller/diego/app_recipe_builder.rb b/lib/cloud_controller/diego/app_recipe_builder.rb index 46ce619baa6..f206e901349 100644 --- a/lib/cloud_controller/diego/app_recipe_builder.rb +++ b/lib/cloud_controller/diego/app_recipe_builder.rb @@ -151,13 +151,15 @@ def health_check_timeout_in_seconds def generate_routes(info) http_routes = (info['http_routes'] || []).map do |i| - { + http_route = { hostnames: [i['hostname']], port: i['port'], route_service_url: i['route_service_url'], isolation_segment: IsolationSegmentSelector.for_space(process.space), protocol: i['protocol'] } + http_route[:options] = i['options'] if i['options'] + http_route end { diff --git a/lib/cloud_controller/diego/protocol/routing_info.rb b/lib/cloud_controller/diego/protocol/routing_info.rb index 278f587f347..e85c061a4fd 100644 --- a/lib/cloud_controller/diego/protocol/routing_info.rb +++ b/lib/cloud_controller/diego/protocol/routing_info.rb @@ -43,6 +43,7 @@ def http_info(process_eager) info['router_group_guid'] = r.domain.router_group_guid if r.domain.is_a?(SharedDomain) && !r.domain.router_group_guid.nil? info['port'] = get_port_to_use(route_mapping) info['protocol'] = route_mapping.protocol + info['options'] = r.options if r.options info end end diff --git a/lib/cloud_controller/route_validator.rb b/lib/cloud_controller/route_validator.rb index 978fda7c02d..637a883ab38 100644 --- a/lib/cloud_controller/route_validator.rb +++ b/lib/cloud_controller/route_validator.rb @@ -4,6 +4,7 @@ class ValidationError < StandardError; end class DomainInvalid < ValidationError; end class RouteInvalid < ValidationError; end class RoutePortTaken < ValidationError; end + class InvalidRouteOptions < ValidationError; end attr_reader :route diff --git a/spec/request/space_manifests_spec.rb b/spec/request/space_manifests_spec.rb index 74b77103bd0..d3f21eaa922 100644 --- a/spec/request/space_manifests_spec.rb +++ b/spec/request/space_manifests_spec.rb @@ -624,6 +624,136 @@ end end + describe 'route options' do + context 'when an empty route options hash is provided' do + let(:yml_manifest) do + { + 'applications' => [ + { + 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://#{route.host}.#{route.domain.name}", + 'options' => {} } + ] + } + ] + }.to_yaml + end + + it 'applies the manifest' do + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response.status).to eq(202) + end + end + + context 'when an invalid route option is provided' do + let(:yml_manifest) do + { + 'applications' => [ + { + 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://#{route.host}.#{route.domain.name}", + 'options' => { + 'doesnt-exist' => 'doesnt-exist' + } } + ] + } + ] + }.to_yaml + end + + it 'returns a 422' do + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response).to have_status_code(422) + expect(last_response).to have_error_message('Routes contains invalid route options') + end + end + + context 'loadbalancing-algorithm' do + context 'when the loadbalancing-algorithm is not supported' do + let(:yml_manifest) do + { + 'applications' => [ + { + 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://#{route.host}.#{route.domain.name}", + 'options' => { + 'loadbalancing-algorithm' => 'unsupported-lb-algorithm' + } } + ] + } + ] + }.to_yaml + end + + it 'returns a 422' do + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response).to have_status_code(422) + expect(last_response).to have_error_message('Routes contains an invalid loadbalancing-algorithm option') + end + end + + context 'when the loadbalancing-algorithm is supported' do + let(:yml_manifest) do + { + 'applications' => [ + { 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://round-robin-app.#{shared_domain.name}", + 'options' => { + 'loadbalancing-algorithm' => 'round-robin' + } } + ] } + ] + }.to_yaml + end + + it 'adds and updates the loadbalancing-algorithm' do + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response.status).to eq(202) + job_guid = VCAP::CloudController::PollableJobModel.last.guid + + Delayed::Worker.new.work_off + expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error + + app1_model.reload + expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + + ### update the loadbalancing-algorithm from the route + + yml_manifest = { + 'applications' => [ + { 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://round-robin-app.#{shared_domain.name}", + 'options' => { + 'loadbalancing-algorithm' => 'least-connections' + } } + ] } + ] + }.to_yaml + + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response.status).to eq(202) + job_guid = VCAP::CloudController::PollableJobModel.last.guid + + Delayed::Worker.new.work_off + expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error + + app1_model.reload + expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'least-connections' }) + end + end + end + end + describe 'audit events' do let!(:process1) { nil } diff --git a/spec/support/legacy_api_dsl.rb b/spec/support/legacy_api_dsl.rb index c5c9c888291..54ab3deb48c 100644 --- a/spec/support/legacy_api_dsl.rb +++ b/spec/support/legacy_api_dsl.rb @@ -4,7 +4,7 @@ module LegacyApiDsl extend ActiveSupport::Concern def validate_response(model, json, expected_values: {}, ignored_attributes: [], expected_attributes: nil) - ignored_attributes.push :guid + ignored_attributes.push :guid, :options (expected_attributes || expected_attributes_for_model(model)).each do |expected_attribute| # refactor: pass exclusions, and figure out which are valid to not be there next if ignored_attributes.include? expected_attribute diff --git a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb index 29d4005a06d..380d75855a3 100644 --- a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb +++ b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb @@ -58,6 +58,16 @@ module VCAP::CloudController expect(manifest_route.valid?).to be(true) end end + + context 'when there is a valid loadbalancing-algorithm' do + let(:route) { 'http://example.com' } + let(:options) { { 'loadbalancing-algorithm' => 'round-robin' } } + + it 'is valid' do + manifest_route = ManifestRoute.parse(route, options) + expect(manifest_route.valid?).to be(true) + end + end end describe 'invalid routes' do @@ -106,6 +116,16 @@ module VCAP::CloudController expect(manifest_route.valid?).to be(false) end end + + context 'when there is an invalid loadbalancing-algorithm' do + let(:route) { 'http://example.com' } + let(:options) { { 'loadbalancing-algorithm': 'invalid' } } + + it 'is invalid' do + manifest_route = ManifestRoute.parse(route, options) + expect(manifest_route.valid?).to be(false) + end + end end end @@ -119,7 +139,8 @@ module VCAP::CloudController { host: 'host', domain: 'sub.some-domain.com' } ], port: nil, - path: '/path' + path: '/path', + options: nil }) end @@ -131,7 +152,8 @@ module VCAP::CloudController { host: '*', domain: 'sub.some-domain.com' } ], port: nil, - path: '/path' + path: '/path', + options: nil }) end @@ -144,7 +166,8 @@ module VCAP::CloudController { host: 'potato', domain: 'sub.some-domain.com' } ], port: nil, - path: '/path' + path: '/path', + options: nil }) end @@ -157,7 +180,22 @@ module VCAP::CloudController { host: 'potato', domain: 'sub.some-domain.com' } ], port: 1234, - path: '' + path: '', + options: nil + }) + end + + it 'parses a route with a loadbalancing-algorithm into route components' do + route = ManifestRoute.parse('http://potato.sub.some-domain.com', { 'loadbalancing-algorithm': 'round-robin' }) + + expect(route.to_hash).to eq({ + candidate_host_domain_pairs: [ + { host: '', domain: 'potato.sub.some-domain.com' }, + { host: 'potato', domain: 'sub.some-domain.com' } + ], + port: nil, + path: '', + options: { lb_algo: 'round-robin' } }) end end diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index 2f894a81c71..e370b3126d0 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -208,6 +208,56 @@ module VCAP::CloudController expect(msg.errors.full_messages).to include("Route protocol must be 'http1', 'http2' or 'tcp'.") end end + + context 'when a route contains empty route options' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => {} } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains invalid route options' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { 'invalid' => 'invalid' } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(false) + end + end + + context 'when a route contains a valid loadbalancing-algorithm' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing-algorithm' => 'round-robin' + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end end end end diff --git a/spec/unit/messages/route_create_message_spec.rb b/spec/unit/messages/route_create_message_spec.rb index 66bb5c6fb26..d64eb61a456 100644 --- a/spec/unit/messages/route_create_message_spec.rb +++ b/spec/unit/messages/route_create_message_spec.rb @@ -19,6 +19,9 @@ module VCAP::CloudController metadata: { labels: { potato: 'yam' }, annotations: { style: 'mashed' } + }, + options: { + lb_algo: 'round-robin' } } end @@ -354,6 +357,111 @@ module VCAP::CloudController end end end + + context 'options' do + context 'when not provided' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + } + } + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when empty' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: {} + } + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when options has invalid key' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { foobar: 'baz' } + } + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:options]).to include("Unknown field(s): 'foobar'") + end + end + + context 'when lb_algo has value round-robin' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { lb_algo: 'round-robin' } + } + end + + it 'is valid' do + expect(subject).to be_valid + end + + context 'when lb_algo has value least-connections' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { lb_algo: 'least-connections' } + } + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when lb_algo has invalid value' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { lb_algo: 'random' } + } + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:options]).to include("Lb algo 'random' is not a supported load-balancing algorithm") + end + end + end + end end describe 'accessor methods' do diff --git a/spec/unit/messages/route_update_message_spec.rb b/spec/unit/messages/route_update_message_spec.rb index f3a9c5a66f3..18e92f49ac6 100644 --- a/spec/unit/messages/route_update_message_spec.rb +++ b/spec/unit/messages/route_update_message_spec.rb @@ -18,11 +18,43 @@ module VCAP::CloudController expect(message).to be_valid end + it 'accepts options params with round-robin load-balancing algorithm' do + message = RouteUpdateMessage.new(params.merge(options: { lb_algo: 'round-robin' })) + expect(message).to be_valid + end + + it 'accepts options params with least-connections load-balancing algorithm' do + message = RouteUpdateMessage.new(params.merge(options: { lb_algo: 'least-connections' })) + expect(message).to be_valid + end + + it 'accepts options: nil to unset options' do + message = RouteUpdateMessage.new(params.merge(options: nil)) + expect(message).to be_valid + end + + it 'accepts lb_algo: nil to unset load-balancing algorithm' do + message = RouteUpdateMessage.new(params.merge(options: { lb_algo: nil })) + expect(message).to be_valid + end + it 'does not accept any other params' do message = RouteUpdateMessage.new(params.merge(unexpected: 'unexpected_value')) expect(message).not_to be_valid expect(message.errors.full_messages[0]).to include("Unknown field(s): 'unexpected'") end + + it 'does not accept unknown load-balancing algorithm' do + message = RouteUpdateMessage.new(params.merge(options: { lb_algo: 'cheesecake' })) + expect(message).not_to be_valid + expect(message.errors.full_messages[0]).to include("Options Lb algo 'cheesecake' is not a supported load-balancing algorithm") + end + + it 'does not accept unknown option' do + message = RouteUpdateMessage.new(params.merge(options: { gorgonzola: 'gouda' })) + expect(message).not_to be_valid + expect(message.errors.full_messages[0]).to include("Options Unknown field(s): 'gorgonzola'") + end end end end diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 303a5f823c0..093f2638aeb 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -529,6 +529,51 @@ class Relationships < VCAP::CloudController::BaseMessage end end + describe 'OptionsValidator' do + class OptionsMessage < VCAP::CloudController::BaseMessage + register_allowed_keys [:options] + + def options_message + VCAP::CloudController::RouteOptionsMessage.new(options&.deep_symbolize_keys) + end + + validates_with OptionsValidator + end + + it 'successfully validates round-robin load-balancing algorithm' do + message = OptionsMessage.new({ options: { lb_algo: 'round-robin' } }) + expect(message).to be_valid + end + + it 'successfully validates least-connections load-balancing algorithm' do + message = OptionsMessage.new({ options: { lb_algo: 'least-connections' } }) + expect(message).to be_valid + end + + it 'successfully validates empty options' do + message = OptionsMessage.new({ options: {} }) + expect(message).to be_valid + end + + it 'adds invalid object error message when options is not an object' do + message = OptionsMessage.new({ options: 'cheesecake' }) + expect(message).not_to be_valid + expect(message.errors_on(:options)).to include('\'options\' is not a valid object') + end + + it 'adds invalid load balancer error message to the base class' do + message = OptionsMessage.new({ options: { lb_algo: 'donuts' } }) + expect(message).not_to be_valid + expect(message.errors_on(:options)).to include('Lb algo \'donuts\' is not a supported load-balancing algorithm') + end + + it 'adds invalid field error message to the base class' do + message = OptionsMessage.new({ options: { cookies: 'round-robin' } }) + expect(message).not_to be_valid + expect(message.errors_on(:options)).to include('Unknown field(s): \'cookies\'') + end + end + describe 'ToOneRelationshipValidator' do let(:to_one_class) do Class.new(fake_class) do diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index 50e493ee45d..173d4cf1234 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -1049,8 +1049,8 @@ module VCAP::CloudController end describe 'Serialization' do - it { is_expected.to export_attributes :host, :domain_guid, :space_guid, :path, :service_instance_guid, :port } - it { is_expected.to import_attributes :host, :domain_guid, :space_guid, :app_guids, :path, :port } + it { is_expected.to export_attributes :host, :domain_guid, :space_guid, :path, :service_instance_guid, :port, :options } + it { is_expected.to import_attributes :host, :domain_guid, :space_guid, :app_guids, :path, :port, :options } end describe 'instance methods' do diff --git a/spec/unit/presenters/v3/route_presenter_spec.rb b/spec/unit/presenters/v3/route_presenter_spec.rb index c7c91dceea1..313a6b6f9f8 100644 --- a/spec/unit/presenters/v3/route_presenter_spec.rb +++ b/spec/unit/presenters/v3/route_presenter_spec.rb @@ -5,6 +5,7 @@ module VCAP::CloudController::Presenters::V3 RSpec.describe RoutePresenter do let!(:app) { VCAP::CloudController::AppModel.make } let(:space) { VCAP::CloudController::Space.make } + let(:options) { { lb_algo: 'round-robin' } } let(:org) { space.organization } let(:route_host) { 'host' } let(:path) { '/path' } @@ -20,7 +21,8 @@ module VCAP::CloudController::Presenters::V3 host: route_host, path: path, space: space, - domain: domain + domain: domain, + options: options ) end @@ -70,6 +72,7 @@ module VCAP::CloudController::Presenters::V3 expect(subject[:updated_at]).to be_a(Time) expect(subject[:host]).to eq(route_host) expect(subject[:path]).to eq(path) + expect(subject[:options]).to eq('lb_algo' => 'round-robin') expect(subject[:url]).to eq("#{route.host}.#{domain.name}#{route.path}") expected_destinations = [ @@ -125,6 +128,21 @@ module VCAP::CloudController::Presenters::V3 end end + context 'when options is empty' do + let(:route) do + VCAP::CloudController::Route.make( + host: 'foobar', + path: path, + space: space, + domain: domain + ) + end + + it 'does not output options' do + expect(subject[:options]).to be_nil + end + end + context 'when there are decorators' do let(:banana_decorator) do Class.new do From 9b3ea30c083b8a5b426302750d6ec9b39fbfaa8d Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 13 Nov 2024 13:35:48 +0100 Subject: [PATCH 2/7] Add route-options documentation Add documentation for the route options object, and its supported fields. --- .../source/includes/api_resources/_routes.erb | 12 +++++++ .../includes/resources/routes/_create.md.erb | 26 ++++++++------- .../includes/resources/routes/_object.md.erb | 33 ++++++++++--------- .../routes/_route_options_object.md.erb | 12 +++++++ .../includes/resources/routes/_update.md.erb | 14 +++++--- docs/v3/source/index.html.md | 1 + 6 files changed, 66 insertions(+), 32 deletions(-) create mode 100644 docs/v3/source/includes/resources/routes/_route_options_object.md.erb diff --git a/docs/v3/source/includes/api_resources/_routes.erb b/docs/v3/source/includes/api_resources/_routes.erb index 3782b4a124f..c1448303e35 100644 --- a/docs/v3/source/includes/api_resources/_routes.erb +++ b/docs/v3/source/includes/api_resources/_routes.erb @@ -34,6 +34,9 @@ "protocol": "tcp" } ], + "options": { + "lb_algo": "round-robin" + }, "metadata": { "labels": <%= metadata.fetch(:labels, {}).to_json(space: ' ', object_nl: ' ')%>, "annotations": <%= metadata.fetch(:annotations, {}).to_json(space: ' ', object_nl: ' ')%> @@ -124,6 +127,9 @@ "protocol": "http1" } ], + "options": { + "lb_algo": "round-robin" + }, "metadata": { "labels": {}, "annotations": {} @@ -212,3 +218,9 @@ "protocol": "http2" } <% end %> + +<% content_for :route_options do %> + { + "lb_algo": "round-robin" + } +<% end %> diff --git a/docs/v3/source/includes/resources/routes/_create.md.erb b/docs/v3/source/includes/resources/routes/_create.md.erb index e6e28d87019..d0315ea03e1 100644 --- a/docs/v3/source/includes/resources/routes/_create.md.erb +++ b/docs/v3/source/includes/resources/routes/_create.md.erb @@ -21,6 +21,9 @@ curl "https://api.example.org/v3/routes" \ "data": { "guid": "space-guid" } } }, + "options": { + "lb_algo": "round-robin" + } "metadata": { "labels": { "key": "value" }, "annotations": { "note": "detailed information"} @@ -44,20 +47,21 @@ Content-Type: application/json #### Required parameters -| Name | Type | Description | -| ----------------------------------------- | ---------------------------------------------- | ---------------------------------------------------------------------------------------------------------- | -| **relationships.space** | [_to-one relationship_](#to-one-relationships) | A relationship to the space containing the route; routes can only be mapped to destinations in that space | -| **relationships.domain** | [_to-one relationship_](#to-one-relationships) | A relationship to the domain of the route | +| Name | Type | Description | +|--------------------------|------------------------------------------------|-----------------------------------------------------------------------------------------------------------| +| **relationships.space** | [_to-one relationship_](#to-one-relationships) | A relationship to the space containing the route; routes can only be mapped to destinations in that space | +| **relationships.domain** | [_to-one relationship_](#to-one-relationships) | A relationship to the domain of the route | #### Optional parameters -| Name | Type | Description | -| ------------------------ | ----------------------------------- | --------------------------------- | -| **host** | _string_ | The host component for the route; not compatible with routes specifying the `tcp` protocol | -| **path** | _string_ | The path component for the route; should begin with a `/` and not compatible with routes specifying the `tcp` protocol | -| **port** | _integer_ | The port the route will listen on; only compatible with routes leveraging a domain that supports the `tcp` protocol. For `tcp` domains, a port will be randomly assigned if not specified -| **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the route | -| **metadata.labels** | [_label object_](#labels) | Labels applied to the route | +| Name | Type | Description | +|--------------------------|----------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| **host** | _string_ | The host component for the route; not compatible with routes specifying the `tcp` protocol | +| **path** | _string_ | The path component for the route; should begin with a `/` and not compatible with routes specifying the `tcp` protocol | +| **port** | _integer_ | The port the route will listen on; only compatible with routes leveraging a domain that supports the `tcp` protocol. For `tcp` domains, a port will be randomly assigned if not specified | +| **options** | [_route option object_](#the-route-options-object) | Options applied to the route | +| **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the route | +| **metadata.labels** | [_label object_](#labels) | Labels applied to the route | #### Permitted roles diff --git a/docs/v3/source/includes/resources/routes/_object.md.erb b/docs/v3/source/includes/resources/routes/_object.md.erb index 4809097f531..a0de69a1675 100644 --- a/docs/v3/source/includes/resources/routes/_object.md.erb +++ b/docs/v3/source/includes/resources/routes/_object.md.erb @@ -8,19 +8,20 @@ Example Route object <%= yield_content :single_route %> ``` -| Name | Type | Description -| -------------- | ------------------------ | ------------------------------------------------------ -| **guid** | _uuid_ | Unique identifier for the route -| **created_at** | _[timestamp](#timestamps)_ | The time with zone when the object was created -| **updated_at** | _[timestamp](#timestamps)_ | The time with zone when the object was last updated -| **protocol** | _string_ | The protocol supported by the route, based on the route's domain configuration. Valid protocols are `http` and `tcp` -| **host** | _string_ | The hostname for the route; not compatible with routes specifying the `tcp` protocol;
must be either a wildcard (*) or be under 63 characters long and only contain letters, numbers, dashes (-) or underscores(_) -| **path** | _string_ | The path for the route; not compatible with routes specifying the `tcp` protocol;
must be under 128 characters long and not contain question marks (?), begin with a slash (/) and not be exactly a slash (/). Must conform to path components from [RFC 2396](https://tools.ietf.org/html/rfc2396) -| **port** | _integer or `null`_ | The port that the route listens on. Only compatible with routes specifying the `tcp` protocol -| **url** | _string_ | The URL for the route; URLs of protocol `http` are a combination of host, domain name, and path; URLs of protocol `tcp` are a combination of domain name and port -| **destinations** | array of [_destination objects_](#the-destination-object) | List of destinations for the route -| **relationships.space** | [_to-one relationship_](#to-one-relationships) | A relationship to the space containing the route. Routes can only be mapped to destinations in that space -| **relationships.domain** | [_to-one relationship_](#to-one-relationships) | A relationship to the domain of the route -| **metadata.labels** | [_label object_](#labels) | Labels applied to the route -| **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the route -| **links** | [_links object_](#links) | Links to related resources +| Name | Type | Description | +|--------------------------|-----------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| **guid** | _uuid_ | Unique identifier for the route | +| **created_at** | _[timestamp](#timestamps)_ | The time with zone when the object was created | +| **updated_at** | _[timestamp](#timestamps)_ | The time with zone when the object was last updated | +| **protocol** | _string_ | The protocol supported by the route, based on the route's domain configuration. Valid protocols are `http` and `tcp` | +| **host** | _string_ | The hostname for the route; not compatible with routes specifying the `tcp` protocol;
must be either a wildcard (*) or be under 63 characters long and only contain letters, numbers, dashes (-) or underscores(_) | +| **path** | _string_ | The path for the route; not compatible with routes specifying the `tcp` protocol;
must be under 128 characters long and not contain question marks (?), begin with a slash (/) and not be exactly a slash (/). Must conform to path components from [RFC 2396](https://tools.ietf.org/html/rfc2396) | +| **port** | _integer or `null`_ | The port that the route listens on. Only compatible with routes specifying the `tcp` protocol | +| **url** | _string_ | The URL for the route; URLs of protocol `http` are a combination of host, domain name, and path; URLs of protocol `tcp` are a combination of domain name and port | +| **destinations** | array of [_destination objects_](#the-destination-object) | List of destinations for the route | +| **relationships.space** | [_to-one relationship_](#to-one-relationships) | A relationship to the space containing the route. Routes can only be mapped to destinations in that space | +| **relationships.domain** | [_to-one relationship_](#to-one-relationships) | A relationship to the domain of the route | +| **options** | [_route option object_](#the-route-options-object) | Options applied to the route | +| **metadata.labels** | [_label object_](#labels) | Labels applied to the route | +| **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the route | +| **links** | [_links object_](#links) | Links to related resources | diff --git a/docs/v3/source/includes/resources/routes/_route_options_object.md.erb b/docs/v3/source/includes/resources/routes/_route_options_object.md.erb new file mode 100644 index 00000000000..c891ad9ee47 --- /dev/null +++ b/docs/v3/source/includes/resources/routes/_route_options_object.md.erb @@ -0,0 +1,12 @@ +### The route options object + +``` +Example Route-Options object +``` +```json +<%= yield_content :route_options %> +``` + +| Name | Type | Description | +|-------------|----------|----------------------------------------------------------------------------------------------------------| +| **lb_algo** | _string_ | The load balancer associated with this route. Valid algorithms are 'round-robin' and 'least-connections' | diff --git a/docs/v3/source/includes/resources/routes/_update.md.erb b/docs/v3/source/includes/resources/routes/_update.md.erb index d1fa9d5ea79..4814a23d171 100644 --- a/docs/v3/source/includes/resources/routes/_update.md.erb +++ b/docs/v3/source/includes/resources/routes/_update.md.erb @@ -10,6 +10,9 @@ curl "https://api.example.org/v3/routes/[guid]" \ -H "Authorization: bearer [token]" \ -H "Content-type: application/json" \ -d '{ + "options": { + "lb_algo": "round-robin" + } "metadata": { "labels": {"key": "value"}, "annotations": {"note": "detailed information"} @@ -33,14 +36,15 @@ Content-Type: application/json #### Optional parameters -| Name | Type | Description | Default | -| ----------- | -------- | ------------------------------------------------------------------------ | ------- | -| **metadata.labels** | [_label object_](#labels) | Labels applied to the route | | -| **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the route | | +| Name | Type | Description | Default | +|----------------------------|-----------------------------------------------------|----------------------------------|---------| +| **options** | [_route option object_](#the-route-options-object) | Options applied to the route | | +| **metadata.labels** | [_label object_](#labels) | Labels applied to the route | | +| **metadata.annotations** | [_annotation object_](#annotations) | Annotations applied to the route | | #### Permitted roles - | +| ----- | Admin | Space Developer | diff --git a/docs/v3/source/index.html.md b/docs/v3/source/index.html.md index cc60bfea116..03c63aecaca 100644 --- a/docs/v3/source/index.html.md +++ b/docs/v3/source/index.html.md @@ -243,6 +243,7 @@ includes: - resources/routes/header - resources/routes/object - resources/routes/destination_object + - resources/routes/route_options_object - resources/routes/create - resources/routes/get - resources/routes/list From 848c83d76977ebf690b65c8a17619c15fd5aa0da Mon Sep 17 00:00:00 2001 From: Alexander Nicke Date: Tue, 19 Nov 2024 12:35:13 +0100 Subject: [PATCH 3/7] Adjust route options behaviour for manifest push Overwrite behaviour for route options is now fixed and tested: Existing options are not modified if options is nil, {} or not provided A single option (e.g. loadbalancing-algorithm) can be removed by setting its value to nil adjust test for manifest push: options {key:nil} should not modify the existing value --- app/actions/manifest_route_update.rb | 4 +- app/actions/route_update.rb | 1 + .../app_manifest/manifest_route.rb | 2 +- spec/request/space_manifests_spec.rb | 180 +++++++++++++----- spec/unit/actions/route_update_spec.rb | 101 +++++++++- spec/unit/messages/validators_spec.rb | 2 +- 6 files changed, 242 insertions(+), 48 deletions(-) diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index 50830da16d3..25f3e08ca80 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -92,7 +92,9 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) domain: existing_domain, manifest_triggered: true ) - elsif route[:options] != manifest_route[:options] + elsif manifest_route[:options] && route[:options] != manifest_route[:options] + # remove nil values from options + manifest_route[:options] = manifest_route[:options].compact message = RouteUpdateMessage.new({ 'options' => manifest_route[:options] }) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 91a1821a4a2..1b6be7fe470 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -11,6 +11,7 @@ def update(route:, message:) route.options.merge(message.options) end end + route.save MetadataUpdate.update(route, message) end diff --git a/lib/cloud_controller/app_manifest/manifest_route.rb b/lib/cloud_controller/app_manifest/manifest_route.rb index 6a3cab14c90..e384fc653f1 100644 --- a/lib/cloud_controller/app_manifest/manifest_route.rb +++ b/lib/cloud_controller/app_manifest/manifest_route.rb @@ -19,7 +19,7 @@ def self.parse(route, options=nil) if options attrs[:options] = {} - attrs[:options][:lb_algo] = options[:'loadbalancing-algorithm'] if options[:'loadbalancing-algorithm'] + attrs[:options][:lb_algo] = options[:'loadbalancing-algorithm'] if options.key?(:'loadbalancing-algorithm') end ManifestRoute.new(attrs) diff --git a/spec/request/space_manifests_spec.rb b/spec/request/space_manifests_spec.rb index d3f21eaa922..28ee8b8856a 100644 --- a/spec/request/space_manifests_spec.rb +++ b/spec/request/space_manifests_spec.rb @@ -606,7 +606,7 @@ before do app1_model.web_processes.first.update(state: VCAP::CloudController::ProcessModel::STARTED, instances: 4) space.update(space_quota_definition: - VCAP::CloudController::SpaceQuotaDefinition.make(organization: space.organization, log_rate_limit: 0)) + VCAP::CloudController::SpaceQuotaDefinition.make(organization: space.organization, log_rate_limit: 0)) end it 'successfully applies the manifest' do @@ -625,7 +625,7 @@ end describe 'route options' do - context 'when an empty route options hash is provided' do + context 'when an invalid route option is provided' do let(:yml_manifest) do { 'applications' => [ @@ -633,46 +633,163 @@ 'name' => app1_model.name, 'routes' => [ { 'route' => "https://#{route.host}.#{route.domain.name}", - 'options' => {} } + 'options' => { + 'doesnt-exist' => 'doesnt-exist' + } } ] } ] }.to_yaml end - it 'applies the manifest' do + it 'returns a 422' do post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) - expect(last_response.status).to eq(202) + expect(last_response).to have_status_code(422) + expect(last_response).to have_error_message('Routes contains invalid route options') end end - context 'when an invalid route option is provided' do - let(:yml_manifest) do - { + context 'updating existing route options' do + # using loadbalancing-algorithm as an example since it is the only route option currently supported + before do + yml_manifest = { 'applications' => [ - { - 'name' => app1_model.name, + { 'name' => app1_model.name, 'routes' => [ - { 'route' => "https://#{route.host}.#{route.domain.name}", + { 'route' => "https://round-robin-app.#{shared_domain.name}", 'options' => { - 'doesnt-exist' => 'doesnt-exist' + 'loadbalancing-algorithm' => 'round-robin' } } - ] - } + ] } ] }.to_yaml + + # apply the manifest with the route option + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response.status).to eq(202) + job_guid = VCAP::CloudController::PollableJobModel.last.guid + + Delayed::Worker.new.work_off + expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error + app1_model.reload + expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) end - it 'returns a 422' do + it 'updates the route option when a new value is provided' do + yml_manifest = { + 'applications' => [ + { 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://round-robin-app.#{shared_domain.name}", + 'options' => { + 'loadbalancing-algorithm' => 'least-connections' + } } + ] } + ] + }.to_yaml + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) - expect(last_response).to have_status_code(422) - expect(last_response).to have_error_message('Routes contains invalid route options') + expect(last_response.status).to eq(202) + job_guid = VCAP::CloudController::PollableJobModel.last.guid + + Delayed::Worker.new.work_off + expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error + app1_model.reload + expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'least-connections' }) + end + + it 'does not modify any route options when the options hash is not provided' do + yml_manifest = { + 'applications' => [ + { 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://round-robin-app.#{shared_domain.name}" } + ] } + ] + }.to_yaml + + # apply the manifest with the route option + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response.status).to eq(202) + job_guid = VCAP::CloudController::PollableJobModel.last.guid + + Delayed::Worker.new.work_off + expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error + app1_model.reload + expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + end + + it 'does not modify any route options options: nil is provided' do + yml_manifest = { + 'applications' => [ + { 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://round-robin-app.#{shared_domain.name}", + 'options' => nil } + ] } + ] + }.to_yaml + + # apply the manifest with the route option + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response.status).to eq(202) + job_guid = VCAP::CloudController::PollableJobModel.last.guid + + Delayed::Worker.new.work_off + expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error + app1_model.reload + expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + end + + it 'does not modify any route options if an empty options hash is provided' do + yml_manifest = { + 'applications' => [ + { 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://round-robin-app.#{shared_domain.name}", + 'options' => {} } + ] } + ] + }.to_yaml + + # apply the manifest with the route option + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response.status).to eq(202) + + app1_model.reload + expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + end + + it 'does not modify any option when options: { key: nil } is provided' do + yml_manifest = { + 'applications' => [ + { 'name' => app1_model.name, + 'routes' => [ + { 'route' => "https://round-robin-app.#{shared_domain.name}", + 'options' => { + 'loadbalancing-algorithm' => nil + } } + ] } + ] + }.to_yaml + + # apply the manifest with the route option + post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) + + expect(last_response.status).to eq(202) + + app1_model.reload + expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) end end - context 'loadbalancing-algorithm' do + context 'route-option: loadbalancing-algorithm' do context 'when the loadbalancing-algorithm is not supported' do let(:yml_manifest) do { @@ -713,7 +830,7 @@ }.to_yaml end - it 'adds and updates the loadbalancing-algorithm' do + it 'adds the loadbalancing-algorithm' do post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) expect(last_response.status).to eq(202) @@ -724,31 +841,6 @@ app1_model.reload expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) - - ### update the loadbalancing-algorithm from the route - - yml_manifest = { - 'applications' => [ - { 'name' => app1_model.name, - 'routes' => [ - { 'route' => "https://round-robin-app.#{shared_domain.name}", - 'options' => { - 'loadbalancing-algorithm' => 'least-connections' - } } - ] } - ] - }.to_yaml - - post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) - - expect(last_response.status).to eq(202) - job_guid = VCAP::CloudController::PollableJobModel.last.guid - - Delayed::Worker.new.work_off - expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error - - app1_model.reload - expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'least-connections' }) end end end diff --git a/spec/unit/actions/route_update_spec.rb b/spec/unit/actions/route_update_spec.rb index 0e6464518c4..7ad61f3f812 100644 --- a/spec/unit/actions/route_update_spec.rb +++ b/spec/unit/actions/route_update_spec.rb @@ -16,6 +16,9 @@ module VCAP::CloudController beet: 'formanova' } end + let(:old_options) do + '{"lb_algo": "round-robin"}' + end let(:new_labels) do { cuisine: 'thai', @@ -33,6 +36,9 @@ module VCAP::CloudController metadata: { labels: new_labels, annotations: new_annotations + }, + options: { + lb_algo: 'round-robin' } } end @@ -41,7 +47,96 @@ module VCAP::CloudController let(:route) { Route.make } subject { RouteUpdate.new } - describe '#update' do + describe '#update metadata' do + context 'when the route has no existing metadata' do + context 'when no metadata is specified' do + let(:body) do + {} + end + + it 'adds no metadata' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.labels.size).to eq(0) + expect(route.annotations.size).to eq(0) + end + end + + context 'when metadata is specified' do + it 'updates the route metadata' do + expect(message).to be_valid + subject.update(route:, message:) + + route.reload + expect(route).to have_labels( + { prefix: 'doordash.com', key_name: 'potato', value: 'mashed' }, + { prefix: nil, key_name: 'fruit', value: 'strawberries' }, + { prefix: nil, key_name: 'cuisine', value: 'thai' } + ) + expect(route).to have_annotations( + { key_name: 'potato', value: 'idaho' } + ) + end + end + end + + context 'when the route has existing metadata' do + before do + VCAP::CloudController::LabelsUpdate.update(route, old_labels, VCAP::CloudController::RouteLabelModel) + VCAP::CloudController::AnnotationsUpdate.update(route, old_annotations, VCAP::CloudController::RouteAnnotationModel) + end + + context 'when no metadata is specified' do + let(:body) do + {} + end + + it 'adds no metadata' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route).to have_labels( + { prefix: nil, key_name: 'fruit', value: 'peach' }, + { prefix: nil, key_name: 'clothing', value: 'blouse' } + ) + expect(route).to have_annotations( + { key_name: 'potato', value: 'celandine' }, + { key_name: 'beet', value: 'formanova' } + ) + end + end + + context 'when metadata is specified' do + let(:body) do + { + metadata: { + labels: new_labels.merge(fruit: nil, newstuff: 'here'), + annotations: new_annotations.merge(beet: nil, asparagus: 'crunchy') + } + } + end + + it 'updates some, deletes nils, leaves unspecified fields alone' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + + expect(route).to have_labels( + { prefix: 'doordash.com', key_name: 'potato', value: 'mashed' }, + { prefix: nil, key_name: 'clothing', value: 'blouse' }, + { prefix: nil, key_name: 'newstuff', value: 'here' }, + { prefix: nil, key_name: 'cuisine', value: 'thai' } + ) + expect(route).to have_annotations( + { key_name: 'potato', value: 'idaho' }, + { key_name: 'asparagus', value: 'crunchy' } + ) + end + end + end + end + describe '#update options' do context 'when the route has no existing metadata' do context 'when no metadata is specified' do let(:body) do @@ -71,6 +166,7 @@ module VCAP::CloudController expect(route).to have_annotations( { key_name: 'potato', value: 'idaho' } ) + expect(route[:options]).to eq('{"lb_algo":"round-robin"}') end end end @@ -79,6 +175,7 @@ module VCAP::CloudController before do VCAP::CloudController::LabelsUpdate.update(route, old_labels, VCAP::CloudController::RouteLabelModel) VCAP::CloudController::AnnotationsUpdate.update(route, old_annotations, VCAP::CloudController::RouteAnnotationModel) + route[:options] = '{"lb_algo": "round-robin"}' end context 'when no metadata is specified' do @@ -98,6 +195,7 @@ module VCAP::CloudController { key_name: 'potato', value: 'celandine' }, { key_name: 'beet', value: 'formanova' } ) + expect(route.options).to include({ 'lb_algo' => 'round-robin' }) end end @@ -130,5 +228,6 @@ module VCAP::CloudController end end end + end end diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 093f2638aeb..931532574c9 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -558,7 +558,7 @@ def options_message it 'adds invalid object error message when options is not an object' do message = OptionsMessage.new({ options: 'cheesecake' }) expect(message).not_to be_valid - expect(message.errors_on(:options)).to include('\'options\' is not a valid object') + expect(message.errors_on(:options)).to include("'options' is not a valid object") end it 'adds invalid load balancer error message to the base class' do From 1d058ddfdde65f6bcdb0751cc3241231b5334c15 Mon Sep 17 00:00:00 2001 From: Alexander Nicke Date: Tue, 26 Nov 2024 16:52:05 +0100 Subject: [PATCH 4/7] Adjust behaviour to new decisions: options default: {} API: options is not nullable specific option is additive specific option is nullable empty hash does not change anything (additive) get empty options -> {} manifest: options and specific option is nullable, but no-op --- app/actions/manifest_route_update.rb | 5 +- app/actions/route_create.rb | 2 +- app/actions/route_update.rb | 6 +- .../manifest_routes_update_message.rb | 31 ++++--- app/messages/route_create_message.rb | 6 +- app/messages/route_options_message.rb | 2 +- app/messages/validators.rb | 3 - .../20241105000000_add_options_to_route.rb | 4 +- .../app_manifest/manifest_route.rb | 8 +- spec/request/app_manifests_spec.rb | 18 ++-- spec/request/routes_spec.rb | 50 ++++++++--- spec/request/space_manifests_spec.rb | 6 +- spec/unit/actions/route_update_spec.rb | 87 +++++++++---------- spec/unit/jobs/deserialization_spec.rb | 1 + .../app_manifest/manifest_route_spec.rb | 8 +- .../diego/protocol/routing_info_spec.rb | 38 ++++---- .../manifest_routes_update_message_spec.rb | 21 +++++ .../messages/route_create_message_spec.rb | 35 ++++++++ .../messages/route_update_message_spec.rb | 10 ++- spec/unit/messages/validators_spec.rb | 11 +++ .../v3/app_manifest_presenter_spec.rb | 4 +- .../presenters/v3/route_presenter_spec.rb | 4 +- 22 files changed, 228 insertions(+), 132 deletions(-) diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index 25f3e08ca80..a6fbc2af9fa 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -92,6 +92,8 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) domain: existing_domain, manifest_triggered: true ) + elsif route.space.guid != app.space_guid + raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces') elsif manifest_route[:options] && route[:options] != manifest_route[:options] # remove nil values from options manifest_route[:options] = manifest_route[:options].compact @@ -99,9 +101,6 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) 'options' => manifest_route[:options] }) route = RouteUpdate.new.update(route:, message:) - - elsif route.space.guid != app.space_guid - raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces') end return route diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index bac1c489739..15be9568456 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -16,7 +16,7 @@ def create(message:, space:, domain:, manifest_triggered: false) port: port(message, domain), space: space, domain: domain, - options: message.options + options: message.options.nil? ? {} : message.options.compact ) Route.db.transaction do diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 1b6be7fe470..e11d34deba8 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -4,11 +4,9 @@ def update(route:, message:) Route.db.transaction do if message.requested?(:options) route.options = if message.options.nil? - nil - elsif route.options.nil? - message.options + {} else - route.options.merge(message.options) + route.options.symbolize_keys.merge(message.options).compact end end diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 00923bee92d..8b9cfe0a39d 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -13,14 +13,8 @@ def validate(record) return end - if contains_invalid_route_options?(record.routes) - record.errors.add(:routes, message: 'contains invalid route options') - return - end - - return unless contains_invalid_lb_algo?(record.routes) - - record.errors.add(:routes, message: 'contains an invalid loadbalancing-algorithm option') + contains_invalid_route_options?(record) + contains_invalid_lb_algo?(record) nil end @@ -32,7 +26,8 @@ def contains_non_route_hash_values?(routes) routes.any? { |r| !(r.is_a?(Hash) && r[:route].present?) } end - def contains_invalid_route_options?(routes) + def contains_invalid_route_options?(record) + routes = record.routes routes.any? do |r| next unless r[:options] @@ -40,15 +35,25 @@ def contains_invalid_route_options?(routes) return false if r[:options].empty? - return r[:options].keys.all? { |key| RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) } + r[:options].each_key do |key| + RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) && + record.errors.add(:base, + message: "Route '#{r[:route]}' contains invalid route option '#{key}'. \ +Valid keys: '#{RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.join(', ')}'") + end end end - def contains_invalid_lb_algo?(routes) - routes.any? do |r| + def contains_invalid_lb_algo?(record) + routes = record.routes + routes.each do |r| next unless r[:options] && r[:options][:'loadbalancing-algorithm'] - return true if r[:options][:'loadbalancing-algorithm'] && RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(r[:options][:'loadbalancing-algorithm']) + lb_algo = r[:options][:'loadbalancing-algorithm'] + RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(lb_algo) && + record.errors.add(:base, + message: "Route '#{r[:route]}' contains invalid load-balancing algorithm '#{lb_algo}'. \ +Valid algorithms: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'") end end end diff --git a/app/messages/route_create_message.rb b/app/messages/route_create_message.rb index 12a1eda8581..6605ded1a3c 100644 --- a/app/messages/route_create_message.rb +++ b/app/messages/route_create_message.rb @@ -14,6 +14,10 @@ class RouteCreateMessage < MetadataBaseMessage options ] + def self.options_requested? + @options_requested ||= proc { |a| a.requested?(:options) } + end + validates :host, allow_nil: true, string: true, @@ -58,7 +62,7 @@ class RouteCreateMessage < MetadataBaseMessage validates_with NoAdditionalKeysValidator validates_with RelationshipValidator - validates_with OptionsValidator + validates_with OptionsValidator, if: options_requested? delegate :space_guid, to: :relationships_message delegate :domain_guid, to: :relationships_message diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 614eccba7f7..c40d278db36 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -6,7 +6,7 @@ class RouteOptionsMessage < BaseMessage VALID_ROUTE_OPTIONS = %i[lb_algo].freeze VALID_LOADBALANCING_ALGORITHMS = %w[round-robin least-connections].freeze - register_allowed_keys %i[lb_algo] + register_allowed_keys VALID_ROUTE_OPTIONS validates_with NoAdditionalKeysValidator validates :lb_algo, inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "'%s' is not a supported load-balancing algorithm" }, diff --git a/app/messages/validators.rb b/app/messages/validators.rb index c9baf7e83a2..48d4d3bdf61 100644 --- a/app/messages/validators.rb +++ b/app/messages/validators.rb @@ -242,9 +242,6 @@ def validate(record) class OptionsValidator < ActiveModel::Validator def validate(record) - # Empty option hashes are allowed, so we skip further validation - record.options.blank? && return - unless record.options.is_a?(Hash) record.errors.add(:options, message: "'options' is not a valid object") return diff --git a/db/migrations/20241105000000_add_options_to_route.rb b/db/migrations/20241105000000_add_options_to_route.rb index 94b75933ced..c9e21eee170 100644 --- a/db/migrations/20241105000000_add_options_to_route.rb +++ b/db/migrations/20241105000000_add_options_to_route.rb @@ -1,9 +1,7 @@ Sequel.migration do up do alter_table(:routes) do - # rubocop:disable Migration/IncludeStringSize - add_column :options, String, text: true, default: nil - # rubocop:enable Migration/IncludeStringSize + add_column :options, String, size: 4096, default: '{}' end end down do diff --git a/lib/cloud_controller/app_manifest/manifest_route.rb b/lib/cloud_controller/app_manifest/manifest_route.rb index e384fc653f1..9c17fc08a94 100644 --- a/lib/cloud_controller/app_manifest/manifest_route.rb +++ b/lib/cloud_controller/app_manifest/manifest_route.rb @@ -16,11 +16,8 @@ def self.parse(route, options=nil) end attrs[:full_route] = route - - if options - attrs[:options] = {} - attrs[:options][:lb_algo] = options[:'loadbalancing-algorithm'] if options.key?(:'loadbalancing-algorithm') - end + attrs[:options] = {} + attrs[:options][:lb_algo] = options[:'loadbalancing-algorithm'] if options && options.key?(:'loadbalancing-algorithm') ManifestRoute.new(attrs) end @@ -28,6 +25,7 @@ def self.parse(route, options=nil) def valid? if @attrs[:options] && !@attrs[:options].empty? return false if @attrs[:options].keys.any? { |key| RouteOptionsMessage::VALID_ROUTE_OPTIONS.exclude?(key) } + # validation for loadbalancing algorithm return false if @attrs[:options][:lb_algo] && RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(@attrs[:options][:lb_algo]) end diff --git a/spec/request/app_manifests_spec.rb b/spec/request/app_manifests_spec.rb index 76c3d7ffca5..de384cb3208 100644 --- a/spec/request/app_manifests_spec.rb +++ b/spec/request/app_manifests_spec.rb @@ -97,11 +97,13 @@ 'routes' => [ { 'route' => "#{route.host}.#{route.domain.name}", - 'protocol' => 'http1' + 'protocol' => 'http1', + 'options' => {} }, { 'route' => "#{second_route.host}.#{second_route.domain.name}/path", - 'protocol' => 'http1' + 'protocol' => 'http1', + 'options' => {} } ], 'metadata' => { 'labels' => { 'potato' => 'idaho' }, 'annotations' => { 'style' => 'mashed' } }, @@ -201,11 +203,13 @@ 'routes' => [ { 'route' => "#{route.host}.#{route.domain.name}", - 'protocol' => 'http1' + 'protocol' => 'http1', + 'options' => {} }, { 'route' => "#{second_route.host}.#{second_route.domain.name}/path", - 'protocol' => 'http1' + 'protocol' => 'http1', + 'options' => {} } ], 'metadata' => { 'labels' => { 'potato' => 'idaho' }, 'annotations' => { 'style' => 'mashed' } }, @@ -273,11 +277,13 @@ 'routes' => [ { 'route' => "#{route.host}.#{route.domain.name}", - 'protocol' => 'http2' + 'protocol' => 'http2', + 'options' => {} }, { 'route' => "#{second_route.host}.#{second_route.domain.name}/path", - 'protocol' => 'http1' + 'protocol' => 'http1', + 'options' => {} } ] } diff --git a/spec/request/routes_spec.rb b/spec/request/routes_spec.rb index 82d154160e8..5a22276e405 100644 --- a/spec/request/routes_spec.rb +++ b/spec/request/routes_spec.rb @@ -89,6 +89,7 @@ labels: {}, annotations: {} }, + options: {}, links: { self: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{route_in_org.guid}} }, space: { href: %r{#{Regexp.escape(link_prefix)}/v3/spaces/#{route_in_org.space.guid}} }, @@ -121,6 +122,7 @@ labels: {}, annotations: {} }, + options: {}, links: { self: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{route_in_other_org.guid}} }, space: { href: %r{#{Regexp.escape(link_prefix)}/v3/spaces/#{route_in_other_org.space.guid}} }, @@ -305,6 +307,7 @@ } } }, + options: {}, links: { self: { href: "http://api2.vcap.me/v3/routes/#{route1_domain1.guid}" }, space: { href: "http://api2.vcap.me/v3/spaces/#{space.guid}" }, @@ -416,6 +419,7 @@ } } }, + options: {}, links: { self: { href: 'http://api2.vcap.me/v3/routes/route-without-host' }, space: { href: "http://api2.vcap.me/v3/spaces/#{space.guid}" }, @@ -451,6 +455,7 @@ } } }, + options: {}, links: { self: { href: 'http://api2.vcap.me/v3/routes/route-without-host2' }, space: { href: "http://api2.vcap.me/v3/spaces/#{space.guid}" }, @@ -489,6 +494,7 @@ } } }, + options: {}, links: { self: { href: 'http://api2.vcap.me/v3/routes/route-without-path' }, space: { href: "http://api2.vcap.me/v3/spaces/#{space.guid}" }, @@ -977,6 +983,7 @@ labels: {}, annotations: {} }, + options: {}, links: { self: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{UUID_REGEX}} }, space: { href: %r{#{Regexp.escape(link_prefix)}/v3/spaces/#{route.space.guid}} }, @@ -1062,6 +1069,7 @@ labels: {}, annotations: {} }, + options: {}, links: { self: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{UUID_REGEX}} }, space: { href: %r{#{Regexp.escape(link_prefix)}/v3/spaces/#{route.space.guid}} }, @@ -1213,7 +1221,8 @@ metadata: { labels: {}, annotations: {} - } + }, + options: {} } end @@ -1259,7 +1268,8 @@ metadata: { labels: { potato: 'yam' }, annotations: { style: 'mashed' } - } + }, + options: {} } end @@ -1291,7 +1301,8 @@ metadata: { labels: { potato: 'yam' }, annotations: { style: 'mashed' } - } + }, + options: {} } end @@ -1376,7 +1387,8 @@ metadata: { labels: {}, annotations: {} - } + }, + options: {} } end @@ -1474,7 +1486,8 @@ metadata: { labels: {}, annotations: {} - } + }, + options: {} } end @@ -1547,7 +1560,8 @@ metadata: { labels: {}, annotations: {} - } + }, + options: {} } end @@ -1634,7 +1648,8 @@ metadata: { labels: {}, annotations: {} - } + }, + options: {} } end @@ -1774,7 +1789,8 @@ space: { href: %r{#{Regexp.escape(link_prefix)}/v3/spaces/#{space.guid}} }, destinations: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{UUID_REGEX}/destinations} }, domain: { href: %r{#{Regexp.escape(link_prefix)}/v3/domains/#{domain.guid}} } - } + }, + options: {} } end @@ -1886,7 +1902,8 @@ metadata: { labels: {}, annotations: {} - } + }, + options: {} } end @@ -2119,7 +2136,8 @@ space: { href: %r{#{Regexp.escape(link_prefix)}/v3/spaces/#{space.guid}} }, destinations: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{UUID_REGEX}/destinations} }, domain: { href: %r{#{Regexp.escape(link_prefix)}/v3/domains/#{domain.guid}} } - } + }, + options: {} } end @@ -2435,7 +2453,8 @@ potato: 'russet', style: 'fried' } - } + }, + options: {} } end @@ -2506,7 +2525,8 @@ potato: 'russet', style: 'fried' } - } + }, + options: {} } end let(:expected_codes_and_responses) do @@ -3606,7 +3626,8 @@ space: { href: %r{#{Regexp.escape(link_prefix)}/v3/spaces/#{route1.space.guid}} }, destinations: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{route1.guid}/destinations} }, domain: { href: %r{#{Regexp.escape(link_prefix)}/v3/domains/#{route1.domain.guid}} } - } + }, + options: {} } end @@ -3649,7 +3670,8 @@ space: { href: %r{#{Regexp.escape(link_prefix)}/v3/spaces/#{route2.space.guid}} }, destinations: { href: %r{#{Regexp.escape(link_prefix)}/v3/routes/#{route2.guid}/destinations} }, domain: { href: %r{#{Regexp.escape(link_prefix)}/v3/domains/#{route2.domain.guid}} } - } + }, + options: {} } end diff --git a/spec/request/space_manifests_spec.rb b/spec/request/space_manifests_spec.rb index 28ee8b8856a..77f8869c8e0 100644 --- a/spec/request/space_manifests_spec.rb +++ b/spec/request/space_manifests_spec.rb @@ -646,7 +646,8 @@ post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) expect(last_response).to have_status_code(422) - expect(last_response).to have_error_message('Routes contains invalid route options') + expect(last_response).to have_error_message("For application '#{app1_model.name}': \ +Route 'https://#{route.host}.#{route.domain.name}' contains invalid route option 'doesnt-exist'. Valid keys: 'loadbalancing-algorithm'") end end @@ -811,7 +812,8 @@ post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) expect(last_response).to have_status_code(422) - expect(last_response).to have_error_message('Routes contains an invalid loadbalancing-algorithm option') + expect(last_response).to have_error_message("For application '#{app1_model.name}': \ +Route 'https://#{route.host}.#{route.domain.name}' contains invalid load-balancing algorithm 'unsupported-lb-algorithm'. Valid algorithms: 'round-robin, least-connections'") end end diff --git a/spec/unit/actions/route_update_spec.rb b/spec/unit/actions/route_update_spec.rb index 7ad61f3f812..bf2908dc0ac 100644 --- a/spec/unit/actions/route_update_spec.rb +++ b/spec/unit/actions/route_update_spec.rb @@ -36,9 +36,6 @@ module VCAP::CloudController metadata: { labels: new_labels, annotations: new_annotations - }, - options: { - lb_algo: 'round-robin' } } end @@ -136,98 +133,94 @@ module VCAP::CloudController end end end + describe '#update options' do - context 'when the route has no existing metadata' do - context 'when no metadata is specified' do + context 'when the route has no existing options' do + context 'when no options are specified' do let(:body) do {} end - it 'adds no metadata' do + it 'adds no options' do expect(message).to be_valid subject.update(route:, message:) route.reload - expect(route.labels.size).to eq(0) - expect(route.annotations.size).to eq(0) + expect(route.options).to eq({}) end end - context 'when metadata is specified' do - it 'updates the route metadata' do + context 'when an option is specified' do + let(:body) do + { + options: { + lb_algo: 'round-robin' + } + } + end + + it 'adds the route option' do expect(message).to be_valid subject.update(route:, message:) route.reload - expect(route).to have_labels( - { prefix: 'doordash.com', key_name: 'potato', value: 'mashed' }, - { prefix: nil, key_name: 'fruit', value: 'strawberries' }, - { prefix: nil, key_name: 'cuisine', value: 'thai' } - ) - expect(route).to have_annotations( - { key_name: 'potato', value: 'idaho' } - ) expect(route[:options]).to eq('{"lb_algo":"round-robin"}') end end end - context 'when the route has existing metadata' do + context 'when the route has existing options' do before do - VCAP::CloudController::LabelsUpdate.update(route, old_labels, VCAP::CloudController::RouteLabelModel) - VCAP::CloudController::AnnotationsUpdate.update(route, old_annotations, VCAP::CloudController::RouteAnnotationModel) route[:options] = '{"lb_algo": "round-robin"}' end - context 'when no metadata is specified' do + context 'when no options are specified' do let(:body) do {} end - it 'adds no metadata' do + it 'modifies nothing' do expect(message).to be_valid subject.update(route:, message:) route.reload - expect(route).to have_labels( - { prefix: nil, key_name: 'fruit', value: 'peach' }, - { prefix: nil, key_name: 'clothing', value: 'blouse' } - ) - expect(route).to have_annotations( - { key_name: 'potato', value: 'celandine' }, - { key_name: 'beet', value: 'formanova' } - ) expect(route.options).to include({ 'lb_algo' => 'round-robin' }) end end - context 'when metadata is specified' do + context 'when an option is specified' do let(:body) do { - metadata: { - labels: new_labels.merge(fruit: nil, newstuff: 'here'), - annotations: new_annotations.merge(beet: nil, asparagus: 'crunchy') + options: { + lb_algo: 'least-connections' } } end - it 'updates some, deletes nils, leaves unspecified fields alone' do + it 'updates the option' do expect(message).to be_valid subject.update(route:, message:) route.reload - expect(route).to have_labels( - { prefix: 'doordash.com', key_name: 'potato', value: 'mashed' }, - { prefix: nil, key_name: 'clothing', value: 'blouse' }, - { prefix: nil, key_name: 'newstuff', value: 'here' }, - { prefix: nil, key_name: 'cuisine', value: 'thai' } - ) - expect(route).to have_annotations( - { key_name: 'potato', value: 'idaho' }, - { key_name: 'asparagus', value: 'crunchy' } - ) + expect(route.options).to include({ 'lb_algo' => 'least-connections' }) + end + end + + context 'when the option value is set to null' do + let(:body) do + { + options: { + lb_algo: nil + } + } + end + + it 'removes this option' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to eq({}) end end end end - end end diff --git a/spec/unit/jobs/deserialization_spec.rb b/spec/unit/jobs/deserialization_spec.rb index 604127c5920..5e3185b2ab6 100644 --- a/spec/unit/jobs/deserialization_spec.rb +++ b/spec/unit/jobs/deserialization_spec.rb @@ -217,6 +217,7 @@ module Jobs :query:#{' '} :fragment:#{' '} :full_route: app.bommel + :options: {} :protocol:#{' '} apply_manifest_action: !ruby/object:VCAP::CloudController::AppApplyManifest user_audit_info: &6 !ruby/object:VCAP::CloudController::UserAuditInfo diff --git a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb index 380d75855a3..289eae0982a 100644 --- a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb +++ b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb @@ -140,7 +140,7 @@ module VCAP::CloudController ], port: nil, path: '/path', - options: nil + options: {} }) end @@ -153,7 +153,7 @@ module VCAP::CloudController ], port: nil, path: '/path', - options: nil + options: {} }) end @@ -167,7 +167,7 @@ module VCAP::CloudController ], port: nil, path: '/path', - options: nil + options: {} }) end @@ -181,7 +181,7 @@ module VCAP::CloudController ], port: 1234, path: '', - options: nil + options: {} }) end diff --git a/spec/unit/lib/cloud_controller/diego/protocol/routing_info_spec.rb b/spec/unit/lib/cloud_controller/diego/protocol/routing_info_spec.rb index 37e920291db..d74502cf615 100644 --- a/spec/unit/lib/cloud_controller/diego/protocol/routing_info_spec.rb +++ b/spec/unit/lib/cloud_controller/diego/protocol/routing_info_spec.rb @@ -48,8 +48,8 @@ class Protocol it 'returns the mapped http routes associated with the app with protocol' do expected_http = [ - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 8080, 'protocol' => 'http2' }, - { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1' } + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 8080, 'protocol' => 'http2', 'options' => {} }, + { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] @@ -66,8 +66,8 @@ class Protocol context 'and app has no ports' do it 'returns the mapped http routes associated with the app with a default of port 8080' do expected_http = [ - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 8080, 'protocol' => 'http1' }, - { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1' } + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 8080, 'protocol' => 'http1', 'options' => {} }, + { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] @@ -80,8 +80,8 @@ class Protocol it 'uses the first port available on the app' do expected_http = [ - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 7890, 'protocol' => 'http1' }, - { 'hostname' => route_without_service.uri, 'port' => 7890, 'protocol' => 'http1' } + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 7890, 'protocol' => 'http1', 'options' => {} }, + { 'hostname' => route_without_service.uri, 'port' => 7890, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] @@ -113,8 +113,8 @@ class Protocol it 'uses 8080 as a default' do expected_http = [ - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 8080, 'protocol' => 'http1' }, - { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1' } + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 8080, 'protocol' => 'http1', 'options' => {} }, + { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] @@ -127,8 +127,8 @@ class Protocol it 'uses the first docker port available on the app' do expected_http = [ - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 1024, 'protocol' => 'http1' }, - { 'hostname' => route_without_service.uri, 'port' => 1024, 'protocol' => 'http1' } + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 1024, 'protocol' => 'http1', 'options' => {} }, + { 'hostname' => route_without_service.uri, 'port' => 1024, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] @@ -154,7 +154,7 @@ class Protocol it 'returns the app port in routing info' do expected_http = [ - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 9090, 'protocol' => 'http1' } + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 9090, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] @@ -169,8 +169,8 @@ class Protocol it 'returns the app port in routing info' do expected_http = [ - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 8080, 'protocol' => 'http1' }, - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 9090, 'protocol' => 'http1' } + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 8080, 'protocol' => 'http1', 'options' => {} }, + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 9090, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] @@ -185,8 +185,8 @@ class Protocol it 'returns the app port in routing info' do expected_http = [ - { 'hostname' => route_without_service.uri, 'port' => 9090, 'protocol' => 'http1' }, - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 9090, 'protocol' => 'http1' } + { 'hostname' => route_without_service.uri, 'port' => 9090, 'protocol' => 'http1', 'options' => {} }, + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 9090, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] @@ -201,8 +201,8 @@ class Protocol it 'returns the app port in routing info' do expected_http = [ - { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1' }, - { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 9090, 'protocol' => 'http1' } + { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1', 'options' => {} }, + { 'hostname' => route_with_service.uri, 'route_service_url' => route_with_service.route_service_url, 'port' => 9090, 'protocol' => 'http1', 'options' => {} } ] expect(ri.keys).to match_array %w[http_routes internal_routes] expect(ri['http_routes']).to match_array expected_http @@ -216,7 +216,7 @@ class Protocol let!(:route_mapping2) { RouteMappingModel.make(app: process2.app, route: route_without_service) } it 'returns only one route without duplications' do - expected_http = { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1' } + expected_http = { 'hostname' => route_without_service.uri, 'port' => 8080, 'protocol' => 'http1', 'options' => {} } expect(ri.keys).to match_array %w[http_routes internal_routes] expect(ri['http_routes']).to contain_exactly expected_http @@ -232,7 +232,7 @@ class Protocol it 'returns the router group guid in the http routing info' do expect(ri.keys).to contain_exactly('http_routes', 'internal_routes') hr = ri['http_routes'][0] - expect(hr.keys).to contain_exactly('router_group_guid', 'port', 'hostname', 'protocol') + expect(hr.keys).to contain_exactly('router_group_guid', 'port', 'hostname', 'protocol', 'options') expect(hr['router_group_guid']).to eql(domain.router_group_guid) expect(hr['port']).to eql(http_route.port) expect(hr['hostname']).to match(/host-[0-9]+\.#{domain.name}/) diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index e370b3126d0..c62f7bf567a 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -238,6 +238,7 @@ module VCAP::CloudController msg = ManifestRoutesUpdateMessage.new(body) expect(msg.valid?).to be(false) + expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid route option 'invalid'. Valid keys: 'loadbalancing-algorithm'") end end @@ -258,6 +259,26 @@ module VCAP::CloudController expect(msg.valid?).to be(true) end end + + context 'when a route contains an invalid loadbalancing-algorithm' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing-algorithm' => 'sushi' + } } + ] } + end + + it 'returns false' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(false) + expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid load-balancing algorithm 'sushi'. \ +Valid algorithms: 'round-robin, least-connections'") + end + end end end end diff --git a/spec/unit/messages/route_create_message_spec.rb b/spec/unit/messages/route_create_message_spec.rb index d64eb61a456..e0d6d7b3d1f 100644 --- a/spec/unit/messages/route_create_message_spec.rb +++ b/spec/unit/messages/route_create_message_spec.rb @@ -392,6 +392,24 @@ module VCAP::CloudController end end + context 'when nil' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: nil + } + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:options]).to include("'options' is not a valid object") + end + end + context 'when options has invalid key' do let(:params) do { @@ -443,6 +461,23 @@ module VCAP::CloudController end end + context 'when lb_algo is nil' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { lb_algo: nil } + } + end + + it 'is valid' do + expect(subject).to be_valid + end + end + context 'when lb_algo has invalid value' do let(:params) do { diff --git a/spec/unit/messages/route_update_message_spec.rb b/spec/unit/messages/route_update_message_spec.rb index 18e92f49ac6..d3c1cac64ff 100644 --- a/spec/unit/messages/route_update_message_spec.rb +++ b/spec/unit/messages/route_update_message_spec.rb @@ -18,6 +18,11 @@ module VCAP::CloudController expect(message).to be_valid end + it 'accepts options: {}' do + message = RouteUpdateMessage.new(params.merge(options: {})) + expect(message).to be_valid + end + it 'accepts options params with round-robin load-balancing algorithm' do message = RouteUpdateMessage.new(params.merge(options: { lb_algo: 'round-robin' })) expect(message).to be_valid @@ -28,9 +33,10 @@ module VCAP::CloudController expect(message).to be_valid end - it 'accepts options: nil to unset options' do + it 'does not accept options: nil' do message = RouteUpdateMessage.new(params.merge(options: nil)) - expect(message).to be_valid + expect(message).not_to be_valid + expect(message.errors.full_messages[0]).to include("Options 'options' is not a valid object") end it 'accepts lb_algo: nil to unset load-balancing algorithm' do diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 931532574c9..dc617e3b2f8 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -555,6 +555,17 @@ def options_message expect(message).to be_valid end + it 'adds invalid options message when options is null' do + message = OptionsMessage.new({ options: nil }) + expect(message).not_to be_valid + expect(message.errors_on(:options)).to include("'options' is not a valid object") + end + + it 'successfully validates empty load balancer' do + message = OptionsMessage.new({ options: { lb_algo: nil } }) + expect(message).to be_valid + end + it 'adds invalid object error message when options is not an object' do message = OptionsMessage.new({ options: 'cheesecake' }) expect(message).not_to be_valid diff --git a/spec/unit/presenters/v3/app_manifest_presenter_spec.rb b/spec/unit/presenters/v3/app_manifest_presenter_spec.rb index c4b0a14de91..161f26171c5 100644 --- a/spec/unit/presenters/v3/app_manifest_presenter_spec.rb +++ b/spec/unit/presenters/v3/app_manifest_presenter_spec.rb @@ -111,8 +111,8 @@ module VCAP::CloudController::Presenters::V3 service_instance2.name ]) expect(application[:routes]).to eq([ - { route: route.uri, protocol: 'http1' }, - { route: route2.uri, protocol: 'http1' } + { route: route.uri, protocol: 'http1', options: {} }, + { route: route2.uri, protocol: 'http1', options: {} } ]) expect(application[:env]).to match({ 'one' => 'potato', 'two' => 'tomato' }) expect(application[:metadata]).to match({ labels: { 'potato' => 'idaho' }, annotations: { 'style' => 'mashed' } }) diff --git a/spec/unit/presenters/v3/route_presenter_spec.rb b/spec/unit/presenters/v3/route_presenter_spec.rb index 313a6b6f9f8..c56394011fd 100644 --- a/spec/unit/presenters/v3/route_presenter_spec.rb +++ b/spec/unit/presenters/v3/route_presenter_spec.rb @@ -138,8 +138,8 @@ module VCAP::CloudController::Presenters::V3 ) end - it 'does not output options' do - expect(subject[:options]).to be_nil + it 'outputs empty options hash' do + expect(subject[:options]).to eq({}) end end From 5567fcd9a76073cc371325624bf263c7df9f257b Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Tue, 3 Dec 2024 14:58:21 +0100 Subject: [PATCH 5/7] Remove route option validations from manifest_route --- .../manifest_routes_update_message.rb | 75 +++++++++---------- .../app_manifest/manifest_route.rb | 6 -- .../app_manifest/manifest_route_spec.rb | 10 --- .../manifest_routes_update_message_spec.rb | 2 + 4 files changed, 39 insertions(+), 54 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 8b9cfe0a39d..f8502cd8f18 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -8,14 +8,9 @@ class ManifestRoutesUpdateMessage < BaseMessage class ManifestRoutesYAMLValidator < ActiveModel::Validator def validate(record) - if is_not_array?(record.routes) || contains_non_route_hash_values?(record.routes) - record.errors.add(:routes, message: 'must be a list of route objects') - return - end + return unless is_not_array?(record.routes) || contains_non_route_hash_values?(record.routes) - contains_invalid_route_options?(record) - contains_invalid_lb_algo?(record) - nil + record.errors.add(:routes, message: 'must be a list of route objects') end def is_not_array?(routes) @@ -25,43 +20,14 @@ def is_not_array?(routes) def contains_non_route_hash_values?(routes) routes.any? { |r| !(r.is_a?(Hash) && r[:route].present?) } end - - def contains_invalid_route_options?(record) - routes = record.routes - routes.any? do |r| - next unless r[:options] - - return true unless r[:options].is_a?(Hash) - - return false if r[:options].empty? - - r[:options].each_key do |key| - RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) && - record.errors.add(:base, - message: "Route '#{r[:route]}' contains invalid route option '#{key}'. \ -Valid keys: '#{RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.join(', ')}'") - end - end - end - - def contains_invalid_lb_algo?(record) - routes = record.routes - routes.each do |r| - next unless r[:options] && r[:options][:'loadbalancing-algorithm'] - - lb_algo = r[:options][:'loadbalancing-algorithm'] - RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(lb_algo) && - record.errors.add(:base, - message: "Route '#{r[:route]}' contains invalid load-balancing algorithm '#{lb_algo}'. \ -Valid algorithms: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'") - end - end end validates_with NoAdditionalKeysValidator validates_with ManifestRoutesYAMLValidator, if: proc { |record| record.requested?(:routes) } validate :routes_are_uris, if: proc { |record| record.requested?(:routes) } validate :route_protocols_are_valid, if: proc { |record| record.requested?(:routes) } + validate :route_options_are_valid, if: proc { |record| record.requested?(:routes) } + validate :lb_algos_are_valid, if: proc { |record| record.requested?(:routes) } validate :no_route_is_boolean validate :default_route_is_boolean validate :random_route_is_boolean @@ -80,6 +46,39 @@ def manifest_route_mappings private + def route_options_are_valid + return if errors[:routes].present? + + routes.any? do |r| + next unless r[:options] + + return true unless r[:options].is_a?(Hash) + + return false if r[:options].empty? + + r[:options].each_key do |key| + RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) && + errors.add(:base, + message: "Route '#{r[:route]}' contains invalid route option '#{key}'. \ +Valid keys: '#{RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.join(', ')}'") + end + end + end + + def lb_algos_are_valid + return if errors[:routes].present? + + routes.each do |r| + next unless r[:options] && r[:options][:'loadbalancing-algorithm'] + + lb_algo = r[:options][:'loadbalancing-algorithm'] + RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(lb_algo) && + errors.add(:base, + message: "Route '#{r[:route]}' contains invalid load-balancing algorithm '#{lb_algo}'. \ +Valid algorithms: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'") + end + end + def routes_are_uris return if errors[:routes].present? diff --git a/lib/cloud_controller/app_manifest/manifest_route.rb b/lib/cloud_controller/app_manifest/manifest_route.rb index 9c17fc08a94..9449328819e 100644 --- a/lib/cloud_controller/app_manifest/manifest_route.rb +++ b/lib/cloud_controller/app_manifest/manifest_route.rb @@ -23,12 +23,6 @@ def self.parse(route, options=nil) end def valid? - if @attrs[:options] && !@attrs[:options].empty? - return false if @attrs[:options].keys.any? { |key| RouteOptionsMessage::VALID_ROUTE_OPTIONS.exclude?(key) } - # validation for loadbalancing algorithm - return false if @attrs[:options][:lb_algo] && RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(@attrs[:options][:lb_algo]) - end - return false if @attrs[:host].blank? return SUPPORTED_TCP_SCHEMES.include?(@attrs[:scheme]) if @attrs[:port] diff --git a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb index 289eae0982a..d685a5c7746 100644 --- a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb +++ b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb @@ -116,16 +116,6 @@ module VCAP::CloudController expect(manifest_route.valid?).to be(false) end end - - context 'when there is an invalid loadbalancing-algorithm' do - let(:route) { 'http://example.com' } - let(:options) { { 'loadbalancing-algorithm': 'invalid' } } - - it 'is invalid' do - manifest_route = ManifestRoute.parse(route, options) - expect(manifest_route.valid?).to be(false) - end - end end end diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index c62f7bf567a..03135aff1c7 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -238,6 +238,7 @@ module VCAP::CloudController msg = ManifestRoutesUpdateMessage.new(body) expect(msg.valid?).to be(false) + expect(msg.errors.errors.length).to eq(1) expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid route option 'invalid'. Valid keys: 'loadbalancing-algorithm'") end end @@ -275,6 +276,7 @@ module VCAP::CloudController msg = ManifestRoutesUpdateMessage.new(body) expect(msg.valid?).to be(false) + expect(msg.errors.errors.length).to eq(1) expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid load-balancing algorithm 'sushi'. \ Valid algorithms: 'round-robin, least-connections'") end From cd7d06a0ef077087b5fbb3721ffe1359fd0b074c Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 5 Dec 2024 14:08:08 +0100 Subject: [PATCH 6/7] Rename 'lb_algo' and 'loadbalancing-algorithm' to 'algorithm' --- .../manifest_routes_update_message.rb | 12 ++--- app/messages/route_options_message.rb | 8 ++-- .../route_properties_presenter.rb | 2 +- .../source/includes/api_resources/_routes.erb | 6 +-- .../includes/resources/routes/_create.md.erb | 2 +- .../routes/_route_options_object.md.erb | 2 +- .../includes/resources/routes/_update.md.erb | 2 +- .../app_manifest/manifest_route.rb | 2 +- spec/request/space_manifests_spec.rb | 36 +++++++-------- spec/unit/actions/route_update_spec.rb | 16 +++---- .../app_manifest/manifest_route_spec.rb | 10 ++--- .../manifest_routes_update_message_spec.rb | 44 ++++++++++++++++--- .../messages/route_create_message_spec.rb | 20 ++++----- .../messages/route_update_message_spec.rb | 12 ++--- spec/unit/messages/validators_spec.rb | 10 ++--- .../presenters/v3/route_presenter_spec.rb | 4 +- 16 files changed, 111 insertions(+), 77 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index f8502cd8f18..f61ae5f2d33 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -27,7 +27,7 @@ def contains_non_route_hash_values?(routes) validate :routes_are_uris, if: proc { |record| record.requested?(:routes) } validate :route_protocols_are_valid, if: proc { |record| record.requested?(:routes) } validate :route_options_are_valid, if: proc { |record| record.requested?(:routes) } - validate :lb_algos_are_valid, if: proc { |record| record.requested?(:routes) } + validate :loadbalancings_are_valid, if: proc { |record| record.requested?(:routes) } validate :no_route_is_boolean validate :default_route_is_boolean validate :random_route_is_boolean @@ -65,16 +65,16 @@ def route_options_are_valid end end - def lb_algos_are_valid + def loadbalancings_are_valid return if errors[:routes].present? routes.each do |r| - next unless r[:options] && r[:options][:'loadbalancing-algorithm'] + next unless r[:options] && r[:options][:'loadbalancing'] - lb_algo = r[:options][:'loadbalancing-algorithm'] - RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(lb_algo) && + loadbalancing = r[:options][:'loadbalancing'] + RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(loadbalancing) && errors.add(:base, - message: "Route '#{r[:route]}' contains invalid load-balancing algorithm '#{lb_algo}'. \ + message: "Route '#{r[:route]}' contains invalid load-balancing algorithm '#{loadbalancing}'. \ Valid algorithms: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'") end end diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index c40d278db36..954abd593cf 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -2,14 +2,14 @@ module VCAP::CloudController class RouteOptionsMessage < BaseMessage - VALID_MANIFEST_ROUTE_OPTIONS = %i[loadbalancing-algorithm].freeze - VALID_ROUTE_OPTIONS = %i[lb_algo].freeze + VALID_MANIFEST_ROUTE_OPTIONS = %i[loadbalancing].freeze + VALID_ROUTE_OPTIONS = %i[loadbalancing].freeze VALID_LOADBALANCING_ALGORITHMS = %w[round-robin least-connections].freeze register_allowed_keys VALID_ROUTE_OPTIONS validates_with NoAdditionalKeysValidator - validates :lb_algo, - inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "'%s' is not a supported load-balancing algorithm" }, + validates :loadbalancing, + inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "'%s' is not supported" }, presence: true, allow_nil: true end diff --git a/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb b/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb index f4cef6ab901..4f6e2dad6be 100644 --- a/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb +++ b/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb @@ -12,7 +12,7 @@ def to_hash(route_mappings:, app:, **_) if route_mapping.route.options route_hash[:options] = {} - route_hash[:options][:'loadbalancing-algorithm'] = route_mapping.route.options[:lb_algo] if route_mapping.route.options[:lb_algo] + route_hash[:options][:'loadbalancing'] = route_mapping.route.options[:loadbalancing] if route_mapping.route.options[:loadbalancing] end route_hash diff --git a/docs/v3/source/includes/api_resources/_routes.erb b/docs/v3/source/includes/api_resources/_routes.erb index c1448303e35..a1f664a76e5 100644 --- a/docs/v3/source/includes/api_resources/_routes.erb +++ b/docs/v3/source/includes/api_resources/_routes.erb @@ -35,7 +35,7 @@ } ], "options": { - "lb_algo": "round-robin" + "loadbalancing": "round-robin" }, "metadata": { "labels": <%= metadata.fetch(:labels, {}).to_json(space: ' ', object_nl: ' ')%>, @@ -128,7 +128,7 @@ } ], "options": { - "lb_algo": "round-robin" + "loadbalancing": "round-robin" }, "metadata": { "labels": {}, @@ -221,6 +221,6 @@ <% content_for :route_options do %> { - "lb_algo": "round-robin" + "loadbalancing": "round-robin" } <% end %> diff --git a/docs/v3/source/includes/resources/routes/_create.md.erb b/docs/v3/source/includes/resources/routes/_create.md.erb index d0315ea03e1..6ad5b93a9f9 100644 --- a/docs/v3/source/includes/resources/routes/_create.md.erb +++ b/docs/v3/source/includes/resources/routes/_create.md.erb @@ -22,7 +22,7 @@ curl "https://api.example.org/v3/routes" \ } }, "options": { - "lb_algo": "round-robin" + "loadbalancing": "round-robin" } "metadata": { "labels": { "key": "value" }, diff --git a/docs/v3/source/includes/resources/routes/_route_options_object.md.erb b/docs/v3/source/includes/resources/routes/_route_options_object.md.erb index c891ad9ee47..fbb69af0916 100644 --- a/docs/v3/source/includes/resources/routes/_route_options_object.md.erb +++ b/docs/v3/source/includes/resources/routes/_route_options_object.md.erb @@ -9,4 +9,4 @@ Example Route-Options object | Name | Type | Description | |-------------|----------|----------------------------------------------------------------------------------------------------------| -| **lb_algo** | _string_ | The load balancer associated with this route. Valid algorithms are 'round-robin' and 'least-connections' | +| **loadbalancing** | _string_ | The load balancer associated with this route. Valid algorithms are 'round-robin' and 'least-connections' | diff --git a/docs/v3/source/includes/resources/routes/_update.md.erb b/docs/v3/source/includes/resources/routes/_update.md.erb index 4814a23d171..60dcbf9be77 100644 --- a/docs/v3/source/includes/resources/routes/_update.md.erb +++ b/docs/v3/source/includes/resources/routes/_update.md.erb @@ -11,7 +11,7 @@ curl "https://api.example.org/v3/routes/[guid]" \ -H "Content-type: application/json" \ -d '{ "options": { - "lb_algo": "round-robin" + "loadbalancing": "round-robin" } "metadata": { "labels": {"key": "value"}, diff --git a/lib/cloud_controller/app_manifest/manifest_route.rb b/lib/cloud_controller/app_manifest/manifest_route.rb index 9449328819e..511e661600d 100644 --- a/lib/cloud_controller/app_manifest/manifest_route.rb +++ b/lib/cloud_controller/app_manifest/manifest_route.rb @@ -17,7 +17,7 @@ def self.parse(route, options=nil) attrs[:full_route] = route attrs[:options] = {} - attrs[:options][:lb_algo] = options[:'loadbalancing-algorithm'] if options && options.key?(:'loadbalancing-algorithm') + attrs[:options][:loadbalancing] = options[:'loadbalancing'] if options && options.key?(:'loadbalancing') ManifestRoute.new(attrs) end diff --git a/spec/request/space_manifests_spec.rb b/spec/request/space_manifests_spec.rb index 77f8869c8e0..75cde1ecc15 100644 --- a/spec/request/space_manifests_spec.rb +++ b/spec/request/space_manifests_spec.rb @@ -647,12 +647,12 @@ expect(last_response).to have_status_code(422) expect(last_response).to have_error_message("For application '#{app1_model.name}': \ -Route 'https://#{route.host}.#{route.domain.name}' contains invalid route option 'doesnt-exist'. Valid keys: 'loadbalancing-algorithm'") +Route 'https://#{route.host}.#{route.domain.name}' contains invalid route option 'doesnt-exist'. Valid keys: 'loadbalancing'") end end context 'updating existing route options' do - # using loadbalancing-algorithm as an example since it is the only route option currently supported + # using loadbalancing as an example since it is the only route option currently supported before do yml_manifest = { 'applications' => [ @@ -660,7 +660,7 @@ 'routes' => [ { 'route' => "https://round-robin-app.#{shared_domain.name}", 'options' => { - 'loadbalancing-algorithm' => 'round-robin' + 'loadbalancing' => 'round-robin' } } ] } ] @@ -675,7 +675,7 @@ Delayed::Worker.new.work_off expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error app1_model.reload - expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) end it 'updates the route option when a new value is provided' do @@ -685,7 +685,7 @@ 'routes' => [ { 'route' => "https://round-robin-app.#{shared_domain.name}", 'options' => { - 'loadbalancing-algorithm' => 'least-connections' + 'loadbalancing' => 'least-connections' } } ] } ] @@ -699,7 +699,7 @@ Delayed::Worker.new.work_off expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error app1_model.reload - expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'least-connections' }) + expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'least-connections' }) end it 'does not modify any route options when the options hash is not provided' do @@ -721,7 +721,7 @@ Delayed::Worker.new.work_off expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error app1_model.reload - expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) end it 'does not modify any route options options: nil is provided' do @@ -744,7 +744,7 @@ Delayed::Worker.new.work_off expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error app1_model.reload - expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) end it 'does not modify any route options if an empty options hash is provided' do @@ -764,7 +764,7 @@ expect(last_response.status).to eq(202) app1_model.reload - expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) end it 'does not modify any option when options: { key: nil } is provided' do @@ -774,7 +774,7 @@ 'routes' => [ { 'route' => "https://round-robin-app.#{shared_domain.name}", 'options' => { - 'loadbalancing-algorithm' => nil + 'loadbalancing' => nil } } ] } ] @@ -786,12 +786,12 @@ expect(last_response.status).to eq(202) app1_model.reload - expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) end end - context 'route-option: loadbalancing-algorithm' do - context 'when the loadbalancing-algorithm is not supported' do + context 'route-option: loadbalancing' do + context 'when the loadbalancing is not supported' do let(:yml_manifest) do { 'applications' => [ @@ -800,7 +800,7 @@ 'routes' => [ { 'route' => "https://#{route.host}.#{route.domain.name}", 'options' => { - 'loadbalancing-algorithm' => 'unsupported-lb-algorithm' + 'loadbalancing' => 'unsupported-lb-algorithm' } } ] } @@ -817,7 +817,7 @@ end end - context 'when the loadbalancing-algorithm is supported' do + context 'when the loadbalancing is supported' do let(:yml_manifest) do { 'applications' => [ @@ -825,14 +825,14 @@ 'routes' => [ { 'route' => "https://round-robin-app.#{shared_domain.name}", 'options' => { - 'loadbalancing-algorithm' => 'round-robin' + 'loadbalancing' => 'round-robin' } } ] } ] }.to_yaml end - it 'adds the loadbalancing-algorithm' do + it 'adds the loadbalancing' do post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) expect(last_response.status).to eq(202) @@ -842,7 +842,7 @@ expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error app1_model.reload - expect(app1_model.routes.first.options).to eq({ 'lb_algo' => 'round-robin' }) + expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) end end end diff --git a/spec/unit/actions/route_update_spec.rb b/spec/unit/actions/route_update_spec.rb index bf2908dc0ac..d9cbb94bd51 100644 --- a/spec/unit/actions/route_update_spec.rb +++ b/spec/unit/actions/route_update_spec.rb @@ -17,7 +17,7 @@ module VCAP::CloudController } end let(:old_options) do - '{"lb_algo": "round-robin"}' + '{"loadbalancing": "round-robin"}' end let(:new_labels) do { @@ -153,7 +153,7 @@ module VCAP::CloudController let(:body) do { options: { - lb_algo: 'round-robin' + loadbalancing: 'round-robin' } } end @@ -163,14 +163,14 @@ module VCAP::CloudController subject.update(route:, message:) route.reload - expect(route[:options]).to eq('{"lb_algo":"round-robin"}') + expect(route[:options]).to eq('{"loadbalancing":"round-robin"}') end end end context 'when the route has existing options' do before do - route[:options] = '{"lb_algo": "round-robin"}' + route[:options] = '{"loadbalancing": "round-robin"}' end context 'when no options are specified' do @@ -182,7 +182,7 @@ module VCAP::CloudController expect(message).to be_valid subject.update(route:, message:) route.reload - expect(route.options).to include({ 'lb_algo' => 'round-robin' }) + expect(route.options).to include({ 'loadbalancing' => 'round-robin' }) end end @@ -190,7 +190,7 @@ module VCAP::CloudController let(:body) do { options: { - lb_algo: 'least-connections' + loadbalancing: 'least-connections' } } end @@ -200,7 +200,7 @@ module VCAP::CloudController subject.update(route:, message:) route.reload - expect(route.options).to include({ 'lb_algo' => 'least-connections' }) + expect(route.options).to include({ 'loadbalancing' => 'least-connections' }) end end @@ -208,7 +208,7 @@ module VCAP::CloudController let(:body) do { options: { - lb_algo: nil + loadbalancing: nil } } end diff --git a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb index d685a5c7746..34b3f963349 100644 --- a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb +++ b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb @@ -59,9 +59,9 @@ module VCAP::CloudController end end - context 'when there is a valid loadbalancing-algorithm' do + context 'when there is a valid loadbalancing' do let(:route) { 'http://example.com' } - let(:options) { { 'loadbalancing-algorithm' => 'round-robin' } } + let(:options) { { 'loadbalancing' => 'round-robin' } } it 'is valid' do manifest_route = ManifestRoute.parse(route, options) @@ -175,8 +175,8 @@ module VCAP::CloudController }) end - it 'parses a route with a loadbalancing-algorithm into route components' do - route = ManifestRoute.parse('http://potato.sub.some-domain.com', { 'loadbalancing-algorithm': 'round-robin' }) + it 'parses a route with a loadbalancing into route components' do + route = ManifestRoute.parse('http://potato.sub.some-domain.com', { 'loadbalancing': 'round-robin' }) expect(route.to_hash).to eq({ candidate_host_domain_pairs: [ @@ -185,7 +185,7 @@ module VCAP::CloudController ], port: nil, path: '', - options: { lb_algo: 'round-robin' } + options: { loadbalancing: 'round-robin' } }) end end diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index 03135aff1c7..e65403fbcb7 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -225,6 +225,22 @@ module VCAP::CloudController end end + context 'when a route contains nil route option' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => nil } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + context 'when a route contains invalid route options' do let(:body) do { 'routes' => @@ -239,17 +255,17 @@ module VCAP::CloudController expect(msg.valid?).to be(false) expect(msg.errors.errors.length).to eq(1) - expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid route option 'invalid'. Valid keys: 'loadbalancing-algorithm'") + expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid route option 'invalid'. Valid keys: 'loadbalancing'") end end - context 'when a route contains a valid loadbalancing-algorithm' do + context 'when a route contains a valid value for loadbalancing' do let(:body) do { 'routes' => [ { 'route' => 'existing.example.com', 'options' => { - 'loadbalancing-algorithm' => 'round-robin' + 'loadbalancing' => 'round-robin' } } ] } end @@ -261,13 +277,31 @@ module VCAP::CloudController end end - context 'when a route contains an invalid loadbalancing-algorithm' do + context 'when a route contains null as a value for loadbalancing' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => nil + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains an invalid value for loadbalancing' do let(:body) do { 'routes' => [ { 'route' => 'existing.example.com', 'options' => { - 'loadbalancing-algorithm' => 'sushi' + 'loadbalancing' => 'sushi' } } ] } end diff --git a/spec/unit/messages/route_create_message_spec.rb b/spec/unit/messages/route_create_message_spec.rb index e0d6d7b3d1f..3bc2fc6fe3b 100644 --- a/spec/unit/messages/route_create_message_spec.rb +++ b/spec/unit/messages/route_create_message_spec.rb @@ -21,7 +21,7 @@ module VCAP::CloudController annotations: { style: 'mashed' } }, options: { - lb_algo: 'round-robin' + loadbalancing: 'round-robin' } } end @@ -428,7 +428,7 @@ module VCAP::CloudController end end - context 'when lb_algo has value round-robin' do + context 'when loadbalancing has value round-robin' do let(:params) do { host: 'some-host', @@ -436,7 +436,7 @@ module VCAP::CloudController space: { data: { guid: 'space-guid' } }, domain: { data: { guid: 'domain-guid' } } }, - options: { lb_algo: 'round-robin' } + options: { loadbalancing: 'round-robin' } } end @@ -444,7 +444,7 @@ module VCAP::CloudController expect(subject).to be_valid end - context 'when lb_algo has value least-connections' do + context 'when loadbalancing has value least-connections' do let(:params) do { host: 'some-host', @@ -452,7 +452,7 @@ module VCAP::CloudController space: { data: { guid: 'space-guid' } }, domain: { data: { guid: 'domain-guid' } } }, - options: { lb_algo: 'least-connections' } + options: { loadbalancing: 'least-connections' } } end @@ -461,7 +461,7 @@ module VCAP::CloudController end end - context 'when lb_algo is nil' do + context 'when loadbalancing is nil' do let(:params) do { host: 'some-host', @@ -469,7 +469,7 @@ module VCAP::CloudController space: { data: { guid: 'space-guid' } }, domain: { data: { guid: 'domain-guid' } } }, - options: { lb_algo: nil } + options: { loadbalancing: nil } } end @@ -478,7 +478,7 @@ module VCAP::CloudController end end - context 'when lb_algo has invalid value' do + context 'when loadbalancing has invalid value' do let(:params) do { host: 'some-host', @@ -486,13 +486,13 @@ module VCAP::CloudController space: { data: { guid: 'space-guid' } }, domain: { data: { guid: 'domain-guid' } } }, - options: { lb_algo: 'random' } + options: { loadbalancing: 'random' } } end it 'is not valid' do expect(subject).not_to be_valid - expect(subject.errors[:options]).to include("Lb algo 'random' is not a supported load-balancing algorithm") + expect(subject.errors[:options]).to include("Loadbalancing 'random' is not supported") end end end diff --git a/spec/unit/messages/route_update_message_spec.rb b/spec/unit/messages/route_update_message_spec.rb index d3c1cac64ff..60b26826b26 100644 --- a/spec/unit/messages/route_update_message_spec.rb +++ b/spec/unit/messages/route_update_message_spec.rb @@ -24,12 +24,12 @@ module VCAP::CloudController end it 'accepts options params with round-robin load-balancing algorithm' do - message = RouteUpdateMessage.new(params.merge(options: { lb_algo: 'round-robin' })) + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'round-robin' })) expect(message).to be_valid end it 'accepts options params with least-connections load-balancing algorithm' do - message = RouteUpdateMessage.new(params.merge(options: { lb_algo: 'least-connections' })) + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'least-connections' })) expect(message).to be_valid end @@ -39,8 +39,8 @@ module VCAP::CloudController expect(message.errors.full_messages[0]).to include("Options 'options' is not a valid object") end - it 'accepts lb_algo: nil to unset load-balancing algorithm' do - message = RouteUpdateMessage.new(params.merge(options: { lb_algo: nil })) + it 'accepts loadbalancing: nil to unset load-balancing algorithm' do + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: nil })) expect(message).to be_valid end @@ -51,9 +51,9 @@ module VCAP::CloudController end it 'does not accept unknown load-balancing algorithm' do - message = RouteUpdateMessage.new(params.merge(options: { lb_algo: 'cheesecake' })) + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'cheesecake' })) expect(message).not_to be_valid - expect(message.errors.full_messages[0]).to include("Options Lb algo 'cheesecake' is not a supported load-balancing algorithm") + expect(message.errors.full_messages[0]).to include("Options Loadbalancing 'cheesecake' is not supported") end it 'does not accept unknown option' do diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index dc617e3b2f8..6388c0234a9 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -541,12 +541,12 @@ def options_message end it 'successfully validates round-robin load-balancing algorithm' do - message = OptionsMessage.new({ options: { lb_algo: 'round-robin' } }) + message = OptionsMessage.new({ options: { loadbalancing: 'round-robin' } }) expect(message).to be_valid end it 'successfully validates least-connections load-balancing algorithm' do - message = OptionsMessage.new({ options: { lb_algo: 'least-connections' } }) + message = OptionsMessage.new({ options: { loadbalancing: 'least-connections' } }) expect(message).to be_valid end @@ -562,7 +562,7 @@ def options_message end it 'successfully validates empty load balancer' do - message = OptionsMessage.new({ options: { lb_algo: nil } }) + message = OptionsMessage.new({ options: { loadbalancing: nil } }) expect(message).to be_valid end @@ -573,9 +573,9 @@ def options_message end it 'adds invalid load balancer error message to the base class' do - message = OptionsMessage.new({ options: { lb_algo: 'donuts' } }) + message = OptionsMessage.new({ options: { loadbalancing: 'donuts' } }) expect(message).not_to be_valid - expect(message.errors_on(:options)).to include('Lb algo \'donuts\' is not a supported load-balancing algorithm') + expect(message.errors_on(:options)).to include('Loadbalancing \'donuts\' is not supported') end it 'adds invalid field error message to the base class' do diff --git a/spec/unit/presenters/v3/route_presenter_spec.rb b/spec/unit/presenters/v3/route_presenter_spec.rb index c56394011fd..423e07c6eb5 100644 --- a/spec/unit/presenters/v3/route_presenter_spec.rb +++ b/spec/unit/presenters/v3/route_presenter_spec.rb @@ -5,7 +5,7 @@ module VCAP::CloudController::Presenters::V3 RSpec.describe RoutePresenter do let!(:app) { VCAP::CloudController::AppModel.make } let(:space) { VCAP::CloudController::Space.make } - let(:options) { { lb_algo: 'round-robin' } } + let(:options) { { loadbalancing: 'round-robin' } } let(:org) { space.organization } let(:route_host) { 'host' } let(:path) { '/path' } @@ -72,7 +72,7 @@ module VCAP::CloudController::Presenters::V3 expect(subject[:updated_at]).to be_a(Time) expect(subject[:host]).to eq(route_host) expect(subject[:path]).to eq(path) - expect(subject[:options]).to eq('lb_algo' => 'round-robin') + expect(subject[:options]).to eq('loadbalancing' => 'round-robin') expect(subject[:url]).to eq("#{route.host}.#{domain.name}#{route.path}") expected_destinations = [ From 97440e0ac92f69945c15158b33e01d2ed87c525d Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 5 Dec 2024 20:27:01 +0100 Subject: [PATCH 7/7] Disallow null in manifest; Cleanup error outputs --- app/actions/route_update.rb | 8 +---- .../manifest_routes_update_message.rb | 23 +++++++++----- app/messages/route_options_message.rb | 2 +- .../route_properties_presenter.rb | 2 +- .../routes/_route_options_object.md.erb | 6 ++-- .../app_manifest/manifest_route.rb | 2 +- spec/request/space_manifests_spec.rb | 30 +++++++++++-------- .../app_manifest/manifest_route_spec.rb | 2 +- .../manifest_routes_update_message_spec.rb | 9 +++--- .../messages/route_create_message_spec.rb | 2 +- .../messages/route_update_message_spec.rb | 2 +- spec/unit/messages/validators_spec.rb | 2 +- 12 files changed, 48 insertions(+), 42 deletions(-) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index e11d34deba8..746dcfcd8ef 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -2,13 +2,7 @@ module VCAP::CloudController class RouteUpdate def update(route:, message:) Route.db.transaction do - if message.requested?(:options) - route.options = if message.options.nil? - {} - else - route.options.symbolize_keys.merge(message.options).compact - end - end + route.options = route.options.symbolize_keys.merge(message.options).compact if message.requested?(:options) route.save MetadataUpdate.update(route, message) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index f61ae5f2d33..bc4c6b9cb5e 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -50,11 +50,12 @@ def route_options_are_valid return if errors[:routes].present? routes.any? do |r| - next unless r[:options] + next unless r.keys.include?(:options) - return true unless r[:options].is_a?(Hash) - - return false if r[:options].empty? + unless r[:options].is_a?(Hash) + errors.add(:base, message: "Route '#{r[:route]}': options must be an object") + next + end r[:options].each_key do |key| RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) && @@ -69,13 +70,19 @@ def loadbalancings_are_valid return if errors[:routes].present? routes.each do |r| - next unless r[:options] && r[:options][:'loadbalancing'] + next unless r.keys.include?(:options) && r[:options].is_a?(Hash) && r[:options].keys.include?(:loadbalancing) - loadbalancing = r[:options][:'loadbalancing'] + loadbalancing = r[:options][:loadbalancing] + unless loadbalancing.is_a?(String) + errors.add(:base, + message: "Invalid value for 'loadbalancing' for Route '#{r[:route]}'; \ +Valid values are: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'") + next + end RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(loadbalancing) && errors.add(:base, - message: "Route '#{r[:route]}' contains invalid load-balancing algorithm '#{loadbalancing}'. \ -Valid algorithms: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'") + message: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \ +Valid values are: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'") end end diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 954abd593cf..818eb039ddf 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -9,7 +9,7 @@ class RouteOptionsMessage < BaseMessage register_allowed_keys VALID_ROUTE_OPTIONS validates_with NoAdditionalKeysValidator validates :loadbalancing, - inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "'%s' is not supported" }, + inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "must be one of '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}' if present" }, presence: true, allow_nil: true end diff --git a/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb b/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb index 4f6e2dad6be..d58c52c3003 100644 --- a/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb +++ b/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb @@ -12,7 +12,7 @@ def to_hash(route_mappings:, app:, **_) if route_mapping.route.options route_hash[:options] = {} - route_hash[:options][:'loadbalancing'] = route_mapping.route.options[:loadbalancing] if route_mapping.route.options[:loadbalancing] + route_hash[:options][:loadbalancing] = route_mapping.route.options[:loadbalancing] if route_mapping.route.options[:loadbalancing] end route_hash diff --git a/docs/v3/source/includes/resources/routes/_route_options_object.md.erb b/docs/v3/source/includes/resources/routes/_route_options_object.md.erb index fbb69af0916..48de982ed2f 100644 --- a/docs/v3/source/includes/resources/routes/_route_options_object.md.erb +++ b/docs/v3/source/includes/resources/routes/_route_options_object.md.erb @@ -7,6 +7,6 @@ Example Route-Options object <%= yield_content :route_options %> ``` -| Name | Type | Description | -|-------------|----------|----------------------------------------------------------------------------------------------------------| -| **loadbalancing** | _string_ | The load balancer associated with this route. Valid algorithms are 'round-robin' and 'least-connections' | +| Name | Type | Description | +|-------------------|----------|------------------------------------------------------------------------------------------------------| +| **loadbalancing** | _string_ | The load-balancer associated with this route. Valid values are 'round-robin' and 'least-connections' | diff --git a/lib/cloud_controller/app_manifest/manifest_route.rb b/lib/cloud_controller/app_manifest/manifest_route.rb index 511e661600d..113dfe99b9b 100644 --- a/lib/cloud_controller/app_manifest/manifest_route.rb +++ b/lib/cloud_controller/app_manifest/manifest_route.rb @@ -17,7 +17,7 @@ def self.parse(route, options=nil) attrs[:full_route] = route attrs[:options] = {} - attrs[:options][:loadbalancing] = options[:'loadbalancing'] if options && options.key?(:'loadbalancing') + attrs[:options][:loadbalancing] = options[:loadbalancing] if options && options.key?(:loadbalancing) ManifestRoute.new(attrs) end diff --git a/spec/request/space_manifests_spec.rb b/spec/request/space_manifests_spec.rb index 75cde1ecc15..55d3ca92fcd 100644 --- a/spec/request/space_manifests_spec.rb +++ b/spec/request/space_manifests_spec.rb @@ -658,7 +658,7 @@ 'applications' => [ { 'name' => app1_model.name, 'routes' => [ - { 'route' => "https://round-robin-app.#{shared_domain.name}", + { 'route' => "https://#{route.host}.#{shared_domain.name}", 'options' => { 'loadbalancing' => 'round-robin' } } @@ -683,7 +683,7 @@ 'applications' => [ { 'name' => app1_model.name, 'routes' => [ - { 'route' => "https://round-robin-app.#{shared_domain.name}", + { 'route' => "https://#{route.host}.#{shared_domain.name}", 'options' => { 'loadbalancing' => 'least-connections' } } @@ -707,7 +707,7 @@ 'applications' => [ { 'name' => app1_model.name, 'routes' => [ - { 'route' => "https://round-robin-app.#{shared_domain.name}" } + { 'route' => "https://#{route.host}.#{shared_domain.name}" } ] } ] }.to_yaml @@ -724,12 +724,12 @@ expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) end - it 'does not modify any route options options: nil is provided' do + it 'returns 422 when options: null is provided' do yml_manifest = { 'applications' => [ { 'name' => app1_model.name, 'routes' => [ - { 'route' => "https://round-robin-app.#{shared_domain.name}", + { 'route' => "https://#{route.host}.#{shared_domain.name}", 'options' => nil } ] } ] @@ -738,9 +738,11 @@ # apply the manifest with the route option post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) - expect(last_response.status).to eq(202) - job_guid = VCAP::CloudController::PollableJobModel.last.guid + expect(last_response.status).to eq(422) + expect(last_response).to have_error_message("For application '#{app1_model.name}': \ +Route 'https://#{route.host}.#{route.domain.name}': options must be an object") + job_guid = VCAP::CloudController::PollableJobModel.last.guid Delayed::Worker.new.work_off expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete, VCAP::CloudController::PollableJobModel.find(guid: job_guid).cf_api_error app1_model.reload @@ -752,7 +754,7 @@ 'applications' => [ { 'name' => app1_model.name, 'routes' => [ - { 'route' => "https://round-robin-app.#{shared_domain.name}", + { 'route' => "https://#{route.host}.#{shared_domain.name}", 'options' => {} } ] } ] @@ -767,12 +769,12 @@ expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) end - it 'does not modify any option when options: { key: nil } is provided' do + it 'returns 422 when { loadbalancing: null } is provided' do yml_manifest = { 'applications' => [ { 'name' => app1_model.name, 'routes' => [ - { 'route' => "https://round-robin-app.#{shared_domain.name}", + { 'route' => "https://#{route.host}.#{shared_domain.name}", 'options' => { 'loadbalancing' => nil } } @@ -783,7 +785,9 @@ # apply the manifest with the route option post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header) - expect(last_response.status).to eq(202) + expect(last_response.status).to eq(422) + expect(last_response).to have_error_message("For application '#{app1_model.name}': \ +Invalid value for 'loadbalancing' for Route 'https://#{route.host}.#{route.domain.name}'; Valid values are: 'round-robin, least-connections'") app1_model.reload expect(app1_model.routes.first.options).to eq({ 'loadbalancing' => 'round-robin' }) @@ -813,7 +817,7 @@ expect(last_response).to have_status_code(422) expect(last_response).to have_error_message("For application '#{app1_model.name}': \ -Route 'https://#{route.host}.#{route.domain.name}' contains invalid load-balancing algorithm 'unsupported-lb-algorithm'. Valid algorithms: 'round-robin, least-connections'") +Cannot use loadbalancing value 'unsupported-lb-algorithm' for Route 'https://#{route.host}.#{route.domain.name}'; Valid values are: 'round-robin, least-connections'") end end @@ -823,7 +827,7 @@ 'applications' => [ { 'name' => app1_model.name, 'routes' => [ - { 'route' => "https://round-robin-app.#{shared_domain.name}", + { 'route' => "https://#{route.host}.#{shared_domain.name}", 'options' => { 'loadbalancing' => 'round-robin' } } diff --git a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb index 34b3f963349..6fa32f816aa 100644 --- a/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb +++ b/spec/unit/lib/cloud_controller/app_manifest/manifest_route_spec.rb @@ -176,7 +176,7 @@ module VCAP::CloudController end it 'parses a route with a loadbalancing into route components' do - route = ManifestRoute.parse('http://potato.sub.some-domain.com', { 'loadbalancing': 'round-robin' }) + route = ManifestRoute.parse('http://potato.sub.some-domain.com', { loadbalancing: 'round-robin' }) expect(route.to_hash).to eq({ candidate_host_domain_pairs: [ diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index e65403fbcb7..60bab7e9639 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -237,7 +237,8 @@ module VCAP::CloudController it 'returns true' do msg = ManifestRoutesUpdateMessage.new(body) - expect(msg.valid?).to be(true) + expect(msg.valid?).to be(false) + expect(msg.errors.full_messages).to include("Route 'existing.example.com': options must be an object") end end @@ -291,7 +292,8 @@ module VCAP::CloudController it 'returns true' do msg = ManifestRoutesUpdateMessage.new(body) - expect(msg.valid?).to be(true) + expect(msg.valid?).to be(false) + expect(msg.errors.full_messages).to include("Invalid value for 'loadbalancing' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connections'") end end @@ -311,8 +313,7 @@ module VCAP::CloudController expect(msg.valid?).to be(false) expect(msg.errors.errors.length).to eq(1) - expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid load-balancing algorithm 'sushi'. \ -Valid algorithms: 'round-robin, least-connections'") + expect(msg.errors.full_messages).to include("Cannot use loadbalancing value 'sushi' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connections'") end end end diff --git a/spec/unit/messages/route_create_message_spec.rb b/spec/unit/messages/route_create_message_spec.rb index 3bc2fc6fe3b..3fa936da526 100644 --- a/spec/unit/messages/route_create_message_spec.rb +++ b/spec/unit/messages/route_create_message_spec.rb @@ -492,7 +492,7 @@ module VCAP::CloudController it 'is not valid' do expect(subject).not_to be_valid - expect(subject.errors[:options]).to include("Loadbalancing 'random' is not supported") + expect(subject.errors[:options]).to include("Loadbalancing must be one of 'round-robin, least-connections' if present") end end end diff --git a/spec/unit/messages/route_update_message_spec.rb b/spec/unit/messages/route_update_message_spec.rb index 60b26826b26..730b87d2ea0 100644 --- a/spec/unit/messages/route_update_message_spec.rb +++ b/spec/unit/messages/route_update_message_spec.rb @@ -53,7 +53,7 @@ module VCAP::CloudController it 'does not accept unknown load-balancing algorithm' do message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'cheesecake' })) expect(message).not_to be_valid - expect(message.errors.full_messages[0]).to include("Options Loadbalancing 'cheesecake' is not supported") + expect(message.errors.full_messages[0]).to include("Options Loadbalancing must be one of 'round-robin, least-connections' if present") end it 'does not accept unknown option' do diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 6388c0234a9..af49e16dc24 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -575,7 +575,7 @@ def options_message it 'adds invalid load balancer error message to the base class' do message = OptionsMessage.new({ options: { loadbalancing: 'donuts' } }) expect(message).not_to be_valid - expect(message.errors_on(:options)).to include('Loadbalancing \'donuts\' is not supported') + expect(message.errors_on(:options)).to include("Loadbalancing must be one of 'round-robin, least-connections' if present") end it 'adds invalid field error message to the base class' do