PoC: Resolution CLI

[m1kola]

Description

This is a PoC for #206.

Right now the tool is capable of resolving a package without a cluster: it is possible to provide a FBC (dir or an image) into the tool as well as a directory containing manifests with Operator objects.

The resolution tool will use Operator from the input manifests and consider them required (as if they were installed on a cluster).

Once we merge this, the next steps will be:

• Gather feedback from the community using this GitHub discussion - Off Cluster CLI Tools #262
• Plan next iterations based on feedback

Also there are a few things going in parallel. For example, we will likely change significantly how we build resolution problems (see operator-framework/deppy#96 and relevant RFC). Once we have more or less stable way to build the resolution problem - we will be able to create some sort of library to share between operator-controller and resolution CLI.

Example usage:

# Resolve prometheus from catalog quay.io/operatorhubio/catalog:latest from default channel
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus from catalog dir ~/Downloads/index from default channel
$ go run ./cmd/resolutioncli/ -index-ref ~/Downloads/index -package-name prometheus
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus version 0.65.1 from default channel
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -package-version 0.65.1
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus version 0.47.0 from default channel
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -package-version 0.47.0
quay.io/operatorhubio/prometheus@sha256:47c659b352bbc4b75b64fbe0077d19afaea80cf691c62d803d44fdfc88dc9c6f

# Resolve prometheus version 0.47.0 from stable channel which doesn't exist (beta is default channel)
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -package-version 0.47.0 -package-channel stable
package 'prometheus' at version '0.47.0' in channel 'stable' not found
exit status 1

# Resolve prometheus version 0.47.0 from beta channel
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -package-version 0.65.1 -package-channel beta
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus from default channel and reading input manifests from ./operator-input/ dir
# Note: at the moment, resolution tool only takes Operator kind as an input and considers it to be
# a required package.
$ mkdir operator-input
$ cat <<EOT > operator-input/operator_v1alpha1.yaml
apiVersion: operators.operatorframework.io/v1alpha1
kind: Operator
metadata:
  name: operator-sample
spec:
  packageName: argocd-operator
EOT
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -input-dir ./operator-input/
quay.io/operatorhubio/prometheus@sha256:85decebfdfe993484834bd722d93fb44725b3a56cb4f9ebc4e2fd398bdcbad54

# Resolve prometheus from default channel and reading input manifests from ./operator-input/ dir
# and prometheus package already exists in the input (e.g. already installed on the cluster)
$ mkdir operator-input
$ cat <<EOT > operator-input/operator_v1alpha1.yaml
apiVersion: operators.operatorframework.io/v1alpha1
kind: Operator
metadata:
  name: operator-sample
spec:
  packageName: prometheus
  version: 0.47.0
EOT
$ go run ./cmd/resolutioncli/ -index-ref quay.io/operatorhubio/catalog:latest -package-name prometheus -input-dir ./operator-input/
duplicate identifier "required package prometheus" in input
exit status 1

Reviewer Checklist

• API Go Documentation
• Tests: Unit Tests (and E2E Tests, if appropriate)
• Comprehensive Commit Messages
• Links to related GitHub Issue(s)

[everettraven]

Overall this looks good to me with the idea in mind that this still has a good bit of follow up work to do to get it production ready. I had a minor nit but nothing that should block this PR IMO.

I'm not very well versed in the variable source / deppy stuff so I'd definitely prefer someone more familiar with that review it prior to merging in case I've missed something.

Great work @m1kola !

[everettraven]

Nit: If these flags are required would it make more sense to make them positional arguments? IMO flags should be reserved for configurable options.

I noticed we aren't using spf13/cobra for building the CLI commands, but it has some nice support for setting required number of arguments that we could use here and automatically generates a pretty nice help message that could be used to show the required positional arguments.

[tmshort]

I'm a fan of explicit arguments; it can get confusing with positional unless there's an explicit logic (e.g. kubectl's verb type [name])

[m1kola]

Package name probably makes sense as a positional argument and everything else as flags. But I suggest that we leave it as is for now and come back to discuss details of the interface once start work on finalising this CLI.

We are not using cobra right now, but I think we should use it in the production-ready version.

[tmshort]

Some small changes (mostly copyright years). However, I would like to see this incorporated in the Makefile as either a target that's automatically built, or a separate target.

[tmshort]

This appears to add CRD Uniqueness and Bundle variable sources to the list, but I don't see them in the new code?
Unless it's wrapped by NewOLMVariableSource, but I don't see it being used.
I guess it's the NewOLMVariableSource() "constructor" that's adding these, rather than the GetVariables?

[m1kola]

I had to remove this chaining within the variable source becase for off-cluster usage we need a bit different set of variable soruces (e.g. we need a variable source which creates a variable from CLI inputs).

instead of chaining I now use NestedVariableSource in NewOLMVariableSource (note after #273 it is NewOperatorVariableSource). NewOLMVariableSource is used in on-cluster operator and it is not in this diff (I did not change the calling code).

https://github.com/operator-framework/operator-controller/blob/9a1a15d7d7c7e069dab21ff5930052d13553e1fc/cmd/manager/main.go#L108C19-L108C19

But NewCRDUniquenessConstraintsVariableSource and NewBundlesAndDepsVariableSource are also used in off-cluster resolution here within olm.NestedVariableSource.

I suspect NestedVariableSource and probably whole composite.go will go away once we switch to the solution proposed in the RFC.

[tmshort]

Is the init() function necessary, give that this is a CLI?

[m1kola]

We can do it in main I think, but I was following the pattern from operator-controller's main.go:

operator-controller/cmd/manager/main.go

Lines 49 to 57 in ebc5cfe

	func init() {
	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
	utilruntime.Must(operatorsv1alpha1.AddToScheme(scheme))
	utilruntime.Must(rukpakv1alpha1.AddToScheme(scheme))
	utilruntime.Must(catalogd.AddToScheme(scheme))
	//+kubebuilder:scaffold:scheme
	}

[tmshort]

I'm a fan of explicit arguments; it can get confusing with positional unless there's an explicit logic (e.g. kubectl's verb type [name])

[codecov]

Codecov Report

Merging #246 (5aa27e5) into main (b7a737a) will increase coverage by 0.86%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
+ Coverage   76.13%   77.00%   +0.86%     
==========================================
  Files          13       15       +2     
  Lines         662      687      +25     
==========================================
+ Hits          504      529      +25     
  Misses        129      129              
  Partials       29       29              

Impacted Files	Coverage Δ
internal/controllers/variable_source.go	100.00% <100.00%> (ø)
internal/resolution/variablesources/composite.go	100.00% <100.00%> (ø)
internal/resolution/variablesources/operator.go	78.94% <100.00%> (+2.47%)	⬆️

[m1kola]

@tmshort I think I addressed your feedback + made sure this PR doesn't decrease test coverage.

However, I would like to see this incorporated in the Makefile as either a target that's automatically built, or a separate target.

I did not want to add this into makefile or goreleaser because it is still a PoC and a lot will change. Didn't want to make it "official" for now.

But if you think that we should still do that - I suggest that we do it in a separate PR.

[m1kola]

I moved it here from internal/resolution/variablesources/. The idea is that building blocks are in internal/resolution/variablesources/ and this composite variable source is close to where it will be used (in controller or in the off-cluster.

[m1kola]

These removed tests are mostly to cover inner variables sources which are no longer hardcoded inside (see this) and have their own tests (bundles_and_dependencies_test.go, required_package_test.go, etc).

[tmshort]

I would like to see a mMakefile entry for this, but perhaps not now.