Skip to content
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

Add validation minimum and change type to int32 for compatibility #57

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Add validation minimum and change type to int32 for compatibility #57

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ecf4ee
Change replicas field type to int32 and add validation
sircthulhu Mar 20, 2024
e801931
Mark Replicas as pointer to in32, remove unnecessary validation
sircthulhu Mar 20, 2024
5d2f7be
remove commented lines of code
sircthulhu Mar 20, 2024
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
Prev Previous commit
Next Next commit
Mark Replicas as pointer to in32, remove unnecessary validation
  • Loading branch information
@sircthulhu
sircthulhu committed Mar 20, 2024
commit e80193135b7ba77162757c4e701abefc7e02a7cf
2 changes: 1 addition & 1 deletion api/v1alpha1/etcdcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type EtcdClusterSpec struct {
// +optional
// +kubebuilder:default:=3
// +kubebuilder:validation:Minimum:=0
Replicas int32 `json:"replicas,omitempty"`
Replicas *int32 `json:"replicas,omitempty"`
Storage Storage `json:"storage,omitempty"`
}

Expand Down
49 changes: 17 additions & 32 deletions api/v1alpha1/etcdcluster_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,8 @@ limitations under the License.
package v1alpha1

import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation/field"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand Down Expand Up @@ -57,19 +54,13 @@ var _ webhook.Validator = &EtcdCluster{}
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
func (r *EtcdCluster) ValidateCreate() (admission.Warnings, error) {
etcdclusterlog.Info("validate create", "name", r.Name)
var allErrors field.ErrorList
if r.Spec.Replicas < 0 {
allErrors = append(allErrors, field.Invalid(
field.NewPath("spec", "replicas"),
r.Spec.Replicas,
"cluster replicas cannot be less than zero",
))
}
if len(allErrors) > 0 {
return nil, apierrors.NewInvalid(
schema.GroupKind{Group: GroupVersion.Group, Kind: "EtcdCluster"},
r.Name, allErrors)
}
//var allErrors field.ErrorList
sircthulhu marked this conversation as resolved.
Show resolved Hide resolved
//// write validation here
//if len(allErrors) > 0 {
// return nil, apierrors.NewInvalid(
// schema.GroupKind{Group: GroupVersion.Group, Kind: "EtcdCluster"},
// r.Name, allErrors)
//}

return nil, nil
}
Expand All @@ -82,22 +73,16 @@ func (r *EtcdCluster) ValidateUpdate(old runtime.Object) (admission.Warnings, er
warnings = append(warnings, "cluster resize is not currently supported")
}

var allErrors field.ErrorList
if r.Spec.Replicas < 0 {
allErrors = append(allErrors, field.Invalid(
field.NewPath("spec", "replicas"),
r.Spec.Replicas,
"cluster replicas cannot be less than zero",
))
}

var err *apierrors.StatusError
if len(allErrors) > 0 {
err = apierrors.NewInvalid(
schema.GroupKind{Group: GroupVersion.Group, Kind: "EtcdCluster"},
r.Name, allErrors)
}
return warnings, err
//var allErrors field.ErrorList
sircthulhu marked this conversation as resolved.
Show resolved Hide resolved
//// write validation here
//var err *apierrors.StatusError
//if len(allErrors) > 0 {
// err = apierrors.NewInvalid(
// schema.GroupKind{Group: GroupVersion.Group, Kind: "EtcdCluster"},
// r.Name, allErrors)
// return warnings, err
//}
return warnings, nil
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
Expand Down
49 changes: 24 additions & 25 deletions api/v1alpha1/etcdcluster_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@ package v1alpha1
import (
. "github.com/onsi/ginkgo/v2"
"github.com/onsi/gomega"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
)

var _ = Describe("EtcdCluster Webhook", func() {
Expand All @@ -30,51 +29,51 @@ var _ = Describe("EtcdCluster Webhook", func() {
It("Should fill in the default value if a required field is empty", func() {
etcdCluster := &EtcdCluster{}
etcdCluster.Default()
gomega.Expect(etcdCluster.Spec.Replicas).To(gomega.Equal(int32(0)), "User should have an opportunity to create cluster with 0 replicas")
gomega.Expect(etcdCluster.Spec.Replicas).To(gomega.BeNil(), "User should have an opportunity to create cluster with 0 replicas")
gomega.Expect(etcdCluster.Spec.Storage.Size).To(gomega.Equal(resource.MustParse("4Gi")))
})

It("Should not override fields with default values if not empty", func() {
etcdCluster := &EtcdCluster{
Spec: EtcdClusterSpec{
Replicas: 5,
Replicas: ptr.To(int32(5)),
Storage: Storage{
StorageClass: "local-path",
Size: resource.MustParse("10Gi"),
},
},
}
etcdCluster.Default()
gomega.Expect(etcdCluster.Spec.Replicas).To(gomega.Equal(int32(5)))
gomega.Expect(*etcdCluster.Spec.Replicas).To(gomega.Equal(int32(5)))
gomega.Expect(etcdCluster.Spec.Storage.Size).To(gomega.Equal(resource.MustParse("10Gi")))
})
})

Context("When creating EtcdCluster under Validating Webhook", func() {
It("Should deny if replicas is negative", func() {
etcdCluster := &EtcdCluster{
Spec: EtcdClusterSpec{
Replicas: -1,
},
}
_, err := etcdCluster.ValidateCreate()
var statusErr *apierrors.StatusError

if gomega.Expect(err).To(gomega.BeAssignableToTypeOf(statusErr)) {
statusErr = err.(*apierrors.StatusError)
gomega.Expect(statusErr.ErrStatus.Reason).To(gomega.Equal(metav1.StatusReasonInvalid))
gomega.Expect(statusErr.ErrStatus.Details.Causes).To(gomega.ContainElement(metav1.StatusCause{
Type: metav1.CauseTypeFieldValueInvalid,
Message: "Invalid value: -1: cluster replicas cannot be less than zero",
Field: "spec.replicas",
}))
}
})
//It("Should deny if replicas is negative", func() {
sircthulhu marked this conversation as resolved.
Show resolved Hide resolved
// etcdCluster := &EtcdCluster{
// Spec: EtcdClusterSpec{
// Replicas: ptr.To(int32(-1)),
// },
// }
// _, err := etcdCluster.ValidateCreate()
// var statusErr *apierrors.StatusError
//
// if gomega.Expect(err).To(gomega.BeAssignableToTypeOf(statusErr)) {
// statusErr = err.(*apierrors.StatusError)
// gomega.Expect(statusErr.ErrStatus.Reason).To(gomega.Equal(metav1.StatusReasonInvalid))
// gomega.Expect(statusErr.ErrStatus.Details.Causes).To(gomega.ContainElement(metav1.StatusCause{
// Type: metav1.CauseTypeFieldValueInvalid,
// Message: "Invalid value: -1: cluster replicas cannot be less than zero",
// Field: "spec.replicas",
// }))
// }
//})

It("Should admit if all required fields are provided", func() {
etcdCluster := &EtcdCluster{
Spec: EtcdClusterSpec{
Replicas: 1,
Replicas: ptr.To(int32(1)),
},
}
w, err := etcdCluster.ValidateCreate()
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/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.

8 changes: 5 additions & 3 deletions internal/controller/etcdcluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package controller
import (
"context"

"k8s.io/utils/ptr"

appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
resource2 "k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -56,7 +58,7 @@ var _ = Describe("EtcdCluster Controller", func() {
Namespace: "default",
},
Spec: etcdaenixiov1alpha1.EtcdClusterSpec{
Replicas: 3,
Replicas: ptr.To(int32(3)),
Storage: etcdaenixiov1alpha1.Storage{
Size: resource2.MustParse("1Gi"),
},
Expand Down Expand Up @@ -142,8 +144,8 @@ var _ = Describe("EtcdCluster Controller", func() {
err = k8sClient.Get(ctx, typeNamespacedName, sts)
Expect(err).NotTo(HaveOccurred(), "cluster statefulset should exist")
// mark sts as ready
sts.Status.ReadyReplicas = etcdcluster.Spec.Replicas
sts.Status.Replicas = etcdcluster.Spec.Replicas
sts.Status.ReadyReplicas = *etcdcluster.Spec.Replicas
sts.Status.Replicas = *etcdcluster.Spec.Replicas
Expect(k8sClient.Status().Update(ctx, sts)).To(Succeed())
// reconcile and check EtcdCluster status
_, err = controllerReconciler.Reconcile(ctx, reconcile.Request{
Expand Down