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/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/Makefile b/cns/multitenantcontroller/mockclients/Makefile index 8644ff2aba..b07821e988 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/cnsclient_generated.go b/cns/multitenantcontroller/mockclients/cnsclient_generated.go deleted file mode 100644 index 0069c6940a..0000000000 --- a/cns/multitenantcontroller/mockclients/cnsclient_generated.go +++ /dev/null @@ -1,108 +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" - 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 { - ctrl *gomock.Controller - recorder *MockAPIClientMockRecorder -} - -// MockAPIClientMockRecorder is the mock recorder for MockAPIClient. -type MockAPIClientMockRecorder struct { - mock *MockAPIClient -} - -// NewMockAPIClient creates a new mock instance. -func NewMockAPIClient(ctrl *gomock.Controller) *MockAPIClient { - mock := &MockAPIClient{ctrl: ctrl} - mock.recorder = &MockAPIClientMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockAPIClient) EXPECT() *MockAPIClientMockRecorder { - return m.recorder -} - -// CreateOrUpdateNC mocks base method. -func (m *MockAPIClient) CreateOrUpdateNC(nc cns.CreateNetworkContainerRequest) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateOrUpdateNC", nc) - ret0, _ := ret[0].(error) - return ret0 -} - -// CreateOrUpdateNC indicates an expected call of CreateOrUpdateNC. -func (mr *MockAPIClientMockRecorder) CreateOrUpdateNC(nc interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateNC", reflect.TypeOf((*MockAPIClient)(nil).CreateOrUpdateNC), nc) -} - -// DeleteNC mocks base method. -func (m *MockAPIClient) DeleteNC(nc cns.DeleteNetworkContainerRequest) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteNC", nc) - ret0, _ := ret[0].(error) - return ret0 -} - -// DeleteNC indicates an expected call of DeleteNC. -func (mr *MockAPIClientMockRecorder) DeleteNC(nc interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNC", reflect.TypeOf((*MockAPIClient)(nil).DeleteNC), nc) -} - -// GetNC mocks base method. -func (m *MockAPIClient) GetNC(nc cns.GetNetworkContainerRequest) (cns.GetNetworkContainerResponse, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetNC", nc) - ret0, _ := ret[0].(cns.GetNetworkContainerResponse) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetNC indicates an expected call of GetNC. -func (mr *MockAPIClientMockRecorder) GetNC(nc interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNC", reflect.TypeOf((*MockAPIClient)(nil).GetNC), nc) -} - -// ReconcileNCState mocks base method. -func (m *MockAPIClient) 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 *MockAPIClientMockRecorder) 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) -} - -// UpdateIPAMPoolMonitor mocks base method. -func (m *MockAPIClient) 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 { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateIPAMPoolMonitor", reflect.TypeOf((*MockAPIClient)(nil).UpdateIPAMPoolMonitor), scalar, spec) -} diff --git a/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go b/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go new file mode 100644 index 0000000000..5e9233acda --- /dev/null +++ b/cns/multitenantcontroller/mockclients/cnsrestservice_generated.go @@ -0,0 +1,82 @@ +//go:build !ignore_uncovered +// +build !ignore_uncovered + +// Code generated by MockGen. DO NOT EDIT. +// Source: 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) +} 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..0e3b1f1b8f 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 @@ -122,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, @@ -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..fa0a8d1116 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.NewMockcnsRESTservice(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..010675af07 --- /dev/null +++ b/cns/restserver/errors.go @@ -0,0 +1,27 @@ +package restserver + +import ( + "fmt" + + "github.com/Azure/azure-container-networking/cns/types" +) + +// 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 == types.Success { + return nil + } + + return &CNSRESTError{ + ResponseCode: responseCode, + } +} 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) + } + }) + } +} 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/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..9981355fc1 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..b1d7830f85 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..fc42b91089 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,19 @@ 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 +644,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 +659,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") } }