Skip to content

Commit a06ecf7

Browse files
runakashrunakash
committed
Addressing PR feedbacks. Nil to denote the k8s namespaces. Removing the logging for unsupported namespace types. Adding more tests.
1 parent 13c4a6a commit a06ecf7

File tree

5 files changed

+106
-41
lines changed

5 files changed

+106
-41
lines changed

‎pkg/cloudmap/api.go‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,14 +75,12 @@ func (sdApi *serviceDiscoveryApi) ListNamespaces(ctx context.Context) ([]*model.
7575
}
7676

7777
for _, ns := range output.Namespaces {
78-
if namespaceType, ok := model.ConvertNamespaceType(ns.Type); ok {
78+
if namespaceType := model.ConvertNamespaceType(ns.Type); !namespaceType.IsUnsupported() {
7979
namespaces = append(namespaces, &model.Namespace{
8080
Id: aws.ToString(ns.Id),
8181
Name: aws.ToString(ns.Name),
82-
Type: *namespaceType,
82+
Type: namespaceType,
8383
})
84-
} else {
85-
sdApi.log.Info("skipping the unsupported namespace type", "namespace", aws.ToString(ns.Name), "type", ns.Type)
8684
}
8785
}
8886
}
@@ -211,11 +209,10 @@ func (sdApi *serviceDiscoveryApi) CreateService(ctx context.Context, namespace m
211209
}
212210

