Add document for BGPPolicy #6524

hongliangl · 2024-07-12T10:05:17Z

No description provided.

docs/bgp-policy.md

jianjuns

LGTM

docs/bgp-policy.md

antoninbas

Only minor comments left, otherwise lgtm

docs/bgp-policy.md

antoninbas · 2024-07-23T18:08:36Z

docs/bgp-policy.md

+  - All Nodes can advertise all ClusterIPs, respecting `internalTrafficPolicy`. If `internalTrafficPolicy` is set to
+    `Local`, a Node will only advertise ClusterIPs with at least one local Endpoint.


This is a bit surprising, because it feels like internalTrafficPolicy is not something that should impact traffic coming from external routers. However, ClusterIPs were not meant to be externally routable anyway so it's already a bit of an unusual situation. cc @tnqn. If this is the behavior that was agreed upon, then this is fine by me.

Right, I thought about this and didn't figure out a better implementation. I think in practice users will only advertise ClusterIPs when they don't have any ExternalIPs and LoadBalancerIPs, and they intend to reduce costs and improve performance by setting the internal traffic policy to local, then it seems fine to only advertise from Nodes that have local Pods to achieve the same goals for external traffic.

Besides, I suppose making external-to-clusterIP traffic not enforce internalTrafficPolicy would impact Antrea Proxy's implementation a lot.

I suppose making external-to-clusterIP traffic not enforce internalTrafficPolicy would impact Antrea Proxy's implementation a lot.

makes sense

docs/bgp-policy.md

antoninbas

Need to make adjustments to my earlier suggestions for the BGP router ID section, after reviewing the actual code. I didn't realize earlier that the annotation had to be an actual IPv4 address string (a.b.c.d) and I though a 32-bit integer was to be used.

docs/bgp-policy.md

antoninbas · 2024-07-23T18:38:46Z

@hongliangl The doc should probably mention the BGP secret, as pointed out by @luolanzone in another PR review

hongliangl · 2024-07-23T20:36:28Z

@hongliangl The doc should probably mention the BGP secret, as pointed out by @luolanzone in another PR review

Added

antoninbas · 2024-07-23T21:03:49Z

docs/bgp-policy.md

+BGP authentication ensures that BGP sessions are established and maintained only with legitimate peers. Antrea supports
+BGP authentication by leveraging a Kubernetes Secret, `antrea-bgp-passwords`, to store authentication passwords.


For the second sentence, I suggest:

Users can provide authentication passwords for the different BGP peering sessions by storing them in a Kubernetes secret. The secret must be defined in the same Namespace as Antrea (kube-system by default) and must be named antrea-bgp-passwords.

docs/bgp-policy.md

antoninbas · 2024-07-23T21:06:51Z

docs/bgp-policy.md

+the password for that BGP peer. If a key for a BGP peer does not exist in the Secret, the peer is considered unconfigured
+for that one.


For the last sentence:

If a given BGP peer does not have a corresponding key in the Secret data, then authentication is considered disabled for that peer.

docs/bgp-policy.md

antoninbas

LGTM

docs/bgp-policy.md

antoninbas

LGTM

luolanzone

LGTM

tnqn · 2024-07-25T14:58:15Z

docs/bgp-policy.md

+1. If the `node.antrea.io/bgp-router-id` annotation is present on the Node and its value is a valid IPv4 address string,
+   we will use the provided value.


This doesn't match the implementation which always uses Node IPv4 when there is one.

Thanks for reminding me about that. I have corrected it.

Thanks for catching this @tnqn. I'm partially to blame here, as I suggested the previous wording without double-checking the implementation.
I don't want to block the BGP PRs for this, but the implementation doesn't make much sense to me. I see no reason why the annotation should not be honored in the IPv4 case. If anything, that just creates confusion. RFC 6286 relaxes the definition of the BGP router ID, and I don't think we need to "force" the ID to be the Node IP in the IPv4 case.

(I am perfectly fine having this discussion and potentially making this change post v2.1 release btw)

@antoninbas I agree with you. I wondered why not to handle IPv4/IPv6 consitently but didn't want to ask for a change in the implementation given the tight schedule. We could unify them in 2.2.

