Local and relative dependencies are now relative to their operators (#…

…1709)

Summary:
Previously the location of a dependency operator was always relative to the caller working director (the directory of the `kudo install ...` call). This made installing and testing operators with dependencies a brittle process as the working directory has to be set accordingly. This PR fixes this for *local operators in directories:* so with an operator structure like this:

```
.
├── child
│   └── operator.yaml
│
└── parent
    └── operator.yaml
```

child operator now has to be referenced like:

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

*Note:* This fix does not apply to tarballs as the tarball reader does not expect any dependency operators' files/folders.

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

pkg/controller/instance/instance_controller.go

@@ -329,7 +329,7 @@ func (r *Reconciler) resolveDependencies(i *kudoapi.Instance, ov *kudoapi.Operat
 	}
 	resolver := &InClusterResolver{ns: ov.Namespace, c: r.Client}
 
-	_, err := dependencies.Resolve(ov, resolver)
+	_, err := dependencies.Resolve(ov.Name, ov, resolver)
 	if err != nil {
 		return engine.ExecutionError{Err: fmt.Errorf("%w%v", engine.ErrFatalExecution, err), EventName: "CircularDependency"}
 	}

pkg/kudoctl/cmd/install/install.go

@@ -10,6 +10,7 @@ import (
 	"github.com/kudobuilder/kudo/pkg/kudoctl/env"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/install"
 	pkgresolver "github.com/kudobuilder/kudo/pkg/kudoctl/packages/resolver"
+	deps "github.com/kudobuilder/kudo/pkg/kudoctl/resources/dependencies"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"
 )
 
@@ -91,6 +92,11 @@ func installOperator(operatorArgument string, options *Options, fs afero.Fs, set
 		return fmt.Errorf("failed to resolve operator package for: %s %w", operatorArgument, err)
 	}
 
+	dependencies, err := deps.Resolve(operatorArgument, pkg.Resources.OperatorVersion, resolver)
+	if err != nil {
+		return err
+	}
+
 	installOpts := install.Options{
 		SkipInstance:    options.SkipInstance,
 		CreateNamespace: options.CreateNameSpace,
@@ -107,6 +113,6 @@ func installOperator(operatorArgument string, options *Options, fs afero.Fs, set
 		settings.Namespace,
 		*pkg.Resources,
 		options.Parameters,
-		resolver,
+		dependencies,
 		installOpts)
 }

pkg/kudoctl/cmd/upgrade.go

@@ -11,6 +11,7 @@ import (
 	"github.com/kudobuilder/kudo/pkg/kudoctl/cmd/params"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/env"
 	pkgresolver "github.com/kudobuilder/kudo/pkg/kudoctl/packages/resolver"
+	deps "github.com/kudobuilder/kudo/pkg/kudoctl/resources/dependencies"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/resources/upgrade"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"
 )
@@ -101,15 +102,20 @@ func runUpgrade(args []string, options *options, fs afero.Fs, settings *env.Sett
 	if err != nil {
 		return fmt.Errorf("could not build operator repository: %w", err)
 	}
+
 	resolver := pkgresolver.New(repository)
 	pkg, err := resolver.Resolve(packageToUpgrade, options.AppVersion, options.OperatorVersion)
 	if err != nil {
 		return fmt.Errorf("failed to resolve operator package for: %s: %w", packageToUpgrade, err)
 	}
 
 	resources := pkg.Resources
-
 	resources.OperatorVersion.SetNamespace(settings.Namespace)
 
-	return upgrade.OperatorVersion(kc, resources.OperatorVersion, options.InstanceName, options.Parameters, resolver)
+	dependencies, err := deps.Resolve(packageToUpgrade, resources.OperatorVersion, resolver)
+	if err != nil {
+		return err
+	}
+
+	return upgrade.OperatorVersion(kc, resources.OperatorVersion, options.InstanceName, options.Parameters, dependencies)
 }

pkg/kudoctl/packages/install/package.go

@@ -7,7 +7,7 @@ import (
 	kudoapi "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
-	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/resolver"
+	deps "github.com/kudobuilder/kudo/pkg/kudoctl/resources/dependencies"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/resources/install"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
 )
