Skip to content

Commit

Permalink
Merge pull request #2388 from detiber/webhookTweaks
Browse files Browse the repository at this point in the history 
✨ Centralize common validations for kubeadmcontrolplane webhooks, also add support for mutating the KubeadmConfigSpec minus InitConfiguration/JoinConfiguration
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Feb 20, 2020
2 parents 94aa714 + 3f4b4ca commit 952da0f
Show file tree
Hide file tree
Showing 2 changed files with 170 additions and 64 deletions.
120 changes: 63 additions & 57 deletions controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ import (
"sigs.k8s.io/controller-runtime/pkg/webhook"
)

func (r *KubeadmControlPlane) SetupWebhookWithManager(mgr ctrl.Manager) error {
func (in *KubeadmControlPlane) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
For(r).
For(in).
Complete()
}

Expand All @@ -39,30 +39,70 @@ var _ webhook.Defaulter = &KubeadmControlPlane{}
var _ webhook.Validator = &KubeadmControlPlane{}

// Default implements webhook.Defaulter so a webhook will be registered for the type
func (r *KubeadmControlPlane) Default() {
if r.Spec.Replicas == nil {
func (in *KubeadmControlPlane) Default() {
if in.Spec.Replicas == nil {
replicas := int32(1)
r.Spec.Replicas = &replicas
in.Spec.Replicas = &replicas
}

if r.Spec.InfrastructureTemplate.Namespace == "" {
r.Spec.InfrastructureTemplate.Namespace = r.Namespace
if in.Spec.InfrastructureTemplate.Namespace == "" {
in.Spec.InfrastructureTemplate.Namespace = in.Namespace
}
}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
func (r *KubeadmControlPlane) ValidateCreate() error {
var allErrs field.ErrorList
func (in *KubeadmControlPlane) ValidateCreate() error {
allErrs := in.validateCommon()
if len(allErrs) > 0 {
return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlane").GroupKind(), in.Name, allErrs)
}

return nil
}

if r.Spec.Replicas == nil {
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (in *KubeadmControlPlane) ValidateUpdate(old runtime.Object) error {
allErrs := in.validateCommon()

prev := old.(*KubeadmControlPlane)
// Verify that we are not making any changes to the InitConfiguration of KubeadmConfigSpec
if !reflect.DeepEqual(in.Spec.KubeadmConfigSpec.InitConfiguration, prev.Spec.KubeadmConfigSpec.InitConfiguration) {
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "kubeadmConfigSpec", "initConfiguration"),
"cannot be modified",
),
)
}
// Verify that we are not making any changes to the ClusterConfiguration of KubeadmConfigSpec
if !reflect.DeepEqual(in.Spec.KubeadmConfigSpec.ClusterConfiguration, prev.Spec.KubeadmConfigSpec.ClusterConfiguration) {
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration"),
"cannot be modified",
),
)
}

if len(allErrs) > 0 {
return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlane").GroupKind(), in.Name, allErrs)
}

return nil
}

