Add BGP datapath interface and implement goBGP integration #6447

hongliangl · 2024-06-14T10:17:45Z

This commit introduces an interface defining methods for managing a BGP
(Border Gateway Protocol) process. Currently, only the goBGP implementation
is available, but more implementations might be added in the future.

Required by #6203

pkg/agent/bgp/types/interface.go

tnqn

@hongliangl I was going to review the PR but found there is no description about what it does and how it does it, to build my expectation on the code, the same for the dependent PR #6203. Could you write a summary for reviewers to better understand the code? See the example.

hongliangl · 2024-06-20T06:42:42Z

@hongliangl I was going to review the PR but found there is no description about what it does and how it does it, to build my expectation on the code, the same for the dependent PR #6203. Could you write a summary for reviewers to better understand the code? See the example.

Done, but not sure if the commit message could provide enough information. Please tell me if you need more information that is not covered by the commit message.

pkg/agent/bgp/engine/gobgp.go

pkg/agent/bgp/engine/logs.go

tnqn · 2024-06-20T15:07:12Z

Please update PR description and link to issue.

antoninbas

I haven't reviewed this in-depth yet (I will give a round of review after Quan's initial comments are addressed), but I have a high-level comment about the file organization. It seems that having the engine sub-directory is not providing much value. I was picturing something like this:

pkg/agent/bgp/
    # Interface used by Antrea, independent of actual BGP implementation
    interface.go
    # Utility functions / objects which are independent of the actual BGP implementation
    utils/
    # Implementation of the interface based on gobgp
    gobgp/

test/integration/agent/gobgp_test.go

hongliangl · 2024-06-21T02:51:57Z

I haven't reviewed this in-depth yet (I will give a round of review after Quan's initial comments are addressed), but I have a high-level comment about the file organization. It seems that having the engine sub-directory is not providing much value. I was picturing something like this:
pkg/agent/bgp/
    # Interface used by Antrea, independent of actual BGP implementation
    interface.go
    # Utility functions / objects which are independent of the actual BGP implementation
    utils/
    # Implementation of the interface based on gobgp
    gobgp/

Much better! Will refactor.

hongliangl · 2024-06-21T02:52:11Z

Please update PR description and link to issue.

Will do.

pkg/agent/bgp/interface.go

pkg/agent/bgp/gobgp/gobgp.go

antoninbas · 2024-06-21T21:38:57Z

pkg/agent/bgp/gobgp/gobgp.go

