OCPBUGS-18641: Set dual-stack IPFamilyPriority for vSphere

We have discovered that in dual-stack setups NodeAddresses field of
the instance metadata contains only IPv4 addresses for VMs that do have
both IPv4 and IPv6 addresses assigned (and detected by the VM agent).

It has been traced back to the function responsible for populating this
metadata field. We found out that for our configuration we always filter
only IPv4 addresses even if running in dual-stack. Reason for that is
that `IPFamilyPriority` has a value of `ipv4` even when running in a
dual-stack setup.

This causes an issue because this instance metadata is cross-checked
with addresses provided by the kubelet as part of the
`alpha.kubernetes.io/provided-node-ip` annotation. Without correct value
of `IPFamilyPriority` we are thus removing all the IPv6 addresses.

This PR takes advantage of the Service Networks configured by the user in
install-config and the fact that we only allow 2 networks to be configured
if the setup is dual-stack.

Fixes: OCPBUGS-18641

Author: mkowalski

pkg/cloud/vsphere/assets/cloud-controller-manager-deployment.yaml

@@ -66,6 +66,8 @@ spec:
               value: {{ .globalCredsSecretNamespace }}
             - name: VSPHERE_SECRET_NAME
               value: {{ .globalCredsSecretName }}
+            - name: ENABLE_ALPHA_DUAL_STACK
+              value: "true"
           resources:
             requests:
               cpu: 200m

pkg/cloud/vsphere/vsphere_config_transformer.go

@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	configv1 "github.com/openshift/api/config/v1"
+	"k8s.io/utils/net"
 
 	ccmConfig "github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/vsphere/vsphere_cloud_config"
 )
