Explicit error handling by introducing NotFound error types, and replacing nil with NotFound with checks in the caller where-ever required. (#244)

Author: runakash

.github/.codecov.yml

@@ -26,3 +26,4 @@ ignore:
   - "mocks/**/*"
   - "integration/shared/scenarios/**/*"
   - "pkg/common/logger.go"
+  - "test/*"

Makefile

@@ -121,7 +121,7 @@ build: test ## Build manager binary.
 	go build -ldflags="-s -w -X ${PKG}.GitVersion=${GIT_TAG} -X ${PKG}.GitCommit=${GIT_COMMIT}" -o bin/manager main.go
 
 run: test ## Run a controller from your host.
-	go run -ldflags="-s -w -X ${PKG}.GitVersion=${GIT_TAG} -X ${PKG}.GitCommit=${GIT_COMMIT}" ./main.go --zap-devel=true $(ARGS)
+	go run -ldflags="-s -w -X ${PKG}.GitVersion=${GIT_TAG} -X ${PKG}.GitCommit=${GIT_COMMIT}" ./main.go --zap-devel=true --zap-time-encoding=rfc3339 $(ARGS)
 
 docker-build: test ## Build docker image with the manager.
 	docker build --no-cache -t ${IMG} .

integration/janitor/janitor.go

@@ -6,6 +6,7 @@ import (
 	"os"
 
 	"github.com/aws/aws-cloud-map-mcs-controller-for-k8s/pkg/cloudmap"
+
 	"github.com/aws/aws-cloud-map-mcs-controller-for-k8s/pkg/model"
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/config"
@@ -81,7 +82,7 @@ func (j *cloudMapJanitor) deregisterInstances(ctx context.Context, nsName string
 		model.ClusterSetIdAttr: j.clusterSetId,
 	}
 
-	insts, err := j.sdApi.DiscoverInstances(ctx, nsName, svcName, &queryParameters)
+	insts, err := j.sdApi.DiscoverInstances(ctx, nsName, svcName, queryParameters)
 	j.checkOrFail(err,
 		fmt.Sprintf("service has %d instances to clean", len(insts)),
 		"could not list instances to cleanup")

integration/janitor/janitor_test.go

@@ -32,7 +32,7 @@ func TestCleanupHappyCase(t *testing.T) {
 		Return(map[string]*model.Namespace{test.HttpNsName: test.GetTestHttpNamespace()}, nil)
 	tj.mockApi.EXPECT().GetServiceIdMap(context.TODO(), test.HttpNsId).
 		Return(map[string]string{test.SvcName: test.SvcId}, nil)
-	tj.mockApi.EXPECT().DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, &map[string]string{
+	tj.mockApi.EXPECT().DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, map[string]string{
 		model.ClusterSetIdAttr: test.ClusterSet,
 	}).
 		Return([]types.HttpInstanceSummary{{InstanceId: aws.String(test.EndptId1)}}, nil)

integration/kind-test/scripts/run-tests.sh

@@ -20,7 +20,7 @@ if ! endpts=$(./integration/shared/scripts/poll-endpoints.sh "$EXPECTED_ENDPOINT
 fi
 
 mkdir -p "$LOGS"
-./bin/manager &> "$LOGS/ctl.log" &
+./bin/manager --zap-devel=true --zap-time-encoding=rfc3339 &> "$LOGS/ctl.log" &
 CTL_PID=$!
 echo "controller PID:$CTL_PID"
 

integration/shared/scenarios/export_service.go

@@ -7,6 +7,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/aws/aws-cloud-map-mcs-controller-for-k8s/pkg/common"
+
 	"github.com/aws/aws-cloud-map-mcs-controller-for-k8s/pkg/cloudmap"
 	"github.com/aws/aws-cloud-map-mcs-controller-for-k8s/pkg/model"
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -86,16 +88,16 @@ func (e *exportServiceScenario) Run() error {
 	return wait.Poll(defaultScenarioPollInterval, defaultScenarioPollTimeout, func() (done bool, err error) {
 		fmt.Println("Polling service...")
 		cmSvc, err := e.sdClient.GetService(context.TODO(), e.expectedSvc.Namespace, e.expectedSvc.Name)
-		if err != nil {
+		if common.IsUnknown(err) {
 			return true, err
 		}
 
-		if cmSvc == nil {
+		if common.IsNotFound(err) {
 			fmt.Println("Service not found.")
 			return false, nil
 		}
 
-		fmt.Printf("Found service: %v\n", cmSvc)
+		fmt.Printf("Found service: %+v\n", cmSvc)
 		return e.compareEndpoints(cmSvc.Endpoints), nil
 	})
 }

pkg/cloudmap/api.go

@@ -30,7 +30,7 @@ type ServiceDiscoveryApi interface {
 	GetServiceIdMap(ctx context.Context, namespaceId string) (serviceIdMap map[string]string, err error)
 
 	// DiscoverInstances returns a list of service instances registered to a given service.
-	DiscoverInstances(ctx context.Context, nsName string, svcName string, queryParameters *map[string]string) (insts []types.HttpInstanceSummary, err error)
+	DiscoverInstances(ctx context.Context, nsName string, svcName string, queryParameters map[string]string) (insts []types.HttpInstanceSummary, err error)
 
 	// ListOperations returns a map of operations to their status matching a list of filters.
 	ListOperations(ctx context.Context, opFilters []types.OperationFilter) (operationStatusMap map[string]types.OperationStatus, err error)
@@ -132,15 +132,15 @@ func (sdApi *serviceDiscoveryApi) GetServiceIdMap(ctx context.Context, nsId stri
 	return serviceIdMap, nil
 }
 
-func (sdApi *serviceDiscoveryApi) DiscoverInstances(ctx context.Context, nsName string, svcName string, queryParameters *map[string]string) (insts []types.HttpInstanceSummary, err error) {
+func (sdApi *serviceDiscoveryApi) DiscoverInstances(ctx context.Context, nsName string, svcName string, queryParameters map[string]string) (insts []types.HttpInstanceSummary, err error) {
 	input := &sd.DiscoverInstancesInput{
 		NamespaceName: aws.String(nsName),
 		ServiceName:   aws.String(svcName),
 		HealthStatus:  types.HealthStatusFilterAll,
 		MaxResults:    aws.Int32(1000),
 	}
 	if queryParameters != nil {
-		input.QueryParameters = *queryParameters
+		input.QueryParameters = queryParameters
 	}
 	out, err := sdApi.awsFacade.DiscoverInstances(ctx, input)
 

pkg/cloudmap/api_test.go

@@ -124,7 +124,7 @@ func TestServiceDiscoveryApi_DiscoverInstances_HappyCase(t *testing.T) {
 			},
 		}, nil)
 
-	insts, err := sdApi.DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, &map[string]string{model.ClusterSetIdAttr: test.ClusterSet})
+	insts, err := sdApi.DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, map[string]string{model.ClusterSetIdAttr: test.ClusterSet})
 	assert.Nil(t, err, "No error for happy case")
 	assert.True(t, len(insts) == 2)
 	assert.Equal(t, test.EndptId1, *insts[0].InstanceId)

pkg/cloudmap/client.go

@@ -40,7 +40,7 @@ type serviceDiscoveryClient struct {
 // from a given AWS client config.
 func NewDefaultServiceDiscoveryClient(cfg *aws.Config, clusterUtils model.ClusterUtils) ServiceDiscoveryClient {
 	return &serviceDiscoveryClient{
-		log:          common.NewLogger("cloudmap"),
+		log:          common.NewLogger("cloudmap", "client"),
 		sdApi:        NewServiceDiscoveryApiFromConfig(cfg),
 		cache:        NewDefaultServiceDiscoveryClientCache(),
 		clusterUtils: clusterUtils,
@@ -49,7 +49,7 @@ func NewDefaultServiceDiscoveryClient(cfg *aws.Config, clusterUtils model.Cluste
 
 func NewServiceDiscoveryClientWithCustomCache(cfg *aws.Config, cacheConfig *SdCacheConfig, clusterUtils model.ClusterUtils) ServiceDiscoveryClient {
 	return &serviceDiscoveryClient{
-		log:          common.NewLogger("cloudmap"),
+		log:          common.NewLogger("cloudmap", "client"),
 		sdApi:        NewServiceDiscoveryApiFromConfig(cfg),
 		cache:        NewServiceDiscoveryClientCache(cacheConfig),
 		clusterUtils: clusterUtils,
@@ -59,6 +59,10 @@ func NewServiceDiscoveryClientWithCustomCache(cfg *aws.Config, cacheConfig *SdCa
 func (sdc *serviceDiscoveryClient) ListServices(ctx context.Context, nsName string) (svcs []*model.Service, err error) {
 	svcIdMap, err := sdc.getServiceIds(ctx, nsName)
 	if err != nil {
+		// Ignore resource not found error, as it will indicate deleted resources in CloudMap
+		if common.IsNotFound(err) {
+			return svcs, nil
+		}
 		return svcs, err
 	}
 
@@ -81,13 +85,13 @@ func (sdc *serviceDiscoveryClient) ListServices(ctx context.Context, nsName stri
 func (sdc *serviceDiscoveryClient) CreateService(ctx context.Context, nsName string, svcName string) error {
 	sdc.log.Info("creating a new service", "namespace", nsName, "name", svcName)
 
-	nsMap, err := sdc.getNamespaces(ctx)
-	if err != nil {
+	namespace, err := sdc.getNamespace(ctx, nsName)
+	if common.IsUnknown(err) {
 		return err
 	}
 
-	namespace := nsMap[nsName]
-	if namespace == nil {
+	if common.IsNotFound(err) {
+		sdc.log.Info("namespace not found for service", "namespace", nsName, "service", svcName)
 		// Create HttpNamespace if the namespace is not present in CloudMap
 		namespace, err = sdc.createNamespace(ctx, nsName)
 		if err != nil {
@@ -107,27 +111,20 @@ func (sdc *serviceDiscoveryClient) CreateService(ctx context.Context, nsName str
 
 func (sdc *serviceDiscoveryClient) GetService(ctx context.Context, nsName string, svcName string) (svc *model.Service, err error) {
 	sdc.log.Info("fetching a service", "namespace", nsName, "name", svcName)
-	endpts, cacheHit := sdc.cache.GetEndpoints(nsName, svcName)
-
-	if cacheHit {
+	if endpts, found := sdc.cache.GetEndpoints(nsName, svcName); found {
 		return &model.Service{
 			Namespace: nsName,
 			Name:      svcName,
 			Endpoints: endpts,
 		}, nil
 	}
 
-	svcIdMap, err := sdc.getServiceIds(ctx, nsName)
+	_, err = sdc.getServiceId(ctx, nsName, svcName)
 	if err != nil {
 		return nil, err
 	}
-	_, found := svcIdMap[svcName]
-	if !found {
-		return nil, nil
-	}
-
-	endpts, err = sdc.getEndpoints(ctx, nsName, svcName)
 
+	endpts, err := sdc.getEndpoints(ctx, nsName, svcName)
 	if err != nil {
 		return nil, err
 	}
@@ -147,14 +144,10 @@ func (sdc *serviceDiscoveryClient) RegisterEndpoints(ctx context.Context, nsName
 
 	sdc.log.Info("registering endpoints", "namespaceName", nsName, "serviceName", svcName, "endpoints", endpts)
 
-	svcIdMap, err := sdc.getServiceIds(ctx, nsName)
+	svcId, err := sdc.getServiceId(ctx, nsName, svcName)
 	if err != nil {
 		return err
 	}
-	svcId, found := svcIdMap[svcName]
-	if !found {
-		return fmt.Errorf("service not found in Cloud Map: %s", svcName)
-	}
 
 	opCollector := NewOperationCollector()
 
@@ -190,14 +183,10 @@ func (sdc *serviceDiscoveryClient) DeleteEndpoints(ctx context.Context, nsName s
 
 	sdc.log.Info("deleting endpoints", "namespaceName", nsName, "serviceName", svcName, "endpoints", endpts)
 
-	svcIdMap, err := sdc.getServiceIds(ctx, nsName)
+	svcId, err := sdc.getServiceId(ctx, nsName, svcName)
 	if err != nil {
 		return err
 	}
-	svcId, found := svcIdMap[svcName]
-	if !found {
-		return fmt.Errorf("service not found in Cloud Map: %s", svcName)
-	}
 
 	opCollector := NewOperationCollector()
 
@@ -225,8 +214,8 @@ func (sdc *serviceDiscoveryClient) DeleteEndpoints(ctx context.Context, nsName s
 }
 
 func (sdc *serviceDiscoveryClient) getEndpoints(ctx context.Context, nsName string, svcName string) (endpts []*model.Endpoint, err error) {
-	endpts, cacheHit := sdc.cache.GetEndpoints(nsName, svcName)
-	if cacheHit {
+	endpts, found := sdc.cache.GetEndpoints(nsName, svcName)
+	if found {
 		return endpts, nil
 	}
 
@@ -239,7 +228,7 @@ func (sdc *serviceDiscoveryClient) getEndpoints(ctx context.Context, nsName stri
 	queryParameters := map[string]string{
 		model.ClusterSetIdAttr: clusterProperties.ClusterSetId(),
 	}
-	insts, err := sdc.sdApi.DiscoverInstances(ctx, nsName, svcName, &queryParameters)
+	insts, err := sdc.sdApi.DiscoverInstances(ctx, nsName, svcName, queryParameters)
 	if err != nil {
 		return nil, err
 	}
@@ -257,10 +246,23 @@ func (sdc *serviceDiscoveryClient) getEndpoints(ctx context.Context, nsName stri
 	return endpts, nil
 }
 
-func (sdc *serviceDiscoveryClient) getNamespaces(ctx context.Context) (namespace map[string]*model.Namespace, err error) {
+func (sdc *serviceDiscoveryClient) getNamespace(ctx context.Context, nsName string) (namespace *model.Namespace, err error) {
+	namespaces, err := sdc.getNamespaces(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	if namespace, ok := namespaces[nsName]; ok {
+		return namespace, nil
+	}
+
+	return nil, common.NotFoundError(fmt.Sprintf("namespace: %s", nsName))
+}
+
+func (sdc *serviceDiscoveryClient) getNamespaces(ctx context.Context) (namespaces map[string]*model.Namespace, err error) {
 	// We are assuming a unique namespace name per account
-	namespaces, cacheHit := sdc.cache.GetNamespaceMap()
-	if cacheHit {
+	namespaces, found := sdc.cache.GetNamespaceMap()
+	if found {
 		return namespaces, nil
 	}
 
@@ -273,15 +275,27 @@ func (sdc *serviceDiscoveryClient) getNamespaces(ctx context.Context) (namespace
 	return namespaces, nil
 }
 
+func (sdc *serviceDiscoveryClient) getServiceId(ctx context.Context, nsName string, svcName string) (svcId string, err error) {
+	svcIdMap, err := sdc.getServiceIds(ctx, nsName)
+	if err != nil {
+		return "", err
+	}
+
+	if svcId, ok := svcIdMap[svcName]; ok {
+		return svcId, nil
+	}
+
+	return "", common.NotFoundError(fmt.Sprintf("service: %s", svcName))
+}
+
 func (sdc *serviceDiscoveryClient) getServiceIds(ctx context.Context, nsName string) (map[string]string, error) {
-	serviceIdMap, cacheHit := sdc.cache.GetServiceIdMap(nsName)
-	if cacheHit {
+	serviceIdMap, found := sdc.cache.GetServiceIdMap(nsName)
+	if found {
 		return serviceIdMap, nil
 	}
 
-	nsMap, err := sdc.getNamespaces(ctx)
-	namespace := nsMap[nsName]
-	if err != nil || namespace == nil {
+	namespace, err := sdc.getNamespace(ctx, nsName)
+	if err != nil {
 		return nil, err
 	}
 

pkg/cloudmap/client_test.go

@@ -48,7 +48,7 @@ func TestServiceDiscoveryClient_ListServices_HappyCase(t *testing.T) {
 	tc.mockCache.EXPECT().CacheServiceIdMap(test.HttpNsName, getServiceIdMapForTest())
 
 	tc.mockCache.EXPECT().GetEndpoints(test.HttpNsName, test.SvcName).Return(nil, false)
-	tc.mockApi.EXPECT().DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, &map[string]string{
+	tc.mockApi.EXPECT().DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, map[string]string{
 		model.ClusterSetIdAttr: test.ClusterSet,
 	}).Return(getHttpInstanceSummaryForTest(), nil)
 
@@ -117,7 +117,7 @@ func TestServiceDiscoveryClient_ListServices_InstanceError(t *testing.T) {
 
 	endptErr := errors.New("error listing endpoints")
 	tc.mockCache.EXPECT().GetEndpoints(test.HttpNsName, test.SvcName).Return(nil, false)
-	tc.mockApi.EXPECT().DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, &map[string]string{
+	tc.mockApi.EXPECT().DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, map[string]string{
 		model.ClusterSetIdAttr: test.ClusterSet,
 	}).
 		Return([]types.HttpInstanceSummary{}, endptErr)
@@ -180,6 +180,23 @@ func TestServiceDiscoveryClient_CreateService_NamespaceError(t *testing.T) {
 	assert.Equal(t, nsErr, err)
 }
 
+func TestServiceDiscoveryClient_CreateService_NamespaceNotFound(t *testing.T) {
+	tc := getTestSdClient(t)
+	defer tc.close()
+
+	tc.mockCache.EXPECT().GetNamespaceMap().Return(map[string]*model.Namespace{}, true)
+	tc.mockApi.EXPECT().CreateHttpNamespace(context.TODO(), test.HttpNsName).Return(test.OpId1, nil)
+	tc.mockApi.EXPECT().PollNamespaceOperation(context.TODO(), test.OpId1).Return(test.HttpNsId, nil)
+	tc.mockCache.EXPECT().EvictNamespaceMap()
+
+	tc.mockApi.EXPECT().CreateService(context.TODO(), *test.GetTestHttpNamespace(), test.SvcName).
+		Return(test.SvcId, nil)
+	tc.mockCache.EXPECT().EvictServiceIdMap(test.HttpNsName)
+
+	err := tc.client.CreateService(context.TODO(), test.HttpNsName, test.SvcName)
+	assert.Nil(t, err)
+}
+
 func TestServiceDiscoveryClient_CreateService_CreateServiceError(t *testing.T) {
 	tc := getTestSdClient(t)
 	defer tc.close()
@@ -264,7 +281,7 @@ func TestServiceDiscoveryClient_GetService_HappyCase(t *testing.T) {
 	tc.mockCache.EXPECT().CacheServiceIdMap(test.HttpNsName, getServiceIdMapForTest())
 
 	tc.mockCache.EXPECT().GetEndpoints(test.HttpNsName, test.SvcName).Return([]*model.Endpoint{}, false)
-	tc.mockApi.EXPECT().DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, &map[string]string{
+	tc.mockApi.EXPECT().DiscoverInstances(context.TODO(), test.HttpNsName, test.SvcName, map[string]string{
 		model.ClusterSetIdAttr: test.ClusterSet,
 	}).
 		Return(getHttpInstanceSummaryForTest(), nil)
@@ -288,6 +305,30 @@ func TestServiceDiscoveryClient_GetService_CachedValues(t *testing.T) {
 	assert.Equal(t, test.GetTestService(), svc)
 }
 
+func TestServiceDiscoveryClient_GetService_ServiceNotFound(t *testing.T) {
+	tc := getTestSdClient(t)
+	defer tc.close()
+
+	tc.mockCache.EXPECT().GetEndpoints(test.HttpNsName, test.SvcName).Return(nil, false)
+
+	tc.mockCache.EXPECT().GetServiceIdMap(test.HttpNsName).Return(nil, false)
+
+	tc.mockCache.EXPECT().GetNamespaceMap().Return(nil, false)
+	tc.mockApi.EXPECT().GetNamespaceMap(context.TODO()).
+		Return(getNamespaceMapForTest(), nil)
+	tc.mockCache.EXPECT().CacheNamespaceMap(getNamespaceMapForTest())
+
+	// return empty list from CloudMap's api
+	tc.mockApi.EXPECT().GetServiceIdMap(context.TODO(), test.HttpNsId).
+		Return(map[string]string{}, nil)
+	tc.mockCache.EXPECT().CacheServiceIdMap(test.HttpNsName, map[string]string{})
+
+	svc, err := tc.client.GetService(context.TODO(), test.HttpNsName, test.SvcName)
+	assert.NotNil(t, err)
+	assert.True(t, common.IsNotFound(err), svc)
+	assert.Contains(t, err.Error(), test.SvcName)
+}
+
 func TestServiceDiscoveryClient_RegisterEndpoints(t *testing.T) {
 	tc := getTestSdClient(t)
 	defer tc.close()