fix(directives): check ambiguity based on requests

Signed-off-by: Hidde Beydals <[email protected]>

internal/controller/freight/finder.go

@@ -168,6 +168,50 @@ func FindImage(
 	}
 }
 
+func HasAmbiguousImageRequest(
+	ctx context.Context,
+	cl client.Client,
+	project string,
+	freightReqs []kargoapi.FreightRequest,
+) (bool, error) {
+	var subscribedRepositories = make(map[string]any)
+
+	for i := range freightReqs {
+		requestedFreight := freightReqs[i]
+		warehouse, err := kargoapi.GetWarehouse(
+			ctx,
+			cl,
+			types.NamespacedName{
+				Name:      requestedFreight.Origin.Name,
+				Namespace: project,
+			},
+		)
+		if err != nil {
+			return false, err
+		}
+		if warehouse == nil {
+			return false, fmt.Errorf(
+				"Warehouse %q not found in namespace %q",
+				requestedFreight.Origin.Name, project,
+			)
+		}
+
+		for _, sub := range warehouse.Spec.Subscriptions {
+			if sub.Image != nil {
+				if _, ok := subscribedRepositories[sub.Image.RepoURL]; ok {
+					return true, fmt.Errorf(
+						"multiple requested Freight could potentially provide a container image from repository %s",
+						sub.Image.RepoURL,
+					)
+				}
+				subscribedRepositories[sub.Image.RepoURL] = struct{}{}
+			}
+		}
+	}
+
+	return false, nil
+}
+
 func FindChart(
 	ctx context.Context,
 	cl client.Client,

internal/controller/freight/finder_test.go

@@ -480,6 +480,135 @@ func TestFindImage(t *testing.T) {
 	}
 }
 