@@ -26,7 +27,7 @@ const (
 // Currently, CloudConfigTransformer is responsible to populate vcenters, labels, and node networking parameters from
 // the Infrastructure resource.
 // Also, this function converts legacy deprecated INI configuration format to a YAML-based one.
-func CloudConfigTransformer(source string, infra *configv1.Infrastructure, _ *configv1.Network) (string, error) {
+func CloudConfigTransformer(source string, infra *configv1.Infrastructure, network *configv1.Network) (string, error) {
 	if infra.Status.PlatformStatus == nil ||
 		infra.Status.PlatformStatus.Type != configv1.VSpherePlatformType {
 		return "", fmt.Errorf("invalid platform, expected to be %s", configv1.VSpherePlatformType)
@@ -42,6 +43,7 @@ func CloudConfigTransformer(source string, infra *configv1.Infrastructure, _ *co
 	// https://github.com/openshift/enhancements/blob/f6b33eb0cd4ba060af71fee6192297cf6bc31e5a/enhancements/installer/vsphere-ipi-zonal.md
 	// https://github.com/openshift/api/pull/1278
 	if infra.Spec.PlatformSpec.VSphere != nil {
+		setDualStack(cpiCfg, infra.Status.PlatformStatus.VSphere, &infra.Spec.PlatformSpec.VSphere.NodeNetworking, network)
 		setNodes(cpiCfg, &infra.Spec.PlatformSpec.VSphere.NodeNetworking)
 		setVirtualCenters(cpiCfg, infra.Spec.PlatformSpec.VSphere)
 
@@ -99,3 +101,47 @@ func setVirtualCenters(cfg *ccmConfig.CPIConfig, vSphereSpec *configv1.VSpherePl
 		}
 	}
 }
+
+// setDualStack updates the configuration required by the cloud-provider-vsphere to explicitly set
+// value of IPFamilyPriority instead of using the default which is IPv4. This is needed by the
+// cloud provider in order to properly filter IP addresses that feed the instance metadata.
+//
+// We rely on the Service Networks configuration that initially comes from o/installer and later
+// from the Cluster Network Operator as those two components take care of validating that clusters
+// with dual-stack configuration have exactly 2 of them and that they match the required order.
+//
+// We are mangling with the ExcludeNetworkSubnetCIDR param here because VM agent by default detects
+// also IP addresses that are used by us internally and which should never be exposed as node IPs
+// (i.e. API VIP and Ingress VIP for IPI installations and fd69::2 which is internal to OVN-K8s).
+//
+// Ref.: https://issues.redhat.com/browse/OCPBUGS-18641
+func setDualStack(cfg *ccmConfig.CPIConfig, status *configv1.VSpherePlatformStatus, nodeNetworking *configv1.VSpherePlatformNodeNetworking, network *configv1.Network) {
+	if network != nil && len(network.Spec.ServiceNetwork) == 2 {
+		// Extensive validations are performed by o/installer so that here we already know that
+		// if the configuration is dual-stack, we will have exactly 2 service networks and if
+		// single-stack then 1 service network. Simplified logic here is applied to avoid code
+		// duplication.
+		//
+		// Ref.: https://github.com/openshift/installer/blob/6471b31/pkg/types/validation/installconfig.go#L241
+		if net.IsIPv4CIDRString(network.Spec.ServiceNetwork[0]) {
+			cfg.Global.IPFamilyPriority = []string{"ipv4", "ipv6"}
+		} else {
+			cfg.Global.IPFamilyPriority = []string{"ipv6", "ipv4"}
+		}
+
+		if status != nil {
+			for _, addr := range append(status.APIServerInternalIPs, status.IngressIPs...) {
+				if net.IsIPv4String(addr) {
+					addr = addr + "/32"
+				} else {
+					addr = addr + "/128"
+				}
+				nodeNetworking.External.ExcludeNetworkSubnetCIDR = append(nodeNetworking.External.ExcludeNetworkSubnetCIDR, addr)
+				nodeNetworking.Internal.ExcludeNetworkSubnetCIDR = append(nodeNetworking.Internal.ExcludeNetworkSubnetCIDR, addr)
+			}
+		}
+
+		nodeNetworking.External.ExcludeNetworkSubnetCIDR = append(nodeNetworking.External.ExcludeNetworkSubnetCIDR, "fd69::2/128")
+		nodeNetworking.Internal.ExcludeNetworkSubnetCIDR = append(nodeNetworking.Internal.ExcludeNetworkSubnetCIDR, "fd69::2/128")
+	}
+}

pkg/cloud/vsphere/vsphere_config_transformer_test.go

@@ -18,17 +18,20 @@ const (
 
 func newVsphereInfraBuilder() infraBuilder {
 	return infraBuilder{
-		platform: configv1.VSpherePlatformType,
 		platformSpec: configv1.PlatformSpec{
 			Type:    configv1.VSpherePlatformType,
 			VSphere: &configv1.VSpherePlatformSpec{},
 		},
+		platformStatus: configv1.PlatformStatus{
+			Type:    configv1.VSpherePlatformType,
+			VSphere: &configv1.VSpherePlatformStatus{},
+		},
 	}
 }
 
 type infraBuilder struct {
-	platform     configv1.PlatformType
-	platformSpec configv1.PlatformSpec
+	platformSpec   configv1.PlatformSpec
+	platformStatus configv1.PlatformStatus
 }
 
 func (b infraBuilder) Build() *configv1.Infrastructure {
@@ -37,9 +40,7 @@ func (b infraBuilder) Build() *configv1.Infrastructure {
 			Name: "cluster",
 		},
 		Status: configv1.InfrastructureStatus{
-			PlatformStatus: &configv1.PlatformStatus{
-				Type: b.platform,
-			},
+			PlatformStatus: &b.platformStatus,
 		},
 		Spec: configv1.InfrastructureSpec{
 			CloudConfig: configv1.ConfigMapFileReference{
@@ -52,7 +53,7 @@ func (b infraBuilder) Build() *configv1.Infrastructure {
 }
 
 func (b infraBuilder) withPlatform(platform configv1.PlatformType) infraBuilder {
-	b.platform = platform
+	b.platformStatus.Type = platform
 	return b
 }
 
@@ -126,10 +127,54 @@ func (b infraBuilder) withVSphereZones() infraBuilder {
 	return b
 }
 
+func (b infraBuilder) withPrimaryIPv4VIP() infraBuilder {
+	b.platformStatus.VSphere.APIServerInternalIPs = []string{"192.168.96.3", "fd65:a1a8:60ad:271c::200"}
+	b.platformStatus.VSphere.IngressIPs = []string{"192.168.96.4", "fd65:a1a8:60ad:271c::201"}
+	return b
+}
+
+func (b infraBuilder) withPrimaryIPv6VIP() infraBuilder {
+	b.platformStatus.VSphere.APIServerInternalIPs = []string{"fd65:a1a8:60ad:271c::200", "192.168.96.3"}
+	b.platformStatus.VSphere.IngressIPs = []string{"fd65:a1a8:60ad:271c::201", "192.168.96.4"}
+	return b
+}
+
 func makeDummyNetworkConfig() *configv1.Network {
 	return &configv1.Network{}
 }
 
+func withDualStackPrimaryIPv4NetworkConfig() *configv1.Network {
+	return &configv1.Network{
+		Spec: configv1.NetworkSpec{
+			ClusterNetwork: []configv1.ClusterNetworkEntry{
+				{CIDR: "10.128.0.0/14", HostPrefix: 14},
+				{CIDR: "fd65:10:128::/56", HostPrefix: 64},
+			},
+			NetworkType: "OVNKubernetes",
+			ServiceNetwork: []string{
+				"172.30.0.0/16",
+				"fd65:172:16::/112",
+			},
+		},
+	}
+}
+
+func withDualStackPrimaryIPv6NetworkConfig() *configv1.Network {
+	return &configv1.Network{
+		Spec: configv1.NetworkSpec{
+			ClusterNetwork: []configv1.ClusterNetworkEntry{
+				{CIDR: "fd65:10:128::/56", HostPrefix: 64},
+				{CIDR: "10.128.0.0/14", HostPrefix: 14},
+			},
+			NetworkType: "OVNKubernetes",
+			ServiceNetwork: []string{
+				"fd65:172:16::/112",
+				"172.30.0.0/16",
+			},
+		},
+	}
+}
+
 const iniConfigWithWorkspace = `
 [Global]
 secret-name = "vsphere-creds"
@@ -213,6 +258,48 @@ nodes:
   excludeInternalNetworkSubnetCidr: 192.0.2.0/24,fe80::1/128
   excludeExternalNetworkSubnetCidr: 192.1.2.0/24,fe80::2/128`
 
+const yamlConfigNodeNetworkingDualStackPrimaryIPv4 = `
+global:
+  insecureFlag: true
+  secretName: vsphere-creds
+  secretNamespace: kube-system
+vcenter:
+  test-server:
+    server: test-server
+    datacenters:
+    - DC1
+    ipFamily:
+    - ipv4
+    - ipv6
+nodes:
+  internalNetworkSubnetCidr: 192.0.3.0/24,fe80::4/128
+  externalNetworkSubnetCidr: 198.51.100.0/24,fe80::3/128
+  internalVmNetworkName: internal-network
+  externalVmNetworkName: external-network
+  excludeInternalNetworkSubnetCidr: 192.0.2.0/24,fe80::1/128,192.168.96.3/32,fd65:a1a8:60ad:271c::200/128,192.168.96.4/32,fd65:a1a8:60ad:271c::201/128,fd69::2/128
+  excludeExternalNetworkSubnetCidr: 192.1.2.0/24,fe80::2/128,192.168.96.3/32,fd65:a1a8:60ad:271c::200/128,192.168.96.4/32,fd65:a1a8:60ad:271c::201/128,fd69::2/128`
+
+const yamlConfigNodeNetworkingDualStackPrimaryIPv6 = `
+global:
+  insecureFlag: true
+  secretName: vsphere-creds
+  secretNamespace: kube-system
+vcenter:
+  test-server:
+    server: test-server
+    datacenters:
+    - DC1
+    ipFamily:
+    - ipv6
+    - ipv4
+nodes:
+  internalNetworkSubnetCidr: 192.0.3.0/24,fe80::4/128
+  externalNetworkSubnetCidr: 198.51.100.0/24,fe80::3/128
+  internalVmNetworkName: internal-network
+  externalVmNetworkName: external-network
+  excludeInternalNetworkSubnetCidr: 192.0.2.0/24,fe80::1/128,fd65:a1a8:60ad:271c::200/128,192.168.96.3/32,fd65:a1a8:60ad:271c::201/128,192.168.96.4/32,fd69::2/128
+  excludeExternalNetworkSubnetCidr: 192.1.2.0/24,fe80::2/128,fd65:a1a8:60ad:271c::200/128,192.168.96.3/32,fd65:a1a8:60ad:271c::201/128,192.168.96.4/32,fd69::2/128`
+
 const iniConfigZonal = `
 [Global]
 secret-name = "vsphere-creds"
@@ -247,87 +334,114 @@ func TestCloudConfigTransformer(t *testing.T) {
 	testcases := []struct {
 		name             string
 		infraBuilder     infraBuilder
+		networkBuilder   *configv1.Network
 		inputConfig      string
 		equivalentConfig string
 		errMsg           string
 	}{
 		{
 			name:             "in-tree to external with empty infra",
 			infraBuilder:     newVsphereInfraBuilder(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      iniConfigWithWorkspace,
 			equivalentConfig: iniConfigWithoutWorkspace,
 		},
 		{
 			name:             "in-tree to external with node networking",
 			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      iniConfigWithWorkspace,
 			equivalentConfig: iniConfigNodeNetworking,
 		},
 		{
 			name:             "populating labels datacenters from zones config",
 			infraBuilder:     newVsphereInfraBuilder().withVSphereZones(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      iniConfigWithWorkspace,
 			equivalentConfig: iniConfigZonal,
 		},
 		{
 			name:             "replacing existing labels with openshift specific",
 			infraBuilder:     newVsphereInfraBuilder().withVSphereZones(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      iniConfigWithExistingLabels,
 			equivalentConfig: iniConfigZonal,
 		},
 		{
 			name:             "yaml and ini config parsing results should be the same",
 			infraBuilder:     newVsphereInfraBuilder(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      yamlConfig,
 			equivalentConfig: iniConfigWithoutWorkspace,
 		},
 		{
 			name:             "yaml and ini config parsing results should be the same, with zones",
 			infraBuilder:     newVsphereInfraBuilder().withVSphereZones(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      yamlConfigZonal,
 			equivalentConfig: iniConfigZonal,
 		},
 		{
 			name:             "yaml and ini config parsing results should be the same, node networking",
 			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      yamlConfigNodeNetworking,
 			equivalentConfig: iniConfigNodeNetworking,
 		},
 		{
 			name:             "yaml config should contain node networking if it's specified in infra",
 			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      yamlConfig,
 			equivalentConfig: yamlConfigNodeNetworking,
 		},
 		{
 			name:             "yaml config should be populated with datacenters and labels if failure domains specified",
 			infraBuilder:     newVsphereInfraBuilder().withVSphereZones(),
+			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      yamlConfig,
 			equivalentConfig: yamlConfigZonal,
 		},
 		{
-			name:         "empty input",
-			infraBuilder: newVsphereInfraBuilder(),
-			errMsg:       "failed to read the cloud.conf: vSphere config is empty",
+			name:             "yaml config should contain ipv4-primary dual-stack config and correct excluded subnets",
+			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking().withPrimaryIPv4VIP(),
+			networkBuilder:   withDualStackPrimaryIPv4NetworkConfig(),
+			inputConfig:      yamlConfig,
+			equivalentConfig: yamlConfigNodeNetworkingDualStackPrimaryIPv4,
+		},
+		{
+			name:             "yaml config should contain ipv6-primary dual-stack config and correct excluded subnets",
+			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking().withPrimaryIPv6VIP(),
+			networkBuilder:   withDualStackPrimaryIPv6NetworkConfig(),
+			inputConfig:      yamlConfig,
+			equivalentConfig: yamlConfigNodeNetworkingDualStackPrimaryIPv6,
+		},
+		{
+			name:           "empty input",
+			infraBuilder:   newVsphereInfraBuilder(),
+			networkBuilder: makeDummyNetworkConfig(),
+			errMsg:         "failed to read the cloud.conf: vSphere config is empty",
 		},
 		{
-			name:         "incorrect platform",
-			infraBuilder: newVsphereInfraBuilder().withPlatform(configv1.NonePlatformType),
-			errMsg:       "invalid platform, expected to be VSphere",
+			name:           "incorrect platform",
+			infraBuilder:   newVsphereInfraBuilder().withPlatform(configv1.NonePlatformType),
+			networkBuilder: makeDummyNetworkConfig(),
+			errMsg:         "invalid platform, expected to be VSphere",
 		},
 		{
-			name:         "invalid ini input",
-			infraBuilder: newVsphereInfraBuilder(),
-			inputConfig:  ":",
-			errMsg:       "failed to read the cloud.conf",
+			name:           "invalid ini input",
+			infraBuilder:   newVsphereInfraBuilder(),
+			networkBuilder: makeDummyNetworkConfig(),
+			inputConfig:    ":",
+			errMsg:         "failed to read the cloud.conf",
 		},
 	}
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
 			g := gmg.NewWithT(t)
 			infraResouce := tc.infraBuilder.Build()
-			transformedConfig, err := CloudConfigTransformer(tc.inputConfig, infraResouce, makeDummyNetworkConfig())
+			transformedConfig, err := CloudConfigTransformer(tc.inputConfig, infraResouce, tc.networkBuilder)
 			if tc.errMsg != "" {
 				g.Expect(err).To(gmg.MatchError(gmg.ContainSubstring(tc.errMsg)))
 				return