BUILD-187: Detect if image trigger ID was cleared

Detect if the lastTriggeredImageID was cleared in a BuildConfig.
If an update cleared the last triggered image, record a warning event.

A metric and associated alert were considered to better inform users
that deprecated behavior is being triggered. However, this was rejected
because the metric would require unbound time series cardinality.

pkg/build/controller/buildconfig/buildconfig_controller.go

@@ -25,6 +25,7 @@ import (
 	buildclientv1 "github.com/openshift/client-go/build/clientset/versioned/typed/build/v1"
 	buildinformer "github.com/openshift/client-go/build/informers/externalversions/build/v1"
 	buildlister "github.com/openshift/client-go/build/listers/build/v1"
+	lgbuildutil "github.com/openshift/library-go/pkg/build/buildutil"
 	"github.com/openshift/openshift-controller-manager/pkg/build/buildscheme"
 	"github.com/openshift/openshift-controller-manager/pkg/build/buildutil"
 	buildcommon "github.com/openshift/openshift-controller-manager/pkg/build/controller/common"
@@ -174,9 +175,52 @@ func (c *BuildConfigController) buildConfigAdded(obj interface{}) {
 // buildconfig is updated or there is a relist of buildconfigs
 func (c *BuildConfigController) buildConfigUpdated(old, cur interface{}) {
 	bc := cur.(*buildv1.BuildConfig)
+	oldBuildConfig := old.(*buildv1.BuildConfig)
+	// TODO: Remove the event after the image trigger behavior is removed.
+	if c.imageChangeTriggerCleared(oldBuildConfig, bc) {
+		c.recorder.Event(bc,
+			corev1.EventTypeWarning,
+			"ImageChangeTriggerCleared",
+			"spec.triggers[*].imagechange.lastTriggeredImageID was cleared, which will trigger a build. This behavior is deprecated and will be removed in a future OpenShift release.")
+	}
 	c.enqueueBuildConfig(bc)
 }
 
+func (c *BuildConfigController) imageChangeTriggerCleared(old, cur *buildv1.BuildConfig) bool {
+	if old == nil || cur == nil {
+		return false
+	}
+	imageLastTriggers := map[corev1.ObjectReference]string{}
+	for _, oldTrigger := range old.Spec.Triggers {
+		if oldTrigger.ImageChange == nil {
+			continue
+		}
+		from := c.getImageChangeTriggerInputReference(old, oldTrigger)
+		imageLastTriggers[*from] = oldTrigger.ImageChange.LastTriggeredImageID
+	}
+	for _, currentTrigger := range cur.Spec.Triggers {
+		if currentTrigger.ImageChange == nil {
+			continue
+		}
+		from := c.getImageChangeTriggerInputReference(cur, currentTrigger)
+		prev, found := imageLastTriggers[*from]
+		if found && len(prev) > 0 && len(currentTrigger.ImageChange.LastTriggeredImageID) == 0 {
+			return true
+		}
+	}
+	return false
+}
+
+func (c *BuildConfigController) getImageChangeTriggerInputReference(bc *buildv1.BuildConfig, trigger buildv1.BuildTriggerPolicy) *corev1.ObjectReference {
+	if trigger.ImageChange == nil {
+		return nil
+	}
+	if trigger.ImageChange.From != nil {
+		return trigger.ImageChange.From
+	}
+	return lgbuildutil.GetInputReference(bc.Spec.Strategy)
+}
+
 // enqueueBuild adds the given build to the queue.
 func (c *BuildConfigController) enqueueBuildConfig(bc *buildv1.BuildConfig) {
 	key, err := kcontroller.KeyFunc(bc)

pkg/build/controller/buildconfig/buildconfig_controller_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	ktesting "k8s.io/client-go/testing"
@@ -105,6 +106,143 @@ func TestHandleBuildConfig(t *testing.T) {
 
 }
 
+func TestCheckImageChangeTriggerCleared(t *testing.T) {
+	cases := []struct {
+		name            string
+		oldTriggers     []tagTriggerID
+		currentTriggers []tagTriggerID
+		setOldNil       bool
+		setCurrentNil   bool
+		expectedResult  bool
+	}{
+		{
+			name:      "old nil",
+			setOldNil: true,
+		},
+		{
+			name:          "current nil",
+			setCurrentNil: true,
+		},
+		{
+			name:          "both nil",
+			setOldNil:     true,
+			setCurrentNil: true,
+		},
+		{
+			name: "no trigger changes",
+			oldTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "abcdef0",
+				},
+			},
+			currentTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "abcdef0",
+				},
+			},
+		},
+		{
+			name: "empty to populated",
+			oldTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "",
+				},
+			},
+			currentTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "abcdef0",
+				},
+			},
+		},
+		{
+			name: "populated to empty",
+			oldTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "abcdef0",
+				},
+			},
+			currentTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "",
+				},
+			},
+			expectedResult: true,
+		},
+		{
+			name: "multi empty to populated",
+			oldTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "abcdef0",
+				},
+				{
+					ImageStreamTag:  "test-build:latest",
+					LastTriggeredId: "",
+				},
+			},
+			currentTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "abcdef0",
+				},
+				{
+					ImageStreamTag:  "test-build:latest",
+					LastTriggeredId: "abcdef0",
+				},
+			},
+		},
+		{
+			name: "multi populated to empty",
+			oldTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "abcdef0",
+				},
+				{
+					ImageStreamTag:  "test-build:latest",
+					LastTriggeredId: "abcdef0",
+				},
+			},
+			currentTriggers: []tagTriggerID{
+				{
+					LastTriggeredId: "abcdef0",
+				},
+				{
+					ImageStreamTag:  "test-build:latest",
+					LastTriggeredId: "",
+				},
+			},
+			expectedResult: true,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			objects := []runtime.Object{}
+
+			var current *buildv1.BuildConfig
+			if !tc.setCurrentNil {
+				current = buildConfigWithImageChangeTriggers(tc.currentTriggers)
+			}
+			if current != nil {
+				objects = append(objects, current)
+			}
+			buildClient := fake.NewSimpleClientset(objects...)
+			controller := &BuildConfigController{
+				buildLister:       &okBuildLister{},
+				buildConfigGetter: buildClient.BuildV1(),
+				buildGetter:       buildClient.BuildV1(),
+				buildConfigLister: &okBuildConfigGetter{BuildConfig: current},
+				recorder:          &record.FakeRecorder{},
+			}
+			var old *buildv1.BuildConfig
+			if !tc.setOldNil {
+				old = buildConfigWithImageChangeTriggers(tc.oldTriggers)
+			}
+			changed := controller.imageChangeTriggerCleared(old, current)
+			if changed != tc.expectedResult {
+				t.Errorf("expected ImageChangeTriggerCleared to be %v, got %v", tc.expectedResult, changed)
+			}
+		})
+	}
+}
+
 func baseBuildConfig() *buildv1.BuildConfig {
 	bc := &buildv1.BuildConfig{}
 	bc.Name = "testBuildConfig"
@@ -128,6 +266,31 @@ func buildConfigWithNonZeroLastVersion() *buildv1.BuildConfig {
 	return bc
 }
 
+func buildConfigWithImageChangeTriggers(triggers []tagTriggerID) *buildv1.BuildConfig {
+	bc := baseBuildConfig()
+	for _, trigger := range triggers {
+		imageChangeTrigger := &buildv1.ImageChangeTrigger{
+			LastTriggeredImageID: trigger.LastTriggeredId,
+		}
+		if len(trigger.ImageStreamTag) > 0 {
+			imageChangeTrigger.From = &corev1.ObjectReference{
+				Kind: "ImageStreamTag",
+				Name: trigger.ImageStreamTag,
+			}
+		}
+		bc.Spec.Triggers = append(bc.Spec.Triggers, buildv1.BuildTriggerPolicy{
+			Type:        buildv1.ImageChangeBuildTriggerType,
+			ImageChange: imageChangeTrigger,
+		})
+	}
+	return bc
+}
+
+type tagTriggerID struct {
+	ImageStreamTag  string
+	LastTriggeredId string
+}
+
 type okBuildLister struct{}
 
 func (okc *okBuildLister) List(label labels.Selector) ([]*buildv1.Build, error) {

pkg/build/metrics/prometheus/metrics.go

@@ -37,6 +37,7 @@ var (
 		[]string{"namespace", "name", "phase", "reason", "strategy"},
 		nil,
 	)
+
 	bc             = buildCollector{}
 	cancelledPhase = string(buildv1.BuildPhaseCancelled)
 	completePhase  = string(buildv1.BuildPhaseComplete)

pkg/build/metrics/prometheus/metrics_test.go

@@ -40,7 +40,7 @@ func (f *fakeResponseWriter) WriteHeader(statusCode int) {
 	f.statusCode = statusCode
 }
 
-func TestMetrics(t *testing.T) {
+func TestBuildMetrics(t *testing.T) {
 	// went per line vs. a block of expected test in case assumptions on ordering are subverted, as well as defer on
 	// getting newlines right
 	expectedResponse := []string{