Support Hash-Based Routing #4746
Conversation
…ute_options_message
philippthun
left a comment
philippthun
left a comment
There was a problem hiding this comment.
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_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.
Maybe add a return to only show one error at a time?
There was a problem hiding this comment.
We earlier discussed that it's probably a better user experience to print all issues, instead of always just showing the first one that occurs. Does your comment refer to this particular validation, or would you now prefer to always only show the first error?
| 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.
The freeze is not really necessary (same below for algorithms).
There was a problem hiding this comment.
Right, but it was frozen in the previous version, when it was a static array. Not a ruby expert here, but we don't expect any modifications to the arrays, and it would be correct to disallow future modifications. Is there another reason not to freeze it?
2 participants
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