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

Fix UpdatePathAggregator4ByteAs() ignores 32bit value #2667

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

floatingstatic
Copy link
Contributor

@floatingstatic floatingstatic commented Jun 12, 2023

I noticed that the UpdatePathAggregator4ByteAs() function is returning an error related to malformed attribute list error:

Reason="notification-sent code 3(update) subcode 1(malformed attribute list)" State=BGP_FSM_ESTABLISHED Topic=Peer

A path that triggered this looked something like:

&{0 [] 138 [{MpReach(ipv6-unicast): {Nexthop: 2001:de8:1:2::33, NLRIs: [2404:b5c0::/32]}} {Origin: i} 134911 {Med: 98} {LocalPref: 55} {Aggregate: {AS: 134911, Address: 10.0.0.1}} {Extcomms: [2:2]} {As4Aggregator: {AS: 134911, Address: 10.0.0.1}} {LargeCommunity: [ 65102:1:1]}] []

We can see in the above the Aggregate.AS was 134911 which falls into 32-bit space, but we were only setting the aggregator in cases where the kind was a uint16 here:

if attr.Value.Askind == reflect.Uint16 {
aggAttr = attr
aggAttr.Value.Askind = reflect.Uint32
}

And as a result we end up matching this condition and sending a notification:

if aggAttr == nil && agg4Attr != nil {
return bgp.NewMessageError(bgp.BGP_ERROR_UPDATE_MESSAGE_ERROR, bgp.BGP_ERROR_SUB_MALFORMED_ATTRIBUTE_LIST, nil, "AS4 AGGREGATOR attribute exists, but AGGREGATOR doesn't")
}

This change corrects this to validate the aggregator in cases where aggregator is a 32bit value.

Additionally as I was stepping through the FSM logic I noticed that table.UpdatePathAttrs4ByteAs() returns an error but we do not check the value of the error. So additionally I am adding a small fix to correct that.

Signed-off-by: Jeremiah Millay <jmillay@fastly.com>
@fujita fujita merged commit 79d301f into osrg:master Jun 14, 2023
39 checks passed
@fujita
Copy link
Member

fujita commented Jun 14, 2023

Thanks a lot!

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.

2 participants