+		if peer.Transport != nil {
+			peerStatus.Port = int32(peer.GetTransport().GetRemotePort())


I don't think you should mix "styles" for accessing fields, e.g., peer.Transport vs peer.GetTransport

You may be able to use something like this:

if transport := peer.GetTransport(); transport != nil { }

Another note: I don't know in which conditions these fields (e.g., Transport) can be nil, but what is the behavior we want in this case?

Edit: looking at the gobgp code, it seems that these fields are always populated when listing peers. See https://github.com/osrg/gobgp/blob/0148e2d22dcf40655d2b75c6f40bf49a378c4740/pkg/config/oc/util.go#L437. The fact that they are pointers is probably for configuration only. We can keep the nil checks but we should add a comment.

I don't think you should mix "styles" for accessing fields

Thanks for pointing out this

Edit: looking at the gobgp code, it seems that these fields are always populated when listing peers. See https://github.com/osrg/gobgp/blob/0148e2d22dcf40655d2b75c6f40bf49a378c4740/pkg/config/oc/util.go#L437. The fact that they are pointers is probably for configuration only. We can keep the nil checks but we should add a comment.

You are right. All pointer fields are not nil when peers are listed. If so, I think we could remove the nil checks, but it seems that it is harmless and safe to reserve them. Not sure which is better.

antoninbas · 2024-06-21T21:46:14Z

pkg/agent/bgp/gobgp/gobgp.go

+	if peerConfig.MultihopTTL != nil && *peerConfig.MultihopTTL != bgp.DefaultBGPMultihopTTL {
+		peer.EbgpMultihop = &gobgpapi.EbgpMultihop{
+			Enabled:     true,
+			MultihopTtl: uint32(*peerConfig.MultihopTTL),
+		}
+	}
+	if peerConfig.GracefulRestartTimeSeconds != nil {
+		peer.GracefulRestart = &gobgpapi.GracefulRestart{
+			Enabled:     true,
+			RestartTime: uint32(*peerConfig.GracefulRestartTimeSeconds),
+		}
+	}


could you clarify why MultihopTTL and GracefulRestartTimeSeconds are treated differently. In particular, we only set the MultihopTTL if it's not set to the default value, but we don't do the same thing for GracefulRestartTimeSeconds. Same question for GetPeers above.

My opinion is that we should only check for nil, not for the default. There is no harm in setting it explicitly to the default, and it makes the code easier to read / maintain.

could you clarify why MultihopTTL and GracefulRestartTimeSeconds are treated differently. In particular, we only set the MultihopTTL if it's not set to the default value, but we don't do the same thing for GracefulRestartTimeSeconds. Same question for GetPeers above.

My bad. They should be treated in the same way. The MultihopTTL and GracefulRestartTimeSeconds are always set since they have default values. Not sure if we should do the nil checks for these two fields, but it is harmless to do so. It seems that it is a convention for pointer field to do nil check even it has a default value (I saw such usage in other projects).

antoninbas · 2024-06-21T21:48:06Z

pkg/agent/bgp/gobgp/gobgp.go

+	if peerConfig.Port != nil {
+		peer.Transport = &gobgpapi.Transport{
+			RemotePort: uint32(*peerConfig.Port),
+		}
+	}
+	if peerConfig.MultihopTTL != nil && *peerConfig.MultihopTTL != bgp.DefaultBGPMultihopTTL {
+		peer.EbgpMultihop = &gobgpapi.EbgpMultihop{
+			Enabled:     true,
+			MultihopTtl: uint32(*peerConfig.MultihopTTL),
+		}
+	}
+	if peerConfig.GracefulRestartTimeSeconds != nil {


Unless I am mistaken, peerConfig.Port, peerConfig.MultihopTTL, peerConfig.GracefulRestartTimeSeconds will always be set at this point, because the default value will be populated by the apiserver? That being said it may be good to keep the nil checks. cc @tnqn

Unless I am mistaken, peerConfig.Port, peerConfig.MultihopTTL, peerConfig.GracefulRestartTimeSeconds will always be set at this point, because the default value will be populated by the apiserver?

You are right, and it seems that it is a convention to do the nil checks even when the pointer fields have default values (not very sure about that).

Personally I would prefer writing code more precisely, i.e. if it should never be nil, then do not handle nil case. It helps making code clean and improves test coverage. For user inputs or values got from external system which is not under control, I would prefer validating them in the first place (e.g. the public functions) and skip validating them repeatedly in private functions.

But I see such error or nil check often too. If it's harmless, I don't have strong opinion.

BTW, if these values are really nil, and we skip setting the corresponding gobpp parameters, will there be a problem?

I believe that if these values are nil, gobgp will use the default behavior (default transport port value, multihop disabled, graceful restart disabled).
I think the current version of the code is ok. But we could also return an error in AddPeer / UpdatePeer if any required field is missing. The "problem" is that the interface here is tightly coupled to the BGPPolicy CRD. When a CR is created through the apiserver, all these fields should be defaulted and should not be nil, but is it a good thing to rely on this when implementing this interface, even if the BGPPolicy controller will be the only consumer of the interface?
I am fine with returning an error if any of these fields is nil. Realistically , this only matters if we have multiple implementations of the interface, and we want to try to achieve the same behavior across all of them, without having to introduce duplicate defaulting logic here (as it should always be provided by the CRD).

BTW, if these values are really nil, and we skip setting the corresponding gobpp parameters, will there be a problem?

As @antoninbas said, gobgp will use the default values. The corresponding code is here: https://github.com/osrg/gobgp/blob/0148e2d22dcf40655d2b75c6f40bf49a378c4740/pkg/config/oc/default.go#L62.

I think the current version of the code is ok. But we could also return an error in AddPeer / UpdatePeer if any required field is missing.

Will add the checks for required fields like peer address and ASN.

go.mod

pkg/agent/bgp/interface.go

pkg/agent/bgp/gobgp/gobgp.go

test/integration/agent/gobgp_test.go

pkg/agent/bgp/gobgp/gobgp.go

test/integration/agent/gobgp_test.go

tnqn

LGTM overall

pkg/agent/bgp/gobgp/gobgp.go

test/integration/agent/gobgp_test.go

go.mod

tnqn

LGTM once the go mod comment is addressed

This commit introduces an interface defining methods for managing a BGP (Border Gateway Protocol) process. Currently, only the goBGP implementation is available, but more implementations might be added in the future. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl · 2024-06-26T04:46:55Z

LGTM once the go mod comment is addressed

Done. Sorry for forgetting to address the go mod comment.

antoninbas

LGTM

antoninbas · 2024-06-26T21:08:45Z

/skip-all

The code is not being used yet, so tests won't reveal anything

PRs antrea-io#6447 and antrea-io#6477 got merged around the same time, and the mock file for the BGP interface was not generated using the latest mockgen version. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>

PRs #6447 and #6477 got merged around the same time, and the mock file for the BGP interface was not generated using the latest mockgen version. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>

hongliangl requested a review from Atish-iaf June 14, 2024 10:21

hongliangl added this to the Antrea v2.1 release milestone Jun 14, 2024

hongliangl added the area/transit/bgp Issues or PRs related to BGP support. label Jun 14, 2024

Atish-iaf reviewed Jun 18, 2024

View reviewed changes

pkg/agent/bgp/types/interface.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240614-bgp-datapath branch 3 times, most recently from adf9e48 to 9a7d373 Compare June 19, 2024 07:08

hongliangl requested review from antoninbas, tnqn, jianjuns, luolanzone and Atish-iaf June 19, 2024 07:08

tnqn reviewed Jun 19, 2024

View reviewed changes

hongliangl force-pushed the 20240614-bgp-datapath branch 2 times, most recently from 5cc3dac to 5db701e Compare June 20, 2024 06:41

hongliangl requested a review from tnqn June 20, 2024 06:42

tnqn reviewed Jun 20, 2024

View reviewed changes

antoninbas reviewed Jun 20, 2024

View reviewed changes

hongliangl commented Jun 21, 2024

View reviewed changes

test/integration/agent/gobgp_test.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240614-bgp-datapath branch 2 times, most recently from 8578e48 to d950832 Compare June 21, 2024 10:43

hongliangl mentioned this pull request Jun 21, 2024

To support BGP stack integration #6189

Closed

4 tasks

hongliangl requested review from antoninbas and tnqn June 21, 2024 10:47

antoninbas reviewed Jun 21, 2024

View reviewed changes

luolanzone reviewed Jun 24, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/agent/bgp/interface.go Outdated Show resolved Hide resolved

pkg/agent/bgp/interface.go Outdated Show resolved Hide resolved

pkg/agent/bgp/interface.go Show resolved Hide resolved

hongliangl force-pushed the 20240614-bgp-datapath branch 2 times, most recently from 163d8f9 to faab6f4 Compare June 24, 2024 10:44

antoninbas reviewed Jun 24, 2024

View reviewed changes

hongliangl force-pushed the 20240614-bgp-datapath branch 3 times, most recently from 7b39dae to a108491 Compare June 25, 2024 05:51

tnqn reviewed Jun 25, 2024

View reviewed changes

antoninbas reviewed Jun 25, 2024

View reviewed changes

test/integration/agent/gobgp_test.go Outdated Show resolved Hide resolved

antoninbas reviewed Jun 25, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240614-bgp-datapath branch from a108491 to 95ad189 Compare June 26, 2024 03:39

tnqn reviewed Jun 26, 2024

View reviewed changes

hongliangl force-pushed the 20240614-bgp-datapath branch from 95ad189 to 742ceaf Compare June 26, 2024 04:43

hongliangl force-pushed the 20240614-bgp-datapath branch from 742ceaf to 4e28e45 Compare June 26, 2024 04:46

This was referenced Jun 26, 2024

Update docs/network-requirements.md #6482

Merged

Add BGPPolicy to docs/api.md #6457

Merged

antoninbas approved these changes Jun 26, 2024

View reviewed changes

antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Jun 26, 2024

antoninbas merged commit 3eefd41 into antrea-io:main Jun 26, 2024
51 of 58 checks passed

antoninbas mentioned this pull request Jun 26, 2024

Add missing whitespace in generated mock file #6489

Merged

hongliangl mentioned this pull request Jul 16, 2024

Implement the controller for API BGPPolicy #6203

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BGP datapath interface and implement goBGP integration #6447

Add BGP datapath interface and implement goBGP integration #6447

hongliangl commented Jun 14, 2024 •

edited

Loading

tnqn left a comment

hongliangl commented Jun 20, 2024

tnqn commented Jun 20, 2024

antoninbas left a comment

hongliangl commented Jun 21, 2024

hongliangl commented Jun 21, 2024

antoninbas Jun 21, 2024

antoninbas Jun 21, 2024

hongliangl Jun 24, 2024

antoninbas Jun 21, 2024

hongliangl Jun 24, 2024

antoninbas Jun 21, 2024

hongliangl Jun 24, 2024

tnqn Jun 25, 2024

tnqn Jun 25, 2024

antoninbas Jun 25, 2024

hongliangl Jun 26, 2024

hongliangl Jun 26, 2024

tnqn left a comment

tnqn left a comment

hongliangl commented Jun 26, 2024

antoninbas left a comment

antoninbas commented Jun 26, 2024

		if peer.Transport != nil {
		peerStatus.Port = int32(peer.GetTransport().GetRemotePort())

Add BGP datapath interface and implement goBGP integration #6447

Add BGP datapath interface and implement goBGP integration #6447

Conversation

hongliangl commented Jun 14, 2024 • edited Loading

tnqn left a comment

Choose a reason for hiding this comment

hongliangl commented Jun 20, 2024

tnqn commented Jun 20, 2024

antoninbas left a comment

Choose a reason for hiding this comment

hongliangl commented Jun 21, 2024

hongliangl commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

hongliangl commented Jun 26, 2024

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Jun 26, 2024

hongliangl commented Jun 14, 2024 •

edited

Loading