⚠️ Allow specifying a different credentials per VSphereMachine #1743

farodin91 · 2023-01-17T14:39:42Z

What this PR does / why we need it:
Allow multiple vcenter in a single cluster

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 #1720

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

I will try to test it.

Release note:

Allow custom identity in vmmachine

k8s-ci-robot · 2023-01-17T14:39:51Z

Hi @farodin91. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-triage-robot · 2023-04-17T15:11:26Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

linux-foundation-easycla · 2023-04-20T13:42:26Z

The committers listed above are authorized under a signed CLA.

✅ login: farodin91 / name: Jan Jansen (148ce73)

k8s-triage-robot · 2023-06-02T19:36:38Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

farodin91 · 2023-06-23T17:16:55Z

@MaxRink would you like to look into this PR?

farodin91 · 2023-07-05T14:05:40Z

@srm09 would you like to look into this PR?

randomvariable · 2023-07-20T17:18:59Z

We discussed this in the call of 2023/07/23:

This will need to be marked as a breaking change, as there is a security implication of allowing users to create MachineDeployments targeting differing vcenters.
We need an expansion of docs about how this works in a cluster and how to set up CPI and CSI in a cluster to make it work

cc @puneetkatyal

randomvariable

As per #1743 (comment)

Signed-off-by: Jan Jansen <[email protected]>

k8s-ci-robot · 2023-08-04T08:48:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign randomvariable for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

farodin91 · 2023-08-04T08:50:06Z

@randomvariable I started to write a documentation.

I added a link to external documentation for csi and cpi, do you think this is enough?

How should i mark this a breaking change?

chrischdi · 2023-08-04T09:12:05Z

/retitle ⚠️ Allow specifying a different credentials per VSphereMachine

(warning because of the security impact)

farodin91 · 2023-08-04T12:35:55Z

@randomvariable Should i add this to commit message?

k8s-ci-robot · 2023-08-16T18:36:13Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

guilhermevillote · 2023-09-07T19:02:38Z

Any updates on this PR?

This is something very useful when your infrastructure has multiple vcenters and you constantly needs to migrate machines from one to another..

In this scenario, this PR is essential to adopt the CAPI.

Thanks

k8s-triage-robot · 2024-01-21T11:20:53Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

guilhermevillote · 2024-02-07T09:34:30Z

Hi, any updates on this PR?

chrischdi

some comments, definetly needs a rebase too.

chrischdi · 2023-08-04T09:05:25Z

apis/v1beta1/types.go

