Tarball packages can now contain local dependencies #1728

zen-dog · 2020-10-24T16:03:13Z

Summary:
Currently, KUDO allows local dependencies like package: ../child or ../child.tgz however, only if the parent operator is also in a local directory. This PR adds the possibility for a packaged operator (a tarball) to contain all the dependencies it needs. While, the preferred way is to keep the individual operators in the repo, sometimes a self-contained operator package (like an uber-jar) is more desirable e.g.:

an air-gaped environment needs a functioning repo before operator installation which significantly increases the overall deployment overhead
some dependency operators might be heavily customized official ones, which makes it hard to contribute them back to the official repo

To keep the packages backward-compatible, we continue to expect the parent operator at the top-level of the tarball, so that all the dependencies must share the same base path and be at least one level deeper e.g.:

.
└── child
│   └── operator.yaml
└── operator.yaml

A dependency operator (child above) is always relative to the parent operator.yaml which declares it. In this case, it should be referenced like:

  - name: child
    kind: KudoOperator
    spec:
      package: "./child"

See this commit for more information.

Fixes #1701

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

Summary: Currently, KUDO allows local dependencies like `package: ../child` or `../child.tgz` however, only if the parent operator is *also* in a local directory. This PR adds the possibility for a packaged operator (a tarball) to contain all the dependencies it needs. While, the preferred way is to keep the individual operators in the repo, sometimes a self-contained operator package (like an uber-jar) is more desirable e.g.: - an air-gaped environment needs a functioning repo before operator installation which significantly increases the overall deployment overhead - some dependency operators might be heavily customized official ones, which makes it hard to contribute them back to the official repo To keep the packages backward-compatible, we continue to expect the parent operator at the top-level of the tarball, so that all the dependencies must share the same base path and be at least one level deeper e.g.: ``` . └── child │ └── operator.yaml └── operator.yaml ``` A dependency operator (`child` above) can have its dependencies which are always relative to the `operator.yaml` which declares them. See [this commit](c237347) for more information. Fixes #1701 Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog · 2020-10-24T16:07:22Z

