Skip to content

bgpv1: Adds CiliumPodIPPool Support to PathAttr Policies #28310

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 15, 2023
Merged

bgpv1: Adds CiliumPodIPPool Support to PathAttr Policies #28310

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 rastislavs mentioned this pull request Sep 28, 2023
Closed
rastislavs
rastislavs reviewed Sep 28, 2023
View reviewed changes
@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 Impacts the Border Gateway Protocol feature. 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
rastislavs
rastislavs reviewed Sep 29, 2023
View reviewed changes
@danehans danehans requested a review from rastislavs September 29, 2023 15:09
@danehans
Copy link
Contributor Author

cc: @harsimran-pabla @YutaroHayakawa

rastislavs
rastislavs approved these changes Oct 2, 2023
View reviewed changes
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 •
edited
Loading

/cc @harsimran-pabla @YutaroHayakawa

1 similar comment
@danehans
Copy link
Contributor Author

danehans commented Oct 3, 2023

/cc @harsimran-pabla @YutaroHayakawa

harsimran-pabla
harsimran-pabla reviewed Oct 3, 2023
View reviewed changes
@danehans
Copy link
Contributor Author

danehans commented Oct 4, 2023

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

harsimran-pabla
harsimran-pabla reviewed Oct 4, 2023
View reviewed changes
@rastislavs rastislavs mentioned this pull request Oct 10, 2023
Merged
@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 danehans requested a review from christarazi October 27, 2023 22:31
@danehans
Copy link
Contributor Author

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

christarazi
christarazi reviewed Nov 6, 2023
View reviewed changes
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.

@danehans danehans requested a review from christarazi November 6, 2023 22:21
@rastislavs
Copy link
Contributor

/test

@danehans
bgpv1: Adds CiliumPodIPPool Support to PathAttr Policies
60f55e2 
- 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
@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
@rastislavs rastislavs mentioned this pull request Nov 15, 2023
Merged
@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
Reviewers

@rastislavs rastislavs rastislavs approved these changes

@christarazi christarazi christarazi approved these changes

@harsimran-pabla harsimran-pabla harsimran-pabla approved these changes

@youngnick youngnick Awaiting requested review from youngnick youngnick is a code owner automatically assigned from cilium/sig-k8s

Assignees

@danehans danehans

Labels
area/bgp Impacts the Border Gateway Protocol feature. kind/feature This introduces new functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bgpv1: Add CiliumPodIPPool Selector Type
6 participants
@danehans @rastislavs @YutaroHayakawa @christarazi @harsimran-pabla @julianwiedmann