Add validation webhook for AWSMachine

api/v1alpha3/awsmachine_webhook.go

@@ -17,8 +17,15 @@ limitations under the License.
 package v1alpha3
 
 import (
+	"reflect"
+
+	"github.com/pkg/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
 
 // log is for logging in this package.
@@ -29,3 +36,59 @@ func (r *AWSMachine) SetupWebhookWithManager(mgr ctrl.Manager) error {
 		For(r).
 		Complete()
 }
+
+// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha3-awsmachine,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=awsmachines,versions=v1alpha3,name=validation.awsmachine.infrastructure.cluster.x-k8s.io
+
+var _ webhook.Validator = &AWSMachine{}
+
+// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
+func (r *AWSMachine) ValidateCreate() error {
+	return nil
+}
+
+// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
+func (r *AWSMachine) ValidateUpdate(old runtime.Object) error {
+	newAWSMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(r)
+	if err != nil {
+		return apierrors.NewInvalid(GroupVersion.WithKind("AWSMachine").GroupKind(), r.Name, field.ErrorList{
+			field.InternalError(nil, errors.Wrap(err, "failed to convert new AWSMachine to unstructured object")),
+		})
+	}
+	oldAWSMachine, err := runtime.DefaultUnstructuredConverter.ToUnstructured(old)
+	if err != nil {
+		return apierrors.NewInvalid(GroupVersion.WithKind("AWSMachine").GroupKind(), r.Name, field.ErrorList{
+			field.InternalError(nil, errors.Wrap(err, "failed to convert old AWSMachine to unstructured object")),
+		})
+	}
+
+	var allErrs field.ErrorList
+
+	newAWSMachineSpec := newAWSMachine["spec"].(map[string]interface{})
+	oldAWSMachineSpec := oldAWSMachine["spec"].(map[string]interface{})
+
+	// allow changes to providerID
+	delete(oldAWSMachineSpec, "providerID")
+	delete(newAWSMachineSpec, "providerID")
+
+	// allow changes to additionalTags
+	delete(oldAWSMachineSpec, "additionalTags")
+	delete(newAWSMachineSpec, "additionalTags")
+
+	// allow changes to additionalSecurityGroups
+	delete(oldAWSMachineSpec, "additionalSecurityGroups")
+	delete(newAWSMachineSpec, "additionalSecurityGroups")
+
+	if !reflect.DeepEqual(oldAWSMachineSpec, newAWSMachineSpec) {
+		allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified"))
+		return apierrors.NewInvalid(
+			GroupVersion.WithKind("AWSMachine").GroupKind(),
+			r.Name, allErrs)
+	}
+
+	return nil
+}
+
+// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
+func (r *AWSMachine) ValidateDelete() error {
+	return nil
+}

api/v1alpha3/awsmachine_webhook_test.go

@@ -0,0 +1,90 @@
+/*
+Copyright 2019 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"
+
+	"k8s.io/utils/pointer"
+)
+
+func TestAWSMachine_ValidateUpdate(t *testing.T) {
+	tests := []struct {
+		name       string
+		oldMachine *AWSMachine
+		newMachine *AWSMachine
+		wantErr    bool
+	}{
+		{
+			name: "change in providerid, tags and securitygroups",
+			oldMachine: &AWSMachine{
+				Spec: AWSMachineSpec{
+					ProviderID:               nil,
+					AdditionalTags:           nil,
+					AdditionalSecurityGroups: nil,
+				},
+			},
+			newMachine: &AWSMachine{
+				Spec: AWSMachineSpec{
+					ProviderID: pointer.StringPtr("ID"),
+					AdditionalTags: Tags{
+						"key-1": "value-1",
+					},
+					AdditionalSecurityGroups: []AWSResourceReference{
+						{
+							ID: pointer.StringPtr("ID"),
+						},
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "change in fields other than providerid, tags and securitygroups",
+			oldMachine: &AWSMachine{
+				Spec: AWSMachineSpec{
+					ProviderID:               nil,
+					AdditionalTags:           nil,
+					AdditionalSecurityGroups: nil,
+				},
+			},
+			newMachine: &AWSMachine{
+				Spec: AWSMachineSpec{
+					ImageLookupOrg: "test",
+					InstanceType:   "test",
+					ProviderID:     pointer.StringPtr("ID"),
+					AdditionalTags: Tags{
+						"key-1": "value-1",
+					},
+					AdditionalSecurityGroups: []AWSResourceReference{
+						{
+							ID: pointer.StringPtr("ID"),
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := tt.newMachine.ValidateUpdate(tt.oldMachine); (err != nil) != tt.wantErr {
+				t.Errorf("ValidateUpdate() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

config/webhook/manifests.yaml

@@ -6,6 +6,24 @@ 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-awsmachine
+  failurePolicy: Fail
+  name: validation.awsmachine.infrastructure.cluster.x-k8s.io
+  rules:
+  - apiGroups:
+    - infrastructure.cluster.x-k8s.io
+    apiVersions:
+    - v1alpha3
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - awsmachines
 - clientConfig:
     caBundle: Cg==
     service:

controllers/awsmachine_controller.go

@@ -20,12 +20,10 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/aws/aws-sdk-go/aws"
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/pointer"
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1alpha3"
@@ -369,14 +367,6 @@ func (r *AWSMachineReconciler) reconcileNormal(ctx context.Context, machineScope
 		return reconcile.Result{}, errors.Errorf("failed to ensure tags: %+v", err)
 	}
 
-	// TODO(ncdc): move this validation logic into a validating webhook
-	if errs := r.validateUpdate(&machineScope.AWSMachine.Spec, instance); len(errs) > 0 {
-		agg := kerrors.NewAggregate(errs)
-		machineScope.Info("Invalid update", "failedUpdates", agg.Error())
-		r.Recorder.Eventf(machineScope.AWSMachine, corev1.EventTypeWarning, "InvalidUpdate", "Invalid update: %s", agg.Error())
-		return reconcile.Result{}, nil
-	}
-
 	return reconcile.Result{}, nil
 }
 
@@ -412,67 +402,6 @@ func (r *AWSMachineReconciler) reconcileLBAttachment(machineScope *scope.Machine
 	return nil
 }
 
-// validateUpdate checks that no immutable fields have been updated and
-// returns a slice of errors representing attempts to change immutable state.
-func (r *AWSMachineReconciler) validateUpdate(spec *infrav1.AWSMachineSpec, i *infrav1.Instance) (errs []error) {
-
-	// EC2 instance attributes start disapearing during shutdown and termination, so do not
-	// perform more checks
-	if i.State == infrav1.InstanceStateTerminated || i.State == infrav1.InstanceStateShuttingDown {
-		return nil
-	}
-
-	// Instance Type
-	if spec.InstanceType != i.Type {
-		errs = append(errs, errors.Errorf("EC2 instance type cannot be mutated from %q to %q", i.Type, spec.InstanceType))
-	}
-
-	// IAM Profile
-	if spec.IAMInstanceProfile != i.IAMProfile {
-		errs = append(errs, errors.Errorf("EC2 instance IAM profile cannot be mutated from %q to %q", i.IAMProfile, spec.IAMInstanceProfile))
-	}
-
-	// SSH Key Name (also account for default)
-	if spec.SSHKeyName != aws.StringValue(i.SSHKeyName) && spec.SSHKeyName != "" {
-		errs = append(errs, errors.Errorf("SSH key name cannot be mutated from %q to %q", aws.StringValue(i.SSHKeyName), spec.SSHKeyName))
-	}
-
-	// Root Device Size
-	if spec.RootDeviceSize > 0 && spec.RootDeviceSize != i.RootDeviceSize {
-		errs = append(errs, errors.Errorf("Root volume size cannot be mutated from %v to %v", i.RootDeviceSize, spec.RootDeviceSize))
-	}
-
-	// Subnet ID
-	// spec.Subnet is a *AWSResourceReference and could technically be
-	// a *string, ARN or Filter. However, elsewhere in the code it is only used
-	// as a *string, so do the same here.
-	if spec.Subnet != nil {
-		if aws.StringValue(spec.Subnet.ID) != i.SubnetID {
-			errs = append(errs, errors.Errorf("machine subnet ID cannot be mutated from %q to %q",
-				i.SubnetID, aws.StringValue(spec.Subnet.ID)))
-		}
-	}
-
-	// PublicIP check is a little more complicated as the machineConfig is a
-	// simple bool indicating if the instance should have a public IP or not,
-	// while the instanceDescription contains the public IP assigned to the
-	// instance.
-	// Work out whether the instance already has a public IP or not based on
-	// the length of the PublicIP string. Anything >0 is assumed to mean it does
-	// have a public IP.
-	instanceHasPublicIP := false
-	if len(aws.StringValue(i.PublicIP)) > 0 {
-		instanceHasPublicIP = true
-	}
-
-	if aws.BoolValue(spec.PublicIP) != instanceHasPublicIP {
-		errs = append(errs, errors.Errorf(`public IP setting cannot be mutated from "%v" to "%v"`,
-			instanceHasPublicIP, aws.BoolValue(spec.PublicIP)))
-	}
-
-	return errs
-}
-
 // AWSClusterToAWSMachine is a handler.ToRequestsFunc to be used to enqeue requests for reconciliation
 // of AWSMachines.
 func (r *AWSMachineReconciler) AWSClusterToAWSMachines(o handler.MapObject) []ctrl.Request {