From 25570fa8f103ba780915570edf048eca9e2d468c Mon Sep 17 00:00:00 2001 From: Adam Janikowski <12255597+ajanikow@users.noreply.github.com> Date: Thu, 29 Feb 2024 11:19:44 +0100 Subject: [PATCH] [Bugfix] Fix Image Error Propagation (#1603) --- CHANGELOG.md | 1 + cmd/cmd.go | 4 +- cmd/cmd_pod_test.go | 294 +++++++++++++++++++++++++++++++++++++ pkg/util/k8sutil/images.go | 6 +- 4 files changed, 300 insertions(+), 5 deletions(-) create mode 100644 cmd/cmd_pod_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index afba1c05d..1e8bdbbe5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - (Bugfix) Fix Image Discovery - (Bugfix) Fix Resources Copy mechanism to prevent invalid pod creation - (Bugfix) Wait for ImageStatus in ImageDiscover +- (Bugfix) Fix Image Error Propagation ## [1.2.38](https://github.com/arangodb/kube-arangodb/tree/1.2.38) (2024-02-22) - (Feature) Extract GRPC Server diff --git a/cmd/cmd.go b/cmd/cmd.go index 5c7f31ef9..bf85f80dd 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -616,7 +616,7 @@ func getMyPodInfoWrap(kubecli kubernetes.Interface, namespace, name string, imag } sa = pod.Spec.ServiceAccountName if i, ok := imageFunc(pod); !ok { - return errors.Wrap(err, "failed to get image ID from pod") + return errors.Errorf("failed to get image ID from pod") } else { image = i } @@ -633,7 +633,7 @@ func getMyImageInfoFunc(status bool) func(pod *core.Pod) (string, bool) { if status { return k8sutil.GetArangoDBImageIDFromContainerStatuses(pod.Status.ContainerStatuses, shared.ServerContainerName, shared.OperatorContainerName, constants.MyContainerNameEnv.GetOrDefault(shared.OperatorContainerName)) } - return k8sutil.GetArangoDBImageIDFromContainers(pod.Spec.Containers, shared.ServerContainerName, shared.OperatorContainerName, constants.MyContainerNameEnv.GetOrDefault(shared.OperatorContainerName)) + return k8sutil.GetArangoDBImageFromContainers(pod.Spec.Containers, shared.ServerContainerName, shared.OperatorContainerName, constants.MyContainerNameEnv.GetOrDefault(shared.OperatorContainerName)) } } diff --git a/cmd/cmd_pod_test.go b/cmd/cmd_pod_test.go new file mode 100644 index 000000000..b6ed9a9f9 --- /dev/null +++ b/cmd/cmd_pod_test.go @@ -0,0 +1,294 @@ +// +// DISCLAIMER +// +// Copyright 2024 ArangoDB GmbH, Cologne, Germany +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Copyright holder is ArangoDB GmbH, Cologne, Germany +// + +package cmd + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + core "k8s.io/api/core/v1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/arangodb/kube-arangodb/pkg/util" + "github.com/arangodb/kube-arangodb/pkg/util/kclient" + "github.com/arangodb/kube-arangodb/pkg/util/tests" +) + +func Test_PodDiscovery(t *testing.T) { + operatorImageDiscovery.timeout = time.Millisecond + + type testCase struct { + Name string + + Pod core.Pod + + Image, ServiceAccount string + + Valid bool + + DefaultStatusDiscovery *bool + } + + var testCases = []testCase{ + { + Name: "Empty pod", + Valid: false, + }, + { + Name: "Not allowed containers", + Valid: false, + Pod: core.Pod{ + ObjectMeta: meta.ObjectMeta{ + Name: "operator", + Namespace: tests.FakeNamespace, + }, + Spec: core.PodSpec{ + Containers: []core.Container{ + { + Name: "unknown", + Image: "image1", + }, + }, + }, + Status: core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{ + { + Name: "unknown", + Image: "image1", + ImageID: "image1", + }, + }, + }, + }, + }, + { + Name: "Allowed Status & Spec", + Valid: true, + Image: "image1", + ServiceAccount: "sa", + Pod: core.Pod{ + ObjectMeta: meta.ObjectMeta{ + Name: "operator", + Namespace: tests.FakeNamespace, + }, + Spec: core.PodSpec{ + ServiceAccountName: "sa", + Containers: []core.Container{ + { + Name: "operator", + Image: "image1", + }, + }, + }, + Status: core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{ + { + Name: "operator", + Image: "image1", + ImageID: "image1", + }, + }, + }, + }, + }, + { + Name: "Allowed Status & Spec", + Valid: true, + Image: "imageStatusID1", + ServiceAccount: "sa", + Pod: core.Pod{ + ObjectMeta: meta.ObjectMeta{ + Name: "operator", + Namespace: tests.FakeNamespace, + }, + Spec: core.PodSpec{ + ServiceAccountName: "sa", + Containers: []core.Container{ + { + Name: "operator", + Image: "imageSpec1", + }, + }, + }, + Status: core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{ + { + Name: "operator", + Image: "imageStatus1", + ImageID: "imageStatusID1", + }, + }, + }, + }, + }, + { + Name: "Allowed Status & Spec - From Spec", + Valid: true, + Image: "imageSpec1", + ServiceAccount: "sa", + DefaultStatusDiscovery: util.NewType(false), + Pod: core.Pod{ + ObjectMeta: meta.ObjectMeta{ + Name: "operator", + Namespace: tests.FakeNamespace, + }, + Spec: core.PodSpec{ + ServiceAccountName: "sa", + Containers: []core.Container{ + { + Name: "operator", + Image: "imageSpec1", + }, + }, + }, + Status: core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{ + { + Name: "operator", + Image: "imageStatus1", + ImageID: "imageStatusID1", + }, + }, + }, + }, + }, + { + Name: "Allowed Spec", + Valid: true, + Image: "imageSpec1", + ServiceAccount: "sa", + Pod: core.Pod{ + ObjectMeta: meta.ObjectMeta{ + Name: "operator", + Namespace: tests.FakeNamespace, + }, + Spec: core.PodSpec{ + ServiceAccountName: "sa", + Containers: []core.Container{ + { + Name: "operator", + Image: "imageSpec1", + }, + }, + }, + }, + }, + { + Name: "Allowed Status & Spec - From Second Pod", + Valid: true, + Image: "imageStatusID2", + ServiceAccount: "sa", + Pod: core.Pod{ + ObjectMeta: meta.ObjectMeta{ + Name: "operator", + Namespace: tests.FakeNamespace, + }, + Spec: core.PodSpec{ + ServiceAccountName: "sa", + Containers: []core.Container{ + { + Name: "test", + Image: "imageSpec1", + }, + { + Name: "operator", + Image: "imageSpec2", + }, + }, + }, + Status: core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{ + { + Name: "test", + Image: "imageStatus1", + ImageID: "imageStatusID1", + }, + { + Name: "operator", + Image: "imageStatus2", + ImageID: "imageStatusID2", + }, + }, + }, + }, + }, + { + Name: "Allowed Status & Spec - From Second Pod Spec", + Valid: true, + Image: "imageSpec2", + ServiceAccount: "sa", + DefaultStatusDiscovery: util.NewType(false), + Pod: core.Pod{ + ObjectMeta: meta.ObjectMeta{ + Name: "operator", + Namespace: tests.FakeNamespace, + }, + Spec: core.PodSpec{ + ServiceAccountName: "sa", + Containers: []core.Container{ + { + Name: "test", + Image: "imageSpec1", + }, + { + Name: "operator", + Image: "imageSpec2", + }, + }, + }, + Status: core.PodStatus{ + ContainerStatuses: []core.ContainerStatus{ + { + Name: "test", + Image: "imageStatus1", + ImageID: "imageStatusID1", + }, + { + Name: "operator", + Image: "imageStatus2", + ImageID: "imageStatusID2", + }, + }, + }, + }, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + operatorImageDiscovery.defaultStatusDiscovery = util.TypeOrDefault(testCase.DefaultStatusDiscovery, true) + + c := kclient.NewFakeClientBuilder().Add(&testCase.Pod).Client() + image, sa, err := getMyPodInfo(c.Kubernetes(), tests.FakeNamespace, "operator") + + if !testCase.Valid { + require.Error(t, err) + require.Empty(t, image) + require.Empty(t, sa) + } else { + require.NoError(t, err) + require.Equal(t, testCase.Image, image) + require.Equal(t, testCase.ServiceAccount, sa) + } + }) + } +} diff --git a/pkg/util/k8sutil/images.go b/pkg/util/k8sutil/images.go index aab27e310..3f0585840 100644 --- a/pkg/util/k8sutil/images.go +++ b/pkg/util/k8sutil/images.go @@ -54,7 +54,7 @@ func GetArangoDBImageIDFromPod(pod *core.Pod, names ...string) (string, error) { return image, nil } - if image, ok := GetArangoDBImageIDFromContainers(pod.Spec.Containers, names...); ok { + if image, ok := GetArangoDBImageFromContainers(pod.Spec.Containers, names...); ok { return image, nil } @@ -89,8 +89,8 @@ func GetArangoDBImageIDFromContainerStatuses(containers []core.ContainerStatus, return "", false } -// GetArangoDBImageIDFromContainers returns the ArangoDB specific image from a container specs -func GetArangoDBImageIDFromContainers(containers []core.Container, names ...string) (string, bool) { +// GetArangoDBImageFromContainers returns the ArangoDB specific image from a container specs +func GetArangoDBImageFromContainers(containers []core.Container, names ...string) (string, bool) { for _, name := range names { if id := container.GetContainerIDByName(containers, name); id != -1 { if image := containers[id].Image; image != "" {