Add cachedir pooling and refactor git.go

Author: kushnaidu

pkg/apiserver/webhooks.go

@@ -329,7 +329,7 @@ func createValidatingWebhook(ctx context.Context, cfg *WebhookConfig, caCert []b
 					Resources:   []string{porchapi.PackageRevisionGVR.Resource},
 				},
 			}},
-			AdmissionReviewVersions: []string{"v1", "v1beta1"},
+			AdmissionReviewVersions: []string{"v1"},
 			SideEffects:             &none,
 			FailurePolicy:           &fail,
 			TimeoutSeconds:          &cfg.timeout,
@@ -357,7 +357,7 @@ func createValidatingWebhook(ctx context.Context, cfg *WebhookConfig, caCert []b
 					Resources:   []string{"repositories"},
 				},
 			}},
-			AdmissionReviewVersions: []string{"v1", "v1beta1"},
+			AdmissionReviewVersions: []string{"v1"},
 			SideEffects:             &none,
 			FailurePolicy:           &fail,
 			TimeoutSeconds:          &cfg.timeout,
@@ -661,12 +661,28 @@ func validateRepository(w http.ResponseWriter, r *http.Request, porchClient clie
 
 	var attempted configapi.Repository
 	if err := json.Unmarshal(admissionReviewRequest.Request.Object.Raw, &attempted); err != nil {
+		klog.Errorf("failed to unmarshal repository object: %v", err)
 		writeErr(fmt.Sprintf("could not unmarshal repository: %v", err), &w)
 		return
 	}
 
+	// For UPDATE operations, check if URL or directory is being modified
+	if admissionReviewRequest.Request.Operation == admissionv1.Update {
+		var existing configapi.Repository
+		if err := json.Unmarshal(admissionReviewRequest.Request.OldObject.Raw, &existing); err != nil {
+			klog.Errorf("failed to unmarshal existing repository object: %v", err)
+			writeErr(fmt.Sprintf("could not unmarshal existing repository: %v", err), &w)
+			return
+		}
+
+		if err := validateRepositoryModification(&existing, &attempted, admissionReviewRequest, &w); err != nil {
+			return
+		}
+	}
+
 	var repoList configapi.RepositoryList
 	if err := porchClient.List(context.Background(), &repoList); err != nil {
+		klog.Errorf("failed to list repositories: %v", err)
 		writeErr(fmt.Sprintf("could not list repositories: %v", err), &w)
 		return
 	}
@@ -676,22 +692,8 @@ func validateRepository(w http.ResponseWriter, r *http.Request, porchClient clie
 			continue
 		}
 		if isConflict(&existing, &attempted) {
-			resp := &admissionv1.AdmissionResponse{
-				Allowed: false,
-				Result: &metav1.Status{
-					Status:  "Failure",
-					Message: fmt.Sprintf("Repository conflict with existing repository: %s/%s", existing.Namespace, existing.Name),
-					Reason:  "RepositoryConflict",
-				},
-			}
-			responseBytes, _ := constructResponse(resp, admissionReviewRequest)
-			w.Header().Set("Content-Type", "application/json")
-			_, err = w.Write(responseBytes)
-			if err != nil {
-				errMsg := fmt.Sprintf("error writing response: %v", err)
-				writeErr(errMsg, &w)
-				return
-			}
+			klog.Errorf("repository validation failed: conflict detected between attempted %s/%s and existing %s/%s", attempted.Namespace, attempted.Name, existing.Namespace, existing.Name)
+			writeModificationResponse(fmt.Sprintf("Repository conflict with existing repository: %s/%s", existing.Namespace, existing.Name), "RepositoryConflict", admissionReviewRequest, &w)
 			return
 		}
 	}
@@ -766,18 +768,64 @@ func isConflict(existing, attempted *configapi.Repository) bool {
 }
 
 func isNestedConflict(a, b string) bool {
-	aParts := strings.Split(a, "/")
-	bParts := strings.Split(b, "/")
+	// Check if one path is nested within the other using filepath.Rel
+	relAtoB, err1 := filepath.Rel(a, b)
+	relBtoA, err2 := filepath.Rel(b, a)
 
-	// a is base of b
-	if len(aParts) < len(bParts) && strings.Join(bParts[:len(aParts)], "/") == a {
-		return true
+	// If either relative path doesn't start with "../", it means one is nested in the other
+	if err1 == nil && !strings.HasPrefix(relAtoB, "../") && relAtoB != "." {
+		return true // b is nested within a
 	}
-
-	// b is base of a
-	if len(bParts) < len(aParts) && strings.Join(aParts[:len(bParts)], "/") == b {
-		return true
+	if err2 == nil && !strings.HasPrefix(relBtoA, "../") && relBtoA != "." {
+		return true // a is nested within b
 	}
 
 	return false
 }
+
+func validateRepositoryModification(existing, attempted *configapi.Repository, admissionReviewRequest *admissionv1.AdmissionReview, w *http.ResponseWriter) error {
+	if isURLModified(existing, attempted) {
+		klog.Errorf("repository validation failed: URL modification not allowed for %s/%s - delete the existing repository and create it if you want to change the URL", attempted.Namespace, attempted.Name)
+		writeModificationResponse("Repository URL cannot be modified after creation. Please delete the existing repository and create it if you want to change the URL", "URLModificationNotAllowed", admissionReviewRequest, w)
+		return fmt.Errorf("URL modification not allowed")
+	}
+
+	if isDirectoryModified(existing, attempted) {
+		klog.Errorf("repository validation failed: directory modification not allowed for %s/%s - delete the existing repository and create it if you want to change the directory", attempted.Namespace, attempted.Name)
+		writeModificationResponse("Repository directory cannot be modified after creation. Please delete the existing repository and create it if you want to change the directory", "DirectoryModificationNotAllowed", admissionReviewRequest, w)
+		return fmt.Errorf("directory modification not allowed")
+	}
+
+	return nil
+}
+
+func isURLModified(existing, attempted *configapi.Repository) bool {
+	return existing.Spec.Git.Repo != attempted.Spec.Git.Repo
+}
+
+func isDirectoryModified(existing, attempted *configapi.Repository) bool {
+	return existing.Spec.Git.Directory != attempted.Spec.Git.Directory
+}
+
+func writeModificationResponse(message, reason string, admissionReviewRequest *admissionv1.AdmissionReview, w *http.ResponseWriter) {
+	resp := &admissionv1.AdmissionResponse{
+		Allowed: false,
+		Result: &metav1.Status{
+			Status:  "Failure",
+			Message: message,
+			Reason:  metav1.StatusReason(reason),
+		},
+	}
+	responseBytes, err := constructResponse(resp, admissionReviewRequest)
+	if err != nil {
+		klog.Errorf("failed to construct modification response: %v", err)
+		writeErr(fmt.Sprintf("error constructing response: %v", err), w)
+		return
+	}
+	(*w).Header().Set("Content-Type", "application/json")
+	_, err = (*w).Write(responseBytes)
+	if err != nil {
+		errMsg := fmt.Sprintf("error writing response: %v", err)
+		writeErr(errMsg, w)
+	}
+}

pkg/cache/crcache/cache.go

@@ -34,8 +34,6 @@ var tracer = otel.Tracer("crcache")
 type Cache struct {
 	repositories  map[repository.RepositoryKey]*cachedRepository
 	mainLock      *sync.RWMutex
-	locks         map[repository.RepositoryKey]*sync.Mutex
-	cacheLocks    map[string]*sync.Mutex
 	metadataStore meta.MetadataStore
 	options       cachetypes.CacheOptions
 }
@@ -53,18 +51,10 @@ func (c *Cache) OpenRepository(ctx context.Context, repositorySpec *configapi.Re
 		return nil, err
 	}
 
-	cacheKey := c.getCacheKey(repositorySpec)
-	cacheLock := c.getOrCreateCacheLock(cacheKey)
-	cacheLock.Lock()
-	defer cacheLock.Unlock()
-
-	lock := c.getOrInsertLock(key)
-	lock.Lock()
-	defer lock.Unlock()
+	c.mainLock.Lock()
+	defer c.mainLock.Unlock()
 
-	c.mainLock.RLock()
 	if repo, ok := c.repositories[key]; ok && repo != nil {
-		c.mainLock.RUnlock()
 		// Keep the spec updated in the cache.
 		repo.repoSpec = repositorySpec
 		// Check external repo connectivity
@@ -79,18 +69,14 @@ func (c *Cache) OpenRepository(ctx context.Context, repositorySpec *configapi.Re
 		}
 		return repo, nil
 	}
-	c.mainLock.RUnlock()
 
 	externalRepo, err := externalrepo.CreateRepositoryImpl(ctx, repositorySpec, c.options.ExternalRepoOptions)
 	if err != nil {
 		return nil, err
 	}
 
 	cachedRepo := newRepository(key, repositorySpec, externalRepo, c.metadataStore, c.options)
-
-	c.mainLock.Lock()
 	c.repositories[key] = cachedRepo
-	c.mainLock.Unlock()
 
 	return cachedRepo, nil
 }
@@ -115,45 +101,14 @@ func (c *Cache) CloseRepository(ctx context.Context, repositorySpec *configapi.R
 		return nil
 	}
 
-	cacheKey := c.getCacheKey(repositorySpec)
-	cacheLock := c.getOrCreateCacheLock(cacheKey)
-	cacheLock.Lock()
-	defer cacheLock.Unlock()
-
-	// check if repositorySpec shares the underlying cached repo with another repository
-	for _, r := range allRepos {
-		if r.Name == repositorySpec.Name && r.Namespace == repositorySpec.Namespace {
-			continue
-		}
-		// For Git repositories, check sharing based on URL only
-		if repositorySpec.Spec.Type == configapi.RepositoryTypeGit &&
-			r.Spec.Type == configapi.RepositoryTypeGit &&
-			r.Spec.Git.Repo == repositorySpec.Spec.Git.Repo {
-			// do not close cached repo if it is shared, but cancel the polling goroutine
-			klog.Infof("Not closing cached repository %q because it is shared", key)
-			return nil
-		}
-	}
-
-	lock := c.getOrInsertLock(key)
-	lock.Lock()
-	defer lock.Unlock()
-
-	if ok {
+	defer func() {
 		c.mainLock.Lock()
-		delete(c.locks, key)
 		delete(c.repositories, key)
 		c.mainLock.Unlock()
+	}()
 
-		if repo != nil {
-			return repo.Close(ctx)
-		} else {
-			klog.Warningf("cached repository with key %q had stored value nil", key)
-		}
-	} else {
-		c.mainLock.Lock()
-		delete(c.locks, key)
-		c.mainLock.Unlock()
+	if repo != nil {
+		return repo.Close(ctx)
 	}
 
 	return nil
@@ -176,43 +131,3 @@ func (c *Cache) GetRepository(repoKey repository.RepositoryKey) repository.Repos
 	defer c.mainLock.RUnlock()
 	return c.repositories[repoKey]
 }
-
-func (c *Cache) getOrInsertLock(key repository.RepositoryKey) *sync.Mutex {
-	c.mainLock.RLock()
-	if lock, exists := c.locks[key]; exists {
-		c.mainLock.RUnlock()
-		return lock
-	}
-	c.mainLock.RUnlock()
-
-	c.mainLock.Lock()
-	lock := &sync.Mutex{}
-	c.locks[key] = lock
-	c.mainLock.Unlock()
-
-	return lock
-}
-
-func (c *Cache) getCacheKey(repositorySpec *configapi.Repository) string {
-	if repositorySpec.Spec.Type == configapi.RepositoryTypeGit {
-		return repositorySpec.Spec.Git.Repo
-	}
-	return repositorySpec.Name + "---" + repositorySpec.Namespace
-}
-
-func (c *Cache) getOrCreateCacheLock(cacheKey string) *sync.Mutex {
-	c.mainLock.Lock()
-	defer c.mainLock.Unlock()
-
-	if c.cacheLocks == nil {
-		c.cacheLocks = make(map[string]*sync.Mutex)
-	}
-
-	if lock, exists := c.cacheLocks[cacheKey]; exists {
-		return lock
-	}
-
-	lock := &sync.Mutex{}
-	c.cacheLocks[cacheKey] = lock
-	return lock
-}

pkg/cache/crcache/cache_test.go

@@ -271,7 +271,6 @@ func openRepositoryFromArchive(t *testing.T, ctx context.Context, testPath, name
 	fakeClient := k8sfake.NewClientBuilder().WithScheme(scheme).WithObjects(apiRepo).Build()
 	cache := &Cache{
 		repositories:  map[repository.RepositoryKey]*cachedRepository{},
-		locks:         map[repository.RepositoryKey]*sync.Mutex{},
 		mainLock:      &sync.RWMutex{},
 		metadataStore: metadataStore,
 		options: cachetypes.CacheOptions{

pkg/cache/crcache/crcachefactory.go

@@ -31,7 +31,6 @@ type CrCacheFactory struct {
 func (f *CrCacheFactory) NewCache(_ context.Context, options cachetypes.CacheOptions) (cachetypes.Cache, error) {
 	return &Cache{
 		repositories:  map[repository.RepositoryKey]*cachedRepository{},
-		locks:         map[repository.RepositoryKey]*sync.Mutex{},
 		mainLock:      &sync.RWMutex{},
 		metadataStore: meta.NewCrdMetadataStore(options.CoreClient),
 		options:       options,

pkg/cache/crcache/repository.go

@@ -148,7 +148,11 @@ func (r *cachedRepository) getRefreshError() error {
 }
 
 func (r *cachedRepository) getPackageRevisions(ctx context.Context, filter repository.ListPackageRevisionFilter, forceRefresh bool) ([]repository.PackageRevision, error) {
-	klog.Infof("Cache::OpenRepository(%s) fetching packages", r.Key())
+	if forceRefresh {
+		klog.Infof("Cache::OpenRepository(%s) fetching packages from external repository", r.Key())
+	} else {
+		klog.V(2).Infof("Cache::OpenRepository(%s) using cached packages", r.Key())
+	}
 	_, packageRevisions, err := r.getCachedPackages(ctx, forceRefresh)
 	if err != nil {
 		return nil, err

pkg/cache/crcache/repository_test.go

@@ -34,13 +34,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-type PorchClient interface {
-	List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error
-}
-
 func TestCachedRepoRefresh(t *testing.T) {
 	mockRepo := mockrepo.NewMockRepository(t)
 	mockMeta := mockmeta.NewMockMetadataStore(t)