@@ -34,7 +34,7 @@ func Package(
 	namespace string,
 	resources packages.Resources,
 	parameters map[string]string,
-	resolver resolver.Resolver,
+	dependencies []deps.Dependency,
 	options Options) error {
 	clog.V(3).Printf(
 		"Preparing %s/%s:%s for installation",
@@ -65,7 +65,7 @@ func Package(
 	}
 
 	if err := install.OperatorAndOperatorVersion(
-		client, resources.Operator, resources.OperatorVersion, resolver); err != nil {
+		client, resources.Operator, resources.OperatorVersion, dependencies); err != nil {
 		return err
 	}
 

pkg/kudoctl/resources/dependencies/resolve.go

@@ -2,7 +2,10 @@ package dependencies
 
 import (
 	"fmt"
+	"path/filepath"
+	"strings"
 
+	"github.com/spf13/afero"
 	"github.com/thoas/go-funk"
 	"github.com/yourbasic/graph"
 
@@ -49,10 +52,13 @@ type Dependency struct {
 	PackageName string
 }
 
-// Resolve resolves all dependencies of an OperatorVersion.
-// Dependencies are resolved recursively.
-// Cyclic dependencies are detected and result in an error.
-func Resolve(operatorVersion *kudoapi.OperatorVersion, resolver pkgresolver.Resolver) ([]Dependency, error) {
+// Resolve resolves all dependencies of an OperatorVersion. Dependencies are resolved recursively.
+// Cyclic dependencies are detected and result in an error. operatorArgument parameter is string that
+// the user passed to install/upgrade an operator which is used to determine whether this is a local
+// operator in a directory. In that case all its relative dependencies (if any exist) will be relative
+// to the operator directory (the one with `operator.yaml` file).
+// See github.com/kudobuilder/kudo/issues/1701 for additional context.
+func Resolve(operatorArgument string, operatorVersion *kudoapi.OperatorVersion, resolver pkgresolver.Resolver) ([]Dependency, error) {
 	root := packages.Resources{
 		OperatorVersion: operatorVersion,
 	}
@@ -66,7 +72,10 @@ func Resolve(operatorVersion *kudoapi.OperatorVersion, resolver pkgresolver.Reso
 		edges: []map[int]struct{}{{}},
 	}
 
-	if err := dependencyWalk(&dependencies, &g, root, 0, resolver); err != nil {
+	// Here we only care whether the path is absolute or not so we can ignore the error
+	operatorDir, _ := operatorAbsPath(operatorArgument)
+
+	if err := dependencyWalk(&dependencies, &g, root, 0, resolver, operatorDir); err != nil {
 		return nil, err
 	}
 
@@ -79,15 +88,34 @@ func dependencyWalk(
 	g *dependencyGraph,
 	parent packages.Resources,
 	parentIndex int,
-	resolver pkgresolver.Resolver) error {
+	resolver pkgresolver.Resolver,
+	operatorDirectory *string) error {
 	//nolint:errcheck
 	childrenTasks := funk.Filter(parent.OperatorVersion.Spec.Tasks, func(task kudoapi.Task) bool {
 		return task.Kind == engtask.KudoOperatorTaskKind
 	}).([]kudoapi.Task)
 
 	for _, childTask := range childrenTasks {
+		childPackageName := childTask.Spec.KudoOperatorTaskSpec.Package
+
+		// if the path to a child dependency is a relative one, we construct the absolute path for the
+		// resolver by combining the absolute path for the operator directory with the relative dependency path
+		// a relative dependency path must begin with either './' or '../'
+		isRelativePackage := strings.HasPrefix(childPackageName, "./") || strings.HasPrefix(childPackageName, "../")
+		if isRelativePackage {
+			if operatorDirectory == nil {
+				return fmt.Errorf("a dependency with a relative path %q is only allowed in a parent %v when it is a local directory", childPackageName, parent.OperatorVersion.FullyQualifiedName())
+			}
+			childDirectory, err := operatorAbsPath(filepath.Join(*operatorDirectory, childPackageName))
+			if err != nil {
+				return fmt.Errorf("local dependency %s of the parent %s has an invalid path: %v", fullyQualifiedName(childTask.Spec.KudoOperatorTaskSpec), parent.OperatorVersion.FullyQualifiedName(), err)
+			}
+			childPackageName = *childDirectory
+
+		}
+
 		childPkg, err := resolver.Resolve(
-			childTask.Spec.KudoOperatorTaskSpec.Package,
+			childPackageName,
 			childTask.Spec.KudoOperatorTaskSpec.AppVersion,
 			childTask.Spec.KudoOperatorTaskSpec.OperatorVersion)
 		if err != nil {
@@ -123,7 +151,11 @@ func dependencyWalk(
 
 		// We only need to walk the dependencies if the package is new
 		if newPackage {
-			if err := dependencyWalk(dependencies, g, *childPkg.Resources, childIndex, resolver); err != nil {
+			var childOperatorDirectory *string
+			if isRelativePackage {
+				childOperatorDirectory = &childPackageName
+			}
+			if err := dependencyWalk(dependencies, g, *childPkg.Resources, childIndex, resolver, childOperatorDirectory); err != nil {
 				return err
 			}
 		}
@@ -157,3 +189,19 @@ func fullyQualifiedName(kt kudoapi.KudoOperatorTaskSpec) string {
 
 	return fmt.Sprintf("Operator: %q, OperatorVersion: %q, AppVersion %q", kt.Package, operatorVersion, appVersion)
 }
+
+func operatorAbsPath(path string) (*string, error) {
+	fs := afero.NewOsFs()
+
+	ok, err := afero.IsDir(fs, path)
+	// force local operators usage to be either absolute or express a relative path
+	// or put another way, a name can NOT be mistaken to be the name of a local folder
+	if ok && filepath.Base(path) != path {
+		abs, err := filepath.Abs(path)
+		if err == nil {
+			return &abs, nil
+		}
+		return nil, fmt.Errorf("failed to detect an absolute path for %q: %v", path, err)
+	}
+	return nil, fmt.Errorf("%q is not a valid directory: %v", path, err)
+}

pkg/kudoctl/resources/dependencies/resolve_test.go

@@ -12,6 +12,7 @@ import (
 	kudoapi "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
 	engtask "github.com/kudobuilder/kudo/pkg/engine/task"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
+	pkgresolver "github.com/kudobuilder/kudo/pkg/kudoctl/packages/resolver"
 )
 
 type nameResolver struct {
@@ -223,7 +224,8 @@ func TestResolve(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			resolver := nameResolver{tt.pkgs}
-			got, err := Resolve(tt.pkgs[0].Resources.OperatorVersion, resolver)
+			operatorArg := tt.pkgs[0].Resources.OperatorVersion.Name
+			got, err := Resolve(operatorArg, tt.pkgs[0].Resources.OperatorVersion, resolver)
 
 			assert.Equal(t, err == nil, tt.wantErr == "")
 
@@ -241,3 +243,65 @@ func TestResolve(t *testing.T) {
 		})
 	}
 }
+
+func Test_isLocalDirPackage(t *testing.T) {
+	tests := []struct {
+		name        string
+		path        string
+		wantNilPath bool
+		wantErr     bool
+	}{
+		{
+			name:        "./",
+			path:        "./",
+			wantNilPath: false,
+			wantErr:     false,
+		},
+		{
+			name:        "../install",
+			path:        "../install",
+			wantNilPath: false,
+			wantErr:     false,
+		},
+		{
+			name:        "./some-fake-path",
+			path:        "./some-fake-path",
+			wantNilPath: true,
+			wantErr:     true,
+		},
+		{
+			name:        "./",
+			path:        "./resolve_test.go",
+			wantNilPath: true,
+			wantErr:     true,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			absPath, err := operatorAbsPath(tt.path)
+			assert.Equal(t, tt.wantErr, err != nil)
+			assert.Equal(t, tt.wantNilPath, absPath == nil)
+		})
+	}
+}
+
+func TestResolveLocalDependencies(t *testing.T) {
+	var resolver = pkgresolver.NewLocal()
+	var operatorArgument = "./testdata/operator-with-dependencies/parent-operator"
+
+	pkg, err := resolver.Resolve(operatorArgument, "", "")
+	assert.NoError(t, err, "failed to resolve operator package for %s", operatorArgument)
+
+	dependencies, err := Resolve(operatorArgument, pkg.Resources.OperatorVersion, resolver)
+	assert.NoError(t, err, "failed to resolve dependencies for %s", operatorArgument)
+	assert.Equal(t, len(dependencies), 1, "expecting to find child-operator dependency")
+
+	assert.NotNil(t, dependencies[0].Operator, "expecting to find child-operator OperatorVersion resource")
+	assert.NotNil(t, dependencies[0].OperatorVersion, "expecting to find child-operator OperatorVersion resource")
+	assert.NotNil(t, dependencies[0].Instance, "expecting to find child-operator OperatorVersion resource")
+
+	assert.Equal(t, dependencies[0].Operator.Name, "child")
+	assert.Equal(t, dependencies[0].OperatorVersion.Name, kudoapi.OperatorVersionName("child", "0.0.1"))
+	assert.Equal(t, dependencies[0].Instance.Name, kudoapi.OperatorInstanceName("child"))
+}

pkg/kudoctl/resources/dependencies/testdata/operator-with-dependencies/child-operator/operator.yaml

@@ -0,0 +1,25 @@
+apiVersion: kudo.dev/v1beta1
+name: "child"
+operatorVersion: "0.0.1"
+kubernetesVersion: 1.15.0
+maintainers:
+  - name: zen-dog
+    email: <your@email.com>
+url: https://kudo.dev
+tasks:
+  - name: deploy
+    kind: Dummy
+    spec:
+      done: true
+      wantErr: false
+
+plans:
+  deploy:
+    strategy: serial
+    phases:
+      - name: main
+        strategy: parallel
+        steps:
+          - name: deploy
+            tasks:
+              - deploy

pkg/kudoctl/resources/dependencies/testdata/operator-with-dependencies/child-operator/params.yaml

@@ -0,0 +1,6 @@
+apiVersion: kudo.dev/v1beta1
+parameters:
+  - name: deploy
+    description: Triggers the deploy plan
+    default: 1
+    trigger: deploy

pkg/kudoctl/resources/dependencies/testdata/operator-with-dependencies/parent-operator/operator.yaml

@@ -0,0 +1,25 @@
+apiVersion: kudo.dev/v1beta1
+name: "parent"
+operatorVersion: "0.1.0"
+kubernetesVersion: 1.15.0
+maintainers:
+  - name: zen-dog
+    email: <your@email.com>
+url: https://kudo.dev
+tasks:
+  - name: deploy-child
+    kind: KudoOperator
+    spec:
+      package: "../child-operator"
+      operatorVersion: 0.0.1
+
+plans:
+  deploy:
+    strategy: serial
+    phases:
+      - name: main
+        strategy: parallel
+        steps:
+          - name: deploy
+            tasks:
+              - deploy-child

pkg/kudoctl/resources/dependencies/testdata/operator-with-dependencies/parent-operator/params.yaml

@@ -0,0 +1,6 @@
+apiVersion: kudo.dev/v1beta1
+parameters:
+  - name: deploy
+    description: Triggers the deploy plan
+    default: 1
+    trigger: deploy

pkg/kudoctl/resources/install/operator.go

@@ -8,7 +8,6 @@ import (
 	kudoapi "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
 	engtask "github.com/kudobuilder/kudo/pkg/engine/task"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
-	"github.com/kudobuilder/kudo/pkg/kudoctl/packages/resolver"
 	deps "github.com/kudobuilder/kudo/pkg/kudoctl/resources/dependencies"
 	"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
 )
@@ -20,7 +19,7 @@ func OperatorAndOperatorVersion(
 	client *kudo.Client,
 	operator *kudoapi.Operator,
 	operatorVersion *kudoapi.OperatorVersion,
-	resolver resolver.Resolver) error {
+	dependencies []deps.Dependency) error {
 	if !client.OperatorExistsInCluster(operator.Name, operator.Namespace) {
 		if _, err := client.InstallOperatorObjToCluster(operator, operator.Namespace); err != nil {
 			return fmt.Errorf(
@@ -42,7 +41,7 @@ func OperatorAndOperatorVersion(
 	}
 
 	if !funk.ContainsString(versionsInstalled, operatorVersion.Spec.Version) {
-		if err := installDependencies(client, operatorVersion, resolver); err != nil {
+		if err := installDependencies(client, operatorVersion, dependencies); err != nil {
 			return fmt.Errorf(
 				"failed to install dependencies of operatorversion %s/%s: %v",
 				operatorVersion.Namespace,
@@ -67,12 +66,7 @@ func OperatorAndOperatorVersion(
 	return nil
 }
 
-func installDependencies(client *kudo.Client, ov *kudoapi.OperatorVersion, resolver resolver.Resolver) error {
-	dependencies, err := deps.Resolve(ov, resolver)
-	if err != nil {
-		return err
-	}
-
+func installDependencies(client *kudo.Client, ov *kudoapi.OperatorVersion, dependencies []deps.Dependency) error {
 	// The KUDO controller will create Instances for the dependencies. For this
 	// it needs to resolve the dependencies again from 'KudoOperatorTaskSpec'.
 	// But it cannot resolve packages like the CLI, because it may