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

BGP Advertised Path Attributes fails to match CiliumLoadBalancerIPPool #32693

Closed
3 tasks done
dswaffordcw opened this issue May 23, 2024 · 0 comments · Fixed by #32694
Closed
3 tasks done

BGP Advertised Path Attributes fails to match CiliumLoadBalancerIPPool #32693

dswaffordcw opened this issue May 23, 2024 · 0 comments · Fixed by #32694
Labels
area/bgp kind/bug This is a bug in the Cilium logic. kind/community-report This was reported by a user in the Cilium community, eg via Slack. sig/agent Cilium agent related.

Comments

@dswaffordcw
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

This bug report is a follow-up from https://cilium.slack.com/archives/C53TG4J4R/p1716491937260819

I attempted to configure the Advertised Paths feature using https://docs.cilium.io/en/stable/network/bgp-control-plane/#advertised-path-attributes. Before configuring the Advertised Paths feature, I had established BGP peering and a BGP path was being announced. This was seen in:

 cilium bgp routes
(Defaulting to `available ipv4 unicast` routes, please see help for more options)

Node                              VRouter   Prefix          NextHop    Age      Attrs
bgp-cplane-dev-v4-control-plane   65001     100.64.8.2/32   10.0.1.1   41m37s   [{Origin: i} {AsPath: 65000 65002} {Nexthop: 10.0.1.1}]
bgp-cplane-dev-v4-worker          65002     100.64.8.2/32   0.0.0.0    46m52s   [{Origin: i} {Nexthop: 0.0.0.0}]

For the Advertised Paths feature, I had configured:

      advertisedPathAttributes:
      - selectorType: CiliumLoadBalancerIPPool
        selector:
          matchLabels:
            environment: production
        communities:
          standard:
          - 65001:100

With this configuration, no BGP communities were present on the announced path.

The full configuration is below:

cat bgpp.yaml

apiVersion: cilium.io/v2alpha1
kind: CiliumBGPPeeringPolicy
metadata:
  name: worker
spec:
  nodeSelector:
    matchLabels:
      kubernetes.io/hostname: bgp-cplane-dev-v4-worker
  virtualRouters:
  - localASN: 65002
    serviceSelector:
      matchExpressions:
       - { key: app, operator: In, values: [hello-node, hello-node2, hello-node3] }
    serviceAdvertisements:
      - LoadBalancerIP # <-- default
      #- ClusterIP      # <-- options
      #- ExternalIP     # <-- options
    neighbors:
    - peerASN: 65000
      peerAddress: 10.0.2.1/32
      gracefulRestart:
        enabled: true
        restartTimeSeconds: 60
      advertisedPathAttributes:
      - selectorType: CiliumLoadBalancerIPPool
        selector:
          matchLabels:
            environment: vrf-mgmt
        communities:
          standard:
          - 65001:100
---
apiVersion: "cilium.io/v2alpha1"
kind: CiliumLoadBalancerIPPool
metadata:
  name: "vrf-mgmt"
  labels:
    environment: vrf-mgmt
spec:
  blocks:
  - cidr: "100.64.8.0/22"
---

There were no route policies observed on the Cilium nodes:

$  k exec cilium-vj8nj -n kube-system -- cilium-dbg bgp route-policies
VRouter   Policy Name   Type   Match Peers   Match Prefixes (Min..Max Len)   RIB Action   Path Actions
$  k exec cilium-lj9b5 -n kube-system -- cilium-dbg bgp route-policies
VRouter   Policy Name   Type   Match Peers   Match Prefixes (Min..Max Len)   RIB Action   Path Actions

Other related information was:

$  k get ippools --show-labels
NAME                DISABLED   CONFLICTING   IPS AVAILABLE   AGE   LABELS
vrf-core-services   false      False         254             64m   <none>
vrf-customers       false      False         254             64m   <none>
vrf-mgmt            false      False         1020            64m   environment=vrf-mgmt
$  k get services --show-labels
NAME           TYPE           CLUSTER-IP     EXTERNAL-IP   PORT(S)          AGE   LABELS
hello-node     LoadBalancer   10.2.157.150   100.64.8.2    8080:32293/TCP   67m   app=hello-node
hello-node-2   LoadBalancer   10.2.88.35     100.64.8.1    8080:32414/TCP   64m   app=hello-node-2
kubernetes     ClusterIP      10.2.0.1       <none>        443/TCP          71m   component=apiserver,provider=kubernetes

