From b6ffeeda877d8bbc847e7610405f801ee4adf670 Mon Sep 17 00:00:00 2001 From: Alexander Demichev Date: Wed, 22 Jul 2020 14:15:56 +0200 Subject: [PATCH 1/6] Pass context to machine scope --- pkg/cloud/gcp/actuators/machine/actuator.go | 4 ++++ pkg/cloud/gcp/actuators/machine/machine_scope.go | 13 +++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/pkg/cloud/gcp/actuators/machine/actuator.go b/pkg/cloud/gcp/actuators/machine/actuator.go index 117b4a56d..fcb564f92 100644 --- a/pkg/cloud/gcp/actuators/machine/actuator.go +++ b/pkg/cloud/gcp/actuators/machine/actuator.go @@ -56,6 +56,7 @@ func (a *Actuator) handleMachineError(machine *machinev1.Machine, err error, eve func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error { klog.Infof("%s: Creating machine", machine.Name) scope, err := newMachineScope(machineScopeParams{ + Context: ctx, coreClient: a.coreClient, machine: machine, }) @@ -74,6 +75,7 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error func (a *Actuator) Exists(ctx context.Context, machine *machinev1.Machine) (bool, error) { klog.Infof("%s: Checking if machine exists", machine.Name) scope, err := newMachineScope(machineScopeParams{ + Context: ctx, coreClient: a.coreClient, machine: machine, }) @@ -91,6 +93,7 @@ func (a *Actuator) Exists(ctx context.Context, machine *machinev1.Machine) (bool func (a *Actuator) Update(ctx context.Context, machine *machinev1.Machine) error { klog.Infof("%s: Updating machine", machine.Name) scope, err := newMachineScope(machineScopeParams{ + Context: ctx, coreClient: a.coreClient, machine: machine, }) @@ -110,6 +113,7 @@ func (a *Actuator) Update(ctx context.Context, machine *machinev1.Machine) error func (a *Actuator) Delete(ctx context.Context, machine *machinev1.Machine) error { klog.Infof("%s: Deleting machine", machine.Name) scope, err := newMachineScope(machineScopeParams{ + Context: ctx, coreClient: a.coreClient, machine: machine, }) diff --git a/pkg/cloud/gcp/actuators/machine/machine_scope.go b/pkg/cloud/gcp/actuators/machine/machine_scope.go index 61011bc08..364e830bb 100644 --- a/pkg/cloud/gcp/actuators/machine/machine_scope.go +++ b/pkg/cloud/gcp/actuators/machine/machine_scope.go @@ -18,12 +18,16 @@ import ( // machineScopeParams defines the input parameters used to create a new MachineScope. type machineScopeParams struct { + context.Context + coreClient controllerclient.Client machine *machinev1.Machine } // machineScope defines a scope defined around a machine and its cluster. type machineScope struct { + context.Context + coreClient controllerclient.Client projectID string providerID string @@ -45,6 +49,10 @@ type machineScope struct { // newMachineScope creates a new MachineScope from the supplied parameters. // This is meant to be called for each machine actuator operation. func newMachineScope(params machineScopeParams) (*machineScope, error) { + if params.Context == nil { + params.Context = context.Background() + } + providerSpec, err := v1beta1.ProviderSpecFromRawExtension(params.machine.Spec.ProviderSpec.Value) if err != nil { return nil, machineapierros.InvalidMachineConfiguration("failed to get machine config: %v", err) @@ -78,6 +86,7 @@ func newMachineScope(params machineScopeParams) (*machineScope, error) { return nil, machineapierros.InvalidMachineConfiguration("error creating compute service: %v", err) } return &machineScope{ + Context: params.Context, coreClient: params.coreClient, projectID: projectID, // https://github.com/kubernetes/kubernetes/blob/8765fa2e48974e005ad16e65cb5c3acf5acff17b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_util.go#L204 @@ -163,7 +172,7 @@ func (s *machineScope) PatchMachine() error { statusCopy := *s.machine.Status.DeepCopy() // patch machine - if err := s.coreClient.Patch(context.Background(), s.machine, s.machineToBePatched); err != nil { + if err := s.coreClient.Patch(s.Context, s.machine, s.machineToBePatched); err != nil { klog.Errorf("Failed to patch machine %q: %v", s.machine.GetName(), err) return err } @@ -171,7 +180,7 @@ func (s *machineScope) PatchMachine() error { s.machine.Status = statusCopy // patch status - if err := s.coreClient.Status().Patch(context.Background(), s.machine, s.machineToBePatched); err != nil { + if err := s.coreClient.Status().Patch(s.Context, s.machine, s.machineToBePatched); err != nil { klog.Errorf("Failed to patch machine status %q: %v", s.machine.GetName(), err) return err } From 0b4546979dcb243a43612544fc64731592851b27 Mon Sep 17 00:00:00 2001 From: Alexander Demichev Date: Wed, 22 Jul 2020 14:24:34 +0200 Subject: [PATCH 2/6] Improve error formatting --- pkg/cloud/gcp/actuators/machine/actuator.go | 25 ++++++++++++--------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/pkg/cloud/gcp/actuators/machine/actuator.go b/pkg/cloud/gcp/actuators/machine/actuator.go index fcb564f92..cf4a06f02 100644 --- a/pkg/cloud/gcp/actuators/machine/actuator.go +++ b/pkg/cloud/gcp/actuators/machine/actuator.go @@ -15,7 +15,8 @@ import ( ) const ( - scopeFailFmt = "%s: failed to create scope for machine: %v" + scopeFailFmt = "%s: failed to create scope for machine: %w" + reconcilerFailFmt = "%s: reconciler failed to %s machine: %w" createEventAction = "Create" updateEventAction = "Update" deleteEventAction = "Delete" @@ -45,10 +46,10 @@ func NewActuator(params ActuatorParams) *Actuator { // Set corresponding event based on error. It also returns the original error // for convenience, so callers can do "return handleMachineError(...)". func (a *Actuator) handleMachineError(machine *machinev1.Machine, err error, eventAction string) error { + klog.Errorf("%v error: %v", machine.GetName(), err) if eventAction != noEventAction { a.eventRecorder.Eventf(machine, corev1.EventTypeWarning, "Failed"+eventAction, "%v", err) } - return err } @@ -61,12 +62,14 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error machine: machine, }) if err != nil { - return a.handleMachineError(machine, err, createEventAction) + fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err) + return a.handleMachineError(machine, fmtErr, createEventAction) } if err := newReconciler(scope).create(); err != nil { // Update machine and machine status in case it was modified scope.Close() - return a.handleMachineError(machine, err, createEventAction) + fmtErr := fmt.Errorf(reconcilerFailFmt, machine.GetName(), createEventAction, err) + return a.handleMachineError(machine, fmtErr, createEventAction) } a.eventRecorder.Eventf(machine, corev1.EventTypeNormal, createEventAction, "Created Machine %v", machine.Name) return scope.Close() @@ -98,13 +101,14 @@ func (a *Actuator) Update(ctx context.Context, machine *machinev1.Machine) error machine: machine, }) if err != nil { - fmtErr := fmt.Sprintf(scopeFailFmt, machine.Name, err) - return a.handleMachineError(machine, fmt.Errorf(fmtErr), updateEventAction) + fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err) + return a.handleMachineError(machine, fmtErr, updateEventAction) } if err := newReconciler(scope).update(); err != nil { // Update machine and machine status in case it was modified scope.Close() - return a.handleMachineError(machine, err, updateEventAction) + fmtErr := fmt.Errorf(reconcilerFailFmt, machine.GetName(), updateEventAction, err) + return a.handleMachineError(machine, fmtErr, updateEventAction) } a.eventRecorder.Eventf(machine, corev1.EventTypeNormal, updateEventAction, "Updated Machine %v", machine.Name) return scope.Close() @@ -118,11 +122,12 @@ func (a *Actuator) Delete(ctx context.Context, machine *machinev1.Machine) error machine: machine, }) if err != nil { - fmtErr := fmt.Sprintf(scopeFailFmt, machine.Name, err) - return a.handleMachineError(machine, fmt.Errorf(fmtErr), deleteEventAction) + fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err) + return a.handleMachineError(machine, fmtErr, deleteEventAction) } if err := newReconciler(scope).delete(); err != nil { - return a.handleMachineError(machine, err, deleteEventAction) + fmtErr := fmt.Errorf(reconcilerFailFmt, machine.GetName(), deleteEventAction, err) + return a.handleMachineError(machine, fmtErr, deleteEventAction) } a.eventRecorder.Eventf(machine, corev1.EventTypeNormal, deleteEventAction, "Deleted machine %v", machine.Name) return nil From c6e62d5b4e23f3daa21e27fff4145d1a0cda5702 Mon Sep 17 00:00:00 2001 From: Alexander Demichev Date: Thu, 23 Jul 2020 14:22:23 +0200 Subject: [PATCH 3/6] Add GCP client builder --- cmd/manager/main.go | 6 ++- pkg/cloud/gcp/actuators/machine/actuator.go | 44 ++++++++++------- .../gcp/actuators/machine/machine_scope.go | 13 ++--- .../gcp/actuators/machineset/controller.go | 8 +--- .../services/compute/computeservice.go | 14 ++++-- .../services/compute/computeservice_mock.go | 47 +++++++++++++------ 6 files changed, 79 insertions(+), 53 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 32bbb0d4f..60d943ebf 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -10,6 +10,7 @@ import ( "github.com/openshift/cluster-api-provider-gcp/pkg/apis" "github.com/openshift/cluster-api-provider-gcp/pkg/cloud/gcp/actuators/machine" machinesetcontroller "github.com/openshift/cluster-api-provider-gcp/pkg/cloud/gcp/actuators/machineset" + computeservice "github.com/openshift/cluster-api-provider-gcp/pkg/cloud/gcp/actuators/services/compute" "github.com/openshift/cluster-api-provider-gcp/pkg/version" "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" capimachine "github.com/openshift/machine-api-operator/pkg/controller/machine" @@ -99,8 +100,9 @@ func main() { // Initialize machine actuator. machineActuator := machine.NewActuator(machine.ActuatorParams{ - CoreClient: mgr.GetClient(), - EventRecorder: mgr.GetEventRecorderFor("gcpcontroller"), + CoreClient: mgr.GetClient(), + EventRecorder: mgr.GetEventRecorderFor("gcpcontroller"), + ComputeClientBuilder: computeservice.NewComputeService, }) if err := apis.AddToScheme(mgr.GetScheme()); err != nil { diff --git a/pkg/cloud/gcp/actuators/machine/actuator.go b/pkg/cloud/gcp/actuators/machine/actuator.go index cf4a06f02..ef7e682ae 100644 --- a/pkg/cloud/gcp/actuators/machine/actuator.go +++ b/pkg/cloud/gcp/actuators/machine/actuator.go @@ -7,6 +7,7 @@ import ( "context" "fmt" + computeservice "github.com/openshift/cluster-api-provider-gcp/pkg/cloud/gcp/actuators/services/compute" machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/record" @@ -25,21 +26,24 @@ const ( // Actuator is responsible for performing machine reconciliation. type Actuator struct { - coreClient controllerclient.Client - eventRecorder record.EventRecorder + coreClient controllerclient.Client + eventRecorder record.EventRecorder + computeClientBuilder computeservice.BuilderFuncType } // ActuatorParams holds parameter information for Actuator. type ActuatorParams struct { - CoreClient controllerclient.Client - EventRecorder record.EventRecorder + CoreClient controllerclient.Client + EventRecorder record.EventRecorder + ComputeClientBuilder computeservice.BuilderFuncType } // NewActuator returns an actuator. func NewActuator(params ActuatorParams) *Actuator { return &Actuator{ - coreClient: params.CoreClient, - eventRecorder: params.EventRecorder, + coreClient: params.CoreClient, + eventRecorder: params.EventRecorder, + computeClientBuilder: params.ComputeClientBuilder, } } @@ -57,9 +61,10 @@ func (a *Actuator) handleMachineError(machine *machinev1.Machine, err error, eve func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error { klog.Infof("%s: Creating machine", machine.Name) scope, err := newMachineScope(machineScopeParams{ - Context: ctx, - coreClient: a.coreClient, - machine: machine, + Context: ctx, + coreClient: a.coreClient, + machine: machine, + computeClientBuilder: a.computeClientBuilder, }) if err != nil { fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err) @@ -78,9 +83,10 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error func (a *Actuator) Exists(ctx context.Context, machine *machinev1.Machine) (bool, error) { klog.Infof("%s: Checking if machine exists", machine.Name) scope, err := newMachineScope(machineScopeParams{ - Context: ctx, - coreClient: a.coreClient, - machine: machine, + Context: ctx, + coreClient: a.coreClient, + machine: machine, + computeClientBuilder: a.computeClientBuilder, }) if err != nil { return false, fmt.Errorf(scopeFailFmt, machine.Name, err) @@ -96,9 +102,10 @@ func (a *Actuator) Exists(ctx context.Context, machine *machinev1.Machine) (bool func (a *Actuator) Update(ctx context.Context, machine *machinev1.Machine) error { klog.Infof("%s: Updating machine", machine.Name) scope, err := newMachineScope(machineScopeParams{ - Context: ctx, - coreClient: a.coreClient, - machine: machine, + Context: ctx, + coreClient: a.coreClient, + machine: machine, + computeClientBuilder: a.computeClientBuilder, }) if err != nil { fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err) @@ -117,9 +124,10 @@ func (a *Actuator) Update(ctx context.Context, machine *machinev1.Machine) error func (a *Actuator) Delete(ctx context.Context, machine *machinev1.Machine) error { klog.Infof("%s: Deleting machine", machine.Name) scope, err := newMachineScope(machineScopeParams{ - Context: ctx, - coreClient: a.coreClient, - machine: machine, + Context: ctx, + coreClient: a.coreClient, + machine: machine, + computeClientBuilder: a.computeClientBuilder, }) if err != nil { fmtErr := fmt.Errorf(scopeFailFmt, machine.GetName(), err) diff --git a/pkg/cloud/gcp/actuators/machine/machine_scope.go b/pkg/cloud/gcp/actuators/machine/machine_scope.go index 364e830bb..7f8155de3 100644 --- a/pkg/cloud/gcp/actuators/machine/machine_scope.go +++ b/pkg/cloud/gcp/actuators/machine/machine_scope.go @@ -9,7 +9,6 @@ import ( "github.com/openshift/cluster-api-provider-gcp/pkg/cloud/gcp/actuators/util" machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" machineapierros "github.com/openshift/machine-api-operator/pkg/controller/machine" - "google.golang.org/api/compute/v1" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog" @@ -20,8 +19,9 @@ import ( type machineScopeParams struct { context.Context - coreClient controllerclient.Client - machine *machinev1.Machine + coreClient controllerclient.Client + machine *machinev1.Machine + computeClientBuilder computeservice.BuilderFuncType } // machineScope defines a scope defined around a machine and its cluster. @@ -76,12 +76,7 @@ func newMachineScope(params machineScopeParams) (*machineScope, error) { } } - oauthClient, err := util.CreateOauth2Client(serviceAccountJSON, compute.CloudPlatformScope) - if err != nil { - return nil, machineapierros.InvalidMachineConfiguration("error creating oauth client: %v", err) - } - - computeService, err := computeservice.NewComputeService(oauthClient) + computeService, err := params.computeClientBuilder(serviceAccountJSON) if err != nil { return nil, machineapierros.InvalidMachineConfiguration("error creating compute service: %v", err) } diff --git a/pkg/cloud/gcp/actuators/machineset/controller.go b/pkg/cloud/gcp/actuators/machineset/controller.go index 554fbe1c0..0ee61c5a5 100644 --- a/pkg/cloud/gcp/actuators/machineset/controller.go +++ b/pkg/cloud/gcp/actuators/machineset/controller.go @@ -24,7 +24,6 @@ import ( "github.com/openshift/cluster-api-provider-gcp/pkg/cloud/gcp/actuators/util" machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" mapierrors "github.com/openshift/machine-api-operator/pkg/controller/machine" - "google.golang.org/api/compute/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -168,12 +167,7 @@ func (r *Reconciler) getRealGCPService(namespace string, providerConfig provider return nil, err } - oauthClient, err := util.CreateOauth2Client(serviceAccountJSON, compute.CloudPlatformScope) - if err != nil { - return nil, mapierrors.InvalidMachineConfiguration("error creating oauth client: %v", err) - } - - computeService, err := computeservice.NewComputeService(oauthClient) + computeService, err := computeservice.NewComputeService(serviceAccountJSON) if err != nil { return nil, mapierrors.InvalidMachineConfiguration("error creating compute service: %v", err) } diff --git a/pkg/cloud/gcp/actuators/services/compute/computeservice.go b/pkg/cloud/gcp/actuators/services/compute/computeservice.go index 9fc15ddb5..8abcbb130 100644 --- a/pkg/cloud/gcp/actuators/services/compute/computeservice.go +++ b/pkg/cloud/gcp/actuators/services/compute/computeservice.go @@ -1,8 +1,7 @@ package computeservice import ( - "net/http" - + "github.com/openshift/cluster-api-provider-gcp/pkg/cloud/gcp/actuators/util" "github.com/openshift/cluster-api-provider-gcp/pkg/version" "google.golang.org/api/compute/v1" ) @@ -26,13 +25,22 @@ type computeService struct { service *compute.Service } +// BuilderFuncType is function type for building gcp client +type BuilderFuncType func(serviceAccountJSON string) (GCPComputeService, error) + // NewComputeService return a new computeService -func NewComputeService(oauthClient *http.Client) (*computeService, error) { +func NewComputeService(serviceAccountJSON string) (GCPComputeService, error) { + oauthClient, err := util.CreateOauth2Client(serviceAccountJSON, compute.CloudPlatformScope) + if err != nil { + return nil, err + } + service, err := compute.New(oauthClient) if err != nil { return nil, err } service.UserAgent = "gcpprovider.openshift.io/" + version.Version.String() + return &computeService{ service: service, }, nil diff --git a/pkg/cloud/gcp/actuators/services/compute/computeservice_mock.go b/pkg/cloud/gcp/actuators/services/compute/computeservice_mock.go index 6da2eec81..a0993d826 100644 --- a/pkg/cloud/gcp/actuators/services/compute/computeservice_mock.go +++ b/pkg/cloud/gcp/actuators/services/compute/computeservice_mock.go @@ -2,6 +2,7 @@ package computeservice import ( compute "google.golang.org/api/compute/v1" + "google.golang.org/api/googleapi" ) const ( @@ -37,23 +38,26 @@ func (c *GCPComputeServiceMock) ZoneOperationsGet(project string, zone string, o } func (c *GCPComputeServiceMock) InstancesGet(project string, zone string, instance string) (*compute.Instance, error) { - return &compute.Instance{ - Name: instance, - Zone: zone, - MachineType: "n1-standard-1", - CanIpForward: true, - NetworkInterfaces: []*compute.NetworkInterface{ - { - NetworkIP: "10.0.0.15", - AccessConfigs: []*compute.AccessConfig{ - { - NatIP: "35.243.147.143", + if c.mockInstancesGet == nil { + return &compute.Instance{ + Name: instance, + Zone: zone, + MachineType: "n1-standard-1", + CanIpForward: true, + NetworkInterfaces: []*compute.NetworkInterface{ + { + NetworkIP: "10.0.0.15", + AccessConfigs: []*compute.AccessConfig{ + { + NatIP: "35.243.147.143", + }, }, }, }, - }, - Status: "RUNNING", - }, nil + Status: "RUNNING", + }, nil + } + return c.mockInstancesGet(project, zone, instance) } func (c *GCPComputeServiceMock) ZonesGet(project string, zone string) (*compute.Zone, error) { @@ -110,3 +114,18 @@ func NewComputeServiceMock() (*compute.Instance, *GCPComputeServiceMock) { } return &receivedInstance, &computeServiceMock } + +func MockBuilderFuncType(serviceAccountJSON string) (GCPComputeService, error) { + _, computeSvc := NewComputeServiceMock() + return computeSvc, nil +} + +func MockBuilderFuncTypeNotFound(serviceAccountJSON string) (GCPComputeService, error) { + _, computeSvc := NewComputeServiceMock() + computeSvc.mockInstancesGet = func(project string, zone string, instance string) (*compute.Instance, error) { + return nil, &googleapi.Error{ + Code: 404, + } + } + return computeSvc, nil +} From 1c2f3efc9689028b25fc42727e450fc7af1c60f1 Mon Sep 17 00:00:00 2001 From: Alexander Demichev Date: Thu, 23 Jul 2020 14:23:05 +0200 Subject: [PATCH 4/6] Validate machine labels in update() --- pkg/cloud/gcp/actuators/machine/reconciler.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cloud/gcp/actuators/machine/reconciler.go b/pkg/cloud/gcp/actuators/machine/reconciler.go index bf180e50c..d6afb930f 100644 --- a/pkg/cloud/gcp/actuators/machine/reconciler.go +++ b/pkg/cloud/gcp/actuators/machine/reconciler.go @@ -168,6 +168,10 @@ func (r *Reconciler) create() error { } func (r *Reconciler) update() error { + if err := validateMachine(*r.machine, *r.providerSpec); err != nil { + return machinecontroller.InvalidMachineConfiguration("failed validating machine provider spec: %v", err) + } + // Add target pools, if necessary if err := r.processTargetPools(true, r.addInstanceToTargetPool); err != nil { return err From d718d470f6fdb1652b44c4514c7d844edc48b411 Mon Sep 17 00:00:00 2001 From: Alexander Demichev Date: Thu, 23 Jul 2020 14:23:20 +0200 Subject: [PATCH 5/6] Add tests for actuator events --- .../gcp/actuators/machine/actuator_test.go | 269 +++++++++++++++++- 1 file changed, 257 insertions(+), 12 deletions(-) diff --git a/pkg/cloud/gcp/actuators/machine/actuator_test.go b/pkg/cloud/gcp/actuators/machine/actuator_test.go index dcfe6b96f..11845fc75 100644 --- a/pkg/cloud/gcp/actuators/machine/actuator_test.go +++ b/pkg/cloud/gcp/actuators/machine/actuator_test.go @@ -1,12 +1,26 @@ package machine import ( + "context" + "fmt" + "path/filepath" "testing" + "time" + . "github.com/onsi/gomega" + "github.com/openshift/cluster-api-provider-gcp/pkg/apis/gcpprovider/v1beta1" + computeservice "github.com/openshift/cluster-api-provider-gcp/pkg/cloud/gcp/actuators/services/compute" machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" + + corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/tools/record" - "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/manager" ) func init() { @@ -14,17 +28,248 @@ func init() { machinev1.AddToScheme(scheme.Scheme) } -func TestActuatorCreate(t *testing.T) { - eventsChannel := make(chan string, 1) - recorder := &record.FakeRecorder{ - Events: eventsChannel, +func TestActuatorEvents(t *testing.T) { + g := NewWithT(t) + timeout := 10 * time.Second + + testEnv := &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "..", "..", "config", "crds")}, + } + + cfg, err := testEnv.Start() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(cfg).ToNot(BeNil()) + defer func() { + g.Expect(testEnv.Stop()).To(Succeed()) + }() + + mgr, err := manager.New(cfg, manager.Options{ + Scheme: scheme.Scheme, + MetricsBindAddress: "0", + }) + if err != nil { + t.Fatal(err) + } + + doneMgr := make(chan struct{}) + defer close(doneMgr) + + go func() { + g.Expect(mgr.Start(doneMgr)).To(Succeed()) + }() + + k8sClient := mgr.GetClient() + eventRecorder := mgr.GetEventRecorderFor("vspherecontroller") + + userDataSecretName := "user-data-test" + credentialsSecretName := "credentials-test" + defaultNamespaceName := "test" + credentialsSecretKey := "service_account.json" + + defaultNamespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaultNamespaceName, + }, + } + g.Expect(k8sClient.Create(context.Background(), defaultNamespace)).To(Succeed()) + defer func() { + g.Expect(k8sClient.Delete(context.Background(), defaultNamespace)).To(Succeed()) + }() + + userDataSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: userDataSecretName, + Namespace: defaultNamespaceName, + }, + Data: map[string][]byte{ + userDataSecretKey: []byte("userDataBlob"), + }, + } + + g.Expect(k8sClient.Create(context.Background(), userDataSecret)).To(Succeed()) + defer func() { + g.Expect(k8sClient.Delete(context.Background(), userDataSecret)).To(Succeed()) + }() + + credentialsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: credentialsSecretName, + Namespace: defaultNamespaceName, + }, + Data: map[string][]byte{ + credentialsSecretKey: []byte("{\"project_id\": \"test\"}"), + }, } - // Initialize machine actuator. - machineActuator := NewActuator(ActuatorParams{ - CoreClient: fake.NewFakeClient(), - EventRecorder: recorder, + + g.Expect(k8sClient.Create(context.Background(), credentialsSecret)).To(Succeed()) + defer func() { + g.Expect(k8sClient.Delete(context.Background(), credentialsSecret)).To(Succeed()) + }() + + providerSpec, err := v1beta1.RawExtensionFromProviderSpec(&v1beta1.GCPMachineProviderSpec{ + CredentialsSecret: &corev1.LocalObjectReference{ + Name: credentialsSecretName, + }, }) - if machineActuator == nil { - t.Errorf("expected machine not nil") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(providerSpec).ToNot(BeNil()) + + cases := []struct { + name string + error string + operation func(actuator *Actuator, machine *machinev1.Machine) + event string + }{ + { + name: "Create machine event failed on invalid machine scope", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + machine.Spec = machinev1.MachineSpec{ + ProviderSpec: machinev1.ProviderSpec{ + Value: &runtime.RawExtension{ + Raw: []byte{'1'}, + }, + }, + } + actuator.Create(context.Background(), machine) + }, + event: "test: failed to create scope for machine: failed to get machine config: error unmarshalling providerSpec: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal number into Go value of type v1beta1.GCPMachineProviderSpec", + }, + { + name: "Create machine event failed, reconciler's create failed", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + machine.Labels[machinev1.MachineClusterIDLabel] = "" + actuator.Create(context.Background(), machine) + }, + event: "test: reconciler failed to Create machine: failed validating machine provider spec: machine is missing \"machine.openshift.io/cluster-api-cluster\" label", + }, + { + name: "Create machine event succeed", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + actuator.Create(context.Background(), machine) + }, + event: "Created Machine test", + }, + { + name: "Update machine event failed on invalid machine scope", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + machine.Spec = machinev1.MachineSpec{ + ProviderSpec: machinev1.ProviderSpec{ + Value: &runtime.RawExtension{ + Raw: []byte{'1'}, + }, + }, + } + actuator.Update(context.Background(), machine) + }, + event: "test: failed to create scope for machine: failed to get machine config: error unmarshalling providerSpec: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal number into Go value of type v1beta1.GCPMachineProviderSpec", + }, + { + name: "Update machine event failed, reconciler's update failed", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + machine.Labels[machinev1.MachineClusterIDLabel] = "" + actuator.Update(context.Background(), machine) + }, + event: "test: reconciler failed to Update machine: failed validating machine provider spec: machine is missing \"machine.openshift.io/cluster-api-cluster\" label", + }, + { + name: "Update machine event succeed and only one event is created", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + actuator.Update(context.Background(), machine) + actuator.Update(context.Background(), machine) + }, + event: "Updated Machine test", + }, + { + name: "Delete machine event failed on invalid machine scope", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + machine.Spec = machinev1.MachineSpec{ + ProviderSpec: machinev1.ProviderSpec{ + Value: &runtime.RawExtension{ + Raw: []byte{'1'}, + }, + }, + } + actuator.Delete(context.Background(), machine) + }, + event: "test: failed to create scope for machine: failed to get machine config: error unmarshalling providerSpec: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal number into Go value of type v1beta1.GCPMachineProviderSpec", + }, + { + name: "Delete machine event failed, reconciler's delete failed", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + actuator.Delete(context.Background(), machine) + }, + event: "test: reconciler failed to Delete machine: requeue in: 20s", + }, + { + name: "Delete machine event succeed", + operation: func(actuator *Actuator, machine *machinev1.Machine) { + actuator.computeClientBuilder = computeservice.MockBuilderFuncTypeNotFound + actuator.Delete(context.Background(), machine) + }, + event: "Deleted machine test", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + gs := NewWithT(t) + + machine := &machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: defaultNamespaceName, + Labels: map[string]string{ + machinev1.MachineClusterIDLabel: "CLUSTERID", + }, + }, + Spec: machinev1.MachineSpec{ + ProviderSpec: machinev1.ProviderSpec{ + Value: providerSpec, + }, + }} + + // Create the machine + gs.Expect(k8sClient.Create(context.Background(), machine)).To(Succeed()) + defer func() { + gs.Expect(k8sClient.Delete(context.Background(), machine)).To(Succeed()) + }() + + // Ensure the machine has synced to the cache + getMachine := func() error { + machineKey := types.NamespacedName{Namespace: machine.Namespace, Name: machine.Name} + return k8sClient.Get(context.Background(), machineKey, machine) + } + gs.Eventually(getMachine, timeout).Should(Succeed()) + + params := ActuatorParams{ + CoreClient: k8sClient, + EventRecorder: eventRecorder, + ComputeClientBuilder: computeservice.MockBuilderFuncType, + } + + actuator := NewActuator(params) + tc.operation(actuator, machine) + + eventList := &v1.EventList{} + waitForEvent := func() error { + err := k8sClient.List(context.Background(), eventList, client.InNamespace(machine.Namespace)) + if err != nil { + return err + } + + if len(eventList.Items) != 1 { + return fmt.Errorf("expected len 1, got %d", len(eventList.Items)) + } + return nil + } + + gs.Eventually(waitForEvent, timeout).Should(Succeed()) + + gs.Expect(eventList.Items[0].Message).To(Equal(tc.event)) + + for i := range eventList.Items { + gs.Expect(k8sClient.Delete(context.Background(), &eventList.Items[i])).To(Succeed()) + } + }) } } From 237c81216ed35d3b82a86891db92a19604d42e0d Mon Sep 17 00:00:00 2001 From: Alexander Demichev Date: Wed, 29 Jul 2020 17:10:10 +0200 Subject: [PATCH 6/6] Add tests for actuator exists() --- .../gcp/actuators/machine/actuator_test.go | 105 +++++++++++++++++- 1 file changed, 100 insertions(+), 5 deletions(-) diff --git a/pkg/cloud/gcp/actuators/machine/actuator_test.go b/pkg/cloud/gcp/actuators/machine/actuator_test.go index 11845fc75..a806465ef 100644 --- a/pkg/cloud/gcp/actuators/machine/actuator_test.go +++ b/pkg/cloud/gcp/actuators/machine/actuator_test.go @@ -19,10 +19,18 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" + controllerfake "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/manager" ) +var ( + userDataSecretName = "user-data-test" + credentialsSecretName = "credentials-test" + defaultNamespaceName = "test" + credentialsSecretKey = "service_account.json" +) + func init() { // Add types to scheme machinev1.AddToScheme(scheme.Scheme) @@ -61,11 +69,6 @@ func TestActuatorEvents(t *testing.T) { k8sClient := mgr.GetClient() eventRecorder := mgr.GetEventRecorderFor("vspherecontroller") - userDataSecretName := "user-data-test" - credentialsSecretName := "credentials-test" - defaultNamespaceName := "test" - credentialsSecretKey := "service_account.json" - defaultNamespace := &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: defaultNamespaceName, @@ -273,3 +276,95 @@ func TestActuatorEvents(t *testing.T) { }) } } + +func TestActuatorExists(t *testing.T) { + userDataSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: userDataSecretName, + Namespace: defaultNamespaceName, + }, + Data: map[string][]byte{ + userDataSecretKey: []byte("userDataBlob"), + }, + } + + credentialsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: credentialsSecretName, + Namespace: defaultNamespaceName, + }, + Data: map[string][]byte{ + credentialsSecretKey: []byte("{\"project_id\": \"test\"}"), + }, + } + + providerSpec, err := v1beta1.RawExtensionFromProviderSpec(&v1beta1.GCPMachineProviderSpec{ + CredentialsSecret: &corev1.LocalObjectReference{ + Name: credentialsSecretName, + }, + }) + if err != nil { + t.Fatal(err) + } + + cases := []struct { + name string + expectError bool + }{ + { + name: "succefuly call reconciler exists", + }, + { + name: "fail to call reconciler exists", + expectError: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + machine := &machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: defaultNamespaceName, + Labels: map[string]string{ + machinev1.MachineClusterIDLabel: "CLUSTERID", + }, + }, + Spec: machinev1.MachineSpec{ + ProviderSpec: machinev1.ProviderSpec{ + Value: providerSpec, + }, + }} + + if tc.expectError { + machine.Spec = machinev1.MachineSpec{ + ProviderSpec: machinev1.ProviderSpec{ + Value: &runtime.RawExtension{ + Raw: []byte{'1'}, + }, + }, + } + } + + params := ActuatorParams{ + CoreClient: controllerfake.NewFakeClient(userDataSecret, credentialsSecret), + ComputeClientBuilder: computeservice.MockBuilderFuncType, + } + + actuator := NewActuator(params) + + _, err := actuator.Exists(nil, machine) + + if tc.expectError { + if err == nil { + t.Fatal("actuator exists expected to return an error") + } + } else { + if err != nil { + t.Fatal("actuator exists is not expected to return an error") + } + } + }) + } + +}