From bc82e3e1bb2030b2bfe5f10101129a1d157319ea Mon Sep 17 00:00:00 2001 From: Zach Jones Date: Thu, 15 Sep 2022 17:52:10 -0600 Subject: [PATCH] feat: renamed csr approval functions and introduced better error handling --- .../admin_openshiftcluster_approvecsr.go | 4 +- .../admin_openshiftcluster_approvecsr_test.go | 4 +- .../certificatesigningrequests.go | 17 +++-- pkg/frontend/adminactions/kubeactions.go | 6 +- pkg/util/mocks/adminactions/adminactions.go | 71 ++++++++----------- 5 files changed, 44 insertions(+), 58 deletions(-) diff --git a/pkg/frontend/admin_openshiftcluster_approvecsr.go b/pkg/frontend/admin_openshiftcluster_approvecsr.go index f1a8c6ede..7aef1c81e 100644 --- a/pkg/frontend/admin_openshiftcluster_approvecsr.go +++ b/pkg/frontend/admin_openshiftcluster_approvecsr.go @@ -54,8 +54,8 @@ func (f *frontend) _postAdminOpenShiftClusterApproveCSR(ctx context.Context, r * } if csrName != "" { - return k.RunCertificateApprove(ctx, csrName) + return k.ApproveCsr(ctx, csrName) } - return k.RunCertificateMassApprove(ctx) + return k.ApproveAllCsrs(ctx) } diff --git a/pkg/frontend/admin_openshiftcluster_approvecsr_test.go b/pkg/frontend/admin_openshiftcluster_approvecsr_test.go index df6ec1d22..c21d0dd98 100644 --- a/pkg/frontend/admin_openshiftcluster_approvecsr_test.go +++ b/pkg/frontend/admin_openshiftcluster_approvecsr_test.go @@ -44,7 +44,7 @@ func TestAdminApproveCSR(t *testing.T) { csrName: "aro-csr", mocks: func(tt *test, k *mock_adminactions.MockKubeActions) { k.EXPECT(). - RunCertificateApprove(gomock.Any(), tt.csrName). + ApproveCsr(gomock.Any(), tt.csrName). Return(nil) }, wantStatusCode: http.StatusOK, @@ -55,7 +55,7 @@ func TestAdminApproveCSR(t *testing.T) { resourceID: fmt.Sprintf("/subscriptions/%s/resourcegroups/resourceGroup/providers/Microsoft.RedHatOpenShift/openShiftClusters/resourceName", mockSubID), mocks: func(tt *test, k *mock_adminactions.MockKubeActions) { k.EXPECT(). - RunCertificateMassApprove(gomock.Any()). + ApproveAllCsrs(gomock.Any()). Return(nil) }, wantStatusCode: http.StatusOK, diff --git a/pkg/frontend/adminactions/certificatesigningrequests.go b/pkg/frontend/adminactions/certificatesigningrequests.go index 1acd7aeb6..aec407708 100644 --- a/pkg/frontend/adminactions/certificatesigningrequests.go +++ b/pkg/frontend/adminactions/certificatesigningrequests.go @@ -15,22 +15,25 @@ import ( "github.com/Azure/ARO-RP/pkg/api" ) -func (k *kubeActions) RunCertificateApprove(ctx context.Context, csrName string) error { +func (k *kubeActions) ApproveCsr(ctx context.Context, csrName string) error { csr, err := k.kubecli.CertificatesV1().CertificateSigningRequests().Get(ctx, csrName, metav1.GetOptions{}) - if kerrors.IsNotFound(err) { - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeResourceNotFound, "", "certificate signing request '%s' was not found.", csrName) + if err != nil { + if kerrors.IsNotFound(err) { + return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeResourceNotFound, "", "certificate signing request '%s' was not found.", csrName) + } + return err } - return k.RunCertificateApprovalUpdate(ctx, csr) + return k.updateCsr(ctx, csr) } -func (k *kubeActions) RunCertificateMassApprove(ctx context.Context) error { +func (k *kubeActions) ApproveAllCsrs(ctx context.Context) error { csrs, err := k.kubecli.CertificatesV1().CertificateSigningRequests().List(ctx, metav1.ListOptions{}) if err != nil { return err } for _, csr := range csrs.Items { - err = k.RunCertificateApprovalUpdate(ctx, &csr) + err = k.updateCsr(ctx, &csr) if err != nil { return err } @@ -38,7 +41,7 @@ func (k *kubeActions) RunCertificateMassApprove(ctx context.Context) error { return nil } -func (k *kubeActions) RunCertificateApprovalUpdate(ctx context.Context, csr *certificatesv1.CertificateSigningRequest) error { +func (k *kubeActions) updateCsr(ctx context.Context, csr *certificatesv1.CertificateSigningRequest) error { modifiedCSR, hasCondition, err := addConditionIfNeeded(csr, string(certificatesv1.CertificateDenied), string(certificatesv1.CertificateApproved), "AROSupportApprove", "This CSR was approved by ARO support personnel.") if err != nil { diff --git a/pkg/frontend/adminactions/kubeactions.go b/pkg/frontend/adminactions/kubeactions.go index 383ae6aa1..a63bb63a1 100644 --- a/pkg/frontend/adminactions/kubeactions.go +++ b/pkg/frontend/adminactions/kubeactions.go @@ -10,7 +10,6 @@ import ( "github.com/Azure/go-autorest/autorest/to" configclient "github.com/openshift/client-go/config/clientset/versioned" "github.com/sirupsen/logrus" - certificatesv1 "k8s.io/api/certificates/v1" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -32,9 +31,8 @@ type KubeActions interface { KubeDelete(ctx context.Context, groupKind, namespace, name string, force bool) error CordonNode(ctx context.Context, nodeName string, unschedulable bool) error DrainNode(ctx context.Context, nodeName string) error - RunCertificateApprove(ctx context.Context, csrName string) error - RunCertificateMassApprove(ctx context.Context) error - RunCertificateApprovalUpdate(ctx context.Context, csr *certificatesv1.CertificateSigningRequest) error + ApproveCsr(ctx context.Context, csrName string) error + ApproveAllCsrs(ctx context.Context) error Upgrade(ctx context.Context, upgradeY bool) error KubeGetPodLogs(ctx context.Context, namespace, name, containerName string) ([]byte, error) } diff --git a/pkg/util/mocks/adminactions/adminactions.go b/pkg/util/mocks/adminactions/adminactions.go index d57c161b9..8e7b06491 100644 --- a/pkg/util/mocks/adminactions/adminactions.go +++ b/pkg/util/mocks/adminactions/adminactions.go @@ -12,7 +12,6 @@ import ( compute "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2020-06-01/compute" gomock "github.com/golang/mock/gomock" logrus "github.com/sirupsen/logrus" - v1 "k8s.io/api/certificates/v1" unstructured "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -39,6 +38,34 @@ func (m *MockKubeActions) EXPECT() *MockKubeActionsMockRecorder { return m.recorder } +// ApproveAllCsrs mocks base method. +func (m *MockKubeActions) ApproveAllCsrs(arg0 context.Context) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ApproveAllCsrs", arg0) + ret0, _ := ret[0].(error) + return ret0 +} + +// ApproveAllCsrs indicates an expected call of ApproveAllCsrs. +func (mr *MockKubeActionsMockRecorder) ApproveAllCsrs(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ApproveAllCsrs", reflect.TypeOf((*MockKubeActions)(nil).ApproveAllCsrs), arg0) +} + +// ApproveCsr mocks base method. +func (m *MockKubeActions) ApproveCsr(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ApproveCsr", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// ApproveCsr indicates an expected call of ApproveCsr. +func (mr *MockKubeActionsMockRecorder) ApproveCsr(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ApproveCsr", reflect.TypeOf((*MockKubeActions)(nil).ApproveCsr), arg0, arg1) +} + // CordonNode mocks base method. func (m *MockKubeActions) CordonNode(arg0 context.Context, arg1 string, arg2 bool) error { m.ctrl.T.Helper() @@ -140,48 +167,6 @@ func (mr *MockKubeActionsMockRecorder) KubeList(arg0, arg1, arg2 interface{}) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "KubeList", reflect.TypeOf((*MockKubeActions)(nil).KubeList), arg0, arg1, arg2) } -// RunCertificateApprovalUpdate mocks base method. -func (m *MockKubeActions) RunCertificateApprovalUpdate(arg0 context.Context, arg1 *v1.CertificateSigningRequest) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RunCertificateApprovalUpdate", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// RunCertificateApprovalUpdate indicates an expected call of RunCertificateApprovalUpdate. -func (mr *MockKubeActionsMockRecorder) RunCertificateApprovalUpdate(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RunCertificateApprovalUpdate", reflect.TypeOf((*MockKubeActions)(nil).RunCertificateApprovalUpdate), arg0, arg1) -} - -// RunCertificateApprove mocks base method. -func (m *MockKubeActions) RunCertificateApprove(arg0 context.Context, arg1 string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RunCertificateApprove", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// RunCertificateApprove indicates an expected call of RunCertificateApprove. -func (mr *MockKubeActionsMockRecorder) RunCertificateApprove(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RunCertificateApprove", reflect.TypeOf((*MockKubeActions)(nil).RunCertificateApprove), arg0, arg1) -} - -// RunCertificateMassApprove mocks base method. -func (m *MockKubeActions) RunCertificateMassApprove(arg0 context.Context) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RunCertificateMassApprove", arg0) - ret0, _ := ret[0].(error) - return ret0 -} - -// RunCertificateMassApprove indicates an expected call of RunCertificateMassApprove. -func (mr *MockKubeActionsMockRecorder) RunCertificateMassApprove(arg0 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RunCertificateMassApprove", reflect.TypeOf((*MockKubeActions)(nil).RunCertificateMassApprove), arg0) -} - // Upgrade mocks base method. func (m *MockKubeActions) Upgrade(arg0 context.Context, arg1 bool) error { m.ctrl.T.Helper()