creator of PackageRepository CR should be able to specify which packages are imported

[cppforlife]

Describe the problem/challenge you have

Currently when PackageRepository CR is created all packages become available for use. Administrators (who create PkgRepo CRs) might be interested in locking down what particular PackageRepository imports into the cluster.

Describe the solution you'd like

Provide a filtering feature on PackageRepository CR for Packages and PackageMetadata that would allow somebody to specify what packages can be imported. I could see us supporting exact names, and wildcard name matches. I dont believe we should allow filtering by version.

Should there be a configurable default on kc to import nothing by default?

---

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible" 👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

[neil-hickey]

Hi there!

I wanted to have a look at contributing this:

So far I have discovered through help from Carvel folks that the CRD definition for PackageRepository is written here:

https://github.com/vmware-tanzu/carvel-kapp-controller/blob/85e814cda7109169809ede1c8a4f211739ad15d2/pkg/apis/packaging/v1alpha1/package_repository.go

Once I make my changes, I need to run ./hack/build.sh to get the crds.yml file to be generated for me.

The main entrypoint for the PackageRepository reconciler is here:

https://github.com/vmware-tanzu/carvel-kapp-controller/blob/85e814cda7109169809ede1c8a4f211739ad15d2/pkg/pkgrepository/reconciler.go#L67

[neil-hickey]

Design Proposal

We could expose a new list property on the PackageRepository CR which would allow the user to specify which packages to allow via a PackageRepo. See options below for details.

Design questions

Should there be a configurable default on kc to import nothing by default?

Based on some conversation in the k8s slack thread I think it aligns with other prior work in the k8s space to assume everything is allowed by default and then this filtering option would be a more advanced task for admins. So I would argue in favour of not having any configurable default for an initial iteration of this feature.

Would an allowList support regex?

A sensible regex to implement after a 1:1 name matching would be to allow wildcard matching.

Could each package have a "Category" type field, that the PackageRepository can filter on?

This feels slightly out of scope and more aligns with a further iteration of this feature, after you can filter by some basic criteria available today.

UX Options

Option 1: AllowList on PackageRepository CR

kind: PackageRepository
spec:
  ...
  filter:
    allowList:
      name: ["example.com", "*.example.com"]

where the default is implicity:

kind: PackageRepository
spec:
  ...
  filter:
    allowList:
      name: ["*"]

Why have a name key?

This allows easy extension to filter using an AND pattern to potentially add other properties such as author:

kind: PackageRepository
spec:
  ...
  filter:
    allowList:
      name: ["example.com", "*.example.com"]
      author: ["VMware"]

Why have a filter key?

We may want to filter based on something that is not an allowlist in the future, such as the ability on only allow the last N versions to be accessible via PackageRepository, or some range of versions:

kind: PackageRepository
spec:
  ...
  filter:
    allowList:
      name: ["example.com", "*.example.com"]
      author: ["VMware"]
    versionLookback: 3 # allow only latest 3 versions

Option 2: DenyList on PackageRepository CR

kind: PackageRepository
spec:
  ...
  filter:
    denyList:
      name: ["*"]

where the default is implicity:

kind: PackageRepository
spec:
  ...
  filter:
    denyList:
      name: []

[cppforlife]

one interesting thing to think about is that we dont just have Packages but also PackageMetadata and potentially upcoming Releases... may be only Packages are important to filter since they are referenced by PackageInstall?

i think we definitely want to go with option 1 (we rarely to deny instead of allow policies).

could you also take a look at other APIs carvel has within other tools. there might be some similarities to take advantage of (e.g. resourceMatchers within kapp).

also what does * mean. is it glob for one level or multiple levels?

[neil-hickey]

may be only Packages are important to filter since they are referenced by PackageInstall?

Good call, having PackageMetadata and/or other information about a Package that doesn't exist on a cluster / isn't viewable via a PackageRepository is probably confusing to a user. If we cannot install a package through a PackageInstall without the Package CR existing, we should be good to go regardless, right?

could you also take a look at other APIs carvel has within other tools. there might be some similarities to take advantage of (e.g. resourceMatchers within kapp).

Oh nice, wasn't aware of those, docs are here. I could definitely see the matcher idea as being useful and makes a powerful extendable filtering feature, i.e:

andMatcher:
  matchers:
  - hasRefName: ["example.com", "*.example.com"]
  - hasAuthor: ["VMware"]

also what does * mean. is it glob for one level or multiple levels?

I was viewing it as a N levels deep comparison, something like a.*.com would match to a.b.com or a.b.c.d.com. Technically, haven't quite thought about the complexity just yet. I have used tools that required *.*.*.*.com and found it super confusing / verbose, so I am quite biased!

@cppforlife ^