From d6b808a0d96d12b90d43c11a6703e0d20ddbb1a9 Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Fri, 6 Sep 2024 16:23:06 -0400 Subject: [PATCH 1/9] updating scope for controllers --- clients/clients.go | 5 ++ cloud/scope/cluster.go | 30 ++++++----- cloud/scope/cluster_test.go | 36 ++++++------- cloud/scope/firewall.go | 20 ++++---- cloud/scope/machine.go | 23 +++++++++ cloud/scope/placement_group.go | 21 ++++---- cloud/scope/vpc.go | 20 ++++---- controller/linodecluster_controller.go | 7 +++ controller/linodefirewall_controller.go | 7 +++ controller/linodemachine_controller.go | 12 +++++ controller/linodeplacementgroup_controller.go | 8 +++ controller/linodevpc_controller.go | 8 +++ mock/client.go | 51 +++++++++++++++++++ 13 files changed, 186 insertions(+), 62 deletions(-) diff --git a/clients/clients.go b/clients/clients.go index bba1801c1..fbf4086a0 100644 --- a/clients/clients.go +++ b/clients/clients.go @@ -19,6 +19,7 @@ type LinodeClient interface { LinodeDNSClient LinodePlacementGroupClient LinodeFirewallClient + LinodeTokenClient } type AkamClient interface { @@ -118,3 +119,7 @@ type LinodeFirewallClient interface { type K8sClient interface { client.Client } + +type LinodeTokenClient interface { + SetToken(token string) *linodego.Client +} diff --git a/cloud/scope/cluster.go b/cloud/scope/cluster.go index b9ccb1c78..b13580d54 100644 --- a/cloud/scope/cluster.go +++ b/cloud/scope/cluster.go @@ -55,21 +55,8 @@ func NewClusterScope(ctx context.Context, linodeClientConfig, dnsClientConfig Cl if err := validateClusterScopeParams(params); err != nil { return nil, err } + //Or just don't do anything, create the linodeClient first using controller credentials and later just replace - // Override the controller credentials with ones from the Cluster's Secret reference (if supplied). - if params.LinodeCluster.Spec.CredentialsRef != nil { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, params.Client, *params.LinodeCluster.Spec.CredentialsRef, params.LinodeCluster.GetNamespace(), "apiToken") - if err != nil { - return nil, fmt.Errorf("credentials from secret ref: %w", err) - } - linodeClientConfig.Token = string(apiToken) - dnsToken, err := getCredentialDataFromRef(ctx, params.Client, *params.LinodeCluster.Spec.CredentialsRef, params.LinodeCluster.GetNamespace(), "dnsToken") - if err != nil || len(dnsToken) == 0 { - dnsToken = apiToken - } - dnsClientConfig.Token = string(dnsToken) - } linodeClient, err := CreateLinodeClient(linodeClientConfig) if err != nil { return nil, fmt.Errorf("failed to create linode client: %w", err) @@ -152,3 +139,18 @@ func (s *ClusterScope) RemoveCredentialsRefFinalizer(ctx context.Context) error *s.LinodeCluster.Spec.CredentialsRef, s.LinodeCluster.GetNamespace(), toFinalizer(s.LinodeCluster)) } + +func (s *ClusterScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { + + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeCluster.Spec.CredentialsRef, s.LinodeCluster.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + dnsToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeCluster.Spec.CredentialsRef, s.LinodeCluster.GetNamespace(), "dnsToken") + if err != nil || len(dnsToken) == 0 { + dnsToken = apiToken + } + s.LinodeDomainsClient = s.LinodeDomainsClient.SetToken(string(dnsToken)) + return nil +} diff --git a/cloud/scope/cluster_test.go b/cloud/scope/cluster_test.go index 9e969ff57..cf077f381 100644 --- a/cloud/scope/cluster_test.go +++ b/cloud/scope/cluster_test.go @@ -239,24 +239,24 @@ func TestNewClusterScope(t *testing.T) { infrav1alpha2.AddToScheme(s) return s }).AnyTimes() - 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 - }) - 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{ - "dnsToken": []byte("example"), - }, - } - *obj = cred - return nil - }) + // 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 + // }) + // 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{ + // "dnsToken": []byte("example"), + // }, + // } + // *obj = cred + // return nil + // }) }, }, { diff --git a/cloud/scope/firewall.go b/cloud/scope/firewall.go index 30dede521..67dcffced 100644 --- a/cloud/scope/firewall.go +++ b/cloud/scope/firewall.go @@ -57,16 +57,6 @@ func NewFirewallScope(ctx context.Context, linodeClientConfig ClientConfig, para if err := validateFirewallScopeParams(params); err != nil { return nil, err } - - // Override the controller credentials with ones from the Firewall's Secret reference (if supplied). - if params.LinodeFirewall.Spec.CredentialsRef != nil { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, params.Client, *params.LinodeFirewall.Spec.CredentialsRef, params.LinodeFirewall.GetNamespace(), "apiToken") - if err != nil { - return nil, fmt.Errorf("credentials from secret ref: %w", err) - } - linodeClientConfig.Token = string(apiToken) - } linodeClient, err := CreateLinodeClient(linodeClientConfig, WithRetryCount(0), ) @@ -126,3 +116,13 @@ func (s *FirewallScope) RemoveCredentialsRefFinalizer(ctx context.Context) error *s.LinodeFirewall.Spec.CredentialsRef, s.LinodeFirewall.GetNamespace(), toFinalizer(s.LinodeFirewall)) } + +func (s *FirewallScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeFirewall.Spec.CredentialsRef, s.LinodeFirewall.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil +} diff --git a/cloud/scope/machine.go b/cloud/scope/machine.go index a6d82133d..5cf1b3916 100644 --- a/cloud/scope/machine.go +++ b/cloud/scope/machine.go @@ -76,6 +76,7 @@ func NewMachineScope(ctx context.Context, linodeClientConfig ClientConfig, param // Use default (controller) credentials } + // Use default (controller) credentials to instantiate the Linode Client if credentialRef != nil { // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. apiToken, err := getCredentialDataFromRef(ctx, params.Client, *credentialRef, defaultNamespace, "apiToken") @@ -180,3 +181,25 @@ func (s *MachineScope) RemoveCredentialsRefFinalizer(ctx context.Context) error *s.LinodeMachine.Spec.CredentialsRef, s.LinodeMachine.GetNamespace(), toFinalizer(s.LinodeMachine)) } + +func (s *MachineScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { + var ( + credentialRef *corev1.SecretReference + defaultNamespace string + ) + switch { + case s.LinodeMachine.Spec.CredentialsRef != nil: + credentialRef = s.LinodeMachine.Spec.CredentialsRef + defaultNamespace = s.LinodeMachine.GetNamespace() + default: + credentialRef = s.LinodeCluster.Spec.CredentialsRef + defaultNamespace = s.LinodeCluster.GetNamespace() + } + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *credentialRef, defaultNamespace, "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil +} diff --git a/cloud/scope/placement_group.go b/cloud/scope/placement_group.go index 79c9a83f5..157d7343e 100644 --- a/cloud/scope/placement_group.go +++ b/cloud/scope/placement_group.go @@ -100,16 +100,6 @@ func NewPlacementGroupScope(ctx context.Context, linodeClientConfig ClientConfig if err := validatePlacementGroupScope(params); err != nil { return nil, err } - - // Override the controller credentials with ones from the Placement Groups's Secret reference (if supplied). - if params.LinodePlacementGroup.Spec.CredentialsRef != nil { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, params.Client, *params.LinodePlacementGroup.Spec.CredentialsRef, params.LinodePlacementGroup.GetNamespace(), "apiToken") - if err != nil { - return nil, fmt.Errorf("credentials from secret ref: %w", err) - } - linodeClientConfig.Token = string(apiToken) - } linodeClient, err := CreateLinodeClient( linodeClientConfig, WithRetryCount(0), @@ -130,3 +120,14 @@ func NewPlacementGroupScope(ctx context.Context, linodeClientConfig ClientConfig PatchHelper: helper, }, nil } + +func (s *PlacementGroupScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodePlacementGroup.Spec.CredentialsRef, s.LinodePlacementGroup.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil + +} diff --git a/cloud/scope/vpc.go b/cloud/scope/vpc.go index e87312495..65acb9b46 100644 --- a/cloud/scope/vpc.go +++ b/cloud/scope/vpc.go @@ -60,16 +60,6 @@ func NewVPCScope(ctx context.Context, linodeClientConfig ClientConfig, params VP if err := validateVPCScopeParams(params); err != nil { return nil, err } - - // Override the controller credentials with ones from the VPC's Secret reference (if supplied). - if params.LinodeVPC.Spec.CredentialsRef != nil { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, params.Client, *params.LinodeVPC.Spec.CredentialsRef, params.LinodeVPC.GetNamespace(), "apiToken") - if err != nil { - return nil, fmt.Errorf("credentials from secret ref: %w", err) - } - linodeClientConfig.Token = string(apiToken) - } linodeClient, err := CreateLinodeClient(linodeClientConfig, WithRetryCount(0), ) @@ -129,3 +119,13 @@ func (s *VPCScope) RemoveCredentialsRefFinalizer(ctx context.Context) error { *s.LinodeVPC.Spec.CredentialsRef, s.LinodeVPC.GetNamespace(), toFinalizer(s.LinodeVPC)) } + +func (s *VPCScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeVPC.Spec.CredentialsRef, s.LinodeVPC.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil +} diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index d3298f2ac..79332b7d5 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -156,6 +156,13 @@ func (r *LinodeClusterReconciler) reconcile( return res, err } + if clusterScope.LinodeCluster.Spec.CredentialsRef != nil { + if err := clusterScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err + } + } + // Create if clusterScope.LinodeCluster.Spec.ControlPlaneEndpoint.Host == "" { if err := r.reconcileCreate(ctx, logger, clusterScope); err != nil { diff --git a/controller/linodefirewall_controller.go b/controller/linodefirewall_controller.go index fe2da5097..f965150a5 100644 --- a/controller/linodefirewall_controller.go +++ b/controller/linodefirewall_controller.go @@ -142,6 +142,13 @@ func (r *LinodeFirewallReconciler) reconcile( return ctrl.Result{}, nil } + // Override the controller credentials with ones from the Firewall's Secret reference (if supplied). + if fwScope.LinodeFirewall.Spec.CredentialsRef != nil { + if err := fwScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return ctrl.Result{}, err + } + } action := "update" if fwScope.LinodeFirewall.Spec.FirewallID != nil { failureReason = infrav1alpha2.UpdateFirewallError diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go index 177d50b81..7858a66e8 100644 --- a/controller/linodemachine_controller.go +++ b/controller/linodemachine_controller.go @@ -197,6 +197,18 @@ func (r *LinodeMachineReconciler) reconcile(ctx context.Context, logger logr.Log return r.reconcileDelete(ctx, logger, machineScope) } + // Override the controller credentials with ones from the Machine's Secret reference (if supplied). + // Credentials will be used in the following order: + // 1. LinodeMachine + // 2. Owner LinodeCluster + // 3. Controller + if machineScope.LinodeMachine.Spec.CredentialsRef != nil || machineScope.LinodeCluster.Spec.CredentialsRef != nil { + if err := machineScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return ctrl.Result{}, err + } + } + // Make sure bootstrap data is available and populated. if machineScope.Machine.Spec.Bootstrap.DataSecretName == nil { logger.Info("Bootstrap data secret is not yet available") diff --git a/controller/linodeplacementgroup_controller.go b/controller/linodeplacementgroup_controller.go index 61079cf12..427763c4e 100644 --- a/controller/linodeplacementgroup_controller.go +++ b/controller/linodeplacementgroup_controller.go @@ -152,6 +152,14 @@ func (r *LinodePlacementGroupReconciler) reconcile( return } + // Override the controller credentials with ones from the Placement Groups's Secret reference (if supplied). + if pgScope.LinodePlacementGroup.Spec.CredentialsRef != nil { + if err := pgScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err + } + } + // Update if pgScope.LinodePlacementGroup.Spec.PGID != nil { logger = logger.WithValues("pgID", *pgScope.LinodePlacementGroup.Spec.PGID) diff --git a/controller/linodevpc_controller.go b/controller/linodevpc_controller.go index c504c4051..3a69bce51 100644 --- a/controller/linodevpc_controller.go +++ b/controller/linodevpc_controller.go @@ -158,6 +158,14 @@ func (r *LinodeVPCReconciler) reconcile( return } + // Override the controller credentials with ones from the VPC's Secret reference (if supplied). + if vpcScope.LinodeVPC.Spec.CredentialsRef != nil { + if err := vpcScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err + } + } + // Update if vpcScope.LinodeVPC.Spec.VPCID != nil { failureReason = infrav1alpha2.UpdateVPCError diff --git a/mock/client.go b/mock/client.go index 7d0127be9..011b17312 100644 --- a/mock/client.go +++ b/mock/client.go @@ -754,6 +754,20 @@ func (mr *MockLinodeClientMockRecorder) ResizeInstanceDisk(ctx, linodeID, diskID return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResizeInstanceDisk", reflect.TypeOf((*MockLinodeClient)(nil).ResizeInstanceDisk), ctx, linodeID, diskID, size) } +// SetToken mocks base method. +func (m *MockLinodeClient) SetToken(token string) *linodego.Client { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetToken", token) + ret0, _ := ret[0].(*linodego.Client) + return ret0 +} + +// SetToken indicates an expected call of SetToken. +func (mr *MockLinodeClientMockRecorder) SetToken(token any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetToken", reflect.TypeOf((*MockLinodeClient)(nil).SetToken), token) +} + // UnassignPlacementGroupLinodes mocks base method. func (m *MockLinodeClient) UnassignPlacementGroupLinodes(ctx context.Context, id int, options linodego.PlacementGroupUnAssignOptions) (*linodego.PlacementGroup, error) { m.ctrl.T.Helper() @@ -2263,3 +2277,40 @@ 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...) } + +// MockLinodeTokenClient is a mock of LinodeTokenClient interface. +type MockLinodeTokenClient struct { + ctrl *gomock.Controller + recorder *MockLinodeTokenClientMockRecorder +} + +// MockLinodeTokenClientMockRecorder is the mock recorder for MockLinodeTokenClient. +type MockLinodeTokenClientMockRecorder struct { + mock *MockLinodeTokenClient +} + +// NewMockLinodeTokenClient creates a new mock instance. +func NewMockLinodeTokenClient(ctrl *gomock.Controller) *MockLinodeTokenClient { + mock := &MockLinodeTokenClient{ctrl: ctrl} + mock.recorder = &MockLinodeTokenClientMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockLinodeTokenClient) EXPECT() *MockLinodeTokenClientMockRecorder { + return m.recorder +} + +// SetToken mocks base method. +func (m *MockLinodeTokenClient) SetToken(token string) *linodego.Client { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetToken", token) + ret0, _ := ret[0].(*linodego.Client) + return ret0 +} + +// SetToken indicates an expected call of SetToken. +func (mr *MockLinodeTokenClientMockRecorder) SetToken(token any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetToken", reflect.TypeOf((*MockLinodeTokenClient)(nil).SetToken), token) +} From 2e8181a39687a4ef99c6f0c311c55b9356f25585 Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Mon, 9 Sep 2024 11:44:36 -0400 Subject: [PATCH 2/9] not mandating adding credential ref in Spec for LinodeMachine templates --- templates/infra/linodeMachineTemplate.yaml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/templates/infra/linodeMachineTemplate.yaml b/templates/infra/linodeMachineTemplate.yaml index bfcebe324..586c37293 100644 --- a/templates/infra/linodeMachineTemplate.yaml +++ b/templates/infra/linodeMachineTemplate.yaml @@ -6,8 +6,6 @@ metadata: spec: template: spec: - credentialsRef: - name: ${CLUSTER_NAME}-credentials image: ${LINODE_OS:="linode/ubuntu22.04"} type: ${LINODE_CONTROL_PLANE_MACHINE_TYPE} region: ${LINODE_REGION} @@ -29,8 +27,6 @@ metadata: spec: template: spec: - credentialsRef: - name: ${CLUSTER_NAME}-credentials image: ${LINODE_OS:="linode/ubuntu22.04"} type: ${LINODE_MACHINE_TYPE} region: ${LINODE_REGION} From 75c75d2393f3b2fd5cbecf89b3f593563aaa0b59 Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Mon, 9 Sep 2024 11:45:03 -0400 Subject: [PATCH 3/9] adding testcases in vpc scope testing --- cloud/scope/vpc_test.go | 129 +++++++++++++++++++++++----------------- 1 file changed, 76 insertions(+), 53 deletions(-) diff --git a/cloud/scope/vpc_test.go b/cloud/scope/vpc_test.go index 02e79d4a5..590b7d8ca 100644 --- a/cloud/scope/vpc_test.go +++ b/cloud/scope/vpc_test.go @@ -94,39 +94,6 @@ func TestNewVPCScope(t *testing.T) { }) }, }, - { - name: "Success - Validate getCredentialDataFromRef() returns some apiKey data and we create a valid ClusterScope", - args: args{ - apiKey: "test-key", - params: VPCScopeParams{ - LinodeVPC: &infrav1alpha2.LinodeVPC{ - Spec: infrav1alpha2.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() - infrav1alpha2.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{ @@ -136,26 +103,6 @@ func TestNewVPCScope(t *testing.T) { 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: &infrav1alpha2.LinodeVPC{ - Spec: infrav1alpha2.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{ @@ -459,3 +406,79 @@ func TestVPCRemoveCredentialsRefFinalizer(t *testing.T) { }) } } + +func TestSetCredentialRefTokenForLinodeClients(t *testing.T) { + t.Parallel() + tests := []struct { + name string + LinodeVPC *infrav1alpha2.LinodeVPC + expects func(mock *mock.MockK8sClient) + expectedError error + }{ + { + name: "Success - Validate getCredentialDataFromRef() returns some apiKey data and we create a valid ClusterScope", + LinodeVPC: &infrav1alpha2.LinodeVPC{ + Spec: infrav1alpha2.LinodeVPCSpec{ + CredentialsRef: &corev1.SecretReference{ + Namespace: "test-namespace", + Name: "test-name", + }, + }, + }, + expectedError: 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{ + "apiToken": []byte("example-api-token"), + }, + } + *obj = cred + return nil + }) + }, + }, + { + name: "Error - error when getting the credentials secret", + LinodeVPC: &infrav1alpha2.LinodeVPC{ + Spec: infrav1alpha2.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"), + }, + } + 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(), + ClientConfig{Token: "test-key"}, + VPCScopeParams{ + Client: mockK8sClient, + LinodeVPC: testcase.LinodeVPC, + }, + ) + if err != nil { + t.Errorf("NewVPCScope() error = %v", err) + } + if err := vScope.SetCredentialRefTokenForLinodeClients(context.Background()); err != nil { + t.Errorf("%v", err) + } + }) + } +} From beb3875c772c942bc1358caf59218d526d743f39 Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Mon, 9 Sep 2024 12:08:39 -0400 Subject: [PATCH 4/9] adding testcases in pg scope testing --- cloud/scope/placement_group_test.go | 128 ++++++++++++++++------------ cloud/scope/vpc_test.go | 2 +- 2 files changed, 76 insertions(+), 54 deletions(-) diff --git a/cloud/scope/placement_group_test.go b/cloud/scope/placement_group_test.go index dc5e7e1a8..aeeadb88d 100644 --- a/cloud/scope/placement_group_test.go +++ b/cloud/scope/placement_group_test.go @@ -94,39 +94,6 @@ func TestNewPlacementGroupScope(t *testing.T) { }) }, }, - { - name: "Success - Validate getCredentialDataFromRef() returns some apiKey data and we create a valid PlacementGroupScope", - args: args{ - apiKey: "test-key", - params: PlacementGroupScopeParams{ - LinodePlacementGroup: &infrav1alpha2.LinodePlacementGroup{ - Spec: infrav1alpha2.LinodePlacementGroupSpec{ - 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() - infrav1alpha2.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{ @@ -136,26 +103,6 @@ func TestNewPlacementGroupScope(t *testing.T) { expects: func(mock *mock.MockK8sClient) {}, expectedError: fmt.Errorf("linodePlacementGroup is required when creating a PlacementGroupScope"), }, - { - name: "Error - Pass in valid args but get an error when getting the credentials secret", - args: args{ - apiKey: "test-key", - params: PlacementGroupScopeParams{ - LinodePlacementGroup: &infrav1alpha2.LinodePlacementGroup{ - Spec: infrav1alpha2.LinodePlacementGroupSpec{ - 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{ @@ -459,3 +406,78 @@ func TestPlacementGroupRemoveCredentialsRefFinalizer(t *testing.T) { }) } } +func TestPlacementGroupSetCredentialRefTokenForLinodeClients(t *testing.T) { + t.Parallel() + tests := []struct { + name string + LinodePlacementGroup *infrav1alpha2.LinodePlacementGroup + expects func(mock *mock.MockK8sClient) + expectedError error + }{ + { + name: "Success - Validate getCredentialDataFromRef() returns some apiKey data", + LinodePlacementGroup: &infrav1alpha2.LinodePlacementGroup{ + Spec: infrav1alpha2.LinodePlacementGroupSpec{ + CredentialsRef: &corev1.SecretReference{ + Namespace: "test-namespace", + Name: "test-name", + }, + }, + }, + expectedError: 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{ + "apiToken": []byte("example-api-token"), + }, + } + *obj = cred + return nil + }) + }, + }, + { + name: "Error - Pass in valid args but get an error when getting the credentials secret", + LinodePlacementGroup: &infrav1alpha2.LinodePlacementGroup{ + Spec: infrav1alpha2.LinodePlacementGroupSpec{ + 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"), + }, + } + 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) + + pgScope, err := NewPlacementGroupScope( + context.Background(), + ClientConfig{Token: "test-key"}, + PlacementGroupScopeParams{ + Client: mockK8sClient, + LinodePlacementGroup: testcase.LinodePlacementGroup, + }, + ) + if err != nil { + t.Errorf("NewPGScope() error = %v", err) + } + if err := pgScope.SetCredentialRefTokenForLinodeClients(context.Background()); err != nil { + t.Errorf("%v", err) + } + }) + } +} diff --git a/cloud/scope/vpc_test.go b/cloud/scope/vpc_test.go index 590b7d8ca..790fe6c5f 100644 --- a/cloud/scope/vpc_test.go +++ b/cloud/scope/vpc_test.go @@ -407,7 +407,7 @@ func TestVPCRemoveCredentialsRefFinalizer(t *testing.T) { } } -func TestSetCredentialRefTokenForLinodeClients(t *testing.T) { +func TestVPCSetCredentialRefTokenForLinodeClients(t *testing.T) { t.Parallel() tests := []struct { name string From 3202a2141a3c92948542722cff240b4aa3a9b420 Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Mon, 9 Sep 2024 14:08:00 -0400 Subject: [PATCH 5/9] adding testcases for the new scope method for all resources --- cloud/scope/cluster.go | 2 - cloud/scope/cluster_test.go | 183 +++++++++++------- cloud/scope/firewall_test.go | 152 +++++++++------ cloud/scope/object_storage_key.go | 21 +- cloud/scope/object_storage_key_test.go | 162 ++++++++++------ cloud/scope/placement_group_test.go | 18 +- cloud/scope/vpc_test.go | 16 +- .../linodeobjectstoragekey_controller.go | 8 + 8 files changed, 363 insertions(+), 199 deletions(-) diff --git a/cloud/scope/cluster.go b/cloud/scope/cluster.go index b13580d54..a82ae1c9b 100644 --- a/cloud/scope/cluster.go +++ b/cloud/scope/cluster.go @@ -55,7 +55,6 @@ func NewClusterScope(ctx context.Context, linodeClientConfig, dnsClientConfig Cl if err := validateClusterScopeParams(params); err != nil { return nil, err } - //Or just don't do anything, create the linodeClient first using controller credentials and later just replace linodeClient, err := CreateLinodeClient(linodeClientConfig) if err != nil { @@ -141,7 +140,6 @@ func (s *ClusterScope) RemoveCredentialsRefFinalizer(ctx context.Context) error } func (s *ClusterScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { - apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeCluster.Spec.CredentialsRef, s.LinodeCluster.GetNamespace(), "apiToken") if err != nil { return fmt.Errorf("credentials from secret ref: %w", err) diff --git a/cloud/scope/cluster_test.go b/cloud/scope/cluster_test.go index cf077f381..72f0fb531 100644 --- a/cloud/scope/cluster_test.go +++ b/cloud/scope/cluster_test.go @@ -214,51 +214,6 @@ func TestNewClusterScope(t *testing.T) { }) }, }, - { - name: "Success - Validate getCredentialDataFromRef() returns some apiKey data and we create a valid ClusterScope", - args: args{ - apiKey: "test-key", - dnsApiKey: "test-key", - params: ClusterScopeParams{ - Client: nil, - Cluster: &clusterv1.Cluster{}, - LinodeCluster: &infrav1alpha2.LinodeCluster{ - Spec: infrav1alpha2.LinodeClusterSpec{ - CredentialsRef: &corev1.SecretReference{ - Name: "example", - Namespace: "test", - }, - }, - }, - }, - }, - expectedError: nil, - expects: func(mock *mock.MockK8sClient) { - mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { - s := runtime.NewScheme() - infrav1alpha2.AddToScheme(s) - return s - }).AnyTimes() - // 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 - // }) - // 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{ - // "dnsToken": []byte("example"), - // }, - // } - // *obj = cred - // return nil - // }) - }, - }, { name: "Error - ValidateClusterScopeParams triggers error because ClusterScopeParams is empty", args: args{ @@ -283,29 +238,6 @@ func TestNewClusterScope(t *testing.T) { mock.EXPECT().Scheme().Return(runtime.NewScheme()) }, }, - { - name: "Error - Using getCredentialDataFromRef(), func returns an error. Unable to create a valid ClusterScope", - args: args{ - apiKey: "test-key", - dnsApiKey: "test-key", - params: ClusterScopeParams{ - Client: nil, - Cluster: &clusterv1.Cluster{}, - LinodeCluster: &infrav1alpha2.LinodeCluster{ - Spec: infrav1alpha2.LinodeClusterSpec{ - CredentialsRef: &corev1.SecretReference{ - Name: "example", - Namespace: "test", - }, - }, - }, - }, - }, - expectedError: fmt.Errorf("credentials from secret ref: get credentials secret test/example: failed to get secret"), - expects: func(mock *mock.MockK8sClient) { - mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to get secret")) - }, - }, { name: "Error - createLinodeCluster throws an error for passing empty apiKey. Unable to create a valid ClusterScope", args: args{ @@ -451,3 +383,118 @@ func TestRemoveCredentialsRefFinalizer(t *testing.T) { }) } } + +func TestClusterSetCredentialRefTokenForLinodeClients(t *testing.T) { + t.Parallel() + type fields struct { + Cluster *clusterv1.Cluster + LinodeCluster *infrav1alpha2.LinodeCluster + LinodeMachineList infrav1alpha2.LinodeMachineList + } + + tests := []struct { + name string + fields fields + expects func(mock *mock.MockK8sClient) + expectedError error + }{ + { + name: "Success - Validate getCredentialDataFromRef() returns some apiKey data", + fields: fields{ + Cluster: &clusterv1.Cluster{}, + LinodeMachineList: infrav1alpha2.LinodeMachineList{}, + LinodeCluster: &infrav1alpha2.LinodeCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: infrav1alpha2.LinodeClusterSpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + }, + expectedError: nil, + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.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{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example", + Namespace: "test", + }, + Data: map[string][]byte{ + "apiToken": []byte("example"), + }, + } + *obj = cred + + return nil + }).AnyTimes() + }, + }, + { + name: "Error - error when getting the credentials secret", + fields: fields{ + Cluster: &clusterv1.Cluster{}, + LinodeMachineList: infrav1alpha2.LinodeMachineList{}, + LinodeCluster: &infrav1alpha2.LinodeCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: infrav1alpha2.LinodeClusterSpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + }, + expectedError: fmt.Errorf("credentials from secret ref: get credentials secret test/example: test error"), + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }) + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("test error")) + }, + }, + } + 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) + + cScope, err := NewClusterScope( + context.Background(), + ClientConfig{Token: "test-key"}, + ClientConfig{Token: "test-key"}, + ClusterScopeParams{ + Cluster: testcase.fields.Cluster, + LinodeCluster: testcase.fields.LinodeCluster, + LinodeMachineList: testcase.fields.LinodeMachineList, + Client: mockK8sClient, + }) + if err != nil { + t.Errorf("NewClusterScope() error = %v", err) + } + + if err := cScope.SetCredentialRefTokenForLinodeClients(context.Background()); err != nil { + assert.ErrorContains(t, err, testcase.expectedError.Error()) + } + }) + } +} diff --git a/cloud/scope/firewall_test.go b/cloud/scope/firewall_test.go index c201db6fb..12031330b 100644 --- a/cloud/scope/firewall_test.go +++ b/cloud/scope/firewall_test.go @@ -94,39 +94,6 @@ func TestNewFirewallScope(t *testing.T) { }) }, }, - { - name: "Success - Validate getCredentialDataFromRef() returns some apiKey data and we create a valid FirewallScope", - args: args{ - apiKey: "test-key", - params: FirewallScopeParams{ - LinodeFirewall: &infrav1alpha2.LinodeFirewall{ - Spec: infrav1alpha2.LinodeFirewallSpec{ - 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() - infrav1alpha2.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{ @@ -136,26 +103,6 @@ func TestNewFirewallScope(t *testing.T) { expects: func(mock *mock.MockK8sClient) {}, expectedError: fmt.Errorf("linodeFirewall is required when creating a FirewallScope"), }, - { - name: "Error - Pass in valid args but get an error when getting the credentials secret", - args: args{ - apiKey: "test-key", - params: FirewallScopeParams{ - LinodeFirewall: &infrav1alpha2.LinodeFirewall{ - Spec: infrav1alpha2.LinodeFirewallSpec{ - 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{ @@ -319,7 +266,7 @@ func TestFirewallAddCredentialsRefFinalizer(t *testing.T) { *obj = cred return nil - }).Times(2) + }).Times(1) mock.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil) }, }, @@ -459,3 +406,100 @@ func TestFirewallRemoveCredentialsRefFinalizer(t *testing.T) { }) } } +func TestFirewallSetCredentialRefTokenForLinodeClients(t *testing.T) { + t.Parallel() + tests := []struct { + name string + LinodeFirewall *infrav1alpha2.LinodeFirewall + expects func(mock *mock.MockK8sClient) + expectedError error + }{ + { + name: "Success - Set credential Ref finalizer ", + LinodeFirewall: &infrav1alpha2.LinodeFirewall{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-fw", + }, + Spec: infrav1alpha2.LinodeFirewallSpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + expectedError: nil, + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.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{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example", + Namespace: "test", + }, + Data: map[string][]byte{ + "apiToken": []byte("example"), + }, + } + *obj = cred + return nil + }).AnyTimes() + }, + }, + { + name: "Error - Get an error when getting the credentials secret", + LinodeFirewall: &infrav1alpha2.LinodeFirewall{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-fw", + }, + Spec: infrav1alpha2.LinodeFirewallSpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }) + 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/example: test error"), + }, + } + 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) + + pgScope, err := NewFirewallScope( + context.Background(), + ClientConfig{Token: "test-key"}, + FirewallScopeParams{ + Client: mockK8sClient, + LinodeFirewall: testcase.LinodeFirewall, + }, + ) + if err != nil { + t.Errorf("NewFirewallScope() error = %v", err) + } + + if err := pgScope.SetCredentialRefTokenForLinodeClients(context.Background()); err != nil { + assert.ErrorContains(t, err, testcase.expectedError.Error()) + } + }) + } +} diff --git a/cloud/scope/object_storage_key.go b/cloud/scope/object_storage_key.go index 7bbd248ed..e9642e2b6 100644 --- a/cloud/scope/object_storage_key.go +++ b/cloud/scope/object_storage_key.go @@ -49,16 +49,6 @@ func NewObjectStorageKeyScope(ctx context.Context, linodeClientConfig ClientConf if err := validateObjectStorageKeyScopeParams(params); err != nil { return nil, err } - - // Override the controller credentials with ones from the Cluster's Secret reference (if supplied). - if params.Key.Spec.CredentialsRef != nil { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, params.Client, *params.Key.Spec.CredentialsRef, params.Key.GetNamespace(), "apiToken") - if err != nil || len(apiToken) == 0 { - return nil, fmt.Errorf("credentials from secret ref: %w", err) - } - linodeClientConfig.Token = string(apiToken) - } linodeClientConfig.Timeout = clientTimeout linodeClient, err := CreateLinodeClient(linodeClientConfig) if err != nil { @@ -177,3 +167,14 @@ func (s *ObjectStorageKeyScope) ShouldRotateKey() bool { return s.Key.Status.LastKeyGeneration != nil && s.Key.Spec.KeyGeneration != *s.Key.Status.LastKeyGeneration } + +// Override the controller credentials with ones from the Cluster's Secret reference (if supplied). +func (s *ObjectStorageKeyScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.Key.Spec.CredentialsRef, s.Key.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil +} diff --git a/cloud/scope/object_storage_key_test.go b/cloud/scope/object_storage_key_test.go index 4f0a8a70b..a87b113c8 100644 --- a/cloud/scope/object_storage_key_test.go +++ b/cloud/scope/object_storage_key_test.go @@ -104,41 +104,6 @@ func TestNewObjectStorageKeyScope(t *testing.T) { }) }, }, - { - name: "with credentials from secret", - args: args{ - apiKey: "apikey", - params: ObjectStorageKeyScopeParams{ - Client: nil, - Key: &infrav1alpha2.LinodeObjectStorageKey{ - Spec: infrav1alpha2.LinodeObjectStorageKeySpec{ - CredentialsRef: &corev1.SecretReference{ - Name: "example", - Namespace: "test", - }, - }, - }, - Logger: &logr.Logger{}, - }, - }, - expectedErr: nil, - expects: func(k8s *mock.MockK8sClient) { - k8s.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { - s := runtime.NewScheme() - infrav1alpha2.AddToScheme(s) - return s - }) - k8s.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, name types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { - cred := corev1.Secret{ - Data: map[string][]byte{ - "apiToken": []byte("example"), - }, - } - *obj = cred - return nil - }) - }, - }, { name: "empty params", args: args{ @@ -163,28 +128,6 @@ func TestNewObjectStorageKeyScope(t *testing.T) { k8s.EXPECT().Scheme().Return(runtime.NewScheme()) }, }, - { - name: "credentials from ref fail", - args: args{ - apiKey: "apikey", - params: ObjectStorageKeyScopeParams{ - Client: nil, - Key: &infrav1alpha2.LinodeObjectStorageKey{ - Spec: infrav1alpha2.LinodeObjectStorageKeySpec{ - CredentialsRef: &corev1.SecretReference{ - Name: "example", - Namespace: "test", - }, - }, - }, - Logger: &logr.Logger{}, - }, - }, - expectedErr: fmt.Errorf("credentials from secret ref: get credentials secret test/example: failed to get secret"), - expects: func(mock *mock.MockK8sClient) { - mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to get secret")) - }, - }, { name: "empty apiKey", args: args{ @@ -224,7 +167,7 @@ func TestNewObjectStorageKeyScope(t *testing.T) { } } -func TestObjectStrorageKeyAddFinalizer(t *testing.T) { +func TestObjectStorageKeyAddFinalizer(t *testing.T) { t.Parallel() tests := []struct { @@ -639,3 +582,106 @@ func TestShouldRotateKey(t *testing.T) { }, }).ShouldRotateKey()) } +func TestObjectStorageKeySetCredentialRefTokenForLinodeClients(t *testing.T) { + t.Parallel() + + type args struct { + apiKey string + params ObjectStorageKeyScopeParams + } + tests := []struct { + name string + args args + expectedErr error + expects func(k8s *mock.MockK8sClient) + clientBuildFunc func(apiKey string) (LinodeClient, error) + }{ + { + name: "with credentials from secret", + args: args{ + apiKey: "apikey", + params: ObjectStorageKeyScopeParams{ + Client: nil, + Key: &infrav1alpha2.LinodeObjectStorageKey{ + Spec: infrav1alpha2.LinodeObjectStorageKeySpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + Logger: &logr.Logger{}, + }, + }, + expectedErr: nil, + expects: func(k8s *mock.MockK8sClient) { + k8s.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }) + k8s.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, name types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error { + cred := corev1.Secret{ + Data: map[string][]byte{ + "apiToken": []byte("example"), + }, + } + *obj = cred + return nil + }) + }, + }, + { + name: "credentials from ref fail", + args: args{ + apiKey: "apikey", + params: ObjectStorageKeyScopeParams{ + Client: nil, + Key: &infrav1alpha2.LinodeObjectStorageKey{ + Spec: infrav1alpha2.LinodeObjectStorageKeySpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + Logger: &logr.Logger{}, + }, + }, + expectedErr: fmt.Errorf("credentials from secret ref: get credentials secret test/example: failed to get secret"), + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }) + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to get secret")) + }, + }, + } + 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 + + kscope, err := NewObjectStorageKeyScope(context.Background(), ClientConfig{Token: testcase.args.apiKey}, testcase.args.params) + + if err != nil { + t.Errorf("NewObjectStorageKeyScope() error = %v", err) + } + + if err := kscope.SetCredentialRefTokenForLinodeClients(context.Background()); err != nil { + assert.ErrorContains(t, err, testcase.expectedErr.Error()) + } + }) + } +} diff --git a/cloud/scope/placement_group_test.go b/cloud/scope/placement_group_test.go index aeeadb88d..9f11e3ea1 100644 --- a/cloud/scope/placement_group_test.go +++ b/cloud/scope/placement_group_test.go @@ -266,7 +266,7 @@ func TestPlacementGroupAddCredentialsRefFinalizer(t *testing.T) { *obj = cred return nil - }).Times(2) + }).Times(1) mock.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil) }, }, @@ -356,7 +356,7 @@ func TestPlacementGroupRemoveCredentialsRefFinalizer(t *testing.T) { *obj = cred return nil - }).Times(2) + }).Times(1) mock.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil) }, }, @@ -426,6 +426,11 @@ func TestPlacementGroupSetCredentialRefTokenForLinodeClients(t *testing.T) { }, expectedError: nil, expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.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{ @@ -438,7 +443,7 @@ func TestPlacementGroupSetCredentialRefTokenForLinodeClients(t *testing.T) { }, }, { - name: "Error - Pass in valid args but get an error when getting the credentials secret", + name: "Error - Get an error when getting the credentials secret", LinodePlacementGroup: &infrav1alpha2.LinodePlacementGroup{ Spec: infrav1alpha2.LinodePlacementGroupSpec{ CredentialsRef: &corev1.SecretReference{ @@ -448,6 +453,11 @@ func TestPlacementGroupSetCredentialRefTokenForLinodeClients(t *testing.T) { }, }, expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }) 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"), @@ -476,7 +486,7 @@ func TestPlacementGroupSetCredentialRefTokenForLinodeClients(t *testing.T) { t.Errorf("NewPGScope() error = %v", err) } if err := pgScope.SetCredentialRefTokenForLinodeClients(context.Background()); err != nil { - t.Errorf("%v", err) + assert.ErrorContains(t, err, testcase.expectedError.Error()) } }) } diff --git a/cloud/scope/vpc_test.go b/cloud/scope/vpc_test.go index 790fe6c5f..ee78e0c95 100644 --- a/cloud/scope/vpc_test.go +++ b/cloud/scope/vpc_test.go @@ -266,7 +266,7 @@ func TestVPCAddCredentialsRefFinalizer(t *testing.T) { *obj = cred return nil - }).Times(2) + }).Times(1) mock.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil) }, }, @@ -356,7 +356,7 @@ func TestVPCRemoveCredentialsRefFinalizer(t *testing.T) { *obj = cred return nil - }).Times(2) + }).Times(1) mock.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil) }, }, @@ -427,6 +427,11 @@ func TestVPCSetCredentialRefTokenForLinodeClients(t *testing.T) { }, expectedError: nil, expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.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{ @@ -449,6 +454,11 @@ func TestVPCSetCredentialRefTokenForLinodeClients(t *testing.T) { }, }, expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }) 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"), @@ -477,7 +487,7 @@ func TestVPCSetCredentialRefTokenForLinodeClients(t *testing.T) { t.Errorf("NewVPCScope() error = %v", err) } if err := vScope.SetCredentialRefTokenForLinodeClients(context.Background()); err != nil { - t.Errorf("%v", err) + assert.ErrorContains(t, err, testcase.expectedError.Error()) } }) } diff --git a/controller/linodeobjectstoragekey_controller.go b/controller/linodeobjectstoragekey_controller.go index cccffa5d0..2e9beaf49 100644 --- a/controller/linodeobjectstoragekey_controller.go +++ b/controller/linodeobjectstoragekey_controller.go @@ -131,6 +131,14 @@ func (r *LinodeObjectStorageKeyReconciler) reconcile(ctx context.Context, keySco return res, err } + // Override the controller credentials with ones from the Key's Secret reference (if supplied). + if keyScope.Key.Spec.CredentialsRef != nil { + if err := keyScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + keyScope.Logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err + } + } + if err := r.reconcileApply(ctx, keyScope); err != nil { return res, err } From 6aaa21cb78320ba6b40d9193d05111637730991e Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Mon, 9 Sep 2024 16:50:17 -0400 Subject: [PATCH 6/9] updating machine scope and added testcases --- cloud/scope/machine.go | 31 ---- cloud/scope/machine_test.go | 281 ++++++++++++++++++++++++------------ 2 files changed, 189 insertions(+), 123 deletions(-) diff --git a/cloud/scope/machine.go b/cloud/scope/machine.go index 5cf1b3916..5efecc3a8 100644 --- a/cloud/scope/machine.go +++ b/cloud/scope/machine.go @@ -55,37 +55,6 @@ func NewMachineScope(ctx context.Context, linodeClientConfig ClientConfig, param if err := validateMachineScopeParams(params); err != nil { return nil, err } - - // Override the controller credentials with ones from the Machine's Secret reference (if supplied). - // Credentials will be used in the following order: - // 1. LinodeMachine - // 2. Owner LinodeCluster - // 3. Controller - var ( - credentialRef *corev1.SecretReference - defaultNamespace string - ) - switch { - case params.LinodeMachine.Spec.CredentialsRef != nil: - credentialRef = params.LinodeMachine.Spec.CredentialsRef - defaultNamespace = params.LinodeMachine.GetNamespace() - case params.LinodeCluster.Spec.CredentialsRef != nil: - credentialRef = params.LinodeCluster.Spec.CredentialsRef - defaultNamespace = params.LinodeCluster.GetNamespace() - default: - // Use default (controller) credentials - } - - // Use default (controller) credentials to instantiate the Linode Client - if credentialRef != nil { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, params.Client, *credentialRef, defaultNamespace, "apiToken") - if err != nil { - return nil, fmt.Errorf("credentials from secret ref: %w", err) - } - linodeClientConfig.Token = string(apiToken) - } - linodeClient, err := CreateLinodeClient(linodeClientConfig, WithRetryCount(0), ) diff --git a/cloud/scope/machine_test.go b/cloud/scope/machine_test.go index 4241c982d..b8b1b6415 100644 --- a/cloud/scope/machine_test.go +++ b/cloud/scope/machine_test.go @@ -3,6 +3,7 @@ package scope import ( "context" "errors" + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -202,58 +203,38 @@ func TestNewMachineScope(t *testing.T) { NewSuite(t, mock.MockK8sClient{}).Run( OneOf( - Path(Result("invalid params", func(ctx context.Context, mck Mock) { - mScope, err := NewMachineScope( - ctx, - ClientConfig{Token: "apiToken"}, - MachineScopeParams{}, - ) - require.ErrorContains(t, err, "is required") - assert.Nil(t, mScope) - })), - Path(Result("no token", func(ctx context.Context, mck Mock) { - mScope, err := NewMachineScope(ctx, ClientConfig{Token: ""}, MachineScopeParams{ - Client: mck.K8sClient, - Cluster: &clusterv1.Cluster{}, - Machine: &clusterv1.Machine{}, - LinodeCluster: &infrav1alpha2.LinodeCluster{}, - LinodeMachine: &infrav1alpha2.LinodeMachine{}, - }) - require.ErrorContains(t, err, "failed to create linode client") - assert.Nil(t, mScope) - })), Path( - Call("no secret", func(ctx context.Context, mck Mock) { - mck.K8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(apierrors.NewNotFound(schema.GroupResource{}, "example")) - }), - Result("error", func(ctx context.Context, mck Mock) { + Result("invalid params", func(ctx context.Context, mck Mock) { + mScope, err := NewMachineScope( + ctx, + ClientConfig{Token: "apiToken"}, + MachineScopeParams{}, + ) + require.ErrorContains(t, err, "is required") + assert.Nil(t, mScope) + })), + Path( + Result("no token", func(ctx context.Context, mck Mock) { mScope, err := NewMachineScope(ctx, ClientConfig{Token: ""}, MachineScopeParams{ Client: mck.K8sClient, Cluster: &clusterv1.Cluster{}, Machine: &clusterv1.Machine{}, LinodeCluster: &infrav1alpha2.LinodeCluster{}, - LinodeMachine: &infrav1alpha2.LinodeMachine{ - Spec: infrav1alpha2.LinodeMachineSpec{ - CredentialsRef: &corev1.SecretReference{ - Name: "example", - Namespace: "test", - }, - }, - }, + LinodeMachine: &infrav1alpha2.LinodeMachine{}, }) - require.ErrorContains(t, err, "credentials from secret ref") + require.ErrorContains(t, err, "failed to create linode client") assert.Nil(t, mScope) - }), - ), + })), ), OneOf( - Path(Call("valid scheme", func(ctx context.Context, mck Mock) { - mck.K8sClient.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { - s := runtime.NewScheme() - infrav1alpha2.AddToScheme(s) - return s - }).AnyTimes() - })), + Path( + Call("valid scheme", func(ctx context.Context, mck Mock) { + mck.K8sClient.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }).AnyTimes() + })), Path( Call("invalid scheme", func(ctx context.Context, mck Mock) { mck.K8sClient.EXPECT().Scheme().Return(runtime.NewScheme()).AnyTimes() @@ -272,18 +253,7 @@ func TestNewMachineScope(t *testing.T) { ), ), OneOf( - Path(Call("credentials in secret", func(ctx context.Context, mck Mock) { - mck.K8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()). - DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj *corev1.Secret, opts ...client.GetOption) error { - *obj = corev1.Secret{ - Data: map[string][]byte{ - "apiToken": []byte("apiToken"), - }, - } - return nil - }).AnyTimes() - })), - Path(Result("default credentials", func(ctx context.Context, mck Mock) { + Path(Result("default credentials used", func(ctx context.Context, mck Mock) { mScope, err := NewMachineScope(ctx, ClientConfig{Token: "apiToken"}, MachineScopeParams{ Client: mck.K8sClient, Cluster: &clusterv1.Cluster{}, @@ -295,44 +265,6 @@ func TestNewMachineScope(t *testing.T) { assert.NotNil(t, mScope) })), ), - OneOf( - Path(Result("credentials from LinodeMachine credentialsRef", func(ctx context.Context, mck Mock) { - mScope, err := NewMachineScope(ctx, ClientConfig{Token: ""}, MachineScopeParams{ - Client: mck.K8sClient, - Cluster: &clusterv1.Cluster{}, - Machine: &clusterv1.Machine{}, - LinodeCluster: &infrav1alpha2.LinodeCluster{}, - LinodeMachine: &infrav1alpha2.LinodeMachine{ - Spec: infrav1alpha2.LinodeMachineSpec{ - CredentialsRef: &corev1.SecretReference{ - Name: "example", - Namespace: "test", - }, - }, - }, - }) - require.NoError(t, err) - assert.NotNil(t, mScope) - })), - Path(Result("credentials from LinodeCluster credentialsRef", func(ctx context.Context, mck Mock) { - mScope, err := NewMachineScope(ctx, ClientConfig{Token: "apiToken"}, MachineScopeParams{ - Client: mck.K8sClient, - Cluster: &clusterv1.Cluster{}, - Machine: &clusterv1.Machine{}, - LinodeCluster: &infrav1alpha2.LinodeCluster{ - Spec: infrav1alpha2.LinodeClusterSpec{ - CredentialsRef: &corev1.SecretReference{ - Name: "example", - Namespace: "test", - }, - }, - }, - LinodeMachine: &infrav1alpha2.LinodeMachine{}, - }) - require.NoError(t, err) - assert.NotNil(t, mScope) - })), - ), ) } @@ -594,3 +526,168 @@ func TestMachineRemoveCredentialsRefFinalizer(t *testing.T) { }) } } + +func TestMachineSetCredentialRefTokenForLinodeClients(t *testing.T) { + t.Parallel() + type fields struct { + LinodeMachine *infrav1alpha2.LinodeMachine + LinodeCluster *infrav1alpha2.LinodeCluster + } + tests := []struct { + name string + fields fields + expects func(mock *mock.MockK8sClient) + expectedError error + }{ + { + name: "Success - Using LinodeMachine.Spec.CredentialsRef", + fields: fields{ + LinodeMachine: &infrav1alpha2.LinodeMachine{ + Spec: infrav1alpha2.LinodeMachineSpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + LinodeCluster: &infrav1alpha2.LinodeCluster{}, + }, + expectedError: nil, + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }).AnyTimes() + 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{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example", + Namespace: "test", + }, + Data: map[string][]byte{ + "apiToken": []byte("example"), + }, + } + *obj = cred + + return nil + }).AnyTimes() + }, + }, + { + name: "Error getting Linode Machine credentials Secret", + fields: fields{ + LinodeMachine: &infrav1alpha2.LinodeMachine{ + Spec: infrav1alpha2.LinodeMachineSpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + LinodeCluster: &infrav1alpha2.LinodeCluster{}, + }, + expectedError: fmt.Errorf("credentials from secret ref: get credentials secret test/example: test error"), + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }).AnyTimes() + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("test error")) + }, + }, + { + name: "Success - Using LinodeCluster.Spec.CredentialsRef", + fields: fields{ + LinodeCluster: &infrav1alpha2.LinodeCluster{ + Spec: infrav1alpha2.LinodeClusterSpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + LinodeMachine: &infrav1alpha2.LinodeMachine{}, + }, + expectedError: nil, + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }).AnyTimes() + 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{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example", + Namespace: "test", + }, + Data: map[string][]byte{ + "apiToken": []byte("example"), + }, + } + *obj = cred + + return nil + }).AnyTimes() + }, + }, + { + name: "Error getting Linode Cluster credentials Secret", + fields: fields{ + LinodeCluster: &infrav1alpha2.LinodeCluster{ + Spec: infrav1alpha2.LinodeClusterSpec{ + CredentialsRef: &corev1.SecretReference{ + Name: "example", + Namespace: "test", + }, + }, + }, + LinodeMachine: &infrav1alpha2.LinodeMachine{}, + }, + expectedError: fmt.Errorf("credentials from secret ref: get credentials secret test/example: test error"), + expects: func(mock *mock.MockK8sClient) { + mock.EXPECT().Scheme().DoAndReturn(func() *runtime.Scheme { + s := runtime.NewScheme() + infrav1alpha2.AddToScheme(s) + return s + }).AnyTimes() + mock.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("test error")) + }, + }, + } + 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) + + mScope, err := NewMachineScope( + context.Background(), + ClientConfig{Token: "apiToken"}, + MachineScopeParams{ + Client: mockK8sClient, + Cluster: &clusterv1.Cluster{}, + Machine: &clusterv1.Machine{}, + LinodeCluster: testcase.fields.LinodeCluster, + LinodeMachine: testcase.fields.LinodeMachine, + }, + ) + if err != nil { + t.Errorf("NewMachineScope() error = %v", err) + } + + if err := mScope.SetCredentialRefTokenForLinodeClients(context.Background()); err != nil { + assert.ErrorContains(t, err, testcase.expectedError.Error()) + } + }) + } +} From 0ca521f8a01984f261ca103ea6c034c0f4bb265a Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Tue, 10 Sep 2024 10:35:15 -0400 Subject: [PATCH 7/9] handling cyclomatic complexity --- cloud/scope/cluster.go | 21 ++++++++++++--------- cloud/scope/firewall.go | 13 ++++++++----- cloud/scope/placement_group.go | 1 - controller/linodecluster_controller.go | 8 +++----- controller/linodefirewall_controller.go | 9 ++++----- 5 files changed, 27 insertions(+), 25 deletions(-) diff --git a/cloud/scope/cluster.go b/cloud/scope/cluster.go index a82ae1c9b..8ac7fc719 100644 --- a/cloud/scope/cluster.go +++ b/cloud/scope/cluster.go @@ -140,15 +140,18 @@ func (s *ClusterScope) RemoveCredentialsRefFinalizer(ctx context.Context) error } func (s *ClusterScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { - apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeCluster.Spec.CredentialsRef, s.LinodeCluster.GetNamespace(), "apiToken") - if err != nil { - return fmt.Errorf("credentials from secret ref: %w", err) - } - s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) - dnsToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeCluster.Spec.CredentialsRef, s.LinodeCluster.GetNamespace(), "dnsToken") - if err != nil || len(dnsToken) == 0 { - dnsToken = apiToken + if s.LinodeCluster.Spec.CredentialsRef != nil { + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeCluster.Spec.CredentialsRef, s.LinodeCluster.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + dnsToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeCluster.Spec.CredentialsRef, s.LinodeCluster.GetNamespace(), "dnsToken") + if err != nil || len(dnsToken) == 0 { + dnsToken = apiToken + } + s.LinodeDomainsClient = s.LinodeDomainsClient.SetToken(string(dnsToken)) + return nil } - s.LinodeDomainsClient = s.LinodeDomainsClient.SetToken(string(dnsToken)) return nil } diff --git a/cloud/scope/firewall.go b/cloud/scope/firewall.go index 67dcffced..44b3b035b 100644 --- a/cloud/scope/firewall.go +++ b/cloud/scope/firewall.go @@ -118,11 +118,14 @@ func (s *FirewallScope) RemoveCredentialsRefFinalizer(ctx context.Context) error } func (s *FirewallScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeFirewall.Spec.CredentialsRef, s.LinodeFirewall.GetNamespace(), "apiToken") - if err != nil { - return fmt.Errorf("credentials from secret ref: %w", err) + if s.LinodeFirewall.Spec.CredentialsRef != nil { + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeFirewall.Spec.CredentialsRef, s.LinodeFirewall.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil } - s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) return nil } diff --git a/cloud/scope/placement_group.go b/cloud/scope/placement_group.go index 157d7343e..299d4697a 100644 --- a/cloud/scope/placement_group.go +++ b/cloud/scope/placement_group.go @@ -129,5 +129,4 @@ func (s *PlacementGroupScope) SetCredentialRefTokenForLinodeClients(ctx context. } s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) return nil - } diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index 01e3abb89..6f571f081 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -157,11 +157,9 @@ func (r *LinodeClusterReconciler) reconcile( return res, err } - if clusterScope.LinodeCluster.Spec.CredentialsRef != nil { - if err := clusterScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - logger.Error(err, "failed to update linode client token from Credential Ref") - return res, err - } + if err := clusterScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err } // Create diff --git a/controller/linodefirewall_controller.go b/controller/linodefirewall_controller.go index f965150a5..3bdf2ba63 100644 --- a/controller/linodefirewall_controller.go +++ b/controller/linodefirewall_controller.go @@ -143,12 +143,11 @@ func (r *LinodeFirewallReconciler) reconcile( } // Override the controller credentials with ones from the Firewall's Secret reference (if supplied). - if fwScope.LinodeFirewall.Spec.CredentialsRef != nil { - if err := fwScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - logger.Error(err, "failed to update linode client token from Credential Ref") - return ctrl.Result{}, err - } + if err := fwScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return ctrl.Result{}, err } + action := "update" if fwScope.LinodeFirewall.Spec.FirewallID != nil { failureReason = infrav1alpha2.UpdateFirewallError From 180534cd7229bdbf8e4d3bea4cc1f09c818cb70d Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Tue, 10 Sep 2024 10:44:30 -0400 Subject: [PATCH 8/9] handling cyclomatic complexity for all controllers --- cloud/scope/object_storage_key.go | 13 ++++++++----- cloud/scope/placement_group.go | 13 ++++++++----- cloud/scope/vpc.go | 13 ++++++++----- controller/linodeobjectstoragekey_controller.go | 8 +++----- controller/linodeplacementgroup_controller.go | 8 +++----- controller/linodevpc_controller.go | 8 +++----- 6 files changed, 33 insertions(+), 30 deletions(-) diff --git a/cloud/scope/object_storage_key.go b/cloud/scope/object_storage_key.go index e9642e2b6..1d0e7dbb9 100644 --- a/cloud/scope/object_storage_key.go +++ b/cloud/scope/object_storage_key.go @@ -170,11 +170,14 @@ func (s *ObjectStorageKeyScope) ShouldRotateKey() bool { // Override the controller credentials with ones from the Cluster's Secret reference (if supplied). func (s *ObjectStorageKeyScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.Key.Spec.CredentialsRef, s.Key.GetNamespace(), "apiToken") - if err != nil { - return fmt.Errorf("credentials from secret ref: %w", err) + if s.Key.Spec.CredentialsRef != nil { + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.Key.Spec.CredentialsRef, s.Key.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil } - s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) return nil } diff --git a/cloud/scope/placement_group.go b/cloud/scope/placement_group.go index 299d4697a..4c62cfdd9 100644 --- a/cloud/scope/placement_group.go +++ b/cloud/scope/placement_group.go @@ -122,11 +122,14 @@ func NewPlacementGroupScope(ctx context.Context, linodeClientConfig ClientConfig } func (s *PlacementGroupScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodePlacementGroup.Spec.CredentialsRef, s.LinodePlacementGroup.GetNamespace(), "apiToken") - if err != nil { - return fmt.Errorf("credentials from secret ref: %w", err) + if s.LinodePlacementGroup.Spec.CredentialsRef != nil { + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodePlacementGroup.Spec.CredentialsRef, s.LinodePlacementGroup.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil } - s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) return nil } diff --git a/cloud/scope/vpc.go b/cloud/scope/vpc.go index 65acb9b46..c25df5746 100644 --- a/cloud/scope/vpc.go +++ b/cloud/scope/vpc.go @@ -121,11 +121,14 @@ func (s *VPCScope) RemoveCredentialsRefFinalizer(ctx context.Context) error { } func (s *VPCScope) SetCredentialRefTokenForLinodeClients(ctx context.Context) error { - // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. - apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeVPC.Spec.CredentialsRef, s.LinodeVPC.GetNamespace(), "apiToken") - if err != nil { - return fmt.Errorf("credentials from secret ref: %w", err) + if s.LinodeVPC.Spec.CredentialsRef != nil { + // TODO: This key is hard-coded (for now) to match the externally-managed `manager-credentials` Secret. + apiToken, err := getCredentialDataFromRef(ctx, s.Client, *s.LinodeVPC.Spec.CredentialsRef, s.LinodeVPC.GetNamespace(), "apiToken") + if err != nil { + return fmt.Errorf("credentials from secret ref: %w", err) + } + s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) + return nil } - s.LinodeClient = s.LinodeClient.SetToken(string(apiToken)) return nil } diff --git a/controller/linodeobjectstoragekey_controller.go b/controller/linodeobjectstoragekey_controller.go index 2e9beaf49..93df76d08 100644 --- a/controller/linodeobjectstoragekey_controller.go +++ b/controller/linodeobjectstoragekey_controller.go @@ -132,11 +132,9 @@ func (r *LinodeObjectStorageKeyReconciler) reconcile(ctx context.Context, keySco } // Override the controller credentials with ones from the Key's Secret reference (if supplied). - if keyScope.Key.Spec.CredentialsRef != nil { - if err := keyScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - keyScope.Logger.Error(err, "failed to update linode client token from Credential Ref") - return res, err - } + if err := keyScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + keyScope.Logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err } if err := r.reconcileApply(ctx, keyScope); err != nil { diff --git a/controller/linodeplacementgroup_controller.go b/controller/linodeplacementgroup_controller.go index 427763c4e..fe4c02a3a 100644 --- a/controller/linodeplacementgroup_controller.go +++ b/controller/linodeplacementgroup_controller.go @@ -153,11 +153,9 @@ func (r *LinodePlacementGroupReconciler) reconcile( } // Override the controller credentials with ones from the Placement Groups's Secret reference (if supplied). - if pgScope.LinodePlacementGroup.Spec.CredentialsRef != nil { - if err := pgScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - logger.Error(err, "failed to update linode client token from Credential Ref") - return res, err - } + if err := pgScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err } // Update diff --git a/controller/linodevpc_controller.go b/controller/linodevpc_controller.go index 3a69bce51..619b40767 100644 --- a/controller/linodevpc_controller.go +++ b/controller/linodevpc_controller.go @@ -159,11 +159,9 @@ func (r *LinodeVPCReconciler) reconcile( } // Override the controller credentials with ones from the VPC's Secret reference (if supplied). - if vpcScope.LinodeVPC.Spec.CredentialsRef != nil { - if err := vpcScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - logger.Error(err, "failed to update linode client token from Credential Ref") - return res, err - } + if err := vpcScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err } // Update From c1f21da1b6aff56fe947c1ec5e02131222a9d057 Mon Sep 17 00:00:00 2001 From: unnatiagg Date: Fri, 20 Sep 2024 13:13:25 -0400 Subject: [PATCH 9/9] making sure the update token is places right --- controller/linodecluster_controller.go | 10 +++++----- controller/linodefirewall_controller.go | 12 ++++++------ controller/linodemachine_controller.go | 12 ++++++------ controller/linodeobjectstoragekey_controller.go | 12 ++++++------ controller/linodeplacementgroup_controller.go | 12 ++++++------ controller/linodevpc_controller.go | 12 ++++++------ 6 files changed, 35 insertions(+), 35 deletions(-) diff --git a/controller/linodecluster_controller.go b/controller/linodecluster_controller.go index 6f571f081..11e693ac0 100644 --- a/controller/linodecluster_controller.go +++ b/controller/linodecluster_controller.go @@ -140,6 +140,11 @@ func (r *LinodeClusterReconciler) reconcile( return res, err } + if err := clusterScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err + } + // Handle deleted clusters if !clusterScope.LinodeCluster.DeletionTimestamp.IsZero() { if err := r.reconcileDelete(ctx, logger, clusterScope); err != nil { @@ -157,11 +162,6 @@ func (r *LinodeClusterReconciler) reconcile( return res, err } - if err := clusterScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - logger.Error(err, "failed to update linode client token from Credential Ref") - return res, err - } - // Create if clusterScope.LinodeCluster.Spec.ControlPlaneEndpoint.Host == "" { if err := r.reconcileCreate(ctx, logger, clusterScope); err != nil { diff --git a/controller/linodefirewall_controller.go b/controller/linodefirewall_controller.go index 3bdf2ba63..9924a498d 100644 --- a/controller/linodefirewall_controller.go +++ b/controller/linodefirewall_controller.go @@ -127,6 +127,12 @@ func (r *LinodeFirewallReconciler) reconcile( } }() + // Override the controller credentials with ones from the Firewall's Secret reference (if supplied). + if err := fwScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return ctrl.Result{}, err + } + // Delete if !fwScope.LinodeFirewall.ObjectMeta.DeletionTimestamp.IsZero() { failureReason = infrav1alpha2.DeleteFirewallError @@ -142,12 +148,6 @@ func (r *LinodeFirewallReconciler) reconcile( return ctrl.Result{}, nil } - // Override the controller credentials with ones from the Firewall's Secret reference (if supplied). - if err := fwScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - logger.Error(err, "failed to update linode client token from Credential Ref") - return ctrl.Result{}, err - } - action := "update" if fwScope.LinodeFirewall.Spec.FirewallID != nil { failureReason = infrav1alpha2.UpdateFirewallError diff --git a/controller/linodemachine_controller.go b/controller/linodemachine_controller.go index 7858a66e8..09244a382 100644 --- a/controller/linodemachine_controller.go +++ b/controller/linodemachine_controller.go @@ -191,12 +191,6 @@ func (r *LinodeMachineReconciler) reconcile(ctx context.Context, logger logr.Log return ctrl.Result{}, err } - // Delete - if !machineScope.LinodeMachine.ObjectMeta.DeletionTimestamp.IsZero() { - failureReason = cerrs.DeleteMachineError - return r.reconcileDelete(ctx, logger, machineScope) - } - // Override the controller credentials with ones from the Machine's Secret reference (if supplied). // Credentials will be used in the following order: // 1. LinodeMachine @@ -209,6 +203,12 @@ func (r *LinodeMachineReconciler) reconcile(ctx context.Context, logger logr.Log } } + // Delete + if !machineScope.LinodeMachine.ObjectMeta.DeletionTimestamp.IsZero() { + failureReason = cerrs.DeleteMachineError + return r.reconcileDelete(ctx, logger, machineScope) + } + // Make sure bootstrap data is available and populated. if machineScope.Machine.Spec.Bootstrap.DataSecretName == nil { logger.Info("Bootstrap data secret is not yet available") diff --git a/controller/linodeobjectstoragekey_controller.go b/controller/linodeobjectstoragekey_controller.go index 93df76d08..4507e771b 100644 --- a/controller/linodeobjectstoragekey_controller.go +++ b/controller/linodeobjectstoragekey_controller.go @@ -123,6 +123,12 @@ func (r *LinodeObjectStorageKeyReconciler) reconcile(ctx context.Context, keySco } }() + // Override the controller credentials with ones from the Key's Secret reference (if supplied). + if err := keyScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + keyScope.Logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err + } + if !keyScope.Key.DeletionTimestamp.IsZero() { return res, r.reconcileDelete(ctx, keyScope) } @@ -131,12 +137,6 @@ func (r *LinodeObjectStorageKeyReconciler) reconcile(ctx context.Context, keySco return res, err } - // Override the controller credentials with ones from the Key's Secret reference (if supplied). - if err := keyScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - keyScope.Logger.Error(err, "failed to update linode client token from Credential Ref") - return res, err - } - if err := r.reconcileApply(ctx, keyScope); err != nil { return res, err } diff --git a/controller/linodeplacementgroup_controller.go b/controller/linodeplacementgroup_controller.go index fe4c02a3a..f57bc7505 100644 --- a/controller/linodeplacementgroup_controller.go +++ b/controller/linodeplacementgroup_controller.go @@ -135,6 +135,12 @@ func (r *LinodePlacementGroupReconciler) reconcile( } }() + // Override the controller credentials with ones from the Placement Groups's Secret reference (if supplied). + if err := pgScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err + } + // Delete if !pgScope.LinodePlacementGroup.ObjectMeta.DeletionTimestamp.IsZero() { failureReason = infrav1alpha2.DeletePlacementGroupError @@ -152,12 +158,6 @@ func (r *LinodePlacementGroupReconciler) reconcile( return } - // Override the controller credentials with ones from the Placement Groups's Secret reference (if supplied). - if err := pgScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - logger.Error(err, "failed to update linode client token from Credential Ref") - return res, err - } - // Update if pgScope.LinodePlacementGroup.Spec.PGID != nil { logger = logger.WithValues("pgID", *pgScope.LinodePlacementGroup.Spec.PGID) diff --git a/controller/linodevpc_controller.go b/controller/linodevpc_controller.go index 619b40767..abbba1044 100644 --- a/controller/linodevpc_controller.go +++ b/controller/linodevpc_controller.go @@ -141,6 +141,12 @@ func (r *LinodeVPCReconciler) reconcile( } }() + // Override the controller credentials with ones from the VPC's Secret reference (if supplied). + if err := vpcScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { + logger.Error(err, "failed to update linode client token from Credential Ref") + return res, err + } + // Delete if !vpcScope.LinodeVPC.ObjectMeta.DeletionTimestamp.IsZero() { failureReason = infrav1alpha2.DeleteVPCError @@ -158,12 +164,6 @@ func (r *LinodeVPCReconciler) reconcile( return } - // Override the controller credentials with ones from the VPC's Secret reference (if supplied). - if err := vpcScope.SetCredentialRefTokenForLinodeClients(ctx); err != nil { - logger.Error(err, "failed to update linode client token from Credential Ref") - return res, err - } - // Update if vpcScope.LinodeVPC.Spec.VPCID != nil { failureReason = infrav1alpha2.UpdateVPCError