Error not thrown when duplicate options are present on CLI

[Rajkumar]

If a neutron CLI is executed with duplicate KNOWN parameters, it does not give a Duplicate Option Error.
It gives a Duplicate Option Error if the duplicate Parameters are ´Extra Arguments´

Steps to Reproduce:
(a) create firewall rule by specifying the protocol field two times.
(b) create a port with vnic-type direct and vnic-type normal.
(c) create a network (net-create) specifying 2 tenant ids.
Actual Results:
http://paste.openstack.org/show/476797/

This is true not only for firewall, but for net-create/net-update and other Neutron CLIs as well

[Koteswara Rao Kelam (koti-kelam)]

All neutron "create" commands are taking last value if user provides duplicate options.
root@koti-icega-osc:/usr/share/pyshared# neutron subnet-create net3 4.4.4.0/24 --name sub11 --name sub22
Created a new subnet:
+------------------+------------------------------------------+
| Field | Value |
+------------------+------------------------------------------+
| allocation_pools | {"start": "4.4.4.2", "end": "4.4.4.254"} |
| cidr | 4.4.4.0/24 |
| dns_nameservers | |
| enable_dhcp | True |
| gateway_ip | 4.4.4.1 |
| host_routes | |
| id | ea1adf4b-a3b6-4f77-9dc5-8ce2fbc24378 |
| ip_version | 4 |
| name | sub22 |<<<<<<<<<<<<<<<<<<<<<<< Took last duplicate value
| network_id | 6be544d1-fc27-466a-a46e-fd9f0db79854 |
| tenant_id | 3824f7b2eb6448b9aa00a3c58f82bc1f |
+------------------+------------------------------------------+

But for neutron "update" commands, ERROR is thrown for duplicate options.
root@koti-icega-osc:/usr/share/pyshared# neutron subnet-update sub22 --name sub11 --name sub22
Duplicated options --name sub11 --name sub22

Same thing should happen for firewall rule create and update. To achieve this I modified firewall-rule-update code to throw "Duplicated options" error.

[Reedip (reedip-banerjee-deactivatedaccount)]

Dear All,
Any updates?
I found this problem affects the create CLI for both --action and --protocol

reedip@reedip-VirtualBox:/opt/stack/python-neutronclient/neutronclient$ neutron firewall-rule-create --action allow --action reject --protocol tcp
Created a new firewall_rule:
+------------------------+--------------------------------------+
| Field | Value |
+------------------------+--------------------------------------+
| action | reject |
| description | |
| destination_ip_address | |
| destination_port | |
| enabled | True |
| firewall_policy_id | |
| id | 502bbe36-76d0-4b53-938a-a6fc2ba26e56 |
| ip_version | 4 |
| name | |
| position | |
| protocol | tcp |
| shared | False |
| source_ip_address | |
| source_port | |
| tenant_id | 83bb2407a0fb484581bde56dc1fae293 |
+------------------------+--------------------------------------+
reedip@reedip-VirtualBox:/opt/stack/python-neutronclient/neutronclient$ neutron firewall-rule-create --action allow --protocol tcp --action allow
Created a new firewall_rule:
+------------------------+--------------------------------------+
| Field | Value |
+------------------------+--------------------------------------+
| action | allow |
| description | |
| destination_ip_address | |
| destination_port | |
| enabled | True |
| firewall_policy_id | |
| id | 42771b96-3d46-4501-9b44-e7e9b6719760 |
| ip_version | 4 |
| name | |
| position | |
| protocol | tcp |
| shared | False |
| source_ip_address | |
| source_port | |
| tenant_id | 83bb2407a0fb484581bde56dc1fae293 |
+------------------------+--------------------------------------+
reedip@reedip-VirtualBox:/opt/stack/python-neutronclient/neutronclient$ neutron firewall-rule-create --action allow --protocol tcp --action reject --protocol udp
Created a new firewall_rule:
+------------------------+----------------------...

[Koteswara Rao Kelam (koti-kelam)]

hi reedip, currently I am not working on this. you can assign it to yourself and work on it.

[Reedip (reedip-banerjee-deactivatedaccount)]

This affects not only neutron firewall but other CLIs as well.Seems like the neutron/v2_0/__init__.py::CreateCommand may be causing the issue( Reason: no run function is defined in the CreateCommand, but it is defined in the UpdateCommand. So it may be the reason. Analyzing further)
Example:
reedip@reedip-VirtualBox:/opt/stack/python-neutronclient/neutronclient/tests/unit$ neutron lb-healthmonitor-create --delay 21 --delay 12 --max-retries 1 --timeout 2 --timeout 10 --type PING --type HTTPS
Created a new health_monitor:
+----------------+--------------------------------------+
| Field | Value |
+----------------+--------------------------------------+
| admin_state_up | True |
| delay | 12 |
| expected_codes | 200 |
| http_method | GET |
| id | 9c9732ee-e264-4e71-8f17-ff64944a927c |
| max_retries | 1 |
| pools | |
| tenant_id | c999294ae135457b9eac85f230bff8de |
| timeout | 10 |
| type | HTTPS |
| url_path | / |
+----------------+--------------------------------------+
reedip@reedip-VirtualBox:/opt/stack/python-neutronclient/neutronclient/tests/unit$

