Repository: deislabs/ratify
Pull request #1380 (2024-04-10 15:25:29)

feat: add ReferrerStoreManager interface to wrap operations on namespaced stores [multi-tenancy PR 4]
# Description

## What this PR does / why we need it:
This is the 4th PR for multi-tenancy support. Please review https://github.com/deislabs/ratify/pull/1359 first. Check diff between PR 3 and PR 4 at: https://github.com/binbin-li/ratify/pull/117

1. Add a ReferrerStoreManager interface to wrap operations on namespaced store resources.
2. Update store references to use the new ReferrerStoreManager interface instead of directly accessing stores.
3. Move the `StoreMap` under controllers to resource_map.go so that it can be shared by clustered and namespaced controllers in the future.
4. Updated unit tests. Some unit tests are not added in this PR as we'll change the behavior soon.
5. For the interface implementation, it's in a transition stage that keeps the old behavior. Will update the implementation once Ratify fully supports multi-tenancy.

## Which issue(s) this PR fixes *(optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged)*:
Fixes #

## Type of change

Please delete options that are not relevant.

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Helm Chart Change (any edit/addition/update that is necessary for changes merged to the `main` branch)
- [ ] This change requires a documentation update

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

- [ ] Test A
- [ ] Test B

# Checklist:

- [ ] Does the affected code have corresponding tests?
- [ ] Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
- [ ] Does this introduce breaking changes that would require an announcement or bumping the major version?
- [ ] Do all new files have appropriate license header?

# Post Merge Requirements
- [ ] MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates `Helm Chart Change`


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/deislabs/ratify/pull/1380?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) Report
Attention: Patch coverage is `96.00000%` with `1 lines` in your changes are missing coverage. Please review.
> Project coverage is 65.15%. Comparing base [(`73ef709`)](https://app.codecov.io/gh/deislabs/ratify/commit/73ef7091e919c18ad37c7ec0a6e00b5471db3748?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) to head [(`5e91637`)](https://app.codecov.io/gh/deislabs/ratify/pull/1380?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs).
> Report is 7 commits behind head on staging.

| [Files](https://app.codecov.io/gh/deislabs/ratify/pull/1380?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) | Patch % | Lines |
|---|---|---|
| [pkg/controllers/store\_controller.go](https://app.codecov.io/gh/deislabs/ratify/pull/1380?src=pr&el=tree&filepath=pkg%2Fcontrollers%2Fstore_controller.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs#diff-cGtnL2NvbnRyb2xsZXJzL3N0b3JlX2NvbnRyb2xsZXIuZ28=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/deislabs/ratify/pull/1380?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs) |

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           staging    #1380      +/-   ##
===========================================
- Coverage    65.31%   65.15%   -0.16%     
===========================================
  Files          105       90      -15     
  Lines         5458     4842     -616     
===========================================
- Hits          3565     3155     -410     
+ Misses        1535     1401     -134     
+ Partials       358      286      -72     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/deislabs/ratify/pull/1380?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deislabs).


Patch:
diff --git a/pkg/controllers/resource_map.go b/pkg/controllers/resource_map.go
index 6a496e6..5352a14 100644
--- a/pkg/controllers/resource_map.go
+++ b/pkg/controllers/resource_map.go
@@ -15,6 +15,7 @@ package controllers
 
 import (
 	"github.com/deislabs/ratify/pkg/customresources/policies"
+	rs "github.com/deislabs/ratify/pkg/customresources/referrerstores"
 	"github.com/deislabs/ratify/pkg/customresources/verifiers"
 )
 
@@ -24,4 +25,7 @@ var (
 	// ActivePolicy is the active policy generated from CRD. There would be exactly
 	// one active policy belonging to a namespace at any given time.
 	ActivePolicies = policies.NewActivePolicies()
+
+	// a map to track active stores
+	StoreMap = rs.NewActiveStores()
 )
diff --git a/pkg/controllers/store_controller.go b/pkg/controllers/store_controller.go
index b4421a6..5371e7c 100644
--- a/pkg/controllers/store_controller.go
+++ b/pkg/controllers/store_controller.go
@@ -27,7 +27,7 @@ import (
 
 	configv1beta1 "github.com/deislabs/ratify/api/v1beta1"
 	"github.com/deislabs/ratify/config"
-	"github.com/deislabs/ratify/pkg/referrerstore"
+	"github.com/deislabs/ratify/internal/constants"
 	rc "github.com/deislabs/ratify/pkg/referrerstore/config"
 	sf "github.com/deislabs/ratify/pkg/referrerstore/factory"
 	"github.com/deislabs/ratify/pkg/referrerstore/types"
@@ -40,11 +40,6 @@ type StoreReconciler struct {
 	Scheme *runtime.Scheme
 }
 
-var (
-	// a map to track active stores
-	StoreMap = map[string]referrerstore.ReferrerStore{}
-)
-
 //+kubebuilder:rbac:groups=config.ratify.deislabs.io,resources=stores,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=config.ratify.deislabs.io,resources=stores/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=config.ratify.deislabs.io,resources=stores/finalizers,verbs=update
@@ -64,7 +59,8 @@ func (r *StoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	if err := r.Get(ctx, req.NamespacedName, &store); err != nil {
 		if apierrors.IsNotFound(err) {
 			storeLogger.Infof("deletion detected, removing store %v", req.Name)
-			storeRemove(resource)
+			// TODO: pass the actual namespace once multi-tenancy is supported.
+			StoreMap.DeleteStore(constants.EmptyNamespace, resource)
 		} else {
 			storeLogger.Error(err, "unable to fetch store")
 		}
@@ -115,17 +111,13 @@ func storeAddOrReplace(spec configv1beta1.StoreSpec, fullname string) error {
 		return fmt.Errorf("store factory failed to create store from store config, err: %w", err)
 	}
 
-	StoreMap[fullname] = storeReference
+	// TODO: pass the actual namespace once multi-tenancy is supported.
+	StoreMap.AddStore(constants.EmptyNamespace, fullname, storeReference)
 	logrus.Infof("store '%v' added to store map", storeReference.Name())
 
 	return nil
 }
 
-// Remove store from map
-func storeRemove(resourceName string) {
-	delete(StoreMap, resourceName)
-}
-
 // Returns a store reference from spec
 func specToStoreConfig(storeSpec configv1beta1.StoreSpec) (rc.StorePluginConfig, error) {
 	storeConfig := rc.StorePluginConfig{}
diff --git a/pkg/customresources/referrerstores/api.go b/pkg/customresources/referrerstores/api.go
new file mode 100644
index 0000000..6b435c7
--- /dev/null
+++ b/pkg/customresources/referrerstores/api.go
@@ -0,0 +1,38 @@
+/*
+Copyright The Ratify Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package referrerstores
+
+import (
+	"github.com/deislabs/ratify/pkg/referrerstore"
+)
+
+// ReferrerStoreManager is an interface that defines the methods for managing referrer stores across different scopes.
+type ReferrerStoreManager interface {
+	// Stores returns the list of referrer stores for the given scope.
+	GetStores(scope string) []referrerstore.ReferrerStore
+
+	// AddStore adds the given store under the given scope.
+	AddStore(scope, storeName string, store referrerstore.ReferrerStore)
+
+	// DeleteStore deletes the policy from the given scope.
+	DeleteStore(scope, storeName string)
+
+	// IsEmpty returns true if there are no stores.
+	IsEmpty() bool
+
+	// GetStoreCount returns the number of stores in all scopes.
+	GetStoreCount() int
+}
diff --git a/pkg/customresources/referrerstores/stores.go b/pkg/customresources/referrerstores/stores.go
new file mode 100644
index 0000000..f60f83b
--- /dev/null
+++ b/pkg/customresources/referrerstores/stores.go
@@ -0,0 +1,89 @@
+/*
+Copyright The Ratify Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package referrerstores
+
+import (
+	"github.com/deislabs/ratify/pkg/referrerstore"
+)
+
+// ActiveStores implements the ReferrerStoreManager interface.
+type ActiveStores struct {
+	// TODO: Implement concurrent safety using sync.Map
+	// The structure of the map is as follows:
+	// The first level maps from scope to stores
+	// The second level maps from store name to store
+	// Example:
+	// {
+	//   "namespace1": {
+	//     "store1": store1,
+	//     "store2": store2
+	//   }
+	// }
+	// Note: Scope is utilized for organizing and isolating stores. In a Kubernetes (K8s) environment, the scope can be either a namespace or an empty string ("") for cluster-wide stores.
+	ScopedStores map[string]map[string]referrerstore.ReferrerStore
+}
+
+func NewActiveStores() ReferrerStoreManager {
+	return &ActiveStores{
+		ScopedStores: make(map[string]map[string]referrerstore.ReferrerStore),
+	}
+}
+
+// GetStores fulfills the ReferrerStoreManager interface.
+// It returns all the stores in the ActiveStores for the given scope. If no stores are found for the given scope, it returns cluster-wide stores.
+// TODO: Current implementation fetches stores for all namespaces including cluster-wide ones. Will support actual namespace based stores in future.
+func (s *ActiveStores) GetStores(_ string) []referrerstore.ReferrerStore {
+	stores := []referrerstore.ReferrerStore{}
+	for _, scopedStores := range s.ScopedStores {
+		for _, store := range scopedStores {
+			stores = append(stores, store)
+		}
+	}
+	return stores
+}
+
+// AddStore fulfills the ReferrerStoreManager interface.
+// It adds the given store under the given scope.
+func (s *ActiveStores) AddStore(scope, storeName string, store referrerstore.ReferrerStore) {
+	if _, ok := s.ScopedStores[scope]; !ok {
+		s.ScopedStores[scope] = make(map[string]referrerstore.ReferrerStore)
+	}
+	s.ScopedStores[scope][storeName] = store
+}
+
+// DeleteStore fulfills the ReferrerStoreManager interface.
+// It deletes the store with the given name under the given scope.
+func (s *ActiveStores) DeleteStore(scope, storeName string) {
+	if stores, ok := s.ScopedStores[scope]; ok {
+		delete(stores, storeName)
+	}
+}
+
+// IsEmpty fulfills the ReferrerStoreManager interface.
+// It returns true if there are no stores in the ActiveStores.
+func (s *ActiveStores) IsEmpty() bool {
+	return s.GetStoreCount() == 0
+}
+
+// GetStore fulfills the ReferrerStoreManager interface.
+// GetStoreCount returns the total number of stores in the ActiveStores.
+func (s *ActiveStores) GetStoreCount() int {
+	count := 0
+	for _, stores := range s.ScopedStores {
+		count += len(stores)
+	}
+	return count
+}
diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go
index bb9dfeb..2527f43 100644
--- a/pkg/manager/manager.go
+++ b/pkg/manager/manager.go
@@ -51,7 +51,6 @@ import (
 	ctxUtils "github.com/deislabs/ratify/internal/context"
 	"github.com/deislabs/ratify/pkg/controllers"
 	ef "github.com/deislabs/ratify/pkg/executor/core"
-	"github.com/deislabs/ratify/pkg/referrerstore"
 	//+kubebuilder:scaffold:imports
 )
 
@@ -84,17 +83,11 @@ func StartServer(httpServerAddress, configFilePath, certDirectory, caCertFile st
 
 	// initialize server
 	server, err := httpserver.NewServer(context.Background(), httpServerAddress, func(ctx context.Context) *ef.Executor {
-		var activeStores []referrerstore.ReferrerStore
 		namespace := ctxUtils.GetNamespace(ctx)
+
 		activeVerifiers := controllers.VerifierMap.GetVerifiers(namespace)
 		activePolicyEnforcer := controllers.ActivePolicies.GetPolicy(namespace)
-
-		// check if there are active stores from crd controller
-		if len(controllers.StoreMap) > 0 {
-			for _, value := range controllers.StoreMap {
-				activeStores = append(activeStores, value)
-			}
-		}
+		activeStores := controllers.StoreMap.GetStores(namespace)
 
 		// return executor with latest configuration
 		executor := ef.Executor{


Test patch:
diff --git a/pkg/controllers/store_controller_test.go b/pkg/controllers/store_controller_test.go
index 8838b9c..1897d57 100644
--- a/pkg/controllers/store_controller_test.go
+++ b/pkg/controllers/store_controller_test.go
@@ -22,7 +22,8 @@ import (
 	"testing"
 
 	configv1beta1 "github.com/deislabs/ratify/api/v1beta1"
-	"github.com/deislabs/ratify/pkg/referrerstore"
+	"github.com/deislabs/ratify/internal/constants"
+	rs "github.com/deislabs/ratify/pkg/customresources/referrerstores"
 	"github.com/deislabs/ratify/pkg/utils"
 	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -47,15 +48,15 @@ func TestStoreAdd_EmptyParameter(t *testing.T) {
 	if err := storeAddOrReplace(testStoreSpec, "oras"); err != nil {
 		t.Fatalf("storeAddOrReplace() expected no error, actual %v", err)
 	}
-	if len(StoreMap) != 1 {
-		t.Fatalf("Store map expected size 1, actual %v", len(StoreMap))
+	if StoreMap.GetStoreCount() != 1 {
+		t.Fatalf("Store map expected size 1, actual %v", StoreMap.GetStoreCount())
 	}
 }
 
 func TestStoreAdd_WithParameters(t *testing.T) {
 	resetStoreMap()
-	if len(StoreMap) != 0 {
-		t.Fatalf("Store map expected size 0, actual %v", len(StoreMap))
+	if StoreMap.GetStoreCount() != 0 {
+		t.Fatalf("Store map expected size 0, actual %v", StoreMap.GetStoreCount())
 	}
 	dirPath, err := utils.CreatePlugin(sampleName)
 	if err != nil {
@@ -68,8 +69,8 @@ func TestStoreAdd_WithParameters(t *testing.T) {
 	if err := storeAddOrReplace(testStoreSpec, "testObject"); err != nil {
 		t.Fatalf("storeAddOrReplace() expected no error, actual %v", err)
 	}
-	if len(StoreMap) != 1 {
-		t.Fatalf("Store map expected size 1, actual %v", len(StoreMap))
+	if StoreMap.GetStoreCount() != 1 {
+		t.Fatalf("Store map expected size 1, actual %v", StoreMap.GetStoreCount())
 	}
 }
 
@@ -137,8 +138,8 @@ func TestStore_UpdateAndDelete(t *testing.T) {
 	if err := storeAddOrReplace(testStoreSpec, sampleName); err != nil {
 		t.Fatalf("storeAddOrReplace() expected no error, actual %v", err)
 	}
-	if len(StoreMap) != 1 {
-		t.Fatalf("Store map expected size 1, actual %v", len(StoreMap))
+	if StoreMap.GetStoreCount() != 1 {
+		t.Fatalf("Store map expected size 1, actual %v", StoreMap.GetStoreCount())
 	}
 
 	// modify the Store
@@ -152,19 +153,19 @@ func TestStore_UpdateAndDelete(t *testing.T) {
 	}
 
 	// validate no Store has been added
-	if len(StoreMap) != 1 {
-		t.Fatalf("Store map should be 1 after replacement, actual %v", len(StoreMap))
+	if StoreMap.GetStoreCount() != 1 {
+		t.Fatalf("Store map should be 1 after replacement, actual %v", StoreMap.GetStoreCount())
 	}
 
-	storeRemove(sampleName)
+	StoreMap.DeleteStore(constants.EmptyNamespace, sampleName)
 
-	if len(StoreMap) != 0 {
-		t.Fatalf("Store map should be 0 after deletion, actual %v", len(StoreMap))
+	if StoreMap.GetStoreCount() != 0 {
+		t.Fatalf("Store map should be 0 after deletion, actual %v", StoreMap.GetStoreCount())
 	}
 }
 
 func resetStoreMap() {
-	StoreMap = map[string]referrerstore.ReferrerStore{}
+	StoreMap = rs.NewActiveStores()
 }
 
 func getOrasStoreSpec(pluginName, pluginPath string) configv1beta1.StoreSpec {
diff --git a/pkg/customresources/referrerstores/stores_test.go b/pkg/customresources/referrerstores/stores_test.go
new file mode 100644
index 0000000..46a3758
--- /dev/null
+++ b/pkg/customresources/referrerstores/stores_test.go
@@ -0,0 +1,104 @@
+/*
+Copyright The Ratify Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package referrerstores
+
+import (
+	"context"
+	"testing"
+
+	"github.com/deislabs/ratify/internal/constants"
+	"github.com/deislabs/ratify/pkg/common"
+	"github.com/deislabs/ratify/pkg/ocispecs"
+	rs "github.com/deislabs/ratify/pkg/referrerstore"
+	"github.com/deislabs/ratify/pkg/referrerstore/config"
+	"github.com/opencontainers/go-digest"
+)
+
+type mockStore struct {
+	name string
+}
+
+func (s mockStore) Name() string {
+	return s.name
+}
+
+func (s mockStore) ListReferrers(_ context.Context, _ common.Reference, _ []string, _ string, _ *ocispecs.SubjectDescriptor) (rs.ListReferrersResult, error) {
+	return rs.ListReferrersResult{}, nil
+}
+
+func (s mockStore) GetBlobContent(_ context.Context, _ common.Reference, _ digest.Digest) ([]byte, error) {
+	return nil, nil
+}
+
+func (s mockStore) GetReferenceManifest(_ context.Context, _ common.Reference, _ ocispecs.ReferenceDescriptor) (ocispecs.ReferenceManifest, error) {
+	return ocispecs.ReferenceManifest{}, nil
+}
+
+func (s mockStore) GetConfig() *config.StoreConfig {
+	return nil
+}
+
+func (s mockStore) GetSubjectDescriptor(_ context.Context, _ common.Reference) (*ocispecs.SubjectDescriptor, error) {
+	return nil, nil
+}
+
+const (
+	namespace1 = constants.EmptyNamespace
+	namespace2 = "namespace2"
+	name1      = "name1"
+	name2      = "name2"
+)
+
+var (
+	store1 = mockStore{name: name1}
+	store2 = mockStore{name: name2}
+)
+
+func TestStoresOperations(t *testing.T) {
+	stores := NewActiveStores()
+	stores.AddStore(namespace1, store1.Name(), store1)
+	stores.AddStore(namespace1, store2.Name(), store2)
+	stores.AddStore(namespace2, store1.Name(), store1)
+	stores.AddStore(namespace2, store2.Name(), store2)
+
+	if stores.GetStoreCount() != 4 {
+		t.Fatalf("Expected 4 namespaces, got %d", stores.GetStoreCount())
+	}
+
+	stores.DeleteStore(namespace2, store1.Name())
+	if len(stores.GetStores(namespace2)) != 3 {
+		t.Fatalf("Expected 3 store in namespace %s, got %d", namespace2, len(stores.GetStores(namespace2)))
+	}
+
+	stores.DeleteStore(namespace2, store2.Name())
+	if len(stores.GetStores(namespace2)) != 2 {
+		t.Fatalf("Expected 2 stores in namespace %s, got %d", namespace2, len(stores.GetStores(namespace2)))
+	}
+
+	stores.DeleteStore(namespace1, store1.Name())
+	if len(stores.GetStores(namespace1)) != 1 {
+		t.Fatalf("Expected 1 store in namespace %s, got %d", namespace1, len(stores.GetStores(namespace1)))
+	}
+
+	stores.DeleteStore(namespace1, store2.Name())
+	if len(stores.GetStores(namespace1)) != 0 {
+		t.Fatalf("Expected 0 stores in namespace %s, got %d", namespace1, len(stores.GetStores(namespace1)))
+	}
+
+	if !stores.IsEmpty() {
+		t.Fatalf("Expected stores to be empty")
+	}
+}