[WIP] Quagga: Add BGP to UI #112

mimugmail · 2017-04-05T13:49:14Z

Ok, so I've changed all the stuff, but still the bgpd.conf is empty and there's no error when trying to enable it.

Added bgpd.conf

Need many replaces

Needs multiple replacements

needs many adjustments

Need to replace Network to router ASN

Perhaps we can use this file as a template for Route-map

mimugmail · 2017-04-12T12:53:45Z

@fabianfrz OK please review, I'm finished now.
I'm able to add und remove peer's, set remote as and next-hop-self and update-source.

With this stage you can run basic eBGP and iBGP.

There are hundreds of options missing like route-maps etc. but for basic BGP this runs fine.

EDIT: Thank you for your help! 👍

fabianfrz

Looks good so far but contains little issues.

@fichtner If the changes are done fast, this may go into the release build.

fabianfrz · 2017-04-12T13:56:27Z

net/quagga/src/opnsense/mvc/app/models/OPNsense/Quagga/BGP.xml

+                                <default></default>
+                                <Required>N</Required>
+                        </updatesource>
+						<nexthopself type="BooleanField">


looks strange (indent)

fabianfrz · 2017-04-12T13:57:12Z

net/quagga/src/opnsense/mvc/app/views/OPNsense/Quagga/bgp.volt

+                <th data-column-id="remoteas" data-type="string" data-visible="true">{{ lang._('Remote AS') }}</th>
+                <th data-column-id="updatesource" data-type="string" data-visible="true">{{ lang._('Update Source Address') }}</th>
+		<th data-column-id="nexthopself" data-type="string" data-formatter="rowtoggle">{{ lang._('Next Hop Self') }}</th>
+		<th data-column-id="defaultoriginate" data-type="string" data-formatter="rowtoggle">{{ lang._('Default Originate') }}</th>


For me it looks ok?

tabs vs. spaces, indent is PSR style, 4 spaces per indent

but we have a style fixer for this too

Hm, within github UI it's ok. So I'll leave it and you'll fix it?

You mixed spaces and tabs. I replaced the tabs in those two lines with the same amount of spaces like the other lines. looks better now. In general, we do not use tabs in the source.

fabianfrz · 2017-04-12T13:58:07Z

net/quagga/src/opnsense/mvc/app/views/OPNsense/Quagga/bgp.volt

+                <td colspan="5"></td>
+                <td>
+                    <button data-action="add" type="button" class="btn btn-xs btn-default"><span class="fa fa-plus"></span></button>
+                    <!-- <button data-action="deleteSelected" type="button" class="btn btn-xs btn-default"><span class="fa fa-trash-o"></span></button> -->


You may remove this button if you like.

I'd let it there if it's ok for you

no problem.

fabianfrz · 2017-04-12T13:59:52Z

net/quagga/src/opnsense/service/templates/OPNsense/Quagga/bgpd.conf

+{%  endif %}
+{% if helpers.exists('OPNsense.quagga.bgp.neighbors.neighbor') %}
+{%   for neighbor in helpers.toList('OPNsense.quagga.bgp.neighbors.neighbor') %}
+{%     if neighbor.enabled == '1' %}


you may put the whole thing under an if statement which checks if it is enabled

Ok I put all under an if, seems to work fine, thanks for the hint!

fabianfrz · 2017-04-12T14:01:40Z

net/quagga/src/opnsense/service/templates/OPNsense/Quagga/bgpd.conf

+{%     if neighbor.enabled == '1' %}
+ neighbor {{ neighbor.address }} remote-as {{ neighbor.remoteas }} 
+{%     endif %}
+{%     if neighbor.enabled == '1' and neighbor.updatesource != '' %}


This is a field which is not required - replace neighbor.enabled == '1' by an existence check - also at the other lines later

If I don't check for neighbor enabled these line will stay if I disable a neighbor. I'll try to put the whole thing under an if as you stated before

fabianfrz · 2017-04-12T14:03:41Z

net/quagga/src/opnsense/mvc/app/views/OPNsense/Quagga/bgp.volt

+{#
+
+OPNsense® is Copyright © 2014 – 2017 by Deciso B.V.
+This file is Copyright © 2017 by Fabian Franz


really not your copyright?

I copied your file and replaced ospf with bgp, it's yours :)
I added a noted with my name

files are copied, your own copyrights are added, no additions that weren't in there before. @fabianfrz must add his copyrights on his own (or not)

fabianfrz · 2017-04-12T14:06:49Z

net/quagga/src/opnsense/mvc/app/controllers/OPNsense/Quagga/Api/BgpController.php

+        return $result;
+    }
+
+///////////////////////////////////////////////////////////////////// TO BE REVIEWED


remove this line

Ok, it's gone and I added some newlines before the functions

fabianfrz · 2017-04-12T14:07:12Z

net/quagga/src/opnsense/mvc/app/controllers/OPNsense/Quagga/Api/BgpController.php

