Skip to content

Commit

Permalink
Mark Replicas as pointer to in32, remove unnecessary validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@sircthulhu
sircthulhu committed Mar 20, 2024
1 parent a8d9ae9 commit b86abaa
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 65 deletions.
2 changes: 1 addition & 1 deletion api/v1alpha1/etcdcluster_types.go
View file
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
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
//// 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
//// 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
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() {
// 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

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

6 changes: 2 additions & 4 deletions internal/controller/etcdcluster_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ func (r *EtcdClusterReconciler) ensureClusterStatefulSet(
notFound = true
// prepare initial cluster members
initialCluster := ""
for i := int32(0); i < cluster.Spec.Replicas; i++ {
for i := int32(0); i < *cluster.Spec.Replicas; i++ {
if i > 0 {
initialCluster += ","
}
Expand All @@ -318,8 +318,7 @@ func (r *EtcdClusterReconciler) ensureClusterStatefulSet(
Name: cluster.Name,
},
Spec: appsv1.StatefulSetSpec{
// initialize static fields that cannot be changed across updates.
Replicas: new(int32),
Replicas: cluster.Spec.Replicas,
ServiceName: cluster.Name,
PodManagementPolicy: appsv1.ParallelPodManagement,
Selector: &metav1.LabelSelector{
Expand Down Expand Up @@ -441,7 +440,6 @@ func (r *EtcdClusterReconciler) ensureClusterStatefulSet(
},
},
}
*statefulSet.Spec.Replicas = cluster.Spec.Replicas
if err := ctrl.SetControllerReference(cluster, statefulSet, r.Scheme); err != nil {
return fmt.Errorf("cannot set controller reference: %w", err)
}
Expand Down
8 changes: 5 additions & 3 deletions internal/controller/etcdcluster_controller_test.go
View file
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 @@ -144,8 +146,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

0 comments on commit b86abaa

Please sign in to comment.