From bf86e2c0d3e06ffd3e9c56a5547de3161c97c635 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Mon, 20 Sep 2021 11:30:23 -0700 Subject: [PATCH 1/8] chore: removing cns http client which did nothing --- cns/client/httpapi/client.go | 64 ---------------- .../mockclients/cnsclient_generated.go | 73 ++++++++++--------- .../multitenantoperator/cnsclient.go | 9 --- .../multitenantcrdcontroller.go | 16 +--- .../multitenantcrdreconciler.go | 23 ++++-- .../multitenantcrdreconciler_test.go | 29 ++++---- cns/restserver/errors.go | 18 +++++ .../kubecontroller/cnsclient.go | 12 --- .../kubecontroller/crdreconciler.go | 9 ++- .../kubecontroller/crdrequestcontroller.go | 34 ++++----- .../crdrequestcontroller_test.go | 44 +++++------ 11 files changed, 129 insertions(+), 202 deletions(-) delete mode 100644 cns/client/httpapi/client.go delete mode 100644 cns/multitenantcontroller/multitenantoperator/cnsclient.go create mode 100644 cns/restserver/errors.go delete mode 100644 cns/singletenantcontroller/kubecontroller/cnsclient.go diff --git a/cns/client/httpapi/client.go b/cns/client/httpapi/client.go deleted file mode 100644 index b4a85d81cb..0000000000 --- a/cns/client/httpapi/client.go +++ /dev/null @@ -1,64 +0,0 @@ -package httpapi - -import ( - "fmt" - - "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/restserver" - "github.com/Azure/azure-container-networking/cns/types" - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" - "github.com/pkg/errors" -) - -// Client implements APIClient interface. Used to update CNS state -type Client struct { - RestService *restserver.HTTPRestService -} - -// CreateOrUpdateNC updates cns state -func (client *Client) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error { - returnCode := client.RestService.CreateOrUpdateNetworkContainerInternal(ncRequest) - - if returnCode != 0 { - return fmt.Errorf("Failed to Create NC request: %+v, errorCode: %d", ncRequest, returnCode) - } - - return nil -} - -// UpdateIPAMPoolMonitor updates IPAM pool monitor. -func (client *Client) UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) { - client.RestService.IPAMPoolMonitor.Update(scalar, spec) -} - -// ReconcileNCState initializes cns state -func (client *Client) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error { - returnCode := client.RestService.ReconcileNCState(ncRequest, podInfoByIP, scalar, spec) - - if returnCode != 0 { - return fmt.Errorf("Failed to Reconcile ncState: ncRequest %+v, podInfoMap: %+v, errorCode: %d", *ncRequest, podInfoByIP, returnCode) - } - - return nil -} - -func (client *Client) GetNC(req cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, error) { - resp, returnCode := client.RestService.GetNetworkContainerInternal(req) - if returnCode != 0 { - if returnCode == types.UnknownContainerID { - return resp, errors.New(returnCode.String()) - } - return resp, errors.Errorf("failed to get NC, request: %+v, errorCode: %d", req, returnCode) - } - - return resp, nil -} - -func (client *Client) DeleteNC(req cns.DeleteNetworkContainerRequest) error { - returnCode := client.RestService.DeleteNetworkContainerInternal(req) - if returnCode != 0 { - return fmt.Errorf("Failed to delete NC, request: %+v, errorCode: %d", req, returnCode) - } - - return nil -} diff --git a/cns/multitenantcontroller/mockclients/cnsclient_generated.go b/cns/multitenantcontroller/mockclients/cnsclient_generated.go index 0069c6940a..e706e2ac99 100644 --- a/cns/multitenantcontroller/mockclients/cnsclient_generated.go +++ b/cns/multitenantcontroller/mockclients/cnsclient_generated.go @@ -11,78 +11,79 @@ import ( reflect "reflect" cns "github.com/Azure/azure-container-networking/cns" + cnstypes "github.com/Azure/azure-container-networking/cns/types" v1alpha "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" gomock "github.com/golang/mock/gomock" ) -// MockAPIClient is a mock of APIClient interface. -type MockAPIClient struct { +// MockCNSRestService is a mock of CNS Rest Server. +type MockCNSRestService struct { ctrl *gomock.Controller - recorder *MockAPIClientMockRecorder + recorder *MockCNSRestServiceMockRecorder } // MockAPIClientMockRecorder is the mock recorder for MockAPIClient. -type MockAPIClientMockRecorder struct { - mock *MockAPIClient +type MockCNSRestServiceMockRecorder struct { + mock *MockCNSRestService } // NewMockAPIClient creates a new mock instance. -func NewMockAPIClient(ctrl *gomock.Controller) *MockAPIClient { - mock := &MockAPIClient{ctrl: ctrl} - mock.recorder = &MockAPIClientMockRecorder{mock} +func NewMockCNSRestServer(ctrl *gomock.Controller) *MockCNSRestService { + mock := &MockCNSRestService{ctrl: ctrl} + mock.recorder = &MockCNSRestServiceMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockAPIClient) EXPECT() *MockAPIClientMockRecorder { +func (m *MockCNSRestService) EXPECT() *MockCNSRestServiceMockRecorder { return m.recorder } -// CreateOrUpdateNC mocks base method. -func (m *MockAPIClient) CreateOrUpdateNC(nc cns.CreateNetworkContainerRequest) error { +// CreateOrUpdateNetworkContainerInternal mocks base method. +func (m *MockCNSRestService) CreateOrUpdateNetworkContainerInternal(nc cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateNC", nc) - ret0, _ := ret[0].(error) + ret := m.ctrl.Call(m, "CreateOrUpdateNetworkContainerInternal", nc) + ret0, _ := ret[0].(cnstypes.ResponseCode) return ret0 } -// CreateOrUpdateNC indicates an expected call of CreateOrUpdateNC. -func (mr *MockAPIClientMockRecorder) CreateOrUpdateNC(nc interface{}) *gomock.Call { +// CreateOrUpdateNetworkContainerInternal indicates an expected call of CreateOrUpdateNetworkContainerInternal. +func (mr *MockCNSRestServiceMockRecorder) CreateOrUpdateNetworkContainerInternal(nc interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateNC", reflect.TypeOf((*MockAPIClient)(nil).CreateOrUpdateNC), nc) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateNetworkContainerInternal", reflect.TypeOf((*MockCNSRestService)(nil).CreateOrUpdateNetworkContainerInternal), nc) } -// DeleteNC mocks base method. -func (m *MockAPIClient) DeleteNC(nc cns.DeleteNetworkContainerRequest) error { +// DeleteNetworkContainerInternal mocks base method. +func (m *MockCNSRestService) DeleteNetworkContainerInternal(nc cns.DeleteNetworkContainerRequest) cnstypes.ResponseCode { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteNC", nc) - ret0, _ := ret[0].(error) + ret := m.ctrl.Call(m, "DeleteNetworkContainerInternal", nc) + ret0, _ := ret[0].(cnstypes.ResponseCode) return ret0 } -// DeleteNC indicates an expected call of DeleteNC. -func (mr *MockAPIClientMockRecorder) DeleteNC(nc interface{}) *gomock.Call { +// DeleteNetworkContainerInternal indicates an expected call of DeleteNetworkContainerInternal. +func (mr *MockCNSRestServiceMockRecorder) DeleteNetworkContainerInternal(nc interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNC", reflect.TypeOf((*MockAPIClient)(nil).DeleteNC), nc) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNetworkContainerInternal", reflect.TypeOf((*MockCNSRestService)(nil).DeleteNetworkContainerInternal), nc) } -// GetNC mocks base method. -func (m *MockAPIClient) GetNC(nc cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, error) { +// GetNetworkContainerInternal mocks base method. +func (m *MockCNSRestService) GetNetworkContainerInternal(nc cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, cnstypes.ResponseCode) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetNC", nc) + ret := m.ctrl.Call(m, "GetNetworkContainerInternal", nc) ret0, _ := ret[0].(cns.GetNetworkContainerResponse) - ret1, _ := ret[1].(error) + ret1, _ := ret[1].(cnstypes.ResponseCode) return ret0, ret1 } -// GetNC indicates an expected call of GetNC. -func (mr *MockAPIClientMockRecorder) GetNC(nc interface{}) *gomock.Call { +// GetNetworkContainerInternal indicates an expected call of GetNetworkContainerInternal. +func (mr *MockCNSRestServiceMockRecorder) GetNetworkContainerInternal(nc interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNC", reflect.TypeOf((*MockAPIClient)(nil).GetNC), nc) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkContainerInternal", reflect.TypeOf((*MockCNSRestService)(nil).GetNetworkContainerInternal), nc) } // ReconcileNCState mocks base method. -func (m *MockAPIClient) ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error { +func (m *MockCNSRestService) ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ReconcileNCState", nc, pods, scalar, spec) ret0, _ := ret[0].(error) @@ -90,19 +91,19 @@ func (m *MockAPIClient) ReconcileNCState(nc *cns.CreateNetworkContainerRequest, } // ReconcileNCState indicates an expected call of ReconcileNCState. -func (mr *MockAPIClientMockRecorder) ReconcileNCState(nc, pods, scalar, spec interface{}) *gomock.Call { +func (mr *MockCNSRestServiceMockRecorder) ReconcileNCState(nc, pods, scalar, spec interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReconcileNCState", reflect.TypeOf((*MockAPIClient)(nil).ReconcileNCState), nc, pods, scalar, spec) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReconcileNCState", reflect.TypeOf((*MockCNSRestService)(nil).ReconcileNCState), nc, pods, scalar, spec) } // UpdateIPAMPoolMonitor mocks base method. -func (m *MockAPIClient) UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) { +func (m *MockCNSRestService) UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) { m.ctrl.T.Helper() m.ctrl.Call(m, "UpdateIPAMPoolMonitor", scalar, spec) } // UpdateIPAMPoolMonitor indicates an expected call of UpdateIPAMPoolMonitor. -func (mr *MockAPIClientMockRecorder) UpdateIPAMPoolMonitor(scalar, spec interface{}) *gomock.Call { +func (mr *MockCNSRestServiceMockRecorder) UpdateIPAMPoolMonitor(scalar, spec interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateIPAMPoolMonitor", reflect.TypeOf((*MockAPIClient)(nil).UpdateIPAMPoolMonitor), scalar, spec) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateIPAMPoolMonitor", reflect.TypeOf((*MockCNSRestService)(nil).UpdateIPAMPoolMonitor), scalar, spec) } diff --git a/cns/multitenantcontroller/multitenantoperator/cnsclient.go b/cns/multitenantcontroller/multitenantoperator/cnsclient.go deleted file mode 100644 index 43a54395c4..0000000000 --- a/cns/multitenantcontroller/multitenantoperator/cnsclient.go +++ /dev/null @@ -1,9 +0,0 @@ -package multitenantoperator - -import "github.com/Azure/azure-container-networking/cns" - -type cnsclient interface { - DeleteNC(req cns.DeleteNetworkContainerRequest) error - GetNC(req cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, error) - CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error -} diff --git a/cns/multitenantcontroller/multitenantoperator/multitenantcrdcontroller.go b/cns/multitenantcontroller/multitenantoperator/multitenantcrdcontroller.go index 549e5efe46..af400133dc 100644 --- a/cns/multitenantcontroller/multitenantoperator/multitenantcrdcontroller.go +++ b/cns/multitenantcontroller/multitenantoperator/multitenantcrdcontroller.go @@ -6,7 +6,6 @@ import ( "os" "sync" - "github.com/Azure/azure-container-networking/cns/client/httpapi" "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/multitenantcontroller" "github.com/Azure/azure-container-networking/cns/restserver" @@ -31,8 +30,7 @@ var _ (multitenantcontroller.RequestController) = (*requestController)(nil) type requestController struct { mgr manager.Manager // Manager starts the reconcile loop which watches for crd status changes KubeClient client.Client // KubeClient is a cached client which interacts with API server - CNSClient cnsclient - nodeName string // name of node running this program + nodeName string // name of node running this program Reconciler *multiTenantCrdReconciler Started bool lock sync.Mutex @@ -72,16 +70,11 @@ func New(restService *restserver.HTTPRestService, kubeconfig *rest.Config) (*req return nil, err } - // Create httpClient - httpClient := &httpapi.Client{ - RestService: restService, - } - // Create multiTenantCrdReconciler reconciler := &multiTenantCrdReconciler{ - KubeClient: mgr.GetClient(), - NodeName: nodeName, - CNSClient: httpClient, + KubeClient: mgr.GetClient(), + NodeName: nodeName, + CNSRestService: restService, } if err := reconciler.SetupWithManager(mgr); err != nil { logger.Errorf("Error setting up new multiTenantCrdReconciler: %v", err) @@ -92,7 +85,6 @@ func New(restService *restserver.HTTPRestService, kubeconfig *rest.Config) (*req return &requestController{ mgr: mgr, KubeClient: mgr.GetClient(), - CNSClient: httpClient, nodeName: nodeName, Reconciler: reconciler, }, nil diff --git a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go index 36b143d70e..d4bff38ad9 100644 --- a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go +++ b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go @@ -9,6 +9,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" + "github.com/Azure/azure-container-networking/cns/restserver" "github.com/Azure/azure-container-networking/cns/types" ncapi "github.com/Azure/azure-container-networking/crd/multitenantnetworkcontainer/api/v1alpha1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -29,11 +30,17 @@ const ( NCStateTerminated = "Terminated" ) +type cnsrestservice interface { + DeleteNetworkContainerInternal(cns.DeleteNetworkContainerRequest) types.ResponseCode + GetNetworkContainerInternal(cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, types.ResponseCode) + CreateOrUpdateNetworkContainerInternal(cns.CreateNetworkContainerRequest) types.ResponseCode +} + // multiTenantCrdReconciler reconciles multi-tenant network containers. type multiTenantCrdReconciler struct { - KubeClient client.Client - NodeName string - CNSClient cnsclient + KubeClient client.Client + NodeName string + CNSRestService cnsrestservice } // Reconcile is called on multi-tenant CRD status changes. @@ -59,9 +66,10 @@ func (r *multiTenantCrdReconciler) Reconcile(ctx context.Context, request reconc } // Remove the deleted network container from CNS. - err := r.CNSClient.DeleteNC(cns.DeleteNetworkContainerRequest{ + responseCode := r.CNSRestService.DeleteNetworkContainerInternal(cns.DeleteNetworkContainerRequest{ NetworkContainerid: nc.Spec.UUID, }) + err := restserver.ResponseCodeToError(responseCode) if err != nil { logger.Errorf("Failed to delete NC %s (UUID: %s) from CNS: %v", request.NamespacedName.String(), nc.Spec.UUID, err) return ctrl.Result{}, err @@ -96,10 +104,11 @@ func (r *multiTenantCrdReconciler) Reconcile(ctx context.Context, request reconc } // Check CNS NC states. - _, err = r.CNSClient.GetNC(cns.GetNetworkContainerRequest{ + _, returnCode := r.CNSRestService.GetNetworkContainerInternal(cns.GetNetworkContainerRequest{ NetworkContainerid: nc.Spec.UUID, OrchestratorContext: orchestratorContext, }) + err = restserver.ResponseCodeToError(returnCode) if err == nil { logger.Printf("NC %s (UUID: %s) has already been created in CNS", request.NamespacedName.String(), nc.Spec.UUID) return ctrl.Result{}, nil @@ -141,7 +150,9 @@ func (r *multiTenantCrdReconciler) Reconcile(ctx context.Context, request reconc }, } logger.Printf("CreateOrUpdateNC with networkContainerRequest: %#v", networkContainerRequest) - if err = r.CNSClient.CreateOrUpdateNC(networkContainerRequest); err != nil { + responseCode := r.CNSRestService.CreateOrUpdateNetworkContainerInternal(networkContainerRequest) + err = restserver.ResponseCodeToError(responseCode) + if err != nil { logger.Errorf("Failed to persist state for NC %s (UUID: %s) to CNS: %v", request.NamespacedName.String(), nc.Spec.UUID, err) return ctrl.Result{}, err } diff --git a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go index 1f3970acb8..7f6292b0ef 100644 --- a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go +++ b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go @@ -3,7 +3,6 @@ package multitenantoperator import ( "context" "encoding/json" - "errors" "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" @@ -19,11 +18,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -var errContainerIDNotFound = errors.New(cnstypes.UnknownContainerID.String()) - var _ = Describe("multiTenantCrdReconciler", func() { var kubeClient *mockclients.MockClient - var cnsClient *mockclients.MockAPIClient + var cnsRestService *mockclients.MockCNSRestService var mockCtl *gomock.Controller var reconciler *multiTenantCrdReconciler const uuidValue = "uuid" @@ -41,11 +38,11 @@ var _ = Describe("multiTenantCrdReconciler", func() { logger.InitLogger("multiTenantCrdReconciler", 0, 0, "") mockCtl = gomock.NewController(GinkgoT()) kubeClient = mockclients.NewMockClient(mockCtl) - cnsClient = mockclients.NewMockAPIClient(mockCtl) + cnsRestService = mockclients.NewMockCNSRestServer(mockCtl) reconciler = &multiTenantCrdReconciler{ - KubeClient: kubeClient, - NodeName: mockNodeName, - CNSClient: cnsClient, + KubeClient: kubeClient, + NodeName: mockNodeName, + CNSRestService: cnsRestService, } }) @@ -129,10 +126,10 @@ var _ = Describe("multiTenantCrdReconciler", func() { Expect(err).To(BeNil()) kubeClient.EXPECT().Get(gomock.Any(), namespacedName, gomock.Any()).SetArg(2, nc) - cnsClient.EXPECT().GetNC(cns.GetNetworkContainerRequest{ + cnsRestService.EXPECT().GetNetworkContainerInternal(cns.GetNetworkContainerRequest{ NetworkContainerid: uuid, OrchestratorContext: orchestratorContext, - }).Return(cns.GetNetworkContainerResponse{}, nil) + }).Return(cns.GetNetworkContainerResponse{}, cnstypes.Success) _, err = reconciler.Reconcile(context.TODO(), reconcile.Request{ NamespacedName: namespacedName, }) @@ -163,15 +160,15 @@ var _ = Describe("multiTenantCrdReconciler", func() { Expect(err).To(BeNil()) kubeClient.EXPECT().Get(gomock.Any(), namespacedName, gomock.Any()).SetArg(2, nc) - cnsClient.EXPECT().GetNC(cns.GetNetworkContainerRequest{ + cnsRestService.EXPECT().GetNetworkContainerInternal(cns.GetNetworkContainerRequest{ NetworkContainerid: uuid, OrchestratorContext: orchestratorContext, - }).Return(cns.GetNetworkContainerResponse{}, errContainerIDNotFound) + }).Return(cns.GetNetworkContainerResponse{}, cnstypes.UnknownContainerID) _, err = reconciler.Reconcile(context.TODO(), reconcile.Request{ NamespacedName: namespacedName, }) Expect(err).NotTo(BeNil()) - Expect(err.Error()).To(ContainSubstring("invalid CIDR address")) + Expect(err.Error()).To(ContainSubstring("UnknownContainerID")) }) It("Should succeed when the NC subnet is in correct format", func() { @@ -221,11 +218,11 @@ var _ = Describe("multiTenantCrdReconciler", func() { statusWriter := mockclients.NewMockStatusWriter(mockCtl) statusWriter.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil) kubeClient.EXPECT().Status().Return(statusWriter) - cnsClient.EXPECT().GetNC(cns.GetNetworkContainerRequest{ + cnsRestService.EXPECT().GetNetworkContainerInternal(cns.GetNetworkContainerRequest{ NetworkContainerid: uuid, OrchestratorContext: orchestratorContext, - }).Return(cns.GetNetworkContainerResponse{}, errContainerIDNotFound) - cnsClient.EXPECT().CreateOrUpdateNC(networkContainerRequest).Return(nil) + }).Return(cns.GetNetworkContainerResponse{}, cnstypes.Success) + cnsRestService.EXPECT().CreateOrUpdateNetworkContainerInternal(networkContainerRequest).Return(cnstypes.Success) _, err = reconciler.Reconcile(context.TODO(), reconcile.Request{ NamespacedName: namespacedName, }) diff --git a/cns/restserver/errors.go b/cns/restserver/errors.go new file mode 100644 index 0000000000..f4fd878d2b --- /dev/null +++ b/cns/restserver/errors.go @@ -0,0 +1,18 @@ +package restserver + +import ( + "errors" + "fmt" + + "github.com/Azure/azure-container-networking/cns/types" +) + +var errResponseCode = errors.New("Response code is error") + +// ResponseCodeToError converts a cns response code to error type. If the response code is OK, then return value is nil +func ResponseCodeToError(responseCode types.ResponseCode) error { + if responseCode == 0 { + return nil + } + return fmt.Errorf("%w: %v", errResponseCode, responseCode) +} diff --git a/cns/singletenantcontroller/kubecontroller/cnsclient.go b/cns/singletenantcontroller/kubecontroller/cnsclient.go deleted file mode 100644 index a1c6c05aff..0000000000 --- a/cns/singletenantcontroller/kubecontroller/cnsclient.go +++ /dev/null @@ -1,12 +0,0 @@ -package kubecontroller - -import ( - "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" -) - -type cnsclient interface { - ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error - CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error - UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) -} diff --git a/cns/singletenantcontroller/kubecontroller/crdreconciler.go b/cns/singletenantcontroller/kubecontroller/crdreconciler.go index a609140da0..035d6db851 100644 --- a/cns/singletenantcontroller/kubecontroller/crdreconciler.go +++ b/cns/singletenantcontroller/kubecontroller/crdreconciler.go @@ -5,6 +5,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" + "github.com/Azure/azure-container-networking/cns/restserver" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" @@ -16,7 +17,7 @@ import ( type CrdReconciler struct { KubeClient KubeClient NodeName string - CNSClient cnsclient + CNSRestService *restserver.HTTPRestService IPAMPoolMonitor cns.IPAMPoolMonitor } @@ -61,13 +62,15 @@ func (r *CrdReconciler) Reconcile(ctx context.Context, request reconcile.Request return reconcile.Result{}, err } - if err = r.CNSClient.CreateOrUpdateNC(ncRequest); err != nil { + responseCode := r.CNSRestService.CreateOrUpdateNetworkContainerInternal(ncRequest) + err = restserver.ResponseCodeToError(responseCode) + if err != nil { logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) // requeue return reconcile.Result{}, err } - r.CNSClient.UpdateIPAMPoolMonitor(nnc.Status.Scaler, nnc.Spec) + r.CNSRestService.IPAMPoolMonitor.Update(nnc.Status.Scaler, nnc.Spec) // record assigned IPs metric assignedIPs.Set(float64(len(nnc.Status.NetworkContainers[0].IPAssignments))) diff --git a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go index 4eb8a51d5c..94a7b553df 100644 --- a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go +++ b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go @@ -7,11 +7,11 @@ import ( "sync" "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/client/httpapi" "github.com/Azure/azure-container-networking/cns/cnireconciler" "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/restserver" "github.com/Azure/azure-container-networking/cns/singletenantcontroller" + "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/crd" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" "github.com/pkg/errors" @@ -42,6 +42,11 @@ type Config struct { var _ singletenantcontroller.RequestController = (*requestController)(nil) +type cnsrestservice interface { + ReconcileNCState(*cns.CreateNetworkContainerRequest, map[string]cns.PodInfo, v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) types.ResponseCode + CreateOrUpdateNetworkContainerInternal(cns.CreateNetworkContainerRequest) types.ResponseCode +} + // requestController // - watches CRD status changes // - updates CRD spec @@ -51,7 +56,7 @@ type requestController struct { KubeClient KubeClient // KubeClient is a cached client which interacts with API server directAPIClient DirectAPIClient // Direct client to interact with API server directCRDClient DirectCRDClient // Direct client to interact with CRDs on API server - CNSClient cnsclient + CNSRestService cnsrestservice nodeName string // name of node running this program Reconciler *CrdReconciler initialized bool @@ -121,16 +126,11 @@ func New(cfg Config) (*requestController, error) { return nil, err } - // Create httpClient - httpClient := &httpapi.Client{ - RestService: cfg.Service, - } - // Create reconciler crdreconciler := &CrdReconciler{ - KubeClient: mgr.GetClient(), - NodeName: nodeName, - CNSClient: httpClient, + KubeClient: mgr.GetClient(), + NodeName: nodeName, + CNSRestService: cfg.Service, } // Setup manager with reconciler @@ -146,7 +146,7 @@ func New(cfg Config) (*requestController, error) { KubeClient: mgr.GetClient(), directAPIClient: directAPIClient, directCRDClient: directCRDClient, - CNSClient: httpClient, + CNSRestService: cfg.Service, nodeName: nodeName, Reconciler: crdreconciler, } @@ -220,10 +220,10 @@ func (rc *requestController) initCNS(ctx context.Context) error { return nil } - // If instance of crd is not found, pass nil to cnsclient + // If instance of crd is not found, pass nil to CNSRestService if client.IgnoreNotFound(err) == nil { //nolint:wrapcheck - return rc.CNSClient.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + return restserver.ResponseCodeToError(rc.CNSRestService.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec)) } // If it's any other error, log it and return @@ -231,10 +231,10 @@ func (rc *requestController) initCNS(ctx context.Context) error { return err } - // If there are no NCs, pass nil to cnsclient + // If there are no NCs, pass nil to CNSRestService if len(nnc.Status.NetworkContainers) == 0 { //nolint:wrapcheck - return rc.CNSClient.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec) + return restserver.ResponseCodeToError(rc.CNSRestService.ReconcileNCState(nil, nil, nnc.Status.Scaler, nnc.Spec)) } // Convert to CreateNetworkContainerRequest @@ -272,8 +272,8 @@ func (rc *requestController) initCNS(ctx context.Context) error { } // errors.Wrap provides additional context, and return nil if the err input arg is nil - // Call cnsclient init cns passing those two things. - return errors.Wrap(rc.CNSClient.ReconcileNCState(&ncRequest, podInfoByIP, nnc.Status.Scaler, nnc.Spec), "err in CNS reconciliation") + // Call CNSRestService init cns passing those two things. + return errors.Wrap(restserver.ResponseCodeToError(rc.CNSRestService.ReconcileNCState(&ncRequest, podInfoByIP, nnc.Status.Scaler, nnc.Spec)), "err in CNS reconciliation") } // kubePodsToPodInfoByIP maps kubernetes pods to cns.PodInfos by IP diff --git a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go index 6760c6abe4..a5f302a777 100644 --- a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go +++ b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" + "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -88,8 +89,8 @@ func (mc MockKubeClient) Update(ctx context.Context, obj client.Object, opts ... return nil } -// MockCNSClient implements API client interface -type MockCNSClient struct { +// MockCNSRestServer implements CNSRestServer interface +type MockCNSRestService struct { MockCNSUpdated bool MockCNSInitialized bool Pods map[string]cns.PodInfo @@ -97,27 +98,16 @@ type MockCNSClient struct { } // we're just testing that reconciler interacts with CNS on Reconcile(). -func (mi *MockCNSClient) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error { - mi.MockCNSUpdated = true - return nil -} - -func (mi *MockCNSClient) UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) { +func (m *MockCNSRestService) CreateOrUpdateNetworkContainerInternal(ncRequest cns.CreateNetworkContainerRequest) types.ResponseCode { + m.MockCNSUpdated = true + return types.Success } -func (mi *MockCNSClient) DeleteNC(nc cns.DeleteNetworkContainerRequest) error { - return nil -} - -func (mi *MockCNSClient) GetNC(nc cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, error) { - return cns.GetNetworkContainerResponse{NetworkContainerID: nc.NetworkContainerid}, nil -} - -func (mi *MockCNSClient) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error { - mi.MockCNSInitialized = true - mi.Pods = podInfoByIP - mi.NCRequest = ncRequest - return nil +func (m *MockCNSRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) types.ResponseCode { + m.MockCNSInitialized = true + m.Pods = podInfoByIP + m.NCRequest = ncRequest + return types.Success } // MockDirectCRDClient implements the DirectCRDClient interface @@ -651,12 +641,12 @@ func TestInitRequestController(t *testing.T) { mockCRDDirectClient := &MockDirectCRDClient{ mockAPI: mockAPI, } - mockCNSClient := &MockCNSClient{} + mockCNSRestService := &MockCNSRestService{} rc := &requestController{ cfg: Config{}, directAPIClient: mockAPIDirectClient, directCRDClient: mockCRDDirectClient, - CNSClient: mockCNSClient, + CNSRestService: mockCNSRestService, nodeName: existingNNCName, } @@ -666,19 +656,19 @@ func TestInitRequestController(t *testing.T) { t.Fatalf("Expected no failure to init cns when given mock clients") } - if !mockCNSClient.MockCNSInitialized { + if !mockCNSRestService.MockCNSInitialized { t.Fatalf("MockCNSClient should have been initialized on request controller init") } - if _, ok := mockCNSClient.Pods[mockPodHostNetwork.Status.PodIP]; ok { + if _, ok := mockCNSRestService.Pods[mockPodHostNetwork.Status.PodIP]; ok { t.Fatalf("Init shouldn't pass cns pods that are part of host network") } - if _, ok := mockCNSClient.Pods[mockPod.Status.PodIP]; !ok { + if _, ok := mockCNSRestService.Pods[mockPod.Status.PodIP]; !ok { t.Fatalf("Init should pass cns pods that aren't part of host network") } - if _, ok := mockCNSClient.NCRequest.SecondaryIPConfigs[allocatedUUID]; !ok { + if _, ok := mockCNSRestService.NCRequest.SecondaryIPConfigs[allocatedUUID]; !ok { t.Fatalf("Expected secondary ip config to be in ncrequest") } } From 51307fea4b7c3d1a82a574cadcd22bf80cf03051 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Mon, 20 Sep 2021 12:09:47 -0700 Subject: [PATCH 2/8] lint: fixing lint errors --- cns/client/client_test.go | 2 +- .../mockclients/cnsclient_generated.go | 2 +- .../multitenantcrdreconciler.go | 4 ++-- cns/restserver/internalapi.go | 8 +++---- cns/restserver/internalapi_test.go | 22 +++++++++---------- .../kubecontroller/crdreconciler.go | 2 +- .../kubecontroller/crdrequestcontroller.go | 2 +- .../crdrequestcontroller_test.go | 7 ++++-- 8 files changed, 26 insertions(+), 23 deletions(-) diff --git a/cns/client/client_test.go b/cns/client/client_test.go index 87a12ec0c2..d55b820f1b 100644 --- a/cns/client/client_test.go +++ b/cns/client/client_test.go @@ -82,7 +82,7 @@ func addTestStateToRestServer(t *testing.T, secondaryIps []string) { secondaryIPConfigs[ipId.String()] = secIpConfig } - req := cns.CreateNetworkContainerRequest{ + req := &cns.CreateNetworkContainerRequest{ NetworkContainerType: dockerContainerType, NetworkContainerid: "testNcId1", IPConfiguration: ipConfig, diff --git a/cns/multitenantcontroller/mockclients/cnsclient_generated.go b/cns/multitenantcontroller/mockclients/cnsclient_generated.go index e706e2ac99..a4dc139b8b 100644 --- a/cns/multitenantcontroller/mockclients/cnsclient_generated.go +++ b/cns/multitenantcontroller/mockclients/cnsclient_generated.go @@ -40,7 +40,7 @@ func (m *MockCNSRestService) EXPECT() *MockCNSRestServiceMockRecorder { } // CreateOrUpdateNetworkContainerInternal mocks base method. -func (m *MockCNSRestService) CreateOrUpdateNetworkContainerInternal(nc cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { +func (m *MockCNSRestService) CreateOrUpdateNetworkContainerInternal(nc *cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "CreateOrUpdateNetworkContainerInternal", nc) ret0, _ := ret[0].(cnstypes.ResponseCode) diff --git a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go index d4bff38ad9..c8bc3120e1 100644 --- a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go +++ b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go @@ -33,7 +33,7 @@ const ( type cnsrestservice interface { DeleteNetworkContainerInternal(cns.DeleteNetworkContainerRequest) types.ResponseCode GetNetworkContainerInternal(cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, types.ResponseCode) - CreateOrUpdateNetworkContainerInternal(cns.CreateNetworkContainerRequest) types.ResponseCode + CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) types.ResponseCode } // multiTenantCrdReconciler reconciles multi-tenant network containers. @@ -131,7 +131,7 @@ func (r *multiTenantCrdReconciler) Reconcile(ctx context.Context, request reconc return ctrl.Result{}, err } prefixLength, _ := ipNet.Mask.Size() - networkContainerRequest := cns.CreateNetworkContainerRequest{ + networkContainerRequest := &cns.CreateNetworkContainerRequest{ NetworkContainerid: nc.Spec.UUID, OrchestratorContext: orchestratorContext, NetworkContainerType: cns.Kubernetes, diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index a59eb29afd..41f84e8fce 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -221,7 +221,7 @@ func (service *HTTPRestService) ReconcileNCState( } // If the NC was created successfully, then reconcile the allocated pod state - returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest) + returnCode := service.CreateOrUpdateNetworkContainerInternal(ncRequest) if returnCode != types.Success { return returnCode } @@ -303,7 +303,7 @@ func (service *HTTPRestService) DeleteNetworkContainerInternal( // This API will be called by CNS RequestController on CRD update. func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal( - req cns.CreateNetworkContainerRequest, + req *cns.CreateNetworkContainerRequest, ) types.ResponseCode { if req.NetworkContainerid == "" { logger.Errorf("[Azure CNS] Error. NetworkContainerid is empty") @@ -344,11 +344,11 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal( } // This will Create Or Update the NC state. - returnCode, returnMessage := service.saveNetworkContainerGoalState(req) + returnCode, returnMessage := service.saveNetworkContainerGoalState(*req) // If the NC was created successfully, log NC snapshot. if returnCode == 0 { - logNCSnapshot(req) + logNCSnapshot(*req) } else { logger.Errorf(returnMessage) } diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go index a272059d8d..2e8609acdf 100644 --- a/cns/restserver/internalapi_test.go +++ b/cns/restserver/internalapi_test.go @@ -231,12 +231,12 @@ func TestReconcileNCWithExistingState(t *testing.T) { } expectedNcCount := len(svc.state.ContainerStatus) - returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) + returnCode := svc.ReconcileNCState(req, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) if returnCode != types.Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } - validateNCStateAfterReconcile(t, &req, expectedNcCount+1, expectedAllocatedPods) + validateNCStateAfterReconcile(t, req, expectedNcCount+1, expectedAllocatedPods) } func TestReconcileNCWithExistingStateFromInterfaceID(t *testing.T) { @@ -264,12 +264,12 @@ func TestReconcileNCWithExistingStateFromInterfaceID(t *testing.T) { } expectedNcCount := len(svc.state.ContainerStatus) - returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) + returnCode := svc.ReconcileNCState(req, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) if returnCode != types.Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } - validateNCStateAfterReconcile(t, &req, expectedNcCount+1, expectedAllocatedPods) + validateNCStateAfterReconcile(t, req, expectedNcCount+1, expectedAllocatedPods) } func TestReconcileNCWithSystemPods(t *testing.T) { @@ -296,13 +296,13 @@ func TestReconcileNCWithSystemPods(t *testing.T) { expectedAllocatedPods["192.168.0.1"] = cns.NewPodInfo("", "", "systempod", "kube-system") expectedNcCount := len(svc.state.ContainerStatus) - returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) + returnCode := svc.ReconcileNCState(req, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) if returnCode != types.Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } delete(expectedAllocatedPods, "192.168.0.1") - validateNCStateAfterReconcile(t, &req, expectedNcCount, expectedAllocatedPods) + validateNCStateAfterReconcile(t, req, expectedNcCount, expectedAllocatedPods) } func setOrchestratorTypeInternal(orchestratorType string) { @@ -385,7 +385,7 @@ func createAndValidateNCRequest(t *testing.T, secondaryIPConfigs map[string]cns. svc.IPAMPoolMonitor.Update( fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) - validateNetworkRequest(t, req) + validateNetworkRequest(t, *req) } // Validate the networkRequest is persisted. @@ -453,7 +453,7 @@ func validateNetworkRequest(t *testing.T, req cns.CreateNetworkContainerRequest) } } -func generateNetworkContainerRequest(secondaryIps map[string]cns.SecondaryIPConfig, ncId string, ncVersion string) cns.CreateNetworkContainerRequest { +func generateNetworkContainerRequest(secondaryIps map[string]cns.SecondaryIPConfig, ncID, ncVersion string) *cns.CreateNetworkContainerRequest { var ipConfig cns.IPConfiguration ipConfig.DNSServers = dnsservers ipConfig.GatewayIPAddress = gatewayIp @@ -464,7 +464,7 @@ func generateNetworkContainerRequest(secondaryIps map[string]cns.SecondaryIPConf req := cns.CreateNetworkContainerRequest{ NetworkContainerType: dockerContainerType, - NetworkContainerid: ncId, + NetworkContainerid: ncID, IPConfiguration: ipConfig, Version: ncVersion, } @@ -479,7 +479,7 @@ func generateNetworkContainerRequest(secondaryIps map[string]cns.SecondaryIPConf fmt.Printf("NC Request %+v", req) - return req + return &req } func validateNCStateAfterReconcile(t *testing.T, ncRequest *cns.CreateNetworkContainerRequest, expectedNcCount int, expectedAllocatedPods map[string]cns.PodInfo) { @@ -549,7 +549,7 @@ func createNCReqInternal(t *testing.T, secondaryIPConfigs map[string]cns.Seconda svc.IPAMPoolMonitor.Update( fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) - return req + return *req } func restartService() { diff --git a/cns/singletenantcontroller/kubecontroller/crdreconciler.go b/cns/singletenantcontroller/kubecontroller/crdreconciler.go index 035d6db851..9981355fc1 100644 --- a/cns/singletenantcontroller/kubecontroller/crdreconciler.go +++ b/cns/singletenantcontroller/kubecontroller/crdreconciler.go @@ -62,7 +62,7 @@ func (r *CrdReconciler) Reconcile(ctx context.Context, request reconcile.Request return reconcile.Result{}, err } - responseCode := r.CNSRestService.CreateOrUpdateNetworkContainerInternal(ncRequest) + responseCode := r.CNSRestService.CreateOrUpdateNetworkContainerInternal(&ncRequest) err = restserver.ResponseCodeToError(responseCode) if err != nil { logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) diff --git a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go index 94a7b553df..b1d7830f85 100644 --- a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go +++ b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller.go @@ -44,7 +44,7 @@ var _ singletenantcontroller.RequestController = (*requestController)(nil) type cnsrestservice interface { ReconcileNCState(*cns.CreateNetworkContainerRequest, map[string]cns.PodInfo, v1alpha.Scaler, v1alpha.NodeNetworkConfigSpec) types.ResponseCode - CreateOrUpdateNetworkContainerInternal(cns.CreateNetworkContainerRequest) types.ResponseCode + CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) types.ResponseCode } // requestController diff --git a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go index a5f302a777..fc42b91089 100644 --- a/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go +++ b/cns/singletenantcontroller/kubecontroller/crdrequestcontroller_test.go @@ -98,12 +98,15 @@ type MockCNSRestService struct { } // we're just testing that reconciler interacts with CNS on Reconcile(). -func (m *MockCNSRestService) CreateOrUpdateNetworkContainerInternal(ncRequest cns.CreateNetworkContainerRequest) types.ResponseCode { +func (m *MockCNSRestService) CreateOrUpdateNetworkContainerInternal(ncRequest *cns.CreateNetworkContainerRequest) types.ResponseCode { m.MockCNSUpdated = true return types.Success } -func (m *MockCNSRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) types.ResponseCode { +func (m *MockCNSRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, + podInfoByIP map[string]cns.PodInfo, + scalar v1alpha.Scaler, + spec v1alpha.NodeNetworkConfigSpec) types.ResponseCode { m.MockCNSInitialized = true m.Pods = podInfoByIP m.NCRequest = ncRequest From 1b3dfab9ac07782524dbc4f025cf3762d9c50ec1 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Mon, 20 Sep 2021 12:16:15 -0700 Subject: [PATCH 3/8] chore: uts for new method --- cns/restserver/errors_test.go | 38 +++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 cns/restserver/errors_test.go diff --git a/cns/restserver/errors_test.go b/cns/restserver/errors_test.go new file mode 100644 index 0000000000..37884c6b87 --- /dev/null +++ b/cns/restserver/errors_test.go @@ -0,0 +1,38 @@ +package restserver + +import ( + "testing" + + "github.com/Azure/azure-container-networking/cns/types" + "github.com/stretchr/testify/assert" +) + +func TestResponseCodeToError(t *testing.T) { + tests := []struct { + name string + responseCode types.ResponseCode + wantErr bool + }{ + { + name: "ok to nil", + responseCode: types.Success, + wantErr: false, + }, + { + name: "anything but ok to error", + responseCode: types.UnknownContainerID, + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + err := ResponseCodeToError(tt.responseCode) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} From 323d8d85c7c57f5d967b51ef1e3ae95c4fc1aa19 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Mon, 20 Sep 2021 16:39:04 -0700 Subject: [PATCH 4/8] chore: regenerated mock code for multitenant scenario --- .../mockclients/cnsrestservice.go | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 cns/multitenantcontroller/mockclients/cnsrestservice.go diff --git a/cns/multitenantcontroller/mockclients/cnsrestservice.go b/cns/multitenantcontroller/mockclients/cnsrestservice.go new file mode 100644 index 0000000000..19b29c72f0 --- /dev/null +++ b/cns/multitenantcontroller/mockclients/cnsrestservice.go @@ -0,0 +1,79 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: /root/go/src/github.com/Azure/azure-container-networking/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go + +// Package mockclients is a generated GoMock package. +package mockclients + +import ( + reflect "reflect" + + cns "github.com/Azure/azure-container-networking/cns" + types "github.com/Azure/azure-container-networking/cns/types" + gomock "github.com/golang/mock/gomock" +) + +// Mockcnsrestservice is a mock of cnsrestservice interface. +type Mockcnsrestservice struct { + ctrl *gomock.Controller + recorder *MockcnsrestserviceMockRecorder +} + +// MockcnsrestserviceMockRecorder is the mock recorder for Mockcnsrestservice. +type MockcnsrestserviceMockRecorder struct { + mock *Mockcnsrestservice +} + +// NewMockcnsrestservice creates a new mock instance. +func NewMockcnsrestservice(ctrl *gomock.Controller) *Mockcnsrestservice { + mock := &Mockcnsrestservice{ctrl: ctrl} + mock.recorder = &MockcnsrestserviceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *Mockcnsrestservice) EXPECT() *MockcnsrestserviceMockRecorder { + return m.recorder +} + +// CreateOrUpdateNetworkContainerInternal mocks base method. +func (m *Mockcnsrestservice) CreateOrUpdateNetworkContainerInternal(arg0 *cns.CreateNetworkContainerRequest) types.ResponseCode { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateOrUpdateNetworkContainerInternal", arg0) + ret0, _ := ret[0].(types.ResponseCode) + return ret0 +} + +// CreateOrUpdateNetworkContainerInternal indicates an expected call of CreateOrUpdateNetworkContainerInternal. +func (mr *MockcnsrestserviceMockRecorder) CreateOrUpdateNetworkContainerInternal(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateNetworkContainerInternal", reflect.TypeOf((*Mockcnsrestservice)(nil).CreateOrUpdateNetworkContainerInternal), arg0) +} + +// DeleteNetworkContainerInternal mocks base method. +func (m *Mockcnsrestservice) DeleteNetworkContainerInternal(arg0 cns.DeleteNetworkContainerRequest) types.ResponseCode { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteNetworkContainerInternal", arg0) + ret0, _ := ret[0].(types.ResponseCode) + return ret0 +} + +// DeleteNetworkContainerInternal indicates an expected call of DeleteNetworkContainerInternal. +func (mr *MockcnsrestserviceMockRecorder) DeleteNetworkContainerInternal(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNetworkContainerInternal", reflect.TypeOf((*Mockcnsrestservice)(nil).DeleteNetworkContainerInternal), arg0) +} + +// GetNetworkContainerInternal mocks base method. +func (m *Mockcnsrestservice) GetNetworkContainerInternal(arg0 cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, types.ResponseCode) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetNetworkContainerInternal", arg0) + ret0, _ := ret[0].(cns.GetNetworkContainerResponse) + ret1, _ := ret[1].(types.ResponseCode) + return ret0, ret1 +} + +// GetNetworkContainerInternal indicates an expected call of GetNetworkContainerInternal. +func (mr *MockcnsrestserviceMockRecorder) GetNetworkContainerInternal(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkContainerInternal", reflect.TypeOf((*Mockcnsrestservice)(nil).GetNetworkContainerInternal), arg0) +} From a00aa243cce9f52e5cee71441e8c17a18145f242 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Tue, 21 Sep 2021 09:49:39 -0700 Subject: [PATCH 5/8] chore: deleting old generated code --- .../mockclients/cnsclient_generated.go | 109 ------------------ .../multitenantcrdreconciler_test.go | 4 +- 2 files changed, 2 insertions(+), 111 deletions(-) delete mode 100644 cns/multitenantcontroller/mockclients/cnsclient_generated.go diff --git a/cns/multitenantcontroller/mockclients/cnsclient_generated.go b/cns/multitenantcontroller/mockclients/cnsclient_generated.go deleted file mode 100644 index a4dc139b8b..0000000000 --- a/cns/multitenantcontroller/mockclients/cnsclient_generated.go +++ /dev/null @@ -1,109 +0,0 @@ -//go:build !ignore_uncovered -// +build !ignore_uncovered - -// Code generated by MockGen. DO NOT EDIT. -// Source: cns/cnsclient/apiclient.go - -// Package mockclients is a generated GoMock package. -package mockclients - -import ( - reflect "reflect" - - cns "github.com/Azure/azure-container-networking/cns" - cnstypes "github.com/Azure/azure-container-networking/cns/types" - v1alpha "github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha" - gomock "github.com/golang/mock/gomock" -) - -// MockCNSRestService is a mock of CNS Rest Server. -type MockCNSRestService struct { - ctrl *gomock.Controller - recorder *MockCNSRestServiceMockRecorder -} - -// MockAPIClientMockRecorder is the mock recorder for MockAPIClient. -type MockCNSRestServiceMockRecorder struct { - mock *MockCNSRestService -} - -// NewMockAPIClient creates a new mock instance. -func NewMockCNSRestServer(ctrl *gomock.Controller) *MockCNSRestService { - mock := &MockCNSRestService{ctrl: ctrl} - mock.recorder = &MockCNSRestServiceMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockCNSRestService) EXPECT() *MockCNSRestServiceMockRecorder { - return m.recorder -} - -// CreateOrUpdateNetworkContainerInternal mocks base method. -func (m *MockCNSRestService) CreateOrUpdateNetworkContainerInternal(nc *cns.CreateNetworkContainerRequest) cnstypes.ResponseCode { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateNetworkContainerInternal", nc) - ret0, _ := ret[0].(cnstypes.ResponseCode) - return ret0 -} - -// CreateOrUpdateNetworkContainerInternal indicates an expected call of CreateOrUpdateNetworkContainerInternal. -func (mr *MockCNSRestServiceMockRecorder) CreateOrUpdateNetworkContainerInternal(nc interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateNetworkContainerInternal", reflect.TypeOf((*MockCNSRestService)(nil).CreateOrUpdateNetworkContainerInternal), nc) -} - -// DeleteNetworkContainerInternal mocks base method. -func (m *MockCNSRestService) DeleteNetworkContainerInternal(nc cns.DeleteNetworkContainerRequest) cnstypes.ResponseCode { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteNetworkContainerInternal", nc) - ret0, _ := ret[0].(cnstypes.ResponseCode) - return ret0 -} - -// DeleteNetworkContainerInternal indicates an expected call of DeleteNetworkContainerInternal. -func (mr *MockCNSRestServiceMockRecorder) DeleteNetworkContainerInternal(nc interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNetworkContainerInternal", reflect.TypeOf((*MockCNSRestService)(nil).DeleteNetworkContainerInternal), nc) -} - -// GetNetworkContainerInternal mocks base method. -func (m *MockCNSRestService) GetNetworkContainerInternal(nc cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, cnstypes.ResponseCode) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetNetworkContainerInternal", nc) - ret0, _ := ret[0].(cns.GetNetworkContainerResponse) - ret1, _ := ret[1].(cnstypes.ResponseCode) - return ret0, ret1 -} - -// GetNetworkContainerInternal indicates an expected call of GetNetworkContainerInternal. -func (mr *MockCNSRestServiceMockRecorder) GetNetworkContainerInternal(nc interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkContainerInternal", reflect.TypeOf((*MockCNSRestService)(nil).GetNetworkContainerInternal), nc) -} - -// ReconcileNCState mocks base method. -func (m *MockCNSRestService) ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.PodInfo, scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ReconcileNCState", nc, pods, scalar, spec) - ret0, _ := ret[0].(error) - return ret0 -} - -// ReconcileNCState indicates an expected call of ReconcileNCState. -func (mr *MockCNSRestServiceMockRecorder) ReconcileNCState(nc, pods, scalar, spec interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReconcileNCState", reflect.TypeOf((*MockCNSRestService)(nil).ReconcileNCState), nc, pods, scalar, spec) -} - -// UpdateIPAMPoolMonitor mocks base method. -func (m *MockCNSRestService) UpdateIPAMPoolMonitor(scalar v1alpha.Scaler, spec v1alpha.NodeNetworkConfigSpec) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "UpdateIPAMPoolMonitor", scalar, spec) -} - -// UpdateIPAMPoolMonitor indicates an expected call of UpdateIPAMPoolMonitor. -func (mr *MockCNSRestServiceMockRecorder) UpdateIPAMPoolMonitor(scalar, spec interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateIPAMPoolMonitor", reflect.TypeOf((*MockCNSRestService)(nil).UpdateIPAMPoolMonitor), scalar, spec) -} diff --git a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go index 7f6292b0ef..b6a0a19f6f 100644 --- a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go +++ b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go @@ -20,7 +20,7 @@ import ( var _ = Describe("multiTenantCrdReconciler", func() { var kubeClient *mockclients.MockClient - var cnsRestService *mockclients.MockCNSRestService + var cnsRestService *mockclients.Mockcnsrestservice var mockCtl *gomock.Controller var reconciler *multiTenantCrdReconciler const uuidValue = "uuid" @@ -38,7 +38,7 @@ var _ = Describe("multiTenantCrdReconciler", func() { logger.InitLogger("multiTenantCrdReconciler", 0, 0, "") mockCtl = gomock.NewController(GinkgoT()) kubeClient = mockclients.NewMockClient(mockCtl) - cnsRestService = mockclients.NewMockCNSRestServer(mockCtl) + cnsRestService = mockclients.NewMockcnsrestservice(mockCtl) reconciler = &multiTenantCrdReconciler{ KubeClient: kubeClient, NodeName: mockNodeName, From 9fbce2971c04736a9542a5a1185d619b4f57d217 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Tue, 21 Sep 2021 09:56:53 -0700 Subject: [PATCH 6/8] chore: make new generated files with _generated suffix --- cns/multitenantcontroller/mockclients/Makefile | 4 ++-- .../{cnsrestservice.go => cnsrestservice_generated.go} | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename cns/multitenantcontroller/mockclients/{cnsrestservice.go => cnsrestservice_generated.go} (95%) diff --git a/cns/multitenantcontroller/mockclients/Makefile b/cns/multitenantcontroller/mockclients/Makefile index 8644ff2aba..3a04439898 100644 --- a/cns/multitenantcontroller/mockclients/Makefile +++ b/cns/multitenantcontroller/mockclients/Makefile @@ -6,9 +6,9 @@ MOCKGEN = $(TOOLS_BIN_DIR)/mockgen .PHONY: generate generate: $(MOCKGEN) ## Generate mock clients - $(MOCKGEN) -source=$(REPO_ROOT)/cns/cnsclient/apiclient.go -package=mockclients APIClient > cnsclient_generated.go + $(MOCKGEN) -source=$(REPO_ROOT)/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go -package=mockclients cnsrestservice > cnsrestservice_generated.go $(MOCKGEN) -source=$(REPO_ROOT)/vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.go -package=mockclients Client > kubeclient_generated.go - @sed -i s,$(REPO_ROOT)/,,g cnsclient_generated.go kubeclient_generated.go + @sed -i s,$(REPO_ROOT)/,,g cnsrestservice_generated.go kubeclient_generated.go $(MOCKGEN): @make -C $(REPO_ROOT) $(MOCKGEN) diff --git a/cns/multitenantcontroller/mockclients/cnsrestservice.go b/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go similarity index 95% rename from cns/multitenantcontroller/mockclients/cnsrestservice.go rename to cns/multitenantcontroller/mockclients/cnsrestservice_generated.go index 19b29c72f0..42c1300260 100644 --- a/cns/multitenantcontroller/mockclients/cnsrestservice.go +++ b/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go @@ -1,5 +1,5 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: /root/go/src/github.com/Azure/azure-container-networking/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go +// Source: cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go // Package mockclients is a generated GoMock package. package mockclients From 433e60366b6dd4d1192123de3aa285731abdae4a Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Tue, 21 Sep 2021 10:06:46 -0700 Subject: [PATCH 7/8] feedback: making cnsrestservice cnsRESTservice --- .../mockclients/Makefile | 2 +- .../mockclients/cnsrestservice_generated.go | 43 ++++++++++--------- .../multitenantcrdreconciler.go | 4 +- .../multitenantcrdreconciler_test.go | 4 +- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/cns/multitenantcontroller/mockclients/Makefile b/cns/multitenantcontroller/mockclients/Makefile index 3a04439898..b07821e988 100644 --- a/cns/multitenantcontroller/mockclients/Makefile +++ b/cns/multitenantcontroller/mockclients/Makefile @@ -6,7 +6,7 @@ MOCKGEN = $(TOOLS_BIN_DIR)/mockgen .PHONY: generate generate: $(MOCKGEN) ## Generate mock clients - $(MOCKGEN) -source=$(REPO_ROOT)/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go -package=mockclients cnsrestservice > cnsrestservice_generated.go + $(MOCKGEN) -source=$(REPO_ROOT)/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go -package=mockclients cnsRESTservice > cnsrestservice_generated.go $(MOCKGEN) -source=$(REPO_ROOT)/vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.go -package=mockclients Client > kubeclient_generated.go @sed -i s,$(REPO_ROOT)/,,g cnsrestservice_generated.go kubeclient_generated.go diff --git a/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go b/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go index 42c1300260..5e9233acda 100644 --- a/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go +++ b/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go @@ -1,3 +1,6 @@ +//go:build !ignore_uncovered +// +build !ignore_uncovered + // Code generated by MockGen. DO NOT EDIT. // Source: cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go @@ -12,31 +15,31 @@ import ( gomock "github.com/golang/mock/gomock" ) -// Mockcnsrestservice is a mock of cnsrestservice interface. -type Mockcnsrestservice struct { +// MockcnsRESTservice is a mock of cnsRESTservice interface. +type MockcnsRESTservice struct { ctrl *gomock.Controller - recorder *MockcnsrestserviceMockRecorder + recorder *MockcnsRESTserviceMockRecorder } -// MockcnsrestserviceMockRecorder is the mock recorder for Mockcnsrestservice. -type MockcnsrestserviceMockRecorder struct { - mock *Mockcnsrestservice +// MockcnsRESTserviceMockRecorder is the mock recorder for MockcnsRESTservice. +type MockcnsRESTserviceMockRecorder struct { + mock *MockcnsRESTservice } -// NewMockcnsrestservice creates a new mock instance. -func NewMockcnsrestservice(ctrl *gomock.Controller) *Mockcnsrestservice { - mock := &Mockcnsrestservice{ctrl: ctrl} - mock.recorder = &MockcnsrestserviceMockRecorder{mock} +// NewMockcnsRESTservice creates a new mock instance. +func NewMockcnsRESTservice(ctrl *gomock.Controller) *MockcnsRESTservice { + mock := &MockcnsRESTservice{ctrl: ctrl} + mock.recorder = &MockcnsRESTserviceMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *Mockcnsrestservice) EXPECT() *MockcnsrestserviceMockRecorder { +func (m *MockcnsRESTservice) EXPECT() *MockcnsRESTserviceMockRecorder { return m.recorder } // CreateOrUpdateNetworkContainerInternal mocks base method. -func (m *Mockcnsrestservice) CreateOrUpdateNetworkContainerInternal(arg0 *cns.CreateNetworkContainerRequest) types.ResponseCode { +func (m *MockcnsRESTservice) CreateOrUpdateNetworkContainerInternal(arg0 *cns.CreateNetworkContainerRequest) types.ResponseCode { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "CreateOrUpdateNetworkContainerInternal", arg0) ret0, _ := ret[0].(types.ResponseCode) @@ -44,13 +47,13 @@ func (m *Mockcnsrestservice) CreateOrUpdateNetworkContainerInternal(arg0 *cns.Cr } // CreateOrUpdateNetworkContainerInternal indicates an expected call of CreateOrUpdateNetworkContainerInternal. -func (mr *MockcnsrestserviceMockRecorder) CreateOrUpdateNetworkContainerInternal(arg0 interface{}) *gomock.Call { +func (mr *MockcnsRESTserviceMockRecorder) CreateOrUpdateNetworkContainerInternal(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateNetworkContainerInternal", reflect.TypeOf((*Mockcnsrestservice)(nil).CreateOrUpdateNetworkContainerInternal), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateNetworkContainerInternal", reflect.TypeOf((*MockcnsRESTservice)(nil).CreateOrUpdateNetworkContainerInternal), arg0) } // DeleteNetworkContainerInternal mocks base method. -func (m *Mockcnsrestservice) DeleteNetworkContainerInternal(arg0 cns.DeleteNetworkContainerRequest) types.ResponseCode { +func (m *MockcnsRESTservice) DeleteNetworkContainerInternal(arg0 cns.DeleteNetworkContainerRequest) types.ResponseCode { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "DeleteNetworkContainerInternal", arg0) ret0, _ := ret[0].(types.ResponseCode) @@ -58,13 +61,13 @@ func (m *Mockcnsrestservice) DeleteNetworkContainerInternal(arg0 cns.DeleteNetwo } // DeleteNetworkContainerInternal indicates an expected call of DeleteNetworkContainerInternal. -func (mr *MockcnsrestserviceMockRecorder) DeleteNetworkContainerInternal(arg0 interface{}) *gomock.Call { +func (mr *MockcnsRESTserviceMockRecorder) DeleteNetworkContainerInternal(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNetworkContainerInternal", reflect.TypeOf((*Mockcnsrestservice)(nil).DeleteNetworkContainerInternal), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNetworkContainerInternal", reflect.TypeOf((*MockcnsRESTservice)(nil).DeleteNetworkContainerInternal), arg0) } // GetNetworkContainerInternal mocks base method. -func (m *Mockcnsrestservice) GetNetworkContainerInternal(arg0 cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, types.ResponseCode) { +func (m *MockcnsRESTservice) GetNetworkContainerInternal(arg0 cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, types.ResponseCode) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetNetworkContainerInternal", arg0) ret0, _ := ret[0].(cns.GetNetworkContainerResponse) @@ -73,7 +76,7 @@ func (m *Mockcnsrestservice) GetNetworkContainerInternal(arg0 cns.GetNetworkCont } // GetNetworkContainerInternal indicates an expected call of GetNetworkContainerInternal. -func (mr *MockcnsrestserviceMockRecorder) GetNetworkContainerInternal(arg0 interface{}) *gomock.Call { +func (mr *MockcnsRESTserviceMockRecorder) GetNetworkContainerInternal(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkContainerInternal", reflect.TypeOf((*Mockcnsrestservice)(nil).GetNetworkContainerInternal), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkContainerInternal", reflect.TypeOf((*MockcnsRESTservice)(nil).GetNetworkContainerInternal), arg0) } diff --git a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go index c8bc3120e1..0e3b1f1b8f 100644 --- a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go +++ b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler.go @@ -30,7 +30,7 @@ const ( NCStateTerminated = "Terminated" ) -type cnsrestservice interface { +type cnsRESTservice interface { DeleteNetworkContainerInternal(cns.DeleteNetworkContainerRequest) types.ResponseCode GetNetworkContainerInternal(cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, types.ResponseCode) CreateOrUpdateNetworkContainerInternal(*cns.CreateNetworkContainerRequest) types.ResponseCode @@ -40,7 +40,7 @@ type cnsrestservice interface { type multiTenantCrdReconciler struct { KubeClient client.Client NodeName string - CNSRestService cnsrestservice + CNSRestService cnsRESTservice } // Reconcile is called on multi-tenant CRD status changes. diff --git a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go index b6a0a19f6f..fa0a8d1116 100644 --- a/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go +++ b/cns/multitenantcontroller/multitenantoperator/multitenantcrdreconciler_test.go @@ -20,7 +20,7 @@ import ( var _ = Describe("multiTenantCrdReconciler", func() { var kubeClient *mockclients.MockClient - var cnsRestService *mockclients.Mockcnsrestservice + var cnsRestService *mockclients.MockcnsRESTservice var mockCtl *gomock.Controller var reconciler *multiTenantCrdReconciler const uuidValue = "uuid" @@ -38,7 +38,7 @@ var _ = Describe("multiTenantCrdReconciler", func() { logger.InitLogger("multiTenantCrdReconciler", 0, 0, "") mockCtl = gomock.NewController(GinkgoT()) kubeClient = mockclients.NewMockClient(mockCtl) - cnsRestService = mockclients.NewMockcnsrestservice(mockCtl) + cnsRestService = mockclients.NewMockcnsRESTservice(mockCtl) reconciler = &multiTenantCrdReconciler{ KubeClient: kubeClient, NodeName: mockNodeName, From 801de93de5df51f7e6e0aa3b4b58e894ce39f5fc Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Tue, 21 Sep 2021 10:23:03 -0700 Subject: [PATCH 8/8] feedback: making CNSRESTError struct --- cns/restserver/errors.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/cns/restserver/errors.go b/cns/restserver/errors.go index f4fd878d2b..010675af07 100644 --- a/cns/restserver/errors.go +++ b/cns/restserver/errors.go @@ -1,18 +1,27 @@ package restserver import ( - "errors" "fmt" "github.com/Azure/azure-container-networking/cns/types" ) -var errResponseCode = errors.New("Response code is error") +// CNSRESTError represents a CNS error +type CNSRESTError struct { + ResponseCode types.ResponseCode +} + +func (c *CNSRESTError) Error() string { + return fmt.Sprintf("response code: %s", c.ResponseCode.String()) +} // ResponseCodeToError converts a cns response code to error type. If the response code is OK, then return value is nil func ResponseCodeToError(responseCode types.ResponseCode) error { - if responseCode == 0 { + if responseCode == types.Success { return nil } - return fmt.Errorf("%w: %v", errResponseCode, responseCode) + + return &CNSRESTError{ + ResponseCode: responseCode, + } }