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.

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

libnl3 updates for MPLS #7195

Merged
merged 1 commit into from
Jun 16, 2021
Merged

libnl3 updates for MPLS #7195

merged 1 commit into from
Jun 16, 2021

Conversation

qbdwlr
Copy link
Contributor

@qbdwlr qbdwlr commented Mar 31, 2021

What I did
SONiC buildimage support for MPLS:

  1. New accessors in libnl3 for MPLS attributes
  2. bug fixes in libnl3 for MPLS attribute parsing

Why I did it
SONiC libnl3 support for MPLS

How I verified it
Unit-tests in sonic-swss/tests/test_mpls.py and sonic-utilities/tests
System tests in sonic-mgmt

Details if related
Refer to HLD: sonic-net/SONiC#706
PR to upstream to libnl3 project: thom311/libnl#284

@@ -0,0 +1,13 @@
diff --git a/lib/route/route_obj.c b/lib/route/route_obj.c
Copy link
Collaborator

Choose a reason for hiding this comment

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

are they upstreamed? can you format the patch properly, author, description, bla, bla

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes are not upstreamed. Please provide an example of how patch files should be formatted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The patch files have been reformatted as requested. A PR has been created to upstream libnl3 changes, but there have been no PRs merged in that project in 2021, so these patches remain necessary.

From c89d1a129f71d3d2f76e6cbadb11ef41d8941a73 Mon Sep 17 00:00:00 2001
From: Ann Pokora <apokora@juniper.net>
Date: Tue, 25 May 2021 18:10:04 -0700
Subject: [PATCH 2/2] mpls remove nl_addr_valid
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we remove the validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan nl_addr_valid() arguments are supposed to be a pointer to an ASCII string and an address family value (AF_MPLS, AF_INET, AF_INET6)... it then uses these to validate that the string format contains an address for the specified family.
The removed calls to nl_addr_valid() were instead passing in a pointer to a binary address and the length of the binary address... this is completely wrong. For most cases, the length of the binary address would not match any of the values of supported address families, so nl_addr_valid() would return true. But if the binary address was an MPLS label stack with 3 labels, then the length would match the value of AF_DECnet (12) and it attempts to parse the binary address as if it were an ASCII string... this fails and nl_addr_valid() returns false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you put the above explaination into the this patch file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan explanation is added to patch file.

@qbdwlr qbdwlr marked this pull request as ready for review June 3, 2021 23:34
@qbdwlr qbdwlr force-pushed the mplsPR branch 2 times, most recently from 36f7338 to ce5205f Compare June 7, 2021 03:03
@qbdwlr qbdwlr requested a review from lguohan June 7, 2021 17:29
@qbdwlr
Copy link
Contributor Author

qbdwlr commented Jun 7, 2021

@lguohan @prsunny Please review/approve this sonic-buildimage PR as the associated sonic-swss PR has build dependencies on the libnl3 changes.

{
struct nl_addr *old = nh->rtnh_newdst;

- if (!nl_addr_valid(nl_addr_get_binary_addr(addr),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing the validation, can we fix the arguments.

replace nl_addr_get_len(addr) with nl_addr_get_family(addr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smahesh That still isn't valid. Look at the code for nl_addr_valid() in libnl3. It is looking for an ASCII string and validating the string format. addr does not container a string, The return from nl_addr_get_binary_addr() is not a string.
We would have to first convert addr to string. It's pointless. nl_addr_valid() is not used anywhere else in libnl3, so I do not know why someone decided to use it in these MPLS functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nl_addr_valid() is using the route_nh_set which is not mpls specific. this is the IP functions. how can this function pass now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan RTA_NEWDST is specific to MPLS. NEWDST contains the MPLS labelstack associated with the NH.

leaf mpls {
type string {
pattern "enable|disable";
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

need description here, what does enable/disable mean here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lguohan description is added.

@qbdwlr qbdwlr force-pushed the mplsPR branch 4 times, most recently from ab18f4d to 90d5aa8 Compare June 14, 2021 15:45
Date: Tue, 25 May 2021 18:10:04 -0700
Subject: [PATCH 2/2] mpls remove nl_addr_valid

The removed calls to nl_addr_valid() are passing in a pointer to a binary address and the length of the binary address, which does not match expected arguments for nl_addr_valid(). nl_addr_valid() expects a pointer to an ASCII string and the address family of the string format. The incorrect arguments cause unexpected failures and the expected arguments are not available in the context.
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you make it more readable, make it multi-line, limit by 80 char per line?

@lguohan
Copy link
Collaborator

lguohan commented Jun 14, 2021

i am only review the libnl part, for the yang model part, it should be reviewed in the sonic yang subgroup meeting. @rlhui,
is this properly communicated to the team?

@qbdwlr qbdwlr force-pushed the mplsPR branch 2 times, most recently from e6cd1ae to 2fc8eb8 Compare June 14, 2021 17:03
@qbdwlr
Copy link
Contributor Author

qbdwlr commented Jun 14, 2021

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 7195 in repo Azure/sonic-buildimage

@prsunny
Copy link
Contributor

prsunny commented Jun 14, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rlhui
Copy link
Contributor

rlhui commented Jun 15, 2021

i am only review the libnl part, for the yang model part, it should be reviewed in the sonic yang subgroup meeting. @rlhui,
is this properly communicated to the team?

I was not aware this PR contains yang changes until last week. @qbdwlr could email the yang subgroup sonic-yang-subgroup@googlegroups.com and @anshuv-mfst to request for review asap.

@qbdwlr qbdwlr reopened this Jun 15, 2021
@qbdwlr
Copy link
Contributor Author

qbdwlr commented Jun 15, 2021

@rlhui @lguohan Changes in sonic-yang-models have been pulled from this PR and placed in a separate PR specifically for sonic-yang-models: #7881

@qbdwlr qbdwlr requested review from lguohan and smaheshm June 15, 2021 19:05
@smaheshm
Copy link
Contributor

Looks good to me.. waiting for builds to pass.

@qbdwlr qbdwlr changed the title sonic-buildimage changes for MPLS libnl3 changes for MPLS Jun 15, 2021
@qbdwlr qbdwlr changed the title libnl3 changes for MPLS libnl3 updates for MPLS Jun 15, 2021
@qbdwlr
Copy link
Contributor Author

qbdwlr commented Jun 16, 2021

@lguohan @smaheshm Please review/approve this PR. The sonic-swss PR (sonic-net/sonic-swss#1686) will not build successfully until this PR is merged to master.

Copy link
Contributor

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

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

👍

@smaheshm
Copy link
Contributor

For posterity can you add the link to libnl3 upstream patch in the description section. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants