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.

The behavior that triggers builds off of `spec` will be removed in BUILD-188 [1].

[1] https://issues.redhat.com/browse/BUILD-188

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,53 @@ 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.
+	// See https://issues.redhat.com/browse/BUILD-188
+	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{