Repository: crossplane/crossplane
Pull request #5809 (2024-07-01 00:13:03)

download/cache full graph of dependencies at `beta validate`
<!--
Thank you for helping to improve Crossplane! Please read the contribution docs
(linked below) if this is your first Crossplane pull request.
-->

### Description of your changes

<!--
Briefly describe what this pull request does, and how it is covered by tests.
Be proactive - direct your reviewers' attention to anything that needs special
consideration.

We love pull requests that fix an open issue. If yours does, use the below line
to indicate which issue it fixes, for example "Fixes #500".
-->
This PR enables 'beta validate' command to calculate all dependencies of the provided configurations and therefore the full graph of dependencies is downloaded/cached. Previously we were only downloading/caching the level-1 dependencies of a configuration which are the dependencies of the provided configuration.. 

Fixes https://github.com/crossplane/crossplane/issues/5803

Here are some examples with this functionality:

1. `configuration-aws-database:v0.10.0`
Dependency graph is: 
    - `provider-aws-rds`
    - `configuration-aws-network:v0.12.0`
        - `provider-aws-ec2:v1.2.0`
        - `function-patch-and-transform:v0.4.0`


    Current behavior:
    ```
    crossplane beta validate .work/xfn/config-aws-db.yaml .work/xfn/
                                                                                                   
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-aws-database:v0.10.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-aws-rds:v1.2.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-aws-network:v0.12.0
    ```
        
    New behavior:
    ```
    go run cmd/crank/main.go beta validate .work/xfn/config-aws-db.yaml .work/xfn/

    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-aws-database:v0.10.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-aws-rds:v1.2.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-aws-network:v0.12.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-aws-ec2:v1.2.0
    package schemas does not exist, downloading:  xpkg.upbound.io/crossplane-contrib/function-patch-and-transform:v0.4.0
    ```

2. `platform-ref-azure:v0.11.0`:
Dependency graph is: 
    - `provider-azure-containerservice:v0.42.1`
    - `configuration-azure-network:v0.8.0`
        - `provider-azure-network:v0.42.1`
        - `function-patch-and-transform:v0.4.0`
    - `configuration-azure-aks:v0.7.0`
        - `provider-azure-containerservice:v0.42.1`
        - `configuration-azure-network:v0.8.0`
            - `provider-azure-network:v0.42.1`
            - `function-patch-and-transform:v0.4.0`
        - `provider-helm:v0.17.0`
        - `provider-kubernetes:v0.12.1`
    - `configuration-azure-database:v0.11.1`
        - `provider-azure-dbformariadb:v0.42.1`
        - `provider-azure-dbforpostgresql:v0.42.1`
        - `configuration-azure-network:v0.8.0`
            - `provider-azure-network:v0.42.1`
            - `function-patch-and-transform:v0.4.0`
        - `function-patch-and-transform:v0.4.0`
    - `configuration-app:v0.5.0`
        - `provider-helm:v0.17.0`
        - `function-patch-and-transform:v0.4.0`
    - `configuration-observability-oss:v0.5.0`
        - `provider-helm:v0.17.0`
        - `provider-kubernetes:v0.12.1`
        - `provider-grafana:v0.8.0`
        - `function-patch-and-transform:v0.4.0`
    - `configuration-gitops-flux:v0.6.0`
        - `provider-helm:v0.17.0`
        - `function-patch-and-transform:v0.4.0`

    `This configuration has 14 distinct dependencies. We need to download it's own yaml plus 14 other yamls`

    Current behavior:
    ```
    crossplane beta validate .work/xfn/config-plat-ref-azure.yaml .work/xfn/
    
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-gitops-flux:v0.6.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/platform-ref-azure:v0.11.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-azure-containerservice:v0.42.1
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-azure-network:v0.8.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-azure-aks:v0.7.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-azure-database:v0.11.1
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-app:v0.5.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-observability-oss:v0.5.0
    ```
        
    New behavior:
    ```
    go run cmd/crank/main.go beta validate .work/xfn/config-plat-ref-azure.yaml .work/xfn/
    
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/platform-ref-azure:v0.11.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-azure-network:v0.8.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-gitops-flux:v0.6.0
    package schemas does not exist, downloading:  xpkg.upbound.io/crossplane-contrib/provider-kubernetes:v0.12.1
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-azure-database:v0.11.1
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-app:v0.5.0
    package schemas does not exist, downloading:  xpkg.upbound.io/crossplane-contrib/function-patch-and-transform:v0.4.0
    package schemas does not exist, downloading:  xpkg.upbound.io/crossplane-contrib/provider-helm:v0.17.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-azure-aks:v0.7.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-azure-network:v0.42.1
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-azure-dbformariadb:v0.42.1
    package schemas does not exist, downloading:  xpkg.upbound.io/grafana/provider-grafana:v0.8.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-azure-containerservice:v0.42.1
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/configuration-observability-oss:v0.5.0
    package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-azure-dbforpostgresql:v0.42.1
    ```