pkg/kudoctl/packages/resolver/resolver.go

 // PackageResolver is the source of resolver of operator packages.
 type PackageResolver struct {
-	local *LocalResolver
-	uri   *URLResolver
+	local *LocalHelper


Previously, LocalResolver and URLResolver would implement the Resolver interface:

Resolve(name string, appVersion string, operatorVersion string) (*packages.Resources, error)

However, they are merely helpers to download/parse the directory/tarballs and would ignore the appVersion and operatorVersion parameters. I kept their overall structure but they don't implement the interface anymore.

zen-dog · 2020-10-24T16:08:16Z

pkg/kudoctl/packages/resolver/resolver.go

@@ -10,32 +12,30 @@ import (
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"
 )

-// Resolver will try to resolve a given package name to either local tarball, folder, remote url or


I had to move the Resolver interface one level higher to avoid import cycles.

zen-dog · 2020-10-24T16:11:47Z

pkg/kudoctl/resources/dependencies/resolve.go

@@ -103,18 +96,10 @@ func dependencyWalk(
 		// a relative dependency path must begin with either './' or '../'
 		isRelativePackage := strings.HasPrefix(childPackageName, "./") || strings.HasPrefix(childPackageName, "../")
 		if isRelativePackage {
-			if operatorDirectory == nil {


There is no need in this whole dance, now that dependencies come with the scope-aware dependency resolver.

zen-dog · 2020-10-24T16:23:01Z

pkg/kudoctl/resources/install/operator.go

@@ -67,18 +66,6 @@ func OperatorAndOperatorVersion(
 }

 func installDependencies(client *kudo.Client, ov *kudoapi.OperatorVersion, dependencies []deps.Dependency) error {
-	// The KUDO controller will create Instances for the dependencies. For this


While working on this PR, I realized that updating the tasks here (in L80) had a bug, now that dependencies are relative to their parent. Consider a folder structure like:

. └── child # first child │ └── child # second child │ │ └── operator.yaml │ └── operator.yaml └── operator.yaml

Now, the parent references the first child as ./child and it references the second child as ./child too. The updateKudoOperatorTasks method below would search the dependencies by the dependency.PackageName and update the corresponding task but with two identical PackageNames this would lead to bugs.

I've moved this logic in the resolve.go::dependencyWalk method. Now each task is updated when the dependency is resolved (which is arguably a much better place to do that).

This setup would lead to issues anyway, right? Because the operatorName for both children is child? It may work if they have different appVersion/operatorVersion, but if those are the same too, the generated name would clash on installation of the OV.

./child is the folder name, the operator name is known after it is resolved (and can be something else completely) 🤷

Ah, right. Yeah, if the operator names are different then it should work. Yeah 👍

zen-dog · 2020-10-24T16:24:09Z

pkg/kudoctl/resources/dependencies/resolve.go

+		//
+		// so that the KUDO controller to be able to grab the right 'OperatorVersion' resources from the cluster
+		// when the task is executed.
+		err = updateResolvedKudoOperatorTask(childTask.Name,


See this comment on why this logic has been moved here.

nfnt

First iteration of review. Looks great so far, my concerns are mostly around naming, because "resolve" is getting super overloaded. Will do another iteration to verify that combinations of tarball, directory and URL dependencies work as expected. I think they do, just need to double-check.

pkg/kudoctl/packages/reader/reader_tar.go

nfnt · 2020-10-26T13:57:56Z

pkg/kudoctl/packages/reader/reader_tar.go

+// ResourcesFromTar extracts files from the tar provides by he `inFs` and the `path` and converts
+// them to resources. All the extracted files are saved in the `outFs` for later use (e.g. searching
+// for local dependencies)
+func ResourcesFromTar(in afero.Fs, out afero.Fs, path string) (*packages.Resources, error) {


Not a fan of having a function parameter that is an output value. Let's make this a return value. I see that this is done to inject a afero.MemMapFs, but can't ExtractTar just create one itself? I don't see why it's important to be able to chose the afero.Fs implementation as a caller here, as it's only called with afero.MemMapFs. Unless, of course, I'm missing something :)

Well... In this case I'm not opposed to passing in the output FS - It would allow to extract multiple tar files into the same MemFS, or into the real Filesystem, etc.

I'm not sure about the Naming though - ExtractTarIntoFs might be more fitting. I'll have to look into it :)

I went back and forth on passing out as a parameter or simply returning it. In the end, I decided to pass it, with the following rationale: the caller of this method might very well decide to pass in a different kind of Fs and frankly, this should be ok 🤷 And, as @ANeumann82 mentioned above, it makes extracting multiple tgzs into one Fs easier.

I agree that this is the best way to abstract away the filesystem used in these calls. However, we only use it with afero.MemMapFs and don't need this, IMO. But I don't feel strongly about this. Though this makes the function signatures a bit harder to parse. Great, that out is used as parameter name to clearly indicate the parameters purpose.

pkg/kudoctl/packages/reader/reader_tar.go

pkg/kudoctl/packages/resolver/resolver.go

pkg/kudoctl/packages/resolver/resolver_url.go

pkg/kudoctl/packages/resolver/resolver_local.go

pkg/kudoctl/packages/types.go

ANeumann82 · 2020-10-27T10:53:31Z

First iteration of review. Looks great so far, my concerns are mostly around naming, because "resolve" is getting super overloaded. Will do another iteration to verify that combinations of tarball, directory and URL dependencies work as expected. I think they do, just need to double-check.

Thanks :) This was handed over from zen-dog, and I'm not sure it's already ready for review, he said that it needs polishing. Thanks for the comments, i'll make a pass now

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog · 2020-10-27T22:05:36Z

pkg/kudoctl/packages/resolver/resolver.go

-	return &InClusterResolver{c: c, ns: ns}
+func (m *PackageResolver) copyWithChangedWorkingDir(workingDir string) packages.Resolver {
+	return &PackageResolver{
+		local: newForFilesystem(m.local.fs, workingDir),


Much better 👏 👏

zen-dog · 2020-11-10T09:33:26Z

@nfnt would you have time to look at this PR again? I believe most of your concerns were addressed.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # pkg/kudoctl/packages/reader/reader.go # pkg/kudoctl/packages/reader/reader_test.go # pkg/kudoctl/packages/resolver/resolver_local.go

Outdated

zen-dog · 2020-12-03T17:15:18Z

Don't forget to put yourself as co-author @ANeumann82 😉

zen-dog requested review from alenkacz, gerred, kensipe and nfnt as code owners October 24, 2020 16:03

zen-dog commented Oct 24, 2020

View reviewed changes

zen-dog requested a review from ANeumann82 October 24, 2020 16:28

zen-dog assigned ANeumann82 Oct 24, 2020

nfnt previously requested changes Oct 27, 2020

View reviewed changes

ANeumann82 added 3 commits October 27, 2020 14:57

First pass of applied reviews

10bf3e9

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

One more rename

98d321e

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Abstracted the directory fully into the resolver

482183d

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog commented Oct 27, 2020

View reviewed changes

ANeumann82 added 2 commits November 20, 2020 16:02

Merge branch 'main' into ad/deps-in-tarball

436252f

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> # Conflicts: # pkg/kudoctl/packages/reader/reader.go # pkg/kudoctl/packages/reader/reader_test.go # pkg/kudoctl/packages/resolver/resolver_local.go

Merge branch 'main' into ad/deps-in-tarball

3080a9c

ANeumann82 approved these changes Nov 27, 2020

View reviewed changes

ANeumann82 merged commit 63fbe53 into main Dec 11, 2020

ANeumann82 deleted the ad/deps-in-tarball branch December 11, 2020 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tarball packages can now contain local dependencies #1728

Tarball packages can now contain local dependencies #1728

zen-dog commented Oct 24, 2020 •

edited

Loading

zen-dog Oct 24, 2020

zen-dog Oct 24, 2020

zen-dog Oct 24, 2020

zen-dog Oct 24, 2020

ANeumann82 Oct 27, 2020

zen-dog Oct 27, 2020 •

edited

Loading

ANeumann82 Oct 28, 2020

zen-dog Oct 24, 2020

nfnt left a comment

nfnt Oct 26, 2020

ANeumann82 Oct 27, 2020

zen-dog Oct 27, 2020 •

edited

Loading

nfnt Oct 27, 2020

ANeumann82 commented Oct 27, 2020

zen-dog Oct 27, 2020

zen-dog commented Nov 10, 2020

zen-dog commented Dec 3, 2020

Tarball packages can now contain local dependencies #1728

Tarball packages can now contain local dependencies #1728

Conversation

zen-dog commented Oct 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zen-dog Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfnt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zen-dog Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ANeumann82 commented Oct 27, 2020

Choose a reason for hiding this comment

zen-dog commented Nov 10, 2020

zen-dog commented Dec 3, 2020

zen-dog commented Oct 24, 2020 •

edited

Loading

zen-dog Oct 27, 2020 •

edited

Loading

zen-dog Oct 27, 2020 •

edited

Loading