I created #6550 yesterday to track this

tnqn · 2024-07-25T14:59:33Z

docs/bgp-policy.md

+3. Otherwise, for IPv6-only clusters, a 32-bit integer will be generated by hashing the Node name, then converted to the
+   string representation of an IPv4 address.
+
+After this selection process, the `node.antrea.io/bgp-router-id` annotation is added / updated as necessary to reflect


ditto, I think only IPv6 updates the annotation.

tnqn

LGTM except two comments on the examples

tnqn · 2024-07-25T15:05:33Z

docs/bgp-policy.md

+  listenPort: 179
+  advertisements:
+    service:
+      ipTypes: [ClusterIP, LoadBalancerIP]


I think we should remove ClusterIP to make the example more reasonable. Normally there is no reason to advertise ClusterIP when the cluster has LoadBalancer enabled.

tnqn · 2024-07-25T15:05:57Z

docs/bgp-policy.md

+  listenPort: 179
+  advertisements:
+    service:
+      ipTypes: [ClusterIP, ExternalIP, LoadBalancerIP]


ditto, we should at least remove ClusterIP.

Signed-off-by: Hongliang Liu <[email protected]>

antoninbas

LGTM

antoninbas · 2024-07-25T20:44:52Z

/skip-all

hongliangl added the kind/documentation Categorizes issue or PR as related to a documentation. label Jul 12, 2024

hongliangl added this to the Antrea v2.1 release milestone Jul 12, 2024

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

hongliangl force-pushed the 20240712-bgp-document branch from 3a474bf to 56a32cd Compare July 15, 2024 02:55

hongliangl marked this pull request as ready for review July 15, 2024 02:58

hongliangl requested review from luolanzone, tnqn, jianjuns and antoninbas July 15, 2024 02:58

luolanzone reviewed Jul 15, 2024

View reviewed changes

hongliangl force-pushed the 20240712-bgp-document branch from b41974a to 344f72c Compare July 15, 2024 07:40

hongliangl requested a review from luolanzone July 15, 2024 07:42

jianjuns reviewed Jul 15, 2024

View reviewed changes

hongliangl mentioned this pull request Jul 16, 2024

Implement the controller for API BGPPolicy #6203

Merged

7 tasks

hongliangl force-pushed the 20240712-bgp-document branch from 344f72c to 5390947 Compare July 16, 2024 10:00

hongliangl requested a review from jianjuns July 16, 2024 10:00

jianjuns reviewed Jul 16, 2024

View reviewed changes

docs/bgp-policy.md Outdated Show resolved Hide resolved

docs/bgp-policy.md Outdated Show resolved Hide resolved

docs/bgp-policy.md Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240712-bgp-document branch from 5390947 to f544197 Compare July 17, 2024 03:01

hongliangl requested a review from jianjuns July 17, 2024 03:02

jianjuns previously approved these changes Jul 17, 2024

View reviewed changes

hongliangl dismissed jianjuns’s stale review via 106f8b6 July 18, 2024 03:28

hongliangl force-pushed the 20240712-bgp-document branch from f544197 to 106f8b6 Compare July 18, 2024 03:28

luolanzone previously approved these changes Jul 19, 2024

View reviewed changes

hongliangl dismissed luolanzone’s stale review via 36c44f8 July 19, 2024 09:09

jianjuns reviewed Jul 19, 2024

View reviewed changes

docs/bgp-policy.md Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240712-bgp-document branch 2 times, most recently from 8ce56c4 to 3018739 Compare July 22, 2024 03:49

jianjuns reviewed Jul 22, 2024

View reviewed changes

docs/bgp-policy.md Outdated Show resolved Hide resolved

antoninbas reviewed Jul 22, 2024

View reviewed changes

hongliangl force-pushed the 20240712-bgp-document branch from 3018739 to 95ca1ca Compare July 23, 2024 04:31

hongliangl requested a review from jianjuns July 23, 2024 04:44

antoninbas reviewed Jul 23, 2024

View reviewed changes

docs/bgp-policy.md Outdated Show resolved Hide resolved

docs/bgp-policy.md Outdated Show resolved Hide resolved

