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

⚠️ Allow specifying a different credentials per VSphereMachine #1743

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apis/v1alpha3/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions apis/v1alpha4/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions apis/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add information about the fallback here?

// +optional
IdentityRef *VSphereIdentityReference `json:"identityRef,omitempty"`
}

// VSphereMachineTemplateResource describes the data needed to create a VSphereMachine from a template
Expand Down
5 changes: 5 additions & 0 deletions apis/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,25 @@ spec:
from which the virtual machine is cloned. Check the compatibility
with the ESXi version before setting the value.
type: string
identityRef:
description: IdentityRef is a reference to either a Secret or VSphereClusterIdentity
that contains the identity to use when reconciling the virtual machine.
properties:
kind:
description: Kind of the identity. Can either be VSphereClusterIdentity
or Secret
enum:
- VSphereClusterIdentity
- Secret
type: string
name:
description: Name of the identity.
minLength: 1
type: string
required:
- kind
- name
type: object
memoryMiB:
description: MemoryMiB is the size of a virtual machine's memory,
in MiB. Defaults to the eponymous property value in the template
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,26 @@ spec:
Check the compatibility with the ESXi version before setting
the value.
type: string
identityRef:
description: IdentityRef is a reference to either a Secret
or VSphereClusterIdentity that contains the identity to
use when reconciling the virtual machine.
properties:
kind:
description: Kind of the identity. Can either be VSphereClusterIdentity
or Secret
enum:
- VSphereClusterIdentity
- Secret
type: string
name:
description: Name of the identity.
minLength: 1
type: string
required:
- kind
- name
type: object
memoryMiB:
description: MemoryMiB is the size of a virtual machine's
memory, in MiB. Defaults to the eponymous property value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,25 @@ spec:
from which the virtual machine is cloned. Check the compatibility
with the ESXi version before setting the value.
type: string
identityRef:
description: IdentityRef is a reference to either a Secret or VSphereClusterIdentity
that contains the identity to use when reconciling the virtual machine.
properties:
kind:
description: Kind of the identity. Can either be VSphereClusterIdentity
or Secret
enum:
- VSphereClusterIdentity
- Secret
type: string
name:
description: Name of the identity.
minLength: 1
type: string
required:
- kind
- name
type: object
memoryMiB:
description: MemoryMiB is the size of a virtual machine's memory,
in MiB. Defaults to the eponymous property value in the template
Expand Down
47 changes: 32 additions & 15 deletions controllers/vspherevm_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -554,29 +554,25 @@ func (r vmReconciler) retrieveVcenterSession(ctx goctx.Context, vsphereVM *infra
params := session.NewParams().
WithServer(vsphereVM.Spec.Server).
WithDatacenter(vsphereVM.Spec.Datacenter).
WithUserInfo(r.ControllerContext.Username, r.ControllerContext.Password).
WithThumbprint(vsphereVM.Spec.Thumbprint).
WithFeatures(session.Feature{
EnableKeepAlive: r.EnableKeepAlive,
KeepAliveDuration: r.KeepAliveDuration,
})
cluster, err := clusterutilv1.GetClusterFromMetadata(r.ControllerContext, r.Client, vsphereVM.ObjectMeta)

vsphereCluster, err := r.retrieveCluster(vsphereVM)
if err != nil {
r.Logger.Info("VsphereVM is missing cluster label or cluster does not exist")
return session.GetOrCreate(r.Context,
params)
return nil, err
}

key := ctrlclient.ObjectKey{
Namespace: cluster.Namespace,
Name: cluster.Spec.InfrastructureRef.Name,
}
vsphereCluster := &infrav1.VSphereCluster{}
err = r.Client.Get(r, key, vsphereCluster)
if err != nil {
r.Logger.Info("VSphereCluster couldn't be retrieved")
return session.GetOrCreate(r.Context,
params)
if vsphereVM.Spec.IdentityRef != nil {
Copy link
Member

Choose a reason for hiding this comment

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

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

creds, err := identity.GetCredentialsWithExternalIdentity(ctx, r.Client, vsphereCluster, vsphereVM.Spec.IdentityRef, r.Namespace)
Copy link
Member

Choose a reason for hiding this comment

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

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

if err != nil {
return nil, err
}

params = params.WithUserInfo(creds.Username, creds.Password)
return session.GetOrCreate(ctx, params)
}

if vsphereCluster.Spec.IdentityRef != nil {
Expand All @@ -589,11 +585,32 @@ func (r vmReconciler) retrieveVcenterSession(ctx goctx.Context, vsphereVM *infra
params)
}

params = params.WithUserInfo(r.ControllerContext.Username, r.ControllerContext.Password)
// Fallback to using credentials provided to the manager
return session.GetOrCreate(r.Context,
params)
}

func (r vmReconciler) retrieveCluster(vsphereVM *infrav1.VSphereVM) (*infrav1.VSphereCluster, error) {
cluster, err := clusterutilv1.GetClusterFromMetadata(r.ControllerContext, r.Client, vsphereVM.ObjectMeta)
if err != nil {
r.Logger.Info("VsphereVM is missing cluster label or cluster does not exist")
return nil, err
}

key := ctrlclient.ObjectKey{
Namespace: cluster.Namespace,
Name: cluster.Spec.InfrastructureRef.Name,
}
vsphereCluster := &infrav1.VSphereCluster{}
err = r.Client.Get(r, key, vsphereCluster)
if err != nil {
r.Logger.Info("VSphereCluster couldn't be retrieved")
return nil, err
}
return vsphereCluster, nil
}

func (r vmReconciler) fetchClusterModuleInfo(clusterModInput fetchClusterModuleInput) (*string, error) {
var (
owner ctrlclient.Object
Expand Down
39 changes: 39 additions & 0 deletions docs/multi-vcenter.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Multi VCenter support
Copy link
Member

Choose a reason for hiding this comment

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

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 ?


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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Also what does the first sentence mean?

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


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``.
Copy link
Member

Choose a reason for hiding this comment

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

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


## Examples

Deploy a `VSphereMachine` with a custom identityRef:

```yaml
apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: VSphereMachine
metadata:
name: new-workload-cluster
spec:
server: vcenter
identityRef:
kind: VSphereClusterIdentity
name: identityName
...
```

Deploy a `VSphereMachineTemplate` with a custom identityRef:

```yaml
apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: VSphereMachineTemplate
metadata:
name: new-workload-cluster
spec:
template:
spec:
server: vcenter
identityRef:
Comment on lines +34 to +35
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
server: vcenter
identityRef:
server: vcenter
identityRef:

kind: VSphereClusterIdentity
name: identityName
...
```
17 changes: 11 additions & 6 deletions pkg/identity/identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,11 @@ type Credentials struct {
Password string
}

func GetCredentials(ctx context.Context, c client.Client, cluster *infrav1.VSphereCluster, controllerNamespace string) (*Credentials, error) {
if err := validateInputs(c, cluster); err != nil {
func GetCredentialsWithExternalIdentity(ctx context.Context, c client.Client, cluster *infrav1.VSphereCluster, ref *infrav1.VSphereIdentityReference, controllerNamespace string) (*Credentials, error) {
if err := validateInputs(c, cluster, ref); err != nil {
return nil, err
}

ref := cluster.Spec.IdentityRef
secret := &apiv1.Secret{}
var secretKey client.ObjectKey

Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

unit tests would be helpful

if err := validateInputs(c, cluster, cluster.Spec.IdentityRef); err != nil {
return nil, err
}
return GetCredentialsWithExternalIdentity(ctx, c, cluster, cluster.Spec.IdentityRef, controllerNamespace)
}

func validateInputs(c client.Client, cluster *infrav1.VSphereCluster, identityRef *infrav1.VSphereIdentityReference) error {
if c == nil {
return errors.New("kubernetes client is required")
}
if cluster == nil {
return errors.New("vsphere cluster is required")
}
ref := cluster.Spec.IdentityRef
if ref == nil {
if identityRef == nil {
return errors.New("IdentityRef is required")
}
return nil
Expand Down
6 changes: 3 additions & 3 deletions pkg/identity/identity_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Member

Choose a reason for hiding this comment

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

would need test cases for the new behavior

Expect(validateInputs(nil, cluster, nil)).NotTo(Succeed())
})
})

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

Context("If the identityRef is missing on cluster", func() {
It("should error if identityRef is missing on cluster", func() {
Expect(validateInputs(k8sclient, cluster)).NotTo(Succeed())
Expect(validateInputs(k8sclient, cluster, nil)).NotTo(Succeed())
})
})
})
Expand Down