213211
func (sdApi *serviceDiscoveryApi) getDnsConfig() types.DnsConfig {
214-
ttl := defaultServiceTTLInSeconds
215212
dnsConfig := types.DnsConfig{
216213
DnsRecords: []types.DnsRecord{
217214
{
218-
TTL: &ttl,
215+
TTL: aws.Int64(defaultServiceTTLInSeconds),
219216
Type: "SRV",
220217
},
221218
},

‎pkg/cloudmap/api_test.go‎

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package cloudmap
22

33
import (
44
"context"
5+
"fmt"
56
"github.com/aws/aws-cloud-map-mcs-controller-for-k8s/mocks/pkg/cloudmap"
67
"github.com/aws/aws-cloud-map-mcs-controller-for-k8s/test"
78
"github.com/aws/aws-sdk-go-v2/aws"
@@ -18,7 +19,7 @@ func TestNewServiceDiscoveryApi(t *testing.T) {
1819
assert.NotNil(t, sdc)
1920
}
2021

21-
func TestListNamespaces_HappyCase(t *testing.T) {
22+
func TestServiceDiscoveryApi_ListNamespaces_HappyCase(t *testing.T) {
2223
mockController := gomock.NewController(t)
2324
defer mockController.Finish()
2425

@@ -39,7 +40,7 @@ func TestListNamespaces_HappyCase(t *testing.T) {
3940
assert.Equal(t, test.GetTestDnsNamespace(), namespaces[0], "")
4041
}
4142

42-
func TestListNamespaces_SkipPublicDNSNotSupported(t *testing.T) {
43+
func TestServiceDiscoveryApi_ListNamespaces_SkipPublicDNSNotSupported(t *testing.T) {
4344
mockController := gomock.NewController(t)
4445
defer mockController.Finish()
4546

@@ -59,6 +60,75 @@ func TestListNamespaces_SkipPublicDNSNotSupported(t *testing.T) {
5960
assert.True(t, len(namespaces) == 0, "Successfully skipped DNS_PUBLIC from the output")
6061
}
6162

63+
func TestServiceDiscoveryApi_CreateService_CreateForHttpNamespace(t *testing.T) {
64+
mockController := gomock.NewController(t)
65+
defer mockController.Finish()
66+
67+
awsFacade := cloudmap.NewMockAwsFacade(mockController)
68+
sdApi := getServiceDiscoveryApi(t, awsFacade)
69+
70+
nsId, svcId, svcName := test.NsId, test.SvcId, test.SvcName
71+
awsFacade.EXPECT().CreateService(context.TODO(), &sd.CreateServiceInput{
72+
Name: &svcName,
73+
NamespaceId: &nsId,
74+
}).
75+
Return(&sd.CreateServiceOutput{
76+
Service: &types.Service{
77+
Id: &svcId,
78+
},
79+
}, nil)
80+
81+
retSvcId, _ := sdApi.CreateService(context.TODO(), *test.GetTestHttpNamespace(), svcName)
82+
assert.Equal(t, svcId, retSvcId, "Successfully created service")
83+
}
84+
85+
func TestServiceDiscoveryApi_CreateService_CreateForDnsNamespace(t *testing.T) {
86+
mockController := gomock.NewController(t)
87+
defer mockController.Finish()
88+
89+
awsFacade := cloudmap.NewMockAwsFacade(mockController)
90+
sdApi := getServiceDiscoveryApi(t, awsFacade)
91+
92+
nsId, svcId, svcName := test.NsId, test.SvcId, test.SvcName
93+
awsFacade.EXPECT().CreateService(context.TODO(), &sd.CreateServiceInput{
94+
Name: &svcName,
95+
NamespaceId: &nsId,
96+
DnsConfig: &types.DnsConfig{
97+
DnsRecords: []types.DnsRecord{{
98+
TTL: aws.Int64(60),
99+
Type: "SRV",
100+
}},
101+
},
102+
}).
103+
Return(&sd.CreateServiceOutput{
104+
Service: &types.Service{
105+
Id: &svcId,
106+
},
107+
}, nil)
108+
109+
retSvcId, _ := sdApi.CreateService(context.TODO(), *test.GetTestDnsNamespace(), svcName)
110+
assert.Equal(t, svcId, retSvcId, "Successfully created service")
111+
}
112+
113+
func TestServiceDiscoveryApi_CreateService_ThrowError(t *testing.T) {
114+
mockController := gomock.NewController(t)
115+
defer mockController.Finish()
116+
117+
awsFacade := cloudmap.NewMockAwsFacade(mockController)
118+
sdApi := getServiceDiscoveryApi(t, awsFacade)
119+
120+
nsId, svcName := test.NsId, test.SvcName
121+
awsFacade.EXPECT().CreateService(context.TODO(), &sd.CreateServiceInput{
122+
Name: &svcName,
123+
NamespaceId: &nsId,
124+
}).
125+
Return(nil, fmt.Errorf("dummy error"))
126+
127+
retSvcId, err := sdApi.CreateService(context.TODO(), *test.GetTestHttpNamespace(), svcName)
128+
assert.Empty(t, retSvcId)
129+
assert.Equal(t, "dummy error", fmt.Sprint(err), "Got error")
130+
}
131+
62132
func getServiceDiscoveryApi(t *testing.T, awsFacade *cloudmap.MockAwsFacade) serviceDiscoveryApi {
63133
return serviceDiscoveryApi{
64134
log: testingLogger.TestLogger{T: t},

‎pkg/cloudmap/client.go‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import (
1414
const (
1515
defaultNamespaceCacheTTL = 2 * time.Minute
1616
defaultNamespaceCacheSize = 100
17-
emptyNamespaceCacheTTL = 2 * time.Minute
1817
defaultServiceIdCacheTTL = 2 * time.Minute
1918
defaultServiceIdCacheSize = 1024
2019
defaultEndpointsCacheTTL = 5 * time.Second
@@ -61,7 +60,7 @@ func NewServiceDiscoveryClient(cfg *aws.Config) ServiceDiscoveryClient {
6160

6261
func (sdc *serviceDiscoveryClient) ListServices(ctx context.Context, nsName string) (svcs []*model.Service, err error) {
6362
namespace, err := sdc.getNamespace(ctx, nsName)
64-
if err != nil || namespace.IsEmpty() {
63+
if err != nil || namespace == nil {
6564
return svcs, err
6665
}
6766

@@ -96,7 +95,7 @@ func (sdc *serviceDiscoveryClient) CreateService(ctx context.Context, nsName str
9695
return err
9796
}
9897

99-
if namespace.IsEmpty() {
98+
if namespace == nil {
10099
// Create HttpNamespace if the namespace is not present in the CloudMap
101100
namespace, err = sdc.createNamespace(ctx, nsName)
102101
if err != nil {
@@ -234,10 +233,12 @@ func (sdc *serviceDiscoveryClient) listEndpoints(ctx context.Context, serviceId
234233

235234
func (sdc *serviceDiscoveryClient) getNamespace(ctx context.Context, nsName string) (namespace *model.Namespace, err error) {
236235
// We are assuming a unique namespace name per account
237-
exists, namespace, err := sdc.getCachedNamespace(nsName)
238-
if exists {
236+
if namespace, exists, err := sdc.getCachedNamespace(nsName); exists {
239237
return namespace, err
240238
}
239+
if err != nil {
240+
return nil, err
241+
}
241242

242243
namespaces, err := sdc.sdApi.ListNamespaces(ctx)
243244
if err != nil {
@@ -255,7 +256,7 @@ func (sdc *serviceDiscoveryClient) getNamespace(ctx context.Context, nsName stri
255256
if namespace == nil {
256257
// This will cache empty namespace for namespaces not in Cloud Map
257258
// This is so that we can avoid ListNamespaces call
258-
namespace = sdc.cacheEmptyNamespace(nsName)
259+
sdc.cacheNilNamespace(nsName)
259260
}
260261

261262
return namespace, nil
@@ -269,7 +270,7 @@ func (sdc *serviceDiscoveryClient) getServiceId(ctx context.Context, nsName stri
269270
}
270271

271272
namespace, err := sdc.getNamespace(ctx, nsName)
272-
if err != nil || namespace.IsEmpty() {
273+
if err != nil || namespace == nil {
273274
return "", err
274275
}
275276

@@ -311,25 +312,26 @@ func (sdc *serviceDiscoveryClient) createNamespace(ctx context.Context, nsName s
311312
return namespace, nil
312313
}
313314

314-
func (sdc *serviceDiscoveryClient) getCachedNamespace(nsName string) (exists bool, namespace *model.Namespace, err error) {
315+
func (sdc *serviceDiscoveryClient) getCachedNamespace(nsName string) (namespace *model.Namespace, exists bool, err error) {
315316
if cachedValue, exists := sdc.namespaceCache.Get(nsName); exists {
317+
if cachedValue == nil {
318+
return nil, exists, nil
319+
}
316320
ns, ok := cachedValue.(model.Namespace)
317321
if !ok {
318-
return exists, nil, fmt.Errorf("failed to cast the cached value for the namespace %s", nsName)
322+
return nil, exists, fmt.Errorf("failed to cast the cached value for the namespace %s", nsName)
319323
}
320-
return exists, &ns, nil
324+
return &ns, exists, nil
321325
}
322-
return exists, nil, nil
326+
return nil, exists, nil
323327
}
324328

325329
func (sdc *serviceDiscoveryClient) cacheNamespace(namespace model.Namespace) {
326330
sdc.namespaceCache.Add(namespace.Name, namespace, defaultNamespaceCacheTTL)
327331
}
328332

329-
func (sdc *serviceDiscoveryClient) cacheEmptyNamespace(nsName string) *model.Namespace {
330-
emptyNamespace := model.GetEmptyNamespace()
331-
sdc.namespaceCache.Add(nsName, emptyNamespace, emptyNamespaceCacheTTL)
332-
return &emptyNamespace
333+
func (sdc *serviceDiscoveryClient) cacheNilNamespace(nsName string) {
334+
sdc.namespaceCache.Add(nsName, nil, defaultNamespaceCacheTTL)
333335
}
334336

335337
func (sdc *serviceDiscoveryClient) cacheServiceId(nsName string, svcName string, svcId string) {

‎pkg/cloudmap/client_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ func TestServiceDiscoveryClient_ListServices_NamespaceNotFound(t *testing.T) {
130130

131131
cachedNs, found := sdc.namespaceCache.Get(test.NsName)
132132
assert.True(t, found)
133-
assert.Equal(t, model.Namespace{}, cachedNs, "Empty Namespace found in the cache")
133+
assert.Nil(t, cachedNs, "Namespace not found in the cache")
134134
}
135135

136136
func TestServiceDiscoveryClient_CreateService_HappyCase(t *testing.T) {
@@ -307,18 +307,18 @@ func TestServiceDiscoveryClient_getNamespace_GetEmptyNamespace(t *testing.T) {
307307
sdApi := cloudmap.NewMockServiceDiscoveryApi(mockController)
308308

309309
sdc := getTestSdClient(t, sdApi)
310-
sdc.namespaceCache.Add(test.NsName, model.GetEmptyNamespace(), time.Minute)
310+
sdc.namespaceCache.Add(test.NsName, nil, time.Minute)
311311

312312
namespace, err := sdc.getNamespace(context.TODO(), test.NsName)
313-
assert.Equal(t, model.Namespace{}, *namespace, "Empty namespace found in the cache")
313+
assert.Nil(t, namespace, "Namespace not found in the cache")
314314
assert.Nil(t, err, "No errors with empty namespace")
315315
}
316316

317317
func TestServiceDiscoveryClient_getCachedNamespace_ErrorCasting(t *testing.T) {
318318
sdc := getTestSdClient(t, nil)
319319
sdc.namespaceCache.Add(test.NsName, struct{ dummy string }{"dummy"}, time.Minute)
320320

321-
exists, namespace, err := sdc.getCachedNamespace(test.NsName)
321+
namespace, exists, err := sdc.getCachedNamespace(test.NsName)
322322
assert.True(t, exists, "Cache exists")
323323
assert.Nil(t, namespace, "No corresponding cached value found")
324324
assert.Equal(t, fmt.Sprintf("failed to cast the cached value for the namespace %s", test.NsName), fmt.Sprint(err), "Got the error for improper casting")

‎pkg/model/types.go‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ type Resource struct {
1919
const (
2020
HttpNamespaceType NamespaceType = "HTTP"
2121
DnsPrivateNamespaceType NamespaceType = "DNS_PRIVATE"
22+
// UnsupportedNamespaceType Placeholder NamespaceType to denote not supported values
23+
UnsupportedNamespaceType NamespaceType = ""
2224
)
2325

2426
type NamespaceType string
@@ -50,14 +52,6 @@ const (
5052
PortAttr = "AWS_INSTANCE_PORT"
5153
)
5254

53-
func (ns Namespace) IsEmpty() bool {
54-
return ns == Namespace{}
55-
}
56-
57-
func GetEmptyNamespace() Namespace {
58-
return Namespace{}
59-
}
60-
6155
// NewEndpointFromInstance converts a Cloud Map InstanceSummary to an endpoint.
6256
func NewEndpointFromInstance(inst *types.InstanceSummary) (*Endpoint, error) {
6357
endpoint := Endpoint{
@@ -128,15 +122,17 @@ func EndpointIdFromIPAddress(address string) string {
128122
return strings.Replace(address, ".", "_", -1)
129123
}
130124

131-
func ConvertNamespaceType(nsType types.NamespaceType) (namespaceType *NamespaceType, ok bool) {
132-
dnsPrivate := DnsPrivateNamespaceType
133-
http := HttpNamespaceType
125+
func ConvertNamespaceType(nsType types.NamespaceType) (namespaceType NamespaceType) {
134126
switch nsType {
135127
case types.NamespaceTypeDnsPrivate:
136-
return &dnsPrivate, true
128+
return DnsPrivateNamespaceType
137129
case types.NamespaceTypeHttp:
138-
return &http, true
130+
return HttpNamespaceType
139131
default:
140-
return nil, false
132+
return UnsupportedNamespaceType
141133
}
142134
}
135+
136+
func (namespaceType *NamespaceType) IsUnsupported() bool {
137+
return *namespaceType == UnsupportedNamespaceType
138+
}

0 commit comments

Comments
 (0)