Support LWTUNNEL_ENCAP_SEG6 by ebiken · Pull Request #282 · vishvananda/netlink

ebiken · 2017-09-27T15:22:43Z

Added LWTUNNEL_ENCAP_SEG6 support.
With this change, one can do things similar to below iproute2 command.

> Add seg6 routes
ip route add 10.0.0.20/32 encap seg6 mode inline segs fc00:00dd::1,fc00:00dd::2 dev enp0s3
ip route add 10.0.0.30/32 nexthop encap seg6 mode encap segs fc00:00bc::1 dev enp0s3 weight 10 \
nexthop encap seg6 mode inline segs fc00:00bc::2 dev enp0s8 weight 20
ip -6 route add fc00:000b::/64 encap seg6 mode encap segs fc00:00bc::1 dev enp0s3

> View seg6 routes
dev2222> ip route
10.0.0.20  encap seg6 mode inline segs 3 [ fc00:dd::1 fc00:dd::2 :: ] dev enp0s3 scope link
10.0.0.30
        nexthop  encap seg6 mode encap segs 1 [ fc00:bc::1 ] dev enp0s3 weight 10
        nexthop  encap seg6 mode inline segs 2 [ fc00:bc::2 :: ] dev enp0s8 weight 20
dev2222>ip -6 route
fc00:b::/64  encap seg6 mode encap segs 1 [ fc00:bc::1 ] dev enp0s3 metric 1024 pref medium

ebiken · 2017-10-11T08:46:37Z

Hi. Let me know if there is any modification I should make for this change. Thanks!

aboch · 2017-11-01T16:37:30Z

nl/seg6_linux.go

