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

bgpd: add [no]neighbor capability fqdn #15192

Merged
merged 3 commits into from
Feb 3, 2024
Merged

bgpd: add [no]neighbor capability fqdn #15192

merged 3 commits into from
Feb 3, 2024

Conversation

fdumontet6WIND
Copy link
Contributor

bgpd: add [no]neighbor dont-capability-negotiate hostname command

cisco routers are not dealing fairly whith unsupported capabilities.
When a cisco router receive an unsupported capabilities it reset the
negociation without notifying the unmatching capability as described in
RFC2842.
Cisco suggest the use of
neighbor x.x.x.x dont-capability-negociate capability-type
to avoid the use of "capability-type" in open message.
Our case is about the hostname capability.

this new command is to remove the use of hostname capability in the
open message with the peer "x.x.x.x".

Link: https://www.cisco.com/c/en/us/support/docs/ip/border-gateway-protocol-bgp/116189-problemsolution-technology-00.pdf

Signed-off-by: Francois Dumontet [email protected]

@frrbot frrbot bot added bgp documentation tests Topotests, make check, etc labels Jan 22, 2024
ton31337
ton31337 requested changes Jan 22, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Can you switch to no neighbor <A.B.C.D|X:X::X:X|WORD> capability fqdn format?

@fdumontet6WIND
Copy link
Contributor Author

Hi Donatas,

the draft defining that capabilty is using hostname:
https://datatracker.ietf.org/doc/html/draft-walton-bgp-hostname-capability-02

what do you prefer?

@ton31337
Copy link
Member

I meant that CLI change should be using neighbor ... capability ..., and not dont-capability-negotiate. Keep the format as others (software-version, orf, dynamic, extended-nexthop).

@fdumontet6WIND
Copy link
Contributor Author

what do you think about
[no] neighbor <A.B.C.D|X:X::X:X|WORD> capability hostname dont-negotiate

@ton31337
Copy link
Member

Why can't it be without the last dont-negotiate?

@fdumontet6WIND
Copy link
Contributor Author

the default will bee hostname capability advertising

@fdumontet6WIND
Copy link
Contributor Author

ok i will do it

@github-actions github-actions bot added the rebase PR needs rebase label Jan 22, 2024
@ton31337
Copy link
Member

And it should be fqdn, not hostname. The draft defines it as FQDN capability, even more: this is hostname + domainname, which means fqdn eventually.

@pguibert6WIND
Copy link
Member

And it should be fqdn, not hostname. The draft defines it as FQDN capability, even more: this is hostname + domainname, which means fqdn eventually.

The draft title is named "hostname", not "fqdn".
"Eventually" is a suggestion, and I think it is preferrable to stick with draft title instead.

@ton31337
Copy link
Member

Disagree, the capability is named FQDN, not hostname.

@fdumontet6WIND
Copy link
Contributor Author

conforming with https://www.iana.org/assignments/capability-codes/capability-codes.xhtml
you are right

@ton31337 ton31337 added this to the 10.0 milestone Jan 23, 2024
@pguibert6WIND pguibert6WIND mentioned this pull request Jan 23, 2024
Closed
@fdumontet6WIND
Copy link
Contributor Author

ci:rerun

ton31337
ton31337 requested changes Jan 24, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Some comments to address, sorry.

bgpd/bgp_open.c Outdated
if (cmd_domainname_get()) {
len = strlen(cmd_domainname_get());
/* FQDN capability */
if (CHECK_FLAG(peer->flags, PEER_FLAG_CAPABILITY_FQDN))
Copy link
Member

Choose a reason for hiding this comment

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

Can we combine cmd_hostname_get() and CHECK_FLAG()? This keeps the same nesting level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken into account

bgpd/bgp_vty.c Outdated
/* neighbor capability fqdn */
DEFPY (neighbor_capability_fqdn,
neighbor_capability_fqdn_cmd,
"[no$no]neighbor <A.B.C.D|X:X::X:X|WORD>$neighbor capability fqdn",
Copy link
Member

Choose a reason for hiding this comment

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

missing whitespace after no.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken into account

bgpd/bgp_vty.c Outdated
@@ -18189,6 +18207,12 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp,
if (peergroup_flag_check(peer, PEER_FLAG_DONT_CAPABILITY))
vty_out(vty, " neighbor %s dont-capability-negotiate\n", addr);

/* capability fqdn*/
Copy link
Member

Choose a reason for hiding this comment

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

/* capability fqdn */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken into account

bgpd/bgp_vty.c Show resolved Hide resolved
bgpd/bgp_vty.c Outdated
@@ -20525,6 +20549,9 @@ void bgp_vty_init(void)
install_element(BGP_NODE, &neighbor_dont_capability_negotiate_cmd);
install_element(BGP_NODE, &no_neighbor_dont_capability_negotiate_cmd);

/* "neighbor capability fqdn" command.*/
Copy link
Member

Choose a reason for hiding this comment

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

/* "neighbor capability fqdn" command. */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken into account

doc/user/bgp.rst Outdated
use of peer's name and domain name.

This capability is activated by default. The ''no neighbor PEER capability
fqdn'' avoid negotiation of that capability. This is usefull for peer that
Copy link
Member

Choose a reason for hiding this comment

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

This is useful for peers who are not supporting this capability or supporting BGP Capabilities Negotiation RFC 2842.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken into account

doc/user/bgp.rst Show resolved Hide resolved
bgpd/bgp_vty.c Outdated
@@ -18189,6 +18207,12 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp,
if (peergroup_flag_check(peer, PEER_FLAG_DONT_CAPABILITY))
vty_out(vty, " neighbor %s dont-capability-negotiate\n", addr);

/* capability fqdn*/
if (!peergroup_flag_check(peer, PEER_FLAG_CAPABILITY_FQDN))
Copy link
Member

Choose a reason for hiding this comment

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

Does this flag work with peer groups? I think we need to add that to peer_group2peer_config_copy().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it works, unitary tests are ok

tests/topotests/bgp_dont_capability_negotiate/test_bgp_dont_capability_negotiate.py Outdated
@@ -150,6 +151,47 @@ def _bgp_check_fqdn(fqdn=None):
_, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5)
assert result is None, "FQDN capability disabled, but we still have a hostname"

step("re enable sending any capability from r2")
Copy link
Member

Choose a reason for hiding this comment

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

nit: Re-enable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken into account

tests/topotests/bgp_dont_capability_negotiate/test_bgp_dont_capability_negotiate.py Show resolved Hide resolved
ton31337
ton31337 requested changes Jan 26, 2024
View reviewed changes
bgpd/bgp_open.c Outdated Show resolved Hide resolved
bgpd/bgp_open.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
doc/user/bgp.rst Outdated Show resolved Hide resolved
tests/bgpd/test_peer_attr.c Outdated Show resolved Hide resolved
@fdumontet6WIND fdumontet6WIND force-pushed the capa_nego branch 2 times, most recently from 7efdf5d to 7570b36 Compare January 26, 2024 22:17
@ton31337
Copy link
Member

@fdumontet6WIND not everything is addressed yet, please look at the comments again.

@Jafaral Jafaral changed the title Capa nego bgpd: add [no]neighbor dont-capability-negotiate hostname command Jan 29, 2024
ton31337
ton31337 reviewed Jan 30, 2024
View reviewed changes
bgpd/bgpd.c Show resolved Hide resolved
@riw777 riw777 self-requested a review January 30, 2024 15:21
@fdumontet6WIND fdumontet6WIND changed the title bgpd: add [no]neighbor dont-capability-negotiate hostname command bgpd: add [no]neighbor capability FQDN Jan 31, 2024
@fdumontet6WIND fdumontet6WIND changed the title bgpd: add [no]neighbor capability FQDN bgpd: add [no]neighbor capability fqdn Jan 31, 2024
@ton31337
Copy link
Member

ton31337 commented Feb 1, 2024

@fdumontet6WIND could you fix all the pending comments before merging?

@fdumontet6WIND
Copy link
Contributor Author

ci:rerun

ton31337
ton31337 reviewed Feb 2, 2024
View reviewed changes
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
ton31337
ton31337 reviewed Feb 2, 2024
View reviewed changes
bgpd/bgpd.c Outdated Show resolved Hide resolved
ton31337
ton31337 reviewed Feb 2, 2024
View reviewed changes
bgpd/bgpd.c Outdated Show resolved Hide resolved
ton31337
ton31337 reviewed Feb 2, 2024
View reviewed changes
bgpd/bgpd.c Outdated Show resolved Hide resolved
ton31337
ton31337 reviewed Feb 2, 2024
View reviewed changes
bgpd/bgpd.c Outdated Show resolved Hide resolved
@fdumontet6WIND
bgpd: add [no]neighbor capability fqdn command
e146ea5 
cisco routers are not dealing fairly whith unsupported capabilities.
When a cisco router receive an unsupported capabilities it reset the
negociation without notifying the unmatching capability as described in
RFC2842.
Cisco suggest the use of
neighbor x.x.x.x capability fqdn
to avoid the use of fqdn in open message.

this new command is to remove the use of fqdn capability in the
open message with the peer "x.x.x.x".

Link: https://www.cisco.com/c/en/us/support/docs/ip/border-gateway-protocol-bgp/116189-problemsolution-technology-00.pdf

Signed-off-by: Francois Dumontet <[email protected]>
@fdumontet6WIND
tests: improve topotest bgp_dont_capability_negotiate
220c063 
add some steps for testing of add [no]neighbor capability fqdn
command support.

Signed-off-by: Francois Dumontet <[email protected]>
@fdumontet6WIND
doc: add neighbor PEER capability fqdn command
d034d19 
improve bgp doc

Signed-off-by: Francois Dumontet <[email protected]>
@ton31337 ton31337 merged commit 8629700 into FRRouting:master Feb 3, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pguibert6WIND pguibert6WIND pguibert6WIND left review comments

@ton31337 ton31337 ton31337 approved these changes

@riw777 riw777 Awaiting requested review from riw777

Assignees
No one assigned
Labels
bgp documentation master rebase PR needs rebase size/L tests Topotests, make check, etc
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@fdumontet6WIND @ton31337 @pguibert6WIND