Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions app/actions/manifest_route_update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,15 @@ 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] && route[:options] != manifest_route[:options]
# remove nil values from options
manifest_route[:options] = manifest_route[:options].compact
# Merge existing route options with manifest options for partial updates
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the merge to RouteUpdate.update to have it in a single place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On route updates, we need to validate after merging, as we need to allow incremental route option updates. E.g., only setting hash_balance would be invalid for a new route, but is valid for a route that is already properly setup with hash-based routing. RouteUpdate.update is the last step in this chain, so we would also need to migrate the validation logic to RouteUpdate. I think the OptionsValidator is the better place to have the validation, and thus also the merging. Would you agree?

merged_options = if route.options
route.options.deep_symbolize_keys.merge(manifest_route[:options]).compact
else
manifest_route[:options].compact
end

message = RouteUpdateMessage.new({
'options' => manifest_route[:options]
'options' => merged_options
})
route = RouteUpdate.new.update(route:, message:)
end
Expand Down
13 changes: 12 additions & 1 deletion app/actions/route_update.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,18 @@ 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)
merged_options = message.options.compact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the merge missing, i.e. route.options.merge(message.options)?


# Clean up invalid option combinations
# If loadbalancing is not 'hash', remove hash-specific options
if merged_options[:loadbalancing] && merged_options[:loadbalancing] != 'hash'
merged_options.delete(:hash_header)
merged_options.delete(:hash_balance)
end

route.options = merged_options
end
route.save
MetadataUpdate.update(route, message)
end
Expand Down
14 changes: 13 additions & 1 deletion app/controllers/v3/routes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,19 @@ def create
end

def update
message = RouteUpdateMessage.new(hashed_params[:body])
params = hashed_params[:body]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The controller should not manipulate the params and the message should reflect what has been provided by the user. Isn't this redundant with RouteUpdate.update anyway?


# Merge existing route options with incoming options for partial updates
if params[:options] && route.options
existing_options = route.options.deep_symbolize_keys
params[:options] = existing_options.merge(params[:options].deep_symbolize_keys)
if params[:options][:loadbalancing] && params[:options][:loadbalancing] != 'hash'
params[:options].delete(:hash_header)
params[:options].delete(:hash_balance)
end
end

message = RouteUpdateMessage.new(params)
unprocessable!(message.errors.full_messages) unless message.valid?

unauthorized! unless permission_queryer.can_manage_apps_in_active_space?(route.space_id)
Expand Down
45 changes: 42 additions & 3 deletions app/messages/manifest_routes_update_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,23 +69,62 @@ def route_options_are_valid
def loadbalancings_are_valid
return if errors[:routes].present?

valid_algorithms = RouteOptionsMessage.valid_loadbalancing_algorithms

routes.each do |r|
next unless r.keys.include?(:options) && r[:options].is_a?(Hash) && r[:options].keys.include?(:loadbalancing)

loadbalancing = r[:options][:loadbalancing]
unless loadbalancing.is_a?(String)
errors.add(:base,
message: "Invalid value for 'loadbalancing' for Route '#{r[:route]}'; \
Valid values are: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'")
Valid values are: '#{valid_algorithms.join(', ')}'")
next
end
RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.exclude?(loadbalancing) &&

if valid_algorithms.exclude?(loadbalancing)
errors.add(:base,
message: "Cannot use loadbalancing value '#{loadbalancing}' for Route '#{r[:route]}'; \
Valid values are: '#{RouteOptionsMessage::VALID_LOADBALANCING_ALGORITHMS.join(', ')}'")
Valid values are: '#{valid_algorithms.join(', ')}'")
next
end

# Validate hash-specific options
if loadbalancing == 'hash'
validate_hash_options_for_route(r)
else
validate_no_hash_options_for_route(r)
end
end
end

def validate_hash_options_for_route(route)
options = route[:options]

# hash_header is required for hash algorithm
errors.add(:base, message: "Route '#{route[:route]}': hash_header must be present when loadbalancing is set to hash") if options[:hash_header].blank?

# hash_balance must be valid if present
return if options[:hash_balance].blank?

begin
hash_balance = Float(options[:hash_balance])
errors.add(:base, message: "Route '#{route[:route]}': hash_balance must be greater than or equal to 0.0") if hash_balance < 0.0
rescue ArgumentError, TypeError
errors.add(:base, message: "Route '#{route[:route]}': hash_balance must be a valid number")
end
end

def validate_no_hash_options_for_route(route)
options = route[:options]

errors.add(:base, message: "Route '#{route[:route]}': hash_header can only be set when loadbalancing is hash") if options[:hash_header].present?

return if options[:hash_balance].blank?

