Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

multiple staticd fixes #8101

Merged
merged 3 commits into from
Feb 17, 2021
Merged

multiple staticd fixes #8101

merged 3 commits into from
Feb 17, 2021

Conversation

idryzhov
Copy link
Contributor

Please, check individual commits descriptions.

@idryzhov
staticd: fix nexthop validation
ddd4551 
When checking for local connected address used as a nexthop gateway, we
should check nexthop VRF, not route VRF.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
@idryzhov
staticd: fix nexthop creation and installation
eec2baa 
Currently, staticd creates a VRF for the nexthop it is trying to install.
Later, when this nexthop is deleted, the VRF stays in the system and can
not be deleted by the user because "no vrf" command doesn't work for this
VRF because it was not created through northbound code.

There is no need to create the VRF. Just set nh_vrf_id to VRF_UNKNOWN
when the VRF doesn't exist.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
@idryzhov
staticd: fix vrf enabling
cb3f5e1 
When enabling the VRF, we should not install the nexthops that rely on
non-existent VRF.

For example, if we have route "1.1.1.0/24 2.2.2.2 vrf red nexthop-vrf blue",
and VRF red is enabled, we should not install it if VRF blue doesn't exist.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
@idryzhov idryzhov mentioned this pull request Feb 17, 2021
Closed
@LabN-CI
Copy link
Collaborator

LabN-CI commented Feb 17, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/8101 cb3f5e1
Date 02/17/2021
Start 07:10:42
Finish 07:50:08
Run-Time 39:26
Total 1815
Pass 1815
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2021-02-17-07:10:42.txt
Log autoscript-2021-02-17-07:11:44.log.bz2
Memory 501 461 427

For details, please contact louberger

mjstapp
mjstapp reviewed Feb 17, 2021
View reviewed changes
staticd/static_nb_config.c
@@ -140,26 +140,14 @@ static bool static_nexthop_create(struct nb_cb_create_args *args,
pn = nb_running_get_entry(args->dnode, NULL, true);
rn = nb_running_get_entry(rn_dnode, NULL, true);

if (!static_add_nexthop_validate(info->svrf, nh_type, &ipaddr))
if (!static_add_nexthop_validate(nh_vrf, nh_type, &ipaddr))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just trying to follow this: what if the nexthop-vrf is not specified? I think the expectation has been that the vrf of the route is used for all nexthops that don't supply a specific nexthop-vrf - is this a change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, if the nexthop-vrf is not specified, then the vrf field in the nexthop dnode will be the same as the vrf of the route. Nothing changed for such nexthops.
But when it is specified, then we need to check the specified one, not the one from the route.

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17193/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Debian 10 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 10 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17193/artifact/DEB10BUILD/ErrorLog/log_lintian.txt)

W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.5.0.3 (current is 4.3.0)
W: frr-snmp: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210217-02-gcb3f5e189-0 (missing) -> 7.7-dev-20210217-02-gcb3f5e189-0~deb10u1
W: frr-rpki-rtrlib: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210217-02-gcb3f5e189-0 (missing) -> 7.7-dev-20210217-02-gcb3f5e189-0~deb10u1
W: frr-pythontools: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210217-02-gcb3f5e189-0 (missing) -> 7.7-dev-20210217-02-gcb3f5e189-0~deb10u1
W: frr-doc: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210217-02-gcb3f5e189-0 (missing) -> 7.7-dev-20210217-02-gcb3f5e189-0~deb10u1
W: frr: changelog-file-missing-explicit-entry 7.5-0 -> 7.7-dev-20210217-02-gcb3f5e189-0 (missing) -> 7.7-dev-20210217-02-gcb3f5e189-0~deb10u1

CLANG Static Analyzer Summary

  • Github Pull Request 8101, comparing to Git base SHA 9ee2ebd

No Changes in Static Analysis warnings compared to base

4 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-17193/artifact/shared/static_analysis/index.html

@mjstapp mjstapp merged commit 09145e2 into FRRouting:master Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjstapp mjstapp mjstapp approved these changes

Assignees
No one assigned
Labels
staticd
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@idryzhov @LabN-CI @NetDEF-CI @mjstapp @polychaeta