Add BGPPolicy API #6009

hongliangl · 2024-02-21T03:09:10Z

No description provided.

pkg/apis/crd/v1alpha2/types.go

build/charts/antrea/crds/bgppolicy.yaml

antoninbas

Added comments on other parts of the API

pkg/apis/crd/v1alpha2/types.go

antoninbas · 2024-03-22T18:08:10Z

pkg/apis/crd/v1alpha2/types.go

+	ASN int32 `json:"asn"`
+
+	// AuthSecretRef is a reference to a Secret storing the authentication password of the external BGP peer.
+	AuthSecretRef *string `json:"authSecretRef,omitempty"`


How does it work exactly? In which Namespace are the secrets defined? I assume they need to be created in the same Namespace as Antrea. Does that mean Antrea will need permissions to read all secrets in the kube-system Namespace (I don't think this is the case today).

This is a thoughtful question. I found the corresponding implementation in other CNIs:

Users assign a Secret reference in a CR, then create the corresponding Serect to store the password. To read the password, the CRD controller needs the get permission to read the corresponding Secret in the kube-system Namespace.

Uses set the base64 coded password in an annotation.

I think we could let users set the based64 coded password in a BGPPolicy CR directly. Though it is not a good practice, it is easy for users to use.

If users want to secure their BGP passwords, they could add a password for a BGP peer to the Serect named antrea-bgp-passwords in the kube-system Namespace (assuming that Antrea is installed in the Namespace).

The secret might be like this:

apiVersion: v1 kind: Secret metadata: name: antrea-bgp-passwords namespace: kube-system type: Opaque data: 192.168.1.1-65512: dmVyeS1zZWNyZXQ= 192.168.1.2-65513: dmVyeS1zZWNyZXQ=

Antrea needs the permission get, maybe watch (to watch the update of the secret)

- apiGroups: - "" resources: - secrets resourceNames: - antrea-bgp-passwords verbs: - get - watch

I think we should just support the Secret-based method.
The idea of having a single Secret, with a well-known name, mapping BGP peer IDs to passwords, is fine by me. We can also provide a configuration parameter for users to change the Secret name if they want to.

pkg/apis/crd/v1alpha2/types.go

build/yamls/antrea-ipsec.yml

rajnkamr · 2024-04-12T05:18:09Z

build/charts/antrea/conf/antrea-agent.conf

@@ -88,6 +88,9 @@ featureGates:
 # Enable L7FlowExporter on Pods and Namespaces to export the application layer flows such as HTTP flows.
 {{- include "featureGate" (dict "featureGates" .Values.featureGates "name" "L7FlowExporter" "default" false) }}

+# Allow users to advertise Service IPs, Pod IPs, and Egress IPs to external BGP peers.


I think with Pod IPs you meant Pod CIDR here, better to change it to pod CIDR instead

hongliangl · 2024-06-12T03:18:49Z

pkg/apis/crd/v1alpha1/types.go

+	BGPPeers []BGPPeer `json:"bgpPeers,omitempty"`
+}
+
+type Advertisements struct {


The updated Advertisements

pkg/apis/crd/v1alpha1/types.go

tnqn · 2024-06-12T15:35:11Z

pkg/apis/crd/v1alpha1/types.go

+	NodeSelector *metav1.LabelSelector `json:"nodeSelector"`
+
+	// LocalASN is the AS number used by the BGP process.
+	LocalASN *int32 `json:"localASN,omitempty"`


Does it have a default value? If not, it shouldn't be a pointer.

IMO, it should be a pointer as long as it is optional, regardless of whether or not there is a non-zero default value. It's easier to stick to that rule.

I assumed it's not optional, there should always be a ASN?

I see your point now.
Yes, there should always be an ASN, but the question is does it make sense to default the ASN to 64512 or should we require users to provide a value every time? @hongliangl if 2 K8s clusters are peering to the same local router, I assume it may create a conflict ? And would there be some conflict if 2 different BGPPolicies in the same cluster (applied to a different subset of Nodes) use the same localASN?

Thinking about this some more, I feel like logically it is perfectly valid to use the same ASN across different BGPPolicies and even different K8s clusters, while peering to the same router. I'll let @hongliangl comment but I think the current API version works well.

Edit: if one wants to use eBGP to provide Pod-to-Pod connectivity between 2 K8s cluster connected to the same BGP router, then maybe using different localASNs will be required?

if 2 K8s clusters are peering to the same local router, I assume it may create a conflict ?

As long as the Node IPs of these 2 k8s clusters are not the same, it's okay for the local router regardless of the value of the ASN. Ideally, each k8s cluster should be treated as an AS, given its fully meshed network implemented by CNI, and the cluster should be assigned a unique ASN. If two k8s clusters use the same ASN, but the network between them is not fully reachable, for the local router, there might be potential BGP issues (I'm not sure what it might be)

