Porch: BaseRevision controller aka Fan Out controller - but more

[johnbelamaric]

Describe your problem

In the Nephio controller PoC I looked at a fan-out controller. We have also discussed this and related issues in #3455, #3387, #3347, #3348 and many other places.

I put together a proposal for a resource and controller that does basic management of the upstream/downstream relationship. Right now it is in gdoc form right now because it's easier to manage comments and collaborate. Please take a look, at some point we can move this to a design proposal.

https://docs.google.com/document/d/1EzUUDxLm5jlEG9d47AQOxA2W6HmSWVjL1zqyIFkqV1I/edit?usp=sharing

[natasha41575]

I have a draft of a design, with active discussion here: https://docs.google.com/document/d/1OxNon_1ri4YOqNtEQivBgeRzIPuX9sOyu-nYukjwN1Q/edit?usp=sharing&resourcekey=0-2nDYYH5Kw58IwCatA4uDQw

Feedback is welcome.

[justinsb]

The concept behind the proposals sound good. In terms of functionality that is missing in kpt:

• We have been adding the mechanisms for controllers (watch, etc). These changes are now merged.
• I have an integration branch that shows how to create Draft packages (it's creating child packages, but we could add functionality / create another controller to update package instances when the upstream changes): klippy: a controller that auto-instantiates packages #3575
• I think we probably want to mark these Draft packages as "machine created" in some way. Labels would be a good start, maybe a structured field. mortent@ is looking into label support (and more!).

Directionally, I like the idea of storing some sort of metadata around when to propose updates in a CRD. I suggest we should start with the simple strategy ("propose asap") - there are still plenty of challenges here e.g. what happens when there's a second update before the first is approved. Then when we have this we can evaluate whether we want to delay proposing updates with a policy CRD. I could see an alternative world where we generate them all but somehow prioritize them / mark them as blocked (with conditions?).

@natasha41575 if you want to do this, I think you're unblocked from doing this now - I think the klippy PR doesn't have anything left in it that can't be hacked around easily (and I'm whittling it down). But if there are any blockers, let me know!

[natasha41575]

Taking a look at the nephio demo controller and comparing it to the DownstreamPackage discussed in my above doc, I think there are a number of similarities and perhaps functionality that we can borrow from the nephio controller and put it in the DownstreamPackage controller.

What the two controllers should definitely have in common is:

• Creating package revisions according to the spec based on input upstreams and target downstreams.
• Performing package updates when the upstream revision in the CR's spec changes. This means updating a
  draft if there is one, or creating a new, updated package revision if there aren't any drafts to update.
• Proposing deletion for package revisions that are no longer needed.

We can put this functionality into a common library for both controllers to share.

Beyond that, the nephio PackageDeployment controller has some other interesting ideas that we could borrow. Namely, the nephio PackageDeployment controller has this idea of "injection". For the nephio project, there are these "Cluster" objects in the cluster that provide a way to target a downstream repository. The nephio PackageDeployment CR maps upstream repositories to these Cluster objects.

The nephio controller has logic to perform an "injection" if these conditions are met:

• There is a resource in the kpt package with the annotation automation.nephio.org/config-injection: true.
• There is a resource in the cluster whose name and namespace matches the name and namespace of the associated Cluster object, and the GVK of the binding resource in the kpt package.

If these conditions are met, the injection occurs. In this case, injection means that the spec of the resource in the kpt package gets overwritten by the spec of the resource in the cluster. In this case (assuming there is only one package revision draft at a time), the controller edits the existing draft package revision if there is one, or creates a new package revision otherwise.

I found this idea of injection not only interesting but also useful (particularly in conjunction with the watches we added there), so I'd like to include some version of this in the DownstreamPackage controller that we will add to porch. But two things will have to be different:

1. Instead of the annotation key automation.nephio.org/config-injection, we should use something porch related. @mortent Do we have any existing convention for porch-related annotations? If not, maybe something like porch.kpt.dev/config-injection?

2. This "Cluster" object is a nephio CRD, so we can't (and shouldn't) use a Cluster object's name and namespace to target resources in the cluster, which is what the nephio PackageDeployment controller does. I'm thinking that the DownstreamPackage controller could have additional fields to explicitly specify these. The UpstreamPolicy creates the DownstreamPackage, so it would have to take in some configuration about where to read them from its selected objects.

For example, for the UpstreamPolicy/DownstreamPackage to handle the use case of the nephio PackageDeployment, it might look something like:

apiVersion: config.porch.kpt.dev/v1alpha1
kind: DownstreamPackage
metadata:
  name: my-dp
  namespace: default 
spec:
  upstream:
    repo: my-upstream-repo
    package: my-upstream-package-name
    revision: v1
  downstream:
    repo: my-downstream-repo
    name: my-downstream-package-name
  clusterObjectMeta:
    matchName: foo 
    matchNamespace: default

and

apiVersion: config.porch.kpt.dev/v1alpha1
kind: UpstreamPolicy
metadata:
  name: my-upstream-policy
  namespace: default 
spec:
   upstream:
     package: 
       repo: my-upstream-repo
       name: my-upstream-package-name
     revision: v1
  targets:
    - objects:
        selectors: 
        - apiVersion: infra.nephio.org/v1alpha1
          kind: Cluster
          labels:
            foo: bar
      repoName:
        fromField: "repositoryRef.name"
      packageName: 
        value: my-downstream-package-name
      clusterObjectMeta:
        matchName: "metadata.name" # some field path in the selected object
        matchNamespace: "metadata.namespace" # some field path in the selected object

This is just a rough idea to explain what I'm thinking at a high level; we could refine the syntax in the doc. @johnbelamaric WDYT? Does this make sense?

[johnbelamaric]

Yes, that makes sense to me, looks very cool. A couple of thoughts:

1. PackageDeployment controller also specifies a few other things that can be done on the downstream package:

   • Set annotations and labels
   • Create a namespace resource in the package (and we should call set-namespace but I don't think we do yet?)

   Any thoughts on those aspects?

2. Instead of clusterObjectMeta, maybe we call it something like injectionSelector and allow name/namespace/labels? Not sure what to do if there are multiple matches, though.

3. We should pick some sensible defaults for the target fields, for example maybe:

   • repoName: required, no default - or we could default to the target object name. Do we need a namespace here too?
   • packageName: default to upstream package name
   • injectionSelector: default to either no injection, or to name/namespace of target object

4. We have discussed allowing a set of mutators to be called on the downstream package as well. We may want to think about that too. This could be the way namespace is handled, rather than what we do today. This can probably come later but might be interesting to think about how it would look in the API.

5. I think maybe it's time to combine this and the gdoc (maybe both gdocs) into a GH design proposal that's more discoverable on GH. WDTY?

With the defaults, your PD-like UpstreamPolicy would become really simple.

[johnbelamaric]

I wonder if PackageVariant is a better name than DownstreamPackage. The idea being that this is effectively a resource that clones a package, applies config mutations via injection and kpt functions, and creates a new variant of the package based on that.

[natasha41575]

I wonder if PackageVariant is a better name than DownstreamPackage.

👍 I like this. I can change the name in a subsequent PR.

[natasha41575]

@johnbelamaric The basic implementation is done, so wdyt about closing this issue? I think other enhancements are captured by #3827, and we can discuss release of these controllers separately

[johnbelamaric]

Sounds good! Closing.