Merge pull request #299 from mkowalski/OCPBUGS-23308

OCPBUGS-23308: Set IPFamilyPriority and ExcludeNetworkSubnetCIDR for vSphere IPv6-only

Author: openshift-merge-bot[bot]

pkg/cloud/vsphere/vsphere_config_transformer.go

@@ -43,7 +43,8 @@ func CloudConfigTransformer(source string, infra *configv1.Infrastructure, netwo
 	// 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)
+		setIPFamilies(cpiCfg, infra.Status.PlatformStatus.VSphere, &infra.Spec.PlatformSpec.VSphere.NodeNetworking, network)
+		setExcludeNetworkSubnetCIDR(cpiCfg, infra.Status.PlatformStatus.VSphere, &infra.Spec.PlatformSpec.VSphere.NodeNetworking, network)
 		setNodes(cpiCfg, &infra.Spec.PlatformSpec.VSphere.NodeNetworking)
 		setVirtualCenters(cpiCfg, infra.Spec.PlatformSpec.VSphere)
 
@@ -102,31 +103,68 @@ func setVirtualCenters(cfg *ccmConfig.CPIConfig, vSphereSpec *configv1.VSpherePl
 	}
 }
 
-// setDualStack updates the configuration required by the cloud-provider-vsphere to explicitly set
+// setIPFamilies 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 use Service Networks as a way to determine IP stack of the cluster as this field is already
+// well-defined and validated by o/installer in the following way
 //
-// 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).
+//   - for single Service Network, its IP stack determines IP stack of the cluster
+//   - for 2 entries in Service Network list, cluster is a dual-stack cluster; order of networks
+//     determines order of IP stacks for the cluster
+//   - number of subnets in Service Network list must be 1 or 2
 //
 // 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 {