And would there be some conflict if 2 different BGPPolicies in the same cluster (applied to a different subset of Nodes) use the same localASN?

In a k8s cluster, all BGPPolicies should use the same ASN as the description above, but it's okay that every Node has its unique ASN.

Edit: if one wants to use eBGP to provide Pod-to-Pod connectivity between 2 K8s cluster connected to the same BGP router, then maybe using different localASNs will be required?

It should work using the same localASN, but it is a little weird. Normally, the traffic within an AS (the source and destination are in the same AS) should only be forwarded in this AS.

As long as the Node IPs of these 2 k8s clusters are not the same

Is this a valid case anyway? I guess it is with different VRFs

We are getting into advanced use cases. I do agree that the recommendation should be that 2 clusters connected to the same BGP router should use different localASNs, and we can include that in the documentation.

But to go back to @tnqn's original point, maybe we should err on the side of caution and require users to explicitly provide a private ASN? Then we would ensure that users are well-aware of this parameter and configure it appropriately for their use case. It seems like a very small inconvenience, especially considering the fact that users may need to also configure the uplink router to authorize peering for this ASN (I guess that depends on the uplink router's configuration).
How do you feel about making this a required parameter?

But to go back to @tnqn's original point, maybe we should err on the side of caution and require users to explicitly provide a private ASN? Then we would ensure that users are well-aware of this parameter and configure it appropriately for their use case. It seems like a very small inconvenience, especially considering the fact that users may need to also configure the uplink router to authorize peering for this ASN (I guess that depends on the uplink router's configuration).

That makes sense to me. Anyway, when users configure the uplink router, they need to set the BGP peers by specifying peer IPs and ASNs. Maybe it's better for users to set the localASN since it is also needed by the configuration of the uplink router.

As long as the Node IPs of these 2 k8s clusters are not the same

Is this a valid case anyway? I guess it is with different VRFs

If I understand correctly, for different VRFs in multiple k8s clusters, and the Nodes in these clusters connect to the same uplink router, the IPs of the Node should not overlap, and then each Node can establish a BGP session with the uplink router. Did I miss some background information on your question?

pkg/apis/crd/v1alpha1/types.go

tnqn · 2024-06-12T15:40:30Z

build/charts/antrea/crds/bgppolicy.yaml

+                  format: int32
+                  minimum: 64512
+                  maximum: 65535
+                  default: 64512


I'm not familiar with this, just want to confirm if it's typical to assign a default local ASN and the default number works in most cases?

For a K8s cluster, the network is fully reachable and implemented by CNI, and the cluster could be treated as an AS (for traditional network, the network is implemented by IGP protocols like OSPF, EIGRP, etc.).

For multiple K8s clusters, at least the network of each cluster is fully reachable. Ideally, every cluster should be treated as an AS, but it's okay for these clusters to use the same ASN when peering to the same router as long as they don't have overlapped Node IPs.

For a K8s cluster, it is also okay that different Nodes use different ASNs to peer to the same router.

build/yamls/antrea-aks.yml

build/charts/antrea/crds/bgppolicy.yaml

pkg/apis/crd/v1alpha1/types.go

tnqn · 2024-06-13T13:29:48Z

