WIP: Alternate approach without use of global constants for DualStack support

Author: sadasu

pkg/controller/clusterconfig/clusterconfig_controller.go

@@ -27,13 +27,13 @@ import (
 )
 
 // and Start it when the Manager is Started.
-func Add(mgr manager.Manager, status *statusmanager.StatusManager, c cnoclient.Client, _ featuregates.FeatureGate) error {
-	return add(mgr, newReconciler(mgr, status, c))
+func Add(mgr manager.Manager, status *statusmanager.StatusManager, c cnoclient.Client, featureGates featuregates.FeatureGate) error {
+	return add(mgr, newReconciler(mgr, status, c, featureGates))
 }
 
 // newReconciler returns a new reconcile.Reconciler
-func newReconciler(mgr manager.Manager, status *statusmanager.StatusManager, c cnoclient.Client) reconcile.Reconciler {
-	return &ReconcileClusterConfig{client: c, scheme: mgr.GetScheme(), status: status}
+func newReconciler(mgr manager.Manager, status *statusmanager.StatusManager, c cnoclient.Client, featureGates featuregates.FeatureGate) reconcile.Reconciler {
+	return &ReconcileClusterConfig{client: c, scheme: mgr.GetScheme(), status: status, featureGates: featureGates}
 }
 
 // add adds a new Controller to mgr with r as the reconcile.Reconciler
@@ -57,9 +57,10 @@ var _ reconcile.Reconciler = &ReconcileClusterConfig{}
 
 // ReconcileClusterConfig reconciles a cluster Network object
 type ReconcileClusterConfig struct {
-	client cnoclient.Client
-	scheme *runtime.Scheme
-	status *statusmanager.StatusManager
+	client       cnoclient.Client
+	scheme       *runtime.Scheme
+	status       *statusmanager.StatusManager
+	featureGates featuregates.FeatureGate
 }
 
 // Reconcile propagates changes from the cluster config to the operator config.
@@ -92,7 +93,7 @@ func (r *ReconcileClusterConfig) Reconcile(ctx context.Context, request reconcil
 	}
 
 	// Validate the cluster config
-	if err := network.ValidateClusterConfig(clusterConfig, r.client); err != nil {
+	if err := network.ValidateClusterConfig(clusterConfig, r.client, r.featureGates); err != nil {
 		log.Printf("Failed to validate Network CR: %v", err)
 		r.status.SetDegraded(statusmanager.ClusterConfig, "InvalidClusterConfig",
 			fmt.Sprintf("The cluster configuration is invalid (%v). Use 'oc edit network.config.openshift.io cluster' to fix.", err))

pkg/controller/operconfig/cluster.go

@@ -31,7 +31,7 @@ func (r *ReconcileOperConfig) MergeClusterConfig(ctx context.Context, operConfig
 	}
 	// Validate cluster config
 	// If invalid just warn and proceed.
-	if err := network.ValidateClusterConfig(clusterConfig, r.client); err != nil {
+	if err := network.ValidateClusterConfig(clusterConfig, r.client, r.featureGates); err != nil {
 		log.Printf("WARNING: ignoring Network.config.openshift.io/v1/cluster - failed validation: %v", err)
 		return nil
 	}

pkg/network/cluster_config.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"net"
-	"strings"
 	"sync"
 
 	configv1 "github.com/openshift/api/config/v1"
@@ -15,6 +14,7 @@ import (
 	"github.com/openshift/cluster-network-operator/pkg/names"
 	"github.com/openshift/cluster-network-operator/pkg/platform"
 	iputil "github.com/openshift/cluster-network-operator/pkg/util/ip"
+	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -32,16 +32,16 @@ import (
 var pluginsUsingHostPrefix = sets.NewString(string(operv1.NetworkTypeOpenShiftSDN), string(operv1.NetworkTypeOVNKubernetes))
 
 // ValidateClusterConfig ensures the cluster config is valid.
-func ValidateClusterConfig(clusterConfig *configv1.Network, client cnoclient.Client) error {
+func ValidateClusterConfig(clusterConfig *configv1.Network, client cnoclient.Client, featureGates featuregates.FeatureGate) error {
 	// If for whatever reason it is not possible to get the platform type, fail
 	infraRes, err := platform.InfraStatus(client)
 	if err != nil {
 		return err
 	}
-	return validateClusterConfig(clusterConfig, infraRes, client)
+	return validateClusterConfig(clusterConfig, infraRes, client, featureGates)
 }
 
-func validateClusterConfig(clusterConfig *configv1.Network, infraRes *bootstrap.InfraStatus, client cnoclient.Client) error {
+func validateClusterConfig(clusterConfig *configv1.Network, infraRes *bootstrap.InfraStatus, client cnoclient.Client, featureGates featuregates.FeatureGate) error {
 
 	// Check all networks for overlaps
 	pool := iputil.IPPool{}
@@ -117,12 +117,9 @@ func validateClusterConfig(clusterConfig *configv1.Network, infraRes *bootstrap.
 		return errors.Errorf("spec.networkType is required")
 	}
 
-	// Validate that this is either a BareMetal or None PlatformType. For all other
-	// PlatformTypes, migration to DualStack is prohibited
 	if ipv4Service && ipv6Service || ipv4Cluster && ipv6Cluster {
-		if !isSupportedDualStackPlatform(infraRes.PlatformType) {
-			return errors.Errorf("%s is not one of the supported platforms for dual stack (%s)", infraRes.PlatformType,
-				strings.Join(dualStackPlatforms.List(), ", "))
+		if !isSupportedDualStackPlatform(infraRes.PlatformType, featureGates) {
+			return errors.Errorf("%s is not one of the supported platforms for dual stack", infraRes.PlatformType)
 		}
 	}
 

pkg/network/cluster_config_test.go

@@ -2,10 +2,10 @@ package network
 
 import (
 	"fmt"
-	"strings"
 	"testing"
 
 	configv1 "github.com/openshift/api/config/v1"
+	apifeatures "github.com/openshift/api/features"
 	v1 "github.com/openshift/api/network/v1"
 	operv1 "github.com/openshift/api/operator/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -17,6 +17,7 @@ import (
 	"github.com/openshift/cluster-network-operator/pkg/client/fake"
 	"github.com/openshift/cluster-network-operator/pkg/hypershift"
 	"github.com/openshift/cluster-network-operator/pkg/names"
+	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 
 	. "github.com/onsi/gomega"
 )
@@ -37,6 +38,20 @@ var ClusterConfig = configv1.NetworkSpec{
 	NetworkType: "None",
 }
 
+func getFeatureGatesWithDualStack() featuregates.FeatureGate {
+	return featuregates.NewFeatureGate(
+		[]configv1.FeatureGateName{apifeatures.FeatureGateAdminNetworkPolicy,
+			apifeatures.FeatureGateDNSNameResolver,
+			apifeatures.FeatureGateNetworkSegmentation,
+			apifeatures.FeatureGateOVNObservability,
+			apifeatures.FeatureGateAWSDualStackInstall,
+			apifeatures.FeatureGateAzureDualStackInstall},
+		[]configv1.FeatureGateName{
+			apifeatures.FeatureGatePreconfiguredUDNAddresses,
+		},
+	)
+}
+
 func TestValidateClusterConfig(t *testing.T) {
 	g := NewGomegaWithT(t)
 
@@ -57,12 +72,13 @@ func TestValidateClusterConfig(t *testing.T) {
 	g.Expect(err).NotTo(HaveOccurred())
 
 	cc := *ClusterConfig.DeepCopy()
-	err = ValidateClusterConfig(&configv1.Network{Spec: cc}, client)
+	featureGates := getFeatureGatesWithDualStack()
+	err = ValidateClusterConfig(&configv1.Network{Spec: cc}, client, featureGates)
 	g.Expect(err).NotTo(HaveOccurred())
 
 	haveError := func(cfg configv1.NetworkSpec, substr string) {
 		t.Helper()
-		err = ValidateClusterConfig(&configv1.Network{Spec: cfg}, client)
+		err = ValidateClusterConfig(&configv1.Network{Spec: cfg}, client, featureGates)
 		g.Expect(err).To(MatchError(ContainSubstring(substr)))
 	}
 
@@ -88,7 +104,7 @@ func TestValidateClusterConfig(t *testing.T) {
 
 	cc = *ClusterConfig.DeepCopy()
 	cc.ClusterNetwork[1].HostPrefix = 0
-	res := ValidateClusterConfig(&configv1.Network{Spec: cc}, client)
+	res := ValidateClusterConfig(&configv1.Network{Spec: cc}, client, featureGates)
 	// Since the NetworkType is None, and the hostprefix is unset we don't validate it
 	g.Expect(res).Should(BeNil())
 
@@ -289,7 +305,8 @@ func TestValidClusterConfigLiveMigration(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			client := fake.NewFakeClient(tt.objects...)
-			err := validateClusterConfig(tt.config, tt.infraRes, client)
+			fg := getFeatureGatesWithDualStack()
+			err := validateClusterConfig(tt.config, tt.infraRes, client, fg)
 			if tt.expectErr {
 				g.Expect(err).To(HaveOccurred())
 				if tt.expectedErrorMsg != "" {
@@ -323,12 +340,13 @@ func TestValidateClusterConfigDualStack(t *testing.T) {
 	g.Expect(err).NotTo(HaveOccurred())
 
 	cc := *ClusterConfig.DeepCopy()
-	err = ValidateClusterConfig(&configv1.Network{Spec: cc}, client)
+	featureGates := getFeatureGatesWithDualStack()
+	err = ValidateClusterConfig(&configv1.Network{Spec: cc}, client, featureGates)
 	g.Expect(err).NotTo(HaveOccurred())
 
 	haveError := func(cfg configv1.NetworkSpec, substr string) {
 		t.Helper()
-		err = ValidateClusterConfig(&configv1.Network{Spec: cc}, client)
+		err = ValidateClusterConfig(&configv1.Network{Spec: cc}, client, featureGates)
 		g.Expect(err).To(MatchError(ContainSubstring(substr)))
 	}
 
@@ -368,11 +386,14 @@ func TestValidateClusterConfigDualStack(t *testing.T) {
 		CIDR:       "fd01::/48",
 		HostPrefix: 64,
 	})
-	err = ValidateClusterConfig(&configv1.Network{Spec: cc}, client)
+	err = ValidateClusterConfig(&configv1.Network{Spec: cc}, client, featureGates)
 	g.Expect(err).NotTo(HaveOccurred())
 
-	// You can't use dual-stack if this is anything else but BareMetal or NonePlatformType
-	infrastructure.Status.PlatformStatus.Type = configv1.AzurePlatformType
+	// You can't use dual-stack if enabled on an unsupported platform
+	infrastructure.Status.PlatformStatus.Type = configv1.GCPPlatformType
+	infrastructure.Status.PlatformStatus.GCP = &configv1.GCPPlatformStatus{
+		Region: "us-west1",
+	}
 	client = fake.NewFakeClient(infrastructure)
 	err = createProxy(client)
 	g.Expect(err).NotTo(HaveOccurred())
@@ -382,8 +403,8 @@ func TestValidateClusterConfigDualStack(t *testing.T) {
 		CIDR:       "fd01::/48",
 		HostPrefix: 64,
 	})
-	haveError(cc, fmt.Sprintf("%s is not one of the supported platforms for dual stack (%s)",
-		infrastructure.Status.PlatformStatus.Type, strings.Join(dualStackPlatforms.List(), ", ")))
+	haveError(cc, fmt.Sprintf("%s is not one of the supported platforms for dual stack",
+		infrastructure.Status.PlatformStatus.Type))
 }
 
 func TestMergeClusterConfig(t *testing.T) {

pkg/network/render.go

@@ -31,13 +31,6 @@ import (
 	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 )
 
-var dualStackPlatforms = sets.NewString(
-	string(configv1.BareMetalPlatformType),
-	string(configv1.NonePlatformType),
-	string(configv1.VSpherePlatformType),
-	string(configv1.OpenStackPlatformType),
-)
-
 const (
 	pluginName = "networking-console-plugin"
 )
@@ -403,10 +396,7 @@ func isNetworkChangeSafe(prev, next *operv1.NetworkSpec, infraRes *bootstrap.Inf
 	// Validate that this is either a BareMetal or None PlatformType. For all other
 	// PlatformTypes, migration to DualStack is prohibited
 	if len(prev.ServiceNetwork) < len(next.ServiceNetwork) {
-		if !isSupportedDualStackPlatform(infraRes.PlatformType) {
-			return errors.Errorf("%s is not one of the supported platforms for dual stack (%s)", infraRes.PlatformType,
-				strings.Join(dualStackPlatforms.List(), ", "))
-		} else if string(configv1.OpenStackPlatformType) == string(infraRes.PlatformType) {
+		if !isConversionToDualStackSupported(infraRes.PlatformType) {
 			return errors.Errorf("%s does not allow conversion to dual-stack cluster", infraRes.PlatformType)
 		}
 	}
@@ -973,10 +963,6 @@ func registerNetworkingConsolePlugin(bootstrapResult *bootstrap.BootstrapResult,
 	})
 }
 
-func isSupportedDualStackPlatform(platformType configv1.PlatformType) bool {
-	return dualStackPlatforms.Has(string(platformType))
-}
-
 func renderAdditionalRoutingCapabilities(conf *operv1.NetworkSpec, manifestDir string) ([]*uns.Unstructured, error) {
 	if conf == nil || conf.AdditionalRoutingCapabilities == nil {
 		return nil, nil
@@ -999,3 +985,25 @@ func renderAdditionalRoutingCapabilities(conf *operv1.NetworkSpec, manifestDir s
 
 	return out, nil
 }
+
+func isSupportedDualStackPlatform(platformType configv1.PlatformType, featureGates featuregates.FeatureGate) bool {
+	switch platformType {
+	case configv1.BareMetalPlatformType, configv1.NonePlatformType, configv1.VSpherePlatformType, configv1.OpenStackPlatformType:
+		return true
+	case configv1.AWSPlatformType:
+		return featureGates.Enabled(apifeatures.FeatureGateAWSDualStackInstall)
+	case configv1.AzurePlatformType:
+		return featureGates.Enabled(apifeatures.FeatureGateAzureDualStackInstall)
+	default:
+		return false
+	}
+}
+
+func isConversionToDualStackSupported(platformType configv1.PlatformType) bool {
+	switch platformType {
+	case configv1.BareMetalPlatformType, configv1.NonePlatformType, configv1.VSpherePlatformType:
+		return true
+	default:
+		return false
+	}
+}

pkg/network/render_test.go

@@ -3,17 +3,18 @@ package network
 import (
 	"fmt"
 	"reflect"
-	"strings"
 
 	. "github.com/onsi/gomega"
 	"github.com/openshift/cluster-network-operator/pkg/client/fake"
 	"github.com/openshift/cluster-network-operator/pkg/hypershift"
+	"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
 	"github.com/stretchr/testify/assert"
 	"k8s.io/client-go/kubernetes/scheme"
 
 	"testing"
 
 	configv1 "github.com/openshift/api/config/v1"
+	apifeatures "github.com/openshift/api/features"
 	operv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/cluster-network-operator/pkg/bootstrap"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -233,7 +234,7 @@ func TestDisallowMultipleClusterNetworksOfOldIPFamily(t *testing.T) {
 	g.Expect(err).To(MatchError(ContainSubstring("cannot add additional ClusterNetwork values of original IP family when migrating to dual stack")))
 }
 
-func TestAllowMigrationOnlyForBareMetalOrNoneType(t *testing.T) {
+func TestAllowMigrationOnlyForSupportedTypes(t *testing.T) {
 	g, infra, prev, next := setupTestInfraAndBasicRenderConfigs(t, OVNKubernetesConfig, OVNKubernetesConfig)
 
 	// You can't migrate from single-stack to dual-stack if this is anything else but
@@ -246,9 +247,12 @@ func TestAllowMigrationOnlyForBareMetalOrNoneType(t *testing.T) {
 		HostPrefix: 64,
 	},
 	)
+	// You can't migrate from single-stack to dual-stack if this is anything else but
+	// BareMetal, NonePlatformType, and VSphere
+	infra.PlatformType = configv1.GCPPlatformType
+
 	err := IsChangeSafe(prev, next, infra)
-	g.Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("%s is not one of the supported platforms for dual stack (%s)", infra.PlatformType,
-		strings.Join(dualStackPlatforms.List(), ", ")))))
+	g.Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("%s does not allow conversion to dual-stack cluster", infra.PlatformType))))
 	// ... but the migration in the other direction should work
 	err = IsChangeSafe(next, prev, infra)
 	g.Expect(err).NotTo(HaveOccurred())
@@ -349,6 +353,20 @@ func TestDisallowCIDRMaskChangeInDualStackUpdate(t *testing.T) {
 	g.Expect(err).To(MatchError(ContainSubstring("cannot add additional ClusterNetwork values of original IP family when migrating to dual stack")))
 }
 
+func getDefaultFeatureGatesWithDualStack() featuregates.FeatureGate {
+	return featuregates.NewFeatureGate(
+		[]configv1.FeatureGateName{apifeatures.FeatureGateAdminNetworkPolicy,
+			apifeatures.FeatureGateDNSNameResolver,
+			apifeatures.FeatureGateNetworkSegmentation,
+			apifeatures.FeatureGateOVNObservability,
+			apifeatures.FeatureGateAWSDualStackInstall,
+			apifeatures.FeatureGateAzureDualStackInstall},
+		[]configv1.FeatureGateName{
+			apifeatures.FeatureGatePreconfiguredUDNAddresses,
+		},
+	)
+}
+
 func TestRenderUnknownNetwork(t *testing.T) {
 	g := NewGomegaWithT(t)
 
@@ -400,7 +418,9 @@ func TestRenderUnknownNetwork(t *testing.T) {
 	bootstrapResult, err := Bootstrap(&config, client)
 	g.Expect(err).NotTo(HaveOccurred())
 
-	objs, _, err := Render(prev, &configv1.NetworkSpec{}, manifestDir, client, getDefaultFeatureGates(), bootstrapResult)
+	featureGatesCNO := getDefaultFeatureGatesWithDualStack()
+
+	objs, _, err := Render(prev, &configv1.NetworkSpec{}, manifestDir, client, featureGatesCNO, bootstrapResult)
 	g.Expect(err).NotTo(HaveOccurred())
 
 	// Validate that openshift-sdn isn't rendered