From c9069d587228603db696496f88b6e686d380f022 Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Mon, 27 Jan 2020 08:44:26 -0800 Subject: [PATCH] Fix long CLI error messages The python-octaviaclient plugin was using "choices" for validating port numbers and weights. The cliff "choices" parameter will output an error that includes every valid "choice". For port numbers, this is 65,000+ valid options. This patch changes this to produce a more user friendly error message. This patch also fixes the listener protocol-port type validation. Change-Id: I4cb32bcc4681bd99f178d31547af37a7c582d210 Story: 2007218 Task: 38470 (cherry picked from commit cc3e82d5de7dea0dc4edce53074ab72db773a3e0) --- octaviaclient/osc/v2/constants.py | 8 ++ octaviaclient/osc/v2/listener.py | 5 ++ octaviaclient/osc/v2/member.py | 12 +-- octaviaclient/osc/v2/validate.py | 38 +++++++++ .../tests/unit/osc/v2/test_listener.py | 6 +- .../tests/unit/osc/v2/test_validations.py | 78 +++++++++++++++++++ 6 files changed, 139 insertions(+), 8 deletions(-) diff --git a/octaviaclient/osc/v2/constants.py b/octaviaclient/osc/v2/constants.py index 1ad8696..6f4925b 100644 --- a/octaviaclient/osc/v2/constants.py +++ b/octaviaclient/osc/v2/constants.py @@ -312,3 +312,11 @@ FLAVORPROFILE_COLUMNS = ( 'name', 'provider_name', ) + +# TCP/UDP port min/max +MIN_PORT_NUMBER = 1 +MAX_PORT_NUMBER = 65535 + +# Member weight min/max +MIN_WEIGHT = 0 +MAX_WEIGHT = 256 diff --git a/octaviaclient/osc/v2/listener.py b/octaviaclient/osc/v2/listener.py index 4eac9ff..f8e407c 100644 --- a/octaviaclient/osc/v2/listener.py +++ b/octaviaclient/osc/v2/listener.py @@ -21,6 +21,7 @@ from osc_lib import utils from octaviaclient.osc.v2 import constants as const from octaviaclient.osc.v2 import utils as v2_utils +from octaviaclient.osc.v2 import validate PROTOCOL_CHOICES = ['TCP', 'HTTP', 'HTTPS', 'TERMINATED_HTTPS', 'UDP'] CLIENT_AUTH_CHOICES = ['NONE', 'OPTIONAL', 'MANDATORY'] @@ -93,6 +94,7 @@ class CreateListener(command.ShowOne): '--protocol-port', metavar='', required=True, + type=int, help="Set the protocol port number for the listener." ) parser.add_argument( @@ -163,6 +165,9 @@ class CreateListener(command.ShowOne): rows = const.LISTENER_ROWS attrs = v2_utils.get_listener_attrs(self.app.client_manager, parsed_args) + + validate.check_listener_attrs(attrs) + body = {"listener": attrs} data = self.app.client_manager.load_balancer.listener_create( json=body) diff --git a/octaviaclient/osc/v2/member.py b/octaviaclient/osc/v2/member.py index fc84101..c6d53b0 100644 --- a/octaviaclient/osc/v2/member.py +++ b/octaviaclient/osc/v2/member.py @@ -20,6 +20,7 @@ from osc_lib import utils from octaviaclient.osc.v2 import constants as const from octaviaclient.osc.v2 import utils as v2_utils +from octaviaclient.osc.v2 import validate class ListMember(lister.Lister): @@ -118,7 +119,6 @@ class CreateMember(command.ShowOne): '--weight', metavar='', type=int, - choices=range(0, 256), help="The weight of a member determines the portion of requests " "or connections it services compared to the other members of " "the pool." @@ -138,7 +138,6 @@ class CreateMember(command.ShowOne): '--protocol-port', metavar='', type=int, - choices=range(1, 65535), help="The protocol port number the backend member server is " "listening on.", required=True @@ -147,7 +146,6 @@ class CreateMember(command.ShowOne): '--monitor-port', metavar='', type=int, - choices=range(1, 65535), help="An alternate protocol port used for health monitoring a " "backend member.", ) @@ -176,6 +174,9 @@ class CreateMember(command.ShowOne): def take_action(self, parsed_args): rows = const.MEMBER_ROWS attrs = v2_utils.get_member_attrs(self.app.client_manager, parsed_args) + + validate.check_member_attrs(attrs) + pool_id = attrs.pop('pool_id') body = {"member": attrs} @@ -227,14 +228,12 @@ class SetMember(command.Command): '--weight', metavar='', type=int, - choices=range(0, 256), help="Set the weight of member in the pool" ) parser.add_argument( '--monitor-port', metavar='', type=int, - choices=range(1, 65535), help="An alternate protocol port used for health monitoring a " "backend member", ) @@ -261,6 +260,9 @@ class SetMember(command.Command): def take_action(self, parsed_args): attrs = v2_utils.get_member_attrs(self.app.client_manager, parsed_args) + + validate.check_member_attrs(attrs) + pool_id = attrs.pop('pool_id') member_id = attrs.pop('member_id') post_data = {"member": attrs} diff --git a/octaviaclient/osc/v2/validate.py b/octaviaclient/osc/v2/validate.py index edc390e..a6fe347 100644 --- a/octaviaclient/osc/v2/validate.py +++ b/octaviaclient/osc/v2/validate.py @@ -13,6 +13,8 @@ from osc_lib import exceptions +from octaviaclient.osc.v2 import constants + def check_l7policy_attrs(attrs): msg = None @@ -39,3 +41,39 @@ def check_l7rule_attrs(attrs): "Missing argument: --type {type_name} requires " "--key ".format(type_name=attrs['type'])) raise exceptions.CommandError(msg) + + +# Handling these range validations here instead of "choices" as "choices" will +# output every possible option in the error message. +def _validate_TCPUDP_port_range(port_number, parameter_name): + if (port_number < constants.MIN_PORT_NUMBER or + port_number > constants.MAX_PORT_NUMBER): + msg = ("Invalid input for field/attribute '{name}', Value: " + "'{port}'. Value must be between {pmin} and {pmax}.".format( + name=parameter_name, port=port_number, + pmin=constants.MIN_PORT_NUMBER, + pmax=constants.MAX_PORT_NUMBER)) + raise exceptions.InvalidValue(msg) + + +def check_listener_attrs(attrs): + if 'protocol_port' in attrs: + _validate_TCPUDP_port_range(attrs['protocol_port'], 'protocol-port') + + +def check_member_attrs(attrs): + if 'protocol_port' in attrs: + _validate_TCPUDP_port_range(attrs['protocol_port'], 'protocol-port') + + if 'member_port' in attrs: + _validate_TCPUDP_port_range(attrs['member_port'], 'member-port') + + if 'weight' in attrs: + if(attrs['weight'] < constants.MIN_WEIGHT or + attrs['weight'] > constants.MAX_WEIGHT): + msg = ("Invalid input for field/attribute 'weight', Value: " + "'{weight}'. Value must be between {wmin} and " + "{wmax}.".format(weight=attrs['weight'], + wmin=constants.MIN_WEIGHT, + wmax=constants.MAX_WEIGHT)) + raise exceptions.InvalidValue(msg) diff --git a/octaviaclient/tests/unit/osc/v2/test_listener.py b/octaviaclient/tests/unit/osc/v2/test_listener.py index 46ac5c1..a38cf43 100644 --- a/octaviaclient/tests/unit/osc/v2/test_listener.py +++ b/octaviaclient/tests/unit/osc/v2/test_listener.py @@ -119,7 +119,7 @@ class TestListenerCreate(TestListener): ('loadbalancer', 'mock_lb_id'), ('name', self._listener.name), ('protocol', 'HTTP'), - ('protocol_port', '80') + ('protocol_port', 80) ] parsed_args = self.check_parser(self.cmd, arglist, verifylist) @@ -149,7 +149,7 @@ class TestListenerCreate(TestListener): ('loadbalancer', 'mock_lb_id'), ('name', self._listener.name), ('protocol', 'TERMINATED_HTTPS'), - ('protocol_port', '443'), + ('protocol_port', 443), ('sni_container_refs', self._listener.sni_container_refs), ('default_tls_container_ref', self._listener.default_tls_container_ref), @@ -180,7 +180,7 @@ class TestListenerCreate(TestListener): ('loadbalancer', 'mock_lb_id'), ('name', self._listener.name), ('protocol', 'HTTP'), - ('protocol_port', '80'), + ('protocol_port', 80), ('timeout_client_data', 123), ('timeout_member_connect', 234), ('timeout_member_data', 345), diff --git a/octaviaclient/tests/unit/osc/v2/test_validations.py b/octaviaclient/tests/unit/osc/v2/test_validations.py index 60d149d..c0adf78 100644 --- a/octaviaclient/tests/unit/osc/v2/test_validations.py +++ b/octaviaclient/tests/unit/osc/v2/test_validations.py @@ -14,6 +14,7 @@ from osc_lib import exceptions from osc_lib.tests import utils +from octaviaclient.osc.v2 import constants from octaviaclient.osc.v2 import validate @@ -75,3 +76,80 @@ class TestValidations(utils.TestCommand): self.assertRaises( exceptions.CommandError, validate.check_l7rule_attrs, attrs_dict) + + def test_validate_TCPUDP_port_range(self): + # Positive tests, should not raise + validate._validate_TCPUDP_port_range(constants.MIN_PORT_NUMBER, + "fake-parameter") + validate._validate_TCPUDP_port_range(constants.MAX_PORT_NUMBER, + "fake-parameter") + + # Negative tests, should raise + self.assertRaises(exceptions.InvalidValue, + validate._validate_TCPUDP_port_range, + constants.MIN_PORT_NUMBER - 1, "fake-parameter") + self.assertRaises(exceptions.InvalidValue, + validate._validate_TCPUDP_port_range, + constants.MAX_PORT_NUMBER + 1, "fake-parameter") + + def test_check_listener_attrs(self): + # Positive tests, should not raise + attrs_dict = {'protocol_port': constants.MIN_PORT_NUMBER} + validate.check_listener_attrs(attrs_dict) + attrs_dict = {'protocol_port': constants.MAX_PORT_NUMBER} + validate.check_listener_attrs(attrs_dict) + + # Negative tests, should raise + attrs_dict = {'protocol_port': constants.MIN_PORT_NUMBER - 1} + self.assertRaises(exceptions.InvalidValue, + validate.check_listener_attrs, attrs_dict) + attrs_dict = {'protocol_port': constants.MAX_PORT_NUMBER + 1} + self.assertRaises(exceptions.InvalidValue, + validate.check_listener_attrs, attrs_dict) + + def test_check_member_attrs(self): + # Positive tests, should not raise + attrs_dict = {'protocol_port': constants.MIN_PORT_NUMBER, + 'member_port': constants.MIN_PORT_NUMBER, + 'weight': constants.MIN_WEIGHT} + validate.check_member_attrs(attrs_dict) + attrs_dict = {'protocol_port': constants.MAX_PORT_NUMBER, + 'member_port': constants.MAX_PORT_NUMBER, + 'weight': constants.MAX_WEIGHT} + validate.check_member_attrs(attrs_dict) + + # Negative tests, should raise - protocol-port + attrs_dict = {'protocol_port': constants.MIN_PORT_NUMBER - 1, + 'member_port': constants.MIN_PORT_NUMBER, + 'weight': constants.MIN_WEIGHT} + self.assertRaises(exceptions.InvalidValue, validate.check_member_attrs, + attrs_dict) + attrs_dict = {'protocol_port': constants.MAX_PORT_NUMBER + 1, + 'member_port': constants.MIN_PORT_NUMBER, + 'weight': constants.MIN_WEIGHT} + self.assertRaises(exceptions.InvalidValue, validate.check_member_attrs, + attrs_dict) + + # Negative tests, should raise - member-port + attrs_dict = {'protocol_port': constants.MIN_PORT_NUMBER, + 'member_port': constants.MIN_PORT_NUMBER - 1, + 'weight': constants.MIN_WEIGHT} + self.assertRaises(exceptions.InvalidValue, validate.check_member_attrs, + attrs_dict) + attrs_dict = {'protocol_port': constants.MIN_PORT_NUMBER, + 'member_port': constants.MAX_PORT_NUMBER + 1, + 'weight': constants.MIN_WEIGHT} + self.assertRaises(exceptions.InvalidValue, validate.check_member_attrs, + attrs_dict) + + # Negative tests, should raise - weight + attrs_dict = {'protocol_port': constants.MIN_PORT_NUMBER, + 'member_port': constants.MIN_PORT_NUMBER, + 'weight': constants.MIN_WEIGHT - 1} + self.assertRaises(exceptions.InvalidValue, validate.check_member_attrs, + attrs_dict) + attrs_dict = {'protocol_port': constants.MIN_PORT_NUMBER, + 'member_port': constants.MIN_PORT_NUMBER, + 'weight': constants.MAX_WEIGHT + 1} + self.assertRaises(exceptions.InvalidValue, validate.check_member_attrs, + attrs_dict)