Why isn't this generated from the store interface using

//go:generate mockgen-wrapper

If simple impl is enough then implement it in sensor/common/compliance/node_matcher_test.go
If it is more complex, then use mockgen.

In both cases remove AddOrUpdateNode from the interface.

This doesn't seem to be used by production code, only for tests. Why does it need to be part of the interface?

I don't understand the design decision of exposing this internal struct of the listeners rather than using a separate interface returning only *storage.Node, which is what is really used outside the listener.

I.e.: I don't see Addresses being used anywhere other than in the listener. So why do they need to be exposed in sensor/common?

Most stores have the similar requirements. The way this is handled for other stores is to have a sensor-wide API for the store (defined in sensor/common), which exposes storage.* type structs. And have its implementation be internal to sensor/kubernetes, where you can use any additional information needed.

Take deployments, for example:

// In sensor/common/store:
type DeploymentStore interface {
	Get(id string) *storage.Deployment
}

// In sensor/kubernetes/listener/resources
type DeploymentStore struct { ... }

func (ds *DeploymentStore) addOrUpdateDeployment(wrap *deploymentWrap) { ... }

// Get returns deployment for supplied id.
func (ds *DeploymentStore) Get(id string) *storage.Deployment {
	wrap := ds.getWrap(id)
        // parse to a storage.Deployment type to comply with public common interface
	return wrap.GetDeployment()
}

Should be in the common file with all other interfaces (only Get func).

The other functions can be in another interface that will be used in the listeners. Then the fucns can be private.

Try to follow the example of Roles an RBACs (optional).

Moving the small interface to types.go should be enough.

I know that this goes against the go convention of "Accept interfaces, return structs", but I would either change this to store.NodeStore or I would change the implementations above. Otherwise it looks like they are different when in reality they're meant to do the same.

+1 to this

I see your point. Let me share mine.

There are three four options here:

1. func (p *InMemoryStoreProvider) Nodes() store.NodeStore

This is a problem cause we rely on the nodeStore interface in some places.

sensor/kubernetes/listener/resources/dispatcher.go:91:65: cannot use storeProvider.Nodes() (value of type store.NodeStore) as nodeStore value in argument to newNodeDispatcher: store.NodeStore does not implement nodeStore (missing method addOrUpdateNode) (typecheck)

2. func (p *InMemoryStoreProvider) Nodes() nodeStore

This is fine for now, but returning an unexported interface may limit the usability of this method in the future.

3. func (p *InMemoryStoreProvider) Nodes() *nodeStoreImpl

The current state. It allows to add checks for the impl to make sure that it implements both interfaces:

var _ nodeStore = (*nodeStoreImpl)(nil)
var _ store.NodeStore = (*nodeStoreImpl)(nil)

4. func (p *InMemoryStoreProvider) Nodes() BigNodeStore

Create new interface BigNodeStore (name subject to change) that would do:

type BigNodeStore interface {
    nodeStore
    store.NodeStore
}

Based on that, I think that option 3 looks the best. What do you think?

I think there is also a 5th option where we make store.NodeStore a copy of nodeStore but the methods from nodeStore (addOrUpdateNode etc.) will be unexported in the new bigger store.NodeStore.

Edit: nah, this is impossible, because the impl would need to be in the same package as the interface and this is not the case for us.

For

type NodeStore interface {
	GetNode(nodeName string) *storage.Node

	addOrUpdateNode(node *storage.Node) bool
	removeNode(node *storage.Node)
	getNode(nodeName string) *storage.Node
	getNodes() []*storage.Node
}

I get

Cannot use 'storeProvider.Nodes()' (type store.NodeStore) as the type nodeStore Type cannot implement 'nodeStore' as it has a non-exported method and is defined in a different package

which in the end would force me to export the unexported methods and I assume we do not want it.

Following the other alternative (returning impls everywhere) would look like this:

type InMemoryStoreProvider struct {
	deploymentStore *DeploymentStore  // Interface named the same as implementation - add Impl to type name?
	podStore        *PodStore
	serviceStore    *serviceStore
	rbacStore       rbac.storeImpl       // Unexported - would need to export `storeImpl`
	endpointManager *endpointManagerImpl 
	nodeStore       *nodeStoreImpl
	entityStore     *clusterentities.Store  // this is actually Impl - add Impl to type name?
}

As it contains a lot of renaming I would like to do this in a followup PR. Would that be okay?

I have a simple suggestion related to (1). The only place where it seems like you should be needing the private struct instead of the public interface is when instantiating the handlers. In sensor/kubernetes/listener/resources/dispatcher.go you could simply do:

nodeDispatcher:            newNodeDispatcher(deploymentStore, storeProvider.nodeStore, endpointManager),

And the dispatcher will be able to call the private store functions.

If I'm not mistaken, when you make this change, you should be able to switch the .Nodes() function to the public interface without any compiler errors.

This is what we do for RBACs and Services stores. Maybe it's not the ideal way of doing things and maybe you're on to something better. But I'd opt for the easiest thing here which is just passing the internal struct to the handler. We can have a sync discussion after this PR on how we could improve this if you'd like.

Could you give it a try?

Add me to that sync discussion if possible 🙂 I have similar feelings as Fred but I feel they might not align with "the Go way".

Here is it 727b9b2 - I cannot check it locally, but CI should do the job for us.

Looks like it worked great! I will be merging this in a moment

+1 to this

I think this interface should go, right? Now that you've added the new interface in sensor/common/store this one doesn't need to exist.

That is correct, I missed that one. Should be fixed now.

Use the public interface from sensor/common/store