diff --git a/cloud/scope/client.go b/cloud/scope/client.go index 4ac98c377..a45f909db 100644 --- a/cloud/scope/client.go +++ b/cloud/scope/client.go @@ -4,7 +4,6 @@ import ( "context" "github.com/linode/linodego" - "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -27,7 +26,3 @@ func CreateLinodeObjectStorageClient(apiKey string) (LinodeObjectStorageClient, type k8sClient interface { client.Client } - -type PatchHelper interface { - Patch(ctx context.Context, obj client.Object, opts ...patch.Option) error -} diff --git a/cloud/scope/cluster.go b/cloud/scope/cluster.go index 6947588e1..8f1ea0f9f 100644 --- a/cloud/scope/cluster.go +++ b/cloud/scope/cluster.go @@ -23,6 +23,7 @@ import ( "github.com/linode/linodego" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" @@ -48,7 +49,7 @@ func validateClusterScopeParams(params ClusterScopeParams) error { // NewClusterScope creates a new Scope from the supplied parameters. // This is meant to be called for each reconcile iteration. -func NewClusterScope(ctx context.Context, apiKey string, params ClusterScopeParams, patchNewHelper patchNewHelper) (*ClusterScope, error) { +func NewClusterScope(ctx context.Context, apiKey string, params ClusterScopeParams) (*ClusterScope, error) { if err := validateClusterScopeParams(params); err != nil { return nil, err } @@ -66,7 +67,7 @@ func NewClusterScope(ctx context.Context, apiKey string, params ClusterScopePara return nil, fmt.Errorf("failed to create linode client: %w", err) } - helper, err := patchNewHelper(params.LinodeCluster, params.Client) + helper, err := patch.NewHelper(params.LinodeCluster, params.Client) if err != nil { return nil, fmt.Errorf("failed to init patch helper: %w", err) } @@ -83,7 +84,7 @@ func NewClusterScope(ctx context.Context, apiKey string, params ClusterScopePara // ClusterScope defines the basic context for an actuator to operate upon. type ClusterScope struct { client k8sClient - PatchHelper PatchHelper + PatchHelper *patch.Helper LinodeClient *linodego.Client Cluster *clusterv1.Cluster LinodeCluster *infrav1alpha1.LinodeCluster diff --git a/cloud/scope/cluster_test.go b/cloud/scope/cluster_test.go index 51bbc23de..aecc207f3 100644 --- a/cloud/scope/cluster_test.go +++ b/cloud/scope/cluster_test.go @@ -25,9 +25,9 @@ import ( "go.uber.org/mock/gomock" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/client" infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" @@ -100,9 +100,9 @@ func TestClusterScopeMethods(t *testing.T) { } tests := []struct { - name string - fields fields - patchErr error + name string + fields fields + expects func(mock *mock.Mockk8sClient) }{ { name: "Success - finalizer should be added to the Linode Cluster object", @@ -114,7 +114,14 @@ func TestClusterScopeMethods(t *testing.T) { }, }, }, - patchErr: nil, + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }).Times(2) + mock.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + }, }, { name: "AddFinalizer error - finalizer should not be added to the Linode Cluster object. Function returns nil since it was already present", @@ -127,7 +134,13 @@ func TestClusterScopeMethods(t *testing.T) { }, }, }, - patchErr: nil, + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }).Times(1) + }, }, } for _, tt := range tests { @@ -138,24 +151,20 @@ func TestClusterScopeMethods(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - mockPatchHelper := mock.NewMockPatchHelper(ctrl) mockK8sClient := mock.NewMockk8sClient(ctrl) - lClient, err := CreateLinodeClient("test-key") - if err != nil { - t.Errorf("createLinodeClient() error = %v", err) - } - - cScope := &ClusterScope{ - client: mockK8sClient, - PatchHelper: mockPatchHelper, - LinodeClient: lClient, - Cluster: testcase.fields.Cluster, - LinodeCluster: testcase.fields.LinodeCluster, - } + testcase.expects(mockK8sClient) - if cScope.LinodeCluster.Finalizers == nil { - mockPatchHelper.EXPECT().Patch(gomock.Any(), gomock.Any()).Return(testcase.patchErr) + cScope, err := NewClusterScope( + context.Background(), + "test-key", + ClusterScopeParams{ + Cluster: testcase.fields.Cluster, + LinodeCluster: testcase.fields.LinodeCluster, + Client: mockK8sClient, + }) + if err != nil { + t.Errorf("ClusterScope() error = %v", err) } if err := cScope.AddFinalizer(context.Background()); err != nil { @@ -179,8 +188,7 @@ func TestNewClusterScope(t *testing.T) { name string args args expectedError error - getFunc func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error - patchFunc func(obj client.Object, crClient client.Client) (*patch.Helper, error) + expects func(mock *mock.Mockk8sClient) }{ { name: "Success - Pass in valid args and get a valid ClusterScope", @@ -192,11 +200,12 @@ func TestNewClusterScope(t *testing.T) { }, }, expectedError: nil, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return nil - }, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }) }, }, { @@ -217,18 +226,21 @@ func TestNewClusterScope(t *testing.T) { }, }, expectedError: nil, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - cred := corev1.Secret{ - Data: map[string][]byte{ - "apiToken": []byte("example"), - }, - } - *obj = cred - - return nil - }, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }) + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { + cred := corev1.Secret{ + Data: map[string][]byte{ + "apiToken": []byte("example"), + }, + } + *obj = cred + return nil + }) }, }, { @@ -238,12 +250,7 @@ func TestNewClusterScope(t *testing.T) { params: ClusterScopeParams{}, }, expectedError: fmt.Errorf("cluster is required when creating a ClusterScope"), - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return nil - }, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil - }, + expects: func(mock *mock.Mockk8sClient) {}, }, { name: "Error - patchHelper returns error. Checking error handle for when new patchHelper is invoked", @@ -254,12 +261,9 @@ func TestNewClusterScope(t *testing.T) { LinodeCluster: &infrav1alpha1.LinodeCluster{}, }, }, - expectedError: fmt.Errorf("failed to init patch helper: obj is nil"), - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return nil - }, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return nil, fmt.Errorf("obj is nil") + expectedError: fmt.Errorf("failed to init patch helper:"), + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().Return(runtime.NewScheme()) }, }, { @@ -280,11 +284,8 @@ func TestNewClusterScope(t *testing.T) { }, }, expectedError: fmt.Errorf("credentials from secret ref: get credentials secret test/example: failed to get secret"), - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return fmt.Errorf("failed to get secret") - }, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to get secret")) }, }, { @@ -297,12 +298,7 @@ func TestNewClusterScope(t *testing.T) { }, }, expectedError: fmt.Errorf("failed to create linode client: missing Linode API key"), - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return nil - }, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil - }, + expects: func(mock *mock.Mockk8sClient) {}, }, } @@ -316,16 +312,14 @@ func TestNewClusterScope(t *testing.T) { mockK8sClient := mock.NewMockk8sClient(ctrl) - if testcase.args.params.LinodeCluster != nil && testcase.args.params.LinodeCluster.Spec.CredentialsRef != nil { - mockK8sClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(testcase.getFunc) - } + testcase.expects(mockK8sClient) testcase.args.params.Client = mockK8sClient - got, err := NewClusterScope(context.Background(), testcase.args.apiKey, testcase.args.params, testcase.patchFunc) + got, err := NewClusterScope(context.Background(), testcase.args.apiKey, testcase.args.params) if testcase.expectedError != nil { - assert.EqualError(t, err, testcase.expectedError.Error()) + assert.ErrorContains(t, err, testcase.expectedError.Error()) } else { assert.NotEmpty(t, got) } diff --git a/cloud/scope/common.go b/cloud/scope/common.go index b14de5f32..3e912f23e 100644 --- a/cloud/scope/common.go +++ b/cloud/scope/common.go @@ -9,14 +9,11 @@ import ( "github.com/linode/linodego" "golang.org/x/oauth2" corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/linode/cluster-api-provider-linode/version" ) -type patchNewHelper func(obj client.Object, crClient client.Client) (*patch.Helper, error) - func CreateLinodeClient(apiKey string) (*linodego.Client, error) { if apiKey == "" { return nil, errors.New("missing Linode API key") diff --git a/cloud/scope/machine.go b/cloud/scope/machine.go index cbfc04a08..e382450ac 100644 --- a/cloud/scope/machine.go +++ b/cloud/scope/machine.go @@ -9,14 +9,14 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" ) type MachineScopeParams struct { - Client client.Client + Client k8sClient Cluster *clusterv1.Cluster Machine *clusterv1.Machine LinodeCluster *infrav1alpha1.LinodeCluster @@ -26,7 +26,7 @@ type MachineScopeParams struct { type MachineScope struct { client k8sClient - PatchHelper PatchHelper + PatchHelper *patch.Helper Cluster *clusterv1.Cluster Machine *clusterv1.Machine LinodeClient *linodego.Client @@ -51,7 +51,7 @@ func validateMachineScopeParams(params MachineScopeParams) error { return nil } -func NewMachineScope(ctx context.Context, apiKey string, params MachineScopeParams, patchNewHelper patchNewHelper) (*MachineScope, error) { +func NewMachineScope(ctx context.Context, apiKey string, params MachineScopeParams) (*MachineScope, error) { if err := validateMachineScopeParams(params); err != nil { return nil, err } @@ -88,7 +88,7 @@ func NewMachineScope(ctx context.Context, apiKey string, params MachineScopePara return nil, fmt.Errorf("failed to create linode client: %w", err) } - helper, err := patchNewHelper(params.LinodeMachine, params.Client) + helper, err := patch.NewHelper(params.LinodeMachine, params.Client) if err != nil { return nil, fmt.Errorf("failed to init patch helper: %w", err) } diff --git a/cloud/scope/machine_test.go b/cloud/scope/machine_test.go index 5f22a6397..5705c6ef8 100644 --- a/cloud/scope/machine_test.go +++ b/cloud/scope/machine_test.go @@ -10,6 +10,7 @@ import ( "go.uber.org/mock/gomock" corev1 "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/utils/ptr" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -106,7 +107,7 @@ func TestValidateMachineScopeParams(t *testing.T) { } } -func TestMachineScopeAddFinalizer(t *testing.T) { +func TestMachineScopeMethods(t *testing.T) { t.Parallel() type fields struct { LinodeMachine *infrav1alpha1.LinodeMachine @@ -114,7 +115,7 @@ func TestMachineScopeAddFinalizer(t *testing.T) { tests := []struct { name string fields fields - wantErr bool + expects func(mock *mock.Mockk8sClient) }{ // TODO: Add test cases. { @@ -122,18 +123,32 @@ func TestMachineScopeAddFinalizer(t *testing.T) { fields{ LinodeMachine: &infrav1alpha1.LinodeMachine{}, }, - false, + func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }).Times(2) + mock.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + }, }, { "AddFinalizer error - finalizer should not be added to the Linode Machine object. Function returns nil since it was already present", fields{ LinodeMachine: &infrav1alpha1.LinodeMachine{ ObjectMeta: metav1.ObjectMeta{ + Name: "test-machine", Finalizers: []string{infrav1alpha1.GroupVersion.String()}, }, }, }, - false, + func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }).Times(1) + }, }, } for _, tt := range tests { @@ -144,25 +159,27 @@ func TestMachineScopeAddFinalizer(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - mockPatchHelper := mock.NewMockPatchHelper(ctrl) mockK8sClient := mock.NewMockk8sClient(ctrl) - mScope := &MachineScope{ - client: mockK8sClient, - PatchHelper: mockPatchHelper, - Cluster: &clusterv1.Cluster{}, - Machine: &clusterv1.Machine{}, - LinodeClient: &linodego.Client{}, - LinodeCluster: &infrav1alpha1.LinodeCluster{}, - LinodeMachine: testcase.fields.LinodeMachine, - } + testcase.expects(mockK8sClient) - if mScope.LinodeMachine.Finalizers == nil { - mockPatchHelper.EXPECT().Patch(gomock.Any(), gomock.Any()).Return(nil) + mScope, err := NewMachineScope( + context.Background(), + "test-key", + MachineScopeParams{ + Client: mockK8sClient, + Cluster: &clusterv1.Cluster{}, + Machine: &clusterv1.Machine{}, + LinodeCluster: &infrav1alpha1.LinodeCluster{}, + LinodeMachine: testcase.fields.LinodeMachine, + }, + ) + if err != nil { + t.Errorf("NewMachineScope() error = %v", err) } - if err := mScope.AddFinalizer(context.Background()); (err != nil) != testcase.wantErr { - t.Errorf("MachineScope.AddFinalizer() error = %v, wantErr %v", err, testcase.wantErr) + if err := mScope.AddFinalizer(context.Background()); err != nil { + t.Errorf("MachineScope.AddFinalizer() error = %v", err) } if mScope.LinodeMachine.Finalizers[0] != infrav1alpha1.GroupVersion.String() { @@ -183,10 +200,8 @@ func TestNewMachineScope(t *testing.T) { args args want *MachineScope expectedErr error - patchFunc func(obj client.Object, crClient client.Client) (*patch.Helper, error) - getFunc func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error + expects func(mock *mock.Mockk8sClient) }{ - // TODO: Add test cases. { name: "Success - Pass in valid args and get a valid MachineScope", args: args{ @@ -199,13 +214,13 @@ func TestNewMachineScope(t *testing.T) { LinodeMachine: &infrav1alpha1.LinodeMachine{}, }, }, - want: &MachineScope{}, expectedErr: nil, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil - }, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return nil + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }) }, }, { @@ -227,20 +242,22 @@ func TestNewMachineScope(t *testing.T) { }, }, }, - want: &MachineScope{}, expectedErr: nil, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil - }, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - cred := corev1.Secret{ - Data: map[string][]byte{ - "apiToken": []byte("example"), - }, - } - *obj = cred - - return nil + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }) + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { + cred := corev1.Secret{ + Data: map[string][]byte{ + "apiToken": []byte("example"), + }, + } + *obj = cred + return nil + }) }, }, { @@ -262,20 +279,22 @@ func TestNewMachineScope(t *testing.T) { LinodeMachine: &infrav1alpha1.LinodeMachine{}, }, }, - want: &MachineScope{}, expectedErr: nil, - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil - }, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - cred := corev1.Secret{ - Data: map[string][]byte{ - "apiToken": []byte("example"), - }, - } - *obj = cred - - return nil + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }) + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { + cred := corev1.Secret{ + Data: map[string][]byte{ + "apiToken": []byte("example"), + }, + } + *obj = cred + return nil + }) }, }, { @@ -297,13 +316,9 @@ func TestNewMachineScope(t *testing.T) { LinodeMachine: &infrav1alpha1.LinodeMachine{}, }, }, - want: &MachineScope{}, expectedErr: errors.New("credentials from cluster secret ref: get credentials secret test/example: Creds not found"), - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil - }, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return errors.New("Creds not found") + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("Creds not found")) }, }, { @@ -318,14 +333,8 @@ func TestNewMachineScope(t *testing.T) { LinodeMachine: &infrav1alpha1.LinodeMachine{}, }, }, - want: nil, expectedErr: errors.New("custer is required when creating a MachineScope"), - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return &patch.Helper{}, nil - }, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return nil - }, + expects: func(mock *mock.Mockk8sClient) {}, }, { name: "Error - Pass in valid args but couldn't get patch helper", @@ -339,14 +348,25 @@ func TestNewMachineScope(t *testing.T) { LinodeMachine: &infrav1alpha1.LinodeMachine{}, }, }, - want: &MachineScope{}, - expectedErr: errors.New("failed to init patch helper: failed to create patch helper"), - patchFunc: func(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return nil, errors.New("failed to create patch helper") + expectedErr: errors.New("failed to init patch helper:"), + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().Return(runtime.NewScheme()) }, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return nil + }, + { + name: "Error - createLinodeClient() returns error for passing empty apiKey", + args: args{ + apiKey: "", + params: MachineScopeParams{ + Client: nil, + Cluster: &clusterv1.Cluster{}, + Machine: &clusterv1.Machine{}, + LinodeCluster: &infrav1alpha1.LinodeCluster{}, + LinodeMachine: &infrav1alpha1.LinodeMachine{}, + }, }, + expectedErr: errors.New("failed to create linode client: missing Linode API key"), + expects: func(mock *mock.Mockk8sClient) {}, }, } @@ -360,20 +380,14 @@ func TestNewMachineScope(t *testing.T) { mockK8sClient := mock.NewMockk8sClient(ctrl) - linodeMachine := testcase.args.params.LinodeMachine - linodeCluster := testcase.args.params.LinodeCluster - - if (linodeMachine != nil && linodeMachine.Spec.CredentialsRef != nil) || - (linodeCluster != nil && linodeCluster.Spec.CredentialsRef != nil) { - mockK8sClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(testcase.getFunc).Times(1) - } + testcase.expects(mockK8sClient) testcase.args.params.Client = mockK8sClient - got, err := NewMachineScope(context.Background(), testcase.args.apiKey, testcase.args.params, testcase.patchFunc) + got, err := NewMachineScope(context.Background(), testcase.args.apiKey, testcase.args.params) if testcase.expectedErr != nil { - assert.EqualError(t, err, testcase.expectedErr.Error()) + assert.ErrorContains(t, err, testcase.expectedErr.Error()) } else { assert.NotEmpty(t, got) } @@ -395,7 +409,7 @@ func TestMachineScopeGetBootstrapData(t *testing.T) { fields fields want []byte expectedErr error - getFunc func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error + expects func(mock *mock.Mockk8sClient) }{ // TODO: Add test cases. { @@ -420,14 +434,16 @@ func TestMachineScopeGetBootstrapData(t *testing.T) { }, want: []byte("test-data"), expectedErr: nil, - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - cred := corev1.Secret{ - Data: map[string][]byte{ - "value": []byte("test-data"), - }, - } - *obj = cred - return nil + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { + cred := corev1.Secret{ + Data: map[string][]byte{ + "value": []byte("test-data"), + }, + } + *obj = cred + return nil + }) }, }, { @@ -452,9 +468,7 @@ func TestMachineScopeGetBootstrapData(t *testing.T) { }, want: nil, expectedErr: errors.New("bootstrap data secret is nil for LinodeMachine test-namespace/test-linode-machine"), - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return nil - }, + expects: func(mock *mock.Mockk8sClient) {}, }, { name: "Error - client.Get return an error while retrieving bootstrap data secret", @@ -478,8 +492,8 @@ func TestMachineScopeGetBootstrapData(t *testing.T) { }, want: nil, expectedErr: errors.New("failed to retrieve bootstrap data secret for LinodeMachine test-namespace/test-linode-machine"), - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - return errors.New("test-error") + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("test-error")) }, }, { @@ -504,12 +518,16 @@ func TestMachineScopeGetBootstrapData(t *testing.T) { }, want: nil, expectedErr: errors.New("bootstrap data secret value key is missing for LinodeMachine test-namespace/test-linode-machine"), - getFunc: func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - cred := corev1.Secret{ - Data: map[string][]byte{}, - } - *obj = cred - return nil + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { + cred := corev1.Secret{ + Data: map[string][]byte{}, + } + *obj = cred + return nil + }, + ) }, }, } @@ -522,10 +540,7 @@ func TestMachineScopeGetBootstrapData(t *testing.T) { defer ctrl.Finish() mockK8sClient := mock.NewMockk8sClient(ctrl) - - if testcase.fields.Machine.Spec.Bootstrap.DataSecretName != nil { - mockK8sClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(testcase.getFunc).Times(1) - } + testcase.expects(mockK8sClient) mScope := &MachineScope{ client: mockK8sClient, diff --git a/cloud/scope/vpc.go b/cloud/scope/vpc.go index 10b092b44..5cc3b8eeb 100644 --- a/cloud/scope/vpc.go +++ b/cloud/scope/vpc.go @@ -23,7 +23,6 @@ import ( "github.com/linode/linodego" "sigs.k8s.io/cluster-api/util/patch" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" @@ -31,7 +30,7 @@ import ( // VPCScope defines the basic context for an actuator to operate upon. type VPCScope struct { - client client.Client + client k8sClient PatchHelper *patch.Helper LinodeClient *linodego.Client @@ -40,7 +39,7 @@ type VPCScope struct { // VPCScopeParams defines the input parameters used to create a new Scope. type VPCScopeParams struct { - Client client.Client + Client k8sClient LinodeVPC *infrav1alpha1.LinodeVPC } diff --git a/cloud/scope/vpc_test.go b/cloud/scope/vpc_test.go new file mode 100644 index 000000000..1110f6b4d --- /dev/null +++ b/cloud/scope/vpc_test.go @@ -0,0 +1,281 @@ +/* +Copyright 2023 Akamai Technologies, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package scope + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + infrav1alpha1 "github.com/linode/cluster-api-provider-linode/api/v1alpha1" + "github.com/linode/cluster-api-provider-linode/mock" +) + +func TestValidateVPCScopeParams(t *testing.T) { + t.Parallel() + tests := []struct { + name string + wantErr bool + params VPCScopeParams + }{ + { + name: "Valid VPCScopeParams", + wantErr: false, + params: VPCScopeParams{ + LinodeVPC: &infrav1alpha1.LinodeVPC{}, + }, + }, + { + name: "Invalid VPCScopeParams", + wantErr: true, + params: VPCScopeParams{}, + }, + } + for _, tt := range tests { + testcase := tt + t.Run(testcase.name, func(t *testing.T) { + t.Parallel() + if err := validateVPCScopeParams(testcase.params); (err != nil) != testcase.wantErr { + t.Errorf("validateVPCScopeParams() error = %v, wantErr %v", err, testcase.wantErr) + } + }) + } +} + +func TestNewVPCScope(t *testing.T) { + t.Parallel() + type args struct { + apiKey string + params VPCScopeParams + } + tests := []struct { + name string + args args + want *VPCScope + expectedError error + expects func(m *mock.Mockk8sClient) + }{ + { + name: "Success - Pass in valid args and get a valid VPCScope", + args: args{ + apiKey: "test-key", + params: VPCScopeParams{ + LinodeVPC: &infrav1alpha1.LinodeVPC{}, + }, + }, + expectedError: nil, + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }) + }, + }, + { + name: "Success - Validate getCredentialDataFromRef() returns some apiKey data and we create a valid ClusterScope", + args: args{ + apiKey: "test-key", + params: VPCScopeParams{ + LinodeVPC: &infrav1alpha1.LinodeVPC{ + Spec: infrav1alpha1.LinodeVPCSpec{ + CredentialsRef: &corev1.SecretReference{ + Namespace: "test-namespace", + Name: "test-name", + }, + }, + }, + }, + }, + expectedError: nil, + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }) + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { + cred := corev1.Secret{ + Data: map[string][]byte{ + "apiToken": []byte("example-api-token"), + }, + } + *obj = cred + return nil + }) + }, + }, + { + name: "Error - Pass in invalid args and get an error", + args: args{ + apiKey: "test-key", + params: VPCScopeParams{}, + }, + expects: func(mock *mock.Mockk8sClient) {}, + expectedError: fmt.Errorf("linodeVPC is required when creating a VPCScope"), + }, + { + name: "Error - Pass in valid args but get an error when getting the credentials secret", + args: args{ + apiKey: "test-key", + params: VPCScopeParams{ + LinodeVPC: &infrav1alpha1.LinodeVPC{ + Spec: infrav1alpha1.LinodeVPCSpec{ + CredentialsRef: &corev1.SecretReference{ + Namespace: "test-namespace", + Name: "test-name", + }, + }, + }, + }, + }, + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("test error")) + }, + expectedError: fmt.Errorf("credentials from secret ref: get credentials secret test-namespace/test-name: test error"), + }, + { + name: "Error - Pass in valid args but get an error when creating a new linode client", + args: args{ + apiKey: "", + params: VPCScopeParams{ + LinodeVPC: &infrav1alpha1.LinodeVPC{}, + }, + }, + expects: func(mock *mock.Mockk8sClient) {}, + expectedError: fmt.Errorf("failed to create linode client: missing Linode API key"), + }, + { + name: "Error - Pass in valid args but get an error when creating a new patch helper", + args: args{ + apiKey: "test-key", + params: VPCScopeParams{ + LinodeVPC: &infrav1alpha1.LinodeVPC{}, + }, + }, + expectedError: fmt.Errorf("failed to init patch helper:"), + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().Return(runtime.NewScheme()) + }, + }, + } + for _, tt := range tests { + testcase := tt + t.Run(testcase.name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockK8sClient := mock.NewMockk8sClient(ctrl) + + testcase.expects(mockK8sClient) + + testcase.args.params.Client = mockK8sClient + + got, err := NewVPCScope(context.Background(), testcase.args.apiKey, testcase.args.params) + + if testcase.expectedError != nil { + assert.ErrorContains(t, err, testcase.expectedError.Error()) + } else { + assert.NotEmpty(t, got) + } + }) + } +} + +func TestVPCScopeMethods(t *testing.T) { + t.Parallel() + tests := []struct { + name string + LinodeVPC *infrav1alpha1.LinodeVPC + expects func(mock *mock.Mockk8sClient) + }{ + { + name: "Success - finalizer should be added to the Linode VPC object", + LinodeVPC: &infrav1alpha1.LinodeVPC{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-vpc", + }, + }, + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }).Times(2) + mock.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + }, + }, + { + name: "AddFinalizer error - finalizer should not be added to the Linode VPC object. Function returns nil since it was already present", + LinodeVPC: &infrav1alpha1.LinodeVPC{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-vpc", + Finalizers: []string{infrav1alpha1.GroupVersion.String()}, + }, + }, + expects: func(mock *mock.Mockk8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha1.AddToScheme(s) + return s + }).Times(1) + }, + }, + } + for _, tt := range tests { + testcase := tt + t.Run(testcase.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockK8sClient := mock.NewMockk8sClient(ctrl) + + testcase.expects(mockK8sClient) + + vScope, err := NewVPCScope( + context.Background(), + "test-key", + VPCScopeParams{ + Client: mockK8sClient, + LinodeVPC: testcase.LinodeVPC, + }, + ) + if err != nil { + t.Errorf("NewVPCScope() error = %v", err) + } + + if err := vScope.AddFinalizer(context.Background()); err != nil { + t.Errorf("ClusterScope.AddFinalizer() error = %v", err) + } + + if vScope.LinodeVPC.Finalizers[0] != infrav1alpha1.GroupVersion.String() { + t.Errorf("Finalizer was not added") + } + }) + } +} diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index 8c05807cb..748aaf5ea 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -100,7 +100,6 @@ func (r *LinodeClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques Cluster: cluster, LinodeCluster: linodeCluster, }, - util.NewPatchHelper, ) if err != nil { diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go index 9b2d18d33..74fa36b6d 100644 --- a/controller/linodemachine_controller.go +++ b/controller/linodemachine_controller.go @@ -130,7 +130,6 @@ func (r *LinodeMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques LinodeCluster: &infrav1alpha1.LinodeCluster{}, LinodeMachine: linodeMachine, }, - util.NewPatchHelper, ) if err != nil { log.Error(err, "Failed to create machine scope") diff --git a/devbox.lock b/devbox.lock index 2daeffbe5..41de68583 100644 --- a/devbox.lock +++ b/devbox.lock @@ -101,23 +101,23 @@ } } }, - "golangci-lint@latest": { - "last_modified": "2024-02-26T19:46:43Z", - "resolved": "github:NixOS/nixpkgs/548a86b335d7ecd8b57ec617781f5e652ab0c38e#golangci-lint", + "golangci-lint@1.56.1": { + "last_modified": "2024-02-12T13:06:46Z", + "resolved": "github:NixOS/nixpkgs/2d627a2a704708673e56346fcb13d25344b8eaf3#golangci-lint", "source": "devbox-search", - "version": "1.56.2", + "version": "1.56.1", "systems": { "aarch64-darwin": { - "store_path": "/nix/store/fs44z0nysjiwl2sj2m9x70dbrx2lbyhh-golangci-lint-1.56.2" + "store_path": "/nix/store/4f7s164lywkq0nzv2r80sj7z0kjvw7vs-golangci-lint-1.56.1" }, "aarch64-linux": { - "store_path": "/nix/store/bs18c2bx56b6xnpf49wb5kzi3vynbqmx-golangci-lint-1.56.2" + "store_path": "/nix/store/dyv5cy8inj2f3m1ymh5jqisjrss53y7w-golangci-lint-1.56.1" }, "x86_64-darwin": { - "store_path": "/nix/store/1xhl4b3nk3npq70d651vg4bamfg8xyga-golangci-lint-1.56.2" + "store_path": "/nix/store/v57g47gzxgbwj6193n5zrhzrdrp53g1s-golangci-lint-1.56.1" }, "x86_64-linux": { - "store_path": "/nix/store/kc58bqdmjdc6mfilih5pywprs7r7lxrw-golangci-lint-1.56.2" + "store_path": "/nix/store/5w4vqr92y2rs826z10lcnkhlv61bcxny-golangci-lint-1.56.1" } } }, diff --git a/mock/client.go b/mock/client.go index 6895442fe..b67a6e022 100644 --- a/mock/client.go +++ b/mock/client.go @@ -18,7 +18,6 @@ import ( meta "k8s.io/apimachinery/pkg/api/meta" runtime "k8s.io/apimachinery/pkg/runtime" schema "k8s.io/apimachinery/pkg/runtime/schema" - patch "sigs.k8s.io/cluster-api/util/patch" client "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -345,45 +344,3 @@ func (mr *Mockk8sClientMockRecorder) Update(ctx, obj any, opts ...any) *gomock.C varargs := append([]any{ctx, obj}, opts...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Update", reflect.TypeOf((*Mockk8sClient)(nil).Update), varargs...) } - -// MockPatchHelper is a mock of PatchHelper interface. -type MockPatchHelper struct { - ctrl *gomock.Controller - recorder *MockPatchHelperMockRecorder -} - -// MockPatchHelperMockRecorder is the mock recorder for MockPatchHelper. -type MockPatchHelperMockRecorder struct { - mock *MockPatchHelper -} - -// NewMockPatchHelper creates a new mock instance. -func NewMockPatchHelper(ctrl *gomock.Controller) *MockPatchHelper { - mock := &MockPatchHelper{ctrl: ctrl} - mock.recorder = &MockPatchHelperMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockPatchHelper) EXPECT() *MockPatchHelperMockRecorder { - return m.recorder -} - -// Patch mocks base method. -func (m *MockPatchHelper) Patch(ctx context.Context, obj client.Object, opts ...patch.Option) error { - m.ctrl.T.Helper() - varargs := []any{ctx, obj} - for _, a := range opts { - varargs = append(varargs, a) - } - ret := m.ctrl.Call(m, "Patch", varargs...) - ret0, _ := ret[0].(error) - return ret0 -} - -// Patch indicates an expected call of Patch. -func (mr *MockPatchHelperMockRecorder) Patch(ctx, obj any, opts ...any) *gomock.Call { - mr.mock.ctrl.T.Helper() - varargs := append([]any{ctx, obj}, opts...) - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Patch", reflect.TypeOf((*MockPatchHelper)(nil).Patch), varargs...) -} diff --git a/util/helpers.go b/util/helpers.go index 4af6c04de..7836f7ed0 100644 --- a/util/helpers.go +++ b/util/helpers.go @@ -2,8 +2,6 @@ package util import ( "github.com/linode/linodego" - "sigs.k8s.io/cluster-api/util/patch" - "sigs.k8s.io/controller-runtime/pkg/client" ) // Pointer returns the pointer of any type @@ -11,10 +9,6 @@ func Pointer[T any](t T) *T { return &t } -func NewPatchHelper(obj client.Object, crClient client.Client) (*patch.Helper, error) { - return patch.NewHelper(obj, crClient) -} - // IgnoreLinodeAPIError returns the error except matches to status code func IgnoreLinodeAPIError(err error, code int) error { apiErr := linodego.Error{Code: code}