docs/bgp-policy.md Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240712-bgp-document branch 2 times, most recently from 16fdf50 to 5f0e135 Compare July 23, 2024 20:35

hongliangl force-pushed the 20240712-bgp-document branch from 5f0e135 to 6ec4a80 Compare July 23, 2024 20:37

antoninbas reviewed Jul 23, 2024

View reviewed changes

hongliangl force-pushed the 20240712-bgp-document branch from 6ec4a80 to 42f81a5 Compare July 23, 2024 23:06

hongliangl requested a review from antoninbas July 23, 2024 23:08

luolanzone reviewed Jul 24, 2024

View reviewed changes

docs/bgp-policy.md Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240712-bgp-document branch 2 times, most recently from 035c96b to 2bd84a2 Compare July 24, 2024 08:58

hongliangl requested a review from luolanzone July 24, 2024 08:58

antoninbas reviewed Jul 24, 2024

View reviewed changes

docs/bgp-policy.md Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240712-bgp-document branch from 2bd84a2 to 360fba6 Compare July 25, 2024 00:58

antoninbas previously approved these changes Jul 25, 2024

View reviewed changes

luolanzone previously approved these changes Jul 25, 2024

View reviewed changes

tnqn reviewed Jul 25, 2024

View reviewed changes

Add document for BGPPolicy

189fb41

Signed-off-by: Hongliang Liu <[email protected]>

hongliangl dismissed stale reviews from luolanzone and antoninbas via 189fb41 July 25, 2024 15:16

hongliangl force-pushed the 20240712-bgp-document branch from 360fba6 to 189fb41 Compare July 25, 2024 15:16

antoninbas approved these changes Jul 25, 2024

View reviewed changes

antoninbas mentioned this pull request Jul 25, 2024

More uniform BGP router ID selection for IPv4 and IPv6 #6550

Closed

antoninbas merged commit 0ca0a01 into antrea-io:main Jul 25, 2024
51 of 54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add document for BGPPolicy #6524

Add document for BGPPolicy #6524

hongliangl commented Jul 12, 2024

jianjuns left a comment

antoninbas left a comment

antoninbas Jul 23, 2024

tnqn Jul 24, 2024

antoninbas Jul 24, 2024

antoninbas left a comment

antoninbas commented Jul 23, 2024

hongliangl commented Jul 23, 2024

antoninbas Jul 23, 2024

antoninbas Jul 23, 2024

antoninbas left a comment

antoninbas left a comment

luolanzone left a comment

tnqn Jul 25, 2024

hongliangl Jul 25, 2024

antoninbas Jul 25, 2024 •

edited

Loading

antoninbas Jul 25, 2024

tnqn Jul 26, 2024

antoninbas Jul 26, 2024

tnqn Jul 25, 2024

hongliangl Jul 25, 2024

tnqn left a comment

tnqn Jul 25, 2024

hongliangl Jul 25, 2024

tnqn Jul 25, 2024

hongliangl Jul 25, 2024

antoninbas left a comment

antoninbas commented Jul 25, 2024

		- All Nodes can advertise all ClusterIPs, respecting `internalTrafficPolicy`. If `internalTrafficPolicy` is set to
		`Local`, a Node will only advertise ClusterIPs with at least one local Endpoint.

		BGP authentication ensures that BGP sessions are established and maintained only with legitimate peers. Antrea supports
		BGP authentication by leveraging a Kubernetes Secret, `antrea-bgp-passwords`, to store authentication passwords.

		the password for that BGP peer. If a key for a BGP peer does not exist in the Secret, the peer is considered unconfigured
		for that one.

		1. If the `node.antrea.io/bgp-router-id` annotation is present on the Node and its value is a valid IPv4 address string,
		we will use the provided value.

Add document for BGPPolicy #6524

Add document for BGPPolicy #6524

Conversation

hongliangl commented Jul 12, 2024

jianjuns left a comment

Choose a reason for hiding this comment

antoninbas left a 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

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Jul 23, 2024

hongliangl commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas Jul 25, 2024 • edited Loading

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

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

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Jul 25, 2024

antoninbas Jul 25, 2024 •

edited

Loading