-
Notifications
You must be signed in to change notification settings - Fork 367
Support Hash-Based Routing #4746
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?
Conversation
…ute_options_message
philippthun
left a comment
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.
Most comments are minor things...
There is a file test-manifest-roundrobin.yml which seems to be some leftover.
I don't think we should modify spec/unit/messages/validators_spec.rb. I see that this was already done for the other route options, but from my point of view this file is more for generic validators and not for specific ones. The tests should be moved into a file route_options_message_spec.rb.
| 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) |
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.
| 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) | |
| 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) |
| false | ||
| rescue ArgumentError, TypeError | ||
| errors.add(:base, message: "Route '#{route[:route]}': Hash balance must be a numeric value") | ||
| false |
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.
Shouldn't this be true (and also in case the error above is added) - my understanding is that true means that the validation found something is further checks are omitted.
| 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) |
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.
To be consistent with the other methods, this should be validation_error_domain!.
| error!("Invalid domain. Domain '#{domain.name}' is not available in organization '#{space.organization.name}'.") | ||
| end | ||
|
|
||
| def check_quota_errors!(error, space) |
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.
To be consistent with the other methods, this should be validation_error_quota!.
| 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) |
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.
To be consistent with the other methods, this should be validation_error_route! or validation_error_route_options!.
| 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') |
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.
All other error messages have a . at the end.
|
|
||
| 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) |
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.
Also a . at the end of the message.
| 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") |
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.
Maybe add a return to only show one error at a time?
| def self.valid_route_options | ||
| options = %i[loadbalancing] | ||
| options += %i[hash_header hash_balance] if VCAP::CloudController::FeatureFlag.enabled?(:hash_based_routing) | ||
| options.freeze |
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.
The freeze is not really necessary (same below for algorithms).
| end | ||
| end | ||
|
|
||
| context 'when a route contains hash_balance exactly 16 characters' do |
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.
The description does not seem to match the tested conditions...
This Pull-Request replaces #4696.
This Pull-Request adds support for hash-based routing to Cloud Controller.
Summary:
The routes model is enhanced as follows:
hashis added as a validloadbalancingoptionhash_headeris added as a per-route option. The option is mandatory whenloadbalancing=hashhash_balanceis added as a per-route option. The option is optional whenloadbalancing=hashValidation of these options is added when creating and updating both via API and via manifest. All validations that do not need the context of a potentially existing route during an update, are executed in the message:
Further validations and cleanups are performed on the route model, as they need the context of existing routes:
hash, then Hash Header must be specifiedThe route options are still stored as a raw JSON string in the routes table.
Links:
Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:
A short explanation of the proposed change:
An explanation of the use cases your change solves
Links to any other associated PRs
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests