Skip to content

Commit

Permalink
Merge pull request #2108 from Nordix/deprecate-noCloudProvider/adil
Browse files Browse the repository at this point in the history 
✨  Deprecate NoCloudProvider and add CloudProviderEnabled
  • Loading branch information
@metal3-io-bot
metal3-io-bot authored Jan 23, 2025
2 parents 4cb222b + 5e733bd commit b4480cf
Show file tree
Hide file tree
Showing 19 changed files with 287 additions and 46 deletions.
7 changes: 7 additions & 0 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ help: # Display this help
## --------------------------------------
##@ tests:

export KUBEBUILDER_ENVTEST_KUBERNETES_VERSION ?= 1.32.0
KUBEBUILDER_ASSETS ?= $(shell $(SETUP_ENVTEST) use --use-env -p path $(KUBEBUILDER_ENVTEST_KUBERNETES_VERSION))

.PHONY: setup-envtest
setup-envtest: $(SETUP_ENVTEST) ## Set up envtest (download kubebuilder assets)
@echo KUBEBUILDER_ASSETS=$(KUBEBUILDER_ASSETS)

.PHONY: unit
unit: $(SETUP_ENVTEST) ## Run unit test
$(shell $(SETUP_ENVTEST) use -p env --os $(ENVTEST_OS) --arch $(ARCH) $(ENVTEST_K8S_VERSION)) && \
Expand Down
13 changes: 12 additions & 1 deletion api/v1beta1/metal3cluster_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,19 @@ 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.
// TODO: Remove this field in release 1.11. Ref: https://github.com/metal3-io/cluster-api-provider-metal3/issues/2255
//
// 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.
// TODO: Change the default value to false in release 1.12. Ref: https://github.com/metal3-io/cluster-api-provider-metal3/issues/2255
// +kubebuilder:default=true
// +optional
CloudProviderEnabled *bool `json:"cloudProviderEnabled,omitempty"`
}

// IsValid returns an error if the object is not valid, otherwise nil. The
Expand Down
57 changes: 53 additions & 4 deletions api/v1beta1/metal3cluster_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand All @@ -68,6 +74,49 @@ func (c *Metal3Cluster) validate() error {
)
}

if c.Spec.CloudProviderEnabled != nil && c.Spec.NoCloudProvider != nil {
if *c.Spec.CloudProviderEnabled == *c.Spec.NoCloudProvider {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "cloudProviderEnabled"),
c.Spec.CloudProviderEnabled,
"cloudProviderEnabled conflicts the value of noCloudProvider",
),
)
}
}

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",
),
)
}
}

// 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.NoCloudProvider,
"noCloudProvider conflicts the value of cloudProviderEnabled",
),
)
}
}
}

if len(allErrs) == 0 {
return nil
}
Expand Down
141 changes: 126 additions & 15 deletions api/v1beta1/metal3cluster_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (

. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
)

func TestMetal3ClusterDefault(t *testing.T) {
Expand Down Expand Up @@ -52,35 +53,145 @@ func TestMetal3ClusterValidation(t *testing.T) {
invalidHost.Spec.ControlPlaneEndpoint.Host = ""

tests := []struct {
name string
expectErr bool
c *Metal3Cluster
name string
expectErrOnCreate bool
expectErrOnUpdate bool
newCluster *Metal3Cluster
oldCluster *Metal3Cluster
}{
{
name: "should return error when endpoint empty",
expectErr: true,
c: invalidHost,
name: "should return error when endpoint empty",
expectErrOnCreate: true,
expectErrOnUpdate: true,
newCluster: invalidHost,
oldCluster: valid,
},
{
name: "should succeed when endpoint correct",
expectErr: false,
c: valid,
name: "should succeed when endpoint correct",
expectErrOnCreate: false,
expectErrOnUpdate: false,
newCluster: valid,
oldCluster: valid,
},
{
name: "should succeed when cloudProviderEnabled and noCloudProvider are not set",
expectErrOnCreate: false,
expectErrOnUpdate: false,
newCluster: valid,
oldCluster: valid,
},
{
name: "should succeed when cloudProviderEnabled is set and noCloudProvider not set",
expectErrOnCreate: false,
expectErrOnUpdate: false,
newCluster: &Metal3Cluster{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
},
Spec: Metal3ClusterSpec{
ControlPlaneEndpoint: APIEndpoint{
Host: "abc.com",
Port: 443,
},
CloudProviderEnabled: ptr.To(true),
},
},
oldCluster: valid,
},
{
name: "should succeed when noCloudProvider is set and cloudProviderEnabled not set",
expectErrOnCreate: false,
expectErrOnUpdate: false,
newCluster: &Metal3Cluster{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
},
Spec: Metal3ClusterSpec{
ControlPlaneEndpoint: APIEndpoint{
Host: "abc.com",
Port: 443,
},
NoCloudProvider: ptr.To(false),
},
},
oldCluster: valid,
},
{
name: "should succeed when cloudProviderEnabled and noCloudProvider do not conflict",
expectErrOnCreate: false,
expectErrOnUpdate: false,
newCluster: &Metal3Cluster{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
},
Spec: Metal3ClusterSpec{
ControlPlaneEndpoint: APIEndpoint{
Host: "abc.com",
Port: 443,
},
CloudProviderEnabled: ptr.To(true),
},
},
oldCluster: &Metal3Cluster{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
},
Spec: Metal3ClusterSpec{
ControlPlaneEndpoint: APIEndpoint{
Host: "abc.com",
Port: 443,
},
NoCloudProvider: ptr.To(false),
},
},
},
{
name: "should not succeed when cloudProviderEnabled and noCloudProvider do conflict on update",
expectErrOnCreate: false,
expectErrOnUpdate: true,
newCluster: &Metal3Cluster{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
},
Spec: Metal3ClusterSpec{
ControlPlaneEndpoint: APIEndpoint{
Host: "abc.com",
Port: 443,
},
CloudProviderEnabled: ptr.To(false),
},
},
oldCluster: &Metal3Cluster{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
},
Spec: Metal3ClusterSpec{
ControlPlaneEndpoint: APIEndpoint{
Host: "abc.com",
Port: 443,
},
NoCloudProvider: ptr.To(false),
},
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

if tt.expectErr {
_, err := tt.c.ValidateCreate()
g.Expect(err).To(HaveOccurred())
_, err = tt.c.ValidateUpdate(nil)
if tt.expectErrOnCreate {
_, err := tt.newCluster.ValidateCreate()
g.Expect(err).To(HaveOccurred())
} else {
_, err := tt.c.ValidateCreate()
_, err := tt.newCluster.ValidateCreate()
g.Expect(err).NotTo(HaveOccurred())
_, err = tt.c.ValidateUpdate(nil)
}
if tt.expectErrOnUpdate {
_, err := tt.newCluster.ValidateUpdate(tt.oldCluster)
g.Expect(err).To(HaveOccurred())
} else {
_, err := tt.newCluster.ValidateUpdate(tt.oldCluster)
g.Expect(err).NotTo(HaveOccurred())
}
})
Expand Down
18 changes: 14 additions & 4 deletions api/v1beta1/zz_generated.deepcopy.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 19 additions & 6 deletions baremetal/metal3machine_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading

0 comments on commit b4480cf

Please sign in to comment.