From 453a5b0bb8886a7f2dcc9f360977ab28c73ab8e3 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 11 Jun 2020 13:50:23 +0100 Subject: [PATCH 01/10] Switch Machine validation to not require validation/defaulting handlers --- pkg/apis/machine/v1beta1/machine_webhook.go | 111 +++++++++--------- .../machine/v1beta1/machine_webhook_test.go | 12 +- 2 files changed, 63 insertions(+), 60 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 7edc742bb6..607c9500e8 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -119,29 +119,28 @@ func getInfra() (*osconfigv1.Infrastructure, error) { return infra, nil } -type handlerValidationFn func(h *validatorHandler, m *Machine) (bool, utilerrors.Aggregate) -type handlerMutationFn func(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) +type machineAdmissionFn func(m *Machine, clusterID string) (bool, utilerrors.Aggregate) -// validatorHandler validates Machine API resources. +// machineValidatorHandler validates Machine API resources. // implements type Handler interface. // https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler -type validatorHandler struct { +type machineValidatorHandler struct { clusterID string - webhookOperations handlerValidationFn + webhookOperations machineAdmissionFn decoder *admission.Decoder } -// defaulterHandler defaults Machine API resources. +// machineDefaulterHandler defaults Machine API resources. // implements type Handler interface. // https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler -type defaulterHandler struct { +type machineDefaulterHandler struct { clusterID string - webhookOperations handlerMutationFn + webhookOperations machineAdmissionFn decoder *admission.Decoder } -// NewValidator returns a new validatorHandler. -func NewMachineValidator() (*validatorHandler, error) { +// NewValidator returns a new machineValidatorHandler. +func NewMachineValidator() (*machineValidatorHandler, error) { infra, err := getInfra() if err != nil { return nil, err @@ -150,29 +149,31 @@ func NewMachineValidator() (*validatorHandler, error) { return createMachineValidator(infra.Status.PlatformStatus.Type, infra.Status.InfrastructureName), nil } -func createMachineValidator(platform osconfigv1.PlatformType, clusterID string) *validatorHandler { - h := &validatorHandler{ - clusterID: clusterID, +func createMachineValidator(platform osconfigv1.PlatformType, clusterID string) *machineValidatorHandler { + return &machineValidatorHandler{ + clusterID: clusterID, + webhookOperations: getMachineValidatorOperation(platform), } +} +func getMachineValidatorOperation(platform osconfigv1.PlatformType) machineAdmissionFn { switch platform { case osconfigv1.AWSPlatformType: - h.webhookOperations = validateAWS + return validateAWS case osconfigv1.AzurePlatformType: - h.webhookOperations = validateAzure + return validateAzure case osconfigv1.GCPPlatformType: - h.webhookOperations = validateGCP + return validateGCP default: // just no-op - h.webhookOperations = func(h *validatorHandler, m *Machine) (bool, utilerrors.Aggregate) { + return func(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { return true, nil } } - return h } -// NewDefaulter returns a new defaulterHandler. -func NewMachineDefaulter() (*defaulterHandler, error) { +// NewDefaulter returns a new machineDefaulterHandler. +func NewMachineDefaulter() (*machineDefaulterHandler, error) { infra, err := getInfra() if err != nil { return nil, err @@ -181,41 +182,43 @@ func NewMachineDefaulter() (*defaulterHandler, error) { return createMachineDefaulter(infra.Status.PlatformStatus, infra.Status.InfrastructureName), nil } -func createMachineDefaulter(platformStatus *osconfigv1.PlatformStatus, clusterID string) *defaulterHandler { - h := &defaulterHandler{ - clusterID: clusterID, +func createMachineDefaulter(platformStatus *osconfigv1.PlatformStatus, clusterID string) *machineDefaulterHandler { + return &machineDefaulterHandler{ + clusterID: clusterID, + webhookOperations: getMachineDefaulterOperation(platformStatus), } +} +func getMachineDefaulterOperation(platformStatus *osconfigv1.PlatformStatus) machineAdmissionFn { switch platformStatus.Type { case osconfigv1.AWSPlatformType: - h.webhookOperations = defaultAWS + return defaultAWS case osconfigv1.AzurePlatformType: - h.webhookOperations = defaultAzure + return defaultAzure case osconfigv1.GCPPlatformType: - h.webhookOperations = gcpDefaulter{projectID: platformStatus.GCP.ProjectID}.defaultGCP + return gcpDefaulter{projectID: platformStatus.GCP.ProjectID}.defaultGCP default: // just no-op - h.webhookOperations = func(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) { + return func(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { return true, nil } } - return h } // InjectDecoder injects the decoder. -func (v *validatorHandler) InjectDecoder(d *admission.Decoder) error { +func (v *machineValidatorHandler) InjectDecoder(d *admission.Decoder) error { v.decoder = d return nil } // InjectDecoder injects the decoder. -func (v *defaulterHandler) InjectDecoder(d *admission.Decoder) error { +func (v *machineDefaulterHandler) InjectDecoder(d *admission.Decoder) error { v.decoder = d return nil } // Handle handles HTTP requests for admission webhook servers. -func (h *validatorHandler) Handle(ctx context.Context, req admission.Request) admission.Response { +func (h *machineValidatorHandler) Handle(ctx context.Context, req admission.Request) admission.Response { m := &Machine{} if err := h.decoder.Decode(req, m); err != nil { @@ -224,7 +227,7 @@ func (h *validatorHandler) Handle(ctx context.Context, req admission.Request) ad klog.V(3).Infof("Validate webhook called for Machine: %s", m.GetName()) - if ok, err := h.webhookOperations(h, m); !ok { + if ok, err := h.webhookOperations(m, h.clusterID); !ok { return admission.Denied(err.Error()) } @@ -232,7 +235,7 @@ func (h *validatorHandler) Handle(ctx context.Context, req admission.Request) ad } // Handle handles HTTP requests for admission webhook servers. -func (h *defaulterHandler) Handle(ctx context.Context, req admission.Request) admission.Response { +func (h *machineDefaulterHandler) Handle(ctx context.Context, req admission.Request) admission.Response { m := &Machine{} if err := h.decoder.Decode(req, m); err != nil { @@ -241,7 +244,7 @@ func (h *defaulterHandler) Handle(ctx context.Context, req admission.Request) ad klog.V(3).Infof("Mutate webhook called for Machine: %s", m.GetName()) - if ok, err := h.webhookOperations(h, m); !ok { + if ok, err := h.webhookOperations(m, h.clusterID); !ok { return admission.Denied(err.Error()) } @@ -252,7 +255,7 @@ func (h *defaulterHandler) Handle(ctx context.Context, req admission.Request) ad return admission.PatchResponseFromRaw(req.Object.Raw, marshaledMachine) } -func defaultAWS(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) { +func defaultAWS(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { klog.V(3).Infof("Defaulting AWS providerSpec") var errs []error @@ -266,7 +269,7 @@ func defaultAWS(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) { providerSpec.InstanceType = defaultAWSInstanceType } if providerSpec.IAMInstanceProfile == nil { - providerSpec.IAMInstanceProfile = &aws.AWSResourceReference{ID: defaultAWSIAMInstanceProfile(h.clusterID)} + providerSpec.IAMInstanceProfile = &aws.AWSResourceReference{ID: defaultAWSIAMInstanceProfile(clusterID)} } if providerSpec.UserDataSecret == nil { providerSpec.UserDataSecret = &corev1.LocalObjectReference{Name: defaultUserDataSecret} @@ -282,7 +285,7 @@ func defaultAWS(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) { Filters: []aws.Filter{ { Name: "tag:Name", - Values: []string{defaultAWSSecurityGroup(h.clusterID)}, + Values: []string{defaultAWSSecurityGroup(clusterID)}, }, }, }, @@ -293,7 +296,7 @@ func defaultAWS(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) { providerSpec.Subnet.Filters = []aws.Filter{ { Name: "tag:Name", - Values: []string{defaultAWSSubnet(h.clusterID, providerSpec.Placement.AvailabilityZone)}, + Values: []string{defaultAWSSubnet(clusterID, providerSpec.Placement.AvailabilityZone)}, }, } } @@ -318,7 +321,7 @@ func unmarshalInto(m *Machine, providerSpec interface{}) error { return nil } -func validateAWS(h *validatorHandler, m *Machine) (bool, utilerrors.Aggregate) { +func validateAWS(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { klog.V(3).Infof("Validating AWS providerSpec") var errs []error @@ -407,7 +410,7 @@ func validateAWS(h *validatorHandler, m *Machine) (bool, utilerrors.Aggregate) { return true, nil } -func defaultAzure(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) { +func defaultAzure(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { klog.V(3).Infof("Defaulting Azure providerSpec") var errs []error @@ -423,26 +426,26 @@ func defaultAzure(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) // Vnet and Subnet need to be provided together by the user if providerSpec.Vnet == "" && providerSpec.Subnet == "" { - providerSpec.Vnet = defaultAzureVnet(h.clusterID) - providerSpec.Subnet = defaultAzureSubnet(h.clusterID) + providerSpec.Vnet = defaultAzureVnet(clusterID) + providerSpec.Subnet = defaultAzureSubnet(clusterID) // NetworkResourceGroup can be set by the user without Vnet and Subnet, // only override if they didn't set it if providerSpec.NetworkResourceGroup == "" { - providerSpec.NetworkResourceGroup = defaultAzureNetworkResourceGroup(h.clusterID) + providerSpec.NetworkResourceGroup = defaultAzureNetworkResourceGroup(clusterID) } } if providerSpec.Image.ResourceID == "" { - providerSpec.Image.ResourceID = defaultAzureImageResourceID(h.clusterID) + providerSpec.Image.ResourceID = defaultAzureImageResourceID(clusterID) } if providerSpec.ManagedIdentity == "" { - providerSpec.ManagedIdentity = defaultAzureManagedIdentiy(h.clusterID) + providerSpec.ManagedIdentity = defaultAzureManagedIdentiy(clusterID) } if providerSpec.ResourceGroup == "" { - providerSpec.ResourceGroup = defaultAzureResourceGroup(h.clusterID) + providerSpec.ResourceGroup = defaultAzureResourceGroup(clusterID) } if providerSpec.UserDataSecret == nil { @@ -483,7 +486,7 @@ func defaultAzure(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) return true, nil } -func validateAzure(h *validatorHandler, m *Machine) (bool, utilerrors.Aggregate) { +func validateAzure(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { klog.V(3).Infof("Validating Azure providerSpec") var errs []error @@ -566,7 +569,7 @@ type gcpDefaulter struct { projectID string } -func (g gcpDefaulter) defaultGCP(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate) { +func (g gcpDefaulter) defaultGCP(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { klog.V(3).Infof("Defaulting GCP providerSpec") var errs []error @@ -582,15 +585,15 @@ func (g gcpDefaulter) defaultGCP(h *defaulterHandler, m *Machine) (bool, utilerr if len(providerSpec.NetworkInterfaces) == 0 { providerSpec.NetworkInterfaces = append(providerSpec.NetworkInterfaces, &gcp.GCPNetworkInterface{ - Network: defaultGCPNetwork(h.clusterID), - Subnetwork: defaultGCPSubnetwork(h.clusterID), + Network: defaultGCPNetwork(clusterID), + Subnetwork: defaultGCPSubnetwork(clusterID), }) } - providerSpec.Disks = defaultGCPDisks(providerSpec.Disks, h.clusterID) + providerSpec.Disks = defaultGCPDisks(providerSpec.Disks, clusterID) if len(providerSpec.Tags) == 0 { - providerSpec.Tags = defaultGCPTags(h.clusterID) + providerSpec.Tags = defaultGCPTags(clusterID) } if providerSpec.UserDataSecret == nil { @@ -602,7 +605,7 @@ func (g gcpDefaulter) defaultGCP(h *defaulterHandler, m *Machine) (bool, utilerr } if len(providerSpec.ServiceAccounts) == 0 { - providerSpec.ServiceAccounts = defaultGCPServiceAccounts(h.clusterID, g.projectID) + providerSpec.ServiceAccounts = defaultGCPServiceAccounts(clusterID, g.projectID) } rawBytes, err := json.Marshal(providerSpec) @@ -644,7 +647,7 @@ func defaultGCPDisks(disks []*gcp.GCPDisk, clusterID string) []*gcp.GCPDisk { return disks } -func validateGCP(h *validatorHandler, m *Machine) (bool, utilerrors.Aggregate) { +func validateGCP(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { klog.V(3).Infof("Validating GCP providerSpec") var errs []error diff --git a/pkg/apis/machine/v1beta1/machine_webhook_test.go b/pkg/apis/machine/v1beta1/machine_webhook_test.go index f476279039..7835c31ba9 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machine_webhook_test.go @@ -124,7 +124,7 @@ func TestValidateAWSProviderSpec(t *testing.T) { } m.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: rawBytes} - ok, err := h.webhookOperations(h, m) + ok, err := h.webhookOperations(m, h.clusterID) if ok != tc.expectedOk { t.Errorf("expected: %v, got: %v", tc.expectedOk, ok) } @@ -215,7 +215,7 @@ func TestDefaultAWSProviderSpec(t *testing.T) { } m.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: rawBytes} - ok, err := h.webhookOperations(h, m) + ok, err := h.webhookOperations(m, h.clusterID) if ok != tc.expectedOk { t.Errorf("expected: %v, got: %v", tc.expectedOk, ok) } @@ -446,7 +446,7 @@ func TestValidateAzureProviderSpec(t *testing.T) { } m.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: rawBytes} - ok, err := h.webhookOperations(h, m) + ok, err := h.webhookOperations(m, h.clusterID) if ok != tc.expectedOk { t.Errorf("expected: %v, got: %v", tc.expectedOk, ok) } @@ -563,7 +563,7 @@ func TestDefaultAzureProviderSpec(t *testing.T) { } m.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: rawBytes} - ok, err := h.webhookOperations(h, m) + ok, err := h.webhookOperations(m, h.clusterID) if ok != tc.expectedOk { t.Errorf("expected: %v, got: %v", tc.expectedOk, ok) } @@ -832,7 +832,7 @@ func TestValidateGCPProviderSpec(t *testing.T) { } m.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: rawBytes} - ok, err := h.webhookOperations(h, m) + ok, err := h.webhookOperations(m, h.clusterID) if ok != tc.expectedOk { t.Errorf("expected: %v, got: %v", tc.expectedOk, ok) } @@ -941,7 +941,7 @@ func TestDefaultGCPProviderSpec(t *testing.T) { } m.Spec.ProviderSpec.Value = &runtime.RawExtension{Raw: rawBytes} - ok, err := h.webhookOperations(h, m) + ok, err := h.webhookOperations(m, h.clusterID) if ok != tc.expectedOk { t.Errorf("expected: %v, got: %v", tc.expectedOk, ok) } From 05bd80cbb40ce7a005a14da4e39f52f7b2a2d51d Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 11 Jun 2020 14:14:01 +0100 Subject: [PATCH 02/10] Add MachineSet defaulting and validating webhook --- cmd/machineset/main.go | 20 ++- ...00_30_machine-api-operator_08_webhook.yaml | 41 +++++ .../machine/v1beta1/machineset_webhook.go | 147 ++++++++++++++++++ 3 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 pkg/apis/machine/v1beta1/machineset_webhook.go diff --git a/cmd/machineset/main.go b/cmd/machineset/main.go index 91d3d57546..80c23fe6a6 100644 --- a/cmd/machineset/main.go +++ b/cmd/machineset/main.go @@ -80,12 +80,22 @@ func main() { } // Enable defaulting and validating webhooks - defaulter, err := v1beta1.NewMachineDefaulter() + machineDefaulter, err := v1beta1.NewMachineDefaulter() if err != nil { log.Fatal(err) } - validator, err := v1beta1.NewMachineValidator() + machineValidator, err := v1beta1.NewMachineValidator() + if err != nil { + log.Fatal(err) + } + + machineSetDefaulter, err := v1beta1.NewMachineSetDefaulter() + if err != nil { + log.Fatal(err) + } + + machineSetValidator, err := v1beta1.NewMachineSetValidator() if err != nil { log.Fatal(err) } @@ -93,8 +103,10 @@ func main() { if *webhookEnabled { mgr.GetWebhookServer().Port = *webhookPort mgr.GetWebhookServer().CertDir = *webhookCertdir - mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: defaulter}) - mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: validator}) + mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineDefaulter}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineValidator}) + mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) } log.Printf("Registering Components.") diff --git a/install/0000_30_machine-api-operator_08_webhook.yaml b/install/0000_30_machine-api-operator_08_webhook.yaml index 2f71c2fb85..5e13a3280b 100644 --- a/install/0000_30_machine-api-operator_08_webhook.yaml +++ b/install/0000_30_machine-api-operator_08_webhook.yaml @@ -28,6 +28,26 @@ webhooks: resources: - machines sideEffects: None + - clientConfig: + service: + name: machine-api-operator-webhook + namespace: openshift-machine-api + path: /mutate-machine-openshift-io-v1beta1-machineset + # failurePolicy is ignore so we don't want to block machine lifecycle on the webhook operational aspects. + # This would be particularly problematic for chicken egg issues when bootstrapping a cluster. + failurePolicy: Ignore + matchPolicy: Equivalent + name: default.machineset.machine.openshift.io + rules: + - apiGroups: + - machine.openshift.io + apiVersions: + - v1beta1 + operations: + - CREATE + resources: + - machinesets + sideEffects: None --- apiVersion: admissionregistration.k8s.io/v1beta1 kind: ValidatingWebhookConfiguration @@ -59,3 +79,24 @@ webhooks: resources: - machines sideEffects: None + - clientConfig: + service: + name: machine-api-operator-webhook + namespace: openshift-machine-api + path: /validate-machine-openshift-io-v1beta1-machineset + # failurePolicy is ignore so we don't want to block machine lifecycle on the webhook operational aspects. + # This would be particularly problematic for chicken egg issues when bootstrapping a cluster. + failurePolicy: Ignore + matchPolicy: Equivalent + name: validation.machineset.machine.openshift.io + rules: + - apiGroups: + - machine.openshift.io + apiVersions: + - v1beta1 + operations: + - CREATE + - UPDATE + resources: + - machinesets + sideEffects: None diff --git a/pkg/apis/machine/v1beta1/machineset_webhook.go b/pkg/apis/machine/v1beta1/machineset_webhook.go new file mode 100644 index 0000000000..29caea1ead --- /dev/null +++ b/pkg/apis/machine/v1beta1/machineset_webhook.go @@ -0,0 +1,147 @@ +package v1beta1 + +import ( + "context" + "encoding/json" + "net/http" + + osconfigv1 "github.com/openshift/api/config/v1" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/klog" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// machineSetValidatorHandler validates MachineSet API resources. +// implements type Handler interface. +// https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler +type machineSetValidatorHandler struct { + clusterID string + webhookOperations machineAdmissionFn + decoder *admission.Decoder +} + +// machineSetDefaulterHandler defaults MachineSet API resources. +// implements type Handler interface. +// https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler +type machineSetDefaulterHandler struct { + clusterID string + webhookOperations machineAdmissionFn + decoder *admission.Decoder +} + +// NewMachineSetValidator returns a new machineSetValidatorHandler. +func NewMachineSetValidator() (*machineSetValidatorHandler, error) { + infra, err := getInfra() + if err != nil { + return nil, err + } + + return createMachineSetValidator(infra.Status.PlatformStatus.Type, infra.Status.InfrastructureName), nil +} + +func createMachineSetValidator(platform osconfigv1.PlatformType, clusterID string) *machineSetValidatorHandler { + return &machineSetValidatorHandler{ + clusterID: clusterID, + webhookOperations: getMachineValidatorOperation(platform), + } +} + +// NewMachineSetDefaulter returns a new machineSetDefaulterHandler. +func NewMachineSetDefaulter() (*machineSetDefaulterHandler, error) { + infra, err := getInfra() + if err != nil { + return nil, err + } + + return createMachineSetDefaulter(infra.Status.PlatformStatus, infra.Status.InfrastructureName), nil +} + +func createMachineSetDefaulter(platformStatus *osconfigv1.PlatformStatus, clusterID string) *machineSetDefaulterHandler { + return &machineSetDefaulterHandler{ + clusterID: clusterID, + webhookOperations: getMachineDefaulterOperation(platformStatus), + } +} + +// InjectDecoder injects the decoder. +func (v *machineSetValidatorHandler) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} + +// InjectDecoder injects the decoder. +func (v *machineSetDefaulterHandler) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} + +// Handle handles HTTP requests for admission webhook servers. +func (h *machineSetValidatorHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + ms := &MachineSet{} + + if err := h.decoder.Decode(req, ms); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + klog.V(3).Infof("Validate webhook called for MachineSet: %s", ms.GetName()) + + if ok, err := h.validateMachineSet(ms); !ok { + return admission.Denied(err.Error()) + } + + return admission.Allowed("MachineSet valid") +} + +// Handle handles HTTP requests for admission webhook servers. +func (h *machineSetDefaulterHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + ms := &MachineSet{} + + if err := h.decoder.Decode(req, ms); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + klog.V(3).Infof("Mutate webhook called for MachineSet: %s", ms.GetName()) + + if ok, err := h.defaultMachineSet(ms); !ok { + return admission.Denied(err.Error()) + } + + marshaledMachineSet, err := json.Marshal(ms) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } + return admission.PatchResponseFromRaw(req.Object.Raw, marshaledMachineSet) +} + +func (h *machineSetValidatorHandler) validateMachineSet(ms *MachineSet) (bool, utilerrors.Aggregate) { + var errs []error + + // Create a Machine from the MachineSet and validate the Machine template + m := &Machine{Spec: ms.Spec.Template.Spec} + if ok, err := h.webhookOperations(m, h.clusterID); !ok { + errs = append(errs, err.Errors()...) + } + + if len(errs) > 0 { + return false, utilerrors.NewAggregate(errs) + } + return true, nil +} + +func (h *machineSetDefaulterHandler) defaultMachineSet(ms *MachineSet) (bool, utilerrors.Aggregate) { + var errs []error + + // Create a Machine from the MachineSet and default the Machine template + m := &Machine{Spec: ms.Spec.Template.Spec} + if ok, err := h.webhookOperations(m, h.clusterID); !ok { + errs = append(errs, err.Errors()...) + } else { + // Restore the defaulted template + ms.Spec.Template.Spec = m.Spec + } + + if len(errs) > 0 { + return false, utilerrors.NewAggregate(errs) + } + return true, nil +} From c754eb5850ef516de2dbaf3849fda6a0e9930ab0 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 11 Jun 2020 17:32:44 +0100 Subject: [PATCH 03/10] Add tests for Machine/MachineSet creation/update via envtest --- pkg/apis/machine/v1beta1/machine_webhook.go | 6 +- .../machine/v1beta1/machine_webhook_test.go | 579 +++++++++++++++++ .../v1beta1/machineset_webhook_test.go | 600 ++++++++++++++++++ .../machine/v1beta1/v1beta1_suite_test.go | 29 +- 4 files changed, 1207 insertions(+), 7 deletions(-) create mode 100644 pkg/apis/machine/v1beta1/machineset_webhook_test.go diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 607c9500e8..3a30015e68 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -196,7 +196,11 @@ func getMachineDefaulterOperation(platformStatus *osconfigv1.PlatformStatus) mac case osconfigv1.AzurePlatformType: return defaultAzure case osconfigv1.GCPPlatformType: - return gcpDefaulter{projectID: platformStatus.GCP.ProjectID}.defaultGCP + projectID := "" + if platformStatus.GCP != nil { + projectID = platformStatus.GCP.ProjectID + } + return gcpDefaulter{projectID: projectID}.defaultGCP default: // just no-op return func(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { diff --git a/pkg/apis/machine/v1beta1/machine_webhook_test.go b/pkg/apis/machine/v1beta1/machine_webhook_test.go index 7835c31ba9..0138f3b1cd 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machine_webhook_test.go @@ -4,17 +4,596 @@ import ( "encoding/json" "testing" + . "github.com/onsi/gomega" osconfigv1 "github.com/openshift/api/config/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" "k8s.io/utils/pointer" aws "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsprovider/v1beta1" azure "sigs.k8s.io/cluster-api-provider-azure/pkg/apis/azureprovider/v1beta1" gcp "sigs.k8s.io/cluster-api-provider-gcp/pkg/apis/gcpprovider/v1beta1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/config" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" yaml "sigs.k8s.io/yaml" ) +func TestMachineCreation(t *testing.T) { + g := NewWithT(t) + + // Override config getter + ctrl.GetConfig = func() (*rest.Config, error) { + return cfg, nil + } + defer func() { + ctrl.GetConfig = config.GetConfig + }() + + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-creation-test", + }, + } + g.Expect(c.Create(ctx, namespace)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, namespace)).To(Succeed()) + }() + + infra := &osconfigv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + } + g.Expect(c.Create(ctx, infra)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, infra)).To(Succeed()) + }() + + testCases := []struct { + name string + platformType osconfigv1.PlatformType + clusterID string + expectedError string + providerSpecValue *runtime.RawExtension + }{ + { + name: "with AWS and no fields set", + platformType: osconfigv1.AWSPlatformType, + clusterID: "aws-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &aws.AWSMachineProviderConfig{}, + }, + expectedError: "providerSpec.ami: Required value: expected either providerSpec.ami.arn or providerSpec.ami.filters or providerSpec.ami.id to be populated", + }, + { + name: "with AWS and an AMI ID set", + platformType: osconfigv1.AWSPlatformType, + clusterID: "aws-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &aws.AWSMachineProviderConfig{ + AMI: aws.AWSResourceReference{ + ID: pointer.StringPtr("ami"), + }, + }, + }, + expectedError: "", + }, + { + name: "with Azure and no fields set", + platformType: osconfigv1.AzurePlatformType, + clusterID: "azure-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &azure.AzureMachineProviderSpec{}, + }, + expectedError: "[providerSpec.location: Required value: location should be set to one of the supported Azure regions, providerSpec.osDisk.diskSizeGB: Invalid value: 0: diskSizeGB must be greater than zero]", + }, + { + name: "with Azure and a location and disk size set", + platformType: osconfigv1.AzurePlatformType, + clusterID: "azure-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &azure.AzureMachineProviderSpec{ + Location: "location", + OSDisk: azure.OSDisk{ + DiskSizeGB: 128, + }, + }, + }, + expectedError: "", + }, + { + name: "with GCP and no fields set", + platformType: osconfigv1.GCPPlatformType, + clusterID: "gcp-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &gcp.GCPMachineProviderSpec{}, + }, + expectedError: "providerSpec.region: Required value: region is required", + }, + { + name: "with GCP and the region and zone set", + platformType: osconfigv1.GCPPlatformType, + clusterID: "gcp-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &gcp.GCPMachineProviderSpec{ + Region: "region", + Zone: "region-zone", + }, + }, + expectedError: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gs := NewWithT(t) + + mgr, err := manager.New(cfg, manager.Options{ + MetricsBindAddress: "0", + Port: testEnv.WebhookInstallOptions.LocalServingPort, + CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + }) + gs.Expect(err).ToNot(HaveOccurred()) + + done := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + }() + defer close(done) + + infra.Status = osconfigv1.InfrastructureStatus{ + InfrastructureName: tc.clusterID, + PlatformStatus: &osconfigv1.PlatformStatus{ + Type: tc.platformType, + GCP: &osconfigv1.GCPPlatformStatus{ + ProjectID: "gcp-project-id", + }, + }, + } + gs.Expect(c.Status().Update(ctx, infra)).To(Succeed()) + + machineDefaulter, err := NewMachineDefaulter() + gs.Expect(err).ToNot(HaveOccurred()) + machineValidator, err := NewMachineValidator() + gs.Expect(err).ToNot(HaveOccurred()) + mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineDefaulter}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineValidator}) + + m := &Machine{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "machine-creation-", + Namespace: namespace.Name, + }, + Spec: MachineSpec{ + ProviderSpec: ProviderSpec{ + Value: tc.providerSpecValue, + }, + }, + } + err = c.Create(ctx, m) + if err == nil { + defer func() { + gs.Expect(c.Delete(ctx, m)).To(Succeed()) + }() + } + + if tc.expectedError != "" { + gs.Expect(err).ToNot(BeNil()) + gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError)) + } else { + gs.Expect(err).To(BeNil()) + } + }) + } +} + +func TestMachineUpdate(t *testing.T) { + awsClusterID := "aws-cluster" + defaultAWSProviderSpec := &aws.AWSMachineProviderConfig{ + AMI: aws.AWSResourceReference{ + ID: pointer.StringPtr("ami"), + }, + InstanceType: defaultAWSInstanceType, + IAMInstanceProfile: &aws.AWSResourceReference{ + ID: defaultAWSIAMInstanceProfile(awsClusterID), + }, + UserDataSecret: &corev1.LocalObjectReference{Name: defaultUserDataSecret}, + CredentialsSecret: &corev1.LocalObjectReference{Name: defaultAWSCredentialsSecret}, + SecurityGroups: []aws.AWSResourceReference{ + { + Filters: []aws.Filter{ + { + Name: "tag:Name", + Values: []string{defaultAWSSecurityGroup(awsClusterID)}, + }, + }, + }, + }, + Placement: aws.Placement{ + Region: "region", + AvailabilityZone: "zone", + }, + Subnet: aws.AWSResourceReference{ + Filters: []aws.Filter{ + { + Name: "tag:Name", + Values: []string{defaultAWSSubnet(awsClusterID, "zone")}, + }, + }, + }, + } + + azureClusterID := "azure-cluster" + defaultAzureProviderSpec := &azure.AzureMachineProviderSpec{ + Location: "location", + VMSize: defaultAzureVMSize, + Vnet: defaultAzureVnet(azureClusterID), + Subnet: defaultAzureSubnet(azureClusterID), + NetworkResourceGroup: defaultAzureNetworkResourceGroup(azureClusterID), + Image: azure.Image{ + ResourceID: defaultAzureImageResourceID(azureClusterID), + }, + ManagedIdentity: defaultAzureManagedIdentiy(azureClusterID), + ResourceGroup: defaultAzureResourceGroup(azureClusterID), + UserDataSecret: &corev1.SecretReference{ + Name: defaultUserDataSecret, + Namespace: defaultSecretNamespace, + }, + CredentialsSecret: &corev1.SecretReference{ + Name: defaultAzureCredentialsSecret, + Namespace: defaultSecretNamespace, + }, + OSDisk: azure.OSDisk{ + DiskSizeGB: 128, + OSType: defaultAzureOSDiskOSType, + ManagedDisk: azure.ManagedDisk{ + StorageAccountType: defaultAzureOSDiskStorageType, + }, + }, + } + + gcpClusterID := "gcp-cluster" + gcpProjectID := "gcp-project-id" + defaultGCPProviderSpec := &gcp.GCPMachineProviderSpec{ + Region: "region", + Zone: "region-zone", + MachineType: defaultGCPMachineType, + NetworkInterfaces: []*gcp.GCPNetworkInterface{ + { + Network: defaultGCPNetwork(gcpClusterID), + Subnetwork: defaultGCPSubnetwork(gcpClusterID), + }, + }, + Disks: []*gcp.GCPDisk{ + { + AutoDelete: true, + Boot: true, + SizeGb: defaultGCPDiskSizeGb, + Type: defaultGCPDiskType, + Image: defaultGCPDiskImage(gcpClusterID), + }, + }, + ServiceAccounts: defaultGCPServiceAccounts(gcpClusterID, gcpProjectID), + Tags: defaultGCPTags(gcpClusterID), + UserDataSecret: &corev1.LocalObjectReference{ + Name: defaultUserDataSecret, + }, + CredentialsSecret: &corev1.LocalObjectReference{ + Name: defaultGCPCredentialsSecret, + }, + } + + g := NewWithT(t) + + // Override config getter + ctrl.GetConfig = func() (*rest.Config, error) { + return cfg, nil + } + defer func() { + ctrl.GetConfig = config.GetConfig + }() + + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine-update-test", + }, + } + g.Expect(c.Create(ctx, namespace)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, namespace)).To(Succeed()) + }() + + infra := &osconfigv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + } + g.Expect(c.Create(ctx, infra)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, infra)).To(Succeed()) + }() + + testCases := []struct { + name string + platformType osconfigv1.PlatformType + clusterID string + expectedError string + baseProviderSpecValue *runtime.RawExtension + updatedProviderSpecValue func() *runtime.RawExtension + }{ + { + name: "with a valid AWS ProviderSpec", + platformType: osconfigv1.AWSPlatformType, + clusterID: awsClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + return &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + } + }, + expectedError: "", + }, + { + name: "with an AWS ProviderSpec, removing the instance type", + platformType: osconfigv1.AWSPlatformType, + clusterID: awsClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAWSProviderSpec.DeepCopy() + object.InstanceType = "" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.instanceType: Required value: expected providerSpec.instanceType to be populated", + }, + { + name: "with an AWS ProviderSpec, removing the instance profile", + platformType: osconfigv1.AWSPlatformType, + clusterID: awsClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAWSProviderSpec.DeepCopy() + object.IAMInstanceProfile = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.iamInstanceProfile: Required value: expected providerSpec.iamInstanceProfile to be populated", + }, + { + name: "with an AWS ProviderSpec, removing the user data secret", + platformType: osconfigv1.AWSPlatformType, + clusterID: awsClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAWSProviderSpec.DeepCopy() + object.UserDataSecret = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.userDataSecret: Required value: expected providerSpec.userDataSecret to be populated", + }, + { + name: "with a valid Azure ProviderSpec", + platformType: osconfigv1.AzurePlatformType, + clusterID: azureClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + return &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + } + }, + expectedError: "", + }, + { + name: "with an Azure ProviderSpec, removing the vm size", + platformType: osconfigv1.AzurePlatformType, + clusterID: azureClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAzureProviderSpec.DeepCopy() + object.VMSize = "" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.vmSize: Required value: vmSize should be set to one of the supported Azure VM sizes", + }, + { + name: "with an Azure ProviderSpec, removing the subnet", + platformType: osconfigv1.AzurePlatformType, + clusterID: azureClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAzureProviderSpec.DeepCopy() + object.Subnet = "" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.subnet: Required value: must provide a subnet when a virtual network is specified", + }, + { + name: "with an Azure ProviderSpec, removing the credentials secret", + platformType: osconfigv1.AzurePlatformType, + clusterID: azureClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAzureProviderSpec.DeepCopy() + object.CredentialsSecret = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.credentialsSecret: Required value: credentialsSecret must be provided", + }, + { + name: "with a valid GCP ProviderSpec", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + return &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + } + }, + expectedError: "", + }, + { + name: "with a GCP ProviderSpec, removing the region", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultGCPProviderSpec.DeepCopy() + object.Region = "" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.region: Required value: region is required", + }, + { + name: "with a GCP ProviderSpec, and an invalid region", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultGCPProviderSpec.DeepCopy() + object.Zone = "zone" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.zone: Invalid value: \"zone\": zone not in configured region (region)", + }, + { + name: "with a GCP ProviderSpec, removing the disks", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultGCPProviderSpec.DeepCopy() + object.Disks = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.disks: Required value: at least 1 disk is required", + }, + { + name: "with a GCP ProviderSpec, removing the service accounts", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultGCPProviderSpec.DeepCopy() + object.ServiceAccounts = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.serviceAccounts: Invalid value: \"0 service accounts supplied\": exactly 1 service account must be supplied", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gs := NewWithT(t) + + mgr, err := manager.New(cfg, manager.Options{ + MetricsBindAddress: "0", + Port: testEnv.WebhookInstallOptions.LocalServingPort, + CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + }) + gs.Expect(err).ToNot(HaveOccurred()) + + done := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + }() + defer close(done) + + infra.Status = osconfigv1.InfrastructureStatus{ + InfrastructureName: tc.clusterID, + PlatformStatus: &osconfigv1.PlatformStatus{ + Type: tc.platformType, + GCP: &osconfigv1.GCPPlatformStatus{ + ProjectID: gcpProjectID, + }, + }, + } + gs.Expect(c.Status().Update(ctx, infra)).To(Succeed()) + + machineDefaulter, err := NewMachineDefaulter() + gs.Expect(err).ToNot(HaveOccurred()) + machineValidator, err := NewMachineValidator() + gs.Expect(err).ToNot(HaveOccurred()) + mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineDefaulter}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineValidator}) + + m := &Machine{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "machine-creation-", + Namespace: namespace.Name, + }, + Spec: MachineSpec{ + ProviderSpec: ProviderSpec{ + Value: tc.baseProviderSpecValue, + }, + }, + } + err = c.Create(ctx, m) + gs.Expect(err).ToNot(HaveOccurred()) + defer func() { + gs.Expect(c.Delete(ctx, m)).To(Succeed()) + }() + + m.Spec.ProviderSpec.Value = tc.updatedProviderSpecValue() + err = c.Update(ctx, m) + if tc.expectedError != "" { + gs.Expect(err).ToNot(BeNil()) + gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError)) + } else { + gs.Expect(err).To(BeNil()) + } + }) + } +} + func TestValidateAWSProviderSpec(t *testing.T) { testCases := []struct { diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go new file mode 100644 index 0000000000..f837965b43 --- /dev/null +++ b/pkg/apis/machine/v1beta1/machineset_webhook_test.go @@ -0,0 +1,600 @@ +package v1beta1 + +import ( + "testing" + + . "github.com/onsi/gomega" + osconfigv1 "github.com/openshift/api/config/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" + "k8s.io/utils/pointer" + aws "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsprovider/v1beta1" + azure "sigs.k8s.io/cluster-api-provider-azure/pkg/apis/azureprovider/v1beta1" + gcp "sigs.k8s.io/cluster-api-provider-gcp/pkg/apis/gcpprovider/v1beta1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/config" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" +) + +func TestMachineSetCreation(t *testing.T) { + g := NewWithT(t) + + // Override config getter + ctrl.GetConfig = func() (*rest.Config, error) { + return cfg, nil + } + defer func() { + ctrl.GetConfig = config.GetConfig + }() + + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machineset-creation-test", + }, + } + g.Expect(c.Create(ctx, namespace)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, namespace)).To(Succeed()) + }() + + infra := &osconfigv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + } + g.Expect(c.Create(ctx, infra)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, infra)).To(Succeed()) + }() + + testCases := []struct { + name string + platformType osconfigv1.PlatformType + clusterID string + expectedError string + providerSpecValue *runtime.RawExtension + }{ + { + name: "with AWS and no fields set", + platformType: osconfigv1.AWSPlatformType, + clusterID: "aws-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &aws.AWSMachineProviderConfig{}, + }, + expectedError: "providerSpec.ami: Required value: expected either providerSpec.ami.arn or providerSpec.ami.filters or providerSpec.ami.id to be populated", + }, + { + name: "with AWS and an AMI ID set", + platformType: osconfigv1.AWSPlatformType, + clusterID: "aws-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &aws.AWSMachineProviderConfig{ + AMI: aws.AWSResourceReference{ + ID: pointer.StringPtr("ami"), + }, + }, + }, + expectedError: "", + }, + { + name: "with Azure and no fields set", + platformType: osconfigv1.AzurePlatformType, + clusterID: "azure-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &azure.AzureMachineProviderSpec{}, + }, + expectedError: "[providerSpec.location: Required value: location should be set to one of the supported Azure regions, providerSpec.osDisk.diskSizeGB: Invalid value: 0: diskSizeGB must be greater than zero]", + }, + { + name: "with Azure and a location and disk size set", + platformType: osconfigv1.AzurePlatformType, + clusterID: "azure-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &azure.AzureMachineProviderSpec{ + Location: "location", + OSDisk: azure.OSDisk{ + DiskSizeGB: 128, + }, + }, + }, + expectedError: "", + }, + { + name: "with GCP and no fields set", + platformType: osconfigv1.GCPPlatformType, + clusterID: "gcp-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &gcp.GCPMachineProviderSpec{}, + }, + expectedError: "providerSpec.region: Required value: region is required", + }, + { + name: "with GCP and the region and zone set", + platformType: osconfigv1.GCPPlatformType, + clusterID: "gcp-cluster", + providerSpecValue: &runtime.RawExtension{ + Object: &gcp.GCPMachineProviderSpec{ + Region: "region", + Zone: "region-zone", + }, + }, + expectedError: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gs := NewWithT(t) + + mgr, err := manager.New(cfg, manager.Options{ + MetricsBindAddress: "0", + Port: testEnv.WebhookInstallOptions.LocalServingPort, + CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + }) + gs.Expect(err).ToNot(HaveOccurred()) + + done := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + }() + defer close(done) + + infra.Status = osconfigv1.InfrastructureStatus{ + InfrastructureName: tc.clusterID, + PlatformStatus: &osconfigv1.PlatformStatus{ + Type: tc.platformType, + GCP: &osconfigv1.GCPPlatformStatus{ + ProjectID: "gcp-project-id", + }, + }, + } + gs.Expect(c.Status().Update(ctx, infra)).To(Succeed()) + + machineSetDefaulter, err := NewMachineSetDefaulter() + gs.Expect(err).ToNot(HaveOccurred()) + machineSetValidator, err := NewMachineSetValidator() + gs.Expect(err).ToNot(HaveOccurred()) + mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) + + ms := &MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "machineset-creation-", + Namespace: namespace.Name, + }, + Spec: MachineSetSpec{ + Template: MachineTemplateSpec{ + Spec: MachineSpec{ + ProviderSpec: ProviderSpec{ + Value: tc.providerSpecValue, + }, + }, + }, + }, + } + err = c.Create(ctx, ms) + if err == nil { + defer func() { + gs.Expect(c.Delete(ctx, ms)).To(Succeed()) + }() + } + + if tc.expectedError != "" { + gs.Expect(err).ToNot(BeNil()) + gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError)) + } else { + gs.Expect(err).To(BeNil()) + } + }) + } +} + +func TestMachineSetUpdate(t *testing.T) { + awsClusterID := "aws-cluster" + defaultAWSProviderSpec := &aws.AWSMachineProviderConfig{ + AMI: aws.AWSResourceReference{ + ID: pointer.StringPtr("ami"), + }, + InstanceType: defaultAWSInstanceType, + IAMInstanceProfile: &aws.AWSResourceReference{ + ID: defaultAWSIAMInstanceProfile(awsClusterID), + }, + UserDataSecret: &corev1.LocalObjectReference{Name: defaultUserDataSecret}, + CredentialsSecret: &corev1.LocalObjectReference{Name: defaultAWSCredentialsSecret}, + SecurityGroups: []aws.AWSResourceReference{ + { + Filters: []aws.Filter{ + { + Name: "tag:Name", + Values: []string{defaultAWSSecurityGroup(awsClusterID)}, + }, + }, + }, + }, + Placement: aws.Placement{ + Region: "region", + AvailabilityZone: "zone", + }, + Subnet: aws.AWSResourceReference{ + Filters: []aws.Filter{ + { + Name: "tag:Name", + Values: []string{defaultAWSSubnet(awsClusterID, "zone")}, + }, + }, + }, + } + + azureClusterID := "azure-cluster" + defaultAzureProviderSpec := &azure.AzureMachineProviderSpec{ + Location: "location", + VMSize: defaultAzureVMSize, + Vnet: defaultAzureVnet(azureClusterID), + Subnet: defaultAzureSubnet(azureClusterID), + NetworkResourceGroup: defaultAzureNetworkResourceGroup(azureClusterID), + Image: azure.Image{ + ResourceID: defaultAzureImageResourceID(azureClusterID), + }, + ManagedIdentity: defaultAzureManagedIdentiy(azureClusterID), + ResourceGroup: defaultAzureResourceGroup(azureClusterID), + UserDataSecret: &corev1.SecretReference{ + Name: defaultUserDataSecret, + Namespace: defaultSecretNamespace, + }, + CredentialsSecret: &corev1.SecretReference{ + Name: defaultAzureCredentialsSecret, + Namespace: defaultSecretNamespace, + }, + OSDisk: azure.OSDisk{ + DiskSizeGB: 128, + OSType: defaultAzureOSDiskOSType, + ManagedDisk: azure.ManagedDisk{ + StorageAccountType: defaultAzureOSDiskStorageType, + }, + }, + } + + gcpClusterID := "gcp-cluster" + gcpProjectID := "gcp-project-id" + defaultGCPProviderSpec := &gcp.GCPMachineProviderSpec{ + Region: "region", + Zone: "region-zone", + MachineType: defaultGCPMachineType, + NetworkInterfaces: []*gcp.GCPNetworkInterface{ + { + Network: defaultGCPNetwork(gcpClusterID), + Subnetwork: defaultGCPSubnetwork(gcpClusterID), + }, + }, + Disks: []*gcp.GCPDisk{ + { + AutoDelete: true, + Boot: true, + SizeGb: defaultGCPDiskSizeGb, + Type: defaultGCPDiskType, + Image: defaultGCPDiskImage(gcpClusterID), + }, + }, + ServiceAccounts: defaultGCPServiceAccounts(gcpClusterID, gcpProjectID), + Tags: defaultGCPTags(gcpClusterID), + UserDataSecret: &corev1.LocalObjectReference{ + Name: defaultUserDataSecret, + }, + CredentialsSecret: &corev1.LocalObjectReference{ + Name: defaultGCPCredentialsSecret, + }, + } + + g := NewWithT(t) + + // Override config getter + ctrl.GetConfig = func() (*rest.Config, error) { + return cfg, nil + } + defer func() { + ctrl.GetConfig = config.GetConfig + }() + + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machineset-update-test", + }, + } + g.Expect(c.Create(ctx, namespace)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, namespace)).To(Succeed()) + }() + + infra := &osconfigv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + } + g.Expect(c.Create(ctx, infra)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, infra)).To(Succeed()) + }() + + testCases := []struct { + name string + platformType osconfigv1.PlatformType + clusterID string + expectedError string + baseProviderSpecValue *runtime.RawExtension + updatedProviderSpecValue func() *runtime.RawExtension + }{ + { + name: "with a valid AWS ProviderSpec", + platformType: osconfigv1.AWSPlatformType, + clusterID: awsClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + return &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + } + }, + expectedError: "", + }, + { + name: "with an AWS ProviderSpec, removing the instance type", + platformType: osconfigv1.AWSPlatformType, + clusterID: awsClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAWSProviderSpec.DeepCopy() + object.InstanceType = "" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.instanceType: Required value: expected providerSpec.instanceType to be populated", + }, + { + name: "with an AWS ProviderSpec, removing the instance profile", + platformType: osconfigv1.AWSPlatformType, + clusterID: awsClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAWSProviderSpec.DeepCopy() + object.IAMInstanceProfile = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.iamInstanceProfile: Required value: expected providerSpec.iamInstanceProfile to be populated", + }, + { + name: "with an AWS ProviderSpec, removing the user data secret", + platformType: osconfigv1.AWSPlatformType, + clusterID: awsClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAWSProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAWSProviderSpec.DeepCopy() + object.UserDataSecret = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.userDataSecret: Required value: expected providerSpec.userDataSecret to be populated", + }, + { + name: "with a valid Azure ProviderSpec", + platformType: osconfigv1.AzurePlatformType, + clusterID: azureClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + return &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + } + }, + expectedError: "", + }, + { + name: "with an Azure ProviderSpec, removing the vm size", + platformType: osconfigv1.AzurePlatformType, + clusterID: azureClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAzureProviderSpec.DeepCopy() + object.VMSize = "" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.vmSize: Required value: vmSize should be set to one of the supported Azure VM sizes", + }, + { + name: "with an Azure ProviderSpec, removing the subnet", + platformType: osconfigv1.AzurePlatformType, + clusterID: azureClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAzureProviderSpec.DeepCopy() + object.Subnet = "" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.subnet: Required value: must provide a subnet when a virtual network is specified", + }, + { + name: "with an Azure ProviderSpec, removing the credentials secret", + platformType: osconfigv1.AzurePlatformType, + clusterID: azureClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultAzureProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultAzureProviderSpec.DeepCopy() + object.CredentialsSecret = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.credentialsSecret: Required value: credentialsSecret must be provided", + }, + { + name: "with a valid GCP ProviderSpec", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + return &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + } + }, + expectedError: "", + }, + { + name: "with a GCP ProviderSpec, removing the region", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultGCPProviderSpec.DeepCopy() + object.Region = "" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.region: Required value: region is required", + }, + { + name: "with a GCP ProviderSpec, and an invalid region", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultGCPProviderSpec.DeepCopy() + object.Zone = "zone" + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.zone: Invalid value: \"zone\": zone not in configured region (region)", + }, + { + name: "with a GCP ProviderSpec, removing the disks", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultGCPProviderSpec.DeepCopy() + object.Disks = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.disks: Required value: at least 1 disk is required", + }, + { + name: "with a GCP ProviderSpec, removing the service accounts", + platformType: osconfigv1.GCPPlatformType, + clusterID: gcpClusterID, + baseProviderSpecValue: &runtime.RawExtension{ + Object: defaultGCPProviderSpec.DeepCopy(), + }, + updatedProviderSpecValue: func() *runtime.RawExtension { + object := defaultGCPProviderSpec.DeepCopy() + object.ServiceAccounts = nil + return &runtime.RawExtension{ + Object: object, + } + }, + expectedError: "providerSpec.serviceAccounts: Invalid value: \"0 service accounts supplied\": exactly 1 service account must be supplied", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gs := NewWithT(t) + + mgr, err := manager.New(cfg, manager.Options{ + MetricsBindAddress: "0", + Port: testEnv.WebhookInstallOptions.LocalServingPort, + CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + }) + gs.Expect(err).ToNot(HaveOccurred()) + + done := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + }() + defer close(done) + + infra.Status = osconfigv1.InfrastructureStatus{ + InfrastructureName: tc.clusterID, + PlatformStatus: &osconfigv1.PlatformStatus{ + Type: tc.platformType, + GCP: &osconfigv1.GCPPlatformStatus{ + ProjectID: gcpProjectID, + }, + }, + } + gs.Expect(c.Status().Update(ctx, infra)).To(Succeed()) + + machineSetDefaulter, err := NewMachineSetDefaulter() + gs.Expect(err).ToNot(HaveOccurred()) + machineSetValidator, err := NewMachineSetValidator() + gs.Expect(err).ToNot(HaveOccurred()) + mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) + + ms := &MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "machineset-update-", + Namespace: namespace.Name, + }, + Spec: MachineSetSpec{ + Template: MachineTemplateSpec{ + Spec: MachineSpec{ + ProviderSpec: ProviderSpec{ + Value: tc.baseProviderSpecValue, + }, + }, + }, + }, + } + err = c.Create(ctx, ms) + gs.Expect(err).ToNot(HaveOccurred()) + defer func() { + gs.Expect(c.Delete(ctx, ms)).To(Succeed()) + }() + + ms.Spec.Template.Spec.ProviderSpec.Value = tc.updatedProviderSpecValue() + err = c.Update(ctx, ms) + if tc.expectedError != "" { + gs.Expect(err).ToNot(BeNil()) + gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError)) + } else { + gs.Expect(err).To(BeNil()) + } + }) + } +} diff --git a/pkg/apis/machine/v1beta1/v1beta1_suite_test.go b/pkg/apis/machine/v1beta1/v1beta1_suite_test.go index fff26e5d4a..4b0b3c3dde 100644 --- a/pkg/apis/machine/v1beta1/v1beta1_suite_test.go +++ b/pkg/apis/machine/v1beta1/v1beta1_suite_test.go @@ -17,6 +17,7 @@ limitations under the License. package v1beta1 import ( + "context" "log" "os" "path/filepath" @@ -24,6 +25,7 @@ import ( "time" fuzz "github.com/google/gofuzz" + osconfigv1 "github.com/openshift/api/config/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtimeserializer "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/client-go/kubernetes/scheme" @@ -32,12 +34,22 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest" ) -var cfg *rest.Config -var c client.Client +var ( + cfg *rest.Config + c client.Client + ctx = context.Background() + testEnv *envtest.Environment +) func TestMain(m *testing.M) { - t := &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "install")}, + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{ + filepath.Join("..", "..", "..", "..", "install"), + filepath.Join("..", "..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1"), + }, + WebhookInstallOptions: envtest.WebhookInstallOptions{ + DirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "install")}, + }, } err := SchemeBuilder.AddToScheme(scheme.Scheme) @@ -45,7 +57,12 @@ func TestMain(m *testing.M) { log.Fatal(err) } - if cfg, err = t.Start(); err != nil { + err = osconfigv1.AddToScheme(scheme.Scheme) + if err != nil { + log.Fatal(err) + } + + if cfg, err = testEnv.Start(); err != nil { log.Fatal(err) } @@ -54,7 +71,7 @@ func TestMain(m *testing.M) { } code := m.Run() - t.Stop() + testEnv.Stop() os.Exit(code) } From 8f538d7db0e64c076666da15428f5035478a7db1 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Jun 2020 14:23:37 +0100 Subject: [PATCH 04/10] Switch handlers to wrap admissionHandler --- pkg/apis/machine/v1beta1/machine_webhook.go | 44 +++++++++---------- .../machine/v1beta1/machineset_webhook.go | 32 +++++--------- 2 files changed, 32 insertions(+), 44 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 3a30015e68..84f61b354f 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -121,22 +121,30 @@ func getInfra() (*osconfigv1.Infrastructure, error) { type machineAdmissionFn func(m *Machine, clusterID string) (bool, utilerrors.Aggregate) +type admissionHandler struct { + clusterID string + webhookOperations machineAdmissionFn + decoder *admission.Decoder +} + +// InjectDecoder injects the decoder. +func (a *admissionHandler) InjectDecoder(d *admission.Decoder) error { + a.decoder = d + return nil +} + // machineValidatorHandler validates Machine API resources. // implements type Handler interface. // https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler type machineValidatorHandler struct { - clusterID string - webhookOperations machineAdmissionFn - decoder *admission.Decoder + *admissionHandler } // machineDefaulterHandler defaults Machine API resources. // implements type Handler interface. // https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler type machineDefaulterHandler struct { - clusterID string - webhookOperations machineAdmissionFn - decoder *admission.Decoder + *admissionHandler } // NewValidator returns a new machineValidatorHandler. @@ -151,8 +159,10 @@ func NewMachineValidator() (*machineValidatorHandler, error) { func createMachineValidator(platform osconfigv1.PlatformType, clusterID string) *machineValidatorHandler { return &machineValidatorHandler{ - clusterID: clusterID, - webhookOperations: getMachineValidatorOperation(platform), + admissionHandler: &admissionHandler{ + clusterID: clusterID, + webhookOperations: getMachineValidatorOperation(platform), + }, } } @@ -184,8 +194,10 @@ func NewMachineDefaulter() (*machineDefaulterHandler, error) { func createMachineDefaulter(platformStatus *osconfigv1.PlatformStatus, clusterID string) *machineDefaulterHandler { return &machineDefaulterHandler{ - clusterID: clusterID, - webhookOperations: getMachineDefaulterOperation(platformStatus), + admissionHandler: &admissionHandler{ + clusterID: clusterID, + webhookOperations: getMachineDefaulterOperation(platformStatus), + }, } } @@ -209,18 +221,6 @@ func getMachineDefaulterOperation(platformStatus *osconfigv1.PlatformStatus) mac } } -// InjectDecoder injects the decoder. -func (v *machineValidatorHandler) InjectDecoder(d *admission.Decoder) error { - v.decoder = d - return nil -} - -// InjectDecoder injects the decoder. -func (v *machineDefaulterHandler) InjectDecoder(d *admission.Decoder) error { - v.decoder = d - return nil -} - // Handle handles HTTP requests for admission webhook servers. func (h *machineValidatorHandler) Handle(ctx context.Context, req admission.Request) admission.Response { m := &Machine{} diff --git a/pkg/apis/machine/v1beta1/machineset_webhook.go b/pkg/apis/machine/v1beta1/machineset_webhook.go index 29caea1ead..595f406028 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook.go @@ -15,18 +15,14 @@ import ( // implements type Handler interface. // https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler type machineSetValidatorHandler struct { - clusterID string - webhookOperations machineAdmissionFn - decoder *admission.Decoder + *admissionHandler } // machineSetDefaulterHandler defaults MachineSet API resources. // implements type Handler interface. // https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler type machineSetDefaulterHandler struct { - clusterID string - webhookOperations machineAdmissionFn - decoder *admission.Decoder + *admissionHandler } // NewMachineSetValidator returns a new machineSetValidatorHandler. @@ -41,8 +37,10 @@ func NewMachineSetValidator() (*machineSetValidatorHandler, error) { func createMachineSetValidator(platform osconfigv1.PlatformType, clusterID string) *machineSetValidatorHandler { return &machineSetValidatorHandler{ - clusterID: clusterID, - webhookOperations: getMachineValidatorOperation(platform), + admissionHandler: &admissionHandler{ + clusterID: clusterID, + webhookOperations: getMachineValidatorOperation(platform), + }, } } @@ -58,23 +56,13 @@ func NewMachineSetDefaulter() (*machineSetDefaulterHandler, error) { func createMachineSetDefaulter(platformStatus *osconfigv1.PlatformStatus, clusterID string) *machineSetDefaulterHandler { return &machineSetDefaulterHandler{ - clusterID: clusterID, - webhookOperations: getMachineDefaulterOperation(platformStatus), + admissionHandler: &admissionHandler{ + clusterID: clusterID, + webhookOperations: getMachineDefaulterOperation(platformStatus), + }, } } -// InjectDecoder injects the decoder. -func (v *machineSetValidatorHandler) InjectDecoder(d *admission.Decoder) error { - v.decoder = d - return nil -} - -// InjectDecoder injects the decoder. -func (v *machineSetDefaulterHandler) InjectDecoder(d *admission.Decoder) error { - v.decoder = d - return nil -} - // Handle handles HTTP requests for admission webhook servers. func (h *machineSetValidatorHandler) Handle(ctx context.Context, req admission.Request) admission.Response { ms := &MachineSet{} From 0b725042226815d8d65911c33a114c60eb8969e2 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Jun 2020 14:59:53 +0100 Subject: [PATCH 05/10] Ensure nil value is handled in machine webhooks --- pkg/apis/machine/v1beta1/machine_webhook.go | 4 ++++ .../machine/v1beta1/machine_webhook_test.go | 21 +++++++++++++++++++ .../v1beta1/machineset_webhook_test.go | 21 +++++++++++++++++++ 3 files changed, 46 insertions(+) diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 84f61b354f..8fa8595f9a 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -319,6 +319,10 @@ func defaultAWS(m *Machine, clusterID string) (bool, utilerrors.Aggregate) { } func unmarshalInto(m *Machine, providerSpec interface{}) error { + if m.Spec.ProviderSpec.Value == nil { + return field.Required(field.NewPath("providerSpec", "value"), "a value must be provided") + } + if err := yaml.Unmarshal(m.Spec.ProviderSpec.Value.Raw, &providerSpec); err != nil { return field.Invalid(field.NewPath("providerSpec", "value"), providerSpec, err.Error()) } diff --git a/pkg/apis/machine/v1beta1/machine_webhook_test.go b/pkg/apis/machine/v1beta1/machine_webhook_test.go index 0138f3b1cd..e8fb30f945 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machine_webhook_test.go @@ -61,6 +61,13 @@ func TestMachineCreation(t *testing.T) { expectedError string providerSpecValue *runtime.RawExtension }{ + { + name: "with AWS and a nil provider spec value", + platformType: osconfigv1.AWSPlatformType, + clusterID: "aws-cluster", + providerSpecValue: nil, + expectedError: "providerSpec.value: Required value: a value must be provided", + }, { name: "with AWS and no fields set", platformType: osconfigv1.AWSPlatformType, @@ -83,6 +90,13 @@ func TestMachineCreation(t *testing.T) { }, expectedError: "", }, + { + name: "with Azure and a nil provider spec value", + platformType: osconfigv1.AzurePlatformType, + clusterID: "azure-cluster", + providerSpecValue: nil, + expectedError: "providerSpec.value: Required value: a value must be provided", + }, { name: "with Azure and no fields set", platformType: osconfigv1.AzurePlatformType, @@ -106,6 +120,13 @@ func TestMachineCreation(t *testing.T) { }, expectedError: "", }, + { + name: "with GCP and a nil provider spec value", + platformType: osconfigv1.GCPPlatformType, + clusterID: "gcp-cluster", + providerSpecValue: nil, + expectedError: "providerSpec.value: Required value: a value must be provided", + }, { name: "with GCP and no fields set", platformType: osconfigv1.GCPPlatformType, diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go index f837965b43..1b9757a900 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook_test.go @@ -58,6 +58,13 @@ func TestMachineSetCreation(t *testing.T) { expectedError string providerSpecValue *runtime.RawExtension }{ + { + name: "with AWS and a nil provider spec value", + platformType: osconfigv1.AWSPlatformType, + clusterID: "aws-cluster", + providerSpecValue: nil, + expectedError: "providerSpec.value: Required value: a value must be provided", + }, { name: "with AWS and no fields set", platformType: osconfigv1.AWSPlatformType, @@ -80,6 +87,13 @@ func TestMachineSetCreation(t *testing.T) { }, expectedError: "", }, + { + name: "with Azure and a nil provider spec value", + platformType: osconfigv1.AWSPlatformType, + clusterID: "azure-cluster", + providerSpecValue: nil, + expectedError: "providerSpec.value: Required value: a value must be provided", + }, { name: "with Azure and no fields set", platformType: osconfigv1.AzurePlatformType, @@ -103,6 +117,13 @@ func TestMachineSetCreation(t *testing.T) { }, expectedError: "", }, + { + name: "with GCP and a nil provider spec value", + platformType: osconfigv1.AWSPlatformType, + clusterID: "gcp-cluster", + providerSpecValue: nil, + expectedError: "providerSpec.value: Required value: a value must be provided", + }, { name: "with GCP and no fields set", platformType: osconfigv1.GCPPlatformType, From 09af45139e7130991937215c51d0c4d9632d11dc Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Jun 2020 16:00:48 +0100 Subject: [PATCH 06/10] Don't create infra object, pass information directly --- .../machine/v1beta1/machine_webhook_test.go | 56 ++++--------------- .../v1beta1/machineset_webhook_test.go | 56 ++++--------------- 2 files changed, 24 insertions(+), 88 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook_test.go b/pkg/apis/machine/v1beta1/machine_webhook_test.go index e8fb30f945..5e20d944ba 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machine_webhook_test.go @@ -44,16 +44,6 @@ func TestMachineCreation(t *testing.T) { g.Expect(c.Delete(ctx, namespace)).To(Succeed()) }() - infra := &osconfigv1.Infrastructure{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - }, - } - g.Expect(c.Create(ctx, infra)).To(Succeed()) - defer func() { - g.Expect(c.Delete(ctx, infra)).To(Succeed()) - }() - testCases := []struct { name string platformType osconfigv1.PlatformType @@ -167,21 +157,15 @@ func TestMachineCreation(t *testing.T) { }() defer close(done) - infra.Status = osconfigv1.InfrastructureStatus{ - InfrastructureName: tc.clusterID, - PlatformStatus: &osconfigv1.PlatformStatus{ - Type: tc.platformType, - GCP: &osconfigv1.GCPPlatformStatus{ - ProjectID: "gcp-project-id", - }, + platformStatus := &osconfigv1.PlatformStatus{ + Type: tc.platformType, + GCP: &osconfigv1.GCPPlatformStatus{ + ProjectID: "gcp-project-id", }, } - gs.Expect(c.Status().Update(ctx, infra)).To(Succeed()) - machineDefaulter, err := NewMachineDefaulter() - gs.Expect(err).ToNot(HaveOccurred()) - machineValidator, err := NewMachineValidator() - gs.Expect(err).ToNot(HaveOccurred()) + machineDefaulter := createMachineDefaulter(platformStatus, tc.clusterID) + machineValidator := createMachineValidator(platformStatus.Type, tc.clusterID) mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineValidator}) @@ -329,16 +313,6 @@ func TestMachineUpdate(t *testing.T) { g.Expect(c.Delete(ctx, namespace)).To(Succeed()) }() - infra := &osconfigv1.Infrastructure{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - }, - } - g.Expect(c.Create(ctx, infra)).To(Succeed()) - defer func() { - g.Expect(c.Delete(ctx, infra)).To(Succeed()) - }() - testCases := []struct { name string platformType osconfigv1.PlatformType @@ -568,21 +542,15 @@ func TestMachineUpdate(t *testing.T) { }() defer close(done) - infra.Status = osconfigv1.InfrastructureStatus{ - InfrastructureName: tc.clusterID, - PlatformStatus: &osconfigv1.PlatformStatus{ - Type: tc.platformType, - GCP: &osconfigv1.GCPPlatformStatus{ - ProjectID: gcpProjectID, - }, + platformStatus := &osconfigv1.PlatformStatus{ + Type: tc.platformType, + GCP: &osconfigv1.GCPPlatformStatus{ + ProjectID: gcpProjectID, }, } - gs.Expect(c.Status().Update(ctx, infra)).To(Succeed()) - machineDefaulter, err := NewMachineDefaulter() - gs.Expect(err).ToNot(HaveOccurred()) - machineValidator, err := NewMachineValidator() - gs.Expect(err).ToNot(HaveOccurred()) + machineDefaulter := createMachineDefaulter(platformStatus, tc.clusterID) + machineValidator := createMachineValidator(platformStatus.Type, tc.clusterID) mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineValidator}) diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go index 1b9757a900..4106ccc895 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook_test.go @@ -41,16 +41,6 @@ func TestMachineSetCreation(t *testing.T) { g.Expect(c.Delete(ctx, namespace)).To(Succeed()) }() - infra := &osconfigv1.Infrastructure{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - }, - } - g.Expect(c.Create(ctx, infra)).To(Succeed()) - defer func() { - g.Expect(c.Delete(ctx, infra)).To(Succeed()) - }() - testCases := []struct { name string platformType osconfigv1.PlatformType @@ -164,21 +154,15 @@ func TestMachineSetCreation(t *testing.T) { }() defer close(done) - infra.Status = osconfigv1.InfrastructureStatus{ - InfrastructureName: tc.clusterID, - PlatformStatus: &osconfigv1.PlatformStatus{ - Type: tc.platformType, - GCP: &osconfigv1.GCPPlatformStatus{ - ProjectID: "gcp-project-id", - }, + platformStatus := &osconfigv1.PlatformStatus{ + Type: tc.platformType, + GCP: &osconfigv1.GCPPlatformStatus{ + ProjectID: "gcp-project-id", }, } - gs.Expect(c.Status().Update(ctx, infra)).To(Succeed()) - machineSetDefaulter, err := NewMachineSetDefaulter() - gs.Expect(err).ToNot(HaveOccurred()) - machineSetValidator, err := NewMachineSetValidator() - gs.Expect(err).ToNot(HaveOccurred()) + machineSetDefaulter := createMachineSetDefaulter(platformStatus, tc.clusterID) + machineSetValidator := createMachineSetValidator(platformStatus.Type, tc.clusterID) mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) @@ -330,16 +314,6 @@ func TestMachineSetUpdate(t *testing.T) { g.Expect(c.Delete(ctx, namespace)).To(Succeed()) }() - infra := &osconfigv1.Infrastructure{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - }, - } - g.Expect(c.Create(ctx, infra)).To(Succeed()) - defer func() { - g.Expect(c.Delete(ctx, infra)).To(Succeed()) - }() - testCases := []struct { name string platformType osconfigv1.PlatformType @@ -569,21 +543,15 @@ func TestMachineSetUpdate(t *testing.T) { }() defer close(done) - infra.Status = osconfigv1.InfrastructureStatus{ - InfrastructureName: tc.clusterID, - PlatformStatus: &osconfigv1.PlatformStatus{ - Type: tc.platformType, - GCP: &osconfigv1.GCPPlatformStatus{ - ProjectID: gcpProjectID, - }, + platformStatus := &osconfigv1.PlatformStatus{ + Type: tc.platformType, + GCP: &osconfigv1.GCPPlatformStatus{ + ProjectID: gcpProjectID, }, } - gs.Expect(c.Status().Update(ctx, infra)).To(Succeed()) - machineSetDefaulter, err := NewMachineSetDefaulter() - gs.Expect(err).ToNot(HaveOccurred()) - machineSetValidator, err := NewMachineSetValidator() - gs.Expect(err).ToNot(HaveOccurred()) + machineSetDefaulter := createMachineSetDefaulter(platformStatus, tc.clusterID) + machineSetValidator := createMachineSetValidator(platformStatus.Type, tc.clusterID) mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) From b6f8df18d7823921bde4ed8197b96804c2659600 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Jun 2020 16:31:48 +0100 Subject: [PATCH 07/10] Wait for manager to stop before ending test --- pkg/apis/machine/v1beta1/machine_webhook_test.go | 14 ++++++++++++-- .../machine/v1beta1/machineset_webhook_test.go | 14 ++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook_test.go b/pkg/apis/machine/v1beta1/machine_webhook_test.go index 5e20d944ba..7de92dfe1e 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machine_webhook_test.go @@ -152,10 +152,15 @@ func TestMachineCreation(t *testing.T) { gs.Expect(err).ToNot(HaveOccurred()) done := make(chan struct{}) + stopped := make(chan struct{}) go func() { gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped }() - defer close(done) platformStatus := &osconfigv1.PlatformStatus{ Type: tc.platformType, @@ -537,10 +542,15 @@ func TestMachineUpdate(t *testing.T) { gs.Expect(err).ToNot(HaveOccurred()) done := make(chan struct{}) + stopped := make(chan struct{}) go func() { gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped }() - defer close(done) platformStatus := &osconfigv1.PlatformStatus{ Type: tc.platformType, diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go index 4106ccc895..a36c53268f 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook_test.go @@ -149,10 +149,15 @@ func TestMachineSetCreation(t *testing.T) { gs.Expect(err).ToNot(HaveOccurred()) done := make(chan struct{}) + stopped := make(chan struct{}) go func() { gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped }() - defer close(done) platformStatus := &osconfigv1.PlatformStatus{ Type: tc.platformType, @@ -538,10 +543,15 @@ func TestMachineSetUpdate(t *testing.T) { gs.Expect(err).ToNot(HaveOccurred()) done := make(chan struct{}) + stopped := make(chan struct{}) go func() { gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped }() - defer close(done) platformStatus := &osconfigv1.PlatformStatus{ Type: tc.platformType, From df071c27f0aa5044649a2589f9f56c13976486c2 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Thu, 18 Jun 2020 17:18:34 +0100 Subject: [PATCH 08/10] Start manager after registering hooks and wait for webhook server before running tests --- .../machine/v1beta1/machine_webhook_test.go | 61 ++++++++++++------- .../v1beta1/machineset_webhook_test.go | 61 ++++++++++++------- .../machine/v1beta1/v1beta1_suite_test.go | 17 ++++-- 3 files changed, 91 insertions(+), 48 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook_test.go b/pkg/apis/machine/v1beta1/machine_webhook_test.go index 7de92dfe1e..1c0c43b37a 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machine_webhook_test.go @@ -2,6 +2,7 @@ package v1beta1 import ( "encoding/json" + "fmt" "testing" . "github.com/onsi/gomega" @@ -151,17 +152,6 @@ func TestMachineCreation(t *testing.T) { }) gs.Expect(err).ToNot(HaveOccurred()) - done := make(chan struct{}) - stopped := make(chan struct{}) - go func() { - gs.Expect(mgr.Start(done)).To(Succeed()) - close(stopped) - }() - defer func() { - close(done) - <-stopped - }() - platformStatus := &osconfigv1.PlatformStatus{ Type: tc.platformType, GCP: &osconfigv1.GCPPlatformStatus{ @@ -174,6 +164,25 @@ func TestMachineCreation(t *testing.T) { mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineValidator}) + done := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped + }() + + gs.Eventually(func() (bool, error) { + resp, err := insecureHTTPClient.Get(fmt.Sprintf("https://127.0.0.1:%d", testEnv.WebhookInstallOptions.LocalServingPort)) + if err != nil { + return false, err + } + return resp.StatusCode == 404, nil + }).Should(BeTrue()) + m := &Machine{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "machine-creation-", @@ -541,17 +550,6 @@ func TestMachineUpdate(t *testing.T) { }) gs.Expect(err).ToNot(HaveOccurred()) - done := make(chan struct{}) - stopped := make(chan struct{}) - go func() { - gs.Expect(mgr.Start(done)).To(Succeed()) - close(stopped) - }() - defer func() { - close(done) - <-stopped - }() - platformStatus := &osconfigv1.PlatformStatus{ Type: tc.platformType, GCP: &osconfigv1.GCPPlatformStatus{ @@ -564,6 +562,25 @@ func TestMachineUpdate(t *testing.T) { mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineValidator}) + done := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped + }() + + gs.Eventually(func() (bool, error) { + resp, err := insecureHTTPClient.Get(fmt.Sprintf("https://127.0.0.1:%d", testEnv.WebhookInstallOptions.LocalServingPort)) + if err != nil { + return false, err + } + return resp.StatusCode == 404, nil + }).Should(BeTrue()) + m := &Machine{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "machine-creation-", diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go index a36c53268f..3128aad2a1 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook_test.go @@ -1,6 +1,7 @@ package v1beta1 import ( + "fmt" "testing" . "github.com/onsi/gomega" @@ -148,17 +149,6 @@ func TestMachineSetCreation(t *testing.T) { }) gs.Expect(err).ToNot(HaveOccurred()) - done := make(chan struct{}) - stopped := make(chan struct{}) - go func() { - gs.Expect(mgr.Start(done)).To(Succeed()) - close(stopped) - }() - defer func() { - close(done) - <-stopped - }() - platformStatus := &osconfigv1.PlatformStatus{ Type: tc.platformType, GCP: &osconfigv1.GCPPlatformStatus{ @@ -171,6 +161,25 @@ func TestMachineSetCreation(t *testing.T) { mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) + done := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped + }() + + gs.Eventually(func() (bool, error) { + resp, err := insecureHTTPClient.Get(fmt.Sprintf("https://127.0.0.1:%d", testEnv.WebhookInstallOptions.LocalServingPort)) + if err != nil { + return false, err + } + return resp.StatusCode == 404, nil + }).Should(BeTrue()) + ms := &MachineSet{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "machineset-creation-", @@ -542,17 +551,6 @@ func TestMachineSetUpdate(t *testing.T) { }) gs.Expect(err).ToNot(HaveOccurred()) - done := make(chan struct{}) - stopped := make(chan struct{}) - go func() { - gs.Expect(mgr.Start(done)).To(Succeed()) - close(stopped) - }() - defer func() { - close(done) - <-stopped - }() - platformStatus := &osconfigv1.PlatformStatus{ Type: tc.platformType, GCP: &osconfigv1.GCPPlatformStatus{ @@ -565,6 +563,25 @@ func TestMachineSetUpdate(t *testing.T) { mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) + done := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped + }() + + gs.Eventually(func() (bool, error) { + resp, err := insecureHTTPClient.Get(fmt.Sprintf("https://127.0.0.1:%d", testEnv.WebhookInstallOptions.LocalServingPort)) + if err != nil { + return false, err + } + return resp.StatusCode == 404, nil + }).Should(BeTrue()) + ms := &MachineSet{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "machineset-update-", diff --git a/pkg/apis/machine/v1beta1/v1beta1_suite_test.go b/pkg/apis/machine/v1beta1/v1beta1_suite_test.go index 4b0b3c3dde..e8a8d3ad10 100644 --- a/pkg/apis/machine/v1beta1/v1beta1_suite_test.go +++ b/pkg/apis/machine/v1beta1/v1beta1_suite_test.go @@ -18,7 +18,9 @@ package v1beta1 import ( "context" + "crypto/tls" "log" + "net/http" "os" "path/filepath" "testing" @@ -35,10 +37,17 @@ import ( ) var ( - cfg *rest.Config - c client.Client - ctx = context.Background() - testEnv *envtest.Environment + cfg *rest.Config + c client.Client + ctx = context.Background() + testEnv *envtest.Environment + insecureHTTPClient = http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + }, + }, + } ) func TestMain(m *testing.M) { From f5a6e429b0f11afac8a57252b45923c8d0c1e26a Mon Sep 17 00:00:00 2001 From: Michael Gugino Date: Wed, 17 Jun 2020 23:51:31 -0400 Subject: [PATCH 09/10] Add MachineSet ControlPlane webhooks Add webhooks for Delete and Update operations on MachineSets that are identified to be Control Plane members. These webhooks prevent deleting these MachineSets or updating them to become non-CP MachineSets (in order to further prevent them from being deleted or misconfigured) --- cmd/machineset/main.go | 4 + ...00_30_machine-api-operator_08_webhook.yaml | 40 +++ .../machine/v1beta1/machineset_cp_webhook.go | 84 +++++ .../machine/v1beta1/machineset_types_test.go | 27 ++ .../v1beta1/machineset_webhook_test.go | 321 ++++++++++++++++++ 5 files changed, 476 insertions(+) create mode 100644 pkg/apis/machine/v1beta1/machineset_cp_webhook.go diff --git a/cmd/machineset/main.go b/cmd/machineset/main.go index 80c23fe6a6..103a048ff4 100644 --- a/cmd/machineset/main.go +++ b/cmd/machineset/main.go @@ -100,6 +100,8 @@ func main() { log.Fatal(err) } + machineSetCPValidator := v1beta1.NewMachineSetCPValidator() + if *webhookEnabled { mgr.GetWebhookServer().Port = *webhookPort mgr.GetWebhookServer().CertDir = *webhookCertdir @@ -107,6 +109,8 @@ func main() { mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machine", &webhook.Admission{Handler: machineValidator}) mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-delete", &webhook.Admission{Handler: machineSetCPValidator}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-update", &webhook.Admission{Handler: machineSetCPValidator}) } log.Printf("Registering Components.") diff --git a/install/0000_30_machine-api-operator_08_webhook.yaml b/install/0000_30_machine-api-operator_08_webhook.yaml index 5e13a3280b..e8caf1b0ad 100644 --- a/install/0000_30_machine-api-operator_08_webhook.yaml +++ b/install/0000_30_machine-api-operator_08_webhook.yaml @@ -100,3 +100,43 @@ webhooks: resources: - machinesets sideEffects: None + - clientConfig: + service: + name: machine-api-operator-webhook + namespace: openshift-machine-api + path: /validate-machine-openshift-io-v1beta1-machineset-cp-delete + # failurePolicy is Fail so we ensure control plane machinesets are never + # deleted. + failurePolicy: Fail + matchPolicy: Equivalent + name: delete.cp.validation.machineset.machine.openshift.io + rules: + - apiGroups: + - machine.openshift.io + apiVersions: + - v1beta1 + operations: + - DELETE + resources: + - machinesets + sideEffects: None + - clientConfig: + service: + name: machine-api-operator-webhook + namespace: openshift-machine-api + path: /validate-machine-openshift-io-v1beta1-machineset-cp-update + # failurePolicy is Fail so we ensure control plane machinesets are never + # made non-CP machinesets. + failurePolicy: Fail + matchPolicy: Equivalent + name: update.cp.validation.machineset.machine.openshift.io + rules: + - apiGroups: + - machine.openshift.io + apiVersions: + - v1beta1 + operations: + - UPDATE + resources: + - machinesets + sideEffects: None diff --git a/pkg/apis/machine/v1beta1/machineset_cp_webhook.go b/pkg/apis/machine/v1beta1/machineset_cp_webhook.go new file mode 100644 index 0000000000..6990bc108e --- /dev/null +++ b/pkg/apis/machine/v1beta1/machineset_cp_webhook.go @@ -0,0 +1,84 @@ +package v1beta1 + +import ( + "context" + "fmt" + "net/http" + + admissionv1beta1 "k8s.io/api/admission/v1beta1" + "k8s.io/klog" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// MachineSetCPHandler validates ControlPlane MachineSet API resources. +// implements type Handler interface. +// https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/webhook/admission#Handler +type MachineSetCPHandler struct { + decoder *admission.Decoder +} + +// NewMachineSetCPValidator returns a new MachineSetCPHandler. +func NewMachineSetCPValidator() (*MachineSetCPHandler, error) { + return createMachineSetCPValidator(), nil +} + +func createMachineSetCPValidator() *MachineSetCPHandler { + return &MachineSetCPHandler{} +} + +// InjectDecoder injects the decoder. +func (v *MachineSetCPHandler) InjectDecoder(d *admission.Decoder) error { + v.decoder = d + return nil +} + +// Handle handles HTTP requests for admission webhook servers. +func (v *MachineSetCPHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + oldMS := &MachineSet{} + + // Delete requests, the req.Object is empty. + if err := v.decoder.DecodeRaw(req.OldObject, oldMS); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + + klog.V(3).Infof("Validate webhook called for CP MachineSets: %s", oldMS.GetName()) + + newMS := &MachineSet{} + if req.Operation != admissionv1beta1.Delete { + if err := v.decoder.Decode(req, newMS); err != nil { + return admission.Errored(http.StatusBadRequest, err) + } + } + + // Succeed if deleting non-CP MachineSet or Updating a non-CP MachineSet + // and the user is not attempting to change it to a CP MachineSet. + if !isCPMS(oldMS) && (req.Operation == admissionv1beta1.Delete || !isCPMS(newMS)) { + return admission.Allowed("MachineSet is Not Control Plane.") + } + + // If the user is updating a CP MachineSet, as long as machine role is + // unchanged, we're ok. + if req.Operation != admissionv1beta1.Delete && isCPMS(newMS) && isCPMS(oldMS) { + return admission.Allowed("Control Plane MachineSet is Valid.") + } + + // User is peforming an unallowed operation + + // TODO(michaelgugino): Ensure we account for MachineDeployment ownership + // of a CP machineset in the future if we use them. + return admission.Denied(fmt.Sprintf("Requested %v of Control Plane MachineSet Not Allowed.", req.Operation)) + +} + +func isCPMS(ms *MachineSet) bool { + if ms.Spec.Template.ObjectMeta.Labels == nil { + return false + } + val, ok := ms.Spec.Template.ObjectMeta.Labels["machine.openshift.io/cluster-api-machine-role"] + if ok { + if val == "master" { + return true + } + } + return false +} diff --git a/pkg/apis/machine/v1beta1/machineset_types_test.go b/pkg/apis/machine/v1beta1/machineset_types_test.go index 53d6f3eca5..78b686cfe2 100644 --- a/pkg/apis/machine/v1beta1/machineset_types_test.go +++ b/pkg/apis/machine/v1beta1/machineset_types_test.go @@ -32,12 +32,39 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" + + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" ) func TestStorageMachineSet(t *testing.T) { key := types.NamespacedName{Name: "foo", Namespace: "default"} created := &MachineSet{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}} + gs := NewWithT(t) + + mgr, err := manager.New(cfg, manager.Options{ + MetricsBindAddress: "0", + Port: testEnv.WebhookInstallOptions.LocalServingPort, + CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + }) + + gs.Expect(err).ToNot(HaveOccurred()) + + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-delete", &webhook.Admission{Handler: createMachineSetMockHandler(true)}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-update", &webhook.Admission{Handler: createMachineSetMockHandler(true)}) + + done := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped + }() + // Test Create fetched := &MachineSet{} if err := c.Create(context.TODO(), created); err != nil { diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go index 3128aad2a1..28d7016b1a 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook_test.go @@ -1,6 +1,7 @@ package v1beta1 import ( + "context" "fmt" "testing" @@ -19,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) func TestMachineSetCreation(t *testing.T) { @@ -160,6 +162,8 @@ func TestMachineSetCreation(t *testing.T) { machineSetValidator := createMachineSetValidator(platformStatus.Type, tc.clusterID) mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-delete", &webhook.Admission{Handler: createMachineSetMockHandler(true)}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-update", &webhook.Admission{Handler: createMachineSetMockHandler(true)}) done := make(chan struct{}) stopped := make(chan struct{}) @@ -562,6 +566,8 @@ func TestMachineSetUpdate(t *testing.T) { machineSetValidator := createMachineSetValidator(platformStatus.Type, tc.clusterID) mgr.GetWebhookServer().Register("/mutate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetDefaulter}) mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset", &webhook.Admission{Handler: machineSetValidator}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-delete", &webhook.Admission{Handler: createMachineSetMockHandler(true)}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-update", &webhook.Admission{Handler: createMachineSetMockHandler(true)}) done := make(chan struct{}) stopped := make(chan struct{}) @@ -614,3 +620,318 @@ func TestMachineSetUpdate(t *testing.T) { }) } } + +func TestCPMachineSetDelete(t *testing.T) { + g := NewWithT(t) + + // Override config getter + ctrl.GetConfig = func() (*rest.Config, error) { + return cfg, nil + } + defer func() { + ctrl.GetConfig = config.GetConfig + }() + + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machineset-cp-delete-test", + }, + } + g.Expect(c.Create(ctx, namespace)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, namespace)).To(Succeed()) + }() + + testCases := []struct { + name string + expectedError string + objectMeta ObjectMeta + }{ + { + name: "is not CP MachineSet", + expectedError: "", + objectMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "worker", + }, + }, + }, + { + name: "is not CP MachineSet, no labels", + expectedError: "", + objectMeta: ObjectMeta{}, + }, + { + name: "is CP MachineSet", + expectedError: "Requested DELETE of Control Plane MachineSet Not Allowed.", + objectMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "master", + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gs := NewWithT(t) + + mgr, err := manager.New(cfg, manager.Options{ + MetricsBindAddress: "0", + Port: testEnv.WebhookInstallOptions.LocalServingPort, + CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + }) + gs.Expect(err).ToNot(HaveOccurred()) + + machineSetCPDeletionValidator, err := NewMachineSetCPValidator() + gs.Expect(err).ToNot(HaveOccurred()) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-delete", &webhook.Admission{Handler: machineSetCPDeletionValidator}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-update", &webhook.Admission{Handler: createMachineSetMockHandler(true)}) + + done := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped + }() + + ms := &MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "machineset-cp-deletion-", + Namespace: namespace.Name, + }, + Spec: MachineSetSpec{ + Template: MachineTemplateSpec{ + Spec: MachineSpec{}, + }, + }, + } + + ms.Spec.Template.ObjectMeta = tc.objectMeta + gs.Expect(c.Create(ctx, ms)).To(Succeed()) + + err = c.Delete(ctx, ms) + + if tc.expectedError != "" { + defer func() { + ms.Spec.Template.ObjectMeta.Labels["machine.openshift.io/cluster-api-machine-role"] = "worker" + gs.Expect(c.Update(ctx, ms)).To(Succeed()) + gs.Expect(c.Delete(ctx, ms)).To(Succeed()) + }() + gs.Expect(err).ToNot(BeNil()) + gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError)) + } else { + gs.Expect(err).To(BeNil()) + } + }) + } +} + +func TestCPMachineSetUpdate(t *testing.T) { + g := NewWithT(t) + + // Override config getter + ctrl.GetConfig = func() (*rest.Config, error) { + return cfg, nil + } + defer func() { + ctrl.GetConfig = config.GetConfig + }() + + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machineset-cp-update-test", + }, + } + g.Expect(c.Create(ctx, namespace)).To(Succeed()) + defer func() { + g.Expect(c.Delete(ctx, namespace)).To(Succeed()) + }() + + testCases := []struct { + name string + expectedError string + originalMeta ObjectMeta + updateMeta ObjectMeta + }{ + { + name: "is not CP MachineSet, not becoming CP", + expectedError: "", + originalMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "worker", + }, + }, + updateMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "worker2", + }, + }, + }, + { + name: "is not CP MachineSet, labels removed", + expectedError: "", + originalMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "worker", + }, + }, + updateMeta: ObjectMeta{ + Labels: nil, + }, + }, + { + name: "no Lables (non-CP) to non-CP labels", + expectedError: "", + originalMeta: ObjectMeta{ + Labels: nil, + }, + updateMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "worker", + }, + }, + }, + { + name: "CP MachineSet, add another label", + expectedError: "", + originalMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "master", + }, + }, + updateMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "master", + "secondlabel": "second label value", + }, + }, + }, + { + name: "CP MachineSet, try to change role", + expectedError: "Requested UPDATE of Control Plane MachineSet Not Allowed.", + originalMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "master", + }, + }, + updateMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "worker", + }, + }, + }, + { + name: "CP MachineSet, try to remove labels", + expectedError: "Requested UPDATE of Control Plane MachineSet Not Allowed.", + originalMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "master", + }, + }, + updateMeta: ObjectMeta{ + Labels: nil, + }, + }, + { + name: "Non CP become CP", + expectedError: "Requested UPDATE of Control Plane MachineSet Not Allowed.", + originalMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "worker", + }, + }, + updateMeta: ObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-machine-role": "master", + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gs := NewWithT(t) + + mgr, err := manager.New(cfg, manager.Options{ + MetricsBindAddress: "0", + Port: testEnv.WebhookInstallOptions.LocalServingPort, + CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + }) + gs.Expect(err).ToNot(HaveOccurred()) + + machineSetCPUpdateValidator, err := NewMachineSetCPValidator() + gs.Expect(err).ToNot(HaveOccurred()) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-delete", &webhook.Admission{Handler: createMachineSetMockHandler(true)}) + mgr.GetWebhookServer().Register("/validate-machine-openshift-io-v1beta1-machineset-cp-update", &webhook.Admission{Handler: machineSetCPUpdateValidator}) + + done := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + gs.Expect(mgr.Start(done)).To(Succeed()) + close(stopped) + }() + defer func() { + close(done) + <-stopped + }() + + ms := &MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "machineset-cp-update-", + Namespace: namespace.Name, + }, + Spec: MachineSetSpec{ + Template: MachineTemplateSpec{ + Spec: MachineSpec{}, + }, + }, + } + + ms.Spec.Template.ObjectMeta = tc.originalMeta + err = c.Create(ctx, ms) + gs.Expect(err).ToNot(HaveOccurred()) + defer func() { + gs.Expect(c.Delete(ctx, ms)).To(Succeed()) + }() + + ms.Spec.Template.ObjectMeta = tc.updateMeta + + err = c.Update(ctx, ms) + + if tc.expectedError != "" { + gs.Expect(err).ToNot(BeNil()) + gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError)) + } else { + gs.Expect(err).To(BeNil()) + } + }) + } +} + +type MachineSetMockHandler struct { + decoder *admission.Decoder + shouldAdmit bool +} + +func createMachineSetMockHandler(shouldAdmit bool) *MachineSetMockHandler { + return &MachineSetMockHandler{shouldAdmit: shouldAdmit} +} + +// InjectDecoder injects the decoder. +func (h *MachineSetMockHandler) InjectDecoder(d *admission.Decoder) error { + h.decoder = d + return nil +} + +// Handle handles HTTP requests for admission webhook servers. +func (h *MachineSetMockHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + if h.shouldAdmit { + return admission.Allowed("OK") + } + return admission.Denied("Not OK") +} From 09288444b29e2765215817fe153f0d042fa32703 Mon Sep 17 00:00:00 2001 From: Michael Gugino Date: Sun, 21 Jun 2020 11:23:39 -0400 Subject: [PATCH 10/10] graceperiod testing --- go.mod | 2 ++ go.sum | 2 ++ pkg/apis/machine/v1beta1/machineset_types_test.go | 1 + pkg/apis/machine/v1beta1/machineset_webhook_test.go | 4 ++++ pkg/apis/machine/v1beta1/v1beta1_suite_test.go | 1 + 5 files changed, 10 insertions(+) diff --git a/go.mod b/go.mod index 453c8ac0f7..762bf13567 100644 --- a/go.mod +++ b/go.mod @@ -39,3 +39,5 @@ replace sigs.k8s.io/cluster-api-provider-aws => github.com/openshift/cluster-api replace sigs.k8s.io/cluster-api-provider-azure => github.com/openshift/cluster-api-provider-azure v0.1.0-alpha.3.0.20200529030741-17d4edc5142f replace sigs.k8s.io/cluster-api-provider-gcp => github.com/openshift/cluster-api-provider-gcp v0.0.1-0.20200528175251-4f2fdeb49fe1 + +replace sigs.k8s.io/controller-runtime => github.com/alvaroaleman/controller-runtime v0.1.5-0.20200619152754-4a802fb9b747 diff --git a/go.sum b/go.sum index 974a3f1de7..ebbc07efcd 100644 --- a/go.sum +++ b/go.sum @@ -36,6 +36,8 @@ github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuy github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= +github.com/alvaroaleman/controller-runtime v0.1.5-0.20200619152754-4a802fb9b747 h1:l5sFFGjYd9YsVYN8u1JRsa4pSURnxBEpM80B1W2y46E= +github.com/alvaroaleman/controller-runtime v0.1.5-0.20200619152754-4a802fb9b747/go.mod h1:qN/IYzFHXI7mP9qhUiGRN9uDH3fdAAqBTCqP1YkMEtQ= github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8= github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8= github.com/asaskevich/govalidator v0.0.0-20180720115003-f9ffefc3facf/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY= diff --git a/pkg/apis/machine/v1beta1/machineset_types_test.go b/pkg/apis/machine/v1beta1/machineset_types_test.go index 78b686cfe2..e5495625f0 100644 --- a/pkg/apis/machine/v1beta1/machineset_types_test.go +++ b/pkg/apis/machine/v1beta1/machineset_types_test.go @@ -47,6 +47,7 @@ func TestStorageMachineSet(t *testing.T) { MetricsBindAddress: "0", Port: testEnv.WebhookInstallOptions.LocalServingPort, CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + GracefulShutdownTimeout: &managerGracePeriodDuration, }) gs.Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go index 28d7016b1a..0d7c1755b7 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook_test.go @@ -148,6 +148,7 @@ func TestMachineSetCreation(t *testing.T) { MetricsBindAddress: "0", Port: testEnv.WebhookInstallOptions.LocalServingPort, CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + GracefulShutdownTimeout: &managerGracePeriodDuration, }) gs.Expect(err).ToNot(HaveOccurred()) @@ -552,6 +553,7 @@ func TestMachineSetUpdate(t *testing.T) { MetricsBindAddress: "0", Port: testEnv.WebhookInstallOptions.LocalServingPort, CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + GracefulShutdownTimeout: &managerGracePeriodDuration, }) gs.Expect(err).ToNot(HaveOccurred()) @@ -680,6 +682,7 @@ func TestCPMachineSetDelete(t *testing.T) { MetricsBindAddress: "0", Port: testEnv.WebhookInstallOptions.LocalServingPort, CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + GracefulShutdownTimeout: &managerGracePeriodDuration, }) gs.Expect(err).ToNot(HaveOccurred()) @@ -861,6 +864,7 @@ func TestCPMachineSetUpdate(t *testing.T) { MetricsBindAddress: "0", Port: testEnv.WebhookInstallOptions.LocalServingPort, CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir, + GracefulShutdownTimeout: &managerGracePeriodDuration, }) gs.Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/apis/machine/v1beta1/v1beta1_suite_test.go b/pkg/apis/machine/v1beta1/v1beta1_suite_test.go index e8a8d3ad10..889634dbba 100644 --- a/pkg/apis/machine/v1beta1/v1beta1_suite_test.go +++ b/pkg/apis/machine/v1beta1/v1beta1_suite_test.go @@ -48,6 +48,7 @@ var ( }, }, } + managerGracePeriodDuration = time.Duration(1 * time.Second) ) func TestMain(m *testing.M) {