+	if len(s1.Segments) != len(s2.Segments) {
+		return false
+	}
+	if len(s1.Segments) != 0 {


Minor comment: This if check is not really needed, if no segments are there, the for loop will be a no op.

Thanks for the review. I will remove this if.

aboch · 2017-11-01T16:48:13Z

nl/seg6_linux.go

+	b[9] = srh.flags
+	native.PutUint16(b[10:], srh.reserved)
+	for _, netIP := range srh.Segments {
+		b = append(b, netIP.To16()...)


Why to16() ? The function is for representing a 4 byte IPv4 address into the 16 bytes IPv4 in IPv6 notation.
Being srh of type Ipv6SrHdr, I was expecting its segments to be already IPv6 addresses.

Yes. Will change to b = append(b, netIP...).

aboch · 2017-11-01T16:54:51Z

nl/seg6_linux.go

+	"net"
+)
+
+type Ipv6SrHdr struct {


Minor: I think IP should be capitalized IPv6SrHdr

OK. Will change to IPv6SrHdr.

aboch · 2017-11-01T17:06:37Z

nl/seg6_linux.go

+type Ipv6SrHdr struct {
+	nexthdr       uint8
+	hdrlen        uint8
+	typ           uint8


Minor: To avoid using the reserved type string, can you please rename this field to routingType ?
I know it is an internal field, but I am not sure typ would directly convey what it represents to somebody reading the code.

I followed what's used as temp var in MPLS code.
But yes, routingType should be easier to understand as a name of IPv6SrHdr member.
I changed ones in nl/seg6_linux.go to routingType.
I kept ones in route_linux.go as is typ since they are temp var used very close to where defined by :=.

aboch · 2017-11-01T17:08:26Z

route_linux.go

+	if !ok {
+		return false
+	}
+	if e == nil && o == nil {


Very minor: This could be replaced by

if e == o { return true }

it would also avoid the subsequent field by field comparison on same object comparison call.

Thank you. Very educational. Will change it.

aboch · 2017-11-01T17:20:30Z

nl/seg6_linux.go

+)
+
+type Ipv6SrHdr struct {
+	nexthdr       uint8


This project in most parts seems to be following the convention of CamelCase naming.
Please avoid underscore in structure field names and change this one to something like hdrLen or headerLen what you prefer.

Changed to below.

type IPv6SrHdr struct { nextHdr uint8 hdrLen uint8 routingType uint8 segmentsLeft uint8 firstSegment uint8

aboch · 2017-11-01T17:27:07Z

nl/seg6_linux.go

+	native.PutUint32(b, uint32(mode))
+	srh.nexthdr = 0                     // use 0 when calling netlink
+	srh.hdrlen = uint8(16 * nsegs >> 3) // in 8-octets unit
+	srh.typ = 4                         // Routing Type to be assigned by IANA (suggested value: 4)


Shouldn't we have a constant for this as does the kernel code

#define IPV6_SRCRT_TYPE_4 4 /* Segment Routing with IPv6 */

Defined them in nl/syscall.go

// routing header types const ( IPV6_SRCRT_STRICT = 0x01 // Deprecated; will be removed IPV6_SRCRT_TYPE_0 = 0 // Deprecated; will be removed IPV6_SRCRT_TYPE_2 = 2 // IPv6 type 2 Routing Header IPV6_SRCRT_TYPE_4 = 4 // Segment Routing with IPv6 )

And changed to: srh.routingType = IPV6_SRCRT_TYPE_4 // Routing Type assigned by IANA

aboch · 2017-11-01T17:31:44Z

nl/seg6_linux.go

+		err := fmt.Errorf("DecodeSEG6Encap: Error parsing Segment List (buf len: %d)\n", len(buf))
+		return mode, srh, err
+	}
+	seglist := make([]net.IP, 0, len(buf)/16)


Very minor: We do not need the extra seglist slice, we could just replace it with srh.Segments.

OK. Removed seglist := make and modified append line as below:
srh.Segments = append(srh.Segments, net.IP(buf[:16])).

aboch · 2017-11-01T17:36:26Z

route_linux.go

+}
+func (e *SEG6Encap) Decode(buf []byte) error {
+	if len(buf) < 4 {
+		return fmt.Errorf("Lack of bytes")


This file and other ones in this project seems to be following the go recommendation where the reported error strings start in lower case. Please adhere to this convention here and in the other places.

OK. I fixed ones in this PR and from MPLS code which had same error message. (ex: "Lack of bytes")

ebiken · 2017-11-02T05:08:20Z

Applied changes based on feedback. Fixed conflict due to syscall -> unix checkin recently done on master.
Please let me know if I missed anything. Thanks!!

aboch · 2017-11-02T21:01:01Z

route_linux.go

+// SEG6 definitions
+type SEG6Encap struct {
+	Mode int
+	Srh  nl.IPv6SrHdr


This is the last thing I am not convinced about.

Seg6Encap is netlink pkg user interface, lib client will have to provide it when calling netlink.AddRoute().
I am not sure we should have client import netlink/nl for setting Srh, as it is done now for convenience I guess, where only its Segments field is exported.

I took a look at other netlink pkg API, for the most no netlink/nl types need to be fed in.

Probably this type could be changed to

type SEG6Encap struct { Mode int Segments []net.IP }

and the respective nl.IPv6SrHdr could be created internally when interfacing with netlink/nl methods.

I have changed to Segments []net.IP as you suggested.
I removed using IPv6SrHdr in EncodeSEG6Encap to make code cleaner and short.
I kept using it in DecodeSEG6Encap in case one wants to do granular check with func (s1 *IPv6SrHdr) Equal(s2 IPv6SrHdr) bool by directly using netlink/nl.
For example, there could be a case that only segmentsLeft firstSegment are different for two seg6 route entries. However, explicitly configuring those values via iproute2 is not currently supported. Thus, will leave Encap part for future when someone actually wants to do such operation.

aboch · 2017-11-02T21:02:28Z

Thank you @ebiken, I have one last comment, otherwise it looks good to me.

ebiken · 2017-11-03T04:31:28Z

Note: I have run the test and confirmed it pass after the change.

# go test -run TestSEG6RouteAddDel
PASS
ok      github.com/ebiken/netlink       0.005s

aboch · 2017-11-03T05:08:23Z

Thank you @ebiken, looks good to me.

ebiken · 2017-11-04T09:04:22Z

Thank you @aboch for many suggestions.
Do you have write access to the main branch? If so, appreciate if you can merge this.

ebiken force-pushed the seg6new branch from 232bec4 to 7de0152 Compare October 5, 2017 09:34

aboch reviewed Nov 1, 2017

View reviewed changes

ebiken force-pushed the seg6new branch 2 times, most recently from a0f9e2d to 8d1eb07 Compare November 2, 2017 05:03

aboch reviewed Nov 2, 2017

View reviewed changes

Support LWTUNNEL_ENCAP_SEG6

5977a4b

ebiken force-pushed the seg6new branch from 8d1eb07 to 5977a4b Compare November 3, 2017 04:15

vishvananda merged commit eb7ed87 into vishvananda:master Nov 8, 2017

ebiken deleted the seg6new branch January 10, 2018 07:06

thaJeztah mentioned this pull request Mar 19, 2018

[wip] Bump vishvananda/netlink to 1.0.0 moby/libnetwork#2118

Closed

thaJeztah mentioned this pull request Apr 10, 2019

Bump vishvananda/netlink to 1.0.0 moby/libnetwork#2366

Merged

Conversation

ebiken commented Sep 27, 2017

Uh oh!

ebiken commented Oct 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboch Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebiken commented Nov 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboch commented Nov 2, 2017

Uh oh!

ebiken commented Nov 3, 2017

Uh oh!

aboch commented Nov 3, 2017

Uh oh!

ebiken commented Nov 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aboch Nov 1, 2017 •

edited

Loading