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

upi/vsphere: add vsphere to cloudproviderconfig #1516

Merged
merged 2 commits into from
Apr 5, 2019
Merged

upi/vsphere: add vsphere to cloudproviderconfig #1516

merged 2 commits into from
Apr 5, 2019

Conversation

staebler
Copy link
Contributor

@staebler staebler commented Apr 2, 2019

  • Add the config for vsphere to cloudproviderconfig.
  • Add vsphere cloud creds to cloud-creds-secret.yaml.
  • Add fields to vsphere platform in installconfig to support cloudproviderconfig.
  • Set the enable_disk_uuid attribute to true for vsphere VMs as that is required to ensure the VMDK properly mounts disks.

See https://vmware.github.io/vsphere-storage-for-kubernetes/documentation/existing.html
for details on the vsphere cloud config file.

@openshift-ci-robot openshift-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. labels Apr 2, 2019
@staebler
Copy link
Contributor Author

staebler commented Apr 2, 2019

/hold

This builds on top of #1479.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 2, 2019
@staebler
Copy link
Contributor Author

staebler commented Apr 2, 2019

$ cat > install-config.yaml << EOF
apiVersion: v1beta4
baseDomain: devcluster.openshift.com
metadata:
  name: mstaeble
networking:
  machineCIDR: "139.178.89.192/26"
platform:
  vsphere:
    virtualCenters:
    - name: vcsa.vmware.devcluster.openshift.com
      username: dummy@e2e.local
      password: MyPassword
      datacenters:
      - dc1
    workspace:
      defaultDatastore: nvme-ds1
    scsiControllerType: pvscsi
    publicNetwork: VM Network
pullSecret: redacted
sshKey: redacted
EOF

$ openshift-install create manifests
INFO Consuming "Install Config" from target directory

$ cat manifests/cloud-provider-config
apiVersion: v1
data:
  config: |-
    [Global]
    secret-name = "vsphere-creds"
    secret-namespace = "kube-system"

    [VirtualCenter "vcsa.vmware.devcluster.openshift.com"]
    datacenters = "dc1"

    [Workspace]
    server = "vcsa.vmware.devcluster.openshift.com"
    datacenter = "dc1"
    default-datastore = "nvme-ds1"
    resourcepool-path = "mstaeble"
    folder = "mstaeble"

    [Disk]
    scsicontrollertype = "pvscsi"

    [Network]
    public-network = "VM Network"
kind: ConfigMap
metadata:
  creationTimestamp: null
  name: cloud-provider-config
  namespace: openshift-config

$ cat openshift/99_cloud-creds-secret.yaml
kind: Secret
apiVersion: v1
metadata:
  namespace: kube-system
  name: vsphere-creds
data:
  vcsa.vmware.devcluster.openshift.com.username: ZHVtbXlAZTJlLmxvY2Fs
  vcsa.vmware.devcluster.openshift.com.password: TXlQYXNzd29yZA==

@DanyC97
Copy link
Contributor

DanyC97 commented Apr 2, 2019

@staebler quick question if you don't mind asking: have you considered/ is on the table for future work to obfuscate the below credentials ?

 vsphere:
    virtualCenters:
    - name: vcsa.vmware.devcluster.openshift.com
      username: dummy@e2e.local
      password: MyPassword

Some users might have use an admin user and i personally would not feel comfortable to put that in clear as everyone will then be able to use the infrastructure.

Same goes with the Secret as is a simple base64 decode.

kind: Secret
apiVersion: v1
metadata:
  namespace: kube-system
  name: vsphere-creds
data:
  vcsa.vmware.devcluster.openshift.com.username = ZHVtbXlAZTJlLmxvY2Fs
  vcsa.vmware.devcluster.openshift.com.password = TXlQYXNzd29yZA==

