Junos fix next-hop self in get_bgp_config()

[ckishimo]

In Junos the nhs key is never updated when using get_bgp_config(). This PR is checking for all export policies applied at the neighbor level if there is a next-hop self action.

The next-hop self action can be applied using a term or without. Both cases are taken into account.

A test case was added. Note that I had to provide mocked data for the policies configured in the existing test case, which I added with a simple action of accept, hope that is ok! Thanks

Example:

carles@vmx# show policy-options policy-statement nhs 
term 1 {
    then {
        next-hop self;
    }
}
term 0 {
    then accept;
}

{
    "internal": {
        "apply_groups": [], 
        "description": "", 
        "export_policy": "", 
        "import_policy": "", 
        "local_address": "", 
        "local_as": 0, 
        "multihop_ttl": 0, 
        "multipath": false, 
        "neighbors": {
            "10.10.10.1": {
                "authentication_key": "", 
                "description": "", 
                "export_policy": "nhs static", 
                "import_policy": "", 
                "local_address": "", 
                "local_as": 0, 
                "nhs": true, 
                "prefix_limit": {}, 
                "remote_as": 0, 
                "route_reflector_client": false
            }
        }, 
        "prefix_limit": {}, 
        "remote_as": 0, 
        "remove_private_as": false, 
        "type": "internal"
    }
}

[coveralls]

Coverage increased (+0.06%) to 78.74% when pulling cbe6d47 on ckishimo:nhs into 0a2a542 on napalm-automation:develop.

[mirceaulinic]

That is going to be a massive performance issue: you are executing a request for each policy. I suggest doing the request (retrieving the entire config for the policies) at the beginning of the method, and just use the result here.

[ckishimo]

@mirceaulinic not sure I understand... I'm executing a request for each policy applied on export on each neighbor (not all policies!), so we know for each neighbor whether there is a policy with an action next-hop self applied.

If I take the entire config for the policies (ie: show policy-options) I will check each and every single policy, which will be even worst, non ? thanks

[dbarrosop]

you can retrieve all the policies and then do the same iteration but getting the policy from the object to retrieved before instead of having to do a request per policy.

[mirceaulinic]

That is a great catch, but please rework the implementation.

[coveralls]

Coverage increased (+0.07%) to 78.927% when pulling 51aaebe on ckishimo:nhs into a5255d0 on napalm-automation:develop.

[mirceaulinic]

This is much better @ckishimo. There are some small changes to make the code easier to understand, and also please add some comments in the code to explain the fact that the table simply provides two fields with booleans.

[mirceaulinic]

Looking at the code you added in the class I found it very confusing:

for result, value in all_policies[p]:
    if value is True:

But it makes sense looking at this table. I suggest making the name more obvious, something like junos_policy_nhs_config_table and, similarly, the fields something better than result1 and result2 which don't make much sense.

[ckishimo]

@dbarrosop thanks for the clarification

@mirceaulinic thanks for the review. I managed to get rid of one of the booleans, simplifying the code. Please let me know if there is any other change needed

[mirceaulinic]

That's great @ckishimo, and nicely implemented. Thank you!