Skip to content

Commit a16844f

Browse files
adilGhaffarDevadilGhaffarDev
committed
Deprecate NoCloudProvider and add CloudProviderEnabled
Signed-off-by: Muhammad Adil Ghaffar <[email protected]>
1 parent c9d6f72 commit a16844f

19 files changed

+317
-46
lines changed

‎Makefile‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,13 @@ help: # Display this help
113113
## --------------------------------------
114114
##@ tests:
115115

116+
export KUBEBUILDER_ENVTEST_KUBERNETES_VERSION ?= 1.32.0
117+
KUBEBUILDER_ASSETS ?= $(shell $(SETUP_ENVTEST) use --use-env -p path $(KUBEBUILDER_ENVTEST_KUBERNETES_VERSION))
118+
119+
.PHONY: setup-envtest
120+
setup-envtest: $(SETUP_ENVTEST) ## Set up envtest (download kubebuilder assets)
121+
@echo KUBEBUILDER_ASSETS=$(KUBEBUILDER_ASSETS)
122+
116123
.PHONY: unit
117124
unit: $(SETUP_ENVTEST) ## Run unit test
118125
$(shell $(SETUP_ENVTEST) use -p env --os $(ENVTEST_OS) --arch $(ARCH) $(ENVTEST_K8S_VERSION)) && \

‎api/v1beta1/metal3cluster_types.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,19 @@ type Metal3ClusterSpec struct {
3737
// Determines if the cluster is not to be deployed with an external cloud provider.
3838
// If set to true, CAPM3 will use node labels to set providerID on the kubernetes nodes.
3939
// If set to false, providerID is set on nodes by other entities and CAPM3 uses the value of the providerID on the m3m resource.
40+
// TODO: Remove this field in release 1.11. Ref: https://github.com/metal3-io/cluster-api-provider-metal3/issues/2255
41+
//
42+
// Deprecated: This field is deprecated, use cloudProviderEnabled instead
43+
//
4044
// +optional
41-
NoCloudProvider bool `json:"noCloudProvider,omitempty"`
45+
NoCloudProvider *bool `json:"noCloudProvider,omitempty"`
46+
// Determines if the cluster is to be deployed with an external cloud provider.
47+
// If set to false, CAPM3 will use node labels to set providerID on the kubernetes nodes.
48+
// If set to true, providerID is set on nodes by other entities and CAPM3 uses the value of the providerID on the m3m resource.
49+
// TODO: Change the default value to false in release 1.12. Ref: https://github.com/metal3-io/cluster-api-provider-metal3/issues/2255
50+
// +kubebuilder:default=true
51+
// +optional
52+
CloudProviderEnabled *bool `json:"cloudProviderEnabled,omitempty"`
4253
}
4354

4455
// IsValid returns an error if the object is not valid, otherwise nil. The

‎api/v1beta1/metal3cluster_webhook.go‎

Lines changed: 83 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ limitations under the License.
1414
package v1beta1
1515

1616
import (
17+
"github.com/pkg/errors"
1718
apierrors "k8s.io/apimachinery/pkg/api/errors"
1819
"k8s.io/apimachinery/pkg/runtime"
1920
"k8s.io/apimachinery/pkg/util/validation/field"
@@ -42,20 +43,25 @@ func (c *Metal3Cluster) Default() {
4243

4344
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
4445
func (c *Metal3Cluster) ValidateCreate() (admission.Warnings, error) {
45-
return nil, c.validate()
46+
return nil, c.validate(nil)
4647
}
4748

4849
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
49-
func (c *Metal3Cluster) ValidateUpdate(_ runtime.Object) (admission.Warnings, error) {
50-
return nil, c.validate()
50+
func (c *Metal3Cluster) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
51+
oldM3C, ok := old.(*Metal3Cluster)
52+
if !ok || oldM3C == nil {
53+
return nil, apierrors.NewInternalError(errors.New("unable to convert existing object"))
54+
}
55+
56+
return nil, c.validate(oldM3C)
5157
}
5258

5359
// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
5460
func (c *Metal3Cluster) ValidateDelete() (admission.Warnings, error) {
5561
return nil, nil
5662
}
5763

58-
func (c *Metal3Cluster) validate() error {
64+
func (c *Metal3Cluster) validate(oldM3C *Metal3Cluster) error {
5965
var allErrs field.ErrorList
6066
if c.Spec.ControlPlaneEndpoint.Host == "" {
6167
allErrs = append(
@@ -68,6 +74,79 @@ func (c *Metal3Cluster) validate() error {
6874
)
6975
}
7076

77+
if c.Spec.CloudProviderEnabled != nil && c.Spec.NoCloudProvider != nil {
78+
if !*c.Spec.CloudProviderEnabled && !*oldM3C.Spec.NoCloudProvider {
79+
allErrs = append(
80+
allErrs,
81+
field.Invalid(
82+
field.NewPath("spec", "cloudProviderEnabled"),
83+
c.Spec.CloudProviderEnabled,
84+
"cloudProviderEnabled conflicts the value of noCloudProvider",
85+
),
86+
)
87+
}
88+
if *c.Spec.CloudProviderEnabled && *oldM3C.Spec.NoCloudProvider {
89+
allErrs = append(
90+
allErrs,
91+
field.Invalid(
92+
field.NewPath("spec", "cloudProviderEnabled"),
93+
c.Spec.CloudProviderEnabled,
94+
"cloudProviderEnabled conflicts the value of noCloudProvider",
95+
),
96+
)
97+
}
98+
}
99+
100+
if oldM3C != nil {
101+
// Validate cloudProviderEnabled
102+
if c.Spec.CloudProviderEnabled != nil && oldM3C.Spec.NoCloudProvider != nil {
103+
if !*c.Spec.CloudProviderEnabled && !*oldM3C.Spec.NoCloudProvider {
104+
allErrs = append(
105+
allErrs,
106+
field.Invalid(
107+
field.NewPath("spec", "cloudProviderEnabled"),
108+
c.Spec.CloudProviderEnabled,
109+
"cloudProviderEnabled conflicts the value of noCloudProvider",
110+
),
111+
)
112+
}
113+
if *c.Spec.CloudProviderEnabled && *oldM3C.Spec.NoCloudProvider {
114+
allErrs = append(
115+
allErrs,
116+
field.Invalid(
117+
field.NewPath("spec", "cloudProviderEnabled"),
118+
c.Spec.CloudProviderEnabled,
119+
"cloudProviderEnabled conflicts the value of noCloudProvider",
120+
),
121+
)
122+
}
123+
}
124+
125+
// Validate noCloudProvider
126+
if c.Spec.NoCloudProvider != nil && oldM3C.Spec.CloudProviderEnabled != nil {
127+
if !*c.Spec.NoCloudProvider && !*oldM3C.Spec.CloudProviderEnabled {
128+
allErrs = append(
129+
allErrs,
130+
field.Invalid(
131+
field.NewPath("spec", "noCloudProvider"),
132+
c.Spec.NoCloudProvider,
133+
"noCloudProvider conflicts the value of cloudProviderEnabled",
134+
),
135+
)
136+
}
137+
if *c.Spec.NoCloudProvider && *oldM3C.Spec.CloudProviderEnabled {
138+
allErrs = append(
139+
allErrs,
140+
field.Invalid(
141+
field.NewPath("spec", "noCloudProvider"),
142+
c.Spec.NoCloudProvider,
143+
"noCloudProvider conflicts the value of cloudProviderEnabled",
144+
),
145+
)
146+
}
147+
}
148+
}
149+
71150
if len(allErrs) == 0 {
72151
return nil
73152
}

‎api/v1beta1/metal3cluster_webhook_test.go‎

Lines changed: 126 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818

1919
. "github.com/onsi/gomega"
2020
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
21+
"k8s.io/utils/ptr"
2122
)
2223

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

5455
tests := []struct {
55-
name string
56-
expectErr bool
57-
c *Metal3Cluster
56+
name string
57+
expectErrOnCreate bool
58+
expectErrOnUpdate bool
59+
newCluster *Metal3Cluster
60+
oldCluster *Metal3Cluster
5861
}{
5962
{
60-
name: "should return error when endpoint empty",
61-
expectErr: true,
62-
c: invalidHost,
63+
name: "should return error when endpoint empty",
64+
expectErrOnCreate: true,
65+
expectErrOnUpdate: true,
66+
newCluster: invalidHost,
67+
oldCluster: valid,
6368
},
6469
{
65-
name: "should succeed when endpoint correct",
66-
expectErr: false,
67-
c: valid,
70+
name: "should succeed when endpoint correct",
71+
expectErrOnCreate: false,
72+
expectErrOnUpdate: false,
73+
newCluster: valid,
74+
oldCluster: valid,
75+
},
76+
{
77+
name: "should succeed when cloudProviderEnabled and noCloudProvider are not set",
78+
expectErrOnCreate: false,
79+
expectErrOnUpdate: false,
80+
newCluster: valid,
81+
oldCluster: valid,
82+
},
83+
{
84+
name: "should succeed when cloudProviderEnabled is set and noCloudProvider not set",
85+
expectErrOnCreate: false,
86+
expectErrOnUpdate: false,
87+
newCluster: &Metal3Cluster{
88+
ObjectMeta: metav1.ObjectMeta{
89+
Namespace: "foo",
90+
},
91+
Spec: Metal3ClusterSpec{
92+
ControlPlaneEndpoint: APIEndpoint{
93+
Host: "abc.com",
94+
Port: 443,
95+
},
96+
CloudProviderEnabled: ptr.To(true),
97+
},
98+
},
99+
oldCluster: valid,
100+
},
101+
{
102+
name: "should succeed when noCloudProvider is set and cloudProviderEnabled not set",
103+
expectErrOnCreate: false,
104+
expectErrOnUpdate: false,
105+
newCluster: &Metal3Cluster{
106+
ObjectMeta: metav1.ObjectMeta{
107+
Namespace: "foo",
108+
},
109+
Spec: Metal3ClusterSpec{
110+
ControlPlaneEndpoint: APIEndpoint{
111+
Host: "abc.com",
112+
Port: 443,
113+
},
114+
NoCloudProvider: ptr.To(false),
115+
},
116+
},
117+
oldCluster: valid,
118+
},
119+
{
120+
name: "should succeed when cloudProviderEnabled and noCloudProvider do not conflict",
121+
expectErrOnCreate: false,
122+
expectErrOnUpdate: false,
123+
newCluster: &Metal3Cluster{
124+
ObjectMeta: metav1.ObjectMeta{
125+
Namespace: "foo",
126+
},
127+
Spec: Metal3ClusterSpec{
128+
ControlPlaneEndpoint: APIEndpoint{
129+
Host: "abc.com",
130+
Port: 443,
131+
},
132+
CloudProviderEnabled: ptr.To(true),
133+
},
134+
},
135+
oldCluster: &Metal3Cluster{
136+
ObjectMeta: metav1.ObjectMeta{
137+
Namespace: "foo",
138+
},
139+
Spec: Metal3ClusterSpec{
140+
ControlPlaneEndpoint: APIEndpoint{
141+
Host: "abc.com",
142+
Port: 443,
143+
},
144+
NoCloudProvider: ptr.To(false),
145+
},
146+
},
147+
},
148+
{
149+
name: "should not succeed when cloudProviderEnabled and noCloudProvider do conflict on update",
150+
expectErrOnCreate: false,
151+
expectErrOnUpdate: true,
152+
newCluster: &Metal3Cluster{
153+
ObjectMeta: metav1.ObjectMeta{
154+
Namespace: "foo",
155+
},
156+
Spec: Metal3ClusterSpec{
157+
ControlPlaneEndpoint: APIEndpoint{
158+
Host: "abc.com",
159+
Port: 443,
160+
},
161+
CloudProviderEnabled: ptr.To(false),
162+
},
163+
},
164+
oldCluster: &Metal3Cluster{
165+
ObjectMeta: metav1.ObjectMeta{
166+
Namespace: "foo",
167+
},
168+
Spec: Metal3ClusterSpec{
169+
ControlPlaneEndpoint: APIEndpoint{
170+
Host: "abc.com",
171+
Port: 443,
172+
},
173+
NoCloudProvider: ptr.To(false),
174+
},
175+
},
68176
},
69177
}
70178

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

75-
if tt.expectErr {
76-
_, err := tt.c.ValidateCreate()
77-
g.Expect(err).To(HaveOccurred())
78-
_, err = tt.c.ValidateUpdate(nil)
183+
if tt.expectErrOnCreate {
184+
_, err := tt.newCluster.ValidateCreate()
79185
g.Expect(err).To(HaveOccurred())
80186
} else {
81-
_, err := tt.c.ValidateCreate()
187+
_, err := tt.newCluster.ValidateCreate()
82188
g.Expect(err).NotTo(HaveOccurred())
83-
_, err = tt.c.ValidateUpdate(nil)
189+
}
190+
if tt.expectErrOnUpdate {
191+
_, err := tt.newCluster.ValidateUpdate(tt.oldCluster)
192+
g.Expect(err).To(HaveOccurred())
193+
} else {
194+
_, err := tt.newCluster.ValidateUpdate(tt.oldCluster)
84195
g.Expect(err).NotTo(HaveOccurred())
85196
}
86197
})

‎api/v1beta1/zz_generated.deepcopy.go‎

Lines changed: 14 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)