Fixes bug 5362, use local initiator IP for BGP neighbor as update-sou…#5531
Fixes bug 5362, use local initiator IP for BGP neighbor as update-sou…#5531n3wtype wants to merge 2 commits into
Conversation
| } | ||
| }); | ||
|
|
||
| function checkNeighborSourceConflict() { |
There was a problem hiding this comment.
this doesn't make much sense, validation should signal the user
There was a problem hiding this comment.
Please elaborate
In UI it currently looks like this when both as set before saving:
and produce error upon save:
There was a problem hiding this comment.
If we want to inform the user beforehand, the help text is the place to do that. errors are handled correctly via the model.
There was a problem hiding this comment.
Ok got it, no interactive validation before making request, validation on model level after making request is sufficient
| neighbor {{ neighbor.address }} disable-connected-check | ||
| {% endif %} | ||
| {% if ':' not in neighbor.address and 'updatesource' in neighbor and neighbor.updatesource != '' %} | ||
| {% if 'localip' in neighbor and neighbor.localip != '' %} |
There was a problem hiding this comment.
at a first glance I would expect we can trust the validation here, in which case we shouldn't have to duplicate the logic in the template.
There was a problem hiding this comment.
I'm not sure if this is duplicates validation. uupdate-source could be both set by both updatesource and localip field. Validation on UI/API makes sure that only of them is set. However still in template we need to check which one is set to use correct variable.
Additionally later on there is additional logic involving updatesource field and IPv6 which I didn't want to change, as I believe may be not fully correct. There could bnothing wrong with setting update-source parameter for ipv6 neighbor and it prevents such setting - next thing to verify.
n3wtype
force-pushed
the
fix_5362_localip
branch
2 times, most recently
from
f5c3860 to
2c9b439
Compare
|
Removed UI validation, updated help messages. Commented change regarding template. |
| } | ||
| if (!$neighbor->updatesource->isEmpty() && !$neighbor->localip->isEmpty()) { | ||
| $messages->appendMessage(new Message( | ||
| gettext("Update-Source Interface and Local Initiator IP are mutually exclusive."), |
There was a problem hiding this comment.
https://docs.opnsense.org/development/frontend/models_constraints.html#adding-constraints
Maybe this could be done with a SingleSelectConstraint
https://github.com/opnsense/core/blob/master/src/opnsense/mvc/app/models/OPNsense/Base/Constraints/SingleSelectConstraint.php
CC @fichtner
There was a problem hiding this comment.
yes sure. this beats documenting things that people won't read regarding updating help texts we can easily avoid
There was a problem hiding this comment.
thanks for pointing out constraints, working on that
…pdate-source value * Added validation - options are mutually exclusive * FRR does not complain if given address is not on the host at the moment
n3wtype
force-pushed
the
fix_5362_localip
branch
2 times, most recently
from
30734ca to
237d2df
Compare
4 participants
…rce value
Important notices
Before you submit a pull request, we ask you kindly to acknowledge the following:
If AI was used, please disclose:
Describe the problem
As reported in 5362 there is no possibility to enforce specific src ip to be used for BGP neighbor (and related bfd session If bfd was enabled for neighbor). There is possibility to select source interface. However, in configurations with floating IP it causes undeterministic address selection.
Describe the proposed solution
Use already existing localip field, now used only for BGP authentication purpose, to be used in FRR configuration.
Explain what this pull request changes and why.
Related issue
If this pull request relates to an issue, link it here.
#5362