vmware-tanzu / tanzu-framework

Tanzu Framework provides a set of building blocks to build atop of the Tanzu platform and leverages Carvel packaging and plugins to provide users with a much stronger, more integrated experience than the loose coupling and stand-alone commands of the previous generation of tools.
Apache License 2.0
196 stars 193 forks source link

Config yaml supports an ipv6 address for VSPHERE_SERVER #529

Open tylerschultz opened 3 years ago

tylerschultz commented 3 years ago

(This is used to request new product features)

Describe the feature request An operator should be able to specify an ipv6 address as the value for VSPHERE_SERVER when deploying using the vSphere provider.

VSPHERE_SERVER: '[fd01:0:106:8:0:a:0:13ee]'
...

When supplying an ipv6 address as such, this error occurs:

Error: unable to set up management cluster: unable to build management cluster configuration: unable to get template:
- library.eval: Evaluating library 'addons/packages/vsphere-cpi/1.18.1/bundle/config':
    in <toplevel>
      02_addons/cpi/cpi_secret_crs.yaml:59 |   value: #@ yaml.encode(overlay.apply(vsphere_cpi_lib.with_data_values(vsphere_cpi_data_values()).eval(), update_vsphere_cpi_image(), remove_versioned_annotation()))

    reason:
     - __ytt_tpl76_set_node: cannot set non-string value ([]interface {}), consider using str(...) to convert to string
         in vsphere_conf
           vendir/vsphere_cpi/_ytt_lib/addons/packages/vsphere-cpi/1.18.1/bundle/config/vsphereconf.lib.txt:11 | =values.vsphereCPI.server
         in <toplevel>
           vendir/vsphere_cpi/_ytt_lib/addons/packages/vsphere-cpi/1.18.1/bundle/config/overlays/update-config.yaml:13 |   vsphere.conf: #@ vsphere_conf(values)

Some preliminary digging around suggests that the ip address is marshalled and unmarshalled, and along the way the quotes surrounding the address are lost. Later processing assumes the value is a yaml array.

Describe alternatives you've considered

Affected product area (please put an X in all that apply)

Additional context

vuil commented 3 years ago

@mcwumbly Quick glance looks like the square-brackets is tripping up yaml interpretation (like there is a list assigned instead of a string). Is this something we need to support?

mcwumbly commented 3 years ago

@vuil It may not be the highest priority, but it'd be valuable in the following scenarios:

  1. user doesn't otherwise need to configure DNS for their vsphere server
  2. avoids requiring bootstrap docker daemon configuration for DNS server: https://github.com/vmware-tanzu/tanzu-framework/issues/106

We tried working around this by using the form https://[$ipv6addr]:443, but that runs into some other issue later with cert-manager.

christianang commented 3 years ago

We continued some digging into the issues when setting the VSPHERE_SERVER to an ipv6 address (e.g [fd01:0:106:8:0:a:0:13ee]). The issues we encountered so far are:

  1. When the tanzu cli tries to get the vcenter thumbprint it attempts to split the host on colon, which fails on ipv6 addresses: https://github.com/vmware-tanzu/tanzu-framework/blob/main/pkg/v1/tkg/vc/helpers.go#L139. This should probably be changed to use the native Go net.SplitHostPort.

  2. If the yaml processing error Tyler mentioned is worked around, the next component to fail is the vsphere-cloud-controller-manager on the management cluster with the following error:

    E0831 17:01:13.552164       1 connection.go:177] Failed to create new client. err: Post "https://[[fd01:0:106:8:0:a:0:13ee]]:443/sdk": dial tcp: address [[fd01:0:106:8:0:a:0:13ee]]:443: missing port in address

    We can see that the vsphere-cloud-controller-manager does not expect brackets around the IPv6 address supplied to it.

  3. Additionally, we also see the capi-controller-manager on the bootstrap cluster fail to reconcile with this error:

    E0831 17:02:48.207025       1 controller.go:257] controller-runtime/controller "msg"="Reconciler error" "error"="failed to create object /v1, Kind=Secret kube-system/cloud-provider-vsphere-credentials: Secret \"cloud-provider-vsphere-credentials\" is invalid: [data[[fd01:0:106:8:0:a:0:13ee].password]: Invalid value: \"[fd01:0:106:8:0:a:0:13ee].password\": a valid config key must consist of alphanumeric characters, '-', '_' or '.' (e.g. 'key.name',  or 'KEY_NAME',  or 'key-name', regex used for validation is '[-._a-zA-Z0-9]+'), data[[fd01:0:106:8:0:a:0:13ee].username]: Invalid value: \"[fd01:0:106:8:0:a:0:13ee].username\": a valid config key must consist of alphanumeric characters, '-', '_' or '.' (e.g. 'key.name',  or 'KEY_NAME',  or 'key-name', regex used for validation is '[-._a-zA-Z0-9]+')]" "controller"="clusterresourceset" "name"="tkg-mgmt-vsphere-20210831094656-vsphere-cpi" "namespace"="tkg-system"

    We can see that the IPv6 address is being used as some sort of key in a hashmap and the regex is rejecting it as a key because it contains both square brackets and colons.

We stopped our spike here, so there may be more issues to fix after these.

mcwumbly commented 3 years ago

As @christianang mentioned, we've decided to put this aside for now. Capturing a few more thoughts for whomever takes a look at this next (perhaps us in the future, but perhaps someone else will be more motivated and get to it first)!

  1. It appears the the following forms are currently supported: ${hostname} or ${ipv4addr}
  2. It appears that some code may expect the following to work with either ${scheme}:// or :${port} included, but none of them do: ${hostname}:${port} ${scheme}://${hostname}:${port}, ${ipv4addr}:${port}, ${scheme}://${ipv4addr}:${port} - it may be worth adding support for these or adding validation to make the lack of support more explicit
  3. If we decide not to support scheme and/or port, perhaps we don't care about [] around the ipv6 addr.
  4. It may be possible to work around the issue in vccm by ommitting or stripping the brackets.
  5. It may be possible to work around the issue in capi by hashing or otherwise encoding the value we use as a key to only use expected characters. Not sure if that'd be the way to go, but worth considering.