+func setIPFamilies(cfg *ccmConfig.CPIConfig, status *configv1.VSpherePlatformStatus, nodeNetworking *configv1.VSpherePlatformNodeNetworking, network *configv1.Network) {
+	if network != nil {
 		// 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 len(network.Spec.ServiceNetwork) == 1 {
+			if net.IsIPv6CIDRString(network.Spec.ServiceNetwork[0]) {
+				cfg.Global.IPFamilyPriority = []string{"ipv6"}
+			}
+			return
+		}
+		if len(network.Spec.ServiceNetwork) == 2 {
+			if net.IsIPv4CIDRString(network.Spec.ServiceNetwork[0]) {
+				cfg.Global.IPFamilyPriority = []string{"ipv4", "ipv6"}
+			} else {
+				cfg.Global.IPFamilyPriority = []string{"ipv6", "ipv4"}
+			}
+			return
+		}
+	}
+}
+
+// setExcludeNetworkSubnetCIDR updates ExcludeNetworkSubnetCIDR param because VM agent by default
+// uses IP addresses that are used by 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).
+//
+// For comaptibility reasons we are running this only for IPv6-only and dual-stack clusters. We
+// could run it also for IPv4-only clusters for completeness but this issue was never observed in
+// those, so to avoid any potential regression we are not changing IPv4-only setups
+//
+// Ref.: https://issues.redhat.com/browse/OCPBUGS-18641
+func setExcludeNetworkSubnetCIDR(cfg *ccmConfig.CPIConfig, status *configv1.VSpherePlatformStatus, nodeNetworking *configv1.VSpherePlatformNodeNetworking, network *configv1.Network) {
+	ipv6 := false
+	if network != nil {
+		for _, addr := range network.Spec.ServiceNetwork {
+			if net.IsIPv6CIDRString(addr) {
+				ipv6 = true
+				break
+			}
+		}
+
+		// If none of Service Networks is an IPv6 subnet then the cluster is IPv4-only then we do
+		// not change any configuration. We simply stop and remaning code will run only for dual-stack
+		// and IPv6-only setups.
+		if !ipv6 {
+			return
 		}
 
 		if status != nil {

pkg/cloud/vsphere/vsphere_config_transformer_test.go

@@ -57,7 +57,7 @@ func (b infraBuilder) withPlatform(platform configv1.PlatformType) infraBuilder
 	return b
 }
 
-func (b infraBuilder) withVSphereNodeNetworking() infraBuilder {
+func (b infraBuilder) withVSphereDefaultNodeNetworking() infraBuilder {
 	vspereSpecRef := b.platformSpec.VSphere
 
 	vspereSpecRef.NodeNetworking.External.Network = "external-network"
@@ -72,6 +72,21 @@ func (b infraBuilder) withVSphereNodeNetworking() infraBuilder {
 	return b
 }
 
+func (b infraBuilder) withVSphereIPv6onlyNodeNetworking() infraBuilder {
+	vspereSpecRef := b.platformSpec.VSphere
+
+	vspereSpecRef.NodeNetworking.External.Network = "external-network"
+	vspereSpecRef.NodeNetworking.Internal.Network = "internal-network"
+
+	vspereSpecRef.NodeNetworking.External.NetworkSubnetCIDR = []string{"fe80::3/128"}
+	vspereSpecRef.NodeNetworking.External.ExcludeNetworkSubnetCIDR = []string{"fe80::2/128"}
+
+	vspereSpecRef.NodeNetworking.Internal.ExcludeNetworkSubnetCIDR = []string{"fe80::1/128"}
+	vspereSpecRef.NodeNetworking.Internal.NetworkSubnetCIDR = []string{"fe80::4/128"}
+
+	return b
+}
+
 func (b infraBuilder) withVSphereZones() infraBuilder {
 	vcenterSpec := configv1.VSpherePlatformVCenterSpec{
 		Server:      "test-server",
@@ -139,6 +154,12 @@ func (b infraBuilder) withPrimaryIPv6VIP() infraBuilder {
 	return b
 }
 
+func (b infraBuilder) withIPv6onlyVIP() infraBuilder {
+	b.platformStatus.VSphere.APIServerInternalIPs = []string{"fd65:a1a8:60ad:271c::200"}
+	b.platformStatus.VSphere.IngressIPs = []string{"fd65:a1a8:60ad:271c::201"}
+	return b
+}
+
 func makeDummyNetworkConfig() *configv1.Network {
 	return &configv1.Network{}
 }
@@ -175,6 +196,20 @@ func withDualStackPrimaryIPv6NetworkConfig() *configv1.Network {
 	}
 }
 
+func withIPv6onlyNetworkConfig() *configv1.Network {
+	return &configv1.Network{
+		Spec: configv1.NetworkSpec{
+			ClusterNetwork: []configv1.ClusterNetworkEntry{
+				{CIDR: "fd65:10:128::/56", HostPrefix: 64},
+			},
+			NetworkType: "OVNKubernetes",
+			ServiceNetwork: []string{
+				"fd65:172:16::/112",
+			},
+		},
+	}
+}
+
 const iniConfigWithWorkspace = `
 [Global]
 secret-name = "vsphere-creds"
@@ -300,6 +335,26 @@ nodes:
   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 yamlConfigNodeNetworkingIPv6only = `
+global:
+  insecureFlag: true
+  secretName: vsphere-creds
+  secretNamespace: kube-system
+vcenter:
+  test-server:
+    server: test-server
+    datacenters:
+    - DC1
+    ipFamily:
+    - ipv6
+nodes:
+  internalNetworkSubnetCidr: fe80::4/128
+  externalNetworkSubnetCidr: fe80::3/128
+  internalVmNetworkName: internal-network
+  externalVmNetworkName: external-network
+  excludeInternalNetworkSubnetCidr: fe80::1/128,fd65:a1a8:60ad:271c::200/128,fd65:a1a8:60ad:271c::201/128,fd69::2/128
+  excludeExternalNetworkSubnetCidr: fe80::2/128,fd65:a1a8:60ad:271c::200/128,fd65:a1a8:60ad:271c::201/128,fd69::2/128`
+
 const iniConfigZonal = `
 [Global]
 secret-name = "vsphere-creds"
@@ -348,7 +403,7 @@ func TestCloudConfigTransformer(t *testing.T) {
 		},
 		{
 			name:             "in-tree to external with node networking",
-			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking(),
+			infraBuilder:     newVsphereInfraBuilder().withVSphereDefaultNodeNetworking(),
 			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      iniConfigWithWorkspace,
 			equivalentConfig: iniConfigNodeNetworking,
@@ -383,14 +438,14 @@ func TestCloudConfigTransformer(t *testing.T) {
 		},
 		{
 			name:             "yaml and ini config parsing results should be the same, node networking",
-			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking(),
+			infraBuilder:     newVsphereInfraBuilder().withVSphereDefaultNodeNetworking(),
 			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      yamlConfigNodeNetworking,
 			equivalentConfig: iniConfigNodeNetworking,
 		},
 		{
 			name:             "yaml config should contain node networking if it's specified in infra",
-			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking(),
+			infraBuilder:     newVsphereInfraBuilder().withVSphereDefaultNodeNetworking(),
 			networkBuilder:   makeDummyNetworkConfig(),
 			inputConfig:      yamlConfig,
 			equivalentConfig: yamlConfigNodeNetworking,
@@ -404,18 +459,25 @@ func TestCloudConfigTransformer(t *testing.T) {
 		},
 		{
 			name:             "yaml config should contain ipv4-primary dual-stack config and correct excluded subnets",
-			infraBuilder:     newVsphereInfraBuilder().withVSphereNodeNetworking().withPrimaryIPv4VIP(),
+			infraBuilder:     newVsphereInfraBuilder().withVSphereDefaultNodeNetworking().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(),
+			infraBuilder:     newVsphereInfraBuilder().withVSphereDefaultNodeNetworking().withPrimaryIPv6VIP(),
 			networkBuilder:   withDualStackPrimaryIPv6NetworkConfig(),
 			inputConfig:      yamlConfig,
 			equivalentConfig: yamlConfigNodeNetworkingDualStackPrimaryIPv6,
 		},
+		{
+			name:             "yaml config should contain ipv6-only config and correct excluded subnets",
+			infraBuilder:     newVsphereInfraBuilder().withVSphereIPv6onlyNodeNetworking().withIPv6onlyVIP(),
+			networkBuilder:   withIPv6onlyNetworkConfig(),
+			inputConfig:      yamlConfig,
+			equivalentConfig: yamlConfigNodeNetworkingIPv6only,
+		},
 		{
 			name:           "empty input",
 			infraBuilder:   newVsphereInfraBuilder(),