Initial commit for handling multiple secrets

[davidvonthenen]

What this PR does / why we need it:
This PR implements the following issue (#233) and a bonus feature! 😀

This allows each [VirtualCenter] to have their own dedicated Secret containing VC credentials (username/password). By having a dedicated Secret, each [VirtualCenter] will be isolated from accessing each other's Secret when said Secret is placed into their own namespace. Maybe an example vsphere.conf might be helpful...

[Global]
# properties in this section will be used for all specified vCenters unless overriden in VirtualCenter section.
port = "443" #Optional
insecure-flag = "1" #set to 1 if the vCenter uses a self-signed cert

[VirtualCenter "10.160.140.218"]
datacenters = "vicdc"
secret-name = "vccm-europe-secret"
secret-namespace = "kube-system"
# port, insecure-flag will be used from Global section.

[VirtualCenter "10.161.60.78"]
datacenters = "vic0dc,vic1dc"
secret-name = "vccm-us-secret"
secret-namespace = "kube-system"
# port, insecure-flag will be used from Global section.

[Labels]
region = k8s-region
zone = k8s-zone

The "bonus" feature is something I am calling TenantRefs. Prior to this PR, a single [VirtualCenter] was constrained to have either all or a subset of Datacenters contribute to the k8s cluster. Those DCs that weren't contributing to the cluster, couldn't have a different configuration because the [VirtualCenter] "key" was the ip/fqdn of that vCenter Server. A TenantRefs is set by defining the server field under the [VirtualCenter] section. If that server field is set, it is assumed that the value in [VirtualCenter "value"] is the TenantRef. By implementing TenantRefs, this allows the potential for each VC/DC to have its own unique configuration based on the access controls of a given set of creds. So what does this feature translate into in English? Think of this as possibly enabling multi-tenancy. Maybe an example configuration might help...

[Global]
# properties in this section will be used for all specified vCenters unless overriden in VirtualCenter section.
port = "443" #Optional
insecure-flag = "1" #set to 1 if the vCenter uses a self-signed cert

[VirtualCenter "us-west-tenant1"]
server = "10.160.140.218"
datacenters = "tenant1"
secret-name = "vccm-tenant1-secret"
secret-namespace = "tenant1"
# port, insecure-flag will be used from Global section.

[VirtualCenter "us-east-tenant2"]
server = "10.161.60.78"
datacenters = "tenant2"
secret-name = "vccm-tenant2-secret"
secret-namespace = "tenant2"
# port, insecure-flag will be used from Global section.

[VirtualCenter "us-east-tenant3"]
server = "10.161.60.78"
datacenters = "us-east"
secret-name = "vccm-tenant3-secret"
secret-namespace = "tenant3"
# port, insecure-flag will be used from Global section.

[VirtualCenter "us-east-tenant4"]
server = "10.161.60.78"
datacenters = "us-east"
secret-name = "vccm-tenant4-secret"
secret-namespace = "tenant4"
# port, insecure-flag will be used from Global section.

[Labels]
region = k8s-region
zone = k8s-zone

Some things to note based on the example configuration above:

• the defined TenantRefs starting from the top of the config file to the bottom are: us-west-tenant1, us-east-tenant2, us-east-tenant3, and us-east-tenant4. TenantRefs must be unique!
• each tenant has their own secret which means each tenant has their own vSphere user account
• tenant3 and tenant4 are sharing a given DC. the resources each tenant is using can be assigned based on the vSphere user account (maybe each account has their own ResourcePool or ComputeCluster)
• tenant2 has its own dedicated DC but sharing the same VC as tenant3 and 4
• tenant1 has their own VC (and subsequently their own DC)

Here is an example vsphere.conf based on what I actually tested:

[Global]
# properties in this section will be used for all specified vCenters unless overridden in VirtualCenter section.
port = "443" #Optional
insecure-flag = "1" #set to 1 if the vCenter uses a self-signed cert

[VirtualCenter "europe"]
server = "10.160.140.218"
datacenters = "vicdc"
secret-name = "vccm-europe-secret"
secret-namespace = "kube-system"
# port, insecure-flag will be used from Global section.

[VirtualCenter "us-west"]
server = "10.161.60.78"
datacenters = "vic0dc"
secret-name = "vccm-us-west-secret"
secret-namespace = "kube-system"
# port, insecure-flag will be used from Global section.

[VirtualCenter "us-east"]
serever = "10.161.60.78"
datacenters = "vic1dc"
secret-name = "vccm-us-east-secret"
secret-namespace = "kube-system"
# port, insecure-flag will be used from Global section.

[Labels]
region = k8s-region
zone = k8s-zone

Other minor changes along for the ride in this PR:

• cleaned up some functions in the Config package. it made more sense to make them member functions of the Config instead of passing the Config into the function
• the old Connect(vcServer string) in ConnectionManager is no longer sufficient to determine which VirtualCenterConfig is being referenced and since there is only one method for Connect() now, I renamed ConnectByInstance() to Connect(). Example of ambiguity in old Connect(vcServer string):

[VirtualCenter "10.161.60.78"]
datacenters = "vic0dc"
...

[VirtualCenter "10.161.60.78"]
datacenters = "vic1dc"
...

• Moved a number of consts and user-defined errors into their own go file. Adding more just started to make the go files with business logic larger in length (ie it's easier to manage this way and a number of package already do this).
• Removed the function removePortFromHost() because URL.Hostname() exists and returns the Hostname minus the port.
• Renamed SecretCredentialManager to CredentialManager because it actually does handle Creds from the Config as well. So the name should accurately reflect what it actually manages... ie all credentials.

Which issue this PR fixes: #233

Special notes for your reviewer:
Tested on vSphere 6.7u2
Test cases included:

• Using the backward compatible (aka Config format before this feature was introduced) with a single global secret
• Using the new TenantRefs with each their own dedicated Secret. In my test configuration, that's 3 Datacenters mapping to 3 [VirtualCenter] with 3 secrets.
• Using a hybrid of the previous two test cases. In other words, each [VirtualCenter] has their own TenantRef, 2 of the 3 [VirtualCenter] had their own dedicated Secret, and the 3rd [VirtualCenter] used the "global" Secret. This is a complicated use case which I am sure no one will use... the point in testing this just means I have confidence everything is working fine. 🙂

Since this feature is a pretty significant change and because the default/current/backward-compatible vsphere.conf file format still works, I am going to do the documentation in a separate PR. This is mainly due to the fact that this will impact CSI and I would like to start the integration of this feature into CSI before the new CNS based implementation PR start making their way over and I lose the ability to accurately test this feature on CSI.

Release note:
Default/current/backward-compatible vsphere.conf still works!

[davidvonthenen]

Oops... my last couple of commits broke the go test and fix some lint/vet/fmt issues. I just need to update the simulated vsphere.conf files.

[davidvonthenen]

Fixed simulated vsphere.conf which corrected the go test and fixed lint/vet/fmt issues. This PR is ready for review.

CC: @frapposelli @andrewsykim @codenrhoden

[andrewsykim]

So the ServiceAccounts are to ensure the CCM can't accidentally modify a VM belonging to another VC but only if the secrets are stored in separate namespaces?

[davidvonthenen]

So the ServiceAccounts are to ensure the CCM can't accidentally modify a VM belonging to another VC but only if the secrets are stored in separate namespaces?

That really depends on how you set everything up. I could see a few different ways of how you architect everything. The CPI's vCenter accounts should only need to be read-only (need to think about that a little more 🤔to be sure). CSI, on the other hand, is probably where the real benefit sits because it needs to have create/modify access for datastores, volumes and VMs.

The service accounts are a method to restrict access to k8s secrets. The VC credentials within those secrets are what actually prevents modification of VMs by reducing scope and access/permissions of what that VC user cred can see/modify/delete on a given vCenter Server. Hope that makes sense.

Still... even the reduced scope for the CPI even using read-only vCenter Server user creds will have significant security benefits. Viewing inventory you shouldn't see is still no bueno.

[codenrhoden]

I added comments where I felt like I saw something. :) But I don't really know enough about this code to use "Request Changes" and feel like you have to.

[andrewsykim]

So maybe I'm missing something still. I’m confused because the default ClusterRole we use for CPI can read secrets from all namespaces, so specifying the SA doesn’t really add additional isolation but it does make the install/configure steps more complicated. Is the intention to remove access to secrets in the cloud-controller-manager role and require SAs (w/ Role + RoleBinding) per VC?

[davidvonthenen]

So maybe I'm missing something still. I’m confused because the default ClusterRole we use for CPI can read secrets from all namespaces, so specifying the SA doesn’t really add additional isolation but it does make the install/configure steps more complicated. Is the intention to remove access to secrets in the cloud-controller-manager role and require SAs (w/ Role + RoleBinding) per VC?

Yup, that's the exact intention. I just added a note that the default cloud-controller-manager-roles.yaml needs to be changed to in the PR. It should also be noted that we will still need the cloud-controller-manager role because that "default" service account is used for listening for the NodeManager for adds/removes/updates to nodes.

[davidvonthenen]

Updated based feedback from @andrewsykim and updated the description (comment #1) of the PR for consistency.

The good news is by changing how we enable ConnectionLabels from what was first implemented, it's made the code much simpler. The short version of this change is you enable the ConnectionLabel by defining the server field under the [VirtualCenter] section. When server which contains the ip/fqdn of the vCenter Server is set, the value in [VirtualCenter "value"] is the ConnectionLabel. Check the updated description for config examples. The logic now is 💯

Thanks @andrewsykim !!

[davidvonthenen]

/hold

[davidvonthenen]

/hold cancel

[davidvonthenen]

Yup, we are good to go!

[codenrhoden]

/lgtm

[andrewsykim]

Overall LGTM, left a few more minor comments

[andrewsykim]

/approve
/hold

LGTM, remaining comments non-blocking, holding in-case you want to address them

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [andrewsykim]

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

[andrewsykim]

/lgtm

[davidvonthenen]

/hold cancel