From 90d606bc1eb5094f56cc5faca9e860d352dd7f1c Mon Sep 17 00:00:00 2001 From: Ben Moss Date: Mon, 30 Mar 2020 21:19:56 +0000 Subject: [PATCH 1/2] Add validation for AWSCluster - Validates Region, ControlPlaneLoadBalancer, ControlPlaneEndpoint, and Bastion - Bastion can be added but not removed --- api/v1alpha3/awscluster_webhook.go | 50 ++++++++ api/v1alpha3/awscluster_webhook_test.go | 145 ++++++++++++++++++++++++ config/webhook/manifests.yaml | 19 ++++ 3 files changed, 214 insertions(+) create mode 100644 api/v1alpha3/awscluster_webhook_test.go diff --git a/api/v1alpha3/awscluster_webhook.go b/api/v1alpha3/awscluster_webhook.go index 5cd7508583..23a15ca53a 100644 --- a/api/v1alpha3/awscluster_webhook.go +++ b/api/v1alpha3/awscluster_webhook.go @@ -17,8 +17,16 @@ limitations under the License. package v1alpha3 import ( + "fmt" + "reflect" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" ctrl "sigs.k8s.io/controller-runtime" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook" ) // log is for logging in this package. @@ -29,3 +37,45 @@ func (r *AWSCluster) SetupWebhookWithManager(mgr ctrl.Manager) error { For(r). Complete() } + +// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awscluster,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=awsclusters,versions=v1alpha3,name=validation.awscluster.infrastructure.cluster.x-k8s.io + +var _ webhook.Validator = &AWSCluster{} + +func (r *AWSCluster) ValidateCreate() error { + return nil +} + +func (r *AWSCluster) ValidateDelete() error { + return nil +} + +func (r *AWSCluster) ValidateUpdate(old runtime.Object) error { + var allErrs field.ErrorList + oldC, ok := old.(*AWSCluster) + if !ok { + return apierrors.NewBadRequest(fmt.Sprintf("expected an AWSCluster but got a %T", old)) + } + if r.Spec.Region != oldC.Spec.Region { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "region"), r.Spec.Region, "field is immutable"), + ) + } + if !reflect.DeepEqual(r.Spec.ControlPlaneLoadBalancer, oldC.Spec.ControlPlaneLoadBalancer) { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer"), r.Spec.ControlPlaneLoadBalancer, "field is immutable"), + ) + } + if !reflect.DeepEqual(oldC.Spec.ControlPlaneEndpoint, clusterv1.APIEndpoint{}) && !reflect.DeepEqual(r.Spec.ControlPlaneEndpoint, oldC.Spec.ControlPlaneEndpoint) { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "controlPlaneEndpoint"), r.Spec.ControlPlaneEndpoint, "field is immutable"), + ) + } + if oldC.Spec.Bastion.Enabled && !r.Spec.Bastion.Enabled { + allErrs = append(allErrs, + field.Invalid(field.NewPath("spec", "bastion.enabled"), r.Spec.Bastion.Enabled, "cannot be disabled"), + ) + } + + return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) +} diff --git a/api/v1alpha3/awscluster_webhook_test.go b/api/v1alpha3/awscluster_webhook_test.go new file mode 100644 index 0000000000..7897bff4fa --- /dev/null +++ b/api/v1alpha3/awscluster_webhook_test.go @@ -0,0 +1,145 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha3 + +import ( + "testing" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" +) + +func TestAWSCluster_ValidateUpdate(t *testing.T) { + tests := []struct { + name string + oldCluster *AWSCluster + newCluster *AWSCluster + wantErr bool + }{ + { + name: "region is immutable", + oldCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + Region: "us-east-1", + }, + }, + newCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + Region: "us-east-2", + }, + }, + wantErr: true, + }, + { + name: "controlPlaneLoadBalancer is immutable", + oldCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{ + Scheme: &ClassicELBSchemeInternal, + }, + }, + }, + newCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{ + Scheme: &ClassicELBSchemeInternetFacing, + }, + }, + }, + wantErr: true, + }, + { + name: "controlPlaneEndpoint is immutable", + oldCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneEndpoint: clusterv1.APIEndpoint{ + Host: "example.com", + Port: int32(8000), + }, + }, + }, + newCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneEndpoint: clusterv1.APIEndpoint{ + Host: "foo.example.com", + Port: int32(9000), + }, + }, + }, + wantErr: true, + }, + { + name: "controlPlaneEndpoint can be updated if it is empty", + oldCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneEndpoint: clusterv1.APIEndpoint{}, + }, + }, + newCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneEndpoint: clusterv1.APIEndpoint{ + Host: "example.com", + Port: int32(8000), + }, + }, + }, + wantErr: false, + }, + { + name: "bastion can be added", + oldCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + Bastion: Bastion{ + Enabled: false, + }, + }, + }, + newCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + Bastion: Bastion{ + Enabled: true, + }, + }, + }, + wantErr: false, + }, + { + name: "bastion cannot be removed", + oldCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + Bastion: Bastion{ + Enabled: true, + }, + }, + }, + newCluster: &AWSCluster{ + Spec: AWSClusterSpec{ + Bastion: Bastion{ + Enabled: false, + }, + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := tt.newCluster.ValidateUpdate(tt.oldCluster); (err != nil) != tt.wantErr { + t.Errorf("ValidateUpdate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 0fd4967a0b..44ab382d42 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -6,6 +6,25 @@ metadata: creationTimestamp: null name: validating-webhook-configuration webhooks: +- clientConfig: + caBundle: Cg== + service: + name: webhook-service + namespace: system + path: /validate-infrastructure-cluster-x-k8s-io-v1alpha3-awscluster + failurePolicy: Fail + matchPolicy: Equivalent + name: validation.awscluster.infrastructure.cluster.x-k8s.io + rules: + - apiGroups: + - infrastructure.cluster.x-k8s.io + apiVersions: + - v1alpha3 + operations: + - CREATE + - UPDATE + resources: + - awsclusters - clientConfig: caBundle: Cg== service: From ff52703acb1552431bb4fd701400c23bda05c6db Mon Sep 17 00:00:00 2001 From: Ben Moss Date: Mon, 6 Apr 2020 15:26:40 +0000 Subject: [PATCH 2/2] Remove bastion validation, formatting --- api/v1alpha3/awscluster_webhook.go | 15 ++++++----- api/v1alpha3/awscluster_webhook_test.go | 36 ------------------------- 2 files changed, 8 insertions(+), 43 deletions(-) diff --git a/api/v1alpha3/awscluster_webhook.go b/api/v1alpha3/awscluster_webhook.go index 23a15ca53a..2df2bbf609 100644 --- a/api/v1alpha3/awscluster_webhook.go +++ b/api/v1alpha3/awscluster_webhook.go @@ -52,30 +52,31 @@ func (r *AWSCluster) ValidateDelete() error { func (r *AWSCluster) ValidateUpdate(old runtime.Object) error { var allErrs field.ErrorList + oldC, ok := old.(*AWSCluster) if !ok { return apierrors.NewBadRequest(fmt.Sprintf("expected an AWSCluster but got a %T", old)) } + if r.Spec.Region != oldC.Spec.Region { allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "region"), r.Spec.Region, "field is immutable"), ) } + if !reflect.DeepEqual(r.Spec.ControlPlaneLoadBalancer, oldC.Spec.ControlPlaneLoadBalancer) { allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer"), r.Spec.ControlPlaneLoadBalancer, "field is immutable"), + field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer"), + r.Spec.ControlPlaneLoadBalancer, "field is immutable"), ) } - if !reflect.DeepEqual(oldC.Spec.ControlPlaneEndpoint, clusterv1.APIEndpoint{}) && !reflect.DeepEqual(r.Spec.ControlPlaneEndpoint, oldC.Spec.ControlPlaneEndpoint) { + + if !reflect.DeepEqual(oldC.Spec.ControlPlaneEndpoint, clusterv1.APIEndpoint{}) && + !reflect.DeepEqual(r.Spec.ControlPlaneEndpoint, oldC.Spec.ControlPlaneEndpoint) { allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneEndpoint"), r.Spec.ControlPlaneEndpoint, "field is immutable"), ) } - if oldC.Spec.Bastion.Enabled && !r.Spec.Bastion.Enabled { - allErrs = append(allErrs, - field.Invalid(field.NewPath("spec", "bastion.enabled"), r.Spec.Bastion.Enabled, "cannot be disabled"), - ) - } return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) } diff --git a/api/v1alpha3/awscluster_webhook_test.go b/api/v1alpha3/awscluster_webhook_test.go index 7897bff4fa..52b9df578c 100644 --- a/api/v1alpha3/awscluster_webhook_test.go +++ b/api/v1alpha3/awscluster_webhook_test.go @@ -98,42 +98,6 @@ func TestAWSCluster_ValidateUpdate(t *testing.T) { }, wantErr: false, }, - { - name: "bastion can be added", - oldCluster: &AWSCluster{ - Spec: AWSClusterSpec{ - Bastion: Bastion{ - Enabled: false, - }, - }, - }, - newCluster: &AWSCluster{ - Spec: AWSClusterSpec{ - Bastion: Bastion{ - Enabled: true, - }, - }, - }, - wantErr: false, - }, - { - name: "bastion cannot be removed", - oldCluster: &AWSCluster{ - Spec: AWSClusterSpec{ - Bastion: Bastion{ - Enabled: true, - }, - }, - }, - newCluster: &AWSCluster{ - Spec: AWSClusterSpec{ - Bastion: Bastion{ - Enabled: false, - }, - }, - }, - wantErr: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {