From 7d149ead7ee9a0320edb1c62400a5b2f911624b2 Mon Sep 17 00:00:00 2001 From: "michal.gubricky" Date: Mon, 4 Mar 2024 14:04:10 +0100 Subject: [PATCH 1/4] Improve image filtering Signed-off-by: michal.gubricky --- .../openstacknodeimagerelease_controller.go | 30 +++-- ...enstacknodeimagerelease_controller_test.go | 115 ++++++++++++++++-- 2 files changed, 121 insertions(+), 24 deletions(-) diff --git a/internal/controller/openstacknodeimagerelease_controller.go b/internal/controller/openstacknodeimagerelease_controller.go index 73c984d3..50dc24fc 100644 --- a/internal/controller/openstacknodeimagerelease_controller.go +++ b/internal/controller/openstacknodeimagerelease_controller.go @@ -147,7 +147,7 @@ func (r *OpenStackNodeImageReleaseReconciler) Reconcile(ctx context.Context, req conditions.MarkTrue(openstacknodeimagerelease, apiv1alpha1.OpenStackImageServiceClientAvailableCondition) - imageID, err := findImageByName(imageClient, openstacknodeimagerelease.Spec.Image.CreateOpts.Name) + imageID, err := getImageID(imageClient, openstacknodeimagerelease.Spec.Image.CreateOpts) if err != nil { conditions.MarkFalse(openstacknodeimagerelease, apiv1alpha1.OpenStackImageReadyCondition, @@ -331,27 +331,35 @@ func (r *OpenStackNodeImageReleaseReconciler) getCloudFromSecret(ctx context.Con return cloud, nil } -func findImageByName(imagesClient *gophercloud.ServiceClient, imageName string) (string, error) { - listOpts := images.ListOpts{ - Name: imageName, +func getImageID(imagesClient *gophercloud.ServiceClient, imageCreateOps *apiv1alpha1.CreateOpts) (string, error) { + var listOpts images.ListOpts + + if imageCreateOps.ID != "" { + return imageCreateOps.ID, nil + } + listOpts = images.ListOpts{ + Name: imageCreateOps.Name, + Tags: imageCreateOps.Tags, } allPages, err := images.List(imagesClient, listOpts).AllPages() if err != nil { - return "", fmt.Errorf("failed to list images with name %s: %w", imageName, err) + return "", fmt.Errorf("failed to list images with name %s: %w", imageCreateOps.Name, err) } imageList, err := images.ExtractImages(allPages) if err != nil { - return "", fmt.Errorf("failed to extract images with name %s: %w", imageName, err) + return "", fmt.Errorf("failed to extract images with name %s: %w", imageCreateOps.Name, err) } - for i := range imageList { - if imageList[i].Name == imageName { - return imageList[i].ID, nil - } + switch len(imageList) { + case 0: + return "", nil + case 1: + return imageList[0].ID, nil + default: + return "", fmt.Errorf("too many images were found with the given image name: %s with tags: %s", imageCreateOps.Name, imageCreateOps.Tags) } - return "", nil } func createImage(imageClient *gophercloud.ServiceClient, createOpts *apiv1alpha1.CreateOpts) (*images.Image, error) { diff --git a/internal/controller/openstacknodeimagerelease_controller_test.go b/internal/controller/openstacknodeimagerelease_controller_test.go index 841faaf6..04dcd468 100644 --- a/internal/controller/openstacknodeimagerelease_controller_test.go +++ b/internal/controller/openstacknodeimagerelease_controller_test.go @@ -188,15 +188,34 @@ clouds: assert.NoError(t, err) } -func TestFindImageByName(t *testing.T) { +func TestGetImageID(t *testing.T) { th.SetupHTTP() defer th.TeardownHTTP() HandleImageListSuccessfully(t) - imageName := "test_image" + imageFilter := &apiv1alpha1.CreateOpts{ + ID: "123", + } + + imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter) + + assert.NoError(t, err) + assert.Equal(t, "123", imageID) +} + +func TestGetImageIDByNameAndTags(t *testing.T) { + th.SetupHTTP() + defer th.TeardownHTTP() + + HandleImageListSuccessfully(t) + + imageFilter := &apiv1alpha1.CreateOpts{ + Name: "test_image", + Tags: []string{"v1"}, + } - imageID, err := findImageByName(fakeclient.ServiceClient(), imageName) + imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter) assert.NoError(t, err) assert.Equal(t, "123", imageID) @@ -213,33 +232,103 @@ func HandleImageListSuccessfully(t *testing.T) { //nolint: gocritic w.WriteHeader(http.StatusOK) fmt.Fprintf(w, `{ "images": [ - {"id": "123", "name": "test_image"}, - {"id": "456", "name": "test_image2"}, - {"id": "789", "name": "test_image3"} + {"id": "123", "name": "test_image", "tags": ["v1"]} + ] + }`) + }) +} + +func TestGetImageIDWithTwoSameImageNames(t *testing.T) { + th.SetupHTTP() + defer th.TeardownHTTP() + + HandleImageListWithTwoImagesSuccessfully(t) + + imageFilter := &apiv1alpha1.CreateOpts{ + Name: "test_image", + Tags: []string{"v1"}, + } + + imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter) + + assert.Error(t, err) // Expecting an error due to multiple images with the same name + assert.Equal(t, "", imageID) + assert.Equal(t, err.Error(), "too many images were found with the given image name: test_image with tags: [v1]") +} + +// HandleImageListWithTwoImagesSuccessfully sets up a fake response for image list request. +func HandleImageListWithTwoImagesSuccessfully(t *testing.T) { //nolint: gocritic + t.Helper() // Indicate that this is a test helper function + th.Mux.HandleFunc("/images", func(w http.ResponseWriter, r *http.Request) { + th.TestMethod(t, r, "GET") + th.TestHeader(t, r, "X-Auth-Token", fakeclient.TokenID) + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprintf(w, `{ + "images": [ + {"id": "123", "name": "test_image", "tags": ["v1"]}, + {"id": "456", "name": "test_image", "tags": ["v1"]} ] }`) }) } -func TestFindImageByNameWrongImageName(t *testing.T) { +func TestGetImageIDNoImageFound(t *testing.T) { + th.SetupHTTP() + defer th.TeardownHTTP() + + HandleImageListWithNoImageFoundSuccessfully(t) + + imageFilter := &apiv1alpha1.CreateOpts{ + Name: "test_image", + Tags: []string{"v1"}, + } + + imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter) + + assert.NoError(t, err) + assert.Equal(t, "", imageID) +} + +// HandleImageListWithNoImageFoundSuccessfully sets up a fake response for image list request. +func HandleImageListWithNoImageFoundSuccessfully(t *testing.T) { //nolint: gocritic + t.Helper() // Indicate that this is a test helper function + th.Mux.HandleFunc("/images", func(w http.ResponseWriter, r *http.Request) { + th.TestMethod(t, r, "GET") + th.TestHeader(t, r, "X-Auth-Token", fakeclient.TokenID) + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprintf(w, `{ + "images": [] + }`) + }) +} + +func TestGetImageIDWrongImageName(t *testing.T) { th.SetupHTTP() defer th.TeardownHTTP() HandleImageListSuccessfully(t) - imageName := "test_bad_image" + imageFilter := &apiv1alpha1.CreateOpts{ + Name: "test_bad_image", + } - imageID, err := findImageByName(fakeclient.ServiceClient(), imageName) + imageID, err := getImageID(fakeclient.ServiceClient(), imageFilter) assert.NoError(t, err) assert.NotEqual(t, "231", imageID) } -func TestFindImageByNameNotFound(t *testing.T) { +func TestGetImageIDNotFound(t *testing.T) { th.SetupHTTP() defer th.TeardownHTTP() - imageName := "test_image" + imageFilter := &apiv1alpha1.CreateOpts{ + Name: "test_image", + } th.Mux.HandleFunc("/images", func(w http.ResponseWriter, r *http.Request) { th.TestMethod(t, r, http.MethodGet) @@ -253,11 +342,11 @@ func TestFindImageByNameNotFound(t *testing.T) { fakeClient := fakeclient.ServiceClient() - imageID, err := findImageByName(fakeClient, imageName) + imageID, err := getImageID(fakeClient, imageFilter) assert.Error(t, err) assert.Equal(t, "", imageID) - assert.Contains(t, err.Error(), fmt.Sprintf("failed to list images with name %s: Resource not found", imageName)) + assert.Contains(t, err.Error(), fmt.Sprintf("failed to list images with name %s: Resource not found", imageFilter.Name)) } // HandleImageCreationSuccessfully test setup. From a6edcb57cd1f27e594052ebd308bc871669c5d5c Mon Sep 17 00:00:00 2001 From: "michal.gubricky" Date: Thu, 7 Mar 2024 14:32:00 +0100 Subject: [PATCH 2/4] Return empty string if image with ID is not found Signed-off-by: michal.gubricky --- .../openstacknodeimagerelease_controller.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/internal/controller/openstacknodeimagerelease_controller.go b/internal/controller/openstacknodeimagerelease_controller.go index 50dc24fc..3fd7d77f 100644 --- a/internal/controller/openstacknodeimagerelease_controller.go +++ b/internal/controller/openstacknodeimagerelease_controller.go @@ -335,11 +335,14 @@ func getImageID(imagesClient *gophercloud.ServiceClient, imageCreateOps *apiv1al var listOpts images.ListOpts if imageCreateOps.ID != "" { - return imageCreateOps.ID, nil - } - listOpts = images.ListOpts{ - Name: imageCreateOps.Name, - Tags: imageCreateOps.Tags, + listOpts = images.ListOpts{ + ID: imageCreateOps.ID, + } + } else { + listOpts = images.ListOpts{ + Name: imageCreateOps.Name, + Tags: imageCreateOps.Tags, + } } allPages, err := images.List(imagesClient, listOpts).AllPages() From 63216adcc6b5dc9c8ba69756e8288d8e32bbc3f7 Mon Sep 17 00:00:00 2001 From: Michal Gubricky Date: Thu, 7 Mar 2024 14:33:04 +0100 Subject: [PATCH 3/4] Update internal/controller/openstacknodeimagerelease_controller.go Co-authored-by: Matej Feder Signed-off-by: Michal Gubricky --- internal/controller/openstacknodeimagerelease_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/openstacknodeimagerelease_controller.go b/internal/controller/openstacknodeimagerelease_controller.go index 3fd7d77f..fe570c3d 100644 --- a/internal/controller/openstacknodeimagerelease_controller.go +++ b/internal/controller/openstacknodeimagerelease_controller.go @@ -361,7 +361,7 @@ func getImageID(imagesClient *gophercloud.ServiceClient, imageCreateOps *apiv1al case 1: return imageList[0].ID, nil default: - return "", fmt.Errorf("too many images were found with the given image name: %s with tags: %s", imageCreateOps.Name, imageCreateOps.Tags) + return "", fmt.Errorf("too many images were found with the given image name: %s and tags: %s", imageCreateOps.Name, imageCreateOps.Tags) } } From 26beceab0ae66ab39264aba08fb27bb57e0322f6 Mon Sep 17 00:00:00 2001 From: "michal.gubricky" Date: Thu, 7 Mar 2024 14:40:13 +0100 Subject: [PATCH 4/4] Fix unit-tests after added suggestion Signed-off-by: michal.gubricky --- .../controller/openstacknodeimagerelease_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/openstacknodeimagerelease_controller_test.go b/internal/controller/openstacknodeimagerelease_controller_test.go index 04dcd468..ac2be359 100644 --- a/internal/controller/openstacknodeimagerelease_controller_test.go +++ b/internal/controller/openstacknodeimagerelease_controller_test.go @@ -253,7 +253,7 @@ func TestGetImageIDWithTwoSameImageNames(t *testing.T) { assert.Error(t, err) // Expecting an error due to multiple images with the same name assert.Equal(t, "", imageID) - assert.Equal(t, err.Error(), "too many images were found with the given image name: test_image with tags: [v1]") + assert.Equal(t, err.Error(), "too many images were found with the given image name: test_image and tags: [v1]") } // HandleImageListWithTwoImagesSuccessfully sets up a fake response for image list request.