i know and understand that it might be a bigger discussion however i thought i should mention (although i'm sure is already known)

@staebler
Copy link
Contributor Author

staebler commented Apr 2, 2019

@staebler quick question if you don't mind asking: have you considered/ is on the table for future work to obfuscate the below credentials ?

@DanyC97 Obfuscation would be good. I'm not sure what options we have, though. We are limited by what the vsphere provider supports, which as far as I know is an un-obfuscated username and password. Do you have any suggestion for how we could add obfuscation?

@staebler
Copy link
Contributor Author

staebler commented Apr 2, 2019

/cc @gnufied How does this look to you with regards to setting up the vSphere cloud config?

@gnufied
Copy link
Member

gnufied commented Apr 2, 2019

@staebler this is looking good. I just have a minor comment about some of validation.

@DanyC97
Copy link
Contributor

DanyC97 commented Apr 2, 2019

@staebler quick question if you don't mind asking: have you considered/ is on the table for future work to obfuscate the below credentials ?

@DanyC97 Obfuscation would be good. I'm not sure what options we have, though. We are limited by what the vsphere provider supports, which as far as I know is an un-obfuscated username and password. Do you have any suggestion for how we could add obfuscation?

@staebler i'm not an terraform expert (still ramping up) however i'll dig a bit into and then open an issue to discuss further so i don't distract the attention on this PR.

public-network = "{{.PublicNetwork}}"`

// CloudProviderConfig generates the cloud provider config for the vSphere platform.
func CloudProviderConfig(installConfig *types.InstallConfig) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: start with Vpshere platform?

@abhinavdahiya
Copy link
Contributor

/hold

This builds on top of #1479.

/hold cancel

@staebler feel free to carry the change from #1479 as part of this PR. i'll close #1479 when this lands.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2019
@abhinavdahiya
Copy link
Contributor

@staebler quick question if you don't mind asking: have you considered/ is on the table for future work to obfuscate the below credentials ?

 vsphere:
    virtualCenters:
    - name: vcsa.vmware.devcluster.openshift.com
      username: dummy@e2e.local
      password: MyPassword

Some users might have use an admin user and i personally would not feel comfortable to put that in clear as everyone will then be able to use the infrastructure.

The install-config does end up on the cluster, as config map. it might be necessary to remove this password when we store the configmap here

Same goes with the Secret as is a simple base64 decode.

kind: Secret
apiVersion: v1
metadata:
  namespace: kube-system
  name: vsphere-creds
data:
  vcsa.vmware.devcluster.openshift.com.username = ZHVtbXlAZTJlLmxvY2Fs
  vcsa.vmware.devcluster.openshift.com.password = TXlQYXNzd29yZA==

Secrets are pushed to the api and then they are always kept encrypted in etcd like any other secret. Only valid users has access to secrets. So there is no requirement for doing anything here.

i know and understand that it might be a bigger discussion however i thought i should mention (although i'm sure is already known)

This configuration file is used to configure the Kubernetes cloud provider integration
when using the built-in cloud provider integration or the external cloud controller manager.

The configmap containing cloud provider config must be in `openshift-config` namespace as required by openshift/api PR 245 [1]

[1]: https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go#L28
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2019
@staebler
Copy link
Contributor Author

staebler commented Apr 3, 2019

The install-config does end up on the cluster, as config map. it might be necessary to remove this password when we store the configmap here

The vSphere usernames and passwords are now redacted from the install-config.

Add the config for vsphere to cloudproviderconfig.

Add vsphere cloud creds to cloud-creds-secret.yaml.

Add fields to vsphere platform in installconfig to support cloudproviderconfig.

Set the enable_disk_uuid attribute to true for vsphere VMs as that
is required to ensure the VMDK properly mounts disks.

See https://vmware.github.io/vsphere-storage-for-kubernetes/documentation/existing.html
for details on the vsphere cloud config file.
@staebler
Copy link
Contributor Author

staebler commented Apr 3, 2019

Updated output

$ cat > install-config.yaml << EOF
apiVersion: v1beta4
baseDomain: devcluster.openshift.com
metadata:
  name: mstaeble
networking:
  machineCIDR: "139.178.89.192/26"
platform:
  vsphere:
    virtualCenters:
    - name: vcsa.vmware.devcluster.openshift.com
      username: dummy@e2e.local
      password: MyPassword
      datacenters:
      - dc1
    workspace:
      defaultDatastore: nvme-ds1
    scsiControllerType: pvscsi
    publicNetwork: VM Network
pullSecret: redacted
sshKey: redacted
EOF

$ openshift-install create manifests
INFO Consuming "Install Config" from target directory

$ cat manifests/cloud-provider-config.yaml
apiVersion: v1
data:
  config: |+
    [Global]
    secret-name      = vsphere-creds
    secret-namespace = kube-system

    [Workspace]
    server            = vcsa.vmware.devcluster.openshift.com
    datacenter        = dc1
    default-datastore = nvme-ds1
    folder            = mstaeble

    [Disk]
    scsicontrollertype = pvscsi

    [Network]
    public-network = VM Network

    [VirtualCenter "vcsa.vmware.devcluster.openshift.com"]
    datacenters = dc1

kind: ConfigMap
metadata:
  creationTimestamp: null
  name: cloud-provider-config
  namespace: openshift-config

$ cat openshift/99_cloud-creds-secret.yaml
kind: Secret
apiVersion: v1
metadata:
  namespace: kube-system
  name: vsphere-creds
data:
  vcsa.vmware.devcluster.openshift.com.username: ZHVtbXlAZTJlLmxvY2Fs
  vcsa.vmware.devcluster.openshift.com.password: TXlQYXNzd29yZA==

@abhinavdahiya
Copy link
Contributor

/approve

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, staebler

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 [abhinavdahiya,staebler]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants