Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Add Node IPAM support for vSphere #3899

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

christianang
Copy link
Contributor

@christianang christianang commented Nov 11, 2022

What this PR does / why we need it

This PR adds support for Node IPAM for vSphere. Node IPAM allows a user to take advantage of this upstream CAPI proposal: https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20220125-ipam-integration.md. tl;dr a user will be able to use an IPAM provider to provide control plane and worker nodes with static ips instead of using DHCP for ip address management.

The PR will add a feature flag that will deploy a default In Cluster IPAM provider to the management cluster. The InCluster IPAM provider will allocate IPs from a given subnet.

tanzu config set features.management-cluster.deploy-in-cluster-ipam-provider-beta true

A user will need to apply an IP Pool resource to the management cluster:

---
apiVersion: ipam.cluster.x-k8s.io/v1alpha1
kind: InClusterIPPool
metadata:
  name: inclusterippool
  namespace: cluster-ns # This will need to match where the workload cluster is deployed
spec:
  subnet: 10.10.100.1/24
  gateway: 10.10.100.1
  start: 10.10.100.100
  end: 10.10.100.200

When a user deploys a workload cluster using ClusterClass they can specify either:

  • Using legacy variables:
NODE_IPAM_IP_POOL_NAME: inclusterippool # or whatever pool name you are using above
  • ClusterClass
apiVersion: cluster.x-k8s.io/v1beta1
kind: Cluster
spec:
  topology:
    variables:
    - name: network
      value:
        addressesFromPools:
        - apiGroup: ipam.cluster.x-k8s.io
          kind: InClusterIPPool
          name: inclusterippool

When this is specified on the workload cluster config the machine templates will specify addressesFromPools and CAPV will claim addresses from the specified pool for the machine.

For further reading see this proposal as well: https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/main/docs/proposal/20220929-ipam-support.md

Which issue(s) this PR fixes

Fixes n/a

Describe testing done for PR

Deployed a management cluster with the feature flag on.
Deployed a workload cluster with the NODE_IPAM_IP_POOL_NAME field specified and saw it use an IP address from the IP pool.

Release note

Add support for Node IPAM for vSphere. IP Addresses can be claimed from an IPAM provider instead of DHCP.

Additional information

This will require CAPV to be bumped to include the Node IPAM feature: kubernetes-sigs/cluster-api-provider-vsphere#1666

Special notes for your reviewer

@christianang christianang requested review from a team as code owners November 11, 2022 00:25
@adobley adobley force-pushed the add-ipam-provider branch 3 times, most recently from 0bf51b5 to d8410de Compare November 15, 2022 00:45
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3899/20221115004535/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3899/20221115005343/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #3899 (0d71f04) into main (164c82d) will decrease coverage by 0.86%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3899      +/-   ##
==========================================
- Coverage   47.99%   47.12%   -0.87%     
==========================================
  Files         423      446      +23     
  Lines       42706    44179    +1473     
==========================================
+ Hits        20496    20821     +325     
- Misses      20256    21381    +1125     
- Partials     1954     1977      +23     
Impacted Files Coverage Δ
tkg/client/client.go 81.25% <ø> (ø)
tkg/client/init.go 4.87% <0.00%> (-0.08%) ⬇️
tkg/clusterclient/clusterclient.go 48.20% <0.00%> (-0.08%) ⬇️
addons/controllers/clusterbootstrap_controller.go 63.75% <0.00%> (-1.57%) ⬇️
packageclients/pkg/packageclient/package_update.go 83.57% <0.00%> (-1.43%) ⬇️
...ons/controllers/packageinstallstatus_controller.go 77.99% <0.00%> (-1.16%) ⬇️
...in/cluster/set_machinehealthcheck_control_plane.go 21.21% <0.00%> (ø)
cmd/cli/plugin/cluster/kubeconfig_get.go 8.82% <0.00%> (ø)
cmd/cli/plugin/cluster/get.go 6.27% <0.00%> (ø)
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3899/20221115174833/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@adobley
Copy link
Contributor

adobley commented Nov 16, 2022

/test install-vc7

@alfredthenarwhal
Copy link
Collaborator

@adobley: /test install-vc7
Commit: 6d41542

Build failed! Build no: 3230

@flawedmatrix
Copy link
Contributor

/test install-vc7

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3899/20221117025648/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@alfredthenarwhal
Copy link
Collaborator

@flawedmatrix: /test install-vc7
Commit: ebb763a

Build failed! Build no: 3233

@adobley
Copy link
Contributor

adobley commented Nov 17, 2022

/test install-vc7

@alfredthenarwhal
Copy link
Collaborator

@adobley: /test install-vc7
Commit: ebb763a

Tests failed! Build no: 3251

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3899/20221121172159/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

- In cluster node IPAM provider can be deployed to the MC when feature
  flag `features.management-cluster.deploy-in-cluster-ipam-provider` is
  enabled
- ClusterClass must be used when deploying a WC with Node IPAM
- Specifying NODE_IPAM_IP_POOL_NAME when deploying a WC will cause capv
  to assign IP addresses to nodes from specified IPAM pool.
- .addressesFromPools can be specified in the ClusterClass network
  section in the variables array, which allows you to specify arbitrary
  IPAM providers and more than one pool

Co-authored-by: Tyler Schultz <tschultz@vmware.com>
Co-authored-by: Edwin Xie <exie@vmware.com>
Co-authored-by: Aidan Obley <aobley@vmware.com>
@randomvariable
Copy link
Contributor

Rebased

@randomvariable
Copy link
Contributor

/test install-vc7

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3899/20221125124223/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@alfredthenarwhal
Copy link
Collaborator

@randomvariable: /test install-vc7
Commit: 0d71f04

Tests failed! Build no: 3352

@christianang
Copy link
Contributor Author

/retest

@navidshaikh
Copy link
Contributor

/test install-vc7

@alfredthenarwhal
Copy link
Collaborator

@christianang: /retest
Commit: 0d71f04

Build failed! Build no: 3370

@alfredthenarwhal
Copy link
Collaborator

@navidshaikh: /test install-vc7
Commit: 0d71f04

Tests passed! Build no: 3371

@srm09
Copy link
Contributor

srm09 commented Nov 29, 2022

CAPV version has been updated to v1.5.0 which has the IPAM controller changes, so this one should be unblocked.

@randomvariable randomvariable added the ok-to-merge PRs should be labelled with this before merging label Nov 29, 2022
@randomvariable randomvariable merged commit 4915076 into vmware-tanzu:main Nov 29, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants