Issue 68 helm charts difficult to consume take2 (#490)

## Description

Fix for issue [
#68](open-component-model/ocm-project#68) in
ocm-project. At least as of 0.23.7 there was primary problem and a
follow on problem that appeared after the first was fixed.

My setup was like this 

![image](https://github.com/user-attachments/assets/c6015cee-5c88-4644-915e-852e0b94bc15)

With the setup and v0.23.7 the first problem was I would get an error
like this
`ocm-system dmi-broker 24m False artifact revision 19090 does not match
chart version 1.3.0-1085.xeae3fba.14`

This because snapshot writer is using the resourceversion of of the
kubernetes object that owns the snapshot. Given my setup, 19090 happened
to be the resourceversion of my Configuration.

As I mentioned when I opened
[#68](open-component-model/ocm-project#68), at
least for helmcharts this is silly as there are several spots that are
better sources for a version tag. So I have fixed this storying the
resource version, taken from the component descriptor, in the identity.
I am only doing this if there isn't already a value stored under
ResourceVersionKey.

Fixing this revealed a problem when you have more than 1 mutation, in my
case a Localization and a Configuration, getting their config a
resource. The mutation objects get their identity from the resource they
reference in `configRef`. Both of my reference the same config.yaml in
my components. The identity is used to compute the snapshot's OCI
repository name.

Prior to my above tag fix being in place, the Localization and
Configuration were both sticking their snapshots into the same
repository. Since I was 'lucky' and the two objects never had the same
resourceversion the tags were used on the images stored in the
repository were different and I never noticed a problem. However once I
put the above fix in place I started seeing sporadic failures. In fact,
due to issue #224, I would see my HelmRelease go back and forth between
being unhealthy and healthy ( but mostly unhealthy ). It would be
unhealthy if/when the FluxDeployer grabbed the data from the snapshot
image when the Localization was the last one to write the snapshot
image.

To fix this added I 'mutation-object-uuid' to the identity of resources
that are output from the mutations ( Localization, Configuration ). This
way each will have its own snapshot repo and they will no longer
overwrite each other's images when the same resource is referenced in
`configRef`.

## What type of PR is this? (check all applicable)

- [ ] 🍕 Feature
- [x ] 🐛 Bug Fix
- [ ] 📝 Documentation Update
- [ ] 🎨 Style
- [ ] 🧑‍💻 Code Refactor
- [ ] 🔥 Performance Improvements
- [ ] ✅ Test
- [ ] 🤖 Build
- [ ] 🔁 CI
- [ ] 📦 Chore (Release)
- [ ] ⏩ Revert

## Related Tickets & Documents

<!-- 
Please use this format link issue numbers: Fixes #123

https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
-->

- Fixes
[68](open-component-model/ocm-project#68)


## Added tests?

- [ ] 👍 yes
- [ ] 🙅 no, because they aren't needed
- [ ] 🙋 no, because I need help
- [ ] Separate ticket for tests # (issue/pr)

As of 2024-07-11 I have just used the fixed build in my POC and
confirmed that my components are being deployed. If/when I get time I'll
try to update the test suite.


## Added to documentation?

- [ ] 📜 README.md
- [ ] 🙅 no documentation needed

## Checklist:

- [x ] My code follows the style guidelines of this project
- [ x] I have performed a self-review of my code
- [ x] I have commented my code, particularly in hard-to-understand
areas
- [ x] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [ x] Any dependent changes have been merged and published in
downstream modules

---------

Co-authored-by: Gergely Brautigam <[email protected]>

Dockerfile

@@ -1,5 +1,5 @@
 # Build the manager binary
-FROM golang:1.22 as builder
+FROM docker.io/library/golang:1.22.4 as builder
 
 WORKDIR /workspace
 # Copy the Go Modules manifests

api/v1alpha1/constants.go

@@ -15,6 +15,7 @@ const (
 	SourceNameKey             = "source-name"
 	SourceNamespaceKey        = "source-namespace"
 	SourceArtifactChecksumKey = "source-artifact-checksum"
+	MutationObjectUUIDKey     = "mutation-object-uuid"
 )
 
 // Externally defined extra identity keys.

controllers/configuration_controller_test.go

@@ -33,11 +33,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/yaml"
 
+	ocmmetav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1"
+
 	"github.com/open-component-model/ocm-controller/api/v1alpha1"
 	cachefakes "github.com/open-component-model/ocm-controller/pkg/cache/fakes"
 	"github.com/open-component-model/ocm-controller/pkg/ocm/fakes"
 	ocmsnapshot "github.com/open-component-model/ocm-controller/pkg/snapshot"
-	ocmmetav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1"
 )
 
 var configurationConfigData = []byte(`kind: ConfigData
@@ -825,8 +826,8 @@ configuration:
 
 			t.Log("extracting the passed in data and checking if the configuration worked")
 			args := cache.PushDataCallingArgumentsOnCall(0)
-			assert.Equal(t, "sha-18322151501422808564", args.Name)
-			assert.Equal(t, "999", args.Version)
+			assert.Equal(t, "sha-5540475038233850640", args.Name)
+			assert.Equal(t, "1.0.0", args.Version)
 			sourceFile := extractFileFromTarGz(t, io.NopCloser(bytes.NewBuffer([]byte(args.Content))), "configmap.yaml")
 			configMap := corev1.ConfigMap{}
 			assert.NoError(t, yaml.Unmarshal(sourceFile, &configMap))
@@ -1024,8 +1025,8 @@ func TestConfigurationValuesFrom(t *testing.T) {
 
 			t.Log("extracting the passed in data and checking if the configuration worked")
 			args := cache.PushDataCallingArgumentsOnCall(0)
-			assert.Equal(t, "sha-6142814068768493943", args.Name)
-			assert.Equal(t, "999", args.Version)
+			assert.Equal(t, "sha-13092443426051895747", args.Name)
+			assert.Equal(t, "1.0.0", args.Version)
 			sourceFile := extractFileFromTarGz(t, io.NopCloser(bytes.NewBuffer([]byte(args.Content))), "configmap.yaml")
 			configMap := corev1.ConfigMap{}
 			assert.NoError(t, yaml.Unmarshal(sourceFile, &configMap))

controllers/localization_controller_test.go

@@ -28,12 +28,13 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	ocmmetav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1"
+
 	"github.com/open-component-model/ocm-controller/api/v1alpha1"
 	cachefakes "github.com/open-component-model/ocm-controller/pkg/cache/fakes"
 	ocmfake "github.com/open-component-model/ocm-controller/pkg/fakes"
 	"github.com/open-component-model/ocm-controller/pkg/ocm/fakes"
 	ocmsnapshot "github.com/open-component-model/ocm-controller/pkg/snapshot"
-	ocmmetav1 "github.com/open-component-model/ocm/pkg/contexts/ocm/compdesc/meta/v1"
 )
 
 var localizationConfigData = []byte(`kind: ConfigData
@@ -762,8 +763,8 @@ localization:
 			}, snapshotOutput)
 			require.NoError(t, err)
 			args := cache.PushDataCallingArgumentsOnCall(0)
-			assert.Equal(t, "sha-18322151501422808564", args.Name)
-			assert.Equal(t, "999", args.Version)
+			assert.Equal(t, "sha-5540475038233850640", args.Name)
+			assert.Equal(t, "1.0.0", args.Version)
 
 			t.Log("extracting the passed in data and checking if the localization worked")
 			require.NoError(t, err)

controllers/mutation_reconcile_looper.go

@@ -28,8 +28,6 @@ import (
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/mandelsoft/spiff/spiffing"
 	"github.com/mandelsoft/vfs/pkg/osfs"
-	"github.com/open-component-model/ocm-controller/pkg/snapshot"
-	"github.com/open-component-model/ocm-controller/pkg/untar"
 	ocmcore "github.com/open-component-model/ocm/pkg/contexts/ocm"
 	"github.com/open-component-model/ocm/pkg/utils/tarutils"
 	"gopkg.in/yaml.v3"
@@ -43,11 +41,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
-	"github.com/open-component-model/ocm-controller/api/v1alpha1"
-	"github.com/open-component-model/ocm-controller/pkg/cache"
-	"github.com/open-component-model/ocm-controller/pkg/component"
-	"github.com/open-component-model/ocm-controller/pkg/configdata"
-	"github.com/open-component-model/ocm-controller/pkg/ocm"
+	"github.com/open-component-model/ocm-controller/pkg/snapshot"
+	"github.com/open-component-model/ocm-controller/pkg/untar"
+
 	"github.com/open-component-model/ocm/pkg/contexts/ocm/accessmethods/localblob"
 	"github.com/open-component-model/ocm/pkg/contexts/ocm/accessmethods/ociartifact"
 	"github.com/open-component-model/ocm/pkg/contexts/ocm/accessmethods/ociblob"
@@ -57,6 +53,12 @@ import (
 	ocmruntime "github.com/open-component-model/ocm/pkg/runtime"
 	"github.com/open-component-model/ocm/pkg/spiff"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+
+	"github.com/open-component-model/ocm-controller/api/v1alpha1"
+	"github.com/open-component-model/ocm-controller/pkg/cache"
+	"github.com/open-component-model/ocm-controller/pkg/component"
+	"github.com/open-component-model/ocm-controller/pkg/configdata"
+	"github.com/open-component-model/ocm-controller/pkg/ocm"
 )
 
 // errTar defines an error that occurs when the resource is not a tar archive.
@@ -749,13 +751,9 @@ func (m *MutationReconcileLooper) getIdentity(ctx context.Context, obj *v1alpha1
 			v1alpha1.ComponentNameKey:    cv.Status.ComponentDescriptor.ComponentDescriptorRef.Name,
 			v1alpha1.ComponentVersionKey: cv.Status.ComponentDescriptor.Version,
 		}
-		if obj.ResourceRef != nil {
-			id[v1alpha1.ResourceNameKey] = obj.ResourceRef.Name
-			id[v1alpha1.ResourceVersionKey] = obj.ResourceRef.Version
 
-			for k, v := range obj.ResourceRef.ExtraIdentity {
-				id[k] = v
-			}
+		if err := m.configureResourceVersion(ctx, obj, id, cv); err != nil {
+			return nil, err
 		}
 	default:
 		// if kind is not ComponentVersion, then fetch resource using dynamic client
@@ -785,6 +783,54 @@ func (m *MutationReconcileLooper) getIdentity(ctx context.Context, obj *v1alpha1
 	return id, err
 }
 
+// 2024-07-10 d :
+// Partial solution for #68
+// Helm chart in the private OCI repository must have a tag that matches the version
+// of the helm chart.
+// So we make sure that if not specified, the id includes the version of the resource
+// in the ComponentDescriptor.
+// Elsewhere, there is a change that will use this as the tag by default.
+// This way none of the Localization, Configuration or FluxDeployer objects need to
+// hard code a version.  i.e.  Neither the Component authors nor the ops folks managing
+// the k8s cluster have to maintain a version spec if they don't want to.
+func (m *MutationReconcileLooper) configureResourceVersion(
+	ctx context.Context,
+	obj *v1alpha1.ObjectReference,
+	id ocmmetav1.Identity,
+	cv *v1alpha1.ComponentVersion,
+) error {
+	if obj.ResourceRef == nil {
+		return nil
+	}
+
+	id[v1alpha1.ResourceNameKey] = obj.ResourceRef.Name
+	id[v1alpha1.ResourceVersionKey] = obj.ResourceRef.Version
+
+	if id[v1alpha1.ResourceVersionKey] == "" {
+		cd, err := component.GetComponentDescriptor(ctx, m.Client, nil, cv.Status.ComponentDescriptor)
+		if err != nil {
+			return err
+		}
+
+		for _, r := range cd.Spec.Resources {
+			if obj.ResourceRef.Name == r.Name {
+				id[v1alpha1.ResourceVersionKey] = r.Version
+
+				break
+			}
+		}
+	}
+
+	for k, v := range obj.ResourceRef.ExtraIdentity {
+		if k == v1alpha1.ResourceVersionKey {
+			continue
+		}
+		id[k] = v
+	}
+
+	return nil
+}
+
 func (m *MutationReconcileLooper) getComponentVersion(ctx context.Context, obj *v1alpha1.ObjectReference) (*v1alpha1.ComponentVersion, error) {
 	if obj.Kind != v1alpha1.ComponentVersionKind {
 		return nil, errors.New("cannot retrieve component version for snapshot")
@@ -960,6 +1006,18 @@ func (m *MutationReconcileLooper) mutateConfigRef(
 	}
 
 	snapshotID, err := m.getIdentity(ctx, spec.ConfigRef)
+
+	// 2024-07-10 d :
+	// Another part of fix for #68
+	// If you try to have
+	// componentversion--(helmchart)-->localization--(helmchart)->configuration-->fluxdeployer
+	// then without this change the controllers may overwrite each other's images.
+	// This because the repo is determined by the config resource's id, which may not be unique
+	// in this case, and the tag is either the mutation object's resource version or it is the
+	// version of your resource. ( e.g. your helm chart version )
+	// In order to avoid these overwrites we make sure the snapshotID incorporates the id of
+	// the mutation object.   This the repos for each mutation object will be distinct.
+	snapshotID[v1alpha1.MutationObjectUUIDKey] = string(obj.GetUID())
 	if err != nil {
 		return "", ocmmetav1.Identity{}, fmt.Errorf("failed to get identity for config ref: %w", err)
 	}

pkg/snapshot/snapshot_writer.go

@@ -84,6 +84,11 @@ func (w *OCIWriter) Write(
 	tag := owner.GetResourceVersion()
 	if v, ok := identity[v1alpha1.ResourceHelmChartVersion]; ok {
 		tag = v
+	} else if v, ok = identity[v1alpha1.ResourceVersionKey]; ok {
+		// 2024-07-10 d :
+		// Partial fix for #68.   If available, default to using the resource
+		// version as the tag.
+		tag = v
 	}
 
 	snapshotDigest, size, err := w.Cache.PushData(ctx, file, "", name, tag)