@@ -204,6 +204,11 @@ type VirtualMachineCloneSpec struct {
 	// Check the compatibility with the ESXi version before setting the value.
 	// +optional
 	HardwareVersion string `json:"hardwareVersion,omitempty"`
+
+	// IdentityRef is a reference to either a Secret or VSphereClusterIdentity that contains
+	// the identity to use when reconciling the virtual machine.


Should we also add information about the fallback here?

chrischdi · 2023-08-04T09:05:35Z

docs/multi-vcenter.md

@@ -0,0 +1,39 @@
+# Multi VCenter support
+
+Cluster API Provider vSphere (CAPV) supports multiple VCenter for a single. Therefore CAPV is allowing to define the used identity for each machine. CAPV will check on every Machine first, if there is a local identity otherwise it fallback on the default selection method. 


Suggested change

Cluster API Provider vSphere (CAPV) supports multiple VCenter for a single. Therefore CAPV is allowing to define the used identity for each machine. CAPV will check on every Machine first, if there is a local identity otherwise it fallback on the default selection method.

Cluster API Provider vSphere (CAPV) supports multiple vCenter for a single cluster. Therefore CAPV allows to define the used identity for each machine. CAPV will check on every Machine first, if there is a local identity otherwise it fallback on the default selection method.

Also what does the first sentence mean?

Cluster API Provider vSphere (CAPV) supports multiple VCenter for a single.

chrischdi · 2023-08-04T09:07:15Z

docs/multi-vcenter.md

+       server: vcenter
+      identityRef:


Suggested change

server: vcenter

identityRef:

server: vcenter

identityRef:

chrischdi · 2023-08-04T09:28:18Z

controllers/vspherevm_controller.go

-		r.Logger.Info("VSphereCluster couldn't be retrieved")
-		return session.GetOrCreate(r.Context,
-			params)
+	if vsphereVM.Spec.IdentityRef != nil {


Could we document the behaviour at the (not existing) function godoc?

chrischdi · 2024-02-07T12:14:25Z

pkg/identity/identity_test.go

@@ -236,19 +236,19 @@ var _ = Describe("validateInputs", func() {

 	Context("If the client is missing", func() {
 		It("should error if client is missing", func() {
-			Expect(validateInputs(nil, cluster)).NotTo(Succeed())


would need test cases for the new behavior

chrischdi · 2024-02-07T12:14:44Z

apis/v1alpha3/zz_generated.conversion.go

@@ -1687,5 +1687,6 @@ func autoConvert_v1beta1_VirtualMachineCloneSpec_To_v1alpha3_VirtualMachineClone
 	// WARNING: in.PciDevices requires manual conversion: does not exist in peer-type
 	// WARNING: in.OS requires manual conversion: does not exist in peer-type
 	// WARNING: in.HardwareVersion requires manual conversion: does not exist in peer-type
+	// WARNING: in.IdentityRef requires manual conversion: does not exist in peer-type


v1alpha3 is gone on main (similar v1alpha4?)

chrischdi · 2024-02-07T12:16:46Z

docs/multi-vcenter.md

+
+Cluster API Provider vSphere (CAPV) supports multiple VCenter for a single. Therefore CAPV is allowing to define the used identity for each machine. CAPV will check on every Machine first, if there is a local identity otherwise it fallback on the default selection method. 
+
+In order to run a CAPV cluster in multiple VCenter, you have to configure CPI & CSI to support multi VCenter, see [guide](https://docs.vmware.com/en/VMware-vSphere-Container-Storage-Plug-in/3.0/vmware-vsphere-csp-getting-started/GUID-8B3B9004-DE37-4E6B-9AA1-234CDA1BD7F9.html). Trivia, `VSphereCluster` can be only in single VCenter. This will just used as a fallback, if you haven't configured a different identity for a `VSphereMachine``.


This only links csi, are there also instructions for CPI?

chrischdi · 2024-02-07T12:17:17Z

pkg/identity/identity.go

@@ -108,15 +107,21 @@ func GetCredentials(ctx context.Context, c client.Client, cluster *infrav1.VSphe
 	return credentials, nil
 }

-func validateInputs(c client.Client, cluster *infrav1.VSphereCluster) error {
+func GetCredentials(ctx context.Context, c client.Client, cluster *infrav1.VSphereCluster, controllerNamespace string) (*Credentials, error) {


unit tests would be helpful

chrischdi · 2024-02-07T12:20:12Z

docs/multi-vcenter.md

@@ -0,0 +1,39 @@
+# Multi VCenter support


Is most of this better suited to Also should ahve some explanation how it works in https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/main/docs/identity_management.md ?

chrischdi · 2024-02-07T12:22:00Z

controllers/vspherevm_controller.go

-		return session.GetOrCreate(r.Context,
-			params)
+	if vsphereVM.Spec.IdentityRef != nil {
+		creds, err := identity.GetCredentialsWithExternalIdentity(ctx, r.Client, vsphereCluster, vsphereVM.Spec.IdentityRef, r.Namespace)


Wouldn't this get overwritten below in l 578 when there is a definition on the cluster level already?

k8s-triage-robot · 2024-03-08T12:39:22Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

guilhermevillote · 2024-03-08T18:44:43Z

/remove-lifecycle rotten

guilhermevillote · 2024-03-11T18:00:49Z

Do we have any estimate of when this change might be released?

chrischdi · 2024-03-12T08:19:55Z

I'm not actively working on this PR so I cannot give any estimate.

farodin91 · 2024-03-12T09:13:50Z

@chrischdi @guilhermevillote If someone like to take over i'm fine. I don't have time at moment to fix it.

chrischdi · 2024-03-13T15:46:17Z

@chrischdi @guilhermevillote If someone like to take over i'm fine. I don't have time at moment to fix it.

Thanks @farodin91 !

/help

sbueringer · 2024-04-09T12:13:23Z

/close

Let's revive if someone has time & interest again

k8s-ci-robot · 2024-04-09T12:13:27Z

@sbueringer: Closed this PR.

In response to this:

/close

Let's revive if someone has time & interest again

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 17, 2023

k8s-ci-robot requested review from MaxRink and vrabbi January 17, 2023 14:39

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 17, 2023

farodin91 force-pushed the allow-other-identity-in-vmmachine branch from d6a85fc to 6070f30 Compare January 17, 2023 14:40

farodin91 changed the title ~~allow other identity in vmmachine~~ ✨ allow other identity in vmmachine Jan 17, 2023

farodin91 force-pushed the allow-other-identity-in-vmmachine branch from 6070f30 to 0567931 Compare January 17, 2023 14:57

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 17, 2023

farodin91 force-pushed the allow-other-identity-in-vmmachine branch from 0567931 to 95ee4af Compare April 20, 2023 13:42

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2023

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 20, 2023

farodin91 changed the base branch from main to release-1.5 April 20, 2023 13:42

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 20, 2023

farodin91 force-pushed the allow-other-identity-in-vmmachine branch from 95ee4af to db9d2d7 Compare May 3, 2023 13:08

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2023

farodin91 changed the base branch from release-1.5 to release-1.6 May 3, 2023 13:09

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 3, 2023

randomvariable requested changes Jul 28, 2023

View reviewed changes

✨ allow other identity in vmmachine

148ce73

Signed-off-by: Jan Jansen <[email protected]>

farodin91 force-pushed the allow-other-identity-in-vmmachine branch from 67b9c07 to 148ce73 Compare August 4, 2023 08:48

k8s-ci-robot changed the title ~~✨ allow other identity in vmmachine~~ ⚠️ Allow specifying a different credentials per VSphereMachine Aug 4, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2024

chrischdi reviewed Feb 7, 2024

View reviewed changes

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 8, 2024

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 8, 2024

k8s-ci-robot closed this Apr 9, 2024

		@@ -0,0 +1,39 @@
		# Multi VCenter support

		Cluster API Provider vSphere (CAPV) supports multiple VCenter for a single. Therefore CAPV is allowing to define the used identity for each machine. CAPV will check on every Machine first, if there is a local identity otherwise it fallback on the default selection method.


		Cluster API Provider vSphere (CAPV) supports multiple VCenter for a single. Therefore CAPV is allowing to define the used identity for each machine. CAPV will check on every Machine first, if there is a local identity otherwise it fallback on the default selection method.

		In order to run a CAPV cluster in multiple VCenter, you have to configure CPI & CSI to support multi VCenter, see [guide](https://docs.vmware.com/en/VMware-vSphere-Container-Storage-Plug-in/3.0/vmware-vsphere-csp-getting-started/GUID-8B3B9004-DE37-4E6B-9AA1-234CDA1BD7F9.html). Trivia, `VSphereCluster` can be only in single VCenter. This will just used as a fallback, if you haven't configured a different identity for a `VSphereMachine``.

⚠️ Allow specifying a different credentials per VSphereMachine #1743

⚠️ Allow specifying a different credentials per VSphereMachine #1743

Conversation

farodin91 commented Jan 17, 2023

k8s-ci-robot commented Jan 17, 2023

k8s-triage-robot commented Apr 17, 2023

linux-foundation-easycla bot commented Apr 20, 2023 • edited Loading

k8s-triage-robot commented Jun 2, 2023

farodin91 commented Jun 23, 2023

farodin91 commented Jul 5, 2023

randomvariable commented Jul 20, 2023

randomvariable left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 4, 2023

farodin91 commented Aug 4, 2023

chrischdi commented Aug 4, 2023

farodin91 commented Aug 4, 2023

k8s-ci-robot commented Aug 16, 2023

guilhermevillote commented Sep 7, 2023 • edited Loading

k8s-triage-robot commented Jan 21, 2024

guilhermevillote commented Feb 7, 2024

chrischdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Mar 8, 2024

guilhermevillote commented Mar 8, 2024

guilhermevillote commented Mar 11, 2024

chrischdi commented Mar 12, 2024

farodin91 commented Mar 12, 2024

chrischdi commented Mar 13, 2024

sbueringer commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

linux-foundation-easycla bot commented Apr 20, 2023 •

edited

Loading

guilhermevillote commented Sep 7, 2023 •

edited

Loading