OCPBUGS-11146: update cloud provider flags on vSphere for storage options

cmd/machine-config-controller/start.go

@@ -124,6 +124,7 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle
 			ctx.InformerFactory.Machineconfiguration().V1().MachineConfigs(),
 			ctx.OpenShiftConfigKubeNamespacedInformerFactory.Core().V1().Secrets(),
 			ctx.ConfigInformerFactory.Config().V1().FeatureGates(),
+			ctx.OperatorInformerFactory.Operator().V1().Storages(),
 			ctx.ClientBuilder.KubeClientOrDie("template-controller"),
 			ctx.ClientBuilder.MachineConfigClientOrDie("template-controller"),
 		),

go.mod

@@ -27,7 +27,7 @@ require (
 	github.com/google/renameio v0.1.0
 	github.com/imdario/mergo v0.3.13
 	github.com/opencontainers/go-digest v1.0.0
-	github.com/openshift/api v0.0.0-20230221095031-69130006bb23
+	github.com/openshift/api v0.0.0-20230330150608-05635858d40f
 	github.com/openshift/client-go v0.0.0-20220831193253-4950ae70c8ea
 	github.com/openshift/library-go v0.0.0-20230112164258-24668b1349e6
 	github.com/openshift/runtime-utils v0.0.0-20220926190846-5c488b20a19f

go.sum

@@ -809,8 +809,8 @@ github.com/opencontainers/runc v1.1.4/go.mod h1:1J5XiS+vdZ3wCyZybsuxXZWGrgSr8fFJ
 github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417 h1:3snG66yBm59tKhhSPQrQ/0bCrv1LQbKt40LnUPiUxdc=
 github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
 github.com/opencontainers/selinux v1.10.0/go.mod h1:2i0OySw99QjzBBQByd1Gr9gSjvuho1lHsJxIJ3gGbJI=
-github.com/openshift/api v0.0.0-20230221095031-69130006bb23 h1:6hkSewbomhxN9+WQhT1ABANfZOJCjAzvBPSQe1OMbRs=
-github.com/openshift/api v0.0.0-20230221095031-69130006bb23/go.mod h1:ctXNyWanKEjGj8sss1KjjHQ3ENKFm33FFnS5BKaIPh4=
+github.com/openshift/api v0.0.0-20230330150608-05635858d40f h1:mGpCtfoehMcvmg/sSYLiv6nCbTl04cmtkUfYzP7H1AQ=
+github.com/openshift/api v0.0.0-20230330150608-05635858d40f/go.mod h1:ctXNyWanKEjGj8sss1KjjHQ3ENKFm33FFnS5BKaIPh4=
 github.com/openshift/client-go v0.0.0-20220831193253-4950ae70c8ea h1:7JbjIzWt3Q75ErY1PAZ+gCA+bErI6HSlpffHFmMMzqM=
 github.com/openshift/client-go v0.0.0-20220831193253-4950ae70c8ea/go.mod h1:+J8DqZC60acCdpYkwVy/KH4cudgWiFZRNOBeghCzdGA=
 github.com/openshift/library-go v0.0.0-20230112164258-24668b1349e6 h1:c0NBJDDuW1bob6E7o9L99JGUBt89iY59QJfUtwU5lXE=

manifests/machineconfigcontroller/clusterrole.yaml

@@ -55,3 +55,11 @@ rules:
   - leases
   verbs:
   - "*"
+- apiGroups:
+  - operator.openshift.io
+  resources:
+  - storages
+  verbs:
+  - get
+  - list
+  - watch

pkg/controller/bootstrap/bootstrap.go

@@ -19,6 +19,7 @@ import (
 	kscheme "k8s.io/client-go/kubernetes/scheme"
 
 	apicfgv1 "github.com/openshift/api/config/v1"
+	apioperatorsv1 "github.com/openshift/api/operator/v1"
 	apioperatorsv1alpha1 "github.com/openshift/api/operator/v1alpha1"
 	mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
 	ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
@@ -68,14 +69,16 @@ func (b *Bootstrap) Run(destDir string) error {
 
 	scheme := runtime.NewScheme()
 	mcfgv1.Install(scheme)
+	apioperatorsv1.Install(scheme)
 	apioperatorsv1alpha1.Install(scheme)
 	apicfgv1.Install(scheme)
 	codecFactory := serializer.NewCodecFactory(scheme)
-	decoder := codecFactory.UniversalDecoder(mcfgv1.GroupVersion, apioperatorsv1alpha1.GroupVersion, apicfgv1.GroupVersion)
+	decoder := codecFactory.UniversalDecoder(mcfgv1.GroupVersion, apioperatorsv1.GroupVersion, apioperatorsv1alpha1.GroupVersion, apicfgv1.GroupVersion)
 
 	var cconfig *mcfgv1.ControllerConfig
 	var featureGate *apicfgv1.FeatureGate
 	var nodeConfig *apicfgv1.Node
+	var storageConfig *apioperatorsv1.Storage
 	var kconfigs []*mcfgv1.KubeletConfig
 	var pools []*mcfgv1.MachineConfigPool
 	var configs []*mcfgv1.MachineConfig
@@ -138,6 +141,10 @@ func (b *Bootstrap) Run(destDir string) error {
 				if obj.GetName() == ctrlcommon.ClusterNodeInstanceName {
 					nodeConfig = obj
 				}
+			case *apioperatorsv1.Storage:
+				if obj.GetName() == ctrlcommon.ClusterStorageInstanceName {
+					storageConfig = obj
+				}
 			default:
 				glog.Infof("skipping %q [%d] manifest because of unhandled %T", file.Name(), idx+1, obji)
 			}
@@ -147,7 +154,7 @@ func (b *Bootstrap) Run(destDir string) error {
 	if cconfig == nil {
 		return fmt.Errorf("error: no controllerconfig found in dir: %q", destDir)
 	}
-	iconfigs, err := template.RunBootstrap(b.templatesDir, cconfig, psraw, featureGate)
+	iconfigs, err := template.RunBootstrap(b.templatesDir, cconfig, psraw, featureGate, storageConfig)
 	if err != nil {
 		return err
 	}

pkg/controller/bootstrap/testdata/bootstrap/machineconfigcontroller-controllerconfig.yaml

@@ -32,6 +32,9 @@ spec:
       controlPlaneTopology: HighlyAvailable
       platformStatus:
         type: None
+  dns:
+    spec:
+      baseDomain: domain.example.com
   kubeAPIServerServingCAData: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCktVQkUgQVBJIFNFUlZFUiBTRVJWSU5HIENBIERBVEEKLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=
   osImageURL: registry.product.example.org/ocp/4.2-DATE-VERSION@sha256:eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
   releaseImage: release-registry.product.example.org/ocp/4.2-date-version@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff

pkg/controller/common/constants.go

@@ -37,6 +37,9 @@ const (
 	// ClusterNodeInstanceName is a singleton name for node configuration
 	ClusterNodeInstanceName = "cluster"
 
+	// ClusterStorageInstanceName is a singleton name for storage configuration
+	ClusterStorageInstanceName = "cluster"
+
 	// MachineConfigPoolMaster is the MachineConfigPool name given to the master
 	MachineConfigPoolMaster = "master"
 	// MachineConfigPoolWorker is the MachineConfigPool name given to the worker

pkg/controller/template/render.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/golang/glog"
 	configv1 "github.com/openshift/api/config/v1"
+	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/library-go/pkg/cloudprovider"
 
 	mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
@@ -27,6 +28,10 @@ type RenderConfig struct {
 	PullSecret  string
 	FeatureGate *configv1.FeatureGate
 
+	// for making decision about vSphere cloud provider kubelet flag
+	// for more info see: https://issues.redhat.com/browse/STOR-1265
+	StorageConfig *operatorv1.Storage
+
 	// no need to set this, will be automatically configured
 	Constants map[string]string
 }
@@ -362,6 +367,7 @@ func renderTemplate(config RenderConfig, path string, b []byte) ([]byte, error)
 	funcs["onPremPlatformShortName"] = onPremPlatformShortName
 	funcs["urlHost"] = urlHost
 	funcs["urlPort"] = urlPort
+	funcs["vSphereCSIMigration"] = isVSphereCSIMigrationEnabled
 	funcs["isOpenShiftManagedDefaultLB"] = isOpenShiftManagedDefaultLB
 	tmpl, err := template.New(path).Funcs(funcs).Parse(string(b))
 	if err != nil {
@@ -391,9 +397,38 @@ func skipMissing(key string) (interface{}, error) {
 	return fmt.Sprintf("{{.%s}}", key), nil
 }
 
+// isCloudProviderExternal is a wrapper for the library-go IsCloudProviderExternal function, it is needed
+// to address an issue related to the vSphere in-tree storage driver and the CSI migration feature gate.
+// On vSphere, this function will check the storage operator configuration to determine if the in-tree
+// driver is in use, if it is this function will return false. Adding this function in the MCO allows
+// us to have a configuration whereby the kubelet can use the in-tree cloud controller logic to configure
+// itself, while allowing the kube-apiserver and kube-controller-manager to use the external cloud controllers.
+// For more information about the root issue, please see the following links:
+// https://github.com/kubernetes/kubernetes/pull/116342
+// https://issues.redhat.com/browse/STOR-1265
+func isCloudProviderExternal(platformStatus *configv1.PlatformStatus, featureGate *configv1.FeatureGate, storageConfig *operatorv1.Storage) (bool, error) {
+	if platformStatus == nil {
+		return false, fmt.Errorf("platformStatus is required")
+	}
+	switch platformStatus.Type {
+	case configv1.VSpherePlatformType:
+		// Platforms that are external based on feature gate presence
+		external, err := cloudprovider.IsCloudProviderExternal(platformStatus, featureGate)
+		if err != nil {
+			return external, nil
+		}
+		isexternal := external && (storageConfig == nil || storageConfig.Spec.VSphereStorageDriver == operatorv1.CSIWithMigrationDriver)
+		return isexternal, nil
+	default:
+		return cloudprovider.IsCloudProviderExternal(platformStatus, featureGate)
+	}
+}
+
 func cloudProvider(cfg RenderConfig) (interface{}, error) {
 	if cfg.Infra.Status.PlatformStatus != nil {
-		external, err := cloudprovider.IsCloudProviderExternal(cfg.Infra.Status.PlatformStatus, cfg.FeatureGate)
+		// check to see if the external cloud controller manager should be specified on the command line --cloud-provider flag,
+		// see the comments for isCloudProviderExternal for more information about the reasons for this wrapper function.
+		external, err := isCloudProviderExternal(cfg.Infra.Status.PlatformStatus, cfg.FeatureGate, cfg.StorageConfig)
 		if err != nil {
 			glog.Error(err)
 		} else if external {
@@ -438,7 +473,7 @@ func cloudConfigFlag(cfg RenderConfig) interface{} {
 		}
 	}
 
-	external, err := cloudprovider.IsCloudProviderExternal(cfg.Infra.Status.PlatformStatus, cfg.FeatureGate)
+	external, err := isCloudProviderExternal(cfg.Infra.Status.PlatformStatus, cfg.FeatureGate, cfg.StorageConfig)
 	if err != nil {
 		glog.Error(err)
 	} else if external {
@@ -708,3 +743,23 @@ func isOpenShiftManagedDefaultLB(cfg RenderConfig) bool {
 	}
 	return false
 }
+
+func isVSphereCSIMigrationEnabled(cfg RenderConfig) interface{} {
+	const enabled = "enabled"
+	const disabled = "disabled"
+
+	// The only time we expect this to be nil is during bootstrap when the
+	// Storage CR doesn't exist yet, and it should enabled for new installs.
+	if cfg.StorageConfig == nil {
+		return enabled
+	}
+
+	// If the Storage CR exists and migration is enabled, set that in the template.
+	if cfg.StorageConfig.Spec.VSphereStorageDriver == operatorv1.CSIWithMigrationDriver {
+		return enabled
+	}
+
+	// Upgraded clusters will default to disabled, until the Storage CR is modified
+	// to explicitly opt-in to the migration on vSphere.
+	return disabled
+}

pkg/controller/template/render_test.go

@@ -12,6 +12,7 @@ import (
 
 	ign3types "github.com/coreos/ignition/v2/config/v3_2/types"
 	configv1 "github.com/openshift/api/config/v1"
+	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/library-go/pkg/cloudprovider"
 	"k8s.io/client-go/kubernetes/scheme"
 
@@ -30,6 +31,7 @@ func TestCloudProvider(t *testing.T) {
 	cases := []struct {
 		platform    configv1.PlatformType
 		featureGate *configv1.FeatureGate
+		storageConf *operatorv1.Storage
 		res         string
 	}{{
 		platform:    configv1.AWSPlatformType,
@@ -50,7 +52,13 @@ func TestCloudProvider(t *testing.T) {
 	}, {
 		platform:    configv1.VSpherePlatformType,
 		featureGate: newFeatures("cluster", "CustomNoUpgrade", []string{cloudprovider.ExternalCloudProviderFeature}, nil),
+		storageConf: &operatorv1.Storage{Spec: operatorv1.StorageSpec{VSphereStorageDriver: operatorv1.CSIWithMigrationDriver}},
 		res:         "external",
+	}, {
+		platform:    configv1.VSpherePlatformType,
+		featureGate: newFeatures("cluster", "CustomNoUpgrade", []string{cloudprovider.ExternalCloudProviderFeature}, nil),
+		storageConf: &operatorv1.Storage{Spec: operatorv1.StorageSpec{VSphereStorageDriver: operatorv1.LegacyDeprecatedInTreeDriver}},
+		res:         "vsphere",
 	}, {
 		platform:    configv1.OpenStackPlatformType,
 		featureGate: newFeatures("cluster", "CustomNoUpgrade", []string{cloudprovider.ExternalCloudProviderFeature}, nil),
@@ -89,8 +97,13 @@ func TestCloudProvider(t *testing.T) {
 		platform: configv1.NonePlatformType,
 		res:      "",
 	}, {
-		platform: configv1.VSpherePlatformType,
-		res:      "external",
+		platform:    configv1.VSpherePlatformType,
+		storageConf: &operatorv1.Storage{Spec: operatorv1.StorageSpec{VSphereStorageDriver: operatorv1.CSIWithMigrationDriver}},
+		res:         "external",
+	}, {
+		platform:    configv1.VSpherePlatformType,
+		storageConf: &operatorv1.Storage{Spec: operatorv1.StorageSpec{VSphereStorageDriver: operatorv1.LegacyDeprecatedInTreeDriver}},
+		res:         "vsphere",
 	}, {
 		platform: configv1.AlibabaCloudPlatformType,
 		res:      "external",
@@ -113,7 +126,7 @@ func TestCloudProvider(t *testing.T) {
 					},
 				},
 			}
-			got, err := renderTemplate(RenderConfig{&config.Spec, `{"dummy":"dummy"}`, c.featureGate, nil}, name, dummyTemplate)
+			got, err := renderTemplate(RenderConfig{&config.Spec, `{"dummy":"dummy"}`, c.featureGate, c.storageConf, nil}, name, dummyTemplate)
 			if err != nil {
 				t.Fatalf("expected nil error %v", err)
 			}
@@ -132,6 +145,7 @@ func TestCloudConfigFlag(t *testing.T) {
 		platform    configv1.PlatformType
 		content     string
 		featureGate *configv1.FeatureGate
+		storageConf *operatorv1.Storage
 		res         string
 	}{{
 		platform: configv1.AWSPlatformType,
@@ -188,7 +202,17 @@ func TestCloudConfigFlag(t *testing.T) {
     option = a
 `,
 		featureGate: newFeatures("cluster", "CustomNoUpgrade", []string{cloudprovider.ExternalCloudProviderFeature}, nil),
+		storageConf: &operatorv1.Storage{Spec: operatorv1.StorageSpec{VSphereStorageDriver: operatorv1.CSIWithMigrationDriver}},
 		res:         "",
+	}, {
+		platform: configv1.VSpherePlatformType,
+		content: `
+[dummy-config]
+    option = a
+`,
+		featureGate: newFeatures("cluster", "CustomNoUpgrade", []string{cloudprovider.ExternalCloudProviderFeature}, nil),
+		storageConf: &operatorv1.Storage{Spec: operatorv1.StorageSpec{VSphereStorageDriver: operatorv1.LegacyDeprecatedInTreeDriver}},
+		res:         "--cloud-config=/etc/kubernetes/cloud.conf",
 	}, {
 		platform: configv1.AzurePlatformType,
 		content: `
@@ -255,7 +279,7 @@ func TestCloudConfigFlag(t *testing.T) {
 					CloudProviderConfig: c.content,
 				},
 			}
-			got, err := renderTemplate(RenderConfig{&config.Spec, `{"dummy":"dummy"}`, c.featureGate, nil}, name, dummyTemplate)
+			got, err := renderTemplate(RenderConfig{&config.Spec, `{"dummy":"dummy"}`, c.featureGate, c.storageConf, nil}, name, dummyTemplate)
 			if err != nil {
 				t.Fatalf("expected nil error %v", err)
 			}
@@ -346,14 +370,14 @@ func TestInvalidPlatform(t *testing.T) {
 
 	// we must treat unrecognized constants as "none"
 	controllerConfig.Spec.Infra.Status.PlatformStatus.Type = "_bad_"
-	_, err = generateTemplateMachineConfigs(&RenderConfig{&controllerConfig.Spec, `{"dummy":"dummy"}`, nil, nil}, templateDir)
+	_, err = generateTemplateMachineConfigs(&RenderConfig{&controllerConfig.Spec, `{"dummy":"dummy"}`, nil, nil, nil}, templateDir)
 	if err != nil {
 		t.Errorf("expect nil error, got: %v", err)
 	}
 
 	// explicitly blocked
 	controllerConfig.Spec.Infra.Status.PlatformStatus.Type = "_base"
-	_, err = generateTemplateMachineConfigs(&RenderConfig{&controllerConfig.Spec, `{"dummy":"dummy"}`, nil, nil}, templateDir)
+	_, err = generateTemplateMachineConfigs(&RenderConfig{&controllerConfig.Spec, `{"dummy":"dummy"}`, nil, nil, nil}, templateDir)
 	expectErr(err, "failed to create MachineConfig for role master: platform _base unsupported")
 }
 
@@ -364,7 +388,7 @@ func TestGenerateMachineConfigs(t *testing.T) {
 			t.Fatalf("failed to get controllerconfig config: %v", err)
 		}
 
-		cfgs, err := generateTemplateMachineConfigs(&RenderConfig{&controllerConfig.Spec, `{"dummy":"dummy"}`, nil, nil}, templateDir)
+		cfgs, err := generateTemplateMachineConfigs(&RenderConfig{&controllerConfig.Spec, `{"dummy":"dummy"}`, nil, nil, nil}, templateDir)
 		if err != nil {
 			t.Fatalf("failed to generate machine configs: %v", err)
 		}
@@ -483,7 +507,7 @@ func TestGetPaths(t *testing.T) {
 			}
 			c.res = append(c.res, platformBase)
 
-			got := getPaths(&RenderConfig{&config.Spec, `{"dummy":"dummy"}`, nil, nil}, config.Spec.Platform)
+			got := getPaths(&RenderConfig{&config.Spec, `{"dummy":"dummy"}`, nil, nil, nil}, config.Spec.Platform)
 			if reflect.DeepEqual(got, c.res) {
 				t.Fatalf("mismatch got: %s want: %s", got, c.res)
 			}