func (in *KubeadmControlPlane) validateCommon() (allErrs field.ErrorList) {
if in.Spec.Replicas == nil {
allErrs = append(
allErrs,
field.Required(
field.NewPath("spec", "replicas"),
"is required",
),
)
} else if *r.Spec.Replicas <= 0 {
} else if *in.Spec.Replicas <= 0 {
// The use of the scale subresource should provide a guarantee that negative values
// should not be accepted for this field, but since we have to validate that Replicas != 0
// it doesn't hurt to also additionally validate for negative numbers here as well.
Expand All @@ -76,14 +116,19 @@ func (r *KubeadmControlPlane) ValidateCreate() error {
}

externalEtcd := false
if r.Spec.KubeadmConfigSpec.InitConfiguration != nil {
if r.Spec.KubeadmConfigSpec.InitConfiguration.Etcd.External != nil {
if in.Spec.KubeadmConfigSpec.InitConfiguration != nil {
if in.Spec.KubeadmConfigSpec.InitConfiguration.Etcd.External != nil {
externalEtcd = true
}
}
if in.Spec.KubeadmConfigSpec.ClusterConfiguration != nil {
if in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External != nil {
externalEtcd = true
}
}

if !externalEtcd {
if r.Spec.Replicas != nil && *r.Spec.Replicas%2 == 0 {
if in.Spec.Replicas != nil && *in.Spec.Replicas%2 == 0 {
allErrs = append(
allErrs,
field.Forbidden(
Expand All @@ -94,60 +139,21 @@ func (r *KubeadmControlPlane) ValidateCreate() error {
}
}

if r.Spec.InfrastructureTemplate.Namespace != r.Namespace {
if in.Spec.InfrastructureTemplate.Namespace != in.Namespace {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "infrastructureTemplate", "namespace"),
r.Spec.InfrastructureTemplate.Namespace,
in.Spec.InfrastructureTemplate.Namespace,
"must match metadata.namespace",
),
)
}

if len(allErrs) == 0 {
return nil
}

return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlane").GroupKind(), r.Name, allErrs)
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (r *KubeadmControlPlane) ValidateUpdate(old runtime.Object) error {
var allErrs field.ErrorList

oldKubeadmControlPlane := old.(*KubeadmControlPlane)
if !reflect.DeepEqual(r.Spec.KubeadmConfigSpec, oldKubeadmControlPlane.Spec.KubeadmConfigSpec) {
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "kubeadmConfigSpec"),
"cannot be modified",
),
)
}

if *r.Spec.Replicas <= 0 {
// The use of the scale subresource should provide a guarantee that negative values
// should not be accepted for this field, but since we have to validate that Replicas != 0
// it doesn't hurt to also additionally validate for negative numbers here as well.
allErrs = append(
allErrs,
field.Forbidden(
field.NewPath("spec", "replicas"),
"cannot be less than or equal to 0",
),
)
}

if len(allErrs) == 0 {
return nil
}

return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmControlPlane").GroupKind(), r.Name, allErrs)
return allErrs
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
func (r *KubeadmControlPlane) ValidateDelete() error {
func (in *KubeadmControlPlane) ValidateDelete() error {
return nil
}
114 changes: 107 additions & 7 deletions controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,34 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
Namespace: "foo",
Name: "infraTemplate",
},
Replicas: pointer.Int32Ptr(1),
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{},
Replicas: pointer.Int32Ptr(1),
KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{
InitConfiguration: &kubeadmv1beta1.InitConfiguration{
LocalAPIEndpoint: kubeadmv1beta1.APIEndpoint{
AdvertiseAddress: "127.0.0.1",
BindPort: int32(443),
},
},
ClusterConfiguration: &kubeadmv1beta1.ClusterConfiguration{
ClusterName: "test",
},
JoinConfiguration: &kubeadmv1beta1.JoinConfiguration{
NodeRegistration: kubeadmv1beta1.NodeRegistrationOptions{
Name: "test",
},
},
},
},
}

invalidUpdate := before.DeepCopy()
invalidUpdate.Spec.KubeadmConfigSpec.InitConfiguration = &kubeadmv1beta1.InitConfiguration{}
invalidUpdateKubeadmConfigInit := before.DeepCopy()
invalidUpdateKubeadmConfigInit.Spec.KubeadmConfigSpec.InitConfiguration = &kubeadmv1beta1.InitConfiguration{}

invalidUpdateKubeadmConfigCluster := before.DeepCopy()
invalidUpdateKubeadmConfigCluster.Spec.KubeadmConfigSpec.ClusterConfiguration = &kubeadmv1beta1.ClusterConfiguration{}

validUpdateKubeadmConfigJoin := before.DeepCopy()
validUpdateKubeadmConfigJoin.Spec.KubeadmConfigSpec.JoinConfiguration = &kubeadmv1beta1.JoinConfiguration{}

validUpdate := before.DeepCopy()
validUpdate.Labels = map[string]string{"blue": "green"}
Expand All @@ -158,33 +179,112 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
scaleToZero := before.DeepCopy()
scaleToZero.Spec.Replicas = pointer.Int32Ptr(0)

scaleToEven := before.DeepCopy()
scaleToEven.Spec.Replicas = pointer.Int32Ptr(2)

invalidNamespace := before.DeepCopy()
invalidNamespace.Spec.InfrastructureTemplate.Namespace = "bar"

missingReplicas := before.DeepCopy()
missingReplicas.Spec.Replicas = nil

beforeExternalEtcdInit := before.DeepCopy()
beforeExternalEtcdInit.Spec.KubeadmConfigSpec.InitConfiguration = &kubeadmv1beta1.InitConfiguration{
ClusterConfiguration: kubeadmv1beta1.ClusterConfiguration{
Etcd: kubeadmv1beta1.Etcd{
External: &kubeadmv1beta1.ExternalEtcd{
Endpoints: []string{"127.0.0.1"},
},
},
},
}
scaleToEvenExternalEtcdInit := beforeExternalEtcdInit.DeepCopy()
scaleToEvenExternalEtcdInit.Spec.Replicas = pointer.Int32Ptr(2)

beforeExternalEtcdCluster := before.DeepCopy()
beforeExternalEtcdCluster.Spec.KubeadmConfigSpec.ClusterConfiguration = &kubeadmv1beta1.ClusterConfiguration{
Etcd: kubeadmv1beta1.Etcd{
External: &kubeadmv1beta1.ExternalEtcd{
Endpoints: []string{"127.0.0.1"},
},
},
}
scaleToEvenExternalEtcdCluster := beforeExternalEtcdCluster.DeepCopy()
scaleToEvenExternalEtcdCluster.Spec.Replicas = pointer.Int32Ptr(2)

tests := []struct {
name string
expectErr bool
before *KubeadmControlPlane
kcp *KubeadmControlPlane
}{
{
name: "should succeed when given a valid config",
expectErr: false,
before: before,
kcp: validUpdate,
},
{
name: "should return error when trying to mutate the kubeadmconfigspec",
name: "should return error when trying to mutate the kubeadmconfigspec initconfiguration",
expectErr: true,
before: before,
kcp: invalidUpdateKubeadmConfigInit,
},
{
name: "should return error when trying to mutate the kubeadmconfigspec clusterconfiguration",
expectErr: true,
kcp: invalidUpdate,
before: before,
kcp: invalidUpdateKubeadmConfigCluster,
},
{
name: "should not return error when trying to mutate the kubeadmconfigspec joinconfiguration",
expectErr: false,
before: before,
kcp: validUpdateKubeadmConfigJoin,
},
{
name: "should return error when trying to scale to zero",
expectErr: true,
before: before,
kcp: scaleToZero,
},
{
name: "should return error when trying to scale to an even number",
expectErr: true,
before: before,
kcp: scaleToEven,
},
{
name: "should return error when trying to reference cross namespace",
expectErr: true,
before: before,
kcp: invalidNamespace,
},
{
name: "should return error when trying to scale to nil",
expectErr: true,
before: before,
kcp: missingReplicas,
},
{
name: "should succeed when trying to scale to an even number with external etcd defined in InitConfiguration",
expectErr: false,
before: beforeExternalEtcdInit,
kcp: scaleToEvenExternalEtcdInit,
},
{
name: "should succeed when trying to scale to an even number with external etcd defined in ClusterConfiguration",
expectErr: false,
before: beforeExternalEtcdCluster,
kcp: scaleToEvenExternalEtcdCluster,
},
}

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

err := tt.kcp.ValidateUpdate(before.DeepCopy())
err := tt.kcp.ValidateUpdate(tt.before.DeepCopy())
if tt.expectErr {
g.Expect(err).To(HaveOccurred())
} else {
Expand Down

0 comments on commit 952da0f

Please sign in to comment.