pkg/apis/crd/v1alpha1/types.go

+	ClusterIPs *bool `json:"clusterIPs,omitempty"`
+
+	// Advertise ExternalIPs of the selected Services (currently all Services without Service selector) if this is set to
+	// true. Otherwise, no ExternalIPs will be advertised.
+	ExternalIPs *bool `json:"externalIPs,omitempty"`
+
+	// Advertise LoadBalancerIPs of the selected Services (currently all Services without Service selector) if this is set to
+	// true. Otherwise, no LoadBalancerIPs will be advertised.
+	LoadBalancerIPs *bool `json:"loadBalancerIPs,omitempty"`


Any reason why these booleans are pointers?

See #6009 (comment)

hongliangl · 2024-06-17T11:31:53Z

pkg/apis/crd/v1alpha1/types.go

+}
+
+// BGPPolicySpec defines the specification for a BGPPolicy.
+type BGPPolicySpec struct {


I have updated some comments of the spec. I would appreciate it if you could help review them for appropriateness and ease of understanding. @antoninbas @tnqn @luolanzone

tnqn

LGTM

antoninbas · 2024-06-17T16:55:42Z

build/charts/antrea/crds/bgppolicy.yaml

+              required:
+                - nodeSelector


I think localASN should be added to the required properties after the recent changes?

I forgot adding this. Will add it. Thanks for pointing out it.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

antoninbas · 2024-06-18T05:28:00Z

/skip-all

antoninbas · 2024-06-18T05:30:50Z

@hongliangl please remember to update docs/api.md in a future PR

hongliangl · 2024-06-18T06:34:42Z

@hongliangl please remember to update docs/api.md in a future PR

Done with #6457

hongliangl added this to the Antrea v2.0 release milestone Feb 21, 2024

hongliangl added area/api Issues or PRs related to an API. action/release-note Indicates a PR that should be included in release notes. area/component/agent Issues or PRs related to the agent component kind/feature Categorizes issue or PR as related to a new feature. labels Feb 21, 2024

hongliangl force-pushed the 20240221-bgp-api branch 2 times, most recently from dae2835 to 6c5b0d7 Compare February 22, 2024 06:22

hongliangl force-pushed the 20240221-bgp-api branch from 6c5b0d7 to 5f60191 Compare March 12, 2024 09:10

hongliangl marked this pull request as ready for review March 12, 2024 09:11

hongliangl requested review from tnqn, antoninbas and jianjuns March 12, 2024 11:32

hongliangl force-pushed the 20240221-bgp-api branch from 5f60191 to 9041f5a Compare March 12, 2024 11:41

antoninbas reviewed Mar 12, 2024

View reviewed changes

pkg/apis/crd/v1alpha2/types.go Outdated Show resolved Hide resolved

pkg/apis/crd/v1alpha2/types.go Outdated Show resolved Hide resolved

pkg/apis/crd/v1alpha2/types.go Outdated Show resolved Hide resolved

jianjuns reviewed Mar 12, 2024

View reviewed changes

pkg/apis/crd/v1alpha2/types.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240221-bgp-api branch 2 times, most recently from 81b15cb to b75d4e4 Compare March 19, 2024 02:19

rajnkamr reviewed Mar 21, 2024

View reviewed changes

build/charts/antrea/crds/bgppolicy.yaml Show resolved Hide resolved

antoninbas reviewed Mar 22, 2024

View reviewed changes

hongliangl force-pushed the 20240221-bgp-api branch from b75d4e4 to c279227 Compare March 26, 2024 03:38

hongliangl requested review from antoninbas, jianjuns and rajnkamr March 27, 2024 11:21

hongliangl force-pushed the 20240221-bgp-api branch from c279227 to 64ad4e6 Compare March 27, 2024 11:23

hongliangl force-pushed the 20240221-bgp-api branch 2 times, most recently from dbe84c8 to 89dbe7e Compare April 10, 2024 07:18

rajnkamr added the area/transit/bgp Issues or PRs related to BGP support. label Apr 11, 2024

antoninbas reviewed Apr 11, 2024

View reviewed changes

pkg/apis/crd/v1alpha2/types.go Outdated Show resolved Hide resolved

build/yamls/antrea-ipsec.yml Outdated Show resolved Hide resolved

rajnkamr reviewed Apr 12, 2024

View reviewed changes

hongliangl force-pushed the 20240221-bgp-api branch from 89dbe7e to 30290bc Compare April 12, 2024 06:21

hongliangl force-pushed the 20240221-bgp-api branch 3 times, most recently from db865f7 to f90e052 Compare June 12, 2024 03:16

hongliangl commented Jun 12, 2024

View reviewed changes

tnqn reviewed Jun 12, 2024

View reviewed changes

antoninbas reviewed Jun 12, 2024

View reviewed changes

pkg/apis/crd/v1alpha1/types.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20240221-bgp-api branch 2 times, most recently from ccc4e92 to 46462cd Compare June 13, 2024 06:08

tnqn reviewed Jun 13, 2024

View reviewed changes

hongliangl force-pushed the 20240221-bgp-api branch 4 times, most recently from fac6a0c to 6c9d01e Compare June 17, 2024 11:26

hongliangl commented Jun 17, 2024

View reviewed changes

tnqn previously approved these changes Jun 17, 2024

View reviewed changes

antoninbas previously approved these changes Jun 17, 2024

View reviewed changes

antoninbas reviewed Jun 17, 2024

View reviewed changes

Add BGPPolicy API

2cb6c19

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl dismissed stale reviews from antoninbas and tnqn via 2cb6c19 June 18, 2024 03:16

hongliangl force-pushed the 20240221-bgp-api branch from 6c9d01e to 2cb6c19 Compare June 18, 2024 03:16

antoninbas approved these changes Jun 18, 2024

View reviewed changes

antoninbas merged commit 28b38ad into antrea-io:main Jun 18, 2024
48 of 54 checks passed

hongliangl deleted the 20240221-bgp-api branch June 18, 2024 06:26

tnqn mentioned this pull request Jun 20, 2024

Advertising Antrea Egress IP via BGP with FRR-k8s #6467

Closed

tnqn mentioned this pull request Jul 26, 2024

Add BGP support to the Antrea Agent #5948

Closed

antoninbas mentioned this pull request Aug 23, 2024

Add packetcapture api #6257

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BGPPolicy API #6009

Add BGPPolicy API #6009

hongliangl commented Feb 21, 2024

antoninbas left a comment

antoninbas Mar 22, 2024

hongliangl Mar 26, 2024

antoninbas Mar 26, 2024

rajnkamr Apr 12, 2024

hongliangl Jun 12, 2024

tnqn Jun 12, 2024

antoninbas Jun 12, 2024

tnqn Jun 12, 2024

antoninbas Jun 12, 2024

antoninbas Jun 12, 2024 •

edited

Loading

hongliangl Jun 12, 2024 •

edited

Loading

hongliangl Jun 12, 2024

antoninbas Jun 12, 2024

hongliangl Jun 13, 2024

hongliangl Jun 13, 2024

tnqn Jun 12, 2024

hongliangl Jun 12, 2024

hongliangl Jun 12, 2024

tnqn Jun 13, 2024

hongliangl Jun 13, 2024

hongliangl Jun 17, 2024

tnqn left a comment

antoninbas Jun 17, 2024

hongliangl Jun 18, 2024

antoninbas commented Jun 18, 2024

antoninbas commented Jun 18, 2024

hongliangl commented Jun 18, 2024

Add BGPPolicy API #6009

Add BGPPolicy API #6009

Conversation

hongliangl commented Feb 21, 2024

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

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

antoninbas Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

hongliangl Jun 12, 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

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

antoninbas commented Jun 18, 2024

antoninbas commented Jun 18, 2024

hongliangl commented Jun 18, 2024

antoninbas Jun 12, 2024 •

edited

Loading

hongliangl Jun 12, 2024 •

edited

Loading