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

[feat] [deps]: New flavor for Cilium BGP load-balancing for Services, bump linode-CCM version #317

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

AshleyDumaine
Copy link
Member

@AshleyDumaine AshleyDumaine commented May 13, 2024

What type of PR is this?
/kind feature

What this PR does / why we need it: New flavor for using the linode-CCM without creating NodeBalancers to perform Service load-balancing.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer: Requires linode/linode-cloud-controller-manager#208 be merged and a new release version to be cut and published for the Helm Chart

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Testing

Deploy a LoadBalancer service (no need to set the loadBalancerClass), and check the following:

  • BGP nodes exist (2 by default in the MachineDeployment)
  • The BGP nodes are labeled with cilium-bgp-peering=true and node.k8s.linode.com/ip-sharing-updated=true once an LB Service is created
  • if you scale up the bgp MachineDeployment (k edit md $CLUSTER_NAME-md-bgp), the new Node eventually joins and gets the cilium-bgp-peering=true and node.k8s.linode.com/ip-sharing-updated=true labels. Looking at Cloud Manager, you should see the new Linode has the shared IP for the LB Service added in the "Networking" tab.

Example LB Service + CNP if you have your Cilium in enforcement mode instead of audit for the host firewall:

apiVersion: v1
kind: Namespace
metadata:
  name: test-ns
---
apiVersion: cilium.io/v2
kind: CiliumNetworkPolicy
metadata:
  name: default-external-policy-svc
  namespace: test-ns
spec:
  description: service traffic
  endpointSelector: {}
  ingress:
  - fromEntities:
    - world
    toPorts:
    - ports:
      - port: "8080" 
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: hello-world
  namespace: test-ns
  labels:
    app: hello-world
spec:
  replicas: 1
  selector:
    matchLabels:
      app: hello-world
  template:
    metadata:
      labels:
        app: hello-world
    spec:
      containers:
      - image: gcr.io/google-samples/node-hello:1.0
        name: hello-world
        ports:
        - containerPort: 8080
---
apiVersion: v1
kind: Service
metadata:
  name: nbless-svc
  namespace: test-ns
spec:
  type: LoadBalancer
  selector:
    app: hello-world
  ports:
    - name: http
      protocol: TCP
      port: 8080
      targetPort: 8080

@AshleyDumaine AshleyDumaine added the feature New feature or request label May 13, 2024
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.13%. Comparing base (fd82259) to head (65a53d0).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #317   +/-   ##
=======================================
  Coverage   66.13%   66.13%           
=======================================
  Files          35       35           
  Lines        2179     2179           
=======================================
  Hits         1441     1441           
  Misses        676      676           
  Partials       62       62           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AshleyDumaine AshleyDumaine force-pushed the cilium-lb branch 2 times, most recently from 85a3cff to 1b21e84 Compare May 14, 2024 14:24
@AshleyDumaine AshleyDumaine force-pushed the cilium-lb branch 4 times, most recently from 78e00cf to 29812d2 Compare May 20, 2024 19:01
@AshleyDumaine AshleyDumaine changed the title New flavor for Cilium BGP load-balancing for Services [feat]: New flavor for Cilium BGP load-balancing for Services May 24, 2024
@AshleyDumaine AshleyDumaine removed the feature New feature or request label May 24, 2024

| Control Plane | CNI | Default OS | Installs ClusterClass | IPv4 | IPv6 |
|---------------|--------|--------------|-----------------------|------|------|
| Kubeadm | Cilium | Ubuntu 22.04 | No | Yes | No |
Copy link
Member Author

Choose a reason for hiding this comment

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

technically this flavor does enable ipv6 but that's specifically because with cilium enable-host-firewall, ipv6 must be enabled for the bgp sessions to become established, otherwise they stay stuck in active and the LB Services remain unreachable even in the absence of any CCNPs. Might be related to cilium/cilium#27484

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we reflect this in the doc since we are enabling dual stack now anyways?

@AshleyDumaine AshleyDumaine marked this pull request as ready for review June 3, 2024 16:55
@AshleyDumaine AshleyDumaine changed the title [feat]: New flavor for Cilium BGP load-balancing for Services [feat]: New flavor for Cilium BGP load-balancing for Services, bump linode-CCM version Jun 3, 2024
@AshleyDumaine AshleyDumaine changed the title [feat]: New flavor for Cilium BGP load-balancing for Services, bump linode-CCM version [feat] [deps]: New flavor for Cilium BGP load-balancing for Services, bump linode-CCM version Jun 3, 2024
@AshleyDumaine AshleyDumaine added the dependencies Pull requests that update a dependency file label Jun 3, 2024
@AshleyDumaine AshleyDumaine force-pushed the cilium-lb branch 3 times, most recently from 398d7e5 to ea11633 Compare June 5, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants