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

add vpc support for capl clusters #159

Merged
merged 10 commits into from
Apr 25, 2024
Merged

add vpc support for capl clusters #159

merged 10 commits into from
Apr 25, 2024

Conversation

rahulait
Copy link
Contributor

@rahulait rahulait commented Mar 1, 2024

What type of PR is this?
/kind feature

What this PR does / why we need it:
With this change, k8s clusters are provisioned by default within a VPC.

  1. Since nodebalancers don't yet work with VPC subnets, we attach two interfaces to a linode:

    • eth0 for public and private nodebalancer traffic
    • eth1 for VPC pod-to-pod traffic

    We have to keep pub+priv on eth0 because we want cloud-init to work within VPC. If eth0 is on VPC, then cloud-init fails unless we explicitly add route to make it go over eth1. Once nodebalancer and cloud-init works natively with VPC, we'll use just one interface.

  2. We have cilium installed and configured with native routing (no vxlan). All pod-to-pod traffic goes over eth1 within the VPC.

root@rah6-control-plane-g8zh6:~# ip a | grep vxlan
root@rah6-control-plane-g8zh6:~#
  1. Each node in a k8s cluster gets assigned a podCIDR. Linode ccm running in cluster with route-controller feature enabled automatically updates the routes within VPC so that pod-to-pod traffic can flow over the VPC subnet. This PR depends on that feature: see feat: add route-controller to linode ccm linode-cloud-controller-manager#184

One should be able to ping to pod ip's from any linode within the VPC.

To check if routes are added to interface or not, one can use curl as well:

curl --header 'Authorization: Bearer $LINODE_API_TOKEN' -X GET https://api.linode.com/v4/linode/instances/55519597/configs | jq .data[0].interfaces[].ip_ranges

The range returned in the output should match with the range present in node's spec k get node <nodename> -o yaml | yq .spec.podCIDRs

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:

TODOs:

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

@rahulait rahulait marked this pull request as ready for review March 2, 2024 06:57
@avestuk
Copy link
Contributor

avestuk commented Mar 4, 2024

What happens to NodePort services in this model? Are they bound on eth0 still?

@rahulait
Copy link
Contributor Author

rahulait commented Mar 5, 2024

What happens to NodePort services in this model? Are they bound on eth0 still?

Request for service exposed as NodePort can come on any ip-address (public, private or vpc address). So they are bound to all interfaces on the host. However, we are going to configure cilium-node to have --direct-routing-device=eth1 (link) so that when cilium-node is forwarding the traffic to the actual pod, it uses eth1's ip-address when doing SNAT.
If we don't specify this, then it modifies the incoming request with src as the node which received it and dst as pod ip. It sets src as eth0's public ip and dst as pod ip (10/10 subnet) which gets sent over eth1. Since eth1 is connected to VPC, it only allows 10/8 subnet traffic(both src and dst) and hence drops the packet. By setting --direct-routing-device=eth1, cilium uses eth1's ip as src ip and now the packet can route over eth1. I am going to push that fix in this PR.

@AshleyDumaine AshleyDumaine added feature New feature or request go Pull requests that update Go code labels Mar 6, 2024
@rahulait rahulait force-pushed the vpc-support branch 5 times, most recently from a359922 to c8c90be Compare March 11, 2024 15:09
# ipMasqAgent:
# enabled: true
# bpf:
# masquerade: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not using bpf by default as I want to get VPC working first. Once the PR is merged, we can have a minor PR to switch from iptables based masquerading to bpf based masquerading.

@rahulait rahulait force-pushed the vpc-support branch 2 times, most recently from 919f64d to 686044d Compare March 13, 2024 20:58
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 60.60606% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 54.21%. Comparing base (3ff0933) to head (fa6fc01).

Files Patch % Lines
controller/linodemachine_controller.go 47.05% 6 Missing and 3 partials ⚠️
controller/linodemachine_controller_helpers.go 75.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   54.42%   54.21%   -0.21%     
==========================================
  Files          27       27              
  Lines        1560     1577      +17     
==========================================
+ Hits          849      855       +6     
- Misses        663      671       +8     
- Partials       48       51       +3     

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

@rahulait rahulait force-pushed the vpc-support branch 2 times, most recently from 03c6152 to ae1bf19 Compare March 14, 2024 21:26
@eljohnson92
Copy link
Collaborator

@rahulait given our default will now leverage VPC+direct routing, can we create a new flavor that includes VPCless deployments so we still have documented support for DCs without VPC support yet?

controller/linodemachine_controller_helpers.go Outdated Show resolved Hide resolved
controller/linodemachine_controller_helpers.go Outdated Show resolved Hide resolved
controller/linodemachine_controller.go Outdated Show resolved Hide resolved
controller/linodemachine_controller_helpers.go Outdated Show resolved Hide resolved
controller/linodemachine_controller_helpers.go Outdated Show resolved Hide resolved
@rahulait
Copy link
Contributor Author

rahulait commented Mar 19, 2024

@rahulait given our default will now leverage VPC+direct routing, can we create a new flavor that includes VPCless deployments so we still have documented support for DCs without VPC support yet?

Sure, that might need a bit more change. Based on our discussions, my understanding was that everything will be there within VPCs and we won't support VPCless. We need metadata service as well and its not available in all datacenters. Let me see how much change it would be and if we can also add support for VPCless deployments.

@rahulait
Copy link
Contributor Author

CCM e2e tests were failing when run against VPC. We need the PR linode/linode-cloud-controller-manager#182 to be merged as well. Without this fix, it doesn't break cluster installs but breaks exposing services of type loadbalancer.

@rahulait rahulait force-pushed the vpc-support branch 4 times, most recently from 80f49b1 to ff1b7c6 Compare April 17, 2024 06:26
cbzzz
cbzzz previously approved these changes Apr 23, 2024
controller/linodemachine_controller.go Outdated Show resolved Hide resolved
@rahulait
Copy link
Contributor Author

Linode CCM tests pass when running within VPC and route-controller enabled

[AfterSuite] PASSED [0.028 seconds]
------------------------------

Ran 22 of 23 Specs in 495.701 seconds
SUCCESS! -- 22 Passed | 0 Failed | 0 Pending | 1 Skipped
PASS

Ginkgo ran 1 suite in 8m18.534643696s
Test Suite Passed

@eljohnson92
Copy link
Collaborator

I was able to test that this successfully creates a cluster and all pods come up within the VPC along with nodes showing the correct internal IP. From my perspective this PR looks good. I don't necessarily want to hold this PR up on a doc update, so can we get a follow up PR to add a VPC topic describing how we are using VPCs by default and what the different configurations on the LinodeCluster/LinodeVPC look like?

@rahulait
Copy link
Contributor Author

Going to merge it in and open doc and VPCLess PR soon.

@rahulait rahulait merged commit 1a10bbb into main Apr 25, 2024
9 checks passed
@rahulait rahulait deleted the vpc-support branch April 29, 2024 19:17
amold1 pushed a commit that referenced this pull request May 17, 2024
* add vpc support for capl clusters

* disable kube-proxy, address review comments

* use updated version of linode-ccm

* address review comments

* don't use /etc/hosts for node-ip

* add additional interface using machinetemplate

* rebase fix and address comments

* reduce cognitive complexity, update tests and use ccm v0.4.3 for updated helm chart

* update linode-ccm to v0.4.4

* address review comments, add vpc note

---------

Co-authored-by: Rahul Sharma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants