nephio-project / nephio

Nephio is a Kubernetes-based automation platform for deploying and managing highly distributed, interconnected workloads such as 5G Network Functions, and the underlying infrastructure on which those workloads depend.
Apache License 2.0
93 stars 52 forks source link

Inconsistent behaviour by kpt if there are 2 repository CR pointing to same git repo but different directory inside the repoo #628

Closed liamfallon closed 1 month ago

liamfallon commented 2 months ago

Original issue URL: https://github.com/kptdev/kpt/issues/3935 Original issue user: https://github.com/Cbkhare Original issue created at: 2023-04-27T16:45:50Z Original issue last updated at: 2023-04-29T00:47:54Z Original issue body: Issue: kpt alpha rpkg init is working incorrectly when I apply 2 Repository CR in the system which are pointing to the same private repository.

Details of my 2 CRs

$ cat test_ct_repo.yaml
apiVersion: config.porch.kpt.dev/v1alpha1
kind: Repository
metadata:
  name: test-catalogue
  namespace: test
spec:
  description: test-catalogue
  content: Package
  deployment: false
  type: git
  git:
    repo: https://github.com/********/**********************.git
    directory: /
    branch: main
    createBranch: true
    secretRef:
      name: cred
$ cat test_ct_repo_out.yaml
apiVersion: config.porch.kpt.dev/v1alpha1
kind: Repository
metadata:
  name: test-catalogue-out
  namespace: test-out
spec:
  description: test-catalogue-out
  content: Package
  deployment: false
  type: git
  git:
    repo: https://github.com/********/**********************.git
    directory: test-pkg-out
    branch: main
    createBranch: true
    secretRef:
      name: cred

Notes: 1) In both of the above CR, repo path is same. Since, its a private repo, I have replaced the username and repo name with *. 2) Namespace and CR name is different. 3) directory is different. 4) secret was created in both the namespaces. 5) test-catalogue was applied first. And then test-catalogue-out

$ kubectl get repositories -A
NAMESPACE   NAME                 TYPE   CONTENT   DEPLOYMENT   READY   ADDRESS
test-out    test-catalogue-out   git    Package   false        True    https://github.com/******/*********************.git
test        test-catalogue       git    Package   false        True    https://github.com/******/*********************.git

Below are my observations after Creating the above CRs.

1) Incorrect revision creation.

$ kpt alpha rpkg init test-pkg11 --workspace=v1 --repository=test-catalogue --namespace=test
NAMESPACE   NAME                                                           PACKAGE     WORKSPACENAME   REVISION   LATEST   LIFECYCLE   REPOSITORY
test-out    test-catalogue-out-233a4ca4388204556f5bceff99a9799d1a86ed1a   test-pkg11    v1              v1         false     Draft   test-catalogue-out

As you can notice in the above command, the namespace and repository passed are test and test-catalogue respectively. Yet, the revision which was created had the details of the repository and namespace of the test-catalogue-out Repository CR.

After facing this issue, I deleted and re-created the porch. And recreated the 2 repo CR as above, this time I was getting a different issue, as below.

2) Not able to init the package.

$ kpt alpha rpkg init test-pkg-repo1 --workspace=v1 --repository=test-catalogue --namespace=test
Error: Internal error occurred: create not allowed while custom resource definition is terminating

Original issue comments: Comment user: https://github.com/mortent Comment created at: 2023-04-28T21:28:39Z Comment last updated at: 2023-04-28T21:28:39Z Comment body: @Cbkhare Thanks for reporting this and the detailed description. I am able to reproduce weird behavior here, but I haven't been able to fully understand what is going on yet.

I see that one of the repositories point to the root of the git repo, while the other point to a subdirectory. This probably isn't a good way to do this, since Porch assumes each package is a direct subdirectory of the directory provided for a repository. So for the test-catalogue repository, you should probably set the directory property to something like test-pkg. However, I'm still seeing odd behavior when I do this, so I will look into it.

Comment user: https://github.com/mortent Comment created at: 2023-04-29T00:47:53Z Comment last updated at: 2023-04-29T00:47:53Z Comment body: So dug deeper into this. This is actually a rather significant bug in Porch, where some parts of the code doesn't account for the fact that it might be multiple Repository objects pointing to the same repository, but with different settings (which probably is a pretty common case). The cache that keeps track of repositories and sets up the sync between git/oci and Porch, uses the repository URL as the key for the inventory of repositories:

https://github.com/GoogleContainerTools/kpt/blob/239a679abe979e058e53fbdcb60a456060cda81c/porch/pkg/cache/cache.go#L120-L125

This is reasonable, as we don't want to poll the same repository multiple times in parallel. But the internal representation of a repository, which has a one-to-one mapping to a git Repository due to the repo url being the key, also includes information specific to the KRM Repository CR:

https://github.com/GoogleContainerTools/kpt/blob/239a679abe979e058e53fbdcb60a456060cda81c/porch/pkg/cache/cache.go#L133-L141

This is something we need to fix.

efiacor commented 1 month ago

May already have been implemented - https://github.com/nephio-project/porch/blob/main/pkg/cache/cache.go#L122

https://github.com/nephio-project/porch/blob/main/pkg/git/git.go#L159

Investigation ongoing.

liamfallon commented 1 month ago

See also https://github.com/nephio-project/nephio/issues/626

efiacor commented 1 month ago

This seems to have been addressed in https://github.com/kptdev/kpt/pull/4097 Some issues regarding polling of redundant repos still exist due to missing watch.Modify Impl - https://github.com/nephio-project/porch/blob/main/pkg/registry/porch/background.go#L136 but are tolerable.

Closing this with a view to address the Repo CR update/modify in a future release.