diff --git a/controllers/component_image_controller.go b/controllers/component_image_controller.go index 3292dcb..16a3bfa 100644 --- a/controllers/component_image_controller.go +++ b/controllers/component_image_controller.go @@ -239,12 +239,12 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( repositoryInfo.Message = "Quay organization plan doesn't allow private image repositories" } else { log.Error(err, "Error in the repository generation process", l.Audit, "true") - return ctrl.Result{}, r.reportError(ctx, component, "failed to generete image repository") + return ctrl.Result{}, r.reportError(ctx, component, "failed to generate image repository") } } else { if repo == nil || pushRobotAccount == nil || pullRobotAccount == nil { log.Error(nil, "Unknown error in the repository generation process", l.Audit, "true") - return ctrl.Result{}, r.reportError(ctx, component, "failed to generete image repository: unknown error") + return ctrl.Result{}, r.reportError(ctx, component, "failed to generate image repository: unknown error") } log.Info(fmt.Sprintf("Prepared image repository %s for Component", repo.Name), l.Action, l.ActionAdd) diff --git a/controllers/component_image_controller_test.go b/controllers/component_image_controller_test.go index 16e9058..fa46814 100644 --- a/controllers/component_image_controller_test.go +++ b/controllers/component_image_controller_test.go @@ -435,6 +435,98 @@ var _ = Describe("Component image controller", func() { Expect(repoImageInfo.Secret).To(Equal(resourceKey.Name)) }) + It("should stop and report error if image repository creation fails", func() { + isCreateRepositoryInvoked := false + CreateRepositoryFunc = func(repository quay.RepositoryRequest) (*quay.Repository, error) { + isCreateRepositoryInvoked = true + return nil, fmt.Errorf("fail to marshal data") + } + + repositoryInfoJsonBytes, _ := json.Marshal(ImageRepositoryStatus{}) + setComponentAnnotationValue(resourceKey, ImageAnnotationName, string(repositoryInfoJsonBytes)) + setComponentAnnotationValue(resourceKey, GenerateImageAnnotationName, `{"visibility": "public"}`) + + Eventually(func() bool { return isCreateRepositoryInvoked }, timeout, interval).Should(BeTrue()) + + expectedValue, _ := json.Marshal(&ImageRepositoryStatus{Message: "failed to generate image repository"}) + waitComponentAnnotationWithValue(resourceKey, ImageAnnotationName, string(expectedValue)) + }) + + It("should do nothing and set error for changing visibility if image is invalid in image annotation", func() { + CreateRepositoryFunc = func(repository quay.RepositoryRequest) (*quay.Repository, error) { + defer GinkgoRecover() + Fail("Image repository creation should not be invoked") + return nil, nil + } + + // An invalid image is set, which does not include registry. + setComponentAnnotationValue(resourceKey, ImageAnnotationName, `{"image": "ns/img:tag", "secret": "1234"}`) + setComponentAnnotationValue(resourceKey, GenerateImageAnnotationName, `{"visibility": "private"}`) + + waitComponentAnnotationGone(resourceKey, GenerateImageAnnotationName) + waitComponentAnnotation(resourceKey, ImageAnnotationName) + + repoImageInfo := &ImageRepositoryStatus{} + component := getComponent(resourceKey) + Expect(json.Unmarshal([]byte(component.Annotations[ImageAnnotationName]), repoImageInfo)).To(Succeed()) + Expect(repoImageInfo.Message).To(Equal("Invalid image url")) + }) + + It("nothing is changed and keep doing reconcile if fail to change repository visibility", func() { + // Work with a specific component in order to avoid potential conflict error happened in any subsequent test. + testComponentKey := types.NamespacedName{ + Name: defaultComponentName + "-stop-if-fail-to-change-repo-visibility", + Namespace: defaultComponentNamespace, + } + createComponent(componentConfig{ComponentKey: testComponentKey}) + + isChangeRepositoryVisibilityInvoked := false + ChangeRepositoryVisibilityFunc = func(string, string, string) error { + isChangeRepositoryVisibilityInvoked = true + return fmt.Errorf("failed to change repository visibility") + } + + repoInfo := map[string]string{ + "name": "img", + "image": "registry/ns/img:0.1", + "secret": "1234", + "visibility": "public", + } + imageAnnotationValue, _ := json.Marshal(repoInfo) + setComponentAnnotationValue(testComponentKey, ImageAnnotationName, string(imageAnnotationValue)) + + // Start to change visibility to private + generateAnnotationValue := `{"visibility": "private"}` + setComponentAnnotationValue(testComponentKey, GenerateImageAnnotationName, generateAnnotationValue) + + Eventually(func() bool { return isChangeRepositoryVisibilityInvoked }, timeout, interval).Should(BeTrue()) + + // Failed to change the visibility, reconciler should return immediately and annotations are not changed + ensureComponentAnnotationUnchangedWithValue(testComponentKey, ImageAnnotationName, string(imageAnnotationValue)) + ensureComponentAnnotationUnchangedWithValue(testComponentKey, GenerateImageAnnotationName, generateAnnotationValue) + + deleteComponent(testComponentKey) + }) + + It("should do nothing and set error if image annotation is invalid JSON", func() { + CreateRepositoryFunc = func(repository quay.RepositoryRequest) (*quay.Repository, error) { + defer GinkgoRecover() + Fail("Image repository creation should not be invoked") + return nil, nil + } + + setComponentAnnotationValue(resourceKey, ImageAnnotationName, `{"image": "registry/ns/img:tag}`) + setComponentAnnotationValue(resourceKey, GenerateImageAnnotationName, `{"visibility": "private"}`) + + waitComponentAnnotationGone(resourceKey, GenerateImageAnnotationName) + waitComponentAnnotation(resourceKey, ImageAnnotationName) + + repoImageInfo := &ImageRepositoryStatus{} + component := getComponent(resourceKey) + Expect(json.Unmarshal([]byte(component.Annotations[ImageAnnotationName]), repoImageInfo)).To(Succeed()) + Expect(repoImageInfo.Message).To(Equal("Invalid image status annotation")) + }) + It("should not block component deletion if clean up fails", func() { waitImageRepositoryFinalizerOnComponent(resourceKey) diff --git a/controllers/suite_util_test.go b/controllers/suite_util_test.go index 8129988..904a616 100644 --- a/controllers/suite_util_test.go +++ b/controllers/suite_util_test.go @@ -174,6 +174,39 @@ func waitComponentAnnotation(componentKey types.NamespacedName, annotationName s }, timeout, interval).Should(BeTrue()) } +// waitComponentAnnotationWithValue waits for a component have had an annotation with a specific value. +func waitComponentAnnotationWithValue(componentKey types.NamespacedName, annotationName, value string) { + Eventually(func() bool { + component := getComponent(componentKey) + annotations := component.GetAnnotations() + if annotations == nil { + return false + } + val, exists := annotations[annotationName] + if exists { + return val == value + } else { + return false + } + }, timeout, interval).Should(BeTrue()) +} + +func ensureComponentAnnotationUnchangedWithValue(componentKey types.NamespacedName, annotationName, value string) { + Consistently(func() bool { + component := getComponent(componentKey) + annotations := component.GetAnnotations() + if annotations == nil { + return false + } + val, exists := annotations[annotationName] + if exists { + return val == value + } else { + return false + } + }, timeout, interval).Should(BeTrue()) +} + func waitComponentAnnotationGone(componentKey types.NamespacedName, annotationName string) { Eventually(func() bool { component := getComponent(componentKey)