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

bgpv2: Fix defaulting of BGP peer config #33392

Merged
merged 1 commit into from
Jun 28, 2024
Merged

bgpv2: Fix defaulting of BGP peer config #33392

merged 1 commit into from
Jun 28, 2024
+22 −15

Conversation

rastislavs
Copy link
Contributor

@rastislavs rastislavs commented Jun 25, 2024 •
edited
Loading

Fixes a crash when PeerConfigRef is not specified & uses the default peer config only when PeerConfigRef is not specified in CiliumBGPClusterConfig / CiliumBGPNodeConfig. If it is specified but not existing, skips the peer configuration instead of using the default values.

This avoids unnecessary session reset during session setup when both CiliumBGPClusterConfig and CiliumBGPPeerConfig are configured in a single batch, but PeerConfig does not exist yet at the time of processing the ClusterConfig / NodeConfig.

It also makes it more obvious for the users if they misspell the peer config name in the PeerConfigRef (peer will not be configured instead of configuring it with the default config).

bgpv2: Fix defaulting of BGP peer config, use the default peer config only when PeerConfigRef is not specified in CiliumBGPClusterConfig.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 25, 2024
@rastislavs rastislavs added area/bgp kind/enhancement This would improve or streamline existing functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jun 25, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 25, 2024
@rastislavs
Copy link
Contributor Author

/test

harsimran-pabla
harsimran-pabla approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@harsimran-pabla harsimran-pabla left a comment

Choose a reason for hiding this comment

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

Looks good 🚀

@harsimran-pabla
Copy link
Contributor

@rastislavs Do you think we should backport it to 1.16?

@harsimran-pabla
Copy link
Contributor

I was able to repro a crash if peerConfigRef is not set in the CRD.

2024-06-26T14:46:35.841603248Z panic: runtime error: invalid memory address or nil pointer dereference
2024-06-26T14:46:35.841609081Z [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x20c10f4]
2024-06-26T14:46:35.841609998Z
2024-06-26T14:46:35.841611957Z goroutine 1353 [running]:
2024-06-26T14:46:35.841614415Z github.com/cilium/cilium/pkg/bgpv1/manager/reconcilerv2.(*CiliumPeerAdvertisement).GetConfiguredAdvertisements(0x4001f209f0, 0x40000ff900, {0x40025c8fa8, 0x1, 0x1})
2024-06-26T14:46:35.841615457Z  /go/src/github.com/cilium/cilium/pkg/bgpv1/manager/reconcilerv2/peer_advertisements.go:68 +0x194

It makes sense to backport it and mark it as a bug instead of an enhancement.

@rastislavs
Copy link
Contributor Author

I was able to repro a crash if peerConfigRef is not set in the CRD.
It makes sense to backport it and mark it as a bug instead of an enhancement.

Ack, will mark & backport as bug.

@rastislavs rastislavs added kind/bug This is a bug in the Cilium logic. and removed kind/enhancement This would improve or streamline existing functionality. labels Jun 26, 2024
@rastislavs rastislavs marked this pull request as ready for review June 26, 2024 14:53
@rastislavs rastislavs requested a review from a team as a code owner June 26, 2024 14:53
@harsimran-pabla
Copy link
Contributor

Ack, will mark & backport as bug.

Also, maybe add a release blocker for 1.16.

@rastislavs
Copy link
Contributor Author

/test

@rastislavs rastislavs added the release-blocker/1.16 This issue will prevent the release of the next version of Cilium. label Jun 26, 2024
@rastislavs rastislavs changed the title bgpv2: Cleanup defaulting of peer config bgpv2: Fix defaulting of BGP peer config Jun 26, 2024
@harsimran-pabla
Copy link
Contributor

Another place this fix is required : method GetConfiguredAdvertisements:68

@rastislavs
Copy link
Contributor Author

Another place this fix is required : method GetConfiguredAdvertisements:68

@harsimran-pabla fixed

@rastislavs
Copy link
Contributor Author

/test

@rastislavs
bgpv2: Cleanup defaulting of peer config
2a19c24 
Use default peer config only when PeerConfigRef is not specified.
If it is specified but not existing, skip the peer configuration
instead of using the default values.

Signed-off-by: Rastislav Szabo <[email protected]>
@rastislavs
Copy link
Contributor Author

/test

@aanm aanm added this pull request to the merge queue Jun 28, 2024
Merged via the queue into cilium:main with commit 486515e Jun 28, 2024
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harsimran-pabla harsimran-pabla harsimran-pabla approved these changes

@YutaroHayakawa YutaroHayakawa Awaiting requested review from YutaroHayakawa YutaroHayakawa is a code owner automatically assigned from cilium/sig-bgp

Assignees
No one assigned
Labels
area/bgp kind/bug This is a bug in the Cilium logic. release-blocker/1.16 This issue will prevent the release of the next version of Cilium. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
Release blockers
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rastislavs @harsimran-pabla @aanm