Need to check the scope of this issue.
If I repeat any optional parameter

[Reedip (reedip-banerjee-deactivatedaccount)]

Ok,
Was working on this issue after the weekend and found a small thing that --action is not an attribute in firewall-rule-update.
As per the current code:
- If user provides the correct arguments to a CLI, the last value for the arguments will be taken, no matter how many times the arguments are repeated.
This is true for Update as well.
The reason for this is the update of the data member values_specs in the Command class.

For Firewall-Rule-Update, duplicated Protocol values:
__________________________________________________________________________________________________
reedip@reedip-VirtualBox:~/DocNCode/devstack$ python -m pdb /usr/local/bin/neutron firewall-rule-update --protocol tcp --protocol udp ec396313-b871-4ad3-8cc8-a383ba5c4f58
> /usr/local/bin/neutron(4)<module>()
-> import re
(Pdb) b /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py:528
Breakpoint 1 at /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py:528
(Pdb) continue
> /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py(528)get_parser()
-> return parser
(Pdb) n
--Return--
> /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py(528)get_parser()->Argument...elp=True)
-> return parser
(Pdb)
--Call--
> /usr/local/lib/python2.7/dist-packages/neutronclient/shell.py(94)run_command()
-> def run_command(cmd, cmd_parser, sub_argv):
(Pdb) b /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py:531
Breakpoint 2 at /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py:531
(Pdb) continue
> /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py(531)run()
-> self.log.debug('run(%s)', parsed_args)
(Pdb) p self.values_specs
[]
(Pdb)
__________________________________________________________________________________________________
For Firewall-Rule-Update, duplicated action values:
reedip@reedip-VirtualBox:~/DocNCode/devstack$ python -m pdb /usr/local/bin/neutron firewall-rule-update --action allow --action deny ec396313-b871-4ad3-8cc8-a383ba5c4f58
> /usr/local/bin/neutron(4)<module>()
-> import re
(Pdb) b /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py:528
Breakpoint 1 at /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py:528
(Pdb) continue
> /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py(528)get_parser()
-> return parser
(Pdb) n
--Return--
> /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py(528)get_parser()->Argument...elp=True)
-> return parser
(Pdb)
--Call--
> /usr/local/lib/python2.7/dist-packages/neutronclient/shell.py(94)run_command()
-> def run_command(cmd, cmd_parser, sub_argv):
(Pdb) b /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py:531
Breakpoint 2 at /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py:531
(Pdb) continue
> /usr/local/lib/python2.7/dist-packages/neutronclient/neutron/v2_0/__init__.py(531)run()
-> self.log.debug('run(%s)', parsed_args)
(Pdb) p self
<neutronclient.neutron.v2_0.fw.firewallrule.UpdateFirewallRule object a...

[Reedip (reedip-banerjee-deactivatedaccount)]

@Akihiro Motoki,@vikram.choudhary, @Ihar Hrachyshka
As per the current sequence of issues, it seems that necessary/optional duplicated parameters are ignored in both Update/Create command( and possibly other commands as well).
Although it is a current behaviour, it would be better if duplicated parameters give a warning to the user.
Maybe this can be taken up as a proposal for Mitaka Release?

Please advise.

[Reedip (reedip-banerjee-deactivatedaccount)]

As per discussion with Akihiro Motoki on IRC, proper error message should be thrown for duplicate parameters, instead of considering the last one as valid

[Reedip (reedip-banerjee-deactivatedaccount)]

Review created at :https://review.openstack.org/#/c/230321/

[Reedip (reedip-banerjee-deactivatedaccount)]

The discussion for this topic is being continued on https://etherpad.openstack.org/p/neutronclient-duplicate-option-handling

[Reedip (reedip-banerjee-deactivatedaccount)]

Hi Armando,
Please let me know what additional information is required for this issue to be moved out the Incomplete status.
I would be happy to provide the same, if possible.

[Cedric Brandily (cbrandily)]

It would be great to use neutron commands directly instead of fwrc/fwru which seem to be aliases to neutron firewall-rule-create/update?

[Reedip (reedip-banerjee-deactivatedaccount)]

@Cedric Brandily : I have updated the required information

[Reedip (reedip-banerjee-deactivatedaccount)]

@Armando, @Cedric: The paste link has been updated with a few more examples

[Armando Migliaccio (armando-migliaccio)]

Even though this would be a breaking change from the way the client used to work, it hides errors and that's not cool.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Reedip (<email address hidden>) on branch: master
Review: https://review.openstack.org/230321

[Akihiro Motoki (amotoki)]

Neutron CLI is now deprecated and we do not change the current behavior anymore.