Deprecate NoCloudProvider and add CloudProviderEnabled

Signed-off-by: Muhammad Adil Ghaffar <[email protected]>
metal3-io · Jan 21, 2025 · 8fc1141 · 8fc1141
1 parent c9d6f72
commit 8fc1141
Show file tree

Hide file tree

Showing 16 changed files with 166 additions and 30 deletions.
diff --git a/api/v1beta1/metal3cluster_types.go b/api/v1beta1/metal3cluster_types.go
@@ -37,8 +37,17 @@ type Metal3ClusterSpec struct {
 	// Determines if the cluster is not to be deployed with an external cloud provider.
 	// If set to true, CAPM3 will use node labels to set providerID on the kubernetes nodes.
 	// If set to false, providerID is set on nodes by other entities and CAPM3 uses the value of the providerID on the m3m resource.
+	//
+	// Deprecated: This field is deprecated use cloudProviderEnabled instead
+	//
 	// +optional
-	NoCloudProvider bool `json:"noCloudProvider,omitempty"`
+	NoCloudProvider *bool `json:"noCloudProvider,omitempty"`
+	// Determines if the cluster is to be deployed with an external cloud provider.
+	// If set to false, CAPM3 will use node labels to set providerID on the kubernetes nodes.
+	// If set to true, providerID is set on nodes by other entities and CAPM3 uses the value of the providerID on the m3m resource.
+	// +kubebuilder:default=true
+	// +optional
+	CloudProviderEnabled *bool `json:"cloudProviderEnabled,omitempty"`
 }
 
 // IsValid returns an error if the object is not valid, otherwise nil. The