+use \OPNsense\Base\UIModelGrid;
+/**
+ *    Copyright (C) 2015 - 2017 Deciso B.V.
+ *    Copyright (C) 2017 Fabian Franz


Not your copyright?

Same as above, it's yours!

fabianfrz · 2017-04-12T14:10:56Z

net/quagga/src/opnsense/mvc/app/models/OPNsense/Quagga/BGP.xml

+                                <Required>N</Required>
+                        </nexthopself>
+						<defaultoriginate type="BooleanField">
+                                <default>0</default>


Already fixed

fichtner · 2017-04-12T16:17:10Z

net/quagga/src/opnsense/mvc/app/controllers/OPNsense/Quagga/BgpController.php

@@ -2,6 +2,7 @@
 namespace OPNsense\Quagga;
 /*
    Copyright (C) 2017 Fabian Franz
+    Modified for BGP functionality by Michael Muenz


It should really read the following:

Copyright (C) 2017 Michael Muenz

don't be shy :)

mimugmail · 2017-04-12T16:30:35Z

@fabianfrz @fichtner Ok I'm done with the reviews so far, thanks for the fast review!

mimugmail · 2017-04-12T18:17:46Z

@fabianfrz Stupid question: Do have to edit the indent stuff on my own or will your commit merged too?

fabianfrz · 2017-04-12T18:53:10Z

Everything will be merged together as it is on your branch.

fabianfrz · 2017-04-13T07:19:04Z

@mimugmail is is probably only the template that needs changes (maybe whitespace stuff too). I think we can merge it after this changes are done. I am sure @fichtner could do the whitespace cleanup.

fabianfrz · 2017-04-14T20:06:27Z

I close this for #121

fabianfrz · 2017-04-15T07:03:05Z

@mimugmail your changes are merged now.

fabianfrz · 2017-04-15T07:04:12Z

Please delete your changes from master now or create a new clone so you can fast forward your master branch.

mimugmail added 30 commits April 1, 2017 07:35

Added bgpd.conf

57f5dca

Added bgpd.conf

First copy of ospfd.conf

5878e48

Need many replaces

Changed ospf to bgp

abf9758

Create BGP.php from OSPF.php

11912b3

Copied OSPF.xml to BGP.xml

011b33b

Needs multiple replacements

Enabled BGP in Menu.xml

0fcf146

Copied OSPF version to BGP

f34684b

needs many adjustments

Copied from OSPF

7e1d3f4

Need to replace Network to router ASN

Copied from OSPF

8823225

Perhaps we can use this file as a template for Route-map

Updated with OspfController settings

2381dd2

Replaced one Ospf setting

cca1474

Added redistribution

42402cd

Update dialogEditBGPNetwork.xml

c5ae465

Update BgpsettingsController.php

8751c83

Update BGP.xml

55cf6c1

Update BgpController.php

fd79b9c

Update bgp.xml

b7b0034

Copied layout from ospf.volt

9c8e413

Update bgp.volt

024329f

Update BGP.xml

498b457

Update BGP.xml

96dcff2

Update bgpd.conf

583668e

Update bgpd.conf

6d4aa50

Update BGP.xml

ccb4b5f

Added AS Number

c81439e

Update bgp.xml

81915fa

Delete dialogEditBGPInterface.xml

964e6ab

Delete dialogEditBGPNetwork.xml

53306a6

Update BgpController.php

20ba34a

Update BgpsettingsController.php

6e9e7e3

mimugmail added 2 commits April 12, 2017 14:34

Update bgpd.conf

38713cb

Update bgpd.conf

4b55ac7

fabianfrz suggested changes Apr 12, 2017

View reviewed changes

fabianfrz reviewed Apr 12, 2017

View reviewed changes

fabianfrz changed the title ~~[WIP] Quagga: Add BGP to UI / 2~~ [WIP] Quagga: Add BGP to UI Apr 12, 2017

mimugmail added 4 commits April 12, 2017 18:07

Update BGP.xml

dbc68f2

Update bgp.volt

a75c8b1

Update BgpController.php

fe90cfb

Update BgpController.php

5de1c32

fichtner reviewed Apr 12, 2017

View reviewed changes

Update bgpd.conf

87da253

mimugmail and others added 6 commits April 12, 2017 18:33

Update bgp.volt

1b395b9

Update BGP.php

a1beb4d

Update BgpController.php

fa3a48a

Update BgpController.php

cdf519b

indent help tags as the other lines

e4f47d1

indent: use the same for all th tags (spaces)

f83600e

Update bgpd.conf

4f5b151

fabianfrz mentioned this pull request Apr 14, 2017

BGP #121

Merged

fabianfrz closed this Apr 14, 2017

fabianfrz reopened this Apr 15, 2017

fabianfrz merged commit 04c0efc into opnsense:master Apr 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Quagga: Add BGP to UI #112

[WIP] Quagga: Add BGP to UI #112

mimugmail commented Apr 5, 2017

mimugmail commented Apr 12, 2017 •

edited

Loading

fabianfrz left a comment

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fichtner Apr 12, 2017

mimugmail Apr 12, 2017

fabianfrz Apr 12, 2017

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fabianfrz Apr 12, 2017

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fichtner Apr 12, 2017 •

edited

Loading

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fabianfrz Apr 12, 2017

mimugmail Apr 12, 2017

fichtner Apr 12, 2017

mimugmail commented Apr 12, 2017

mimugmail commented Apr 12, 2017

fabianfrz commented Apr 12, 2017

fabianfrz commented Apr 13, 2017

fabianfrz commented Apr 14, 2017

fabianfrz commented Apr 15, 2017

fabianfrz commented Apr 15, 2017

[WIP] Quagga: Add BGP to UI #112

[WIP] Quagga: Add BGP to UI #112

Conversation

mimugmail commented Apr 5, 2017

mimugmail commented Apr 12, 2017 • edited Loading

fabianfrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fichtner Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimugmail commented Apr 12, 2017

mimugmail commented Apr 12, 2017

fabianfrz commented Apr 12, 2017

fabianfrz commented Apr 13, 2017

fabianfrz commented Apr 14, 2017

fabianfrz commented Apr 15, 2017

fabianfrz commented Apr 15, 2017

mimugmail commented Apr 12, 2017 •

edited

Loading

fichtner Apr 12, 2017 •

edited

Loading