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

bgpv1: Adds CiliumPodIPPool Support to PathAttr Policies #28310

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

danehans
Copy link
Contributor

  • Adds the CiliumPodIPPool selector type to BGP CP CiliumBGPPathAttributes API.
  • Updates RoutePolicyReconciler to support CiliumPodIPPool.
  • Added unit tests for the CiliumPodIPPool selector type.
  • Regenerated CiliumBGPPeeringPolicy CRD.

Fixes: #28296

Adds the CiliumPodIPPool selector type to BGP CP AdvertisedPathAttributes to match CiliumPodIPPool custom resources. Path attributes apply to routes announced for selected CiliumPodIPPools.

@danehans danehans requested review from a team as code owners September 27, 2023 19:05
@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 Sep 27, 2023
@danehans danehans marked this pull request as draft September 27, 2023 19:05
@danehans
Copy link
Contributor Author

@rastislavs I marked this PR as a draft b/c I am not seeing the path attributes being added for prefixes being advertised from a CiliumPodIPPool. I temporarily added logging throughout for troubleshooting but I'm unable to root cause the issue. When you have time, can you take a look?

Test resources:

apiVersion: cilium.io/v2alpha1
kind: CiliumPodIPPool
metadata:
  creationTimestamp: "2023-09-26T21:30:52Z"
  generation: 1
  labels:
    foo: bar
  name: default
  resourceVersion: "1757"
  uid: 8c64dc70-e811-4108-b01d-f3bfa88c3a99
spec:
  ipv4:
    cidrs:
    - 10.10.0.0/16
    maskSize: 24
---
apiVersion: cilium.io/v2alpha1
kind: CiliumBGPPeeringPolicy
metadata:
  name: cp
spec:
  nodeSelector:
    matchLabels:
      kubernetes.io/hostname: kind-control-plane
  virtualRouters:
  - exportPodCIDR: false
    localASN: 65100
    neighbors:
    - advertisedPathAttributes:
      - communities:
          standard:
          - 65001:100
        selector:
          matchLabels:
            foo: bar
        selectorType: CiliumPodIPPool
      connectRetryTimeSeconds: 120
      eBGPMultihopTTL: 1
      holdTimeSeconds: 90
      keepAliveTimeSeconds: 30
      peerASN: 65200
      peerAddress: 192.168.64.3/32
      peerPort: 179
    podIPPoolSelector:
      matchLabels:
        foo: bar

Logs:
cp_not_working.log
cp_working.log

@rastislavs
Copy link
Contributor

@rastislavs I marked this PR as a draft b/c I am not seeing the path attributes being added for prefixes being advertised from a CiliumPodIPPool.

Thanks for the PR! I think the reason why this is not working ATM is in this comment: https://github.com/cilium/cilium/pull/28310/files#r1339661669

FYI, I plan to add a CLI command for listing the routing policies, so that similar issues are easier to debug in the future.

@rastislavs rastislavs added area/bgp kind/feature This introduces new functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Sep 28, 2023
@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 Sep 28, 2023
@danehans danehans marked this pull request as ready for review September 28, 2023 16:33
@danehans
Copy link
Contributor Author

cc: @harsimran-pabla @YutaroHayakawa

Copy link
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks!

@rastislavs
Copy link
Contributor

/test

@danehans
Copy link
Contributor Author

danehans commented Oct 2, 2023

/cc @harsimran-pabla @YutaroHayakawa

1 similar comment
@danehans
Copy link
Contributor Author

danehans commented Oct 3, 2023

/cc @harsimran-pabla @YutaroHayakawa

@danehans
Copy link
Contributor Author

danehans commented Oct 4, 2023

@harsimran-pabla when you have a moment, do you mind doing another review?

@rastislavs
Copy link
Contributor

/test

@danehans
Copy link
Contributor Author

/ci-l4lb

@danehans
Copy link
Contributor Author

Commit 301fa26 is a rebase to see if it fixes the ci-l4lb test failures.

@danehans
Copy link
Contributor Author

/test

@danehans danehans self-assigned this Oct 18, 2023
@danehans
Copy link
Contributor Author

@rastislavs @harsimran-pabla @YutaroHayakawa This PR is passing CI. Do you have suggestions on how to get this merged?

@YutaroHayakawa
Copy link
Member

Seems like we still need a review from @youngnick. Once his review is in, you can put the tag ready-to-merge then the tophat will merge it.

@danehans
Copy link
Contributor Author

@youngnick This PR has been hanging around for a few weeks. Do you mind reviewing when you have a moment or can you assign another reviewer?

@danehans
Copy link
Contributor Author

@christarazi anyone else from sig-k8s you recommend to unblock this PR?

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

The K8s changes LGTM to me. Could you add justification in the commit msg? Without it, we lose potentially important context in git history if later down the line we bisect this commit.

@danehans
Copy link
Contributor Author

danehans commented Nov 6, 2023

@christarazi thanks for the review. I have updated the commit message to include additional change details and the fixed issue.

@rastislavs
Copy link
Contributor

/test

- Adds the CiliumPodIPPool selector type to BGP CP CiliumBGPPathAttributes API.
- Updates RoutePolicyReconciler to support CiliumPodIPPool.
- Added unit tests for the CiliumPodIPPool selector type.
- Regenerated CiliumBGPPeeringPolicy CRD.

Fixes: cilium#28296

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@danehans
Copy link
Contributor Author

Commit 60f55e2 rebases to fix CI flakes.

@rastislavs
Copy link
Contributor

/test

@rastislavs rastislavs added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 14, 2023
@julianwiedmann julianwiedmann merged commit 6dbb653 into cilium:main Nov 15, 2023
61 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Nov 15, 2023
@danehans danehans deleted the issue_28296 branch March 8, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bgp kind/feature This introduces new functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bgpv1: Add CiliumPodIPPool Selector Type
6 participants