Aligning CAPV with vSphere Cloud Provider

[andrewsykim]

As it stands today, there are some behaviors between CAPV and both the in-tree and out-of-tree vSphere cloud provider that need to be aligned. Some examples of this below:

1. Networking configuration - in Support for kubeadm bootstrapping & using new imgs #307, CAPV will assign a MAC addr for a VM network prior to the VM powering on. The MAC OUI prefix needs to be acceptable for the cloud provider integration. For the in-tree case, that means the OUI must have a prefix of 00:05:69, 00:0c:29 , 00:1c:14 or 00:50:56. For the out-of-tree case the mac addr assigned shouldn't matter.

2. Resource management - today the datacenter, datastore and resource pool a VM is created inside is determined at the machine spec level, however, the vSphere cloud provider may not always recognize the same values set by CAPV. For example, a machine spec may have been provisioned under a certain resource pool but the configured resource pool set on the cloud config file may differ. We need to ensure the requirements for these values between CAPV and the cloud provider are clear. Differing values between the two will break certain features in the cluster.

[akutz]

I've spent the last several days working on a PR to address the second item called out in this issue description -- the configuration of the vSphere cloud provider when deployed with CAPV.

This work has solved problems, but also raised new points of discussion. I am writing this comment to set the context for the discussion of how to align the vSphere cloud controller manager and CAPV with respect to the cloud controller's configuration types.

Configuring the cloud provider

PR #471 introduces a new type to CAPV, CloudProviderConfiguration. This may be defined in the CAPV cluster spec like so:

apiVersion: "vsphere.cluster.k8s.io/v1alpha1"
kind: "VsphereClusterProviderSpec"
server: "my.vcenter.com"
username: "my.user"
password: "my.pass"
cloudProviderConfiguration:
  global:
    secretName: "cloud-provider-vsphere-credentials"
    secretNamespace: "kube-system"
  virtualCenter:
    my.vcenter.com:
  network:
    name: "sddc-cgw-network-5"
  workspace:
    server: "my.vcenter.com"
    datacenter: "SDDC-Datacenter"
    datastore: "WorkloadDatastore"
    resourcePool: "clusterapi"
    folder: "clusterapi"

This neatly aligns the configuration of the cloud provider at the cluster level, as it should be. However, this solution required the following:

• The ability to marshal/unmarshal a vSphere cloud provider config to/from JSON/YAML
• The ability to marshal a vSphere cloud provider config to INI

Neither of the above is possible today using the vSphere cloud provider config types from the in-tree or external cloud provider projects for several reasons:

1. Incorrect tag usage - gcfg is mispelled as dcfg
2. Lack of JSON tags - none of the config types have any JSON tags defined on fields
3. The use of nested, anonymous structs - the DeepCopy generator does not support nested, anonymous structs
4. Lack of DeepCopy - none of the config types define DeepCopy
5. Lack of MarshalINI - the library used to unmarshal the vSphere INI configuration data does not support marshalling objects back to INI

The pull request

PR #471 introduces solutions to each of the above blockers by implement new configuration types for the vSphere cloud provider.

1. Gcfg tags - the gcfg tags are corrected
2. JSON tags - all types and their fields have proper JSON tags
3. First-class structs - there is no more use of nested, anonymous structs
4. Generate DeepCopy - all types implement DeepCopy
5. Marshal to INI - a MarshalINI function is introduced to support writing the cloud provider config back to INI

MarshalINI

The MarshalINI function in PR #471 is implemented using reflection. It works well for the use case, but it would be far more desirable to rely on some well-known and well-tested library for marshalling structs to INI.

As previously mentioned, the package used to unmarshal from INI, gopkg.in/gcfg.v1, does not support marshalling back to INI.

The package gopkg.in/go-ini/ini.v1 looked promising, but it does not support marshalling structs with fields of type map[TYPE]TYPE. Issue go-ini/ini#199 has been filed in hopes that this package will introduce support for this as otherwise the package works incredibly well.

For now MarshalINI is implemented using reflection, as opposed to Go templates, to ensure that section and property names in the INI match the gcfg tags defined on the structs' fields.

The long-term solution

Long term it simply doesn't make sense for the types and functionality defined in PR #471 to be located in the CAPV project. Instead, these types and features should exist in the external vSphere cloud provider repository.

Why not add them there in the first place? One reason is time. It was far faster to implement them in CAPV and relocate them to the cloud provider repository later.

The second reason begins a much larger discussion.

Future of INI

Why continue to support INI?

• Defining the cloud provider config alongside the CAPV cluster configuration in such a way to get API server validation via the CAPV CRD requires the cloud provider config be defined as YAML.
• Then the CAPV cluster config both stores and transmits the cloud provider config as YAML. The only reason to continue to support INI is for backwards compatibility.

Why not:

1. Introduce support to the vSphere cloud provider for unmarshalling its configuration from YAML
2. Retain support for INI until release/date X

This plan obviates the need for a package/function to marshal a cloud provider config object to INI, and it is in-line with Kubernetes projects.

cc @frapposelli @dvonthenen

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.