I have: <!--You MUST either [x] check or [ ] ~strike through~ every item.-->

- [x] Read and followed Crossplane's [contribution process].
- [x] Run `earthly +reviewable` to ensure this PR is ready for review.
- [x] Added or updated unit tests.
- [ ] ~Added or updated e2e tests.~
- [ ] ~Linked a PR or a [docs tracking issue] to [document this change].~
- [ ] ~Added `backport release-x.y` labels to auto-backport this PR.~

Need help with this checklist? See the [cheat sheet].

[contribution process]: https://github.com/crossplane/crossplane/tree/master/contributing
[docs tracking issue]: https://github.com/crossplane/docs/issues/new
[document this change]: https://docs.crossplane.io/contribute/contribute
[cheat sheet]: https://github.com/crossplane/crossplane/tree/master/contributing#checklist-cheat-sheet


enesonus: > Nice @enesonus!! Are you able to add to the PR description the steps you took to test this change outside of unit tests? i'd love to test it out on my machine too :)

I added the examples now @jbw976 thanks for the reminder!
jbw976: > I added the examples now @jbw976 thanks for the suggestion!

Awesome, that is definitely helpful @enesonus! If I could humbly provide a bit more opinion on how you've included these testing scenarios in the PR... 🙇‍♂️ 😇 

The commands you have look similar to:
```
go run cmd/crank/main.go beta validate .work/xfn/config-aws-db.yaml .work/xfn/
```

I'm not sure everyone 100% agrees with me 😅, but I am very appreciative when I come to review a PR and I don't have to think or put much effort into recreating the testing scenarios the author is describing. I'm a bit slow 🐢, so it's ideal at least for me when the testing scenarios can be completely copy/pasted without any further work or thinking required at least to run them. Certainly there is thinking needed to evaluate and verify them, but getting the effort to _run_ them down to 0 is really great.

All that to say - do you have examples of what is in your `.work/xfn` directory too? does it have other YAML files? I'd love to know exactly what you are testing so I can reproduce it exactly. If there are multiple files involved in a testing/repro scenario, it can be helpful to include them in a gist on https://gist.github.com/ so they don't clutter up the PR description too much.

Does that make sense? Other maintainers may not be as lazy, yet still thorough, as me 😂 

enesonus: > All that to say - do you have examples of what is in your `.work/xfn` directory too? does it have other YAML files? I'd love to know exactly what you are testing so I can reproduce it exactly. If there are multiple files involved in a testing/repro scenario, it can be helpful to include them in a gist on https://gist.github.com/ so they don't clutter up the PR description too much.

These are very valuable feedbacks on my side @jbw976! I wasn't very aware of how powerful was the github gists. I tried to make PR description as brief as possible so I did not add details. I will try to create a gist for this PR on how I was testing it.
enesonus: I changed the dependency addition function `(m *Manager) addDependencies()` to the new recursive `(m *Manager) addDependencies(confs map[string]bool)` so that we can go deeper at configurations. At this PR I declared `m.confs` variable type as `map[string]bool` but we have a possible type change at PR https://github.com/crossplane/crossplane/pull/5815 . We will probably need to merge https://github.com/crossplane/crossplane/pull/5815 before this PR and update the `m.confs` type in this one before merging. 

I created a script that runs the `beta validate` N times on a configuration yaml to check if it outputs the same number of lines at dependency download phase. [Here is the gist](https://gist.github.com/enesonus/481dd30a911f6e27c1d03c52d0fad1aa) on how to test this code with that script. The gist you provided was really helpful, thanks for that one @jbw976!
jbw976: Very cool @enesonus, I was able to use your gist and testing script to get reliable download results as well. Looks like this approach is working well!

Quick question about the test script: https://gist.github.com/enesonus/481dd30a911f6e27c1d03c52d0fad1aa#file-test_add_dep-sh-L24.  What does that kubectl context check for? I don't see `kubectl` used anywhere else, so it didn't seem needed to me. I actually removed the check from the script when I was running it 😇 
enesonus: > Quick question about the test script: https://gist.github.com/enesonus/481dd30a911f6e27c1d03c52d0fad1aa#file-test_add_dep-sh-L24. What does that kubectl context check for? I don't see `kubectl` used anywhere else, so it didn't seem needed to me. I actually removed the check from the script when I was running it 😇

TBH I dont really know what it actually affects 😅 At my initial run of script I got:
```
[!] could not find CRD/XRD for: /apiextensions.crossplane.io, Kind=Composition
```
I tried some different things and somehow it got fixed. I didn't understand what fixed it so did not want to remove it and possibly break it again 😅 I tried running it on a different computer, it was running so I didnt spend much time thinking on the script's code :)