+func TestHasAmbiguousImageRequest(t *testing.T) {
+	const testNamespace = "test-namespace"
+	const testRepoURL = "fake-repo-url"
+
+	testOrigin1 := kargoapi.FreightOrigin{
+		Kind: kargoapi.FreightOriginKindWarehouse,
+		Name: "test-warehouse",
+	}
+	testOrigin2 := kargoapi.FreightOrigin{
+		Kind: kargoapi.FreightOriginKindWarehouse,
+		Name: "some-other-warehouse",
+	}
+
+	scheme := runtime.NewScheme()
+	require.NoError(t, kargoapi.AddToScheme(scheme))
+
+	testCases := []struct {
+		name       string
+		client     func() client.Client
+		freight    []kargoapi.FreightRequest
+		assertions func(*testing.T, bool, error)
+	}{
+		{
+			name: "no ambiguous image request",
+			client: func() client.Client {
+				return fake.NewClientBuilder().WithScheme(scheme).WithObjects(
+					&kargoapi.Warehouse{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: testNamespace,
+							Name:      testOrigin1.Name,
+						},
+						Spec: kargoapi.WarehouseSpec{
+							Subscriptions: []kargoapi.RepoSubscription{{
+								Image: &kargoapi.ImageSubscription{
+									RepoURL: testRepoURL,
+								},
+							}},
+						},
+					},
+				).Build()
+			},
+			freight: []kargoapi.FreightRequest{
+				{
+					Origin: testOrigin1,
+				},
+			},
+			assertions: func(t *testing.T, ambiguous bool, err error) {
+				require.NoError(t, err)
+				require.False(t, ambiguous)
+			},
+		},
+		{
+			name: "ambiguous image request",
+			client: func() client.Client {
+				return fake.NewClientBuilder().WithScheme(scheme).WithObjects(
+					&kargoapi.Warehouse{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: testNamespace,
+							Name:      testOrigin1.Name,
+						},
+						Spec: kargoapi.WarehouseSpec{
+							Subscriptions: []kargoapi.RepoSubscription{{
+								Image: &kargoapi.ImageSubscription{
+									RepoURL: testRepoURL,
+								},
+							}},
+						},
+					},
+					&kargoapi.Warehouse{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: testNamespace,
+							Name:      testOrigin2.Name,
+						},
+						Spec: kargoapi.WarehouseSpec{
+							Subscriptions: []kargoapi.RepoSubscription{{
+								Image: &kargoapi.ImageSubscription{
+									RepoURL: testRepoURL,
+								},
+							}},
+						},
+					},
+				).Build()
+			},
+			freight: []kargoapi.FreightRequest{
+				{
+					Origin: testOrigin1,
+				},
+				{
+					Origin: testOrigin2,
+				},
+			},
+			assertions: func(t *testing.T, ambiguous bool, err error) {
+				require.True(t, ambiguous)
+				require.ErrorContains(t, err, "multiple requested Freight could potentially provide")
+			},
+		},
+		{
+			name: "origin not found",
+			client: func() client.Client {
+				return fake.NewClientBuilder().WithScheme(scheme).Build()
+			},
+			freight: []kargoapi.FreightRequest{
+				{
+					Origin: testOrigin1,
+				},
+			},
+			assertions: func(t *testing.T, _ bool, err error) {
+				require.ErrorContains(t, err, "not found in namespace")
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			var cl client.Client
+			if testCase.client != nil {
+				cl = testCase.client()
+			}
+			ok, err := HasAmbiguousImageRequest(
+				context.Background(),
+				cl,
+				testNamespace,
+				testCase.freight,
+			)
+			testCase.assertions(t, ok, err)
+		})
+	}
+}
+
 func TestFindChart(t *testing.T) {
 	const testNamespace = "test-namespace"
 	const testRepoURL = "fake-repo-url"

internal/directives/kustomize_image_setter.go

@@ -98,7 +98,7 @@ func (k *kustomizeImageSetter) runPromotionStep(
 		targetImages, err = k.buildTargetImagesFromConfig(ctx, stepCtx, cfg.Images)
 	default:
 		// Attempt to automatically set target images based on the Freight references.
-		targetImages, err = k.buildTargetImagesAutomatically(stepCtx)
+		targetImages, err = k.buildTargetImagesAutomatically(ctx, stepCtx)
 	}
 	if err != nil {
 		return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, err
@@ -167,22 +167,31 @@ func (k *kustomizeImageSetter) buildTargetImagesFromConfig(
 }
 
 func (k *kustomizeImageSetter) buildTargetImagesAutomatically(
+	ctx context.Context,
 	stepCtx *PromotionStepContext,
 ) (map[string]kustypes.Image, error) {
+	// Check if there are any ambiguous image requests.
+	//
+	// We do this based on the request because the Freight references may not
+	// contain all the images that are requested, which could lead eventually
+	// to an ambiguous result.
+	if ambiguous, ambErr := freight.HasAmbiguousImageRequest(
+		ctx, stepCtx.KargoClient, stepCtx.Project, stepCtx.FreightRequests,
+	); ambErr != nil || ambiguous {
+		err := errors.New("manual configuration required due to ambiguous result")
+		if ambErr != nil {
+			err = fmt.Errorf("%w: %v", err, ambErr)
+		}
+		return nil, err
+	}
+
 	var images = make(map[string]kustypes.Image)
 	for _, freightRef := range stepCtx.Freight.References() {
 		if len(freightRef.Images) == 0 {
 			continue
 		}
 
 		for _, img := range freightRef.Images {
-			if _, ok := images[img.RepoURL]; ok {
-				return nil, fmt.Errorf(
-					"manual configuration required due to ambiguous result: found multiple images for repository %q",
-					img.RepoURL,
-				)
-			}
-
 			images[img.RepoURL] = kustypes.Image{
 				Name:   img.RepoURL,
 				NewTag: img.Tag,

internal/directives/kustomize_image_setter_test.go

@@ -591,9 +591,13 @@ func Test_kustomizeImageSetter_buildTargetImages(t *testing.T) {
 }
 
 func Test_kustomizeImageSetter_buildTargetImagesAutomatically(t *testing.T) {
+	const testNamespace = "test-project"
+
 	tests := []struct {
 		name              string
 		freightReferences map[string]kargoapi.FreightReference
+		freightRequests   []kargoapi.FreightRequest
+		objects           []runtime.Object
 		assertions        func(*testing.T, map[string]kustypes.Image, error)
 	}{
 		{
@@ -615,6 +619,28 @@ func Test_kustomizeImageSetter_buildTargetImagesAutomatically(t *testing.T) {
 					},
 				},
 			},
+			freightRequests: []kargoapi.FreightRequest{
+				{Origin: kargoapi.FreightOrigin{Name: "warehouse1", Kind: kargoapi.FreightOriginKindWarehouse}},
+				{Origin: kargoapi.FreightOrigin{Name: "warehouse2", Kind: kargoapi.FreightOriginKindWarehouse}},
+				{Origin: kargoapi.FreightOrigin{Name: "warehouse3", Kind: kargoapi.FreightOriginKindWarehouse}},
+			},
+			objects: []runtime.Object{
+				mockWarehouse(testNamespace, "warehouse1", kargoapi.WarehouseSpec{
+					Subscriptions: []kargoapi.RepoSubscription{
+						{Image: &kargoapi.ImageSubscription{RepoURL: "nginx"}},
+					},
+				}),
+				mockWarehouse(testNamespace, "warehouse2", kargoapi.WarehouseSpec{
+					Subscriptions: []kargoapi.RepoSubscription{
+						{Image: &kargoapi.ImageSubscription{RepoURL: "redis"}},
+					},
+				}),
+				mockWarehouse(testNamespace, "warehouse3", kargoapi.WarehouseSpec{
+					Subscriptions: []kargoapi.RepoSubscription{
+						{Image: &kargoapi.ImageSubscription{RepoURL: "postgres"}},
+					},
+				}),
+			},
 			assertions: func(t *testing.T, result map[string]kustypes.Image, err error) {
 				require.NoError(t, err)
 				assert.Equal(t, map[string]kustypes.Image{
@@ -638,6 +664,22 @@ func Test_kustomizeImageSetter_buildTargetImagesAutomatically(t *testing.T) {
 					},
 				},
 			},
+			freightRequests: []kargoapi.FreightRequest{
+				{Origin: kargoapi.FreightOrigin{Name: "warehouse1", Kind: kargoapi.FreightOriginKindWarehouse}},
+				{Origin: kargoapi.FreightOrigin{Name: "warehouse2", Kind: kargoapi.FreightOriginKindWarehouse}},
+			},
+			objects: []runtime.Object{
+				mockWarehouse(testNamespace, "warehouse1", kargoapi.WarehouseSpec{
+					Subscriptions: []kargoapi.RepoSubscription{
+						{Image: &kargoapi.ImageSubscription{RepoURL: "nginx"}},
+					},
+				}),
+				mockWarehouse(testNamespace, "warehouse2", kargoapi.WarehouseSpec{
+					Subscriptions: []kargoapi.RepoSubscription{
+						{Image: &kargoapi.ImageSubscription{RepoURL: "nginx"}},
+					},
+				}),
+			},
 			assertions: func(t *testing.T, _ map[string]kustypes.Image, err error) {
 				require.ErrorContains(t, err, "manual configuration required due to ambiguous result")
 			},
@@ -648,13 +690,20 @@ func Test_kustomizeImageSetter_buildTargetImagesAutomatically(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			scheme := runtime.NewScheme()
+			require.NoError(t, kargoapi.AddToScheme(scheme))
+			fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(tt.objects...).Build()
+
 			stepCtx := &PromotionStepContext{
+				KargoClient:     fakeClient,
+				Project:         testNamespace,
+				FreightRequests: tt.freightRequests,
 				Freight: kargoapi.FreightCollection{
 					Freight: tt.freightReferences,
 				},
 			}
 
-			result, err := runner.buildTargetImagesAutomatically(stepCtx)
+			result, err := runner.buildTargetImagesAutomatically(context.Background(), stepCtx)
 			tt.assertions(t, result, err)
 		})
 	}