From aecc74f4d36a1e2d5e75596737fa41d7472faed9 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Tue, 10 Dec 2024 18:03:09 +0100 Subject: [PATCH] feat(directives): optional `images` for `kustomize-set-image` (#3115) Signed-off-by: Hidde Beydals --- docs/docs/35-references/10-promotion-steps.md | 2 +- internal/controller/freight/finder.go | 44 +++++ internal/controller/freight/finder_test.go | 129 +++++++++++++ internal/directives/kustomize_image_setter.go | 61 +++++- .../directives/kustomize_image_setter_test.go | 182 ++++++++++++++++-- .../schemas/kustomize-set-image-config.json | 3 +- internal/directives/zz_config_types.go | 5 +- .../kustomize-set-image-config.json | 2 +- 8 files changed, 402 insertions(+), 26 deletions(-) diff --git a/docs/docs/35-references/10-promotion-steps.md b/docs/docs/35-references/10-promotion-steps.md index e0ea5f0ba..f168c38cc 100644 --- a/docs/docs/35-references/10-promotion-steps.md +++ b/docs/docs/35-references/10-promotion-steps.md @@ -312,7 +312,7 @@ to executing `kustomize edit set image`. This step is commonly followed by a | Name | Type | Required | Description | |------|------|----------|-------------| | `path` | `string` | Y | Path to a directory containing a `kustomization.yaml` file. This path is relative to the temporary workspace that Kargo provisions for use by the promotion process. | -| `images` | `[]object` | Y | The details of changes to be applied to the `kustomization.yaml` file. At least one must be specified. | +| `images` | `[]object` | N | The details of changes to be applied to the `kustomization.yaml` file. When left unspecified, all images from the Freight collection will be set in the Kustomization file. Unless there is an ambiguous image name (for example, due to two Warehouses subscribing to the same repository), which requires manual configuration. | | `images[].image` | `string` | Y | Name/URL of the image being updated. | | `images[].tag` | `string` | N | A tag naming a specific revision of `image`. Mutually exclusive with `digest` and `useDigest=true`. If none of these are specified, the tag specified by a piece of Freight referencing `image` will be used as the value of this field. | | `images[].digest` | `string` | N | A digest naming a specific revision of `image`. Mutually exclusive with `tag` and `useDigest=true`. If none of these are specified, the tag specified by a piece of Freight referencing `image` will be used as the value of `tag`. | diff --git a/internal/controller/freight/finder.go b/internal/controller/freight/finder.go index b1db7ad2c..605e6dca7 100644 --- a/internal/controller/freight/finder.go +++ b/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, diff --git a/internal/controller/freight/finder_test.go b/internal/controller/freight/finder_test.go index 1c2644c9d..2df033e5d 100644 --- a/internal/controller/freight/finder_test.go +++ b/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" diff --git a/internal/directives/kustomize_image_setter.go b/internal/directives/kustomize_image_setter.go index e16515768..95b66da45 100644 --- a/internal/directives/kustomize_image_setter.go +++ b/internal/directives/kustomize_image_setter.go @@ -91,8 +91,15 @@ func (k *kustomizeImageSetter) runPromotionStep( fmt.Errorf("could not discover kustomization file: %w", err) } - // Discover image origins and collect target images. - targetImages, err := k.buildTargetImages(ctx, stepCtx, cfg.Images) + var targetImages map[string]kustypes.Image + switch { + case len(cfg.Images) > 0: + // Discover image origins and collect target images. + targetImages, err = k.buildTargetImagesFromConfig(ctx, stepCtx, cfg.Images) + default: + // Attempt to automatically set target images based on the Freight references. + targetImages, err = k.buildTargetImagesAutomatically(ctx, stepCtx) + } if err != nil { return PromotionStepResult{Status: kargoapi.PromotionPhaseErrored}, err } @@ -111,7 +118,7 @@ func (k *kustomizeImageSetter) runPromotionStep( return result, nil } -func (k *kustomizeImageSetter) buildTargetImages( +func (k *kustomizeImageSetter) buildTargetImagesFromConfig( ctx context.Context, stepCtx *PromotionStepContext, images []KustomizeSetImageConfigImage, @@ -159,6 +166,42 @@ func (k *kustomizeImageSetter) buildTargetImages( return targetImages, nil } +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 { + images[img.RepoURL] = kustypes.Image{ + Name: img.RepoURL, + NewTag: img.Tag, + Digest: img.Digest, + } + } + } + return images, nil +} + func (k *kustomizeImageSetter) generateCommitMessage(path string, images map[string]kustypes.Image) string { if len(images) == 0 { return "" @@ -171,7 +214,17 @@ func (k *kustomizeImageSetter) generateCommitMessage(path string, images map[str } _, _ = commitMsg.WriteString("\n") - for _, i := range images { + // Sort the images by name, in descending order for consistency. + imageNames := make([]string, 0, len(images)) + for name := range images { + imageNames = append(imageNames, name) + } + slices.Sort(imageNames) + + // Append each image to the commit message. + for _, name := range imageNames { + i := images[name] + ref := i.Name if i.NewName != "" { ref = i.NewName diff --git a/internal/directives/kustomize_image_setter_test.go b/internal/directives/kustomize_image_setter_test.go index 3f0b7fd56..f43b819bd 100644 --- a/internal/directives/kustomize_image_setter_test.go +++ b/internal/directives/kustomize_image_setter_test.go @@ -40,22 +40,6 @@ func Test_kustomizeImageSetter_validate(t *testing.T) { "path: String length must be greater than or equal to 1", }, }, - { - name: "images is null", - config: Config{}, - expectedProblems: []string{ - "(root): images is required", - }, - }, - { - name: "images is empty", - config: Config{ - "images": []Config{}, - }, - expectedProblems: []string{ - "images: Array must have at least 1 items", - }, - }, { name: "image not specified", config: Config{ @@ -259,6 +243,51 @@ kind: Kustomization assert.Contains(t, string(b), "newTag: 1.21.0") }, }, + { + name: "automatically sets image", + setupFiles: func(t *testing.T) string { + tempDir := t.TempDir() + kustomizationContent := `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +` + err := os.WriteFile(filepath.Join(tempDir, "kustomization.yaml"), []byte(kustomizationContent), 0o600) + require.NoError(t, err) + return tempDir + }, + cfg: KustomizeSetImageConfig{ + Path: ".", + Images: nil, // Automatically set all images + }, + setupStepCtx: func(_ *testing.T, workDir string) *PromotionStepContext { + return &PromotionStepContext{ + WorkDir: workDir, + Freight: kargoapi.FreightCollection{ + Freight: map[string]kargoapi.FreightReference{ + "Warehouse/warehouse1": { + Images: []kargoapi.Image{{RepoURL: "nginx", Digest: "sha256:123"}}, + }, + "Warehouse/warehouse2": { + Images: []kargoapi.Image{{RepoURL: "redis", Tag: "6.2.5"}}, + }, + }, + }, + } + }, + assertions: func(t *testing.T, workDir string, result PromotionStepResult, err error) { + require.NoError(t, err) + assert.Equal(t, PromotionStepResult{ + Status: kargoapi.PromotionPhaseSucceeded, + Output: map[string]any{ + "commitMessage": "Updated . to use new images\n\n- nginx@sha256:123\n- redis:6.2.5", + }, + }, result) + + b, err := os.ReadFile(filepath.Join(workDir, "kustomization.yaml")) + require.NoError(t, err) + assert.Contains(t, string(b), "newTag: 6.2.5") + assert.Contains(t, string(b), "digest: sha256:123") + }, + }, { name: "Kustomization file not found", setupFiles: func(t *testing.T) string { @@ -555,7 +584,126 @@ func Test_kustomizeImageSetter_buildTargetImages(t *testing.T) { }, } - result, err := runner.buildTargetImages(context.Background(), stepCtx, tt.images) + result, err := runner.buildTargetImagesFromConfig(context.Background(), stepCtx, tt.images) + tt.assertions(t, result, err) + }) + } +} + +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) + }{ + { + name: "successfully builds target images", + freightReferences: map[string]kargoapi.FreightReference{ + "Warehouse/warehouse1": { + Images: []kargoapi.Image{ + {RepoURL: "nginx", Tag: "1.21.0", Digest: "sha256:abcdef1234567890"}, + }, + }, + "Warehouse/warehouse2": { + Images: []kargoapi.Image{ + {RepoURL: "redis", Tag: "6.2.5"}, + }, + }, + "Warehouse/warehouse3": { + Images: []kargoapi.Image{ + {RepoURL: "postgres", Digest: "sha256:abcdef1234567890"}, + }, + }, + }, + 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{ + "nginx": {Name: "nginx", NewTag: "1.21.0", Digest: "sha256:abcdef1234567890"}, + "redis": {Name: "redis", NewTag: "6.2.5"}, + "postgres": {Name: "postgres", Digest: "sha256:abcdef1234567890"}, + }, result) + }, + }, + { + name: "error on ambiguous image match", + freightReferences: map[string]kargoapi.FreightReference{ + "Warehouse/warehouse1": { + Images: []kargoapi.Image{ + {RepoURL: "nginx", Tag: "1.21.0", Digest: "sha256:abcdef1234567890"}, + }, + }, + "Warehouse/warehouse2": { + Images: []kargoapi.Image{ + {RepoURL: "nginx", Tag: "1.21.0", Digest: "sha256:abcdef1234567890"}, + }, + }, + }, + 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") + }, + }, + } + + runner := &kustomizeImageSetter{} + + 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(context.Background(), stepCtx) tt.assertions(t, result, err) }) } diff --git a/internal/directives/schemas/kustomize-set-image-config.json b/internal/directives/schemas/kustomize-set-image-config.json index aa2a44ba3..ab1f28399 100644 --- a/internal/directives/schemas/kustomize-set-image-config.json +++ b/internal/directives/schemas/kustomize-set-image-config.json @@ -12,8 +12,7 @@ }, "images": { "type": "array", - "description": "Images is a list of container images to set or update in the Kustomization file.", - "minItems": 1, + "description": "Images is a list of container images to set or update in the Kustomization file. When left unspecified, all images from the Freight collection will be set in the Kustomization file. Unless there is an ambiguous image name (for example, due to two Warehouses subscribing to the same repository), which requires manual configuration.", "items": { "type": "object", "additionalProperties": false, diff --git a/internal/directives/zz_config_types.go b/internal/directives/zz_config_types.go index 2d8812f47..e09b38c60 100644 --- a/internal/directives/zz_config_types.go +++ b/internal/directives/zz_config_types.go @@ -378,7 +378,10 @@ type Helm struct { } type KustomizeSetImageConfig struct { - // Images is a list of container images to set or update in the Kustomization file. + // Images is a list of container images to set or update in the Kustomization file. When + // left unspecified, all images from the Freight collection will be set in the Kustomization + // file. Unless there is an ambiguous image name (for example, due to two Warehouses + // subscribing to the same repository), which requires manual configuration. Images []KustomizeSetImageConfigImage `json:"images"` // Path to the directory containing the Kustomization file. Path string `json:"path"` diff --git a/ui/src/gen/directives/kustomize-set-image-config.json b/ui/src/gen/directives/kustomize-set-image-config.json index 8146ef8d0..9a7dd7f6d 100644 --- a/ui/src/gen/directives/kustomize-set-image-config.json +++ b/ui/src/gen/directives/kustomize-set-image-config.json @@ -11,7 +11,7 @@ }, "images": { "type": "array", - "description": "Images is a list of container images to set or update in the Kustomization file.", + "description": "Images is a list of container images to set or update in the Kustomization file. When left unspecified, all images from the Freight collection will be set in the Kustomization file. Unless there is an ambiguous image name (for example, due to two Warehouses subscribing to the same repository), which requires manual configuration.", "items": { "type": "object", "additionalProperties": false,