From 8fc114163c7433057c292fc1d8c90a02c10f6e00 Mon Sep 17 00:00:00 2001 From: Muhammad Adil Ghaffar Date: Fri, 22 Nov 2024 03:10:25 +0200 Subject: [PATCH] Deprecate NoCloudProvider and add CloudProviderEnabled Signed-off-by: Muhammad Adil Ghaffar --- api/v1beta1/metal3cluster_types.go | 11 +++- api/v1beta1/metal3cluster_webhook.go | 64 +++++++++++++++++-- api/v1beta1/zz_generated.deepcopy.go | 18 ++++-- baremetal/metal3machine_manager.go | 25 ++++++-- baremetal/metal3machine_manager_test.go | 8 +-- ...cture.cluster.x-k8s.io_metal3clusters.yaml | 9 +++ ...uster.x-k8s.io_metal3clustertemplates.yaml | 9 +++ controllers/metal3cluster_controller.go | 15 +++++ controllers/suite_test.go | 4 +- docs/api.md | 12 +++- docs/architecture.md | 6 +- examples/cluster/cluster.yaml | 3 +- .../clusterctl-cluster.yaml | 3 +- examples/templates/clusterclass.yaml | 3 +- .../bases/cluster/cluster-with-kcp.yaml | 3 +- .../cluster-with-kcp.yaml | 3 +- 16 files changed, 166 insertions(+), 30 deletions(-) diff --git a/api/v1beta1/metal3cluster_types.go b/api/v1beta1/metal3cluster_types.go index e91db43b8c..824461c9ea 100644 --- 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 index c7aa82218f..bf5328736b 100644 --- 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,12 +43,17 @@ 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. @@ -55,7 +61,7 @@ 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 index caaa375c60..5b7f91cafd 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -358,7 +358,7 @@ func (in *Metal3Cluster) DeepCopyInto(out *Metal3Cluster) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) in.Status.DeepCopyInto(&out.Status) } @@ -416,6 +416,16 @@ func (in *Metal3ClusterList) DeepCopyObject() runtime.Object { func (in *Metal3ClusterSpec) DeepCopyInto(out *Metal3ClusterSpec) { *out = *in out.ControlPlaneEndpoint = in.ControlPlaneEndpoint + if in.NoCloudProvider != nil { + in, out := &in.NoCloudProvider, &out.NoCloudProvider + *out = new(bool) + **out = **in + } + if in.CloudProviderEnabled != nil { + in, out := &in.CloudProviderEnabled, &out.CloudProviderEnabled + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Metal3ClusterSpec. @@ -469,7 +479,7 @@ func (in *Metal3ClusterTemplate) DeepCopyInto(out *Metal3ClusterTemplate) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Metal3ClusterTemplate. @@ -525,7 +535,7 @@ func (in *Metal3ClusterTemplateList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Metal3ClusterTemplateResource) DeepCopyInto(out *Metal3ClusterTemplateResource) { *out = *in - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Metal3ClusterTemplateResource. @@ -541,7 +551,7 @@ func (in *Metal3ClusterTemplateResource) DeepCopy() *Metal3ClusterTemplateResour // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Metal3ClusterTemplateSpec) DeepCopyInto(out *Metal3ClusterTemplateSpec) { *out = *in - out.Template = in.Template + in.Template.DeepCopyInto(&out.Template) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Metal3ClusterTemplateSpec. diff --git a/baremetal/metal3machine_manager.go b/baremetal/metal3machine_manager.go index 72cd44a9f4..f6a5f801a8 100644 --- 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 index 9228b0ec46..5a62d64841 100644 --- 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 index 6f7d8e7f92..770feb143e 100644 --- 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 index 405b81984e..990089f780 100644 --- 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 index 264ecb7223..2147a4440b 100644 --- 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 index df39b2e1a8..13b4f03e6d 100644 --- 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 index c6963c512e..d38eb9672e 100644 --- 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 index a0378b358d..7e40d86a10 100644 --- 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 index fbd991681e..9f62061582 100644 --- 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 \ No newline at end of file + 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 index 8381c30def..89904ca601 100644 --- 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 index 5c52be1eb9..1afce0eaf6 100644 --- 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 diff --git a/test/e2e/data/infrastructure-metal3/bases/cluster/cluster-with-kcp.yaml b/test/e2e/data/infrastructure-metal3/bases/cluster/cluster-with-kcp.yaml index 50fb65f226..5906da5902 100644 --- a/test/e2e/data/infrastructure-metal3/bases/cluster/cluster-with-kcp.yaml +++ b/test/e2e/data/infrastructure-metal3/bases/cluster/cluster-with-kcp.yaml @@ -29,7 +29,8 @@ spec: controlPlaneEndpoint: host: ${CLUSTER_APIENDPOINT_HOST} port: ${CLUSTER_APIENDPOINT_PORT} - noCloudProvider: true + noCloudProvider: true # This field is deprecated use cloudProviderEnabled instead + cloudProviderEnabled: false --- apiVersion: controlplane.cluster.x-k8s.io/${CAPI_VERSION} kind: KubeadmControlPlane diff --git a/test/e2e/data/infrastructure-metal3/bases/clusterclass-cluster/cluster-with-kcp.yaml b/test/e2e/data/infrastructure-metal3/bases/clusterclass-cluster/cluster-with-kcp.yaml index 620ffef6a3..8ff5e740c5 100644 --- a/test/e2e/data/infrastructure-metal3/bases/clusterclass-cluster/cluster-with-kcp.yaml +++ b/test/e2e/data/infrastructure-metal3/bases/clusterclass-cluster/cluster-with-kcp.yaml @@ -15,7 +15,8 @@ spec: controlPlaneEndpoint: host: ${CLUSTER_APIENDPOINT_HOST} port: ${CLUSTER_APIENDPOINT_PORT} - noCloudProvider: true + noCloudProvider: true # This field is deprecated use cloudProviderEnabled instead + cloudProviderEnabled: false --- apiVersion: infrastructure.cluster.x-k8s.io/${CAPM3_VERSION} kind: Metal3MachineTemplate