errors.add(:base, message: "Route '#{route[:route]}': hash_balance can only be set when loadbalancing is hash")
Comment on lines +123 to +125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not errors.add(...) if options[:hash_balance].present? as above?

end

def routes_are_uris
return if errors[:routes].present?

Expand Down
69 changes: 62 additions & 7 deletions app/messages/route_options_message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,70 @@

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
VALID_MANIFEST_ROUTE_OPTIONS = %i[loadbalancing hash_header hash_balance].freeze
VALID_ROUTE_OPTIONS = %i[loadbalancing hash_header hash_balance].freeze
VALID_LOADBALANCING_ALGORITHMS_WITH_HASH = %w[round-robin least-connection hash].freeze
VALID_LOADBALANCING_ALGORITHMS_WITHOUT_HASH = %w[round-robin least-connection].freeze
Comment on lines +7 to +8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to define VALID_LOADBALANCING_ALGORITHMS_WITH_HASH as VALID_LOADBALANCING_ALGORITHMS_WITHOUT_HASH + 'hash'?


register_allowed_keys VALID_ROUTE_OPTIONS
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 :validate_loadbalancing_with_feature_flag

validate :validate_hash_options, if: -> { errors[:loadbalancing].empty? }

def self.valid_loadbalancing_algorithms
if FeatureFlag.enabled?(:hash_based_routing)
VALID_LOADBALANCING_ALGORITHMS_WITH_HASH
else
VALID_LOADBALANCING_ALGORITHMS_WITHOUT_HASH
end
end

private

def validate_loadbalancing_with_feature_flag
return if loadbalancing.nil?

valid_algorithms = self.class.valid_loadbalancing_algorithms
return if valid_algorithms.include?(loadbalancing)

errors.add(:loadbalancing, "must be one of '#{valid_algorithms.join(', ')}' if present")
end

def validate_hash_options
if loadbalancing == 'hash'
validate_hash_header_present
validate_hash_balance_format
else
validate_hash_options_not_present_for_non_hash
end
end

def validate_hash_header_present
if hash_header.blank?
errors.add(:hash_header, 'must be present when loadbalancing is set to hash')
elsif !hash_header.is_a?(String)
errors.add(:hash_header, 'must be a string')
end
end

def validate_hash_balance_format
return if hash_balance.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use blank? instead of nil? as in other places?


# Convert string to float if needed (from CLI input)
begin
hash_balance_float = Float(hash_balance)
errors.add(:hash_balance, 'must be greater than or equal to 0.0') if hash_balance_float < 0.0
rescue ArgumentError, TypeError
errors.add(:hash_balance, 'must be a valid number')
end
end

def validate_hash_options_not_present_for_non_hash
errors.add(:hash_header, 'can only be set when loadbalancing is hash') if hash_header.present?
return if hash_balance.blank?

errors.add(:hash_balance, 'can only be set when loadbalancing is hash')
Comment on lines +66 to +68
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer errors.add(...) if hash_balance.present?.

end
end
end
3 changes: 2 additions & 1 deletion app/models/runtime/feature_flag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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[
Expand Down
19 changes: 18 additions & 1 deletion app/models/runtime/route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down Expand Up @@ -216,6 +217,22 @@ def wildcard_host?

private

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.dup
hash_balance_key = opts.key?('hash_balance') ? 'hash_balance' : :hash_balance
hash_balance_value = opts[hash_balance_key]

if hash_balance_value.present?
# Always convert to string for consistent storage in JSON
normalized[hash_balance_key] = hash_balance_value.to_s
end

normalized
Comment on lines +228 to +233
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

return opts if hash_balance_value.blank?

normalized = opts.dup
normalized[hash_balance_key] = hash_balance_value.to_s

normalized

end

def before_destroy
destroy_route_bindings
super
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,13 @@ def to_hash(route_mappings:, app:, **_)
}

if route_mapping.route.options
opts = route_mapping.route.options

route_hash[:options] = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not route_hash[:options] = route_mapping.route.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

Expand Down
2 changes: 2 additions & 0 deletions app/repositories/app_event_repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
29 changes: 28 additions & 1 deletion docs/v3/source/includes/resources/routes/_create.md.erb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ curl "https://api.example.org/v3/routes" \
},
"options": {
"loadbalancing": "round-robin"
}
},
"metadata": {
"labels": { "key": "value" },
"annotations": { "note": "detailed information"}
Expand Down Expand Up @@ -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.

Original file line number Diff line number Diff line change
Expand Up @@ -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'. |
2 changes: 2 additions & 0 deletions lib/cloud_controller/app_manifest/manifest_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion spec/api/documentation/feature_flags_api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Loading