Merge pull request #18513 from tnozicka/fix-annotation-trigger-reconciliation

Automatic merge from submit-queue (batch tested with PRs 18503, 18399, 13701, 18513, 18515).

Fix annotation trigger to reconcile on container image change

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1541685

@smarterclayton this should fix your issues with DS and annotation trigger

Author: openshift-merge-robot

pkg/image/OWNERS

@@ -5,9 +5,11 @@ reviewers:
   - miminar
   - mfojtik
   - liggitt
+  - tnozicka
 approvers:
   - bparees
   - smarterclayton
   - soltysh
   - mfojtik
   - liggitt
+  - tnozicka

pkg/image/trigger/annotations/annotations.go

@@ -172,6 +172,33 @@ func UpdateObjectFromImages(obj runtime.Object, tagRetriever trigger.TagRetrieve
 	return updated, nil
 }
 
+// ContainerImageChanged returns true if any container image referenced by newTriggers changed.
+func ContainerImageChanged(oldObj, newObj runtime.Object, newTriggers []triggerapi.ObjectFieldTrigger) bool {
+	for _, trigger := range newTriggers {
+		if trigger.Paused {
+			continue
+		}
+
+		newContainer, _, err := ContainerForObjectFieldPath(newObj, trigger.FieldPath)
+		if err != nil {
+			glog.V(5).Infof("%v", err)
+			continue
+		}
+
+		oldContainer, _, err := ContainerForObjectFieldPath(oldObj, trigger.FieldPath)
+		if err != nil {
+			// might just be a result of the update
+			continue
+		}
+
+		if newContainer.GetImage() != oldContainer.GetImage() {
+			return true
+		}
+	}
+
+	return false
+}
+
 // annotationTriggerIndexer uses annotations on objects to trigger changes.
 type annotationTriggerIndexer struct {
 	prefix string
@@ -236,6 +263,8 @@ func (i annotationTriggerIndexer) Index(obj, old interface{}) (string, *trigger.
 			change = cache.Added
 		case !reflect.DeepEqual(oldTriggers, triggers):
 			change = cache.Updated
+		case ContainerImageChanged(old.(runtime.Object), obj.(runtime.Object), triggers):
+			change = cache.Updated
 		}
 	}
 

test/extended/extended_test.go