diff --git a/api/v1beta1/metal3cluster_webhook.go b/api/v1beta1/metal3cluster_webhook.go
@@ -14,6 +14,7 @@ limitations under the License.
 package v1beta1
 
 import (
+	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -42,20 +43,25 @@ func (c *Metal3Cluster) Default() {
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
 func (c *Metal3Cluster) ValidateCreate() (admission.Warnings, error) {
-	return nil, c.validate()
+	return nil, c.validate(nil)
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (c *Metal3Cluster) ValidateUpdate(_ runtime.Object) (admission.Warnings, error) {
-	return nil, c.validate()
+func (c *Metal3Cluster) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
+	oldM3C, ok := old.(*Metal3Cluster)
+	if !ok || oldM3C == nil {
+		return nil, apierrors.NewInternalError(errors.New("unable to convert existing object"))
+	}
+
+	return nil, c.validate(oldM3C)
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
 func (c *Metal3Cluster) ValidateDelete() (admission.Warnings, error) {
 	return nil, nil
 }
 
-func (c *Metal3Cluster) validate() error {
+func (c *Metal3Cluster) validate(oldM3C *Metal3Cluster) error {
 	var allErrs field.ErrorList
 	if c.Spec.ControlPlaneEndpoint.Host == "" {
 		allErrs = append(
@@ -68,6 +74,56 @@ func (c *Metal3Cluster) validate() error {
 		)
 	}
 
+	if oldM3C != nil {
+		// Validate cloudProviderEnabled
+		if c.Spec.CloudProviderEnabled != nil && oldM3C.Spec.NoCloudProvider != nil {
+			if !*c.Spec.CloudProviderEnabled && !*oldM3C.Spec.NoCloudProvider {
+				allErrs = append(
+					allErrs,
+					field.Invalid(
+						field.NewPath("spec", "cloudProviderEnabled"),
+						c.Spec.CloudProviderEnabled,
+						"cloudProviderEnabled conflicts the value of noCloudProvider",
+					),
+				)
+			}
+			if *c.Spec.CloudProviderEnabled && *oldM3C.Spec.NoCloudProvider {
+				allErrs = append(
+					allErrs,
+					field.Invalid(
+						field.NewPath("spec", "cloudProviderEnabled"),
+						c.Spec.CloudProviderEnabled,
+						"cloudProviderEnabled conflicts the value of noCloudProvider",
+					),
+				)
+			}
+		}
+
+		// Validate noCloudProvider
+		if c.Spec.NoCloudProvider != nil && oldM3C.Spec.CloudProviderEnabled != nil {
+			if !*c.Spec.NoCloudProvider && !*oldM3C.Spec.CloudProviderEnabled {
+				allErrs = append(
+					allErrs,
+					field.Invalid(
+						field.NewPath("spec", "noCloudProvider"),
+						c.Spec.CloudProviderEnabled,
+						"noCloudProvider conflicts the value of cloudProviderEnabled",
+					),
+				)
+			}
+			if *c.Spec.NoCloudProvider && *oldM3C.Spec.CloudProviderEnabled {
+				allErrs = append(
+					allErrs,
+					field.Invalid(
+						field.NewPath("spec", "noCloudProvider"),
+						c.Spec.CloudProviderEnabled,
+						"noCloudProvider conflicts the value of cloudProviderEnabled",
+					),
+				)
+			}
+		}
+	}
+
 	if len(allErrs) == 0 {
 		return nil
 	}

diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/baremetal/metal3machine_manager.go b/baremetal/metal3machine_manager.go
@@ -1295,13 +1295,26 @@ func (m *MachineManager) SetNodeProviderID(ctx context.Context, providerIDOnM3M
 		m.Log.Info(errMessage)
 		return WithTransientError(errors.New(errMessage), requeueAfter)
 	}
-	if !m.Metal3Cluster.Spec.NoCloudProvider && matchingNodesCount == 0 {
-		// The node could either be still running cloud-init or
-		// kubernetes has not set the node.spec.ProviderID field yet.
-		errMessage := "Some target nodes do not have spec.providerID field set yet, requeuing"
-		m.Log.Info(errMessage)
-		return WithTransientError(errors.New(errMessage), requeueAfter)
+
+	if m.Metal3Cluster.Spec.NoCloudProvider != nil {
+		if !*m.Metal3Cluster.Spec.NoCloudProvider && matchingNodesCount == 0 {
+			// The node could either be still running cloud-init or
+			// kubernetes has not set the node.spec.ProviderID field yet.
+			errMessage := "Some target nodes do not have spec.providerID field set yet, requeuing"
+			m.Log.Info(errMessage)
+			return WithTransientError(errors.New(errMessage), requeueAfter)
+		}
 	}
+	if m.Metal3Cluster.Spec.CloudProviderEnabled != nil {
+		if *m.Metal3Cluster.Spec.CloudProviderEnabled && matchingNodesCount == 0 {
+			// The node could either be still running cloud-init or
+			// kubernetes has not set the node.spec.ProviderID field yet.
+			errMessage := "Some target nodes do not have spec.providerID field set yet, requeuing"
+			m.Log.Info(errMessage)
+			return WithTransientError(errors.New(errMessage), requeueAfter)
+		}
+	}
+
 	if matchingNodesCount == 1 {
 		return nil
 	}

diff --git a/baremetal/metal3machine_manager_test.go b/baremetal/metal3machine_manager_test.go
@@ -2627,14 +2627,14 @@ var _ = Describe("Metal3Machine manager", func() {
 
 				machineMgr, err := NewMachineManager(fakeClient, newCluster(clusterName),
 					newMetal3Cluster(metal3ClusterName, bmcOwnerRef,
-						&infrav1.Metal3ClusterSpec{NoCloudProvider: true}, nil,
+						&infrav1.Metal3ClusterSpec{NoCloudProvider: ptr.To(true), CloudProviderEnabled: ptr.To(false)}, nil,
 					),
 					&clusterv1.Machine{}, &infrav1.Metal3Machine{}, logr.Discard(),
 				)
 				if tc.M3MHasHostAnnotation {
 					machineMgr, err = NewMachineManager(fakeClient, newCluster(clusterName),
 						newMetal3Cluster(metal3ClusterName, bmcOwnerRef,
-							&infrav1.Metal3ClusterSpec{NoCloudProvider: true}, nil,
+							&infrav1.Metal3ClusterSpec{NoCloudProvider: ptr.To(true), CloudProviderEnabled: ptr.To(false)}, nil,
 						),
 						&clusterv1.Machine{}, &infrav1.Metal3Machine{
 							ObjectMeta: metav1.ObjectMeta{
@@ -2731,7 +2731,7 @@ var _ = Describe("Metal3Machine manager", func() {
 				M3MHasHostAnnotation: true,
 			}),
 		)
-		DescribeTable("Test SetNodeProviderID with noCloudProvider set to false",
+		DescribeTable("Test SetNodeProviderID with CloudProviderEnabled set to true",
 			func(tc testCaseSetNodePoviderID) {
 				BMHHost := newBareMetalHost(baremetalhostName, nil, bmov1alpha1.StateNone, nil, false, "metadata", false, tc.HostID)
 				fakeClient := fake.NewClientBuilder().WithScheme(s).WithObjects(BMHHost).Build()
@@ -2744,7 +2744,7 @@ var _ = Describe("Metal3Machine manager", func() {
 
 				machineMgr, err := NewMachineManager(fakeClient, newCluster(clusterName),
 					newMetal3Cluster(metal3ClusterName, bmcOwnerRef,
-						&infrav1.Metal3ClusterSpec{NoCloudProvider: false}, nil,
+						&infrav1.Metal3ClusterSpec{NoCloudProvider: ptr.To(false), CloudProviderEnabled: ptr.To(true)}, nil,
 					),
 					&clusterv1.Machine{}, &infrav1.Metal3Machine{
 						ObjectMeta: metav1.ObjectMeta{

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_metal3clusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_metal3clusters.yaml
@@ -68,6 +68,13 @@ spec:
           spec:
             description: Metal3ClusterSpec defines the desired state of Metal3Cluster.
             properties:
+              cloudProviderEnabled:
+                default: true
+                description: |-
+                  Determines if the cluster is to be deployed with an external cloud provider.
+                  If set to false, CAPM3 will use node labels to set providerID on the kubernetes nodes.
+                  If set to true, providerID is set on nodes by other entities and CAPM3 uses the value of the providerID on the m3m resource.
+                type: boolean
               controlPlaneEndpoint:
                 description: ControlPlaneEndpoint represents the endpoint used to
                   communicate with the control plane.
@@ -87,6 +94,8 @@ spec:
                   Determines if the cluster is not to be deployed with an external cloud provider.
                   If set to true, CAPM3 will use node labels to set providerID on the kubernetes nodes.
                   If set to false, providerID is set on nodes by other entities and CAPM3 uses the value of the providerID on the m3m resource.
+
+                  Deprecated: This field is deprecated use cloudProviderEnabled instead
                 type: boolean
             type: object
           status:

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_metal3clustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_metal3clustertemplates.yaml
@@ -51,6 +51,13 @@ spec:
                   spec:
                     description: Metal3ClusterSpec defines the desired state of Metal3Cluster.
                     properties:
+                      cloudProviderEnabled:
+                        default: true
+                        description: |-
+                          Determines if the cluster is to be deployed with an external cloud provider.
+                          If set to false, CAPM3 will use node labels to set providerID on the kubernetes nodes.
+                          If set to true, providerID is set on nodes by other entities and CAPM3 uses the value of the providerID on the m3m resource.
+                        type: boolean
                       controlPlaneEndpoint:
                         description: ControlPlaneEndpoint represents the endpoint
                           used to communicate with the control plane.
@@ -72,6 +79,8 @@ spec:
                           Determines if the cluster is not to be deployed with an external cloud provider.
                           If set to true, CAPM3 will use node labels to set providerID on the kubernetes nodes.
                           If set to false, providerID is set on nodes by other entities and CAPM3 uses the value of the providerID on the m3m resource.
+
+                          Deprecated: This field is deprecated use cloudProviderEnabled instead
                         type: boolean
                     type: object
                 required:

diff --git a/controllers/metal3cluster_controller.go b/controllers/metal3cluster_controller.go
@@ -80,6 +80,21 @@ func (r *Metal3ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, err
 	}
 
+	// This is checking if default values are changed or not if the default
+	// value of CloudProviderEnabled or NoCloudProvider is changed then update
+	// the other value too to avoid conflicts.
+	// TODO: Remove this code after v1.10 when NoCloudProvider is completely
+	// removed. Ref: https://github.com/metal3-io/cluster-api-provider-metal3/issues/2255
+	if metal3Cluster.Spec.CloudProviderEnabled != nil {
+		if !*metal3Cluster.Spec.CloudProviderEnabled {
+			*metal3Cluster.Spec.NoCloudProvider = true
+		}
+	} else if metal3Cluster.Spec.NoCloudProvider != nil {
+		if *metal3Cluster.Spec.NoCloudProvider {
+			*metal3Cluster.Spec.CloudProviderEnabled = false
+		}
+	}
+
 	patchHelper, err := patch.NewHelper(metal3Cluster, r.Client)
 	if err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "failed to init patch helper")

diff --git a/controllers/suite_test.go b/controllers/suite_test.go
@@ -33,6 +33,7 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"k8s.io/klog/v2"
+	"k8s.io/utils/ptr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
@@ -168,7 +169,8 @@ func bmcSpec() *infrav1.Metal3ClusterSpec {
 			Host: "192.168.111.249",
 			Port: 6443,
 		},
-		NoCloudProvider: true,
+		NoCloudProvider:      ptr.To(true),
+		CloudProviderEnabled: ptr.To(false),
 	}
 }
 

diff --git a/docs/api.md b/docs/api.md
@@ -80,8 +80,13 @@ cluster on Baremetal. It currently has two specification fields :
 
 - **controlPlaneEndpoint**: contains the target cluster API server address and
   port
-- **noCloudProvider**: (true/false) Whether the cluster will not be deployed
-  with an external cloud provider. If set to true, CAPM3 will patch the target
+- **noCloudProvider(Deprecated use CloudProviderEnabled)**: (true/false) Whether
+  the cluster will not be deployed with an external cloud provider. If set to
+  true, CAPM3 will patch the target cluster node objects to add a providerID.
+  This will allow the CAPI process to continue even if the cluster is deployed
+  without cloud provider.
+- **CloudProviderEnabled**: (true/false) Whether the cluster will be deployed
+  with an external cloud provider. If set to false, CAPM3 will patch the target
   cluster node objects to add a providerID. This will allow the CAPI process to
   continue even if the cluster is deployed without cloud provider.
 
@@ -97,7 +102,8 @@ spec:
   controlPlaneEndpoint:
     host: 192.168.111.249
     port: 6443
-  noCloudProvider: true
+  noCloudProvider: true  # This field is deprecated use cloudProviderEnabled instead
+  cloudProviderEnabled: false
 ```
 
 ## KubeadmControlPlane

diff --git a/docs/architecture.md b/docs/architecture.md
@@ -121,7 +121,8 @@ metadata:
   name: test1
 spec:
   apiEndpoint: https://192.168.111.249:6443
-  noCloudProvider: true
+  noCloudProvider: true  # This field is deprecated use cloudProviderEnabled instead
+  cloudProviderEnabled: false
 ```
 
 Metal3Cluster, after reconciliation
@@ -142,7 +143,8 @@ metadata:
   |----------------------------------------------------------------------------|
 spec:
   apiEndpoint: https://192.168.111.249:6443
-  noCloudProvider: true
+  noCloudProvider: true  # This field is deprecated use cloudProviderEnabled instead
+  cloudProviderEnabled: false
 status:
   apiEndpoints:
   - host: 192.168.111.249

diff --git a/examples/cluster/cluster.yaml b/examples/cluster/cluster.yaml
@@ -33,4 +33,5 @@ spec:
   controlPlaneEndpoint:
     host: ${CLUSTER_APIENDPOINT_HOST}
     port: ${CLUSTER_APIENDPOINT_PORT}
-  noCloudProvider: true
+  noCloudProvider: true # This field is deprecated use cloudProviderEnabled instead
+  cloudProviderEnabled: false
diff --git a/examples/clusterctl-templates/clusterctl-cluster.yaml b/examples/clusterctl-templates/clusterctl-cluster.yaml
@@ -30,7 +30,8 @@ spec:
   controlPlaneEndpoint:
     host: ${CLUSTER_APIENDPOINT_HOST}
     port: ${CLUSTER_APIENDPOINT_PORT}
-  noCloudProvider: true
+  noCloudProvider: true # This field is deprecated use cloudProviderEnabled instead
+  cloudProviderEnabled: false
 ---
 kind: KubeadmControlPlane
 apiVersion: controlplane.cluster.x-k8s.io/v1beta1

diff --git a/examples/templates/clusterclass.yaml b/examples/templates/clusterclass.yaml
@@ -9,7 +9,8 @@ spec:
       controlPlaneEndpoint:
         host: 127.0.0.1
         port: 6443
-      noCloudProvider: true
+        noCloudProvider: true # This field is deprecated use cloudProviderEnabled instead
+        cloudProviderEnabled: false
 ---
 apiVersion: controlplane.cluster.x-k8s.io/v1beta1
 kind: KubeadmControlPlaneTemplate