Skip to content

Commit

Permalink
refactor: revert to NamespaceFilter interface
Browse files Browse the repository at this point in the history
  • Loading branch information
marcsanmi committed Jun 7, 2022
1 parent e57ba69 commit 95f8b9d
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 53 deletions.
44 changes: 10 additions & 34 deletions internal/discovery/namespace_filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

"github.com/newrelic/nri-kubernetes/v3/internal/config"
"github.com/newrelic/nri-kubernetes/v3/internal/storer"
"github.com/newrelic/nri-kubernetes/v3/src/definition"

log "github.com/sirupsen/logrus"
v1 "k8s.io/api/core/v1"
Expand All @@ -18,9 +17,9 @@ import (

const defaultNamespaceResyncDuration = 10 * time.Minute

// Filterer provider a interface to match from a given RawMetrics.
type Filterer interface {
Match(metrics definition.RawMetrics) bool
// NamespaceFilterer provides an interface to filter from a given namespace.
type NamespaceFilterer interface {
IsAllowed(namespace string) bool
}

// NamespaceFilter is a struct holding pointers to the config and the namespace lister.
Expand Down Expand Up @@ -50,20 +49,8 @@ func NewNamespaceFilter(c *config.NamespaceSelector, client kubernetes.Interface
}
}

// Match checks given any metrics, if a namespace it's allowed to be scraped given a certain match labels or
// expressions configuration.
func (nf *NamespaceFilter) Match(metrics definition.RawMetrics) bool {
namespaceRaw, ok := metrics["namespace"]
if !ok {
return true
}

namespace, ok := namespaceRaw.(string)
if !ok {
log.Tracef("Allowing %q as namespace as invalid type provided %t", namespaceRaw, namespaceRaw)
return true
}

// IsAllowed checks if a namespace is allowed to be scraped given a certain match labels or expressions configuration.
func (nf *NamespaceFilter) IsAllowed(namespace string) bool {
if nf.c == nil {
log.Tracef("Allowing %q namespace as selector is nil", namespace)
return true
Expand Down Expand Up @@ -129,38 +116,27 @@ func (nf *NamespaceFilter) Close() error {

// CachedNamespaceFilter is a wrapper of the NamespaceFilterer and the cache.
type CachedNamespaceFilter struct {
NsFilter Filterer
NsFilter NamespaceFilterer
cache storer.Storer
}

// NewCachedNamespaceFilter create a new CachedNamespaceFilter, wrapping the cache and the NamespaceFilterer.
func NewCachedNamespaceFilter(ns Filterer, storer storer.Storer) *CachedNamespaceFilter {
func NewCachedNamespaceFilter(ns NamespaceFilterer, storer storer.Storer) *CachedNamespaceFilter {
return &CachedNamespaceFilter{
NsFilter: ns,
cache: storer,
}
}

// Match check the cache and calls the underlying NamespaceFilter if the result is not found.
func (cnf *CachedNamespaceFilter) Match(metrics definition.RawMetrics) bool {
namespaceRaw, ok := metrics["namespace"]
if !ok {
return true
}

namespace, ok := namespaceRaw.(string)
if !ok {
log.Tracef("Allowing %q from cache as namespace invalid type provided %t", namespaceRaw, namespaceRaw)
return true
}

// IsAllowed check the cache and calls the underlying NamespaceFilter if the result is not found.
func (cnf *CachedNamespaceFilter) IsAllowed(namespace string) bool {
// Check if the namespace is already in the cache.
var allowed bool
if _, err := cnf.cache.Get(namespace, &allowed); err == nil {
return allowed
}

allowed = cnf.NsFilter.Match(metrics)
allowed = cnf.NsFilter.IsAllowed(namespace)

// Save the namespace in the cache.
_ = cnf.cache.Set(namespace, allowed)
Expand Down
33 changes: 14 additions & 19 deletions internal/discovery/namespace_filter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"github.com/newrelic/nri-kubernetes/v3/internal/config"
"github.com/newrelic/nri-kubernetes/v3/internal/discovery"
"github.com/newrelic/nri-kubernetes/v3/internal/storer"
"github.com/newrelic/nri-kubernetes/v3/src/definition"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"

Expand All @@ -24,8 +23,6 @@ const namespaceName = "test_namespace"
func TestNamespaceFilterer_IsAllowed(t *testing.T) {
t.Parallel()

metrics := definition.RawMetrics{"namespace": namespaceName}

type testData struct {
namespaceLabels labels.Set
namespaceSelector config.NamespaceSelector
Expand Down Expand Up @@ -157,16 +154,14 @@ func TestNamespaceFilterer_IsAllowed(t *testing.T) {
ns.Close()
})

require.Equal(t, testData.expected, ns.Match(metrics))
require.Equal(t, testData.expected, ns.IsAllowed(namespaceName))
})
}
}

func TestNamespaceFilterer_Cache(t *testing.T) {
t.Parallel()

metrics := definition.RawMetrics{"namespace": namespaceName}

type testData struct {
warmCache func(cache *storer.InMemoryStore)
prepare func(nsFilterMock *NamespaceFilterMock)
Expand All @@ -178,10 +173,10 @@ func TestNamespaceFilterer_Cache(t *testing.T) {
"namespace_cache_miss_fallback_to_call_informer": {
warmCache: func(cache *storer.InMemoryStore) {},
prepare: func(nsFilterMock *NamespaceFilterMock) {
nsFilterMock.On("Match", metrics).Return(true).Once()
nsFilterMock.On("IsAllowed", namespaceName).Return(true).Once()
},
assert: func(expected bool, cnsf *discovery.CachedNamespaceFilter) {
require.Equal(t, expected, cnsf.Match(metrics))
require.Equal(t, expected, cnsf.IsAllowed(namespaceName))
},
expected: true,
},
Expand All @@ -190,10 +185,10 @@ func TestNamespaceFilterer_Cache(t *testing.T) {
cache.Set(namespaceName, true)
},
prepare: func(nsFilterMock *NamespaceFilterMock) {
nsFilterMock.AssertNotCalled(t, "Match")
nsFilterMock.AssertNotCalled(t, "IsAllowed")
},
assert: func(expected bool, cnsf *discovery.CachedNamespaceFilter) {
require.Equal(t, expected, cnsf.Match(metrics))
require.Equal(t, expected, cnsf.IsAllowed(namespaceName))
},
expected: true,
},
Expand All @@ -202,21 +197,21 @@ func TestNamespaceFilterer_Cache(t *testing.T) {
cache.Set(namespaceName, false)
},
prepare: func(nsFilterMock *NamespaceFilterMock) {
nsFilterMock.AssertNotCalled(t, "Match")
nsFilterMock.AssertNotCalled(t, "IsAllowed")
},
assert: func(expected bool, cnsf *discovery.CachedNamespaceFilter) {
require.Equal(t, expected, cnsf.Match(metrics))
require.Equal(t, expected, cnsf.IsAllowed(namespaceName))
},
expected: false,
},
"namespace_cache_miss_subsequent_call_uses_cache": {
warmCache: func(cache *storer.InMemoryStore) {},
prepare: func(nsFilterMock *NamespaceFilterMock) {
nsFilterMock.On("Match", metrics).Return(true).Once()
nsFilterMock.On("IsAllowed", namespaceName).Return(true).Once()
},
assert: func(expected bool, cnsf *discovery.CachedNamespaceFilter) {
require.Equal(t, expected, cnsf.Match(metrics))
require.Equal(t, expected, cnsf.Match(metrics))
require.Equal(t, expected, cnsf.IsAllowed(namespaceName))
require.Equal(t, expected, cnsf.IsAllowed(namespaceName))
},
expected: true,
},
Expand Down Expand Up @@ -272,7 +267,7 @@ func TestNamespaceFilter_InformerCacheSync(t *testing.T) {
nil,
)
// Check that recently created namespace is not allowed.
require.Equal(t, false, ns.Match(definition.RawMetrics{"namespace": namespaceName}))
require.Equal(t, false, ns.IsAllowed(namespaceName))

t.Cleanup(func() {
cancel()
Expand All @@ -289,7 +284,7 @@ func TestNamespaceFilter_InformerCacheSync(t *testing.T) {

// Give some room to the informer to sync, and check that the new namespace is filtered properly.
err = wait.PollImmediateUntilWithContext(ctx, 1*time.Second, func(context.Context) (bool, error) {
return ns.Match(definition.RawMetrics{"namespace": anotherNamespaceName}), nil
return ns.IsAllowed(anotherNamespaceName), nil
})
require.NoError(t, err, "Timed out waiting for the informer to sync")
}
Expand All @@ -302,8 +297,8 @@ func newNamespaceFilterMock() *NamespaceFilterMock {
return &NamespaceFilterMock{}
}

func (ns *NamespaceFilterMock) Match(metrics definition.RawMetrics) bool {
args := ns.Called(metrics)
func (ns *NamespaceFilterMock) IsAllowed(namespace string) bool {
args := ns.Called(namespace)
return args.Bool(0)
}

Expand Down

0 comments on commit 95f8b9d

Please sign in to comment.