diff --git a/api/v1alpha3/awsmachine_webhook.go b/api/v1alpha3/awsmachine_webhook.go index 3847473f21..cc5b712829 100644 --- a/api/v1alpha3/awsmachine_webhook.go +++ b/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 +} diff --git a/api/v1alpha3/awsmachine_webhook_test.go b/api/v1alpha3/awsmachine_webhook_test.go new file mode 100644 index 0000000000..c8b42e964e --- /dev/null +++ b/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) + } + }) + } +} diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 6f9c99cd30..5319b0df20 100644 --- a/config/webhook/manifests.yaml +++ b/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: diff --git a/controllers/awsmachine_controller.go b/controllers/awsmachine_controller.go index a52b4720ba..e301516d00 100644 --- a/controllers/awsmachine_controller.go +++ b/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 {