From 0d3d46a4d93406ffa035882d2d08723c84269774 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Thu, 12 Jan 2023 16:03:43 +0800 Subject: [PATCH 1/5] relax the case for last applied annotation is removed --- .../work/apply_controller_helper_test.go | 4 +- .../work/apply_controller_integration_test.go | 65 +++++++++++++++++++ pkg/controllers/work/patch_util.go | 11 ++-- 3 files changed, 74 insertions(+), 6 deletions(-) diff --git a/pkg/controllers/work/apply_controller_helper_test.go b/pkg/controllers/work/apply_controller_helper_test.go index e4aaf29e1..e0cd3f935 100644 --- a/pkg/controllers/work/apply_controller_helper_test.go +++ b/pkg/controllers/work/apply_controller_helper_test.go @@ -51,8 +51,8 @@ func verifyAppliedConfigMap(cm *corev1.ConfigMap) *corev1.ConfigMap { for key := range cm.Annotations { Expect(appliedCM.Annotations[key]).Should(Equal(cm.Annotations[key])) } - Expect(appliedCM.Annotations[manifestHashAnnotation]).ShouldNot(BeNil()) - Expect(appliedCM.Annotations[lastAppliedConfigAnnotation]).ShouldNot(BeNil()) + Expect(appliedCM.Annotations[manifestHashAnnotation]).ShouldNot(BeEmpty()) + Expect(appliedCM.Annotations[lastAppliedConfigAnnotation]).ShouldNot(BeEmpty()) By("Check the config map data") Expect(cmp.Diff(appliedCM.Data, cm.Data)).Should(BeEmpty()) diff --git a/pkg/controllers/work/apply_controller_integration_test.go b/pkg/controllers/work/apply_controller_integration_test.go index eb8c167c9..22a0c282f 100644 --- a/pkg/controllers/work/apply_controller_integration_test.go +++ b/pkg/controllers/work/apply_controller_integration_test.go @@ -474,6 +474,71 @@ var _ = Describe("Work Controller", func() { Expect(appliedCM.OwnerReferences[1].Name).Should(SatisfyAny(Equal(work.GetName()), Equal(work2.GetName()))) }) + It("Check that the apply still works if the last applied annotation does not exist", func() { + cmName := "test-merge-without-lastapply" + cmNamespace := defaultNS + cm = &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: cmName, + Namespace: cmNamespace, + Labels: map[string]string{ + "labelKey1": "value1", + "labelKey2": "value2", + "labelKey3": "value3", + }, + }, + Data: map[string]string{ + "data1": "test1", + }, + } + + By("create the work") + work = createWorkWithManifest(workNamespace, cm) + err := k8sClient.Create(context.Background(), work) + Expect(err).ToNot(HaveOccurred()) + + By("wait for the work to be applied") + waitForWorkToApply(work.GetName(), work.GetNamespace()) + + By("Check applied configMap") + appliedCM := verifyAppliedConfigMap(cm) + + By("Delete the last applied annotation from the current resource") + delete(appliedCM.Annotations, lastAppliedConfigAnnotation) + Expect(k8sClient.Update(context.Background(), appliedCM)).Should(Succeed()) + + By("Get the last applied config map and verify it does not have the last applied annotation") + var modifiedCM corev1.ConfigMap + Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: cm.GetName(), Namespace: cm.GetNamespace()}, &modifiedCM)).Should(Succeed()) + Expect(modifiedCM.Annotations[lastAppliedConfigAnnotation]).Should(BeEmpty()) + + By("Modify the manifest") + // modify one data + cm.Data["data1"] = "modifiedValue" + // add a conflict data + cm.Data["data2"] = "added by manifest" + // change label key3 with a new value + cm.Labels["labelKey3"] = "added-back-by-manifest" + + By("update the work") + resultWork := waitForWorkToApply(work.GetName(), work.GetNamespace()) + rawCM, err := json.Marshal(cm) + Expect(err).Should(Succeed()) + resultWork.Spec.Workload.Manifests[0].Raw = rawCM + Expect(k8sClient.Update(context.Background(), resultWork)).Should(Succeed()) + + By("wait for the change of the work to be applied") + waitForWorkToApply(work.GetName(), work.GetNamespace()) + + By("Check applied configMap is modified even without the last applied annotation") + Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: cmName, Namespace: cmNamespace}, appliedCM)).Should(Succeed()) + verifyAppliedConfigMap(cm) + }) + It("Check that failed to apply manifest has the proper identification", func() { broadcastName := "testfail" namespace := defaultNS diff --git a/pkg/controllers/work/patch_util.go b/pkg/controllers/work/patch_util.go index 10d433b1d..7684466fe 100644 --- a/pkg/controllers/work/patch_util.go +++ b/pkg/controllers/work/patch_util.go @@ -2,7 +2,6 @@ package controllers import ( "encoding/json" - "errors" "fmt" "k8s.io/apimachinery/pkg/api/meta" @@ -12,6 +11,7 @@ import ( "k8s.io/apimachinery/pkg/util/mergepatch" "k8s.io/apimachinery/pkg/util/strategicpatch" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -114,14 +114,17 @@ func setModifiedConfigurationAnnotation(obj runtime.Object) error { func getOriginalConfiguration(obj runtime.Object) ([]byte, error) { annots, err := metadataAccessor.Annotations(obj) if err != nil { - return nil, fmt.Errorf("cannot access metadata.annotations: %w", err) + klog.Warning("cannot access metadata.annotations", "error", err) + return nil, nil } if annots == nil { - return nil, errors.New("object does not have lastAppliedConfigAnnotation") + klog.Warning("object does not have lastAppliedConfigAnnotation", "obj", obj) + return nil, nil } original, ok := annots[lastAppliedConfigAnnotation] if !ok { - return nil, errors.New("object does not have lastAppliedConfigAnnotation") + klog.Warning("object does not have lastAppliedConfigAnnotation", "obj", obj) + return nil, nil } return []byte(original), nil } From 47c51b529c10f05d2cef9f90a831242cb796ad73 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Fri, 13 Jan 2023 15:03:51 +0800 Subject: [PATCH 2/5] only trun trivy after it's merged to main --- .github/workflows/trivy.yml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/.github/workflows/trivy.yml b/.github/workflows/trivy.yml index ab9140f53..765d3dbfb 100644 --- a/.github/workflows/trivy.yml +++ b/.github/workflows/trivy.yml @@ -3,11 +3,9 @@ on: push: branches: - main - pull_request: - branches: - - main - - release-* - paths-ignore: [docs/**, "**.md", "**.mdx", "**.png", "**.jpg"] + create: + # Publish semver tags as releases. + tags: [ 'v*.*.*' ] permissions: contents: read @@ -32,7 +30,7 @@ jobs: # registry must be in lowercase # store the images under dev # TODO: need to cleanup dev images periodically - echo "::set-output name=registry::$(echo "${{ env.REGISTRY }}/${{ github.repository }}/dev" | tr [:upper:] [:lower:])" + echo "::set-output name=registry::$(echo "${{ env.REGISTRY }}/${{ github.repository }}" | tr [:upper:] [:lower:])" scan-images: needs: export-registry env: From 1cc5031db5df4259699fe6be1c39d0c0289640d9 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Fri, 13 Jan 2023 15:13:05 +0800 Subject: [PATCH 3/5] fix comment --- pkg/controllers/work/patch_util.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controllers/work/patch_util.go b/pkg/controllers/work/patch_util.go index 7684466fe..cf7ad3c54 100644 --- a/pkg/controllers/work/patch_util.go +++ b/pkg/controllers/work/patch_util.go @@ -114,11 +114,11 @@ func setModifiedConfigurationAnnotation(obj runtime.Object) error { func getOriginalConfiguration(obj runtime.Object) ([]byte, error) { annots, err := metadataAccessor.Annotations(obj) if err != nil { - klog.Warning("cannot access metadata.annotations", "error", err) - return nil, nil + klog.ErrorS(err, "cannot access metadata.annotations", "gvk", obj.GetObjectKind().GroupVersionKind()) + return nil, err } if annots == nil { - klog.Warning("object does not have lastAppliedConfigAnnotation", "obj", obj) + klog.Warning("object does not have annotation", "obj", obj) return nil, nil } original, ok := annots[lastAppliedConfigAnnotation] From b0a787d00e7502fac3c7e165afad547204be27de Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Fri, 13 Jan 2023 16:09:56 +0800 Subject: [PATCH 4/5] address comments --- .../work/apply_controller_integration_test.go | 13 +++++++------ pkg/controllers/work/patch_util.go | 1 + 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/controllers/work/apply_controller_integration_test.go b/pkg/controllers/work/apply_controller_integration_test.go index 22a0c282f..195d10f28 100644 --- a/pkg/controllers/work/apply_controller_integration_test.go +++ b/pkg/controllers/work/apply_controller_integration_test.go @@ -475,6 +475,7 @@ var _ = Describe("Work Controller", func() { }) It("Check that the apply still works if the last applied annotation does not exist", func() { + ctx = context.Background() cmName := "test-merge-without-lastapply" cmNamespace := defaultNS cm = &corev1.ConfigMap{ @@ -498,8 +499,8 @@ var _ = Describe("Work Controller", func() { By("create the work") work = createWorkWithManifest(workNamespace, cm) - err := k8sClient.Create(context.Background(), work) - Expect(err).ToNot(HaveOccurred()) + err := k8sClient.Create(ctx, work) + Expect(err).Should(Succeed()) By("wait for the work to be applied") waitForWorkToApply(work.GetName(), work.GetNamespace()) @@ -509,11 +510,11 @@ var _ = Describe("Work Controller", func() { By("Delete the last applied annotation from the current resource") delete(appliedCM.Annotations, lastAppliedConfigAnnotation) - Expect(k8sClient.Update(context.Background(), appliedCM)).Should(Succeed()) + Expect(k8sClient.Update(ctx, appliedCM)).Should(Succeed()) By("Get the last applied config map and verify it does not have the last applied annotation") var modifiedCM corev1.ConfigMap - Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: cm.GetName(), Namespace: cm.GetNamespace()}, &modifiedCM)).Should(Succeed()) + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cm.GetName(), Namespace: cm.GetNamespace()}, &modifiedCM)).Should(Succeed()) Expect(modifiedCM.Annotations[lastAppliedConfigAnnotation]).Should(BeEmpty()) By("Modify the manifest") @@ -529,13 +530,13 @@ var _ = Describe("Work Controller", func() { rawCM, err := json.Marshal(cm) Expect(err).Should(Succeed()) resultWork.Spec.Workload.Manifests[0].Raw = rawCM - Expect(k8sClient.Update(context.Background(), resultWork)).Should(Succeed()) + Expect(k8sClient.Update(ctx, resultWork)).Should(Succeed()) By("wait for the change of the work to be applied") waitForWorkToApply(work.GetName(), work.GetNamespace()) By("Check applied configMap is modified even without the last applied annotation") - Expect(k8sClient.Get(context.Background(), types.NamespacedName{Name: cmName, Namespace: cmNamespace}, appliedCM)).Should(Succeed()) + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cmName, Namespace: cmNamespace}, appliedCM)).Should(Succeed()) verifyAppliedConfigMap(cm) }) diff --git a/pkg/controllers/work/patch_util.go b/pkg/controllers/work/patch_util.go index cf7ad3c54..846eacf2e 100644 --- a/pkg/controllers/work/patch_util.go +++ b/pkg/controllers/work/patch_util.go @@ -117,6 +117,7 @@ func getOriginalConfiguration(obj runtime.Object) ([]byte, error) { klog.ErrorS(err, "cannot access metadata.annotations", "gvk", obj.GetObjectKind().GroupVersionKind()) return nil, err } + // the threeWayMergePatch lib can handle the case that the original is empty if annots == nil { klog.Warning("object does not have annotation", "obj", obj) return nil, nil From 8b7bcdbbbf6575f110294eed32190d2d82c44083 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Fri, 13 Jan 2023 01:32:58 -0800 Subject: [PATCH 5/5] Update pkg/controllers/work/patch_util.go Co-authored-by: Zhiying Lin <54013513+zhiying-lin@users.noreply.github.com> --- pkg/controllers/work/patch_util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controllers/work/patch_util.go b/pkg/controllers/work/patch_util.go index 846eacf2e..6964d731a 100644 --- a/pkg/controllers/work/patch_util.go +++ b/pkg/controllers/work/patch_util.go @@ -117,7 +117,7 @@ func getOriginalConfiguration(obj runtime.Object) ([]byte, error) { klog.ErrorS(err, "cannot access metadata.annotations", "gvk", obj.GetObjectKind().GroupVersionKind()) return nil, err } - // the threeWayMergePatch lib can handle the case that the original is empty + // The func threeWayMergePatch can handle the case that the original is empty. if annots == nil { klog.Warning("object does not have annotation", "obj", obj) return nil, nil