Skip to content

✨ Deprecate NoCloudProvider and add CloudProviderEnabled #2108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 23, 2025
Merged

✨ Deprecate NoCloudProvider and add CloudProviderEnabled #2108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Makefile
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop

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
Open in desktop
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
Loading