@@ -14,6 +14,7 @@ import (
 	_ "github.com/openshift/origin/test/extended/image_ecosystem"
 	_ "github.com/openshift/origin/test/extended/imageapis"
 	_ "github.com/openshift/origin/test/extended/images"
+	_ "github.com/openshift/origin/test/extended/images/trigger"
 	_ "github.com/openshift/origin/test/extended/jobs"
 	_ "github.com/openshift/origin/test/extended/localquota"
 	_ "github.com/openshift/origin/test/extended/networking"

test/extended/images/trigger/annotation.go

@@ -0,0 +1,68 @@
+package trigger
+
+import (
+	"time"
+
+	g "github.com/onsi/ginkgo"
+	o "github.com/onsi/gomega"
+
+	appsv1 "k8s.io/api/apps/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/kubernetes/test/e2e/framework"
+
+	exutil "github.com/openshift/origin/test/extended/util"
+)
+
+var (
+	SyncTimeout = 30 * time.Second
+)
+
+var _ = g.Describe("[Feature:AnnotationTrigger] Annotation trigger", func() {
+	defer g.GinkgoRecover()
+
+	oc := exutil.NewCLI("cli-deployment", exutil.KubeConfigPath())
+
+	var (
+		deploymentFixture = exutil.FixturePath("testdata", "image", "deployment-with-annotation-trigger.yaml")
+	)
+
+	g.It("reconciles after the image is overwritten", func() {
+		namespace := oc.Namespace()
+
+		g.By("creating a Deployment")
+		deployment, err := readDeploymentFixture(deploymentFixture)
+		o.Expect(err).NotTo(o.HaveOccurred())
+		o.Expect(deployment.Spec.Template.Spec.Containers).To(o.HaveLen(1))
+		o.Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(o.Equal(" "))
+
+		deployment, err = oc.KubeClient().AppsV1().Deployments(namespace).Create(deployment)
+		o.Expect(err).NotTo(o.HaveOccurred())
+		o.Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(o.Equal(" "))
+
+		g.By("tagging the docker.io/library/centos:latest as test:v1 image to create ImageStream")
+		out, err := oc.Run("tag").Args("docker.io/library/centos:latest", "test:v1").Output()
+		framework.Logf("%s", out)
+		o.Expect(err).NotTo(o.HaveOccurred())
+
+		g.By("waiting for the initial image to be replaced from ImageStream")
+		deployment, err = waitForDeploymentModification(oc.KubeClient().AppsV1(), deployment.ObjectMeta, SyncTimeout, func(d *appsv1.Deployment) (bool, error) {
+			return d.Spec.Template.Spec.Containers[0].Image != deployment.Spec.Template.Spec.Containers[0].Image, nil
+		})
+		o.Expect(err).NotTo(o.HaveOccurred())
+
+		g.By("setting Deployment image repeatedly to ' ' to fight with annotation trigger")
+		for i := 0; i < 50; i++ {
+			deployment, err = oc.KubeClient().AppsV1().Deployments(namespace).Patch(deployment.Name, types.StrategicMergePatchType,
+				[]byte(`{"spec":{"template":{"spec":{"containers":[{"name":"test","image":" "}]}}}}`))
+			o.Expect(err).NotTo(o.HaveOccurred())
+			o.Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(o.Equal(" "))
+		}
+
+		g.By("waiting for the image to be injected by annotation trigger")
+		o.Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(o.Equal(" "))
+		deployment, err = waitForDeploymentModification(oc.KubeClient().AppsV1(), deployment.ObjectMeta, SyncTimeout, func(d *appsv1.Deployment) (bool, error) {
+			return d.Spec.Template.Spec.Containers[0].Image != deployment.Spec.Template.Spec.Containers[0].Image, nil
+		})
+		o.Expect(err).NotTo(o.HaveOccurred())
+	})
+})

test/extended/images/trigger/utils.go

@@ -0,0 +1,47 @@
+package trigger
+
+import (
+	"fmt"
+	"io/ioutil"
+	"time"
+
+	"github.com/ghodss/yaml"
+
+	appsv1 "k8s.io/api/apps/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/watch"
+	appsv1clientset "k8s.io/client-go/kubernetes/typed/apps/v1"
+)
+
+func readDeploymentFixture(path string) (*appsv1.Deployment, error) {
+	data, err := ioutil.ReadFile(path)
+	if err != nil {
+		return nil, err
+	}
+
+	deployment := new(appsv1.Deployment)
+	err = yaml.Unmarshal(data, deployment)
+	if err != nil {
+		return nil, err
+	}
+
+	return deployment, err
+}
+
+func waitForDeploymentModification(appsClient appsv1clientset.AppsV1Interface, objMeta metav1.ObjectMeta, timeout time.Duration, condition func(deployment *appsv1.Deployment) (bool, error)) (*appsv1.Deployment, error) {
+	watcher, err := appsClient.Deployments(objMeta.Namespace).Watch(metav1.SingleObject(objMeta))
+	if err != nil {
+		return nil, err
+	}
+
+	event, err := watch.Until(timeout, watcher, func(event watch.Event) (bool, error) {
+		if event.Type != watch.Modified && (objMeta.ResourceVersion == "" && event.Type != watch.Added) {
+			return true, fmt.Errorf("different kind of event appeared while waiting for Deployment modification: event: %#v", event)
+		}
+		return condition(event.Object.(*appsv1.Deployment))
+	})
+	if err != nil {
+		return nil, err
+	}
+	return event.Object.(*appsv1.Deployment), nil
+}

test/extended/testdata/bindata.go

@@ -0,0 +1,26 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    image.openshift.io/triggers: '[{"from":{"kind":"ImageStreamTag","name":"test:v1"},"fieldPath":"spec.template.spec.containers[?(@.name==\"test\")].image"}]'
+  name: test
+spec:
+  progressDeadlineSeconds: 600
+  replicas: 1
+  revisionHistoryLimit: 10
+  selector:
+    matchLabels:
+      app: test
+  strategy:
+    type: Recreate
+  template:
+    metadata:
+      labels:
+        app: test
+    spec:
+      containers:
+        - image: " "
+          name: test
+          command: ["/bin/sleep"]
+          args:
+           - infinity