Patch:
diff --git a/cmd/crank/beta/validate/manager.go b/cmd/crank/beta/validate/manager.go
index 56898180e..5dae3ce7a 100644
--- a/cmd/crank/beta/validate/manager.go
+++ b/cmd/crank/beta/validate/manager.go
@@ -165,7 +165,7 @@ func (m *Manager) CacheAndLoad(cleanCache bool) error {
 		return errors.Wrapf(err, "cannot initialize cache directory")
 	}
 
-	if err := m.addDependencies(); err != nil {
+	if err := m.addDependencies(m.confs); err != nil {
 		return errors.Wrapf(err, "cannot add package dependencies")
 	}
 
@@ -181,8 +181,13 @@ func (m *Manager) CacheAndLoad(cleanCache bool) error {
 	return m.PrepExtensions(schemas)
 }
 
-func (m *Manager) addDependencies() error {
-	for image := range m.confs {
+func (m *Manager) addDependencies(confs map[string]*metav1.Configuration) error {
+	if len(confs) == 0 {
+		return nil
+	}
+
+	deepConfs := make(map[string]*metav1.Configuration)
+	for image := range confs {
 		cfg := m.confs[image]
 
 		if cfg == nil {
@@ -216,11 +221,16 @@ func (m *Manager) addDependencies() error {
 			if len(image) > 0 {
 				image = fmt.Sprintf(imageFmt, image, dep.Version)
 				m.deps[image] = true
+
+				if _, ok := m.confs[image]; !ok && dep.Configuration != nil {
+					deepConfs[image] = nil
+					m.confs[image] = nil
+				}
 			}
 		}
 	}
 
-	return nil
+	return m.addDependencies(deepConfs)
 }
 
 func (m *Manager) cacheDependencies() error {


Test patch:
diff --git a/cmd/crank/beta/validate/manager_test.go b/cmd/crank/beta/validate/manager_test.go
index 1168547c8..839f065dc 100644
--- a/cmd/crank/beta/validate/manager_test.go
+++ b/cmd/crank/beta/validate/manager_test.go
@@ -32,7 +32,7 @@ import (
 )
 
 var (
-	// config-pkg:v1.3.0
+	// config-pkg:v1.3.0.
 	configPkg = []byte(`apiVersion: meta.pkg.crossplane.io/v1alpha1
 kind: Configuration
 metadata:
@@ -45,7 +45,7 @@ spec:
 
 `)
 
-	// provider-dep-1:v1.3.0
+	// provider-dep-1:v1.3.0.
 	providerYaml = []byte(`apiVersion: meta.pkg.crossplane.io/v1
 kind: Provider
 metadata:
@@ -54,13 +54,41 @@ metadata:
 
 `)
 
-	// function-dep-1:v1.3.0
+	// function-dep-1:v1.3.0.
 	funcYaml = []byte(`apiVersion: meta.pkg.crossplane.io/v1beta1
 kind: Function
 metadata:
   name: function-dep-1
 ---
 
+`)
+
+	// config-dep-1:v1.3.0.
+	configDep1Yaml = []byte(`apiVersion: meta.pkg.crossplane.io/v1alpha1
+kind: Configuration
+metadata:
+  name: config-dep-1
+spec:
+  dependsOn:
+    - configuration: config-dep-2
+      version: "v1.3.0"
+---
+
+`)
+
+	// config-dep-2:v1.3.0.
+	configDep2Yaml = []byte(`apiVersion: meta.pkg.crossplane.io/v1alpha1
+kind: Configuration
+metadata:
+  name: config-dep-2
+spec:
+  dependsOn:
+    - provider: provider-dep-1
+      version: "v1.3.0"
+    - function: function-dep-1
+      version: "v1.3.0"
+---
+
 `)
 )
 
@@ -97,8 +125,8 @@ func TestConfigurationTypeSupport(t *testing.T) {
 		want   want
 	}{
 		"SuccessfulConfigPkg": {
-			//config-pkg
-			//└─►provider-dep-1
+			// config-pkg
+			// └─►provider-dep-1
 			reason: "All dependencies should be successfully added from Configuration.pkg",
 			args: args{
 				extensions: []*unstructured.Unstructured{
@@ -124,8 +152,8 @@ func TestConfigurationTypeSupport(t *testing.T) {
 			},
 		},
 		"SuccessfulConfigMeta": {
-			//config-meta
-			//└─►function-dep-1
+			// config-meta
+			// └─►function-dep-1
 			reason: "All dependencies should be successfully added from Configuration.meta",
 			args: args{
 				extensions: []*unstructured.Unstructured{
@@ -156,8 +184,8 @@ func TestConfigurationTypeSupport(t *testing.T) {
 			},
 		},
 		"SuccessfulConfigMetaAndPkg": {
-			//config-meta
-			//└─►function-dep-1
+			// config-meta
+			// └─►function-dep-1
 			//config-pkg
 			//└─►provider-dep-1
 			reason: "All dependencies should be successfully added from both Configuration.meta and Configuration.pkg",
@@ -206,7 +234,7 @@ func TestConfigurationTypeSupport(t *testing.T) {
 		fs := afero.NewMemMapFs()
 		w := &bytes.Buffer{}
 
-		m := NewManager(".crossplane/cache", fs, w)
+		m := NewManager("", fs, w)
 		t.Run(name, func(t *testing.T) {
 			m.fetcher = &MockFetcher{tc.args.fetchMock}
 			err := m.PrepExtensions(tc.args.extensions)
@@ -215,7 +243,7 @@ func TestConfigurationTypeSupport(t *testing.T) {
 				t.Errorf("\n%s\nPrepExtensions(...): -want error, +got error:\n%s", tc.reason, diff)
 			}
 
-			err = m.addDependencies()
+			err = m.addDependencies(m.confs)
 			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
 				t.Errorf("\n%s\naddDependencies(...): -want error, +got error:\n%s", tc.reason, diff)
 			}
@@ -230,6 +258,95 @@ func TestConfigurationTypeSupport(t *testing.T) {
 	}
 }
 
+func TestAddDependencies(t *testing.T) {
+	cd1 := static.NewLayer(configDep1Yaml, types.OCILayer)
+	cd2 := static.NewLayer(configDep2Yaml, types.OCILayer)
+	pd1 := static.NewLayer(providerYaml, types.OCILayer)
+	fd1 := static.NewLayer(funcYaml, types.OCILayer)
+
+	fetchMockFunc := func(image string) (*conregv1.Layer, error) {
+		switch image {
+		case "config-dep-1:v1.3.0":
+			return &cd1, nil
+		case "config-dep-2:v1.3.0":
+			return &cd2, nil
+		case "provider-dep-1:v1.3.0":
+			return &pd1, nil
+		case "function-dep-1:v1.3.0":
+			return &fd1, nil
+		default:
+			return nil, fmt.Errorf("unknown image: %s", image)
+		}
+	}
+
+	type args struct {
+		extensions []*unstructured.Unstructured
+		fetchMock  func(image string) (*conregv1.Layer, error)
+	}
+	type want struct {
+		confs int
+		deps  int
+		err   error
+	}
+	cases := map[string]struct {
+		reason string
+		args   args
+		want   want
+	}{
+		"SuccessfulDependenciesAddition": {
+			// config-dep-1
+			// └─►config-dep-2
+			//   ├─►provider-dep-1
+			//   └─►function-dep-1
+			reason: "All dependencies should be successfully fetched and added",
+			args: args{
+				fetchMock: fetchMockFunc,
+				extensions: []*unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"apiVersion": "pkg.crossplane.io/v1alpha1",
+							"kind":       "Configuration",
+							"metadata": map[string]interface{}{
+								"name": "config-dep-1",
+							},
+							"spec": map[string]interface{}{
+								"package": "config-dep-1:v1.3.0",
+							},
+						},
+					},
+				},
+			},
+			want: want{
+				confs: 2, // 1 Base configuration (config-dep-1), 1 child configuration (config-dep-2)
+				deps:  4, // 2 configurations (config-dep-1, config-dep-2), 1 provider (provider-dep-1), 1 function (function-dep-1)
+				err:   nil,
+			},
+		},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			fs := afero.NewMemMapFs()
+			w := &bytes.Buffer{}
+
+			m := NewManager("", fs, w)
+			_ = m.PrepExtensions(tc.args.extensions)
+
+			m.fetcher = &MockFetcher{tc.args.fetchMock}
+			err := m.addDependencies(m.confs)
+
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\naddDependencies(...): -want error, +got error:\n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(tc.want.confs, len(m.confs)); diff != "" {
+				t.Errorf("\n%s\naddDependencies(...): -want confs, +got confs:\n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(tc.want.deps, len(m.deps)); diff != "" {
+				t.Errorf("\n%s\naddDependencies(...): -want deps, +got deps:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}
+
 type MockFetcher struct {
 	fetch func(image string) (*conregv1.Layer, error)
 }