In the Slack thread, Harsimran Pabla pointed out that the existing BGP reconciler references a deprecated CRD field (cidrs) for CiliumLoadBalancerIPPool. In https://github.com/cilium/cilium/blob/main/pkg/bgpv1/manager/reconciler/route_policy.go, he pointed out:

            for _, cidrBlock := range pool.Spec.Cidrs {

As a test, for the CiliumLoadBalancerIPPool I changed:

spec:
  blocks:
  - cidr: "100.64.8.0/22"

to:

spec:
  cidrs:
  - "100.64.8.0/22"

This corrected my issue! The configured BGP community, via the Advertised Paths Feature, was now present on the announcement:

$  cilium bgp routes
(Defaulting to `available ipv4 unicast` routes, please see help for more options)

Node                              VRouter   Prefix          NextHop    Age      Attrs
bgp-cplane-dev-v4-control-plane   65001     100.64.8.2/32   10.0.1.1   7s       [{Origin: i} {AsPath: 65000 65002} {Nexthop: 10.0.1.1} {Communities: 65001:100}]
bgp-cplane-dev-v4-worker          65002     100.64.8.2/32   0.0.0.0    59m12s   [{Origin: i} {Nexthop: 0.0.0.0}]
$  k exec cilium-vj8nj -n kube-system -- cilium-dbg bgp route-policies
VRouter   Policy Name                                   Type     Match Peers   Match Prefixes (Min..Max Len)   RIB Action   Path Actions
65002     10.0.2.1/32-CiliumLoadBalancerIPPool-52e...   export   10.0.2.1/32   100.64.8.0/22 (32..32)          none         AddCommunities: [65001:100]
$  k exec cilium-vj8nj -n kube-system -- cilium-dbg bgp route-policies
VRouter   Policy Name                                   Type     Match Peers   Match Prefixes (Min..Max Len)   RIB Action   Path Actions
65002     10.0.2.1/32-CiliumLoadBalancerIPPool-52e...   export   10.0.2.1/32   100.64.8.0/22 (32..32)          none         AddCommunities: [65001:100]

This bug request is being opened to capture the necessary changes to https://github.com/cilium/cilium/blob/main/pkg/bgpv1/manager/reconciler/route_policy.go to allow one to specify either the deprecated (cidrs) or current field (blocks).

Cilium Version

$  cilium version
cilium-cli: v0.16.7-40-g9316d0ac compiled with go1.22.2 on linux/amd64
cilium image (default): v1.15.5
cilium image (stable): v1.15.5
cilium image (running): 1.16.0-dev

Kernel Version

$  uname -a
Linux hostname 6.5.0-26-generic #26~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Tue Mar 12 10:22:43 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Kubernetes Version

$  k version
Client Version: version.Info{Major:"1", Minor:"20+", GitVersion:"v1.20.15-enhanced-describe-dirty", GitCommit:"ac2e2baa7d4039cc4c68f2e869e4edbe2d60b305", GitTreeState:"dirty", BuildDate:"2023-03-02T00:33:46Z", GoVersion:"go1.20.1", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"30", GitVersion:"v1.30.0", GitCommit:"7c48c2bd72b9bf5c44d21d7338cc7bea77d0ad2a", GitTreeState:"clean", BuildDate:"2024-05-13T22:00:36Z", GoVersion:"go1.22.2", Compiler:"gc", Platform:"linux/amd64"}

Regression

No response

Sysdump

cilium-sysdump-20240523-133822.zip

Relevant log output

No response

Anything else?

No response

Cilium Users Document

  • Are you a user of Cilium? Please add yourself to the Users doc

Code of Conduct

  • I agree to follow this project's Code of Conduct
@dswaffordcw dswaffordcw added kind/bug This is a bug in the Cilium logic. kind/community-report This was reported by a user in the Cilium community, eg via Slack. needs/triage This issue requires triaging to establish severity and next steps. labels May 23, 2024
@rastislavs rastislavs added area/bgp and removed needs/triage This issue requires triaging to establish severity and next steps. labels May 24, 2024
@lmb lmb added the sig/agent Cilium agent related. label May 24, 2024
dswaffordcw added a commit to dswaffordcw/cilium that referenced this issue May 24, 2024
…dr fields for CiliumLoadBalancerIPPool

In cilium@27322f3, the `CiliumLoadBalancerIPPool`'s field named `cidrs` was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides examples of configuring a `CiliumLoadBalancerIPPool` using the field named `blocks`.  While attempting to configure a BGP policy using the Advertised Path Attributes feature (https://docs.cilium.io/en/stable/network/bgp-control-plane/#advertised-path-attributes), I was unable to configure a working policy.  While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was looking only for the deprecated field.  The former name for `blocks` was `cidrs`.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.  With this update, the BGPv1 reconciler will first look for a `CiliumLoadBalancerIPPool`'s using the updated `blocks` and then continues to evaluate the deprecated field named `cidrs`.  The update includes a check to remove duplicates when the same prefix is specified using both new and old fields.
dswaffordcw added a commit to dswaffordcw/cilium that referenced this issue May 24, 2024
…dr fields for CiliumLoadBalancerIPPool

In cilium@27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature (https://docs.cilium.io/en/stable/network/bgp-control-plane/#advertised-path-attributes), I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.
dswaffordcw added a commit to dswaffordcw/cilium that referenced this issue May 24, 2024
…dr fields for CiliumLoadBalancerIPPool

In cilium@27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature (https://docs.cilium.io/en/stable/network/bgp-control-plane/#advertised-path-attributes), I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this issue May 24, 2024
…or CiliumLoadBalancerIPPool

In cilium@27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature (https://docs.cilium.io/en/stable/network/bgp-control-plane/#advertised-path-attributes), I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: cilium#32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this issue May 28, 2024
…or CiliumLoadBalancerIPPool

In cilium@27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature (https://docs.cilium.io/en/stable/network/bgp-control-plane/#advertised-path-attributes), I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: cilium#32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this issue May 29, 2024
In cilium@27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: cilium#32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this issue May 30, 2024
In cilium@27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: cilium#32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
github-merge-queue bot pushed a commit that referenced this issue May 31, 2024
In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
sayboras pushed a commit that referenced this issue Jun 4, 2024
In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
viktor-kurchenko pushed a commit that referenced this issue Jun 4, 2024
[ upstream commit 686876c ]

In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
viktor-kurchenko pushed a commit that referenced this issue Jun 4, 2024
[ upstream commit 686876c ]
[ backporter notes: added missing import for sets dependency in the route_policy.go ]

In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
viktor-kurchenko pushed a commit that referenced this issue Jun 5, 2024
[ upstream commit 686876c ]
[
  backporter notes:
  * added missing import for sets dependency in the route_policy.go
  * added wellKnownCommunity variable in the route_policy_test.go
]

In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
viktor-kurchenko added a commit that referenced this issue Jun 5, 2024
[ upstream commit 686876c ]
[
  backporter notes:
  * added missing import for sets dependency in the route_policy.go
  * added wellKnownCommunity variable in the route_policy_test.go
]

In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
viktor-kurchenko pushed a commit that referenced this issue Jun 5, 2024
[ upstream commit 686876c ]
[
  backporter notes:
  * added missing import for sets dependency in the route_policy.go
  * removed wellKnownCommunity variable in the route_policy_test.go
]

In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
nbusseneau pushed a commit that referenced this issue Jun 5, 2024
[ upstream commit 686876c ]
[
  backporter notes:
  * added missing import for sets dependency in the route_policy.go
  * removed wellKnownCommunity variable in the route_policy_test.go
]

In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
sayboras pushed a commit that referenced this issue Jun 10, 2024
[ upstream commit 686876c ]
[
  backporter notes:
  * added missing import for sets dependency in the route_policy.go
  * removed wellKnownCommunity variable in the route_policy_test.go
]

In 27322f3, the CiliumLoadBalancerIPPool's field named "cidrs" was deprecated.  The documentation on https://docs.cilium.io/en/stable/network/lb-ipam/ provides an example of configuring a CiliumLoadBalancerIPPool using the field named "blocks".  While testing a BGP policy configured with the Advertised Path Attributes feature, I was not able to achieve the desired policy.  BGP attributes configured were not being applied.

While discussing this in Cilium's Slack channel, it was pointed out that the BGPv1 reconciler was only aware of the deprecated field.

This commit updates Cilium's BGPv1 reconciler to support both the deprecated and updated fields.

Fixes: #32693

Signed-off-by: David Swafford <dswafford@coreweave.com>
Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
(cherry picked from commit 66f5c93)
Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bgp kind/bug This is a bug in the Cilium logic. kind/community-report This was reported by a user in the Cilium community, eg via Slack. sig/agent Cilium agent related.
Projects
None yet
3 participants