From 2c8e8540925edeb7d7a2dc20626c605326963c28 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Tue, 16 Dec 2025 17:13:52 +0100 Subject: [PATCH 01/45] Add loadbalancing option hash, and hash_header/hash_balance. Add validations --- .../manifest_routes_update_message.rb | 16 ++-- app/messages/route_options_message.rb | 59 ++++++++++-- app/models/runtime/feature_flag.rb | 3 +- .../includes/resources/routes/_create.md.erb | 29 +++++- .../routes/_route_options_object.md.erb | 4 +- .../documentation/feature_flags_api_spec.rb | 2 +- spec/unit/messages/validators_spec.rb | 91 +++++++++++++++++++ 7 files changed, 184 insertions(+), 20 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index bc4c6b9cb5e..0a32bec7a8f 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -58,10 +58,10 @@ def route_options_are_valid end r[:options].each_key do |key| - RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.exclude?(key) && + RouteOptionsMessage.valid_route_options.exclude?(key) && errors.add(:base, - message: "Route '#{r[:route]}' contains invalid route option '#{key}'. \ -Valid keys: '#{RouteOptionsMessage::VALID_MANIFEST_ROUTE_OPTIONS.join(', ')}'") + message: "Route '#{r[:route]}' contains invalid route option '#{key}'. \ +Valid keys: '#{RouteOptionsMessage.valid_route_options.join(', ')}'") end end end @@ -75,14 +75,14 @@ def loadbalancings_are_valid 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(', ')}'") + 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) && + RouteOptionsMessage.valid_loadbalancing_algorithms.exclude?(loadbalancing) && errors.add(:base, - message: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \ -Valid values are: '#{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 d2fd43e28a6..775c53ccece 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -2,15 +2,58 @@ module VCAP::CloudController class RouteOptionsMessage < BaseMessage - VALID_MANIFEST_ROUTE_OPTIONS = %i[loadbalancing].freeze - VALID_ROUTE_OPTIONS = %i[loadbalancing].freeze - VALID_LOADBALANCING_ALGORITHMS = %w[round-robin least-connection].freeze + # Register all possible keys upfront so attr_accessors are created + register_allowed_keys %i[loadbalancing hash_header hash_balance] - register_allowed_keys VALID_ROUTE_OPTIONS + def self.valid_route_options + options = %i[loadbalancing] + if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + options += %i[hash_header hash_balance] + end + options.freeze + end + + def self.valid_loadbalancing_algorithms + algorithms = %w[round-robin least-connection] + algorithms << 'hash' if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + algorithms.freeze + end + + def self.allowed_keys + valid_route_options + end validates_with NoAdditionalKeysValidator - validates :loadbalancing, - inclusion: { in: VALID_LOADBALANCING_ALGORITHMS, message: "must be one of '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}' if present" }, - presence: true, - allow_nil: true + validate :loadbalancing_algorithm_is_valid + validate :hash_options_only_with_hash_loadbalancing + + def loadbalancing_algorithm_is_valid + return if loadbalancing.nil? + return if self.class.valid_loadbalancing_algorithms.include?(loadbalancing) + + errors.add(:loadbalancing, "must be one of '#{self.class.valid_loadbalancing_algorithms.join(', ')}' if present") + end + + def hash_options_only_with_hash_loadbalancing + # When feature flag is disabled, these options are not allowed at all + feature_enabled = VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + + if hash_header.present? && !feature_enabled + errors.add(:base, 'Hash header can only be set when loadbalancing is hash') + return + end + + if hash_balance.present? && !feature_enabled + errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') + return + end + + if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' + errors.add(:base, 'Hash header can only be set when loadbalancing is hash') + end + + if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' + errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') + end + end end end diff --git a/app/models/runtime/feature_flag.rb b/app/models/runtime/feature_flag.rb index 88a5c24a353..df991d89f7e 100644 --- a/app/models/runtime/feature_flag.rb +++ b/app/models/runtime/feature_flag.rb @@ -23,7 +23,8 @@ class UndefinedFeatureFlagError < StandardError service_instance_sharing: false, hide_marketplace_from_unauthenticated_users: false, resource_matching: true, - route_sharing: false + route_sharing: false, + hash_based_routing: false }.freeze ADMIN_SKIPPABLE = %i[ diff --git a/docs/v3/source/includes/resources/routes/_create.md.erb b/docs/v3/source/includes/resources/routes/_create.md.erb index 6ad5b93a9f9..702ce526d7c 100644 --- a/docs/v3/source/includes/resources/routes/_create.md.erb +++ b/docs/v3/source/includes/resources/routes/_create.md.erb @@ -23,7 +23,7 @@ curl "https://api.example.org/v3/routes" \ }, "options": { "loadbalancing": "round-robin" - } + }, "metadata": { "labels": { "key": "value" }, "annotations": { "note": "detailed information"} @@ -71,3 +71,30 @@ Admin | Space Developer | Space Supporter | +#### Example with hash-based routing + +```shell +curl "https://api.example.org/v3/routes" \ + -X POST \ + -H "Authorization: bearer [token]" \ + -H "Content-type: application/json" \ + -d '{ + "host": "user-specific-app", + "relationships": { + "domain": { + "data": { "guid": "domain-guid" } + }, + "space": { + "data": { "guid": "space-guid" } + } + }, + "options": { + "loadbalancing": "hash", + "hash_header": "X-User-ID", + "hash_balance": "50.0" + } + }' +``` + +This creates a route that uses hash-based routing on the `X-User-ID` header with a load balance factor of 50.0. + 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 01fc9c82b56..1842fc46288 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,6 @@ Example Route-Options object | Name | Type | Description | |-------------------|----------|------------------------------------------------------------------------------------------------------| -| **loadbalancing** | _string_ | The load-balancer associated with this route. Valid values are 'round-robin' and 'least-connection' | +| **loadbalancing** | _string_ | The load-balancer associated with this route. Valid values are 'round-robin', 'least-connection', and 'hash' | +| **hash_header** | _string_ | HTTP header name to hash for routing (e.g., 'X-User-ID', 'Cookie'). Required when loadbalancing is 'hash'. Cannot be set when loadbalancing is not 'hash'. | +| **hash_balance** | _string_ | Weight factor for load balancing (0.0-100.0). 0.0 means ignore server load, higher values consider load more. Optional when loadbalancing is 'hash'. Cannot be set when loadbalancing is not 'hash'. | diff --git a/spec/api/documentation/feature_flags_api_spec.rb b/spec/api/documentation/feature_flags_api_spec.rb index ce55fbd2c6b..5fe6740eb30 100644 --- a/spec/api/documentation/feature_flags_api_spec.rb +++ b/spec/api/documentation/feature_flags_api_spec.rb @@ -18,7 +18,7 @@ client.get '/v2/config/feature_flags', {}, headers expect(status).to eq(200) - expect(parsed_response.length).to eq(18) + expect(parsed_response.length).to eq(19) expect(parsed_response).to include( { 'name' => 'user_org_creation', diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index a92ac3ced52..e396775303e 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -3,6 +3,7 @@ require 'messages/base_message' require 'messages/empty_lifecycle_data_message' require 'messages/buildpack_lifecycle_data_message' +require 'messages/route_options_message' require 'cloud_controller/diego/lifecycles/app_docker_lifecycle' require 'cloud_controller/diego/lifecycles/app_buildpack_lifecycle' require 'cloud_controller/diego/lifecycles/lifecycles' @@ -530,6 +531,22 @@ class Relationships < VCAP::CloudController::BaseMessage end describe 'OptionsValidator' do + # Stub the FeatureFlag class for lightweight tests + before do + unless defined?(VCAP::CloudController::FeatureFlag) + feature_flag_class = Class.new do + def self.enabled?(flag_name, **) + # Default: hash_based_routing is disabled + return false if flag_name == :hash_based_routing + + false + end + end + + stub_const('VCAP::CloudController::FeatureFlag', feature_flag_class) + end + end + class OptionsMessage < VCAP::CloudController::BaseMessage register_allowed_keys [:options] @@ -555,6 +572,21 @@ def options_message expect(message).to be_valid end + it 'adds invalid message for hash load-balancing algorithm' do + message = OptionsMessage.new({ options: { loadbalancing: 'hash' } }) + expect(message).not_to be_valid + end + + it 'adds invalid message for hash_header option' do + message = OptionsMessage.new({ options: { hash_header: 'X-Potatoes' } }) + expect(message).not_to be_valid + end + + it 'adds invalid message for hash_balance option' do + message = OptionsMessage.new({ options: { hash_balance: '1.2' } }) + expect(message).not_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 @@ -583,6 +615,65 @@ def options_message expect(message).not_to be_valid expect(message.errors_on(:options)).to include('Unknown field(s): \'cookies\'') end + + context 'when hash_based_routing feature flag is disabled' do + it 'does not allow hash_header option' do + message = OptionsMessage.new({ options: { hash_header: 'X-User-ID' } }) + expect(message).not_to be_valid + expect(message.errors_on(:options)).to include("Hash header can only be set when loadbalancing is hash") + end + + it 'does not allow hash_balance option' do + message = OptionsMessage.new({ options: { hash_balance: '1.5' } }) + expect(message).not_to be_valid + expect(message.errors_on(:options)).to include("Hash balance can only be set when loadbalancing is hash") + end + + it 'does not allow hash load-balancing algorithm' do + message = OptionsMessage.new({ options: { loadbalancing: 'hash' } }) + expect(message).not_to be_valid + expect(message.errors_on(:options)).to include("Loadbalancing must be one of 'round-robin, least-connection' if present") + end + end + + context 'when hash_based_routing feature flag is enabled' do + before do + feature_flag_class = Class.new do + def self.enabled?(flag_name, **) + return true if flag_name == :hash_based_routing + + false + end + end + + stub_const('VCAP::CloudController::FeatureFlag', feature_flag_class) + end + + it 'allows hash loadbalancing option' do + message = OptionsMessage.new({ options: { loadbalancing: 'hash' } }) + expect(message).to be_valid + end + + it 'allows hash_balance option' do + message = OptionsMessage.new({ options: { hash_balance: '1.5' } }) + expect(message).to be_valid + end + + it 'allows hash_header option' do + message = OptionsMessage.new({ options: { hash_header: 'X-User-ID' } }) + expect(message).to be_valid + end + + it 'does not allow hash_header without hash load-balancing' do + message = OptionsMessage.new({ options: { loadbalancing: 'round-robin', hash_header: 'X-User-ID' } }) + expect(message).not_to be_valid + end + + it 'does not allow hash_balance without hash load-balancing' do + message = OptionsMessage.new({ options: { loadbalancing: 'round-robin', hash_balance: '1.5' } }) + expect(message).not_to be_valid + end + end end describe 'ToOneRelationshipValidator' do From 9d751e4dead89197a10f0a52731c4a769e5c5432 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Tue, 16 Dec 2025 17:41:36 +0100 Subject: [PATCH 02/45] Add basic options validation to manifest_routes_update_message --- .../manifest_routes_update_message.rb | 40 ++++++++++++++++++- app/messages/route_options_message.rb | 7 +++- spec/unit/messages/validators_spec.rb | 2 + 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 0a32bec7a8f..720914ded27 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -28,6 +28,7 @@ def contains_non_route_hash_values?(routes) validate :route_protocols_are_valid, if: proc { |record| record.requested?(:routes) } validate :route_options_are_valid, if: proc { |record| record.requested?(:routes) } validate :loadbalancings_are_valid, if: proc { |record| record.requested?(:routes) } + validate :hash_options_are_valid, if: proc { |record| record.requested?(:routes) } validate :no_route_is_boolean validate :default_route_is_boolean validate :random_route_is_boolean @@ -75,17 +76,52 @@ def loadbalancings_are_valid loadbalancing = r[:options][:loadbalancing] unless loadbalancing.is_a?(String) errors.add(:base, - message: "Invalid value for 'loadbalancing' for Route '#{r[:route]}'; \ + 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: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \ + message: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \ Valid values are: '#{RouteOptionsMessage.valid_loadbalancing_algorithms.join(', ')}'") end end + def hash_options_are_valid + return if errors[:routes].present? + + feature_enabled = VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + + routes.each do |r| + next unless r.keys.include?(:options) && r[:options].is_a?(Hash) + + options = r[:options] + loadbalancing = options[:loadbalancing] + hash_header = options[:hash_header] + hash_balance = options[:hash_balance] + + # When feature flag is disabled, hash options are not allowed at all + if hash_header.present? && !feature_enabled + errors.add(:base, message: "Route '#{r[:route]}': Hash header can only be set when loadbalancing is hash") + next + end + + if hash_balance.present? && !feature_enabled + errors.add(:base, message: "Route '#{r[:route]}': Hash balance can only be set when loadbalancing is hash") + next + end + + # When loadbalancing is explicitly set to non-hash value, hash options are not allowed + if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' + errors.add(:base, message: "Route '#{r[:route]}': Hash header can only be set when loadbalancing is hash") + end + + if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' + errors.add(:base, message: "Route '#{r[:route]}': Hash balance can only be set when loadbalancing is hash") + end + end + end + def routes_are_uris return if errors[:routes].present? diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 775c53ccece..dc7885230ab 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -34,7 +34,11 @@ def loadbalancing_algorithm_is_valid end def hash_options_only_with_hash_loadbalancing - # When feature flag is disabled, these options are not allowed at all + # We cannot rely solely on NoAdditionalKeysValidator because: + # 1. register_allowed_keys sets the ALLOWED_KEYS constant at class load time + # 2. Even though we override allowed_keys method, the constant is already set + # 3. We need explicit runtime validation based on feature flag + feature_enabled = VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) if hash_header.present? && !feature_enabled @@ -47,6 +51,7 @@ def hash_options_only_with_hash_loadbalancing return end + # When loadbalancing is explicitly set to non-hash value, hash options are not allowed if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' errors.add(:base, 'Hash header can only be set when loadbalancing is hash') end diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index e396775303e..cd24153b07f 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -580,11 +580,13 @@ def options_message it 'adds invalid message for hash_header option' do message = OptionsMessage.new({ options: { hash_header: 'X-Potatoes' } }) expect(message).not_to be_valid + expect(message.errors_on(:options)).to include("Hash header can only be set when loadbalancing is hash") end it 'adds invalid message for hash_balance option' do message = OptionsMessage.new({ options: { hash_balance: '1.2' } }) expect(message).not_to be_valid + expect(message.errors_on(:options)).to include("Hash balance can only be set when loadbalancing is hash") end it 'adds invalid options message when options is null' do From a8e3b884b0bb04ba8580d90d7673fad87a4a0264 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Tue, 16 Dec 2025 17:50:10 +0100 Subject: [PATCH 03/45] Add basic tests for manifest route updates --- .../manifest_routes_update_message.rb | 6 +- .../manifest_routes_update_message_spec.rb | 217 ++++++++++++++++++ 2 files changed, 220 insertions(+), 3 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 720914ded27..7ed67ce83c5 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -90,7 +90,7 @@ def loadbalancings_are_valid def hash_options_are_valid return if errors[:routes].present? - feature_enabled = VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + hash_based_routing_feature_enabled = VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) routes.each do |r| next unless r.keys.include?(:options) && r[:options].is_a?(Hash) @@ -101,12 +101,12 @@ def hash_options_are_valid hash_balance = options[:hash_balance] # When feature flag is disabled, hash options are not allowed at all - if hash_header.present? && !feature_enabled + if hash_header.present? && !hash_based_routing_feature_enabled errors.add(:base, message: "Route '#{r[:route]}': Hash header can only be set when loadbalancing is hash") next end - if hash_balance.present? && !feature_enabled + if hash_balance.present? && !hash_based_routing_feature_enabled errors.add(:base, message: "Route '#{r[:route]}': Hash balance can only be set when loadbalancing is hash") next end diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index f10b4050303..18904ade947 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -316,6 +316,223 @@ module VCAP::CloudController expect(msg.errors.full_messages).to include("Cannot use loadbalancing value 'sushi' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connection'") end end + + context 'hash options validation' do + context 'when hash_based_routing feature flag is disabled' do + before do + VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: false) + end + + context 'when a route contains hash_header' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'hash_header' => 'X-User-ID' + } } + ] } + 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': Hash header can only be set when loadbalancing is hash") + end + end + + context 'when a route contains hash_balance' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'hash_balance' => '1.5' + } } + ] } + 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': Hash balance can only be set when loadbalancing is hash") + end + end + + context 'when a route contains both hash_header and hash_balance' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'hash_header' => 'X-User-ID', + 'hash_balance' => '1.5' + } } + ] } + end + + it 'returns false with appropriate error' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(false) + expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash header can only be set when loadbalancing is hash") + end + end + end + + context 'when hash_based_routing feature flag is enabled' do + before do + VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: true) + end + + context 'when a route contains hash_header with hash loadbalancing' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID' + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains hash_balance with hash loadbalancing' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => '1.5' + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains hash_header without loadbalancing' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'hash_header' => 'X-User-ID' + } } + ] } + end + + it 'returns true (loadbalancing is omitted)' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains hash_balance without loadbalancing' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'hash_header' => 'X-User-ID', + 'hash_balance' => '2.0' + } } + ] } + end + + it 'returns true (loadbalancing is omitted)' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains hash_header with non-hash loadbalancing' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'round-robin', + 'hash_header' => 'X-User-ID' + } } + ] } + 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': Hash header can only be set when loadbalancing is hash") + end + end + + context 'when a route contains hash_balance with non-hash loadbalancing' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'least-connection', + 'hash_balance' => '1.5' + } } + ] } + 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': Hash balance can only be set when loadbalancing is hash") + end + end + + context 'when multiple routes have mixed valid and invalid hash options' do + let(:body) do + { 'routes' => + [ + { 'route' => 'valid1.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID' + } }, + { 'route' => 'invalid.example.com', + 'options' => { + 'loadbalancing' => 'round-robin', + 'hash_header' => 'X-User-ID' + } }, + { 'route' => 'valid2.example.com', + 'options' => { + 'hash_header' => 'X-Session-ID' + } } + ] } + end + + it 'returns false and reports the invalid route' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(false) + expect(msg.errors.full_messages).to include("Route 'invalid.example.com': Hash header can only be set when loadbalancing is hash") + end + end + end + end end end end From 1e63c79040205cfea65a7727c062cb52e83bde2b Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 17 Dec 2025 10:45:23 +0100 Subject: [PATCH 04/45] Simplify validations, improve error messages, adjust tests --- .../manifest_routes_update_message.rb | 14 ++--- app/messages/route_options_message.rb | 31 +++++------ .../manifest_routes_update_message_spec.rb | 54 +++++++++++++------ spec/unit/messages/validators_spec.rb | 11 ++-- 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 7ed67ce83c5..86618be37d7 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -90,7 +90,9 @@ def loadbalancings_are_valid def hash_options_are_valid return if errors[:routes].present? - hash_based_routing_feature_enabled = VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + # Note: route_options_are_valid already validates that hash_header and hash_balance + # are only allowed when the feature flag is enabled (via valid_route_options). + # Here we only validate that they aren't used with non-hash loadbalancing algorithms. routes.each do |r| next unless r.keys.include?(:options) && r[:options].is_a?(Hash) @@ -100,16 +102,6 @@ def hash_options_are_valid hash_header = options[:hash_header] hash_balance = options[:hash_balance] - # When feature flag is disabled, hash options are not allowed at all - if hash_header.present? && !hash_based_routing_feature_enabled - errors.add(:base, message: "Route '#{r[:route]}': Hash header can only be set when loadbalancing is hash") - next - end - - if hash_balance.present? && !hash_based_routing_feature_enabled - errors.add(:base, message: "Route '#{r[:route]}': Hash balance can only be set when loadbalancing is hash") - next - end # When loadbalancing is explicitly set to non-hash value, hash options are not allowed if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index dc7885230ab..5bde9d2665f 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -19,9 +19,6 @@ def self.valid_loadbalancing_algorithms algorithms.freeze end - def self.allowed_keys - valid_route_options - end validates_with NoAdditionalKeysValidator validate :loadbalancing_algorithm_is_valid validate :hash_options_only_with_hash_loadbalancing @@ -34,21 +31,19 @@ def loadbalancing_algorithm_is_valid end def hash_options_only_with_hash_loadbalancing - # We cannot rely solely on NoAdditionalKeysValidator because: - # 1. register_allowed_keys sets the ALLOWED_KEYS constant at class load time - # 2. Even though we override allowed_keys method, the constant is already set - # 3. We need explicit runtime validation based on feature flag - - feature_enabled = VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) - - if hash_header.present? && !feature_enabled - errors.add(:base, 'Hash header can only be set when loadbalancing is hash') - return - end - - if hash_balance.present? && !feature_enabled - errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') - return + # Check if hash options are allowed (feature flag check via valid_route_options) + valid_options = self.class.valid_route_options + + # Check all requested keys (options that were actually provided) + # Check needs to be done manually, as the set of allowed options may change during runtime (feature flag) + requested_keys.each do |key| + value = public_send(key) if respond_to?(key) + next unless value.present? + + unless valid_options.include?(key) + errors.add(:base, "Unknown field(s): '#{key}'") + return + end end # When loadbalancing is explicitly set to non-hash value, hash options are not allowed diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index 18904ade947..7174b0178ec 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -212,10 +212,10 @@ module VCAP::CloudController context 'when a route contains empty route options' do let(:body) do { 'routes' => - [ - { 'route' => 'existing.example.com', - 'options' => {} } - ] } + [ + { 'route' => 'existing.example.com', + 'options' => {} } + ] } end it 'returns true' do @@ -245,10 +245,10 @@ module VCAP::CloudController context 'when a route contains invalid route options' do let(:body) do { 'routes' => - [ - { 'route' => 'existing.example.com', - 'options' => { 'invalid' => 'invalid' } } - ] } + [ + { 'route' => 'existing.example.com', + 'options' => { 'invalid' => 'invalid' } } + ] } end it 'returns true' do @@ -263,12 +263,12 @@ module VCAP::CloudController context 'when a route contains a valid value for loadbalancing' do let(:body) do { 'routes' => - [ - { 'route' => 'existing.example.com', - 'options' => { - 'loadbalancing' => 'round-robin' - } } - ] } + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'round-robin' + } } + ] } end it 'returns true' do @@ -323,6 +323,24 @@ module VCAP::CloudController VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: false) end + context 'when a route contains loadbalancing=hash' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash' + } } + ] } + end + it 'returns false' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(false) + expect(msg.errors.full_messages).to include("Cannot use loadbalancing value 'hash' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connection'") + end + end + context 'when a route contains hash_header' do let(:body) do { 'routes' => @@ -338,7 +356,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': Hash header can only be set when loadbalancing is hash") + expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid route option 'hash_header'. Valid keys: 'loadbalancing'") end end @@ -357,7 +375,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': Hash balance can only be set when loadbalancing is hash") + expect(msg.errors.full_messages).to include("Route 'existing.example.com' contains invalid route option 'hash_balance'. Valid keys: 'loadbalancing'") end end @@ -377,7 +395,8 @@ module VCAP::CloudController msg = ManifestRoutesUpdateMessage.new(body) expect(msg.valid?).to be(false) - expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash header can only be set when loadbalancing is hash") + # Should get error for invalid route options (both hash_header and hash_balance are invalid) + expect(msg.errors.full_messages.any? { |m| m.include?("contains invalid route option") }).to be(true) end end end @@ -536,3 +555,4 @@ module VCAP::CloudController end end end + diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index cd24153b07f..86a2ac4e85b 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -580,13 +580,13 @@ def options_message it 'adds invalid message for hash_header option' do message = OptionsMessage.new({ options: { hash_header: 'X-Potatoes' } }) expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Hash header can only be set when loadbalancing is hash") + expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_header'") end it 'adds invalid message for hash_balance option' do message = OptionsMessage.new({ options: { hash_balance: '1.2' } }) expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Hash balance can only be set when loadbalancing is hash") + expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_balance'") end it 'adds invalid options message when options is null' do @@ -622,13 +622,13 @@ def options_message it 'does not allow hash_header option' do message = OptionsMessage.new({ options: { hash_header: 'X-User-ID' } }) expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Hash header can only be set when loadbalancing is hash") + expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_header'") end it 'does not allow hash_balance option' do message = OptionsMessage.new({ options: { hash_balance: '1.5' } }) expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Hash balance can only be set when loadbalancing is hash") + expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_balance'") end it 'does not allow hash load-balancing algorithm' do @@ -669,12 +669,15 @@ def self.enabled?(flag_name, **) it 'does not allow hash_header without hash load-balancing' do message = OptionsMessage.new({ options: { loadbalancing: 'round-robin', hash_header: 'X-User-ID' } }) expect(message).not_to be_valid + expect(message.errors_on(:options)).to include('Hash header can only be set when loadbalancing is hash') end it 'does not allow hash_balance without hash load-balancing' do message = OptionsMessage.new({ options: { loadbalancing: 'round-robin', hash_balance: '1.5' } }) expect(message).not_to be_valid + expect(message.errors_on(:options)).to include('Hash balance can only be set when loadbalancing is hash') end + end end From 0371984135be331a7de36c192250418d5644ec17 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 17 Dec 2025 10:58:43 +0100 Subject: [PATCH 05/45] Add validation for hash balance being a float --- .../manifest_routes_update_message.rb | 8 +++ app/messages/route_options_message.rb | 10 ++++ .../manifest_routes_update_message_spec.rb | 59 +++++++++++++++++++ spec/unit/messages/validators_spec.rb | 21 +++++++ 4 files changed, 98 insertions(+) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 86618be37d7..afe9868de90 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -102,6 +102,14 @@ def hash_options_are_valid hash_header = options[:hash_header] hash_balance = options[:hash_balance] + # Validate hash_balance is numeric if present + if hash_balance.present? + begin + Float(hash_balance) + rescue ArgumentError, TypeError + errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be a numeric value") + end + end # When loadbalancing is explicitly set to non-hash value, hash options are not allowed if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 5bde9d2665f..e1b3dccd0d7 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -22,6 +22,7 @@ def self.valid_loadbalancing_algorithms validates_with NoAdditionalKeysValidator validate :loadbalancing_algorithm_is_valid validate :hash_options_only_with_hash_loadbalancing + validate :hash_balance_is_numeric def loadbalancing_algorithm_is_valid return if loadbalancing.nil? @@ -30,6 +31,15 @@ def loadbalancing_algorithm_is_valid errors.add(:loadbalancing, "must be one of '#{self.class.valid_loadbalancing_algorithms.join(', ')}' if present") end + def hash_balance_is_numeric + return if hash_balance.nil? + + # Try to convert to float + Float(hash_balance) + rescue ArgumentError, TypeError + errors.add(:hash_balance, 'must be a numeric value') + end + def hash_options_only_with_hash_loadbalancing # Check if hash options are allowed (feature flag check via valid_route_options) valid_options = self.class.valid_route_options diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index 7174b0178ec..939fc241bfd 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -522,6 +522,65 @@ module VCAP::CloudController end end + context 'when a route contains non-numeric hash_balance' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'hash_balance' => 'not-a-number' + } } + ] } + 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': Hash balance must be a numeric value") + end + end + + context 'when a route contains numeric string hash_balance' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => '2.5' + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains float hash_balance' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => 1.5 + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + context 'when multiple routes have mixed valid and invalid hash options' do let(:body) do { 'routes' => diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 86a2ac4e85b..3d5c08bd0b1 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -678,6 +678,27 @@ def self.enabled?(flag_name, **) expect(message.errors_on(:options)).to include('Hash balance can only be set when loadbalancing is hash') end + it 'does not allow non-numeric hash_balance' do + message = OptionsMessage.new({ options: { hash_balance: 'not-a-number' } }) + expect(message).not_to be_valid + expect(message.errors.full_messages).to include('Options Hash balance must be a numeric value') + end + + it 'allows numeric string hash_balance' do + message = OptionsMessage.new({ options: { hash_balance: '2.5' } }) + expect(message).to be_valid + end + + it 'allows integer string hash_balance' do + message = OptionsMessage.new({ options: { hash_balance: '3' } }) + expect(message).to be_valid + end + + it 'allows float hash_balance' do + message = OptionsMessage.new({ options: { hash_balance: 1.5 } }) + expect(message).to be_valid + end + end end From afd8569a7cadc23a03c6a9fd91815ea73d5ad0df Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 17 Dec 2025 14:11:17 +0100 Subject: [PATCH 06/45] Have consistent validations in manifest_routes_updates_message and route_options_message --- .../manifest_routes_update_message.rb | 4 +++ app/messages/route_options_message.rb | 34 +++++++++++-------- .../manifest_routes_update_message_spec.rb | 22 ++++++++++++ 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index afe9868de90..fa7b082445f 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -90,6 +90,10 @@ def loadbalancings_are_valid def hash_options_are_valid return if errors[:routes].present? + # Only validate hash-specific options when the feature flag is enabled + # If disabled, route_options_are_valid will already report them as invalid + return unless VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + # Note: route_options_are_valid already validates that hash_header and hash_balance # are only allowed when the feature flag is enabled (via valid_route_options). # Here we only validate that they aren't used with non-hash loadbalancing algorithms. diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index e1b3dccd0d7..41b75d6dde7 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -21,8 +21,8 @@ def self.valid_loadbalancing_algorithms validates_with NoAdditionalKeysValidator validate :loadbalancing_algorithm_is_valid - validate :hash_options_only_with_hash_loadbalancing - validate :hash_balance_is_numeric + validate :route_options_are_valid + validate :hash_options_are_valid def loadbalancing_algorithm_is_valid return if loadbalancing.nil? @@ -31,20 +31,10 @@ def loadbalancing_algorithm_is_valid errors.add(:loadbalancing, "must be one of '#{self.class.valid_loadbalancing_algorithms.join(', ')}' if present") end - def hash_balance_is_numeric - return if hash_balance.nil? - - # Try to convert to float - Float(hash_balance) - rescue ArgumentError, TypeError - errors.add(:hash_balance, 'must be a numeric value') - end - - def hash_options_only_with_hash_loadbalancing - # Check if hash options are allowed (feature flag check via valid_route_options) + def route_options_are_valid valid_options = self.class.valid_route_options - # Check all requested keys (options that were actually provided) + # Check if any requested options are not in valid_route_options # Check needs to be done manually, as the set of allowed options may change during runtime (feature flag) requested_keys.each do |key| value = public_send(key) if respond_to?(key) @@ -55,6 +45,22 @@ def hash_options_only_with_hash_loadbalancing return end end + end + + def hash_options_are_valid + # Only validate hash-specific options when the feature flag is enabled + # If disabled, route_options_are_valid will already report them as unknown fields + return unless VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) + + # Feature flag is enabled - validate hash-specific options + # Validate hash_balance is numeric if present + if hash_balance.present? + begin + Float(hash_balance) + rescue ArgumentError, TypeError + errors.add(:hash_balance, 'must be a numeric value') + end + end # When loadbalancing is explicitly set to non-hash value, hash options are not allowed if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index 939fc241bfd..eb72f50a9d4 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -379,6 +379,28 @@ module VCAP::CloudController end end + context 'when a route contains hash_balance with round-robin loadbalancing' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'round-robin', + 'hash_balance' => '1.2' + } } + ] } + end + + it 'returns false with only one error (invalid option), and skips hash-based routing specific error messages' do + 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 'hash_balance'. Valid keys: 'loadbalancing'") + expect(msg.errors.full_messages).not_to include("Route 'existing.example.com': Hash balance can only be set when loadbalancing is hash") + expect(msg.errors.full_messages.length).to eq(1) + end + end + context 'when a route contains both hash_header and hash_balance' do let(:body) do { 'routes' => From 41fb78a59496ef6f0791c97c3d115a8e1f089f9a Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 17 Dec 2025 14:22:52 +0100 Subject: [PATCH 07/45] Minor refactoring --- .../manifest_routes_update_message.rb | 7 +++--- app/messages/route_options_message.rb | 24 ++++++------------- .../manifest_routes_update_message_spec.rb | 10 ++++---- spec/unit/messages/validators_spec.rb | 7 +++++- 4 files changed, 22 insertions(+), 26 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index fa7b082445f..ffd18b35ebe 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -76,13 +76,13 @@ def loadbalancings_are_valid loadbalancing = r[:options][:loadbalancing] unless loadbalancing.is_a?(String) errors.add(:base, - message: "Invalid value for 'loadbalancing' for Route '#{r[:route]}'; \ + 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: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \ + message: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \ Valid values are: '#{RouteOptionsMessage.valid_loadbalancing_algorithms.join(', ')}'") end end @@ -94,9 +94,8 @@ def hash_options_are_valid # If disabled, route_options_are_valid will already report them as invalid return unless VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) - # Note: route_options_are_valid already validates that hash_header and hash_balance + # NOTE: route_options_are_valid already validates that hash_header and hash_balance # are only allowed when the feature flag is enabled (via valid_route_options). - # Here we only validate that they aren't used with non-hash loadbalancing algorithms. routes.each do |r| next unless r.keys.include?(:options) && r[:options].is_a?(Hash) diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 41b75d6dde7..b67d3e08749 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -7,9 +7,7 @@ class RouteOptionsMessage < BaseMessage def self.valid_route_options options = %i[loadbalancing] - if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) - options += %i[hash_header hash_balance] - end + options += %i[hash_header hash_balance] if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) options.freeze end @@ -36,15 +34,12 @@ def route_options_are_valid # Check if any requested options are not in valid_route_options # Check needs to be done manually, as the set of allowed options may change during runtime (feature flag) - requested_keys.each do |key| + invalid_keys = requested_keys.select do |key| value = public_send(key) if respond_to?(key) - next unless value.present? - - unless valid_options.include?(key) - errors.add(:base, "Unknown field(s): '#{key}'") - return - end + value.present? && valid_options.exclude?(key) end + + errors.add(:base, "Unknown field(s): '#{invalid_keys.join("', '")}'") if invalid_keys.any? end def hash_options_are_valid @@ -63,13 +58,8 @@ def hash_options_are_valid end # When loadbalancing is explicitly set to non-hash value, hash options are not allowed - if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' - errors.add(:base, 'Hash header can only be set when loadbalancing is hash') - end - - if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' - errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') - end + errors.add(:base, 'Hash header can only be set when loadbalancing is hash') if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' + errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' 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 eb72f50a9d4..5206d42efd7 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -333,11 +333,13 @@ module VCAP::CloudController } } ] } end + it 'returns false' do msg = ManifestRoutesUpdateMessage.new(body) expect(msg.valid?).to be(false) - expect(msg.errors.full_messages).to include("Cannot use loadbalancing value 'hash' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connection'") + expect(msg.errors.full_messages).to include( + "Cannot use loadbalancing value 'hash' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connection'") end end @@ -356,7 +358,8 @@ 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 'hash_header'. Valid keys: 'loadbalancing'") + expect(msg.errors.full_messages).to include( + "Route 'existing.example.com' contains invalid route option 'hash_header'. Valid keys: 'loadbalancing'") end end @@ -418,7 +421,7 @@ module VCAP::CloudController expect(msg.valid?).to be(false) # Should get error for invalid route options (both hash_header and hash_balance are invalid) - expect(msg.errors.full_messages.any? { |m| m.include?("contains invalid route option") }).to be(true) + expect(msg.errors.full_messages.any? { |m| m.include?('contains invalid route option') }).to be(true) end end end @@ -636,4 +639,3 @@ module VCAP::CloudController end end end - diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 3d5c08bd0b1..37278301738 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -631,6 +631,12 @@ def options_message expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_balance'") end + it 'reports multiple invalid keys together' do + message = OptionsMessage.new({ options: { hash_header: 'X-User-ID', hash_balance: '1.5' } }) + expect(message).not_to be_valid + expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_header', 'hash_balance'") + end + it 'does not allow hash load-balancing algorithm' do message = OptionsMessage.new({ options: { loadbalancing: 'hash' } }) expect(message).not_to be_valid @@ -698,7 +704,6 @@ def self.enabled?(flag_name, **) message = OptionsMessage.new({ options: { hash_balance: 1.5 } }) expect(message).to be_valid end - end end From 16e66df58096d7416f7cf1076271b2bbcc65965d Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 17 Dec 2025 16:03:39 +0100 Subject: [PATCH 08/45] Add check for hash_balance value being 0 or >=1.1 --- .../manifest_routes_update_message.rb | 6 +- app/messages/route_options_message.rb | 6 +- .../manifest_routes_update_message_spec.rb | 81 +++++++++++++++++++ spec/unit/messages/validators_spec.rb | 21 +++++ 4 files changed, 112 insertions(+), 2 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index ffd18b35ebe..45d1e893642 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -108,7 +108,11 @@ def hash_options_are_valid # Validate hash_balance is numeric if present if hash_balance.present? begin - Float(hash_balance) + balance_float = Float(hash_balance) + # Must be either 0 or >= 1.1 + unless balance_float == 0 || balance_float >= 1.1 + errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be either 0 or greater than or equal to 1.1") + end rescue ArgumentError, TypeError errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be a numeric value") end diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index b67d3e08749..69b4eedf9c8 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -51,7 +51,11 @@ def hash_options_are_valid # Validate hash_balance is numeric if present if hash_balance.present? begin - Float(hash_balance) + balance_float = Float(hash_balance) + # Must be either 0 or >= 1.1 + unless balance_float == 0 || balance_float >= 1.1 + errors.add(:hash_balance, 'must be either 0 or greater than or equal to 1.1') + end rescue ArgumentError, TypeError errors.add(:hash_balance, 'must be a numeric value') end diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index 5206d42efd7..e5d1e8322d1 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -566,6 +566,87 @@ module VCAP::CloudController end end + context 'when a route contains hash_balance of 0' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => 0 + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains hash_balance between 0 and 1.1' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => 0.5 + } } + ] } + 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': Hash balance must be either 0 or greater than or equal to 1.1") + end + end + + context 'when a route contains hash_balance of 1.1' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => 1.1 + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + + context 'when a route contains hash_balance greater than 1.1' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => 3.5 + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + context 'when a route contains numeric string hash_balance' do let(:body) do { 'routes' => diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 37278301738..5c7099687ff 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -690,6 +690,27 @@ def self.enabled?(flag_name, **) expect(message.errors.full_messages).to include('Options Hash balance must be a numeric value') end + it 'allows hash_balance of 0' do + message = OptionsMessage.new({ options: { hash_balance: 0 } }) + expect(message).to be_valid + end + + it 'allows hash_balance of 1.1' do + message = OptionsMessage.new({ options: { hash_balance: 1.1 } }) + expect(message).to be_valid + end + + it 'allows hash_balance greater than 1.1' do + message = OptionsMessage.new({ options: { hash_balance: 2.5 } }) + expect(message).to be_valid + end + + it 'does not allow hash_balance between 0 and 1.1' do + message = OptionsMessage.new({ options: { hash_balance: 0.5 } }) + expect(message).not_to be_valid + expect(message.errors.full_messages).to include('Options Hash balance must be either 0 or greater than or equal to 1.1') + end + it 'allows numeric string hash_balance' do message = OptionsMessage.new({ options: { hash_balance: '2.5' } }) expect(message).to be_valid From a7a8862bed534609a0bc0e9a76577d6fe5207675 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 17 Dec 2025 16:28:09 +0100 Subject: [PATCH 09/45] Transform hash_balance to string in the route model before saving --- app/models/runtime/route.rb | 14 ++- spec/unit/models/runtime/route_spec.rb | 116 +++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index 70fd484ca42..e8ea683f285 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -71,7 +71,8 @@ def as_summary_json end def options_with_serialization=(opts) - self.options_without_serialization = Oj.dump(opts) + normalized_opts = normalize_hash_balance_to_string(opts) + self.options_without_serialization = Oj.dump(normalized_opts) end alias_method :options_without_serialization=, :options= @@ -317,6 +318,17 @@ def validate_total_reserved_route_ports errors.add(:organization, :total_reserved_route_ports_exceeded) end + def normalize_hash_balance_to_string(opts) + return opts unless opts.is_a?(Hash) + return opts unless opts.key?('hash_balance') || opts.key?(:hash_balance) + + normalized = opts.deep_symbolize_keys + if normalized[:hash_balance].present? + normalized[:hash_balance] = normalized[:hash_balance].to_s + end + normalized + end + def validate_uniqueness_on_host_and_domain validates_unique [:host, :domain_id] do |ds| ds.where(path: '', port: 0) diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index 8e04a79e87b..ac880d27808 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -1055,6 +1055,122 @@ module VCAP::CloudController it { is_expected.to import_attributes :host, :domain_guid, :space_guid, :app_guids, :path, :port, :options } end + describe 'options normalization' do + let(:space) { Space.make } + let(:domain) { PrivateDomain.make(owning_organization: space.organization) } + + context 'when hash_balance is provided as a float' do + it 'stores hash_balance as a string in the database' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 1.5 } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['hash_balance']).to be_a(String) + expect(parsed_options['hash_balance']).to eq('1.5') + end + end + + context 'when hash_balance is provided as an integer' do + it 'stores hash_balance as a string in the database' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 2 } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['hash_balance']).to be_a(String) + expect(parsed_options['hash_balance']).to eq('2') + end + end + + context 'when hash_balance is provided as a string' do + it 'keeps hash_balance as a string in the database' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '1.25' } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['hash_balance']).to be_a(String) + expect(parsed_options['hash_balance']).to eq('1.25') + end + end + + context 'when hash_balance is 0' do + it 'stores hash_balance as a string "0" in the database' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 0 } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['hash_balance']).to be_a(String) + expect(parsed_options['hash_balance']).to eq('0') + end + end + + context 'when options do not include hash_balance' do + it 'does not add hash_balance to the options' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'round-robin' } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options).not_to have_key('hash_balance') + end + end + + context 'when options are nil' do + it 'handles nil options gracefully' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: nil + ) + + route.reload + expect(route.options).to be_nil + end + end + + context 'when updating an existing route with hash_balance as float' do + it 'converts hash_balance to string on update' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 2.5 } + ) + + route.update(options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 2.5 }) + route.reload + + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['hash_balance']).to be_a(String) + expect(parsed_options['hash_balance']).to eq('2.5') + end + end + end + describe 'instance methods' do let(:space) { Space.make } From 42b01f38a4ba5437c6d3abee529160c6487e7508 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 17 Dec 2025 16:42:58 +0100 Subject: [PATCH 10/45] Validate the route merged with the route update from the message. --- app/actions/route_update.rb | 39 ++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 3520ba17877..9ba2892b899 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -2,7 +2,24 @@ module VCAP::CloudController class RouteUpdate def update(route:, message:) Route.db.transaction do - route.options = route.options.symbolize_keys.merge(message.options).compact if message.requested?(:options) + if message.requested?(:options) + # Merge existing options with new options from message + existing_options = route.options&.deep_symbolize_keys || {} + merged_options = existing_options.merge(message.options).compact + + # Remove hash-specific options if switching to non-hash loadbalancing + if merged_options[:loadbalancing] && merged_options[:loadbalancing] != 'hash' + merged_options.delete(:hash_header) + merged_options.delete(:hash_balance) + end + + # Validate the merged options + validate_route_options!(merged_options) + + # Set the options on the route + route.options = merged_options + end + route.save MetadataUpdate.update(route, message) end @@ -14,5 +31,25 @@ def update(route:, message:) end route end + + private + + def validate_route_options!(options) + return if options.blank? + + loadbalancing = options[:loadbalancing] + return if loadbalancing != 'hash' + + hash_header = options[:hash_header] + + if hash_header.blank? + error!('Hash header must be present when loadbalancing is set to hash') + end + + end + + def error!(message) + raise Error.new(message) + end end end From dfde6ecfee4f58b28643b2622fbe8438963b3b57 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 17 Dec 2025 16:59:52 +0100 Subject: [PATCH 11/45] Attempt to forward errors in route_update validation to the client --- app/actions/manifest_route_update.rb | 2 +- app/actions/route_update.rb | 4 ++++ app/controllers/v3/routes_controller.rb | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index e00182e0295..df49e50eae8 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -52,7 +52,7 @@ def update(app_guid, message, user_audit_info) ) end end - rescue Sequel::ValidationFailed, RouteCreate::Error => e + rescue Sequel::ValidationFailed, RouteCreate::Error, RouteUpdate::Error => e raise InvalidRoute.new(e.message) end diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 9ba2892b899..25759d2c092 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -1,5 +1,8 @@ module VCAP::CloudController class RouteUpdate + class Error < StandardError + end + def update(route:, message:) Route.db.transaction do if message.requested?(:options) @@ -42,6 +45,7 @@ def validate_route_options!(options) hash_header = options[:hash_header] + if hash_header.blank? error!('Hash header must be present when loadbalancing is set to hash') end diff --git a/app/controllers/v3/routes_controller.rb b/app/controllers/v3/routes_controller.rb index 86f042dc3a7..7c7e48ce212 100644 --- a/app/controllers/v3/routes_controller.rb +++ b/app/controllers/v3/routes_controller.rb @@ -108,6 +108,8 @@ def update VCAP::CloudController::RouteUpdate.new.update(route:, message:) render status: :ok, json: Presenters::V3::RoutePresenter.new(route) + rescue RouteUpdate::Error => e + unprocessable!(e.message) end def destroy From 0710ef181fa23daa183479420d3c857221e2fac3 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 18 Dec 2025 10:24:52 +0100 Subject: [PATCH 12/45] Add route options changes to app event --- app/repositories/app_event_repository.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/repositories/app_event_repository.rb b/app/repositories/app_event_repository.rb index 84e3e4023e5..4b1c99c14a7 100644 --- a/app/repositories/app_event_repository.rb +++ b/app/repositories/app_event_repository.rb @@ -110,6 +110,7 @@ def record_map_route(user_audit_info, route_mapping, manifest_triggered: false) weight: route_mapping.weight, protocol: route_mapping.protocol }) + metadata[:route_options] = route.options if route.options.present? create_app_audit_event(EventTypes::APP_MAP_ROUTE, app, app.space, actor_hash, metadata) end @@ -126,6 +127,7 @@ def record_unmap_route(user_audit_info, route_mapping, manifest_triggered: false weight: route_mapping.weight, protocol: route_mapping.protocol }) + metadata[:route_options] = route.options if route.options.present? create_app_audit_event(EventTypes::APP_UNMAP_ROUTE, app, app.space, actor_hash, metadata) end From 6676cf0cd13b3412ae4a8ec1eb2ba5325c2ba6f7 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 18 Dec 2025 10:25:14 +0100 Subject: [PATCH 13/45] Add new hash options to mnifest route --- lib/cloud_controller/app_manifest/manifest_route.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/cloud_controller/app_manifest/manifest_route.rb b/lib/cloud_controller/app_manifest/manifest_route.rb index 113dfe99b9b..3c68ffc9fe0 100644 --- a/lib/cloud_controller/app_manifest/manifest_route.rb +++ b/lib/cloud_controller/app_manifest/manifest_route.rb @@ -18,6 +18,8 @@ def self.parse(route, options=nil) attrs[:full_route] = route attrs[:options] = {} attrs[:options][:loadbalancing] = options[:loadbalancing] if options && options.key?(:loadbalancing) + attrs[:options][:hash_header] = options[:hash_header] if options && options.key?(:hash_header) + attrs[:options][:hash_balance] = options[:hash_balance] if options && options.key?(:hash_balance) ManifestRoute.new(attrs) end From bc90e0611b0c31301bccc246974614e42b2a9048 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 18 Dec 2025 10:25:36 +0100 Subject: [PATCH 14/45] Add new hash options to route properties presenter --- .../app_manifest_presenters/route_properties_presenter.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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 d58c52c3003..824567e1b52 100644 --- a/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb +++ b/app/presenters/v3/app_manifest_presenters/route_properties_presenter.rb @@ -11,10 +11,13 @@ def to_hash(route_mappings:, app:, **_) } if route_mapping.route.options + opts = 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] = opts['loadbalancing'] if opts.key?('loadbalancing') + route_hash[:options][:hash_header] = opts['hash_header'] if opts.key?('hash_header') + route_hash[:options][:hash_balance] = opts['hash_balance'] if opts.key?('hash_balance') end - route_hash end From 3edb661081e4088a2621306514b4b12100d83e43 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 18 Dec 2025 10:26:20 +0100 Subject: [PATCH 15/45] Move additional validation to route model --- app/actions/route_update.rb | 27 ---- app/models/runtime/route.rb | 16 +++ spec/unit/models/runtime/route_spec.rb | 177 +++++++++++++++++++++++++ 3 files changed, 193 insertions(+), 27 deletions(-) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 25759d2c092..6c71cd883d2 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -1,8 +1,5 @@ module VCAP::CloudController class RouteUpdate - class Error < StandardError - end - def update(route:, message:) Route.db.transaction do if message.requested?(:options) @@ -16,9 +13,6 @@ def update(route:, message:) merged_options.delete(:hash_balance) end - # Validate the merged options - validate_route_options!(merged_options) - # Set the options on the route route.options = merged_options end @@ -34,26 +28,5 @@ def update(route:, message:) end route end - - private - - def validate_route_options!(options) - return if options.blank? - - loadbalancing = options[:loadbalancing] - return if loadbalancing != 'hash' - - hash_header = options[:hash_header] - - - if hash_header.blank? - error!('Hash header must be present when loadbalancing is set to hash') - end - - end - - def error!(message) - raise Error.new(message) - end end end diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index e8ea683f285..9574cf66154 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -126,6 +126,7 @@ def validate validate_total_routes validate_ports validate_total_reserved_route_ports if port && port > 0 + validate_route_options RouteValidator.new(self).validate rescue RoutingApi::UaaUnavailable @@ -329,6 +330,21 @@ def normalize_hash_balance_to_string(opts) normalized end + def validate_route_options + return if options.blank? + + route_options = options.is_a?(Hash) ? options : options.deep_symbolize_keys + loadbalancing = route_options[:loadbalancing] || route_options['loadbalancing'] + + return if loadbalancing != 'hash' + + hash_header = route_options[:hash_header] || route_options['hash_header'] + + if hash_header.blank? + errors.add(:options, 'Hash header must be present when loadbalancing is set to hash') + end + end + def validate_uniqueness_on_host_and_domain validates_unique [:host, :domain_id] do |ds| ds.where(path: '', port: 0) diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index ac880d27808..f0c41bcfd7b 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -1171,6 +1171,183 @@ module VCAP::CloudController end end + describe 'route options validation' do + let(:space) { Space.make } + let(:domain) { PrivateDomain.make(owning_organization: space.organization) } + + context 'when loadbalancing is hash' do + context 'and hash_header is present' do + it 'is valid' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID' } + ) + + expect(route).to be_valid + end + end + + context 'and hash_header is missing' do + it 'is invalid and adds an error' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash' } + ) + + expect(route).not_to be_valid + expect(route.errors[:options]).to include('Hash header must be present when loadbalancing is set to hash') + end + end + + context 'and hash_header is blank string' do + it 'is invalid and adds an error' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: '' } + ) + + expect(route).not_to be_valid + expect(route.errors[:options]).to include('Hash header must be present when loadbalancing is set to hash') + end + end + + context 'and hash_header and hash_balance are both present' do + it 'is valid' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '1.5' } + ) + + expect(route).to be_valid + end + end + end + + context 'when loadbalancing is round-robin' do + context 'and hash_header is present' do + it 'is valid (no validation error for non-hash loadbalancing)' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'round-robin' } + ) + + expect(route).to be_valid + end + end + end + + context 'when loadbalancing is least-connection' do + it 'is valid' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'least-connection' } + ) + + expect(route).to be_valid + end + end + + context 'when options are nil' do + it 'is valid' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: nil + ) + + expect(route).to be_valid + end + end + + context 'when options are empty hash' do + it 'is valid' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: {} + ) + + expect(route).to be_valid + end + end + + context 'when updating an existing route' do + context 'changing to hash loadbalancing without hash_header' do + it 'is invalid' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'round-robin' } + ) + + route.options = { loadbalancing: 'hash' } + + expect(route).not_to be_valid + expect(route.errors[:options]).to include('Hash header must be present when loadbalancing is set to hash') + end + end + + context 'changing to hash loadbalancing with hash_header' do + it 'is valid' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'round-robin' } + ) + + route.options = { loadbalancing: 'hash', hash_header: 'X-Request-ID' } + + expect(route).to be_valid + end + end + end + + context 'when options use string keys instead of symbols' do + context 'and hash_header is present' do + it 'is valid' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID' } + ) + + expect(route).to be_valid + end + end + + context 'and hash_header is missing' do + it 'is invalid and adds an error' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { 'loadbalancing' => 'hash' } + ) + + expect(route).not_to be_valid + expect(route.errors[:options]).to include('Hash header must be present when loadbalancing is set to hash') + end + end + end + end + describe 'instance methods' do let(:space) { Space.make } From 9bf2395ccf1a4a39bb5f2a384fc105cdceb90e67 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 18 Dec 2025 10:57:54 +0100 Subject: [PATCH 16/45] Move final options cleanup to route model --- app/actions/route_update.rb | 8 +- app/models/runtime/route.rb | 18 +- spec/unit/models/runtime/route_spec.rb | 227 +++++++++++++++++++++++++ 3 files changed, 245 insertions(+), 8 deletions(-) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 6c71cd883d2..fef5d110249 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -7,13 +7,7 @@ def update(route:, message:) existing_options = route.options&.deep_symbolize_keys || {} merged_options = existing_options.merge(message.options).compact - # Remove hash-specific options if switching to non-hash loadbalancing - if merged_options[:loadbalancing] && merged_options[:loadbalancing] != 'hash' - merged_options.delete(:hash_header) - merged_options.delete(:hash_balance) - end - - # Set the options on the route + # Set the options on the route (cleanup is handled by model) route.options = merged_options end diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index 9574cf66154..7c2a4171f52 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -71,7 +71,8 @@ def as_summary_json end def options_with_serialization=(opts) - normalized_opts = normalize_hash_balance_to_string(opts) + cleaned_opts = remove_hash_options_for_non_hash_loadbalancing(opts) + normalized_opts = normalize_hash_balance_to_string(cleaned_opts) self.options_without_serialization = Oj.dump(normalized_opts) end @@ -330,6 +331,21 @@ def normalize_hash_balance_to_string(opts) normalized end + def remove_hash_options_for_non_hash_loadbalancing(opts) + return opts unless opts.is_a?(Hash) + + opts_symbolized = opts.deep_symbolize_keys + loadbalancing = opts_symbolized[:loadbalancing] + + # Remove hash-specific options if loadbalancing is set to non-hash value + if loadbalancing.present? && loadbalancing != 'hash' + opts_symbolized.delete(:hash_header) + opts_symbolized.delete(:hash_balance) + end + + opts_symbolized + end + def validate_route_options return if options.blank? diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index f0c41bcfd7b..dd43451ef5c 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -1171,6 +1171,233 @@ module VCAP::CloudController end end + describe 'normalize_hash_balance_to_string' do + let(:space) { Space.make } + let(:domain) { PrivateDomain.make(owning_organization: space.organization) } + let(:route) { Route.new(host: 'test', domain: domain, space: space) } + + context 'when hash_balance is provided as a float' do + it 'converts it to a string' do + result = route.send(:normalize_hash_balance_to_string, { hash_balance: 1.5 }) + expect(result[:hash_balance]).to eq('1.5') + expect(result[:hash_balance]).to be_a(String) + end + end + + context 'when hash_balance is provided as an integer' do + it 'converts it to a string' do + result = route.send(:normalize_hash_balance_to_string, { hash_balance: 2 }) + expect(result[:hash_balance]).to eq('2') + expect(result[:hash_balance]).to be_a(String) + end + end + + context 'when hash_balance is provided as 0' do + it 'converts it to string "0"' do + result = route.send(:normalize_hash_balance_to_string, { hash_balance: 0 }) + expect(result[:hash_balance]).to eq('0') + expect(result[:hash_balance]).to be_a(String) + end + end + + context 'when hash_balance is already a string' do + it 'keeps it as a string' do + result = route.send(:normalize_hash_balance_to_string, { hash_balance: '1.25' }) + expect(result[:hash_balance]).to eq('1.25') + expect(result[:hash_balance]).to be_a(String) + end + end + + context 'when hash_balance is provided with string key' do + it 'converts it to a string with symbol key' do + result = route.send(:normalize_hash_balance_to_string, { 'hash_balance' => 2.5 }) + expect(result[:hash_balance]).to eq('2.5') + expect(result[:hash_balance]).to be_a(String) + end + end + + context 'when hash_balance is not present' do + it 'returns the hash unchanged' do + result = route.send(:normalize_hash_balance_to_string, { loadbalancing: 'hash', hash_header: 'X-User-ID' }) + expect(result[:loadbalancing]).to eq('hash') + expect(result[:hash_header]).to eq('X-User-ID') + expect(result).not_to have_key(:hash_balance) + end + end + + context 'when hash_balance is nil' do + it 'does not convert nil to string' do + result = route.send(:normalize_hash_balance_to_string, { hash_balance: nil }) + expect(result[:hash_balance]).to be_nil + end + end + + context 'when hash_balance is an empty string' do + it 'does not convert empty string' do + result = route.send(:normalize_hash_balance_to_string, { hash_balance: '' }) + expect(result[:hash_balance]).to eq('') + end + end + + context 'when options is not a hash' do + it 'returns the input unchanged' do + result = route.send(:normalize_hash_balance_to_string, nil) + expect(result).to be_nil + end + end + + context 'when options is an empty hash' do + it 'returns an empty hash' do + result = route.send(:normalize_hash_balance_to_string, {}) + expect(result).to eq({}) + end + end + + context 'with complete options hash' do + it 'normalizes hash_balance while preserving other options' do + result = route.send(:normalize_hash_balance_to_string, { + loadbalancing: 'hash', + hash_header: 'X-User-ID', + hash_balance: 3.14159 + }) + expect(result[:loadbalancing]).to eq('hash') + expect(result[:hash_header]).to eq('X-User-ID') + expect(result[:hash_balance]).to eq('3.14159') + expect(result[:hash_balance]).to be_a(String) + end + end + end + + describe 'hash options cleanup for non-hash loadbalancing' do + let(:space) { Space.make } + let(:domain) { PrivateDomain.make(owning_organization: space.organization) } + + context 'when creating a route with round-robin and hash options' do + it 'removes hash_header and hash_balance' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'round-robin', hash_header: 'X-User-ID', hash_balance: '1.5' } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['loadbalancing']).to eq('round-robin') + expect(parsed_options).not_to have_key('hash_header') + expect(parsed_options).not_to have_key('hash_balance') + end + end + + context 'when creating a route with least-connection and hash options' do + it 'removes hash_header and hash_balance' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'least-connection', hash_header: 'X-User-ID', hash_balance: '2.0' } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['loadbalancing']).to eq('least-connection') + expect(parsed_options).not_to have_key('hash_header') + expect(parsed_options).not_to have_key('hash_balance') + end + end + + context 'when creating a route with hash loadbalancing' do + it 'keeps hash_header and hash_balance' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '1.5' } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['loadbalancing']).to eq('hash') + expect(parsed_options['hash_header']).to eq('X-User-ID') + expect(parsed_options['hash_balance']).to eq('1.5') + end + end + + context 'when updating a route from hash to round-robin' do + it 'removes hash_header and hash_balance' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '1.5' } + ) + + route.update(options: { loadbalancing: 'round-robin' }) + route.reload + + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['loadbalancing']).to eq('round-robin') + expect(parsed_options).not_to have_key('hash_header') + expect(parsed_options).not_to have_key('hash_balance') + end + end + + context 'when updating a route from hash to least-connection' do + it 'removes hash_header and hash_balance' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-Request-ID', hash_balance: '2.5' } + ) + + route.update(options: { loadbalancing: 'least-connection' }) + route.reload + + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['loadbalancing']).to eq('least-connection') + expect(parsed_options).not_to have_key('hash_header') + expect(parsed_options).not_to have_key('hash_balance') + end + end + + context 'when updating a route from round-robin to hash' do + it 'keeps hash_header and hash_balance if provided' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'round-robin' } + ) + + route.update(options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '1.5' }) + route.reload + + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['loadbalancing']).to eq('hash') + expect(parsed_options['hash_header']).to eq('X-User-ID') + expect(parsed_options['hash_balance']).to eq('1.5') + end + end + + context 'when using string keys instead of symbols' do + it 'still removes hash options for non-hash loadbalancing' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { 'loadbalancing' => 'round-robin', 'hash_header' => 'X-User-ID', 'hash_balance' => '1.5' } + ) + + route.reload + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options['loadbalancing']).to eq('round-robin') + expect(parsed_options).not_to have_key('hash_header') + expect(parsed_options).not_to have_key('hash_balance') + end + end + end + describe 'route options validation' do let(:space) { Space.make } let(:domain) { PrivateDomain.make(owning_organization: space.organization) } From 9d60a3efe4216676183597dbe436371d336122f4 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 18 Dec 2025 12:01:03 +0100 Subject: [PATCH 17/45] Proper error validation and forwarding --- app/actions/route_create.rb | 2 ++ app/models/runtime/route.rb | 30 +++++++++++++++--------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index 15be9568456..d973daaf528 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -78,6 +78,8 @@ def validation_error!(error, host, path, port, space, domain) error!("Reserved route ports quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_reserved_route_ports_exceeded) + error!("Hash header must be present when loadbalancing is set to hash") if error.errors.on(:route)&.include?(:hash_header_missing) + validation_error_routing_api!(error) validation_error_host!(error, host, domain) validation_error_path!(error, host, path, domain) diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index 7c2a4171f52..8d0d7a95220 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -320,6 +320,21 @@ def validate_total_reserved_route_ports errors.add(:organization, :total_reserved_route_ports_exceeded) end + def validate_route_options + return if options.blank? + + route_options = options.is_a?(Hash) ? options : options.deep_symbolize_keys + loadbalancing = route_options[:loadbalancing] || route_options['loadbalancing'] + + return if loadbalancing != 'hash' + + hash_header = route_options[:hash_header] || route_options['hash_header'] + + if hash_header.blank? + errors.add(:route, :hash_header_missing) + end + end + def normalize_hash_balance_to_string(opts) return opts unless opts.is_a?(Hash) return opts unless opts.key?('hash_balance') || opts.key?(:hash_balance) @@ -346,21 +361,6 @@ def remove_hash_options_for_non_hash_loadbalancing(opts) opts_symbolized end - def validate_route_options - return if options.blank? - - route_options = options.is_a?(Hash) ? options : options.deep_symbolize_keys - loadbalancing = route_options[:loadbalancing] || route_options['loadbalancing'] - - return if loadbalancing != 'hash' - - hash_header = route_options[:hash_header] || route_options['hash_header'] - - if hash_header.blank? - errors.add(:options, 'Hash header must be present when loadbalancing is set to hash') - end - end - def validate_uniqueness_on_host_and_domain validates_unique [:host, :domain_id] do |ds| ds.where(path: '', port: 0) From 1a4b35b6a5b50e7673c43bbaafd5e084950f3462 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 18 Dec 2025 12:50:00 +0100 Subject: [PATCH 18/45] Forward proper error message on route updates --- app/actions/route_update.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index fef5d110249..4f064c146e1 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -1,5 +1,8 @@ module VCAP::CloudController class RouteUpdate + class Error < StandardError + end + def update(route:, message:) Route.db.transaction do if message.requested?(:options) @@ -21,6 +24,20 @@ def update(route:, message:) end end route + rescue Sequel::ValidationFailed => e + validation_error!(e, route) + end + + private + + def validation_error!(error, route) + # Handle hash_header validation error for hash loadbalancing + if error.errors.on(:route)&.include?(:hash_header_missing) + raise Error.new('Hash header must be present when loadbalancing is set to hash') + end + + # Fallback for any other validation errors + raise Error.new(error.message) end end end From 0dddc1d2ebbd366105b9867508a7a65f2876822b Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 19 Dec 2025 10:06:12 +0100 Subject: [PATCH 19/45] Add some logging to find the manifest issue --- app/actions/app_apply_manifest.rb | 4 ++++ app/actions/manifest_route_update.rb | 6 ++++++ app/actions/route_update.rb | 3 +++ app/models/runtime/route.rb | 6 ++++++ 4 files changed, 19 insertions(+) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index 3b632ae55c0..a340bf28870 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -25,6 +25,8 @@ def initialize(user_audit_info) end def apply(app_guid, message) + logger = Steno.logger('app_appy_manifest') + logger.info("Applying app manifest #{message} to app: #{app_guid}") app = AppModel.first(guid: app_guid) app_instance_not_found!(app_guid) unless app @@ -90,6 +92,8 @@ def find_sidecar(app, sidecar_name) end def update_routes(app, message) + logger = Steno.logger('update_routes') + logger.info("Applying route message #{message} to app: #{app}") update_message = message.manifest_routes_update_message existing_routes = RouteMappingModel.where(app_guid: app.guid).all diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index df49e50eae8..3f25d49cefa 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -9,6 +9,9 @@ class InvalidRoute < StandardError class << self def update(app_guid, message, user_audit_info) + logger = Steno.logger('cc.action.manifest_route_update') + logger.error("ManifestRouteUpdate.update called", app_guid: app_guid, message: message.inspect, location: "#{__FILE__}:#{__LINE__}") + return unless message.requested?(:routes) app = AppModel.find(guid: app_guid) @@ -75,6 +78,9 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) ) ) + logger = Steno.logger('cc.action.manifest_route_update') + logger.info("Route lookup completed", manifest_route: manifest_route.inspect, route: route.inspect, location: "#{__FILE__}:#{__LINE__}") + if !route FeatureFlag.raise_unless_enabled!(:route_creation) raise CloudController::Errors::ApiError.new_from_details('NotAuthorized') if host == '*' && existing_domain.shared? diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 4f064c146e1..e65a050ce0c 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -4,6 +4,9 @@ class Error < StandardError end def update(route:, message:) + logger = Steno.logger('cc.action.route_update') + logger.info("RouteUpdate.update called", route_guid: route.guid, route_options: route.options.inspect, message: message.inspect, location: "#{__FILE__}:#{__LINE__}") + Route.db.transaction do if message.requested?(:options) # Merge existing options with new options from message diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index 8d0d7a95220..cd009cff85e 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -107,6 +107,9 @@ def route_service_url end def validate + logger = Steno.logger('cc.model.route') + logger.info("Route.validate called, route_guid: #{guid}, route_host: #{host}, route_options: #{options}, location: #{__FILE__}:#{__LINE__}") + validates_presence :domain validates_presence :space @@ -347,6 +350,9 @@ def normalize_hash_balance_to_string(opts) end def remove_hash_options_for_non_hash_loadbalancing(opts) + logger = Steno.logger('cc.model.route') + logger.info("remove_hash_options_for_non_hash_loadbalancing called", opts: opts.inspect, location: "#{__FILE__}:#{__LINE__}") + return opts unless opts.is_a?(Hash) opts_symbolized = opts.deep_symbolize_keys From 7b0f56c40565337182ecd55e7d73e493cccc5688 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 19 Dec 2025 11:11:06 +0100 Subject: [PATCH 20/45] Add more logging --- app/actions/manifest_route_update.rb | 9 +++++++-- app/actions/route_create.rb | 5 ++++- app/actions/route_update.rb | 4 +++- app/models/runtime/route.rb | 5 +++++ 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index 3f25d49cefa..71ea7cdba1b 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -79,9 +79,10 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) ) logger = Steno.logger('cc.action.manifest_route_update') - logger.info("Route lookup completed", manifest_route: manifest_route.inspect, route: route.inspect, location: "#{__FILE__}:#{__LINE__}") + logger.info("Route lookup completed", manifest_route: manifest_route.inspect, route_found: !route.nil?, route_guid: route&.guid, location: "#{__FILE__}:#{__LINE__}") if !route + logger.info("Creating new route", location: "#{__FILE__}:#{__LINE__}") FeatureFlag.raise_unless_enabled!(:route_creation) raise CloudController::Errors::ApiError.new_from_details('NotAuthorized') if host == '*' && existing_domain.shared? @@ -99,14 +100,18 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) manifest_triggered: true ) elsif !route.available_in_space?(app.space) + logger.info("Route not available in space", location: "#{__FILE__}:#{__LINE__}") raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces') - elsif manifest_route[:options] && route[:options] != manifest_route[:options] + elsif manifest_route[:options] && route.options != manifest_route[:options] + logger.info("Updating existing route options", existing_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") # remove nil values from options manifest_route[:options] = manifest_route[:options].compact message = RouteUpdateMessage.new({ 'options' => manifest_route[:options] }) route = RouteUpdate.new.update(route:, message:) + else + logger.info("Route exists, no update needed", route_has_options: route.options.present?, manifest_has_options: manifest_route[:options].present?, route_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") end return route diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index d973daaf528..8112efd157f 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -20,7 +20,10 @@ def create(message:, space:, domain:, manifest_triggered: false) ) Route.db.transaction do - route.save + logger = Steno.logger('cc.action.route_create') + logger.info("About to call route.save", route_host: route.host, route_options: route.options.inspect, raise_on_failure: true, location: "#{__FILE__}:#{__LINE__}") + + route.save(raise_on_failure: true) MetadataUpdate.update(route, message) end diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index e65a050ce0c..bd38eda1d53 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -17,7 +17,9 @@ def update(route:, message:) route.options = merged_options end - route.save + logger.info("About to call route.save", route_guid: route.guid, route_options: route.options.inspect, raise_on_failure: true, location: "#{__FILE__}:#{__LINE__}") + + route.save(raise_on_failure: true) MetadataUpdate.update(route, message) end diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index cd009cff85e..d211c32440d 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -71,9 +71,14 @@ def as_summary_json end def options_with_serialization=(opts) + logger = Steno.logger('cc.model.route') + logger.info("options_with_serialization= setter called", opts: opts.inspect, location: "#{__FILE__}:#{__LINE__}") + cleaned_opts = remove_hash_options_for_non_hash_loadbalancing(opts) normalized_opts = normalize_hash_balance_to_string(cleaned_opts) self.options_without_serialization = Oj.dump(normalized_opts) + + logger.info("options_with_serialization= setter completed", normalized_opts: normalized_opts.inspect, json: self.options_without_serialization.inspect) end alias_method :options_without_serialization=, :options= From 981e130efbf6d8c01349d66fcc50c2e1a178c7d9 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 8 Jan 2026 12:29:44 +0100 Subject: [PATCH 21/45] Logging in app_manifest_message --- app/controllers/v3/space_manifests_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/v3/space_manifests_controller.rb b/app/controllers/v3/space_manifests_controller.rb index 9e8f8cd91ae..5664202d9cc 100644 --- a/app/controllers/v3/space_manifests_controller.rb +++ b/app/controllers/v3/space_manifests_controller.rb @@ -11,6 +11,8 @@ class SpaceManifestsController < ApplicationController before_action :validate_content_type! def apply_manifest + logger = Steno.logger('app_manifest_message/apply_manifest') + logger.info("Running into apply_manifest...") space = Space.find(guid: hashed_params[:guid]) space_not_found! unless space && permission_queryer.can_read_from_space?(space.id, space.organization_id) can_write_space(space) From 308173eb9a325827b187e33da3a4ff2dcaf0709e Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 8 Jan 2026 12:46:54 +0100 Subject: [PATCH 22/45] Test a few things --- app/actions/manifest_route_update.rb | 7 ++++++- app/actions/route_update.rb | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index 71ea7cdba1b..61169572d78 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -62,6 +62,9 @@ def update(app_guid, message, user_audit_info) private def find_or_create_valid_route(app, manifest_route, user_audit_info) + logger = Steno.logger('cc.action.manifest_route_update') + logger.info("find_or_create_valid_route called", manifest_route: manifest_route.inspect, has_options: manifest_route[:options].present?, options_value: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") + manifest_route[:candidate_host_domain_pairs].each do |candidate| potential_domain = candidate[:domain] existing_domain = Domain.find(name: potential_domain) @@ -102,13 +105,15 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) elsif !route.available_in_space?(app.space) logger.info("Route not available in space", location: "#{__FILE__}:#{__LINE__}") raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces') - elsif manifest_route[:options] && route.options != manifest_route[:options] + elsif manifest_route[:options] logger.info("Updating existing route options", existing_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") # remove nil values from options manifest_route[:options] = manifest_route[:options].compact + logger.info("About to call RouteUpdate.new.update", manifest_route_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") message = RouteUpdateMessage.new({ 'options' => manifest_route[:options] }) + logger.info("Created RouteUpdateMessage", message: message.inspect, message_options: message.options.inspect, location: "#{__FILE__}:#{__LINE__}") route = RouteUpdate.new.update(route:, message:) else logger.info("Route exists, no update needed", route_has_options: route.options.present?, manifest_has_options: manifest_route[:options].present?, route_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index bd38eda1d53..d85b2203ca7 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -5,7 +5,7 @@ class Error < StandardError def update(route:, message:) logger = Steno.logger('cc.action.route_update') - logger.info("RouteUpdate.update called", route_guid: route.guid, route_options: route.options.inspect, message: message.inspect, location: "#{__FILE__}:#{__LINE__}") + logger.info("RouteUpdate.update called", route_guid: route.guid, route_options: route.options.inspect, message: message.inspect, message_options: message.options.inspect, message_requested_options: message.requested?(:options), location: "#{__FILE__}:#{__LINE__}") Route.db.transaction do if message.requested?(:options) From c93cbaa9ba2ff5ee70770cd5f5f031855e1f2426 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 8 Jan 2026 13:12:13 +0100 Subject: [PATCH 23/45] Test a few things pt 2 --- app/actions/manifest_route_update.rb | 15 ++++++++------- app/actions/route_update.rb | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index 61169572d78..b40739178df 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -82,10 +82,10 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) ) logger = Steno.logger('cc.action.manifest_route_update') - logger.info("Route lookup completed", manifest_route: manifest_route.inspect, route_found: !route.nil?, route_guid: route&.guid, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: Route lookup completed", manifest_route: manifest_route.inspect, route_found: !route.nil?, route_guid: route&.guid, route_options: route&.options.inspect, manifest_options: manifest_route[:options].inspect, manifest_options_present: manifest_route[:options].present?, location: "#{__FILE__}:#{__LINE__}") if !route - logger.info("Creating new route", location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: Creating new route (route is nil)", location: "#{__FILE__}:#{__LINE__}") FeatureFlag.raise_unless_enabled!(:route_creation) raise CloudController::Errors::ApiError.new_from_details('NotAuthorized') if host == '*' && existing_domain.shared? @@ -103,20 +103,21 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) manifest_triggered: true ) elsif !route.available_in_space?(app.space) - logger.info("Route not available in space", location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: Route not available in space", location: "#{__FILE__}:#{__LINE__}") raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces') elsif manifest_route[:options] - logger.info("Updating existing route options", existing_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: Updating existing route options - ENTERING THIS BRANCH", existing_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") # remove nil values from options manifest_route[:options] = manifest_route[:options].compact - logger.info("About to call RouteUpdate.new.update", manifest_route_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: About to call RouteUpdate.new.update", manifest_route_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") message = RouteUpdateMessage.new({ 'options' => manifest_route[:options] }) - logger.info("Created RouteUpdateMessage", message: message.inspect, message_options: message.options.inspect, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: Created RouteUpdateMessage", message: message.inspect, message_options: message.options.inspect, location: "#{__FILE__}:#{__LINE__}") route = RouteUpdate.new.update(route:, message:) + logger.error("CRITICAL: RouteUpdate.new.update completed", updated_route_options: route.options.inspect, location: "#{__FILE__}:#{__LINE__}") else - logger.info("Route exists, no update needed", route_has_options: route.options.present?, manifest_has_options: manifest_route[:options].present?, route_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: Route exists, no update needed - ELSE BRANCH", route_has_options: route.options.present?, manifest_has_options: manifest_route[:options].present?, route_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") end return route diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index d85b2203ca7..cc966b6af34 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -5,7 +5,7 @@ class Error < StandardError def update(route:, message:) logger = Steno.logger('cc.action.route_update') - logger.info("RouteUpdate.update called", route_guid: route.guid, route_options: route.options.inspect, message: message.inspect, message_options: message.options.inspect, message_requested_options: message.requested?(:options), location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: RouteUpdate.update called", route_guid: route.guid, route_options: route.options.inspect, message: message.inspect, message_options: message.options.inspect, message_requested_options: message.requested?(:options), location: "#{__FILE__}:#{__LINE__}") Route.db.transaction do if message.requested?(:options) @@ -17,7 +17,7 @@ def update(route:, message:) route.options = merged_options end - logger.info("About to call route.save", route_guid: route.guid, route_options: route.options.inspect, raise_on_failure: true, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: About to call route.save", route_guid: route.guid, route_options: route.options.inspect, raise_on_failure: true, location: "#{__FILE__}:#{__LINE__}") route.save(raise_on_failure: true) MetadataUpdate.update(route, message) From ed35a675cb9ae350ebd3765729d7bf3a4b766715 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 8 Jan 2026 13:38:15 +0100 Subject: [PATCH 24/45] Test a few things pt 3 --- app/actions/manifest_route_update.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index b40739178df..eff1f51f71b 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -7,10 +7,13 @@ class ManifestRouteUpdate class InvalidRoute < StandardError end + # This log fires when the class is loaded - if you don't see it, the new code isn't being loaded + Steno.logger('cc.action.manifest_route_update').error("CRITICAL: ManifestRouteUpdate class loaded at #{Time.now}") + class << self def update(app_guid, message, user_audit_info) logger = Steno.logger('cc.action.manifest_route_update') - logger.error("ManifestRouteUpdate.update called", app_guid: app_guid, message: message.inspect, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: ManifestRouteUpdate.update called", app_guid: app_guid, message: message.inspect, message_routes: message.routes.inspect, location: "#{__FILE__}:#{__LINE__}") return unless message.requested?(:routes) @@ -63,7 +66,7 @@ def update(app_guid, message, user_audit_info) def find_or_create_valid_route(app, manifest_route, user_audit_info) logger = Steno.logger('cc.action.manifest_route_update') - logger.info("find_or_create_valid_route called", manifest_route: manifest_route.inspect, has_options: manifest_route[:options].present?, options_value: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: find_or_create_valid_route called", manifest_route: manifest_route.inspect, has_options: manifest_route[:options].present?, options_value: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") manifest_route[:candidate_host_domain_pairs].each do |candidate| potential_domain = candidate[:domain] From e3cffab62376ae2297a416948313034a49a811af Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 8 Jan 2026 14:17:05 +0100 Subject: [PATCH 25/45] Test a few things pt 4 --- app/actions/app_apply_manifest.rb | 5 ++++- app/actions/manifest_route_update.rb | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index a340bf28870..e1b729bc5f2 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -93,16 +93,19 @@ def find_sidecar(app, sidecar_name) def update_routes(app, message) logger = Steno.logger('update_routes') - logger.info("Applying route message #{message} to app: #{app}") + logger.error("CRITICAL: update_routes called, app: #{app.name}, message: #{message.inspect}") update_message = message.manifest_routes_update_message + logger.error("CRITICAL: update_message: #{update_message.inspect}, routes: #{update_message.routes.inspect}, requested_routes: #{update_message.requested?(:routes)}") existing_routes = RouteMappingModel.where(app_guid: app.guid).all if update_message.no_route + logger.error("CRITICAL: no_route branch") RouteMappingDelete.new(@user_audit_info, manifest_triggered: true).delete(existing_routes) return end if update_message.routes + logger.error("CRITICAL: Calling ManifestRouteUpdate.update") ManifestRouteUpdate.update(app.guid, update_message, @user_audit_info) return end diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index eff1f51f71b..6d1a98dafa4 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -13,9 +13,12 @@ class InvalidRoute < StandardError class << self def update(app_guid, message, user_audit_info) logger = Steno.logger('cc.action.manifest_route_update') - logger.error("CRITICAL: ManifestRouteUpdate.update called", app_guid: app_guid, message: message.inspect, message_routes: message.routes.inspect, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: ManifestRouteUpdate.update called", app_guid: app_guid, message: message.inspect, message_routes: message.routes.inspect, message_requested_routes: message.requested?(:routes), location: "#{__FILE__}:#{__LINE__}") - return unless message.requested?(:routes) + unless message.requested?(:routes) + logger.error("CRITICAL: Early return - routes not requested", location: "#{__FILE__}:#{__LINE__}") + return + end app = AppModel.find(guid: app_guid) not_found! unless app From 7379cff9605820b778fbea2e4fab08876c0e9ec7 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 8 Jan 2026 15:20:42 +0100 Subject: [PATCH 26/45] Test a few things pt 5 --- app/actions/route_create.rb | 5 ++++- app/models/runtime/route.rb | 16 ++++++++++++---- test-manifest-roundrobin.yml | 0 3 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 test-manifest-roundrobin.yml diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index 8112efd157f..6ff103b912c 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -8,6 +8,9 @@ def initialize(user_audit_info) end def create(message:, space:, domain:, manifest_triggered: false) + logger = Steno.logger('cc.action.route_create') + logger.error("CRITICAL: RouteCreate.create called", manifest_triggered: manifest_triggered, message_options: message.options.inspect, location: "#{__FILE__}:#{__LINE__}") + validate_tcp_route!(domain, message) route = Route.new( @@ -21,7 +24,7 @@ def create(message:, space:, domain:, manifest_triggered: false) Route.db.transaction do logger = Steno.logger('cc.action.route_create') - logger.info("About to call route.save", route_host: route.host, route_options: route.options.inspect, raise_on_failure: true, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: About to call route.save in RouteCreate", route_host: route.host, route_options: route.options.inspect, raise_on_failure: true, location: "#{__FILE__}:#{__LINE__}") route.save(raise_on_failure: true) diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index d211c32440d..ade11413113 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -72,13 +72,14 @@ def as_summary_json def options_with_serialization=(opts) logger = Steno.logger('cc.model.route') - logger.info("options_with_serialization= setter called", opts: opts.inspect, location: "#{__FILE__}:#{__LINE__}") + caller_info = caller[0..5].join("\n ") + logger.error("CRITICAL: options_with_serialization= setter called", opts: opts.inspect, caller: caller_info, location: "#{__FILE__}:#{__LINE__}") cleaned_opts = remove_hash_options_for_non_hash_loadbalancing(opts) normalized_opts = normalize_hash_balance_to_string(cleaned_opts) self.options_without_serialization = Oj.dump(normalized_opts) - logger.info("options_with_serialization= setter completed", normalized_opts: normalized_opts.inspect, json: self.options_without_serialization.inspect) + logger.error("CRITICAL: options_with_serialization= setter completed", normalized_opts: normalized_opts.inspect, json: self.options_without_serialization.inspect) end alias_method :options_without_serialization=, :options= @@ -113,7 +114,8 @@ def route_service_url def validate logger = Steno.logger('cc.model.route') - logger.info("Route.validate called, route_guid: #{guid}, route_host: #{host}, route_options: #{options}, location: #{__FILE__}:#{__LINE__}") + caller_info = caller[0..3].join("\n ") + logger.error("CRITICAL: Route.validate called, route_guid: #{guid}, route_host: #{host}, route_options: #{options.inspect}, caller: #{caller_info}, location: #{__FILE__}:#{__LINE__}") validates_presence :domain validates_presence :space @@ -356,19 +358,25 @@ def normalize_hash_balance_to_string(opts) def remove_hash_options_for_non_hash_loadbalancing(opts) logger = Steno.logger('cc.model.route') - logger.info("remove_hash_options_for_non_hash_loadbalancing called", opts: opts.inspect, location: "#{__FILE__}:#{__LINE__}") + logger.error("CRITICAL: remove_hash_options_for_non_hash_loadbalancing called", opts: opts.inspect, location: "#{__FILE__}:#{__LINE__}") return opts unless opts.is_a?(Hash) opts_symbolized = opts.deep_symbolize_keys loadbalancing = opts_symbolized[:loadbalancing] + logger.error("CRITICAL: loadbalancing value", loadbalancing: loadbalancing.inspect, location: "#{__FILE__}:#{__LINE__}") + # Remove hash-specific options if loadbalancing is set to non-hash value if loadbalancing.present? && loadbalancing != 'hash' + logger.error("CRITICAL: Removing hash_header and hash_balance because loadbalancing=#{loadbalancing}", location: "#{__FILE__}:#{__LINE__}") opts_symbolized.delete(:hash_header) opts_symbolized.delete(:hash_balance) + else + logger.error("CRITICAL: NOT removing hash options", loadbalancing_present: loadbalancing.present?, loadbalancing_is_hash: (loadbalancing == 'hash'), location: "#{__FILE__}:#{__LINE__}") end + logger.error("CRITICAL: remove_hash_options result", result: opts_symbolized.inspect, location: "#{__FILE__}:#{__LINE__}") opts_symbolized end diff --git a/test-manifest-roundrobin.yml b/test-manifest-roundrobin.yml new file mode 100644 index 00000000000..e69de29bb2d From 9eec93ccb16625311f7b7d02792624fe9cc5f31a Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 8 Jan 2026 15:40:20 +0100 Subject: [PATCH 27/45] Test a few things pt 6 --- app/actions/app_apply_manifest.rb | 3 ++- app/jobs/space_apply_manifest_action_job.rb | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index e1b729bc5f2..735982729ae 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -25,7 +25,8 @@ def initialize(user_audit_info) end def apply(app_guid, message) - logger = Steno.logger('app_appy_manifest') + logger = Steno.logger('app_apply_manifest') + logger.error("CRITICAL: AppApplyManifest.apply called for app_guid: #{app_guid}") logger.info("Applying app manifest #{message} to app: #{app_guid}") app = AppModel.first(guid: app_guid) app_instance_not_found!(app_guid) unless app diff --git a/app/jobs/space_apply_manifest_action_job.rb b/app/jobs/space_apply_manifest_action_job.rb index 88a485ad418..e2465db954b 100644 --- a/app/jobs/space_apply_manifest_action_job.rb +++ b/app/jobs/space_apply_manifest_action_job.rb @@ -1,6 +1,9 @@ module VCAP::CloudController module Jobs class SpaceApplyManifestActionJob < VCAP::CloudController::Jobs::CCJob + # This log fires when the worker loads the class + Steno.logger('cc.background').error("CRITICAL: SpaceApplyManifestActionJob class loaded at #{Time.now}") + def initialize(space, app_guid_message_hash, apply_manifest_action, user_audit_info) @space = space @app_guid_message_hash = app_guid_message_hash @@ -10,9 +13,11 @@ def initialize(space, app_guid_message_hash, apply_manifest_action, user_audit_i def perform logger = Steno.logger('cc.background') + logger.error("CRITICAL: SpaceApplyManifestActionJob.perform called for space: #{resource_guid}, apps: #{app_guid_message_hash.keys.inspect}") logger.info("Applying app manifest to app: #{resource_guid}") app_guid_message_hash.each do |app_guid, message| + logger.error("CRITICAL: Processing app_guid: #{app_guid}, message has routes: #{message.routes.present?}") apply_manifest_action.apply(app_guid, message) rescue AppPatchEnvironmentVariables::InvalidApp, AppUpdate::InvalidApp, From 4eb3b362f127f86cfe9d41ecfa29ffbd2fc7098c Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Thu, 8 Jan 2026 17:10:00 +0100 Subject: [PATCH 28/45] Cleanup logging --- app/actions/app_apply_manifest.rb | 8 ------ app/actions/manifest_route_update.rb | 25 +------------------ app/actions/route_create.rb | 6 ----- app/actions/route_update.rb | 5 ---- .../v3/space_manifests_controller.rb | 2 -- app/jobs/space_apply_manifest_action_job.rb | 5 ---- app/models/runtime/route.rb | 13 ---------- 7 files changed, 1 insertion(+), 63 deletions(-) diff --git a/app/actions/app_apply_manifest.rb b/app/actions/app_apply_manifest.rb index 735982729ae..3b632ae55c0 100644 --- a/app/actions/app_apply_manifest.rb +++ b/app/actions/app_apply_manifest.rb @@ -25,9 +25,6 @@ def initialize(user_audit_info) end def apply(app_guid, message) - logger = Steno.logger('app_apply_manifest') - logger.error("CRITICAL: AppApplyManifest.apply called for app_guid: #{app_guid}") - logger.info("Applying app manifest #{message} to app: #{app_guid}") app = AppModel.first(guid: app_guid) app_instance_not_found!(app_guid) unless app @@ -93,20 +90,15 @@ def find_sidecar(app, sidecar_name) end def update_routes(app, message) - logger = Steno.logger('update_routes') - logger.error("CRITICAL: update_routes called, app: #{app.name}, message: #{message.inspect}") update_message = message.manifest_routes_update_message - logger.error("CRITICAL: update_message: #{update_message.inspect}, routes: #{update_message.routes.inspect}, requested_routes: #{update_message.requested?(:routes)}") existing_routes = RouteMappingModel.where(app_guid: app.guid).all if update_message.no_route - logger.error("CRITICAL: no_route branch") RouteMappingDelete.new(@user_audit_info, manifest_triggered: true).delete(existing_routes) return end if update_message.routes - logger.error("CRITICAL: Calling ManifestRouteUpdate.update") ManifestRouteUpdate.update(app.guid, update_message, @user_audit_info) return end diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index 6d1a98dafa4..c5c7a72a39f 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -7,18 +7,9 @@ class ManifestRouteUpdate class InvalidRoute < StandardError end - # This log fires when the class is loaded - if you don't see it, the new code isn't being loaded - Steno.logger('cc.action.manifest_route_update').error("CRITICAL: ManifestRouteUpdate class loaded at #{Time.now}") - class << self def update(app_guid, message, user_audit_info) - logger = Steno.logger('cc.action.manifest_route_update') - logger.error("CRITICAL: ManifestRouteUpdate.update called", app_guid: app_guid, message: message.inspect, message_routes: message.routes.inspect, message_requested_routes: message.requested?(:routes), location: "#{__FILE__}:#{__LINE__}") - - unless message.requested?(:routes) - logger.error("CRITICAL: Early return - routes not requested", location: "#{__FILE__}:#{__LINE__}") - return - end + return unless message.requested?(:routes) app = AppModel.find(guid: app_guid) not_found! unless app @@ -68,9 +59,6 @@ def update(app_guid, message, user_audit_info) private def find_or_create_valid_route(app, manifest_route, user_audit_info) - logger = Steno.logger('cc.action.manifest_route_update') - logger.error("CRITICAL: find_or_create_valid_route called", manifest_route: manifest_route.inspect, has_options: manifest_route[:options].present?, options_value: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") - manifest_route[:candidate_host_domain_pairs].each do |candidate| potential_domain = candidate[:domain] existing_domain = Domain.find(name: potential_domain) @@ -87,11 +75,7 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) ) ) - logger = Steno.logger('cc.action.manifest_route_update') - logger.error("CRITICAL: Route lookup completed", manifest_route: manifest_route.inspect, route_found: !route.nil?, route_guid: route&.guid, route_options: route&.options.inspect, manifest_options: manifest_route[:options].inspect, manifest_options_present: manifest_route[:options].present?, location: "#{__FILE__}:#{__LINE__}") - if !route - logger.error("CRITICAL: Creating new route (route is nil)", location: "#{__FILE__}:#{__LINE__}") FeatureFlag.raise_unless_enabled!(:route_creation) raise CloudController::Errors::ApiError.new_from_details('NotAuthorized') if host == '*' && existing_domain.shared? @@ -109,21 +93,14 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) manifest_triggered: true ) elsif !route.available_in_space?(app.space) - logger.error("CRITICAL: Route not available in space", location: "#{__FILE__}:#{__LINE__}") raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces') elsif manifest_route[:options] - logger.error("CRITICAL: Updating existing route options - ENTERING THIS BRANCH", existing_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") # remove nil values from options manifest_route[:options] = manifest_route[:options].compact - logger.error("CRITICAL: About to call RouteUpdate.new.update", manifest_route_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") message = RouteUpdateMessage.new({ 'options' => manifest_route[:options] }) - logger.error("CRITICAL: Created RouteUpdateMessage", message: message.inspect, message_options: message.options.inspect, location: "#{__FILE__}:#{__LINE__}") route = RouteUpdate.new.update(route:, message:) - logger.error("CRITICAL: RouteUpdate.new.update completed", updated_route_options: route.options.inspect, location: "#{__FILE__}:#{__LINE__}") - else - logger.error("CRITICAL: Route exists, no update needed - ELSE BRANCH", route_has_options: route.options.present?, manifest_has_options: manifest_route[:options].present?, route_options: route.options.inspect, manifest_options: manifest_route[:options].inspect, location: "#{__FILE__}:#{__LINE__}") end return route diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index 6ff103b912c..87b9a95eb44 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -8,9 +8,6 @@ def initialize(user_audit_info) end def create(message:, space:, domain:, manifest_triggered: false) - logger = Steno.logger('cc.action.route_create') - logger.error("CRITICAL: RouteCreate.create called", manifest_triggered: manifest_triggered, message_options: message.options.inspect, location: "#{__FILE__}:#{__LINE__}") - validate_tcp_route!(domain, message) route = Route.new( @@ -23,9 +20,6 @@ def create(message:, space:, domain:, manifest_triggered: false) ) Route.db.transaction do - logger = Steno.logger('cc.action.route_create') - logger.error("CRITICAL: About to call route.save in RouteCreate", route_host: route.host, route_options: route.options.inspect, raise_on_failure: true, location: "#{__FILE__}:#{__LINE__}") - route.save(raise_on_failure: true) MetadataUpdate.update(route, message) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index cc966b6af34..505af85e43c 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -4,8 +4,6 @@ class Error < StandardError end def update(route:, message:) - logger = Steno.logger('cc.action.route_update') - logger.error("CRITICAL: RouteUpdate.update called", route_guid: route.guid, route_options: route.options.inspect, message: message.inspect, message_options: message.options.inspect, message_requested_options: message.requested?(:options), location: "#{__FILE__}:#{__LINE__}") Route.db.transaction do if message.requested?(:options) @@ -16,9 +14,6 @@ def update(route:, message:) # Set the options on the route (cleanup is handled by model) route.options = merged_options end - - logger.error("CRITICAL: About to call route.save", route_guid: route.guid, route_options: route.options.inspect, raise_on_failure: true, location: "#{__FILE__}:#{__LINE__}") - route.save(raise_on_failure: true) MetadataUpdate.update(route, message) end diff --git a/app/controllers/v3/space_manifests_controller.rb b/app/controllers/v3/space_manifests_controller.rb index 5664202d9cc..9e8f8cd91ae 100644 --- a/app/controllers/v3/space_manifests_controller.rb +++ b/app/controllers/v3/space_manifests_controller.rb @@ -11,8 +11,6 @@ class SpaceManifestsController < ApplicationController before_action :validate_content_type! def apply_manifest - logger = Steno.logger('app_manifest_message/apply_manifest') - logger.info("Running into apply_manifest...") space = Space.find(guid: hashed_params[:guid]) space_not_found! unless space && permission_queryer.can_read_from_space?(space.id, space.organization_id) can_write_space(space) diff --git a/app/jobs/space_apply_manifest_action_job.rb b/app/jobs/space_apply_manifest_action_job.rb index e2465db954b..88a485ad418 100644 --- a/app/jobs/space_apply_manifest_action_job.rb +++ b/app/jobs/space_apply_manifest_action_job.rb @@ -1,9 +1,6 @@ module VCAP::CloudController module Jobs class SpaceApplyManifestActionJob < VCAP::CloudController::Jobs::CCJob - # This log fires when the worker loads the class - Steno.logger('cc.background').error("CRITICAL: SpaceApplyManifestActionJob class loaded at #{Time.now}") - def initialize(space, app_guid_message_hash, apply_manifest_action, user_audit_info) @space = space @app_guid_message_hash = app_guid_message_hash @@ -13,11 +10,9 @@ def initialize(space, app_guid_message_hash, apply_manifest_action, user_audit_i def perform logger = Steno.logger('cc.background') - logger.error("CRITICAL: SpaceApplyManifestActionJob.perform called for space: #{resource_guid}, apps: #{app_guid_message_hash.keys.inspect}") logger.info("Applying app manifest to app: #{resource_guid}") app_guid_message_hash.each do |app_guid, message| - logger.error("CRITICAL: Processing app_guid: #{app_guid}, message has routes: #{message.routes.present?}") apply_manifest_action.apply(app_guid, message) rescue AppPatchEnvironmentVariables::InvalidApp, AppUpdate::InvalidApp, diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index ade11413113..55ec73042c6 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -113,10 +113,6 @@ def route_service_url end def validate - logger = Steno.logger('cc.model.route') - caller_info = caller[0..3].join("\n ") - logger.error("CRITICAL: Route.validate called, route_guid: #{guid}, route_host: #{host}, route_options: #{options.inspect}, caller: #{caller_info}, location: #{__FILE__}:#{__LINE__}") - validates_presence :domain validates_presence :space @@ -357,26 +353,17 @@ def normalize_hash_balance_to_string(opts) end def remove_hash_options_for_non_hash_loadbalancing(opts) - logger = Steno.logger('cc.model.route') - logger.error("CRITICAL: remove_hash_options_for_non_hash_loadbalancing called", opts: opts.inspect, location: "#{__FILE__}:#{__LINE__}") - return opts unless opts.is_a?(Hash) opts_symbolized = opts.deep_symbolize_keys loadbalancing = opts_symbolized[:loadbalancing] - logger.error("CRITICAL: loadbalancing value", loadbalancing: loadbalancing.inspect, location: "#{__FILE__}:#{__LINE__}") - # Remove hash-specific options if loadbalancing is set to non-hash value if loadbalancing.present? && loadbalancing != 'hash' - logger.error("CRITICAL: Removing hash_header and hash_balance because loadbalancing=#{loadbalancing}", location: "#{__FILE__}:#{__LINE__}") opts_symbolized.delete(:hash_header) opts_symbolized.delete(:hash_balance) - else - logger.error("CRITICAL: NOT removing hash options", loadbalancing_present: loadbalancing.present?, loadbalancing_is_hash: (loadbalancing == 'hash'), location: "#{__FILE__}:#{__LINE__}") end - logger.error("CRITICAL: remove_hash_options result", result: opts_symbolized.inspect, location: "#{__FILE__}:#{__LINE__}") opts_symbolized end From 69a3ffeb5b2855d468f75237a18c134928247fdb Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 9 Jan 2026 09:22:55 +0100 Subject: [PATCH 29/45] Use symbolized_keys in route_update --- app/actions/route_update.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 505af85e43c..ab2c43fadda 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -8,7 +8,7 @@ def update(route:, message:) Route.db.transaction do if message.requested?(:options) # Merge existing options with new options from message - existing_options = route.options&.deep_symbolize_keys || {} + existing_options = route.options || {} merged_options = existing_options.merge(message.options).compact # Set the options on the route (cleanup is handled by model) From b3f631f92cc1746aa787565bee7c6cc20316b08f Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 9 Jan 2026 15:53:57 +0100 Subject: [PATCH 30/45] WIP --- app/actions/route_update.rb | 11 +-- app/models/runtime/route.rb | 15 ++- .../routes/_route_options_object.md.erb | 10 +- spec/unit/actions/route_update_spec.rb | 32 +++++- spec/unit/models/runtime/route_spec.rb | 98 ++++++------------- 5 files changed, 75 insertions(+), 91 deletions(-) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index ab2c43fadda..47295af0e44 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -6,14 +6,7 @@ class Error < StandardError def update(route:, message:) Route.db.transaction do - if message.requested?(:options) - # Merge existing options with new options from message - existing_options = route.options || {} - merged_options = existing_options.merge(message.options).compact - - # Set the options on the route (cleanup is handled by model) - route.options = merged_options - end + route.options = route.options.symbolize_keys.merge(message.options).compact if message.requested?(:options) route.save(raise_on_failure: true) MetadataUpdate.update(route, message) end @@ -30,7 +23,7 @@ def update(route:, message:) private - def validation_error!(error, route) + def validation_error!(error) # Handle hash_header validation error for hash loadbalancing if error.errors.on(:route)&.include?(:hash_header_missing) raise Error.new('Hash header must be present when loadbalancing is set to hash') diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index 55ec73042c6..968d22b743b 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -337,29 +337,26 @@ def validate_route_options hash_header = route_options[:hash_header] || route_options['hash_header'] if hash_header.blank? - errors.add(:route, :hash_header_missing) + errors.add(:options, 'Hash header must be present when loadbalancing is set to hash') end end def normalize_hash_balance_to_string(opts) return opts unless opts.is_a?(Hash) - return opts unless opts.key?('hash_balance') || opts.key?(:hash_balance) - - normalized = opts.deep_symbolize_keys - if normalized[:hash_balance].present? - normalized[:hash_balance] = normalized[:hash_balance].to_s - end + # We have a flat structure on options, so no deep_symbolize required + normalized = opts.symbolize_keys + normalized[:hash_balance] = normalized[:hash_balance].to_s if normalized[:hash_balance].present? normalized end def remove_hash_options_for_non_hash_loadbalancing(opts) return opts unless opts.is_a?(Hash) - opts_symbolized = opts.deep_symbolize_keys + opts_symbolized = opts.symbolize_keys loadbalancing = opts_symbolized[:loadbalancing] # Remove hash-specific options if loadbalancing is set to non-hash value - if loadbalancing.present? && loadbalancing != 'hash' + if loadbalancing != 'hash' opts_symbolized.delete(:hash_header) opts_symbolized.delete(:hash_balance) end 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 1842fc46288..53729e872fa 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,8 +7,8 @@ Example Route-Options object <%= yield_content :route_options %> ``` -| Name | Type | Description | -|-------------------|----------|------------------------------------------------------------------------------------------------------| -| **loadbalancing** | _string_ | The load-balancer associated with this route. Valid values are 'round-robin', 'least-connection', and 'hash' | -| **hash_header** | _string_ | HTTP header name to hash for routing (e.g., 'X-User-ID', 'Cookie'). Required when loadbalancing is 'hash'. Cannot be set when loadbalancing is not 'hash'. | -| **hash_balance** | _string_ | Weight factor for load balancing (0.0-100.0). 0.0 means ignore server load, higher values consider load more. Optional when loadbalancing is 'hash'. Cannot be set when loadbalancing is not 'hash'. | +| Name | Type | Description | +|-------------------|----------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| **loadbalancing** | _string_ | The load-balancer associated with this route. Valid values are 'round-robin', 'least-connection', and 'hash' | +| **hash_header** | _string_ | HTTP header name to hash for routing (e.g., 'X-User-ID', 'Cookie'). Required when loadbalancing is 'hash'. Cannot be set when loadbalancing is not 'hash'. | +| **hash_balance** | _string_ | Weight factor for load balancing (1.1 - 10, or 0 for disabling balancing). Higher values consider load more. Optional when loadbalancing is 'hash'. Cannot be set when loadbalancing is not 'hash'. | diff --git a/spec/unit/actions/route_update_spec.rb b/spec/unit/actions/route_update_spec.rb index 472714d2cad..b04a6b0ce34 100644 --- a/spec/unit/actions/route_update_spec.rb +++ b/spec/unit/actions/route_update_spec.rb @@ -190,7 +190,37 @@ module VCAP::CloudController end end - context 'when the route has existing options' do + context 'when the route has existing options for loadbalancing=hash' do + before do + route[:options] = '{"loadbalancing": "hash", "hash_header": "foobar", "hash_balance": "2"}' + end + + context 'when the loadbalancing option value is set to null' do + let(:body) do + { + options: { + loadbalancing: nil + } + } + end + + it 'removes this option and hash options' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to eq({}) + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + + end + + + context 'when the route has existing option loadbalancing=round-robin' do before do route[:options] = '{"loadbalancing": "round-robin"}' end diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index dd43451ef5c..e633457ffc9 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -899,15 +899,15 @@ module VCAP::CloudController let(:http_route) do Route.new(space: space, - domain: http_domain, - host: 'bar') + domain: http_domain, + host: 'bar') end subject(:tcp_route) do Route.new(space: space, - domain: tcp_domain, - host: '', - port: 6000) + domain: tcp_domain, + host: '', + port: 6000) end before do router_group = double('router_group', type: 'tcp', reservable_ports: [4444, 6000]) @@ -1151,24 +1151,6 @@ module VCAP::CloudController expect(route.options).to be_nil end end - - context 'when updating an existing route with hash_balance as float' do - it 'converts hash_balance to string on update' do - route = Route.make( - host: 'test-route', - domain: domain, - space: space, - options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 2.5 } - ) - - route.update(options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 2.5 }) - route.reload - - parsed_options = Oj.load(route.options_without_serialization) - expect(parsed_options['hash_balance']).to be_a(String) - expect(parsed_options['hash_balance']).to eq('2.5') - end - end end describe 'normalize_hash_balance_to_string' do @@ -1272,40 +1254,6 @@ module VCAP::CloudController let(:space) { Space.make } let(:domain) { PrivateDomain.make(owning_organization: space.organization) } - context 'when creating a route with round-robin and hash options' do - it 'removes hash_header and hash_balance' do - route = Route.make( - host: 'test-route', - domain: domain, - space: space, - options: { loadbalancing: 'round-robin', hash_header: 'X-User-ID', hash_balance: '1.5' } - ) - - route.reload - parsed_options = Oj.load(route.options_without_serialization) - expect(parsed_options['loadbalancing']).to eq('round-robin') - expect(parsed_options).not_to have_key('hash_header') - expect(parsed_options).not_to have_key('hash_balance') - end - end - - context 'when creating a route with least-connection and hash options' do - it 'removes hash_header and hash_balance' do - route = Route.make( - host: 'test-route', - domain: domain, - space: space, - options: { loadbalancing: 'least-connection', hash_header: 'X-User-ID', hash_balance: '2.0' } - ) - - route.reload - parsed_options = Oj.load(route.options_without_serialization) - expect(parsed_options['loadbalancing']).to eq('least-connection') - expect(parsed_options).not_to have_key('hash_header') - expect(parsed_options).not_to have_key('hash_balance') - end - end - context 'when creating a route with hash loadbalancing' do it 'keeps hash_header and hash_balance' do route = Route.make( @@ -1380,6 +1328,24 @@ module VCAP::CloudController end end + context 'when removing hash loadbalancing option' do + it 'deletes hash_header and hash_balance when present' do + route = Route.make( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '1.5' } + ) + + route.update(options: { loadbalancing: nil }) + route.reload + + parsed_options = Oj.load(route.options_without_serialization) + expect(parsed_options).not_to have_key('hash_header') + expect(parsed_options).not_to have_key('hash_balance') + end + end + context 'when using string keys instead of symbols' do it 'still removes hash options for non-hash loadbalancing' do route = Route.make( @@ -1459,17 +1425,15 @@ module VCAP::CloudController end context 'when loadbalancing is round-robin' do - context 'and hash_header is present' do - it 'is valid (no validation error for non-hash loadbalancing)' do - route = Route.new( - host: 'test-route', - domain: domain, - space: space, - options: { loadbalancing: 'round-robin' } - ) + it 'is valid' do + route = Route.new( + host: 'test-route', + domain: domain, + space: space, + options: { loadbalancing: 'round-robin' } + ) - expect(route).to be_valid - end + expect(route).to be_valid end end From cde7879cd1cdc2231142fe078eb10acebb616189 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 9 Jan 2026 16:46:10 +0100 Subject: [PATCH 31/45] Add more validations and tests --- app/actions/route_create.rb | 2 +- .../manifest_routes_update_message.rb | 21 ++- app/messages/route_options_message.rb | 22 ++- app/models/runtime/route.rb | 24 +++- spec/unit/actions/route_create_spec.rb | 112 +++++++++++++++ .../manifest_routes_update_message_spec.rb | 108 +++++++++++++- .../messages/route_create_message_spec.rb | 133 ++++++++++++++++++ .../messages/route_update_message_spec.rb | 39 +++++ spec/unit/models/runtime/route_spec.rb | 14 +- 9 files changed, 456 insertions(+), 19 deletions(-) diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index 87b9a95eb44..b755eb7cc16 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -78,7 +78,7 @@ def validation_error!(error, host, path, port, space, domain) error!("Reserved route ports quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_reserved_route_ports_exceeded) - error!("Hash header must be present when loadbalancing is set to hash") if error.errors.on(:route)&.include?(:hash_header_missing) + error!("Hash header must be present when loadbalancing is set to hash") if error.errors.on(:options)&.include?(:hash_header_missing) validation_error_routing_api!(error) validation_error_host!(error, host, domain) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 45d1e893642..7438f526d40 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -105,13 +105,28 @@ def hash_options_are_valid hash_header = options[:hash_header] hash_balance = options[:hash_balance] + # Validate hash_header length if present + if hash_header.present? + # Check length (at most 128 characters) + if hash_header.to_s.length > 128 + errors.add(:base, message: "Route '#{r[:route]}': Hash header must be at most 128 characters") + next + end + end + # Validate hash_balance is numeric if present if hash_balance.present? + # Check length first (at most 16 characters) + if hash_balance.to_s.length > 16 + errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be at most 16 characters") + next + end + begin balance_float = Float(hash_balance) - # Must be either 0 or >= 1.1 - unless balance_float == 0 || balance_float >= 1.1 - errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be either 0 or greater than or equal to 1.1") + # Must be either 0 or >= 1.1 and <= 10.0 + unless balance_float == 0 || (balance_float >= 1.1 && balance_float <= 10) + errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be either 0 or between to 1.1 and 10.0") end rescue ArgumentError, TypeError errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be a numeric value") diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 69b4eedf9c8..77a8b76211c 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -48,13 +48,29 @@ def hash_options_are_valid return unless VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) # Feature flag is enabled - validate hash-specific options + + # Validate hash_header length if present + if hash_header.present? + # Check length (at most 128 characters) + if hash_header.to_s.length > 128 + errors.add(:hash_header, 'must be at most 128 characters') + return + end + end + # Validate hash_balance is numeric if present if hash_balance.present? + # Check length first (at most 16 characters) + if hash_balance.to_s.length > 16 + errors.add(:hash_balance, 'must be at most 16 characters') + return + end + begin balance_float = Float(hash_balance) - # Must be either 0 or >= 1.1 - unless balance_float == 0 || balance_float >= 1.1 - errors.add(:hash_balance, 'must be either 0 or greater than or equal to 1.1') + # Must be either 0 or >= 1.1 and <= 10.0 + unless balance_float == 0 || (balance_float >= 1.1 && balance_float <= 10) + errors.add(:hash_balance, 'must be either 0 or between 1.1 and 10.0') end rescue ArgumentError, TypeError errors.add(:hash_balance, 'must be a numeric value') diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index 968d22b743b..ca616d7e044 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -76,7 +76,8 @@ def options_with_serialization=(opts) logger.error("CRITICAL: options_with_serialization= setter called", opts: opts.inspect, caller: caller_info, location: "#{__FILE__}:#{__LINE__}") cleaned_opts = remove_hash_options_for_non_hash_loadbalancing(opts) - normalized_opts = normalize_hash_balance_to_string(cleaned_opts) + rounded_opts = round_hash_balance_to_one_decimal(cleaned_opts) + normalized_opts = normalize_hash_balance_to_string(rounded_opts) self.options_without_serialization = Oj.dump(normalized_opts) logger.error("CRITICAL: options_with_serialization= setter completed", normalized_opts: normalized_opts.inspect, json: self.options_without_serialization.inspect) @@ -337,10 +338,29 @@ def validate_route_options hash_header = route_options[:hash_header] || route_options['hash_header'] if hash_header.blank? - errors.add(:options, 'Hash header must be present when loadbalancing is set to hash') + errors.add(:options, :hash_header_missing) end end + def round_hash_balance_to_one_decimal(opts) + return opts unless opts.is_a?(Hash) + + opts_symbolized = opts.symbolize_keys + hash_balance = opts_symbolized[:hash_balance] + + if hash_balance.present? + begin + balance_float = Float(hash_balance) + # Round to at most 1 decimal place + opts_symbolized[:hash_balance] = (balance_float * 10).round / 10.0 + rescue ArgumentError, TypeError + # If conversion fails, leave it as is - validation will catch it + end + end + + opts_symbolized + end + def normalize_hash_balance_to_string(opts) return opts unless opts.is_a?(Hash) # We have a flat structure on options, so no deep_symbolize required diff --git a/spec/unit/actions/route_create_spec.rb b/spec/unit/actions/route_create_spec.rb index fe40a2b50a4..c5b4c6a0035 100644 --- a/spec/unit/actions/route_create_spec.rb +++ b/spec/unit/actions/route_create_spec.rb @@ -108,6 +108,118 @@ module VCAP::CloudController end end + context 'when given route options' do + context 'when creating a route with loadbalancing=hash' do + context 'with hash_header but without hash_balance' do + let(:message_with_hash_options) do + RouteCreateMessage.new({ + relationships: { + space: { + data: { guid: space.guid } + }, + domain: { + data: { guid: domain.guid } + } + }, + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID' + } + }) + end + + it 'creates a route with hash loadbalancing and hash_header options' do + expect do + subject.create(message: message_with_hash_options, space: space, domain: domain) + end.to change(Route, :count).by(1) + + route = Route.last + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID' }) + end + end + + context 'with both hash_header and hash_balance' do + let(:message_with_hash_options) do + RouteCreateMessage.new({ + relationships: { + space: { + data: { guid: space.guid } + }, + domain: { + data: { guid: domain.guid } + } + }, + options: { + loadbalancing: 'hash', + hash_header: 'X-Session-ID', + hash_balance: '2' + } + }) + end + + it 'creates a route with hash loadbalancing, hash_header, and hash_balance options' do + expect do + subject.create(message: message_with_hash_options, space: space, domain: domain) + end.to change(Route, :count).by(1) + + route = Route.last + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-Session-ID', 'hash_balance' => '2.0' }) + end + end + + context 'without hash_header (required)' do + let(:message_without_hash_header) do + RouteCreateMessage.new({ + relationships: { + space: { + data: { guid: space.guid } + }, + domain: { + data: { guid: domain.guid } + } + }, + options: { + loadbalancing: 'hash' + } + }) + end + + it 'raises an error indicating hash_header is required' do + expect do + subject.create(message: message_without_hash_header, space: space, domain: domain) + end.to raise_error(RouteCreate::Error, 'Hash header must be present when loadbalancing is set to hash') + end + end + end + + context 'when creating a route with other loadbalancing options' do + let(:message_with_round_robin) do + RouteCreateMessage.new({ + relationships: { + space: { + data: { guid: space.guid } + }, + domain: { + data: { guid: domain.guid } + } + }, + options: { + loadbalancing: 'round-robin' + } + }) + end + + it 'creates a route with the specified loadbalancing option' do + expect do + subject.create(message: message_with_round_robin, space: space, domain: domain) + end.to change(Route, :count).by(1) + + route = Route.last + expect(route.options).to include({ 'loadbalancing' => 'round-robin' }) + end + end + end + context 'when the domain has an owning org that is different from the space\'s parent org' do let(:other_org) { VCAP::CloudController::Organization.make } let(:inaccessible_domain) { VCAP::CloudController::PrivateDomain.make(owning_organization: other_org) } diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index e5d1e8322d1..de1593fea43 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -488,6 +488,45 @@ module VCAP::CloudController end end + context 'when a route contains hash_header longer than 128 characters' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X' * 129 + } } + ] } + 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': Hash header must be at most 128 characters") + end + end + + context 'when a route contains hash_header exactly 128 characters' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X' * 128 + } } + ] } + end + + it 'returns true' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + context 'when a route contains hash_balance without loadbalancing' do let(:body) do { 'routes' => @@ -603,7 +642,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': Hash balance must be either 0 or greater than or equal to 1.1") + expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash balance must be either 0 or between to 1.1 and 10.0") end end @@ -627,7 +666,28 @@ module VCAP::CloudController end end - context 'when a route contains hash_balance greater than 1.1' do + context 'when a route contains hash_balance greater than 10.0' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => 10.1 + } } + ] } + 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': Hash balance must be either 0 or between to 1.1 and 10.0") + end + end + + context 'when a route contains hash_balance exactly 10.0' do let(:body) do { 'routes' => [ @@ -635,7 +695,7 @@ module VCAP::CloudController 'options' => { 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID', - 'hash_balance' => 3.5 + 'hash_balance' => 10.0 } } ] } end @@ -647,6 +707,7 @@ module VCAP::CloudController end end + context 'when a route contains numeric string hash_balance' do let(:body) do { 'routes' => @@ -687,6 +748,47 @@ module VCAP::CloudController end end + context 'when a route contains hash_balance longer than 16 characters' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => '12345678901234567' + } } + ] } + 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': Hash balance must be at most 16 characters") + end + end + + context 'when a route contains hash_balance exactly 16 characters' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X-User-ID', + 'hash_balance' => '9.9' + } } + ] } + end + + it 'returns true if the value is numeric and within range' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(true) + end + end + context 'when multiple routes have mixed valid and invalid hash options' do let(:body) do { 'routes' => diff --git a/spec/unit/messages/route_create_message_spec.rb b/spec/unit/messages/route_create_message_spec.rb index cbfe96b8b3d..c77b9c6d15e 100644 --- a/spec/unit/messages/route_create_message_spec.rb +++ b/spec/unit/messages/route_create_message_spec.rb @@ -496,6 +496,139 @@ module VCAP::CloudController end end end + + context 'when hash_based_routing feature flag is enabled' do + before do + VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: true) + end + + context 'when hash_header is longer than 128 characters' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { + loadbalancing: 'hash', + hash_header: 'X' * 129 + } + } + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:options]).to include('Hash header must be at most 128 characters') + end + end + + context 'when hash_header is exactly 128 characters' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { + loadbalancing: 'hash', + hash_header: 'X' * 128 + } + } + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when hash_balance is longer than 16 characters' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID', + hash_balance: '12345678901234567' + } + } + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:options]).to include('Hash balance must be at most 16 characters') + end + end + + context 'when hash_balance is exactly 16 characters' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID', + hash_balance: '9.9' + } + } + end + + it 'is valid' do + expect(subject).to be_valid + end + end + + context 'when hash_balance is greater than 10.0' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID', + hash_balance: 10.1 + } + } + end + + it 'is not valid' do + expect(subject).not_to be_valid + expect(subject.errors[:options]).to include('Hash balance must be either 0 or between 1.1 and 10.0') + end + end + + context 'when hash_balance is exactly 10.0' do + let(:params) do + { + host: 'some-host', + relationships: { + space: { data: { guid: 'space-guid' } }, + domain: { data: { guid: 'domain-guid' } } + }, + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID', + hash_balance: 10.0 + } + } + end + + it 'is valid' do + expect(subject).to be_valid + end + end + end end end diff --git a/spec/unit/messages/route_update_message_spec.rb b/spec/unit/messages/route_update_message_spec.rb index 926e8289ead..6c7b1e3f65a 100644 --- a/spec/unit/messages/route_update_message_spec.rb +++ b/spec/unit/messages/route_update_message_spec.rb @@ -61,6 +61,45 @@ module VCAP::CloudController expect(message).not_to be_valid expect(message.errors.full_messages[0]).to include("Options Unknown field(s): 'gorgonzola'") end + + context 'when hash_based_routing feature flag is enabled' do + before do + VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: true) + end + + it 'does not accept hash_header longer than 128 characters' do + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'hash', hash_header: 'X' * 129 })) + expect(message).not_to be_valid + expect(message.errors.full_messages[0]).to include('Hash header must be at most 128 characters') + end + + it 'accepts hash_header exactly 128 characters' do + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'hash', hash_header: 'X' * 128 })) + expect(message).to be_valid + end + + it 'does not accept hash_balance longer than 16 characters' do + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '12345678901234567' })) + expect(message).not_to be_valid + expect(message.errors.full_messages[0]).to include('Hash balance must be at most 16 characters') + end + + it 'accepts hash_balance exactly 16 characters' do + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '9.9' })) + expect(message).to be_valid + end + + it 'does not accept hash_balance greater than 10.0' do + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 10.1 })) + expect(message).not_to be_valid + expect(message.errors.full_messages[0]).to include('Hash balance must be either 0 or between 1.1 and 10.0') + end + + it 'accepts hash_balance exactly 10.0' do + message = RouteUpdateMessage.new(params.merge(options: { loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 10.0 })) + expect(message).to be_valid + end + end end end end diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index e633457ffc9..2e38969032c 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -1087,7 +1087,7 @@ module VCAP::CloudController route.reload parsed_options = Oj.load(route.options_without_serialization) expect(parsed_options['hash_balance']).to be_a(String) - expect(parsed_options['hash_balance']).to eq('2') + expect(parsed_options['hash_balance']).to eq('2.0') end end @@ -1103,7 +1103,7 @@ module VCAP::CloudController route.reload parsed_options = Oj.load(route.options_without_serialization) expect(parsed_options['hash_balance']).to be_a(String) - expect(parsed_options['hash_balance']).to eq('1.25') + expect(parsed_options['hash_balance']).to eq('1.3') end end @@ -1119,7 +1119,7 @@ module VCAP::CloudController route.reload parsed_options = Oj.load(route.options_without_serialization) expect(parsed_options['hash_balance']).to be_a(String) - expect(parsed_options['hash_balance']).to eq('0') + expect(parsed_options['hash_balance']).to eq('0.0') end end @@ -1392,7 +1392,7 @@ module VCAP::CloudController ) expect(route).not_to be_valid - expect(route.errors[:options]).to include('Hash header must be present when loadbalancing is set to hash') + expect(route.errors[:options]).to include :hash_header_missing end end @@ -1406,7 +1406,7 @@ module VCAP::CloudController ) expect(route).not_to be_valid - expect(route.errors[:options]).to include('Hash header must be present when loadbalancing is set to hash') + expect(route.errors[:options]).to include :hash_header_missing end end @@ -1489,7 +1489,7 @@ module VCAP::CloudController route.options = { loadbalancing: 'hash' } expect(route).not_to be_valid - expect(route.errors[:options]).to include('Hash header must be present when loadbalancing is set to hash') + expect(route.errors[:options]).to include :hash_header_missing end end @@ -1533,7 +1533,7 @@ module VCAP::CloudController ) expect(route).not_to be_valid - expect(route.errors[:options]).to include('Hash header must be present when loadbalancing is set to hash') + expect(route.errors[:options]).to include :hash_header_missing end end end From 3e414e99be2fe37bbe33172534bef981b7c38825 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 9 Jan 2026 17:01:56 +0100 Subject: [PATCH 32/45] Add more tests for route_update --- app/actions/route_create.rb | 2 +- app/actions/route_update.rb | 2 +- app/models/runtime/route.rb | 2 +- spec/unit/actions/route_update_spec.rb | 225 +++++++++++++++++++++++++ spec/unit/models/runtime/route_spec.rb | 8 +- 5 files changed, 232 insertions(+), 7 deletions(-) diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index b755eb7cc16..87b9a95eb44 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -78,7 +78,7 @@ def validation_error!(error, host, path, port, space, domain) error!("Reserved route ports quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_reserved_route_ports_exceeded) - error!("Hash header must be present when loadbalancing is set to hash") if error.errors.on(:options)&.include?(:hash_header_missing) + error!("Hash header must be present when loadbalancing is set to hash") if error.errors.on(:route)&.include?(:hash_header_missing) validation_error_routing_api!(error) validation_error_host!(error, host, domain) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 47295af0e44..10068c5ac51 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -18,7 +18,7 @@ def update(route:, message:) end route rescue Sequel::ValidationFailed => e - validation_error!(e, route) + validation_error!(e) end private diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index ca616d7e044..b74db9ae1ea 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -338,7 +338,7 @@ def validate_route_options hash_header = route_options[:hash_header] || route_options['hash_header'] if hash_header.blank? - errors.add(:options, :hash_header_missing) + errors.add(:route, :hash_header_missing) end end diff --git a/spec/unit/actions/route_update_spec.rb b/spec/unit/actions/route_update_spec.rb index b04a6b0ce34..61da17a18ff 100644 --- a/spec/unit/actions/route_update_spec.rb +++ b/spec/unit/actions/route_update_spec.rb @@ -192,6 +192,7 @@ module VCAP::CloudController context 'when the route has existing options for loadbalancing=hash' do before do + VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: true) route[:options] = '{"loadbalancing": "hash", "hash_header": "foobar", "hash_balance": "2"}' end @@ -217,6 +218,160 @@ module VCAP::CloudController end end + context 'when updating only hash_header' do + let(:body) do + { + options: { + hash_header: 'X-New-Header' + } + } + end + + it 'updates hash_header while keeping other options' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-New-Header', 'hash_balance' => '2.0' }) + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + + context 'when updating only hash_balance' do + let(:body) do + { + options: { + hash_balance: '3.5' + } + } + end + + it 'updates hash_balance while keeping other options' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'foobar', 'hash_balance' => '3.5' }) + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + + context 'when updating both hash_header and hash_balance' do + let(:body) do + { + options: { + hash_header: 'X-Updated-Header', + hash_balance: '5.0' + } + } + end + + it 'updates both hash options while keeping loadbalancing' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-Updated-Header', 'hash_balance' => '5.0' }) + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + + context 'when setting hash_balance to null' do + let(:body) do + { + options: { + hash_balance: nil + } + } + end + + it 'removes hash_balance while keeping other options' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'foobar' }) + expect(route.options).not_to have_key('hash_balance') + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + + context 'when updating from hash to round-robin' do + let(:body) do + { + options: { + loadbalancing: 'round-robin' + } + } + end + + it 'updates to round-robin and removes hash options' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to eq({ 'loadbalancing' => 'round-robin' }) + expect(route.options).not_to have_key('hash_header') + expect(route.options).not_to have_key('hash_balance') + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + + context 'when updating from hash to least-connection' do + let(:body) do + { + options: { + loadbalancing: 'least-connection' + } + } + end + + it 'updates to least-connection and removes hash options' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to eq({ 'loadbalancing' => 'least-connection' }) + expect(route.options).not_to have_key('hash_header') + expect(route.options).not_to have_key('hash_balance') + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + + context 'when setting hash_header to null' do + let(:body) do + { + options: { + hash_header: nil + } + } + end + + it 'raises an error because hash_header is required for hash loadbalancing' do + expect(message).to be_valid + expect do + subject.update(route:, message:) + end.to raise_error(RouteUpdate::Error, 'Hash header must be present when loadbalancing is set to hash') + end + end end @@ -243,6 +398,76 @@ module VCAP::CloudController end end + context 'when hash_based_routing feature flag is enabled' do + before do + VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: true) + end + + context 'when updating to hash loadbalancing without hash_header' do + let(:body) do + { + options: { + loadbalancing: 'hash' + } + } + end + + it 'raises an error' do + expect(message).to be_valid + expect do + subject.update(route:, message:) + end.to raise_error(RouteUpdate::Error, 'Hash header must be present when loadbalancing is set to hash') + end + end + + context 'when updating to hash loadbalancing with hash_header' do + let(:body) do + { + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID' + } + } + end + + it 'successfully updates to hash loadbalancing' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID' }) + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + + context 'when updating to hash loadbalancing with hash_header and hash_balance' do + let(:body) do + { + options: { + loadbalancing: 'hash', + hash_header: 'X-Session-ID', + hash_balance: '2.5' + } + } + end + + it 'successfully updates to hash loadbalancing with all options' do + expect(message).to be_valid + subject.update(route:, message:) + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-Session-ID', 'hash_balance' => '2.5' }) + end + + it 'notifies the backend' do + expect(fake_route_handler).to receive(:notify_backend_of_route_update) + subject.update(route:, message:) + end + end + end + context 'when an option is specified' do let(:body) do { diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index 2e38969032c..8a498c04721 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -1392,7 +1392,7 @@ module VCAP::CloudController ) expect(route).not_to be_valid - expect(route.errors[:options]).to include :hash_header_missing + expect(route.errors[:route]).to include :hash_header_missing end end @@ -1406,7 +1406,7 @@ module VCAP::CloudController ) expect(route).not_to be_valid - expect(route.errors[:options]).to include :hash_header_missing + expect(route.errors[:route]).to include :hash_header_missing end end @@ -1489,7 +1489,7 @@ module VCAP::CloudController route.options = { loadbalancing: 'hash' } expect(route).not_to be_valid - expect(route.errors[:options]).to include :hash_header_missing + expect(route.errors[:route]).to include :hash_header_missing end end @@ -1533,7 +1533,7 @@ module VCAP::CloudController ) expect(route).not_to be_valid - expect(route.errors[:options]).to include :hash_header_missing + expect(route.errors[:route]).to include :hash_header_missing end end end From 7373fe2e9d7de452c8360d9999616dfd7813cfdb Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 9 Jan 2026 17:11:49 +0100 Subject: [PATCH 33/45] Add more tests --- .../actions/manifest_route_update_spec.rb | 264 ++++++++++++++++++ 1 file changed, 264 insertions(+) diff --git a/spec/unit/actions/manifest_route_update_spec.rb b/spec/unit/actions/manifest_route_update_spec.rb index 623aa6384b6..3c6935605c5 100644 --- a/spec/unit/actions/manifest_route_update_spec.rb +++ b/spec/unit/actions/manifest_route_update_spec.rb @@ -393,6 +393,270 @@ module VCAP::CloudController end.to raise_error(ManifestRouteUpdate::InvalidRoute, /Host format is invalid/) end end + + context 'when route options are provided' do + let!(:domain) { VCAP::CloudController::SharedDomain.make(name: 'tomato.avocado-toast.com') } + + before do + VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: true) + end + + context 'when creating a new route with loadbalancing options' do + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { loadbalancing: 'round-robin' } + } + ] + ) + end + + it 'creates the route with the specified loadbalancing option' do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + + routes = app.reload.routes + expect(routes.length).to eq(1) + + route = routes.first + expect(route.options).to include({ 'loadbalancing' => 'round-robin' }) + end + end + + context 'when creating a new route with hash loadbalancing and hash_header' do + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID' + } + } + ] + ) + end + + it 'creates the route with hash loadbalancing options' do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + + routes = app.reload.routes + expect(routes.length).to eq(1) + + route = routes.first + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID' }) + end + end + + context 'when creating a new route with hash loadbalancing, hash_header, and hash_balance' do + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { + loadbalancing: 'hash', + hash_header: 'X-Session-ID', + hash_balance: '2.5' + } + } + ] + ) + end + + it 'creates the route with all hash loadbalancing options' do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + + routes = app.reload.routes + expect(routes.length).to eq(1) + + route = routes.first + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-Session-ID', 'hash_balance' => '2.5' }) + end + end + + context 'when creating a new route with hash loadbalancing but missing hash_header' do + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { loadbalancing: 'hash' } + } + ] + ) + end + + it 'raises an error indicating hash_header is required' do + expect do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + end.to raise_error(ManifestRouteUpdate::InvalidRoute, /Hash header must be present when loadbalancing is set to hash/) + end + end + + context 'when updating an existing route with new loadbalancing options' do + let!(:route) { Route.make(host: 'potato', domain: domain, path: '/some-path', space: app.space) } + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { loadbalancing: 'least-connection' } + } + ] + ) + end + + it 'updates the existing route with the new loadbalancing option' do + expect do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + end.not_to(change { Route.count }) + + route.reload + expect(route.options).to include({ 'loadbalancing' => 'least-connection' }) + end + end + + context 'when updating an existing route from round-robin to hash' do + let!(:route) do + Route.make( + host: 'potato', + domain: domain, + path: '/some-path', + space: app.space, + options: { loadbalancing: 'round-robin' } + ) + end + + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID' + } + } + ] + ) + end + + it 'updates the route to hash loadbalancing with hash_header' do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID' }) + end + end + + context 'when updating an existing hash route with new hash_header' do + let!(:route) do + Route.make( + host: 'potato', + domain: domain, + path: '/some-path', + space: app.space, + options: { + loadbalancing: 'hash', + hash_header: 'X-Old-Header', + hash_balance: '2.0' + } + ) + end + + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { hash_header: 'X-New-Header' } + } + ] + ) + end + + it 'updates only the hash_header while keeping other options' do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-New-Header', 'hash_balance' => '2.0' }) + end + end + + context 'when updating an existing hash route with new hash_balance' do + let!(:route) do + Route.make( + host: 'potato', + domain: domain, + path: '/some-path', + space: app.space, + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID', + hash_balance: '2.0' + } + ) + end + + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { hash_balance: '5.0' } + } + ] + ) + end + + it 'updates only the hash_balance while keeping other options' do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + + route.reload + expect(route.options).to include({ 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID', 'hash_balance' => '5.0' }) + end + end + + context 'when updating an existing hash route to remove loadbalancing' do + let!(:route) do + Route.make( + host: 'potato', + domain: domain, + path: '/some-path', + space: app.space, + options: { + loadbalancing: 'hash', + hash_header: 'X-User-ID', + hash_balance: '2.0' + } + ) + end + + let(:message) do + ManifestRoutesUpdateMessage.new( + routes: [ + { + route: 'http://potato.tomato.avocado-toast.com/some-path', + options: { loadbalancing: nil } + } + ] + ) + end + + it 'removes loadbalancing and hash options' do + ManifestRouteUpdate.update(app.guid, message, user_audit_info) + + route.reload + expect(route.options).to eq({}) + expect(route.options).not_to have_key('loadbalancing') + expect(route.options).not_to have_key('hash_header') + expect(route.options).not_to have_key('hash_balance') + end + end + end end end end From 335b9100a5a23cb585468553b16f349d2cd8849a Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 9 Jan 2026 18:13:09 +0100 Subject: [PATCH 34/45] Some test fixes and cleanup --- app/actions/manifest_route_update.rb | 2 -- app/actions/route_create.rb | 2 +- app/actions/route_update.rb | 4 ++-- app/models/runtime/route.rb | 4 +++- spec/unit/messages/validators_spec.rb | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/actions/manifest_route_update.rb b/app/actions/manifest_route_update.rb index c5c7a72a39f..59fab7b4e6f 100644 --- a/app/actions/manifest_route_update.rb +++ b/app/actions/manifest_route_update.rb @@ -95,8 +95,6 @@ def find_or_create_valid_route(app, manifest_route, user_audit_info) elsif !route.available_in_space?(app.space) raise InvalidRoute.new('Routes cannot be mapped to destinations in different spaces') elsif 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_create.rb b/app/actions/route_create.rb index 87b9a95eb44..d973daaf528 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -20,7 +20,7 @@ def create(message:, space:, domain:, manifest_triggered: false) ) Route.db.transaction do - route.save(raise_on_failure: true) + route.save MetadataUpdate.update(route, message) end diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 10068c5ac51..2268aca8f64 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -6,8 +6,8 @@ class Error < StandardError def update(route:, message:) Route.db.transaction do - route.options = route.options.symbolize_keys.merge(message.options).compact if message.requested?(:options) - route.save(raise_on_failure: true) + route.options = route.options.symbolize_keys.merge(message.options) if message.requested?(:options) + route.save MetadataUpdate.update(route, message) end diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index b74db9ae1ea..663f6f77f83 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -78,6 +78,8 @@ def options_with_serialization=(opts) cleaned_opts = remove_hash_options_for_non_hash_loadbalancing(opts) rounded_opts = round_hash_balance_to_one_decimal(cleaned_opts) normalized_opts = normalize_hash_balance_to_string(rounded_opts) + # Remove nil values after all processing + normalized_opts = normalized_opts.compact if normalized_opts.is_a?(Hash) self.options_without_serialization = Oj.dump(normalized_opts) logger.error("CRITICAL: options_with_serialization= setter completed", normalized_opts: normalized_opts.inspect, json: self.options_without_serialization.inspect) @@ -330,7 +332,7 @@ def validate_total_reserved_route_ports def validate_route_options return if options.blank? - route_options = options.is_a?(Hash) ? options : options.deep_symbolize_keys + route_options = options.is_a?(Hash) ? options : options.symbolize_keys loadbalancing = route_options[:loadbalancing] || route_options['loadbalancing'] return if loadbalancing != 'hash' diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 5c7099687ff..250175ace57 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -708,7 +708,7 @@ def self.enabled?(flag_name, **) it 'does not allow hash_balance between 0 and 1.1' do message = OptionsMessage.new({ options: { hash_balance: 0.5 } }) expect(message).not_to be_valid - expect(message.errors.full_messages).to include('Options Hash balance must be either 0 or greater than or equal to 1.1') + expect(message.errors.full_messages).to include('Options Hash balance must be either 0 or between 1.1 and 10.0') end it 'allows numeric string hash_balance' do From 0c7c7477ae991e7fa033d58fac669beb42bdcbb7 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Fri, 9 Jan 2026 21:10:18 +0100 Subject: [PATCH 35/45] Use blank? instead of nil? for loadbalancing check --- app/messages/route_options_message.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 77a8b76211c..6dc8b4366c9 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -23,7 +23,7 @@ def self.valid_loadbalancing_algorithms validate :hash_options_are_valid def loadbalancing_algorithm_is_valid - return if loadbalancing.nil? + return if loadbalancing.blank? return if self.class.valid_loadbalancing_algorithms.include?(loadbalancing) errors.add(:loadbalancing, "must be one of '#{self.class.valid_loadbalancing_algorithms.join(', ')}' if present") From e293c12e7199ef116cb9f92dd53cefa8171e3d66 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Mon, 12 Jan 2026 09:22:22 +0100 Subject: [PATCH 36/45] Fix rubocop issues --- app/actions/route_create.rb | 2 +- app/actions/route_update.rb | 5 +---- app/messages/manifest_routes_update_message.rb | 17 +++++++---------- app/messages/route_options_message.rb | 13 ++++--------- app/models/runtime/route.rb | 13 ++++--------- 5 files changed, 17 insertions(+), 33 deletions(-) diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index d973daaf528..a77326ee899 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -78,7 +78,7 @@ def validation_error!(error, host, path, port, space, domain) error!("Reserved route ports quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_reserved_route_ports_exceeded) - error!("Hash header must be present when loadbalancing is set to hash") if error.errors.on(:route)&.include?(:hash_header_missing) + error!('Hash header must be present when loadbalancing is set to hash') if error.errors.on(:route)&.include?(:hash_header_missing) validation_error_routing_api!(error) validation_error_host!(error, host, domain) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 2268aca8f64..30f8b2a64da 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -4,7 +4,6 @@ class Error < StandardError end def update(route:, message:) - Route.db.transaction do route.options = route.options.symbolize_keys.merge(message.options) if message.requested?(:options) route.save @@ -25,9 +24,7 @@ def update(route:, message:) def validation_error!(error) # Handle hash_header validation error for hash loadbalancing - if error.errors.on(:route)&.include?(:hash_header_missing) - raise Error.new('Hash header must be present when loadbalancing is set to hash') - end + raise Error.new('Hash header must be present when loadbalancing is set to hash') if error.errors.on(:route)&.include?(:hash_header_missing) # Fallback for any other validation errors raise Error.new(error.message) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 7438f526d40..2fecc238aac 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -61,7 +61,7 @@ def route_options_are_valid r[:options].each_key do |key| RouteOptionsMessage.valid_route_options.exclude?(key) && errors.add(:base, - message: "Route '#{r[:route]}' contains invalid route option '#{key}'. \ + message: "Route '#{r[:route]}' contains invalid route option '#{key}'. \ Valid keys: '#{RouteOptionsMessage.valid_route_options.join(', ')}'") end end @@ -76,13 +76,13 @@ def loadbalancings_are_valid loadbalancing = r[:options][:loadbalancing] unless loadbalancing.is_a?(String) errors.add(:base, - message: "Invalid value for 'loadbalancing' for Route '#{r[:route]}'; \ + 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: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \ + message: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \ Valid values are: '#{RouteOptionsMessage.valid_loadbalancing_algorithms.join(', ')}'") end end @@ -106,12 +106,9 @@ def hash_options_are_valid hash_balance = options[:hash_balance] # Validate hash_header length if present - if hash_header.present? - # Check length (at most 128 characters) - if hash_header.to_s.length > 128 - errors.add(:base, message: "Route '#{r[:route]}': Hash header must be at most 128 characters") - next - end + if hash_header.present? && (hash_header.to_s.length > 128) + errors.add(:base, message: "Route '#{r[:route]}': Hash header must be at most 128 characters") + next end # Validate hash_balance is numeric if present @@ -125,7 +122,7 @@ def hash_options_are_valid begin balance_float = Float(hash_balance) # Must be either 0 or >= 1.1 and <= 10.0 - unless balance_float == 0 || (balance_float >= 1.1 && balance_float <= 10) + unless balance_float == 0 || balance_float.between?(1.1, 10) errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be either 0 or between to 1.1 and 10.0") end rescue ArgumentError, TypeError diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index 6dc8b4366c9..f37ab2d939d 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -50,12 +50,9 @@ def hash_options_are_valid # Feature flag is enabled - validate hash-specific options # Validate hash_header length if present - if hash_header.present? - # Check length (at most 128 characters) - if hash_header.to_s.length > 128 - errors.add(:hash_header, 'must be at most 128 characters') - return - end + if hash_header.present? && (hash_header.to_s.length > 128) + errors.add(:hash_header, 'must be at most 128 characters') + return end # Validate hash_balance is numeric if present @@ -69,9 +66,7 @@ def hash_options_are_valid begin balance_float = Float(hash_balance) # Must be either 0 or >= 1.1 and <= 10.0 - unless balance_float == 0 || (balance_float >= 1.1 && balance_float <= 10) - errors.add(:hash_balance, 'must be either 0 or between 1.1 and 10.0') - end + errors.add(:hash_balance, 'must be either 0 or between 1.1 and 10.0') unless balance_float == 0 || balance_float.between?(1.1, 10) rescue ArgumentError, TypeError errors.add(:hash_balance, 'must be a numeric value') end diff --git a/app/models/runtime/route.rb b/app/models/runtime/route.rb index 663f6f77f83..1a54bd72b44 100644 --- a/app/models/runtime/route.rb +++ b/app/models/runtime/route.rb @@ -71,18 +71,12 @@ def as_summary_json end def options_with_serialization=(opts) - logger = Steno.logger('cc.model.route') - caller_info = caller[0..5].join("\n ") - logger.error("CRITICAL: options_with_serialization= setter called", opts: opts.inspect, caller: caller_info, location: "#{__FILE__}:#{__LINE__}") - cleaned_opts = remove_hash_options_for_non_hash_loadbalancing(opts) rounded_opts = round_hash_balance_to_one_decimal(cleaned_opts) normalized_opts = normalize_hash_balance_to_string(rounded_opts) # Remove nil values after all processing normalized_opts = normalized_opts.compact if normalized_opts.is_a?(Hash) self.options_without_serialization = Oj.dump(normalized_opts) - - logger.error("CRITICAL: options_with_serialization= setter completed", normalized_opts: normalized_opts.inspect, json: self.options_without_serialization.inspect) end alias_method :options_without_serialization=, :options= @@ -339,9 +333,9 @@ def validate_route_options hash_header = route_options[:hash_header] || route_options['hash_header'] - if hash_header.blank? - errors.add(:route, :hash_header_missing) - end + return if hash_header.present? + + errors.add(:route, :hash_header_missing) end def round_hash_balance_to_one_decimal(opts) @@ -365,6 +359,7 @@ def round_hash_balance_to_one_decimal(opts) def normalize_hash_balance_to_string(opts) return opts unless opts.is_a?(Hash) + # We have a flat structure on options, so no deep_symbolize required normalized = opts.symbolize_keys normalized[:hash_balance] = normalized[:hash_balance].to_s if normalized[:hash_balance].present? From 9589e1614c5c05be5029cc86b40f7c12c9c87858 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Mon, 12 Jan 2026 10:40:25 +0100 Subject: [PATCH 37/45] Fix rubocop spec issues --- .../unit/actions/manifest_route_update_spec.rb | 2 +- spec/unit/actions/route_update_spec.rb | 1 - .../manifest_routes_update_message_spec.rb | 7 ++++--- spec/unit/models/runtime/route_spec.rb | 18 +++++++++--------- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/unit/actions/manifest_route_update_spec.rb b/spec/unit/actions/manifest_route_update_spec.rb index 3c6935605c5..1b4794198df 100644 --- a/spec/unit/actions/manifest_route_update_spec.rb +++ b/spec/unit/actions/manifest_route_update_spec.rb @@ -512,7 +512,7 @@ module VCAP::CloudController it 'updates the existing route with the new loadbalancing option' do expect do ManifestRouteUpdate.update(app.guid, message, user_audit_info) - end.not_to(change { Route.count }) + end.not_to(change(Route, :count)) route.reload expect(route.options).to include({ 'loadbalancing' => 'least-connection' }) diff --git a/spec/unit/actions/route_update_spec.rb b/spec/unit/actions/route_update_spec.rb index 61da17a18ff..219bc4fc426 100644 --- a/spec/unit/actions/route_update_spec.rb +++ b/spec/unit/actions/route_update_spec.rb @@ -374,7 +374,6 @@ module VCAP::CloudController end end - context 'when the route has existing option loadbalancing=round-robin' do before do route[:options] = '{"loadbalancing": "round-robin"}' diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index de1593fea43..981d339c715 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -339,7 +339,8 @@ module VCAP::CloudController expect(msg.valid?).to be(false) expect(msg.errors.full_messages).to include( - "Cannot use loadbalancing value 'hash' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connection'") + "Cannot use loadbalancing value 'hash' for Route 'existing.example.com'; Valid values are: 'round-robin, least-connection'" + ) end end @@ -359,7 +360,8 @@ module VCAP::CloudController expect(msg.valid?).to be(false) expect(msg.errors.full_messages).to include( - "Route 'existing.example.com' contains invalid route option 'hash_header'. Valid keys: 'loadbalancing'") + "Route 'existing.example.com' contains invalid route option 'hash_header'. Valid keys: 'loadbalancing'" + ) end end @@ -707,7 +709,6 @@ module VCAP::CloudController end end - context 'when a route contains numeric string hash_balance' do let(:body) do { 'routes' => diff --git a/spec/unit/models/runtime/route_spec.rb b/spec/unit/models/runtime/route_spec.rb index 8a498c04721..c6554939adc 100644 --- a/spec/unit/models/runtime/route_spec.rb +++ b/spec/unit/models/runtime/route_spec.rb @@ -899,15 +899,15 @@ module VCAP::CloudController let(:http_route) do Route.new(space: space, - domain: http_domain, - host: 'bar') + domain: http_domain, + host: 'bar') end subject(:tcp_route) do Route.new(space: space, - domain: tcp_domain, - host: '', - port: 6000) + domain: tcp_domain, + host: '', + port: 6000) end before do router_group = double('router_group', type: 'tcp', reservable_ports: [4444, 6000]) @@ -1238,10 +1238,10 @@ module VCAP::CloudController context 'with complete options hash' do it 'normalizes hash_balance while preserving other options' do result = route.send(:normalize_hash_balance_to_string, { - loadbalancing: 'hash', - hash_header: 'X-User-ID', - hash_balance: 3.14159 - }) + loadbalancing: 'hash', + hash_header: 'X-User-ID', + hash_balance: 3.14159 + }) expect(result[:loadbalancing]).to eq('hash') expect(result[:hash_header]).to eq('X-User-ID') expect(result[:hash_balance]).to eq('3.14159') From d39f0a3184a5102a08e5d3346f04ce32f2d7fe2e Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Mon, 12 Jan 2026 13:41:31 +0100 Subject: [PATCH 38/45] Refactor to resolve rubocop complexity findings --- app/actions/route_create.rb | 32 ++++--- .../manifest_routes_update_message.rb | 84 +++++++++++-------- app/messages/route_options_message.rb | 46 +++++----- 3 files changed, 96 insertions(+), 66 deletions(-) diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index a77326ee899..4191a0c1cd1 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -68,24 +68,34 @@ def route_resource_manager end def validation_error!(error, host, path, port, space, domain) - error!("Invalid domain. Domain '#{domain.name}' is not available in organization '#{space.organization.name}'.") if error.errors.on(:domain)&.include?(:invalid_relation) + check_domain_errors!(error, space, domain) + check_quota_errors!(error, space) + check_route_errors!(error) + validation_error_routing_api!(error) + validation_error_host!(error, host, domain) + validation_error_path!(error, host, path, domain) + validation_error_port!(error, host, port, domain) - error!("Routes quota exceeded for space '#{space.name}'.") if error.errors.on(:space)&.include?(:total_routes_exceeded) + error!(error.message) + end - error!("Reserved route ports quota exceeded for space '#{space.name}'.") if error.errors.on(:space)&.include?(:total_reserved_route_ports_exceeded) + def check_domain_errors!(error, space, domain) + return unless error.errors.on(:domain)&.include?(:invalid_relation) - error!("Routes quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_routes_exceeded) + error!("Invalid domain. Domain '#{domain.name}' is not available in organization '#{space.organization.name}'.") + end + def check_quota_errors!(error, space) + error!("Routes quota exceeded for space '#{space.name}'.") if error.errors.on(:space)&.include?(:total_routes_exceeded) + error!("Reserved route ports quota exceeded for space '#{space.name}'.") if error.errors.on(:space)&.include?(:total_reserved_route_ports_exceeded) + error!("Routes quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_routes_exceeded) error!("Reserved route ports quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_reserved_route_ports_exceeded) + end - error!('Hash header must be present when loadbalancing is set to hash') if error.errors.on(:route)&.include?(:hash_header_missing) - - validation_error_routing_api!(error) - validation_error_host!(error, host, domain) - validation_error_path!(error, host, path, domain) - validation_error_port!(error, host, port, domain) + def check_route_errors!(error) + return unless error.errors.on(:route)&.include?(:hash_header_missing) - error!(error.message) + error!('Hash header must be present when loadbalancing is set to hash') end def validation_error_routing_api!(error) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 2fecc238aac..8cdcac7e2f8 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -100,45 +100,59 @@ def hash_options_are_valid routes.each do |r| next unless r.keys.include?(:options) && r[:options].is_a?(Hash) - options = r[:options] - loadbalancing = options[:loadbalancing] - hash_header = options[:hash_header] - hash_balance = options[:hash_balance] - - # Validate hash_header length if present - if hash_header.present? && (hash_header.to_s.length > 128) - errors.add(:base, message: "Route '#{r[:route]}': Hash header must be at most 128 characters") - next - end + validate_route_hash_options(r) + end + end - # Validate hash_balance is numeric if present - if hash_balance.present? - # Check length first (at most 16 characters) - if hash_balance.to_s.length > 16 - errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be at most 16 characters") - next - end - - begin - balance_float = Float(hash_balance) - # Must be either 0 or >= 1.1 and <= 10.0 - unless balance_float == 0 || balance_float.between?(1.1, 10) - errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be either 0 or between to 1.1 and 10.0") - end - rescue ArgumentError, TypeError - errors.add(:base, message: "Route '#{r[:route]}': Hash balance must be a numeric value") - end - end + def validate_route_hash_options(route) + options = route[:options] + loadbalancing = options[:loadbalancing] + hash_header = options[:hash_header] + hash_balance = options[:hash_balance] - # When loadbalancing is explicitly set to non-hash value, hash options are not allowed - if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' - errors.add(:base, message: "Route '#{r[:route]}': Hash header can only be set when loadbalancing is hash") - end + return if validate_route_hash_header(route, hash_header) + return if validate_route_hash_balance(route, hash_balance) - if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' - errors.add(:base, message: "Route '#{r[:route]}': Hash balance can only be set when loadbalancing is hash") - end + validate_route_hash_options_with_loadbalancing(route, loadbalancing, hash_header, hash_balance) + end + + def validate_route_hash_header(route, hash_header) + return false unless hash_header.present? && (hash_header.to_s.length > 128) + + errors.add(:base, message: "Route '#{route[:route]}': Hash header must be at most 128 characters") + true + end + + def validate_route_hash_balance(route, hash_balance) + return false if hash_balance.blank? + + if hash_balance.to_s.length > 16 + errors.add(:base, message: "Route '#{route[:route]}': Hash balance must be at most 16 characters") + return true + end + + validate_route_hash_balance_numeric(route, hash_balance) + end + + def validate_route_hash_balance_numeric(route, hash_balance) + balance_float = Float(hash_balance) + # Must be either 0 or >= 1.1 and <= 10.0 + errors.add(:base, message: "Route '#{route[:route]}': Hash balance must be either 0 or between to 1.1 and 10.0") unless balance_float == 0 || balance_float.between?(1.1, 10) + false + rescue ArgumentError, TypeError + errors.add(:base, message: "Route '#{route[:route]}': Hash balance must be a numeric value") + false + end + + def validate_route_hash_options_with_loadbalancing(route, loadbalancing, hash_header, hash_balance) + # When loadbalancing is explicitly set to non-hash value, hash options are not allowed + if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' + errors.add(:base, message: "Route '#{route[:route]}': Hash header can only be set when loadbalancing is hash") end + + return unless hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' + + errors.add(:base, message: "Route '#{route[:route]}': Hash balance can only be set when loadbalancing is hash") end def routes_are_uris diff --git a/app/messages/route_options_message.rb b/app/messages/route_options_message.rb index f37ab2d939d..f79a2bb6a0c 100644 --- a/app/messages/route_options_message.rb +++ b/app/messages/route_options_message.rb @@ -47,31 +47,37 @@ def hash_options_are_valid # If disabled, route_options_are_valid will already report them as unknown fields return unless VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) - # Feature flag is enabled - validate hash-specific options + validate_hash_header_length + validate_hash_balance_value + validate_hash_options_with_loadbalancing + end + + def validate_hash_header_length + return unless hash_header.present? && (hash_header.to_s.length > 128) + + errors.add(:hash_header, 'must be at most 128 characters') + end + + def validate_hash_balance_value + return if hash_balance.blank? - # Validate hash_header length if present - if hash_header.present? && (hash_header.to_s.length > 128) - errors.add(:hash_header, 'must be at most 128 characters') + if hash_balance.to_s.length > 16 + errors.add(:hash_balance, 'must be at most 16 characters') return end - # Validate hash_balance is numeric if present - if hash_balance.present? - # Check length first (at most 16 characters) - if hash_balance.to_s.length > 16 - errors.add(:hash_balance, 'must be at most 16 characters') - return - end - - begin - balance_float = Float(hash_balance) - # Must be either 0 or >= 1.1 and <= 10.0 - errors.add(:hash_balance, 'must be either 0 or between 1.1 and 10.0') unless balance_float == 0 || balance_float.between?(1.1, 10) - rescue ArgumentError, TypeError - errors.add(:hash_balance, 'must be a numeric value') - end - end + validate_hash_balance_numeric + end + + def validate_hash_balance_numeric + balance_float = Float(hash_balance) + # Must be either 0 or >= 1.1 and <= 10.0 + errors.add(:hash_balance, 'must be either 0 or between 1.1 and 10.0') unless balance_float == 0 || balance_float.between?(1.1, 10) + rescue ArgumentError, TypeError + errors.add(:hash_balance, 'must be a numeric value') + end + def validate_hash_options_with_loadbalancing # When loadbalancing is explicitly set to non-hash value, hash options are not allowed errors.add(:base, 'Hash header can only be set when loadbalancing is hash') if hash_header.present? && loadbalancing.present? && loadbalancing != 'hash' errors.add(:base, 'Hash balance can only be set when loadbalancing is hash') if hash_balance.present? && loadbalancing.present? && loadbalancing != 'hash' From d460de20e87c7afa8b390d84bcbf9f020cf70c8c Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 14 Jan 2026 08:48:40 +0100 Subject: [PATCH 39/45] Remove test file --- test-manifest-roundrobin.yml | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 test-manifest-roundrobin.yml diff --git a/test-manifest-roundrobin.yml b/test-manifest-roundrobin.yml deleted file mode 100644 index e69de29bb2d..00000000000 From 3b9f420ce790f63a8d2d4c952f041d522e444bc3 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 14 Jan 2026 10:13:48 +0100 Subject: [PATCH 40/45] Move all route option message tests from validator spec to new spec file --- .../messages/route_options_message_spec.rb | 212 ++++++++++++++++++ spec/unit/messages/validators_spec.rb | 199 ---------------- 2 files changed, 212 insertions(+), 199 deletions(-) create mode 100644 spec/unit/messages/route_options_message_spec.rb diff --git a/spec/unit/messages/route_options_message_spec.rb b/spec/unit/messages/route_options_message_spec.rb new file mode 100644 index 00000000000..57646d21950 --- /dev/null +++ b/spec/unit/messages/route_options_message_spec.rb @@ -0,0 +1,212 @@ +require 'spec_helper' +require 'messages/route_options_message' + +module VCAP::CloudController + RSpec.describe RouteOptionsMessage do + describe 'basic validations' do + it 'successfully validates round-robin load-balancing algorithm' do + message = RouteOptionsMessage.new({ loadbalancing: 'round-robin' }) + expect(message).to be_valid + end + + it 'successfully validates least-connection load-balancing algorithm' do + message = RouteOptionsMessage.new({ loadbalancing: 'least-connection' }) + expect(message).to be_valid + end + + it 'successfully validates empty options' do + message = RouteOptionsMessage.new({}) + expect(message).to be_valid + end + + it 'successfully validates empty load balancer' do + message = RouteOptionsMessage.new({ loadbalancing: nil }) + expect(message).to be_valid + end + + it 'adds invalid load balancer error message' do + message = RouteOptionsMessage.new({ loadbalancing: 'donuts' }) + expect(message).not_to be_valid + expect(message.errors_on(:loadbalancing)).to include("must be one of 'round-robin, least-connection' if present") + end + + it 'adds invalid field error message' do + message = RouteOptionsMessage.new({ cookies: 'round-robin' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'cookies'") + end + end + + describe 'hash-based routing validations' do + context 'when hash_based_routing feature flag is disabled' do + it 'does not allow hash_header option' do + message = RouteOptionsMessage.new({ hash_header: 'X-User-ID' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'hash_header'") + end + + it 'does not allow hash_balance option' do + message = RouteOptionsMessage.new({ hash_balance: '1.5' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'hash_balance'") + end + + it 'reports multiple invalid keys together' do + message = RouteOptionsMessage.new({ hash_header: 'X-User-ID', hash_balance: '1.5' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include("Unknown field(s): 'hash_header', 'hash_balance'") + end + + it 'does not allow hash load-balancing algorithm' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash' }) + expect(message).not_to be_valid + expect(message.errors_on(:loadbalancing)).to include("must be one of 'round-robin, least-connection' if present") + end + end + + context 'when hash_based_routing feature flag is enabled' do + before do + VCAP::CloudController::FeatureFlag.make(name: 'hash_based_routing', enabled: true) + end + + describe 'loadbalancing algorithm' do + it 'allows hash loadbalancing option' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X-User-ID' }) + expect(message).to be_valid + end + + it 'allows round-robin loadbalancing' do + message = RouteOptionsMessage.new({ loadbalancing: 'round-robin' }) + expect(message).to be_valid + end + + it 'allows least-connection loadbalancing' do + message = RouteOptionsMessage.new({ loadbalancing: 'least-connection' }) + expect(message).to be_valid + end + end + + describe 'hash_header validation' do + it 'allows hash_header option' do + message = RouteOptionsMessage.new({ hash_header: 'X-User-ID' }) + expect(message).to be_valid + end + + it 'does not allow hash_header without hash load-balancing' do + message = RouteOptionsMessage.new({ loadbalancing: 'round-robin', hash_header: 'X-User-ID' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include('Hash header can only be set when loadbalancing is hash') + end + + context 'hash_header length validation' do + it 'does not accept hash_header longer than 128 characters' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X' * 129 }) + expect(message).not_to be_valid + expect(message.errors_on(:hash_header)).to include('must be at most 128 characters') + end + + it 'accepts hash_header exactly 128 characters' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X' * 128 }) + expect(message).to be_valid + end + end + end + + describe 'hash_balance validation' do + it 'allows hash_balance option' do + message = RouteOptionsMessage.new({ hash_balance: '1.5' }) + expect(message).to be_valid + end + + it 'does not allow hash_balance without hash load-balancing' do + message = RouteOptionsMessage.new({ loadbalancing: 'round-robin', hash_balance: '1.5' }) + expect(message).not_to be_valid + expect(message.errors_on(:base)).to include('Hash balance can only be set when loadbalancing is hash') + end + + context 'numeric validation' do + it 'does not allow non-numeric hash_balance' do + message = RouteOptionsMessage.new({ hash_balance: 'not-a-number' }) + expect(message).not_to be_valid + expect(message.errors.full_messages).to include('Hash balance must be a numeric value') + end + + it 'allows hash_balance of 0' do + message = RouteOptionsMessage.new({ hash_balance: 0 }) + expect(message).to be_valid + end + + it 'allows hash_balance of 1.1' do + message = RouteOptionsMessage.new({ hash_balance: 1.1 }) + expect(message).to be_valid + end + + it 'allows hash_balance greater than 1.1' do + message = RouteOptionsMessage.new({ hash_balance: 2.5 }) + expect(message).to be_valid + end + + it 'does not allow hash_balance between 0 and 1.1' do + message = RouteOptionsMessage.new({ hash_balance: 0.5 }) + expect(message).not_to be_valid + expect(message.errors.full_messages).to include('Hash balance must be either 0 or between 1.1 and 10.0') + end + + it 'allows numeric string hash_balance' do + message = RouteOptionsMessage.new({ hash_balance: '2.5' }) + expect(message).to be_valid + end + + it 'allows integer string hash_balance' do + message = RouteOptionsMessage.new({ hash_balance: '3' }) + expect(message).to be_valid + end + + it 'allows float hash_balance' do + message = RouteOptionsMessage.new({ hash_balance: 1.5 }) + expect(message).to be_valid + end + end + + context 'length validation' do + it 'does not accept hash_balance longer than 16 characters' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '12345678901234567' }) + expect(message).not_to be_valid + expect(message.errors_on(:hash_balance)).to include('must be at most 16 characters') + end + + it 'accepts hash_balance exactly 16 characters' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '9.9' }) + expect(message).to be_valid + end + end + + context 'range validation' do + it 'does not accept hash_balance greater than 10.0' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 10.1 }) + expect(message).not_to be_valid + expect(message.errors_on(:hash_balance)).to include('must be either 0 or between 1.1 and 10.0') + end + + it 'accepts hash_balance exactly 10.0' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: 10.0 }) + expect(message).to be_valid + end + end + end + + describe 'combined hash options' do + it 'allows hash loadbalancing with hash_header and hash_balance' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X-User-ID', hash_balance: '2.5' }) + expect(message).to be_valid + end + + it 'allows hash loadbalancing with only hash_header' do + message = RouteOptionsMessage.new({ loadbalancing: 'hash', hash_header: 'X-User-ID' }) + expect(message).to be_valid + end + end + end + end + end +end diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 250175ace57..03e31652f02 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -3,7 +3,6 @@ require 'messages/base_message' require 'messages/empty_lifecycle_data_message' require 'messages/buildpack_lifecycle_data_message' -require 'messages/route_options_message' require 'cloud_controller/diego/lifecycles/app_docker_lifecycle' require 'cloud_controller/diego/lifecycles/app_buildpack_lifecycle' require 'cloud_controller/diego/lifecycles/lifecycles' @@ -530,204 +529,6 @@ class Relationships < VCAP::CloudController::BaseMessage end end - describe 'OptionsValidator' do - # Stub the FeatureFlag class for lightweight tests - before do - unless defined?(VCAP::CloudController::FeatureFlag) - feature_flag_class = Class.new do - def self.enabled?(flag_name, **) - # Default: hash_based_routing is disabled - return false if flag_name == :hash_based_routing - - false - end - end - - stub_const('VCAP::CloudController::FeatureFlag', feature_flag_class) - end - end - - 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: { loadbalancing: 'round-robin' } }) - expect(message).to be_valid - end - - it 'successfully validates least-connection load-balancing algorithm' do - message = OptionsMessage.new({ options: { loadbalancing: 'least-connection' } }) - 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 message for hash load-balancing algorithm' do - message = OptionsMessage.new({ options: { loadbalancing: 'hash' } }) - expect(message).not_to be_valid - end - - it 'adds invalid message for hash_header option' do - message = OptionsMessage.new({ options: { hash_header: 'X-Potatoes' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_header'") - end - - it 'adds invalid message for hash_balance option' do - message = OptionsMessage.new({ options: { hash_balance: '1.2' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_balance'") - 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: { loadbalancing: 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 - 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: { loadbalancing: 'donuts' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Loadbalancing must be one of 'round-robin, least-connection' if present") - 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 - - context 'when hash_based_routing feature flag is disabled' do - it 'does not allow hash_header option' do - message = OptionsMessage.new({ options: { hash_header: 'X-User-ID' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_header'") - end - - it 'does not allow hash_balance option' do - message = OptionsMessage.new({ options: { hash_balance: '1.5' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_balance'") - end - - it 'reports multiple invalid keys together' do - message = OptionsMessage.new({ options: { hash_header: 'X-User-ID', hash_balance: '1.5' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Unknown field(s): 'hash_header', 'hash_balance'") - end - - it 'does not allow hash load-balancing algorithm' do - message = OptionsMessage.new({ options: { loadbalancing: 'hash' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include("Loadbalancing must be one of 'round-robin, least-connection' if present") - end - end - - context 'when hash_based_routing feature flag is enabled' do - before do - feature_flag_class = Class.new do - def self.enabled?(flag_name, **) - return true if flag_name == :hash_based_routing - - false - end - end - - stub_const('VCAP::CloudController::FeatureFlag', feature_flag_class) - end - - it 'allows hash loadbalancing option' do - message = OptionsMessage.new({ options: { loadbalancing: 'hash' } }) - expect(message).to be_valid - end - - it 'allows hash_balance option' do - message = OptionsMessage.new({ options: { hash_balance: '1.5' } }) - expect(message).to be_valid - end - - it 'allows hash_header option' do - message = OptionsMessage.new({ options: { hash_header: 'X-User-ID' } }) - expect(message).to be_valid - end - - it 'does not allow hash_header without hash load-balancing' do - message = OptionsMessage.new({ options: { loadbalancing: 'round-robin', hash_header: 'X-User-ID' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include('Hash header can only be set when loadbalancing is hash') - end - - it 'does not allow hash_balance without hash load-balancing' do - message = OptionsMessage.new({ options: { loadbalancing: 'round-robin', hash_balance: '1.5' } }) - expect(message).not_to be_valid - expect(message.errors_on(:options)).to include('Hash balance can only be set when loadbalancing is hash') - end - - it 'does not allow non-numeric hash_balance' do - message = OptionsMessage.new({ options: { hash_balance: 'not-a-number' } }) - expect(message).not_to be_valid - expect(message.errors.full_messages).to include('Options Hash balance must be a numeric value') - end - - it 'allows hash_balance of 0' do - message = OptionsMessage.new({ options: { hash_balance: 0 } }) - expect(message).to be_valid - end - - it 'allows hash_balance of 1.1' do - message = OptionsMessage.new({ options: { hash_balance: 1.1 } }) - expect(message).to be_valid - end - - it 'allows hash_balance greater than 1.1' do - message = OptionsMessage.new({ options: { hash_balance: 2.5 } }) - expect(message).to be_valid - end - - it 'does not allow hash_balance between 0 and 1.1' do - message = OptionsMessage.new({ options: { hash_balance: 0.5 } }) - expect(message).not_to be_valid - expect(message.errors.full_messages).to include('Options Hash balance must be either 0 or between 1.1 and 10.0') - end - - it 'allows numeric string hash_balance' do - message = OptionsMessage.new({ options: { hash_balance: '2.5' } }) - expect(message).to be_valid - end - - it 'allows integer string hash_balance' do - message = OptionsMessage.new({ options: { hash_balance: '3' } }) - expect(message).to be_valid - end - - it 'allows float hash_balance' do - message = OptionsMessage.new({ options: { hash_balance: 1.5 } }) - expect(message).to be_valid - end - end - end - describe 'ToOneRelationshipValidator' do let(:to_one_class) do Class.new(fake_class) do From 97d408442111e3d768a53732cc79d5ab0e8af5d5 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 14 Jan 2026 10:14:13 +0100 Subject: [PATCH 41/45] Introduce stub config in validator spec to fix execution of standalone spec test --- spec/unit/messages/validators_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index 03e31652f02..a7e24a23239 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -1,4 +1,20 @@ require 'lightweight_spec_helper' + +# Stub Config for lightweight spec (must be before validators.rb is required) +module VCAP + module CloudController + class Config + def self.config + self + end + + def self.get(key) + 'buildpack' if key == :default_app_lifecycle + end + end + end +end + require 'messages/validators' require 'messages/base_message' require 'messages/empty_lifecycle_data_message' From e1a1ab53dcce6c00a4db1770c83bbe4ec23a7d17 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 14 Jan 2026 10:26:55 +0100 Subject: [PATCH 42/45] Fix typos and validation logic --- .../manifest_routes_update_message.rb | 15 +++++------ .../manifest_routes_update_message_spec.rb | 27 +++++++++++++++++-- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/app/messages/manifest_routes_update_message.rb b/app/messages/manifest_routes_update_message.rb index 8cdcac7e2f8..f00003ac550 100644 --- a/app/messages/manifest_routes_update_message.rb +++ b/app/messages/manifest_routes_update_message.rb @@ -110,25 +110,24 @@ def validate_route_hash_options(route) hash_header = options[:hash_header] hash_balance = options[:hash_balance] - return if validate_route_hash_header(route, hash_header) - return if validate_route_hash_balance(route, hash_balance) + validate_route_hash_header(route, hash_header) + validate_route_hash_balance(route, hash_balance) validate_route_hash_options_with_loadbalancing(route, loadbalancing, hash_header, hash_balance) end def validate_route_hash_header(route, hash_header) - return false unless hash_header.present? && (hash_header.to_s.length > 128) + return unless hash_header.present? && (hash_header.to_s.length > 128) errors.add(:base, message: "Route '#{route[:route]}': Hash header must be at most 128 characters") - true end def validate_route_hash_balance(route, hash_balance) - return false if hash_balance.blank? + return if hash_balance.blank? if hash_balance.to_s.length > 16 errors.add(:base, message: "Route '#{route[:route]}': Hash balance must be at most 16 characters") - return true + return end validate_route_hash_balance_numeric(route, hash_balance) @@ -137,11 +136,9 @@ def validate_route_hash_balance(route, hash_balance) def validate_route_hash_balance_numeric(route, hash_balance) balance_float = Float(hash_balance) # Must be either 0 or >= 1.1 and <= 10.0 - errors.add(:base, message: "Route '#{route[:route]}': Hash balance must be either 0 or between to 1.1 and 10.0") unless balance_float == 0 || balance_float.between?(1.1, 10) - false + errors.add(:base, message: "Route '#{route[:route]}': Hash balance must be either 0 or between 1.1 and 10.0") unless balance_float == 0 || balance_float.between?(1.1, 10) rescue ArgumentError, TypeError errors.add(:base, message: "Route '#{route[:route]}': Hash balance must be a numeric value") - false end def validate_route_hash_options_with_loadbalancing(route, loadbalancing, hash_header, hash_balance) diff --git a/spec/unit/messages/manifest_routes_update_message_spec.rb b/spec/unit/messages/manifest_routes_update_message_spec.rb index 981d339c715..9e48eb55ba0 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -644,7 +644,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': Hash balance must be either 0 or between to 1.1 and 10.0") + expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash balance must be either 0 or between 1.1 and 10.0") end end @@ -685,7 +685,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': Hash balance must be either 0 or between to 1.1 and 10.0") + expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash balance must be either 0 or between 1.1 and 10.0") end end @@ -790,6 +790,29 @@ module VCAP::CloudController end end + context 'when a route contains multiple issues with hash options' do + let(:body) do + { 'routes' => + [ + { 'route' => 'existing.example.com', + 'options' => { + 'loadbalancing' => 'hash', + 'hash_header' => 'X' * 129, + 'hash_balance' => '1' * 17 + } } + ] } + end + + it 'returns false and prints all errors' do + msg = ManifestRoutesUpdateMessage.new(body) + + expect(msg.valid?).to be(false) + expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash header must be at most 128 characters") + expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash balance must be at most 16 characters") + + end + end + context 'when multiple routes have mixed valid and invalid hash options' do let(:body) do { 'routes' => From a1b01b23a124a2561b96b60a557b72f33842d787 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 14 Jan 2026 10:29:19 +0100 Subject: [PATCH 43/45] Fix validation function name inconsistencies --- app/actions/route_create.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index 4191a0c1cd1..23ca937db89 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -68,9 +68,9 @@ def route_resource_manager end def validation_error!(error, host, path, port, space, domain) - check_domain_errors!(error, space, domain) - check_quota_errors!(error, space) - check_route_errors!(error) + validation_error_domain!(error, space, domain) + validation_error_quota!(error, space) + validation_error_route!(error) validation_error_routing_api!(error) validation_error_host!(error, host, domain) validation_error_path!(error, host, path, domain) @@ -79,20 +79,20 @@ def validation_error!(error, host, path, port, space, domain) error!(error.message) end - def check_domain_errors!(error, space, domain) + def validation_error_domain!(error, space, domain) return unless error.errors.on(:domain)&.include?(:invalid_relation) error!("Invalid domain. Domain '#{domain.name}' is not available in organization '#{space.organization.name}'.") end - def check_quota_errors!(error, space) + def validation_error_quota!(error, space) error!("Routes quota exceeded for space '#{space.name}'.") if error.errors.on(:space)&.include?(:total_routes_exceeded) error!("Reserved route ports quota exceeded for space '#{space.name}'.") if error.errors.on(:space)&.include?(:total_reserved_route_ports_exceeded) error!("Routes quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_routes_exceeded) error!("Reserved route ports quota exceeded for organization '#{space.organization.name}'.") if error.errors.on(:organization)&.include?(:total_reserved_route_ports_exceeded) end - def check_route_errors!(error) + def validation_error_route!(error) return unless error.errors.on(:route)&.include?(:hash_header_missing) error!('Hash header must be present when loadbalancing is set to hash') From e03fe06aa645890d2efd1009dc7dd9bd24727800 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 14 Jan 2026 10:43:58 +0100 Subject: [PATCH 44/45] Fix remaining issues found in review --- app/actions/route_create.rb | 2 +- app/actions/route_update.rb | 2 +- spec/unit/actions/manifest_route_update_spec.rb | 2 +- spec/unit/actions/route_create_spec.rb | 2 +- spec/unit/actions/route_update_spec.rb | 4 ++-- spec/unit/messages/manifest_routes_update_message_spec.rb | 7 +++---- 6 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/actions/route_create.rb b/app/actions/route_create.rb index 23ca937db89..5a494b798e1 100644 --- a/app/actions/route_create.rb +++ b/app/actions/route_create.rb @@ -95,7 +95,7 @@ def validation_error_quota!(error, space) def validation_error_route!(error) return unless error.errors.on(:route)&.include?(:hash_header_missing) - error!('Hash header must be present when loadbalancing is set to hash') + error!('Hash header must be present when loadbalancing is set to hash.') end def validation_error_routing_api!(error) diff --git a/app/actions/route_update.rb b/app/actions/route_update.rb index 30f8b2a64da..df3a325f378 100644 --- a/app/actions/route_update.rb +++ b/app/actions/route_update.rb @@ -24,7 +24,7 @@ def update(route:, message:) def validation_error!(error) # Handle hash_header validation error for hash loadbalancing - raise Error.new('Hash header must be present when loadbalancing is set to hash') if error.errors.on(:route)&.include?(:hash_header_missing) + raise Error.new('Hash header must be present when loadbalancing is set to hash.') if error.errors.on(:route)&.include?(:hash_header_missing) # Fallback for any other validation errors raise Error.new(error.message) diff --git a/spec/unit/actions/manifest_route_update_spec.rb b/spec/unit/actions/manifest_route_update_spec.rb index 1b4794198df..35d6e8a5e19 100644 --- a/spec/unit/actions/manifest_route_update_spec.rb +++ b/spec/unit/actions/manifest_route_update_spec.rb @@ -492,7 +492,7 @@ module VCAP::CloudController it 'raises an error indicating hash_header is required' do expect do ManifestRouteUpdate.update(app.guid, message, user_audit_info) - end.to raise_error(ManifestRouteUpdate::InvalidRoute, /Hash header must be present when loadbalancing is set to hash/) + end.to raise_error(ManifestRouteUpdate::InvalidRoute, /Hash header must be present when loadbalancing is set to hash./) end end diff --git a/spec/unit/actions/route_create_spec.rb b/spec/unit/actions/route_create_spec.rb index c5b4c6a0035..86fdca59888 100644 --- a/spec/unit/actions/route_create_spec.rb +++ b/spec/unit/actions/route_create_spec.rb @@ -187,7 +187,7 @@ module VCAP::CloudController it 'raises an error indicating hash_header is required' do expect do subject.create(message: message_without_hash_header, space: space, domain: domain) - end.to raise_error(RouteCreate::Error, 'Hash header must be present when loadbalancing is set to hash') + end.to raise_error(RouteCreate::Error, 'Hash header must be present when loadbalancing is set to hash.') end end end diff --git a/spec/unit/actions/route_update_spec.rb b/spec/unit/actions/route_update_spec.rb index 219bc4fc426..be966d96fe2 100644 --- a/spec/unit/actions/route_update_spec.rb +++ b/spec/unit/actions/route_update_spec.rb @@ -369,7 +369,7 @@ module VCAP::CloudController expect(message).to be_valid expect do subject.update(route:, message:) - end.to raise_error(RouteUpdate::Error, 'Hash header must be present when loadbalancing is set to hash') + end.to raise_error(RouteUpdate::Error, 'Hash header must be present when loadbalancing is set to hash.') end end end @@ -415,7 +415,7 @@ module VCAP::CloudController expect(message).to be_valid expect do subject.update(route:, message:) - end.to raise_error(RouteUpdate::Error, 'Hash header must be present when loadbalancing is set to hash') + end.to raise_error(RouteUpdate::Error, 'Hash header must be present when loadbalancing is set to hash.') 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 9e48eb55ba0..11d719fd686 100644 --- a/spec/unit/messages/manifest_routes_update_message_spec.rb +++ b/spec/unit/messages/manifest_routes_update_message_spec.rb @@ -757,7 +757,7 @@ module VCAP::CloudController 'options' => { 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID', - 'hash_balance' => '12345678901234567' + 'hash_balance' => '2.' + ('1' * 15) } } ] } end @@ -778,7 +778,7 @@ module VCAP::CloudController 'options' => { 'loadbalancing' => 'hash', 'hash_header' => 'X-User-ID', - 'hash_balance' => '9.9' + 'hash_balance' => '2.' + ('1' * 14) } } ] } end @@ -798,7 +798,7 @@ module VCAP::CloudController 'options' => { 'loadbalancing' => 'hash', 'hash_header' => 'X' * 129, - 'hash_balance' => '1' * 17 + 'hash_balance' => '2.' + ('1' * 15) } } ] } end @@ -809,7 +809,6 @@ module VCAP::CloudController expect(msg.valid?).to be(false) expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash header must be at most 128 characters") expect(msg.errors.full_messages).to include("Route 'existing.example.com': Hash balance must be at most 16 characters") - end end From d25820a040f499783d73dfdc2a72d3054df4cab3 Mon Sep 17 00:00:00 2001 From: Clemens Hoffmann Date: Wed, 14 Jan 2026 11:38:59 +0100 Subject: [PATCH 45/45] revert change to validators spec --- spec/unit/messages/validators_spec.rb | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/spec/unit/messages/validators_spec.rb b/spec/unit/messages/validators_spec.rb index a7e24a23239..03e31652f02 100644 --- a/spec/unit/messages/validators_spec.rb +++ b/spec/unit/messages/validators_spec.rb @@ -1,20 +1,4 @@ require 'lightweight_spec_helper' - -# Stub Config for lightweight spec (must be before validators.rb is required) -module VCAP - module CloudController - class Config - def self.config - self - end - - def self.get(key) - 'buildpack' if key == :default_app_lifecycle - end - end - end -end - require 'messages/validators' require 'messages/base_message' require 'messages/empty_lifecycle_data_message'