From c927f0e7c181b03f990251baf67112320894104c Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Mon, 25 Nov 2024 16:29:00 +0100 Subject: [PATCH 1/9] Add GetResourceImages Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/sync/common/types.go | 2 ++ pkg/sync/sync_context.go | 1 + pkg/utils/kube/kube.go | 43 ++++++++++++++++++++++++++ pkg/utils/kube/kube_test.go | 61 +++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+) diff --git a/pkg/sync/common/types.go b/pkg/sync/common/types.go index cb0a7ef66..c8c1e607b 100644 --- a/pkg/sync/common/types.go +++ b/pkg/sync/common/types.go @@ -136,6 +136,8 @@ func NewHookDeletePolicy(p string) (HookDeletePolicy, bool) { type ResourceSyncResult struct { // holds associated resource key ResourceKey kube.ResourceKey + // holds the images associated with the resource + Images []string // holds resource version Version string // holds the execution order diff --git a/pkg/sync/sync_context.go b/pkg/sync/sync_context.go index 7d43899c9..0e44ecbb6 100644 --- a/pkg/sync/sync_context.go +++ b/pkg/sync/sync_context.go @@ -1376,6 +1376,7 @@ func (sc *syncContext) setResourceResult(task *syncTask, syncStatus common.Resul res := common.ResourceSyncResult{ ResourceKey: kubeutil.GetResourceKey(task.obj()), + Images: kubeutil.GetResourceImages(task.obj()), Version: task.version(), Status: task.syncStatus, Message: task.message, diff --git a/pkg/utils/kube/kube.go b/pkg/utils/kube/kube.go index e08b414c1..925ec6e12 100644 --- a/pkg/utils/kube/kube.go +++ b/pkg/utils/kube/kube.go @@ -407,6 +407,49 @@ func GetDeploymentReplicas(u *unstructured.Unstructured) *int64 { return &val } +func GetResourceImages(u *unstructured.Unstructured) []string { + var containers []any + var found bool + var err error + var images []string + + containerPaths := [][]string{ + // Resources without template, like pods + {"spec", "containers"}, + // Resources with template, like deployments + {"spec", "template", "spec", "containers"}, + // Cronjobs + {"spec", "jobTemplate", "spec", "template", "spec", "containers"}, + } + + for _, path := range containerPaths { + containers, found, err = unstructured.NestedSlice(u.Object, path...) + if found && err == nil { + break + } + } + + if !found || err != nil { + return nil + } + + for _, container := range containers { + containerMap, ok := container.(map[string]any) + if !ok { + continue + } + + image, found, err := unstructured.NestedString(containerMap, "image") + if !found || err != nil { + continue + } + + images = append(images, image) + } + + return images +} + // RetryUntilSucceed keep retrying given action with specified interval until action succeed or specified context is done. func RetryUntilSucceed(ctx context.Context, interval time.Duration, desc string, log logr.Logger, action func() error) { pollErr := wait.PollUntilContextCancel(ctx, interval, true, func(_ context.Context) (bool /*done*/, error) { diff --git a/pkg/utils/kube/kube_test.go b/pkg/utils/kube/kube_test.go index 6a60b974b..ce443d52b 100644 --- a/pkg/utils/kube/kube_test.go +++ b/pkg/utils/kube/kube_test.go @@ -178,6 +178,67 @@ spec: assert.Nil(t, GetDeploymentReplicas(&noDeployment)) } +func TestGetResourceImagesForResourcesWithTemplate(t *testing.T) { + manifest := []byte(` +apiVersion: extensions/v1beta2 +kind: Deployment +metadata: + name: nginx-deployment + labels: + foo: bar +spec: + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: nginx:1.7.9 + ports: + - containerPort: 80 + - name: agent + image: agent:1.0.0 +`) + + expected := []string{"nginx:1.7.9", "agent:1.0.0"} + + deployment := unstructured.Unstructured{} + err := yaml.Unmarshal([]byte(manifest), &deployment) + require.NoError(t, err) + + images := GetResourceImages(&deployment) + assert.Equal(t, expected, images) +} + +func TestGetResourceImagesForPod(t *testing.T) { + manifest := []byte(` +apiVersion: v1 +kind: Pod +metadata: + name: example-pod + labels: + app: my-app +spec: + containers: + - name: nginx-container + image: nginx:1.21 + ports: + - containerPort: 80 + - name: sidecar-container + image: busybox:1.35 + command: ["sh", "-c", "echo Hello from the sidecar; sleep 3600"] +`) + expected := []string{"nginx:1.21", "busybox:1.35"} + + pod := unstructured.Unstructured{} + err := yaml.Unmarshal([]byte(manifest), &pod) + require.NoError(t, err) + + images := GetResourceImages(&pod) + assert.Equal(t, expected, images) +} + func TestSplitYAML_SingleObject(t *testing.T) { objs, err := SplitYAML([]byte(depWithLabel)) require.NoError(t, err) From 5533710e40cacf3ed64551bc8bb688e03271364a Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Mon, 25 Nov 2024 17:10:14 +0100 Subject: [PATCH 2/9] Use require instead of assert Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/utils/kube/kube_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/utils/kube/kube_test.go b/pkg/utils/kube/kube_test.go index ce443d52b..c5625ea62 100644 --- a/pkg/utils/kube/kube_test.go +++ b/pkg/utils/kube/kube_test.go @@ -208,7 +208,7 @@ spec: require.NoError(t, err) images := GetResourceImages(&deployment) - assert.Equal(t, expected, images) + require.Equal(t, expected, images) } func TestGetResourceImagesForPod(t *testing.T) { @@ -236,7 +236,7 @@ spec: require.NoError(t, err) images := GetResourceImages(&pod) - assert.Equal(t, expected, images) + require.Equal(t, expected, images) } func TestSplitYAML_SingleObject(t *testing.T) { From 4308173bf81f45bd773a58df37d4008423a7a1a2 Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Wed, 27 Nov 2024 11:12:41 +0100 Subject: [PATCH 3/9] Add test for empty images case Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/utils/kube/kube_test.go | 79 +++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/pkg/utils/kube/kube_test.go b/pkg/utils/kube/kube_test.go index c5625ea62..e7cfe7eb0 100644 --- a/pkg/utils/kube/kube_test.go +++ b/pkg/utils/kube/kube_test.go @@ -239,6 +239,85 @@ spec: require.Equal(t, expected, images) } +func TestGetImages_NoImagesPresent(t *testing.T) { + manifests := [][]byte{ + []byte(` +apiVersion: v1 +kind: ConfigMap +metadata: + name: example-config + namespace: default + labels: + app: my-app +data: + app.properties: | + key1=value1 + key2=value2 + key3=value3 + log.level: debug +`), + []byte(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: deployment-no-containers + labels: + foo: bar +spec: + replicas: 1 + selector: + matchLabels: + app: agent + template: + metadata: + labels: + app: agent + spec: + volumes: + - name: config-volume + configMap: + name: config +`), + []byte(` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: deployment-without-image +spec: + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: text-service + command: ["echo", "hello"] +`), + []byte(` +apiVersion: v1 +kind: Pod +metadata: + name: example-pod + labels: + app: my-app +spec: + containers: + - name: no-image-container + command: ["echo", "hello"] +`, + ), + } + + for _, manifest := range manifests { + resource := unstructured.Unstructured{} + err := yaml.Unmarshal([]byte(manifest), &resource) + require.NoError(t, err) + + images := GetResourceImages(&resource) + require.Empty(t, images) + } +} + func TestSplitYAML_SingleObject(t *testing.T) { objs, err := SplitYAML([]byte(depWithLabel)) require.NoError(t, err) From b0cea1964cbd20bd67abc9056f95b5b630f4d7a5 Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Wed, 27 Nov 2024 11:15:48 +0100 Subject: [PATCH 4/9] Rename test function to match regex Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/utils/kube/kube_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/utils/kube/kube_test.go b/pkg/utils/kube/kube_test.go index e7cfe7eb0..2049d0276 100644 --- a/pkg/utils/kube/kube_test.go +++ b/pkg/utils/kube/kube_test.go @@ -239,7 +239,7 @@ spec: require.Equal(t, expected, images) } -func TestGetImages_NoImagesPresent(t *testing.T) { +func TestGetImagesNoImagesPresent(t *testing.T) { manifests := [][]byte{ []byte(` apiVersion: v1 From 6a2f8090214e42d14a29abcf6ddde7945543b819 Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Wed, 27 Nov 2024 12:21:45 +0100 Subject: [PATCH 5/9] Add support for conjobs, refactor images implementation and add test for cronjobs Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/utils/kube/kube_test.go | 74 ++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 25 deletions(-) diff --git a/pkg/utils/kube/kube_test.go b/pkg/utils/kube/kube_test.go index 2049d0276..e9e9e5102 100644 --- a/pkg/utils/kube/kube_test.go +++ b/pkg/utils/kube/kube_test.go @@ -178,8 +178,16 @@ spec: assert.Nil(t, GetDeploymentReplicas(&noDeployment)) } -func TestGetResourceImagesForResourcesWithTemplate(t *testing.T) { - manifest := []byte(` +func TestGetResourceImages(t *testing.T) { + type testCase struct { + manifest []byte + expected []string + description string + } + + testCases := []testCase{ + { + manifest: []byte(` apiVersion: extensions/v1beta2 kind: Deployment metadata: @@ -198,21 +206,12 @@ spec: ports: - containerPort: 80 - name: agent - image: agent:1.0.0 -`) - - expected := []string{"nginx:1.7.9", "agent:1.0.0"} - - deployment := unstructured.Unstructured{} - err := yaml.Unmarshal([]byte(manifest), &deployment) - require.NoError(t, err) - - images := GetResourceImages(&deployment) - require.Equal(t, expected, images) -} - -func TestGetResourceImagesForPod(t *testing.T) { - manifest := []byte(` + image: agent:1.0.0`), + expected: []string{"nginx:1.7.9", "agent:1.0.0"}, + description: "deployment with two containers", + }, + { + manifest: []byte(` apiVersion: v1 kind: Pod metadata: @@ -228,15 +227,40 @@ spec: - name: sidecar-container image: busybox:1.35 command: ["sh", "-c", "echo Hello from the sidecar; sleep 3600"] -`) - expected := []string{"nginx:1.21", "busybox:1.35"} - - pod := unstructured.Unstructured{} - err := yaml.Unmarshal([]byte(manifest), &pod) - require.NoError(t, err) +`), + expected: []string{"nginx:1.21", "busybox:1.35"}, + description: "pod with containers", + }, + { + manifest: []byte(` +apiVersion: batch/v1 +kind: CronJob +metadata: + name: hello +spec: + schedule: "* * * * *" + jobTemplate: + spec: + template: + spec: + containers: + - name: hello + image: busybox:1.28 +`), + expected: []string{"busybox:1.28"}, + description: "cronjob with containers", + }, + } - images := GetResourceImages(&pod) - require.Equal(t, expected, images) + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + resource := unstructured.Unstructured{} + err := yaml.Unmarshal(tc.manifest, &resource) + require.NoError(t, err) + images := GetResourceImages(&resource) + require.Equal(t, tc.expected, images) + }) + } } func TestGetImagesNoImagesPresent(t *testing.T) { From 4928e6d842c37dc5ef51de6c209555c33fdfeb1d Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Wed, 27 Nov 2024 14:15:56 +0100 Subject: [PATCH 6/9] Move missing images tests to single function Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/utils/kube/kube_test.go | 57 +++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/pkg/utils/kube/kube_test.go b/pkg/utils/kube/kube_test.go index e9e9e5102..8a411f9ec 100644 --- a/pkg/utils/kube/kube_test.go +++ b/pkg/utils/kube/kube_test.go @@ -250,22 +250,8 @@ spec: expected: []string{"busybox:1.28"}, description: "cronjob with containers", }, - } - - for _, tc := range testCases { - t.Run(tc.description, func(t *testing.T) { - resource := unstructured.Unstructured{} - err := yaml.Unmarshal(tc.manifest, &resource) - require.NoError(t, err) - images := GetResourceImages(&resource) - require.Equal(t, tc.expected, images) - }) - } -} - -func TestGetImagesNoImagesPresent(t *testing.T) { - manifests := [][]byte{ - []byte(` + { + manifest: []byte(` apiVersion: v1 kind: ConfigMap metadata: @@ -280,7 +266,11 @@ data: key3=value3 log.level: debug `), - []byte(` + expected: nil, + description: "configmap without containers", + }, + { + manifest: []byte(` apiVersion: apps/v1 kind: Deployment metadata: @@ -302,7 +292,11 @@ spec: configMap: name: config `), - []byte(` + expected: nil, + description: "deployment without containers", + }, + { + manifest: []byte(` apiVersion: apps/v1 kind: Deployment metadata: @@ -317,7 +311,11 @@ spec: - name: text-service command: ["echo", "hello"] `), - []byte(` + expected: nil, + description: "deployment with container without image", + }, + { + manifest: []byte(` apiVersion: v1 kind: Pod metadata: @@ -328,17 +326,20 @@ spec: containers: - name: no-image-container command: ["echo", "hello"] -`, - ), +`), + expected: nil, + description: "pod with container without image", + }, } - for _, manifest := range manifests { - resource := unstructured.Unstructured{} - err := yaml.Unmarshal([]byte(manifest), &resource) - require.NoError(t, err) - - images := GetResourceImages(&resource) - require.Empty(t, images) + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + resource := unstructured.Unstructured{} + err := yaml.Unmarshal(tc.manifest, &resource) + require.NoError(t, err) + images := GetResourceImages(&resource) + require.Equal(t, tc.expected, images) + }) } } From e6f8fbec407893fc858ce0e028d649881254624f Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Wed, 27 Nov 2024 14:17:32 +0100 Subject: [PATCH 7/9] Refactor test Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/utils/kube/kube_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/utils/kube/kube_test.go b/pkg/utils/kube/kube_test.go index 8a411f9ec..efb4fde9d 100644 --- a/pkg/utils/kube/kube_test.go +++ b/pkg/utils/kube/kube_test.go @@ -179,13 +179,11 @@ spec: } func TestGetResourceImages(t *testing.T) { - type testCase struct { + testCases := []struct { manifest []byte expected []string description string - } - - testCases := []testCase{ + }{ { manifest: []byte(` apiVersion: extensions/v1beta2 From 914b89cb6a825d46f792a420ff43dd7760bd3115 Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Wed, 26 Feb 2025 15:12:23 +0100 Subject: [PATCH 8/9] Add benchmark for sync Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/sync/sync_context_test.go | 49 +++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/pkg/sync/sync_context_test.go b/pkg/sync/sync_context_test.go index 21f9730bb..52d565e11 100644 --- a/pkg/sync/sync_context_test.go +++ b/pkg/sync/sync_context_test.go @@ -7,12 +7,14 @@ import ( "net/http" "net/http/httptest" "reflect" + "strings" "testing" "time" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -2061,3 +2063,50 @@ func TestWaitForCleanUpBeforeNextWave(t *testing.T) { assert.Equal(t, synccommon.ResultCodePruned, result[1].Status) assert.Equal(t, synccommon.ResultCodePruned, result[2].Status) } + +func BenchmarkSync(b *testing.B) { + podManifest := `{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": { + "name": "my-pod" + }, + "spec": { + "containers": [ + ${containers} + ] + } + }` + container := `{ + "image": "nginx:1.7.9", + "name": "nginx", + "resources": { + "requests": { + "cpu": "0.2" + } + } + }` + + maxContainers := 10 + for i := 0; i < b.N; i++ { + b.StopTimer() + containerCount := min(i+1, maxContainers) + + containerStr := strings.Repeat(container+",", containerCount) + containerStr = containerStr[:len(containerStr)-1] + + manifest := strings.ReplaceAll(podManifest, "${containers}", containerStr) + pod := testingutils.Unstructured(manifest) + pod.SetNamespace(testingutils.FakeArgoCDNamespace) + + syncCtx := newTestSyncCtx(nil, WithOperationSettings(false, true, false, false)) + syncCtx.log = logr.Discard() + syncCtx.resources = groupResources(ReconciliationResult{ + Live: []*unstructured.Unstructured{nil, pod}, + Target: []*unstructured.Unstructured{testingutils.NewService(), nil}, + }) + + b.StartTimer() + syncCtx.Sync() + } +} From 4fbe25e3fbc446b9bd8622c7a5ff8547eceb6d3c Mon Sep 17 00:00:00 2001 From: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> Date: Thu, 27 Feb 2025 10:59:01 +0100 Subject: [PATCH 9/9] Update comment on images Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Aaron Hoffman <31711338+Aaron-9900@users.noreply.github.com> --- pkg/sync/common/types.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/sync/common/types.go b/pkg/sync/common/types.go index c8c1e607b..002bb23da 100644 --- a/pkg/sync/common/types.go +++ b/pkg/sync/common/types.go @@ -136,7 +136,9 @@ func NewHookDeletePolicy(p string) (HookDeletePolicy, bool) { type ResourceSyncResult struct { // holds associated resource key ResourceKey kube.ResourceKey - // holds the images associated with the resource + // Images holds the images associated with the resource. These images are collected on a best-effort basis + // from fields used by known workload resources. This does not necessarily reflect the exact list of images + // used by workloads in the application. Images []string // holds resource version Version string