-
Notifications
You must be signed in to change notification settings - Fork 366
Support Hash-Based Routing #4696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the |
||
|
|
||
| # 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,7 +99,19 @@ def create | |
| end | ||
|
|
||
| def update | ||
| message = RouteUpdateMessage.new(hashed_params[:body]) | ||
| params = hashed_params[:body] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| # 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) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not |
||
| end | ||
|
|
||
| def routes_are_uris | ||
| return if errors[:routes].present? | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to define |
||
|
|
||
| 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? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use |
||
|
|
||
| # 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer |
||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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= | ||
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: |
||
| end | ||
|
|
||
| def before_destroy | ||
| destroy_route_bindings | ||
| super | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,10 +11,13 @@ def to_hash(route_mappings:, app:, **_) | |
| } | ||
|
|
||
| if route_mapping.route.options | ||
| opts = route_mapping.route.options | ||
|
|
||
| route_hash[:options] = {} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not |
||
| 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 | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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.updateto have it in a single place.There was a problem hiding this comment.
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_balancewould be invalid for a new route, but is valid for a route that is already properly setup with hash-based routing.RouteUpdate.updateis the last step in this chain, so we would also need to migrate the validation logic toRouteUpdate. I think theOptionsValidatoris the better place to have the validation, and thus also the merging. Would you agree?