networktocode / fortimanager-ansible

Ansible Modules to manage Fortinet FortiManager
Other
65 stars 34 forks source link

Add "/" character escaping in API url parameters #50

Closed jcsicard closed 6 years ago

jcsicard commented 6 years ago

Didn't hit that condition in the unittests I ran. Do you have a specific unittest case to reproduce?

jmcgill298 commented 6 years ago

you can run the fortimanager_facts tests

jcsicard commented 6 years ago

So it took two months, but I finally got around to re-working my modification to handle None value parameters.
Passes the following unitttests: fortimgr_service_group_unittest.yml fortimgr_vip_group_unittest.yml fortimgr_vip_unittest.yml fortimgr_address_group_unittest.yml fortimgr_address_unittest.yml fortimgr_facts_unittest.yml fortimgr_ip_pool_unittest.yml fortimgr_lock_unittest.yml fortimgr_policy_unittest_no_policy_name.yml fortimgr_revision_unittest.yml

I'm not setup to run the other tests right now as the require a FG configured...

jmcgill298 commented 6 years ago

I tested fortimgr_address_group.py and get this failure:

TASK [CREATE ADDRESS GROUP AGAIN - NO CHANGE] **********************************
...
fatal: [fm01]: FAILED! => {
    "changed": false, 
    "fortimanager_response": {
        "result": [
            {
                "status": {
                    "code": -2, 
                    "message": "Object already exists"
                }, 
                "url": "/pm/config/adom/lab/obj/firewall/addrgrp"
            }
        ]
    }, 
    "invocation": {
        "module_args": {
            "address_group_name": "addrgrp4/slash", 
            "adom": "lab", 
            "allow_routing": "enable", 
            "color": 3, 
            "comment": "addrgroup unit test", 
            "host": "10.1.100.122", 
            "lock": true, 
            "members": [
                "addr1", 
                "10.1.1.1/32 addr7"
            ], 
            "password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER", 
            "port": null, 
            "provider": null, 
            "session_id": null, 
            "state": null, 
            "use_ssl": null, 
            "username": "admin", 
            "validate_certs": null
        }
    }, 
    "locked": true, 
    "msg": "Unable to Apply Config", 
    "saved": false, 
    "unlocked": true
}
jcsicard commented 6 years ago

Could it not be my latest version (ie: I may have botched the PR...) ? That test is named "CREATE ADDRESS GROUP SLASH IN NAME AGAIN - NO CHANGE" in my commit.

jmcgill298 commented 6 years ago

I cloned your fix-slash-objects branch and ran it

jmcgill298 commented 6 years ago

Actually, I reran it, and I believe my initial tests were not using the library directory from your branch. This is passing now, so let me continue running tests.

jmcgill298 commented 6 years ago

I believe you need to to typecast url to string:

if url is not None:
    return str(url).replace('/', '\\/')
else:
    return None

I need to retest some stuff, but I was getting an issue with the route module because the unique identifier being used is the sequence number, which is an integer. I think it makes sense to typecast in this method instead of letting each module remember that it needs to send a string.

jcsicard commented 6 years ago

Ok, I added the explicit typecast of the url var. I need to spin up a fortigate-vm (lab1) to run the other unittests including the route module... Are there assumed pre-configurations of the lab1 device in the unittests for them to pass?